Skip to content

p2p/discv5: fix reg lookup, polish code, use logger#15737

Merged
fjl merged 1 commit into
ethereum:masterfrom
karalabe:p2p-discv5-cleanups
Dec 28, 2017
Merged

p2p/discv5: fix reg lookup, polish code, use logger#15737
fjl merged 1 commit into
ethereum:masterfrom
karalabe:p2p-discv5-cleanups

Conversation

@karalabe
Copy link
Copy Markdown
Member

@karalabe karalabe commented Dec 22, 2017

This PR does a few polishes and cleanups in the p2p discoveryv5 ticketing code.

Functionality wise it fixes a (probably harmless) bug when iterating the known topics for registration. The previous code in nextRegisterLookup created a list of topics to iterate over, and then exhausted that list (recreating if it became empty), until it found the same topic it started iterating from.

This logic is a bit convoluted, because topic queuing and iteration was spanned out over multiple methods, which both messed around some internal cache. One bug was that doing a "full round" of iteration meant that the first topic will be consumed twice. This is a harmless issue in this case.

The second potentially bigger bug was that iteration order was random due to being backed by a map, so the queue constructor could make the following queues in subsequent iterations: [a, b, c, d], [d, a, b, c]. If my nextRegisterLookup invocation starts at topic d from the first listing, which is empty, the next call will again be d, causing the method to return a 40 second sleep, even though there are plenty more topics to try.

The fix in the PR is to take the leftover queue from the previous invocation, and expand it randomly with all the topics not contained within it. This makes everything simpler: a full iteration is guaranteed to go over all the topics, without duplicates, and to correctly stop when every one of them was tried.


Furthermore the PR swaps tickets map[Topic]topicTickets to tickets map[Topic]*topicTickets. This is needed because the code was full of s.topics[topic].buckets == nil checks, which is essence only intended to check if s.topics[topic] existed, but in the non-pointer version they always return the zero value of the struct, making the code harder to understand.

Lastly, the PR removes all the debugLog invocations in favor of log.Trace. This allows us to use the fine grained controls out logging framework has to debug discovery issues. I've also added some minor simplifications in the logs. Now we have a much more readable stream of events to trace with:

SS

@karalabe karalabe requested review from fjl and zsfelfoldi December 22, 2017 10:46
@karalabe karalabe added this to the 1.8.0 milestone Dec 22, 2017
Copy link
Copy Markdown
Contributor

@zsfelfoldi zsfelfoldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fjl fjl merged commit c15d76a into ethereum:master Dec 28, 2017
mariameda pushed a commit to NiluPlatform/go-nilu that referenced this pull request Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants