Page MenuHomePhabricator

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

Authored by deadalnix on Feb 21 2019, 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
Branch
avanodetrack
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5046
Build 8155: Bitcoin ABC Buildbot (legacy)
Build 8154: arc lint + arc unit

Event Timeline

jasonbcox requested changes to this revision.Feb 22 2019, 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.Feb 22 2019, 00:46
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.

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.Feb 27 2019, 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.Feb 27 2019, 17:52

shouldVote => addNodeToQuorum

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

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