Kademlia EachBin bug fix and unit tests#18338
Kademlia EachBin bug fix and unit tests#18338holisticode wants to merge 8 commits intoethereum:masterfrom
Conversation
holisticode
changed the title
zelig
left a comment
zelig
left a comment
There was a problem hiding this comment.
pending on merge of new depth calculation otherwise chunks in bins with no peers (empty bins shallower than depth) will NOT be synced
| if startPo > 0 && endPo != k.MaxProxDisplay { | ||
| startPo = endPo + 1 | ||
| } | ||
| //if the peer's bin is smaller than the kademlia depth, |
There was a problem hiding this comment.
but now this PR really needs to wait for the new depth calculation
There was a problem hiding this comment.
Please do not use "smaller". Please review the terminology introduced here:
If you don't agree with the terms, let's discuss and settle on something everyone can agree with.
nolash
left a comment
nolash
left a comment
There was a problem hiding this comment.
Shouldn't this be PR'd to ethersphere instead?
| //EachBin iterates over each connection of a kademlia table, per bin. | ||
| //So `po` will be representing the bin value (0..255), while `val` represents an | ||
| //existing connection inside that bin. | ||
| //For any such connection found, the parameter func `eachBinFunc` will be executed |
There was a problem hiding this comment.
I've also made a comment for this. Which one do we keep?
| if startPo > 0 && endPo != k.MaxProxDisplay { | ||
| startPo = endPo + 1 | ||
| } | ||
| //if the peer's bin is smaller than the kademlia depth, |
There was a problem hiding this comment.
Please do not use "smaller". Please review the terminology introduced here:
If you don't agree with the terms, let's discuss and settle on something everyone can agree with.
| 015 0 | 0 | ||
| ========================================================================= | ||
| */ | ||
| func TestEachBin(t *testing.T) { |
There was a problem hiding this comment.
A unit test should not be opinionated about the context which it's used by consumers. I find the terms "subscription" etc wholly out of place here. Keep it minimal to the exact expectations of what this function should provide and nothing more.
There was a problem hiding this comment.
But since that is the point of the test not the iterator (despite the name of the PR), the test should be in the stream package rather than here.
|
has been obsoleted by #18355 |
3 participants
This PR adds a unit test for the
kademlia.EachBinfunction.This function is responsible for the correct registration of syncing subscriptions for the
streamprotocol.Apart from fixing a couple of bugs, most importantly this PR introduces a unit test.
ethersphere/swarm#1071 is an issue to refactor the
EachBinfunction; with a working unit test that refactor gets a test-driven approach.Replaces #18284 (which was for discussion)