Page MenuHomePhabricator

[avalanche] Reject votes that fromnodes which already are in the quorum.
ClosedPublic

Authored by deadalnix on Thu, Feb 21, 16:40.

Details

Summary

This ensure we do not poll the same node again and again. By ensuring the quorum is diverse and that we do not have too many requests in flight, we ensure that we do not run ahead o the network.

Depends on D2583 and D2585

Test Plan

Added tests cases

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Thu, Feb 21, 16:40
Herald added a reviewer: Restricted Project. · View Herald TranscriptThu, Feb 21, 16:40
Herald added a subscriber: schancel. · View Herald Transcript
jasonbcox requested changes to this revision.Fri, Feb 22, 00:46
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/avalanche.cpp
47 ↗(On Diff #7426)

tice -> twice

102 ↗(On Diff #7426)

Please move/improve src/test/lcg.h to reduce code duplication

117 ↗(On Diff #7426)

A vote has not occurred if the function name shouldVote is to be believed. Looks like these two lines belong in registerVote.

src/avalanche.h
76 ↗(On Diff #7426)

Hardcoding the size made sense in the tests before this diff, but here it's clear it should be a constant defined above.

src/test/avalanche_tests.cpp
705 ↗(On Diff #7426)

a node -> nodes since this creates multiple nodes by default

718 ↗(On Diff #7426)

There seems no reason to leave node 0 out of the loop

727 ↗(On Diff #7426)

should be moved immediately after avanodes is initialized

731 ↗(On Diff #7426)

Check if round is not equal to the previous round

745 ↗(On Diff #7426)

Relying on the internals of functions called in the previous loop that creates a dependency on node 0 doesn't seem intuitive. Wouldn't it make more sense to call AvalancheTest::getSuitableNodeToQuery(p) before this loop and use that node's ID here?

This revision now requires changes to proceed.Fri, Feb 22, 00:46
deadalnix added inline comments.Fri, Feb 22, 16:23
src/avalanche.cpp
102 ↗(On Diff #7426)

That wouldn't work and I don't think it'd be beneficial that it is made to work.

117 ↗(On Diff #7426)

You need to increase the count with setting the filter. There are not separable operations.

src/test/avalanche_tests.cpp
731 ↗(On Diff #7426)

What purpose does that serve ?

745 ↗(On Diff #7426)

One could do this, the inconvenient would be that the test would not test what it is supposed to test anymore.

deadalnix updated this revision to Diff 7440.Fri, Feb 22, 16:52

Fix various comments

jasonbcox added inline comments.Fri, Feb 22, 19:00
src/avalanche.cpp
117 ↗(On Diff #7426)

Yes, that's why I said:

Looks like these **two lines** belong in `registerVote`.
src/test/avalanche_tests.cpp
731 ↗(On Diff #7426)

If we do not check it, and some behavior change or bug causes this test to not start a new round where we expect, the test will fail in the loop with p.registerVotes(...) below. It may not be immediately obvious that this is happening because the round has not progressed, rather than some other bug.

jasonbcox requested changes to this revision.Wed, Feb 27, 17:52
jasonbcox added inline comments.
src/avalanche.cpp
117 ↗(On Diff #7426)

Talked offline:

  • Make shouldVote() private
  • Rename shouldVote() to make it clear that this function call has side effects
src/test/avalanche_tests.cpp
745 ↗(On Diff #7426)

Talked offline. Amaury will fix this.

This revision now requires changes to proceed.Wed, Feb 27, 17:52
deadalnix updated this revision to Diff 7529.Thu, Feb 28, 16:58

shouldVote => addNodeToQuorum

And improve the test to node rely on a specific node id.

jasonbcox accepted this revision.Thu, Feb 28, 22:29
This revision is now accepted and ready to land.Thu, Feb 28, 22:29
This revision was automatically updated to reflect the committed changes.