DHT: add bootstrapping, fix bugs, cleanup code
Review Request #130083 - Created April 11, 2017 and discarded
Comprehensive revision of DHT codebase:
- Implemented bootstrapping from well-known nodes (like most other torrent clients do) so that DHT is usable right away (i.e. when enabling it for the first time)
- Transferred lots of important hard-coded values to constants in header files. This is a first step, in a second step those values should be made configurable from within KTorrent
- Added comments for important constants, describing them and comparing their values to the specification and/or the DHT reference implementation
- Updated several important values (timeouts, minimum/maximum limits) according to the values proposed by the current DHT reference implementation
- Small bugfixes (e.g. Node::loadTable in src/dht/node.cpp, DHT::addDHTNode in src/dht/dht.cpp)
- Performed general code cleanup (comments, newlines, whitespace, structuring etc.)
For further details, refer to '@author Fonic' comments within code.
The following files contain changes other than cleanup:
Notes on DHT bootstrapping:
- To test DHT bootstrapping, (re)move any existing KTorrent user configuration, start KTorrent and enable DHT in settings. Then, watch the DHT entry in the status bar and/or the debug output in the log/console. Bootstrapping should only take a couple of seconds. After that, the number of known nodes should be in the range of 50 - 100. Here's a screenshot showing what should happen.
- I chose to implement a simple/basic bootstrapping mechanism: add well-known bootstrap nodes, wait a specific amount of time (timer), compare DHT routing table entries to specific minimum amount to check if bootstrapping was successful, retry if bootstrapping failed. The DHT reference implementation uses a more sophisticated approach, evaluating the depth of the resulting DHT routing table to check if bootstrapping was successful. Currently, I'm not sure if the sophisticated approach is really necessary - I'll investigate this further.
- Currently, the well-known DHT nodes used for bootstrapping are hard-coded (as it is with most other torrent clients). I think it might be best to create a new plugin for DHT bootstrapping that allows to specifiy a list of well-known DHT nodes and also allows to control when to bootstrap (i.e. only if routing table is empty, always on startup, ...)
Sorry that this is an all-in-one request with only one commit. This is due to the fact that I already did this in Dec. 2016 on GitHub before noticing that it's only used as an unmanaged mirror, so I had to port my changes from there, losing commit history.
Fixed list in description; Added trufanov in reference to /r/130053 (interested in DHT bootstrapping)
Added notes on DHT bootstrapping, added screenshot
To test DHT bootstrapping, (re)move any existing KTorrent user configuration
It should be enough to locate and delete
ktorrent/dht_table.ipv6before KTorrent start.
I'm afraid this request may be quite hard to review. Too many things changed at once. I've started contributing to KDE 2 months ago with skanlite project. But submitting reviews for KTorrent was more difficult. The
ktorrrentgroup of reviewers isn't active much. My requests were in pending state for a while till I was adviced to include Albert (aacid) as a reviewer. Unless someone from ktorrent group don't show up he seems to be only one here who has decision power to approve the review. And according to my understanding Albert is backing up ktorrent group and doing additional work apart from his own. Which means we better make his work easier and this take some time.
I would suggest to split this review into pieces: bugfixes (perhaps one review per bug), values updating, transfering them into header etc. And perhaps even submit these for review one by one to simplify decision making process. And I would recommend postpone code cleanup and comments as a least important thing. And perhaps even submit these for review one by one to simplify decign making process.
As for DHT bootstrapping - I'm the one who is both hands for this feature and would like to advocate for it. Frankly, I thought KT has it and it's a bug that it doesn't work. It's very frustrating when KT can't download file via magnet link for months, especially if magnet URL didn't contain
&tr=values that points to trackers and has only hash, while
qBittorentstart downloading it immeiately like a charm. (I can even suggest this as a test case for DHT bootstrapping: get magnet url with non 0 seeds, strip off everything except hash from it and try to import in KT and qBittorent.) So I think this is something that must be fixed in libktorrent.
The list of URLs used for bootstrapping should be fine as the same is used in qbittorent. But I believe that it shouldn't be a part of libktorrent. All it need is to export one more function like
resetDHTTables()and KT then could invokeresetDHTTables(); addDHTNode(QString("router.bittorrent.com"), 6881); addDHTNode(QString("router.utorrent.com"), 6881); etc..
on client side where end user have full control over a list of nodes used for bootstrapping. And that might cause binary compatibility break so I think this will not be simple.. and we'll have a lot to discuss and argue. It will certanly take a few weeks.
So my suggestion is to split this review, omit noncritical changes, keep it simple and move step by step. You can ping me a message via skype (trufanov-nok) if you want some nuances on my expirience of commiting changes to KDE projects.