Details
- Reviewers
jasonbcox - Group Reviewers
Restricted Project - Commits
- rSTAGINGfb8b2109af5d: [avalanche] Reject votes that fromnodes which already are in the quorum.
rABCfb8b2109af5d: [avalanche] Reject votes that fromnodes which already are in the quorum.
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
| 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? |
| 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:
|
| 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. |
shouldVote => addNodeToQuorum
And improve the test to node rely on a specific node id.