This puts the conflicting proofs to votes on the avalanche network. It's currently limited to polling for proofs only, and never called outside of the tests.
Ref T1854.
Depends on D10219.
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project - Maniphest Tasks
- Restricted Maniphest Task
Restricted Maniphest Task - Commits
- rABCbabaeea2cf14: [avalanche] Add conflicting proofs to the vote
ninja check-avalanche-processor_tests
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- avalanche_add_conflicting_proofs_to_vote
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 16825 Build 33493: Build Diff lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang · build-clang-tidy Build 33492: arc lint + arc unit
Event Timeline
Comment Actions
There are obvious problems with this. For instance, one can completely disable avalanche by flooding the system with proofs. While it is not expected that all problems are solved right out of the bat, this is somewhat surprising that none of the limitations have been thought through and do not inspire confidence that this is moving in the right direction at all. Documenting known problems in the code, in addition of having the benefice that problems are documented in the code, is the only way a reviewer can asses if the person writing the code is actually moving somewhere sensible with it.
| src/avalanche/peermanager.cpp | ||
|---|---|---|
| 260 ↗ | (On Diff #29750) | This is computed but not used anywhere. |
| 267 ↗ | (On Diff #29750) | This seems wasteful to compute all of this when none of it matters when accepted is false. |
| 271 ↗ | (On Diff #29750) | El famoso if (condition) return true; retrun false; What about that instead? return condition; |
| src/avalanche/peermanager.h | ||
| 189 ↗ | (On Diff #29750) | This is a change in the general locking model of the thread manager. Why? |
| src/avalanche/processor.cpp | ||
| 486–499 ↗ | (On Diff #29750) | This whole mess this to be a pretty string signal to me that this list belong in the Processor, just like blocks, not in the PeerManager. |
Comment Actions
- Move the vote records to the processor
- Update the local vote algo to make it compatible with stake aggregation
- Use a notification handler to signal conflicting proofs: this makes it possible to test the local vote status
- Various changes to address the review comments, especially adding comments to document existing flaws
Comment Actions
Ok I think I'm getting a sense of where this is going. This patch definitely suffers from doing too many things. The whole ConflictingProofHandlerbusiness could be extracted from this and put in another patch.
This would move much smoother if you limited each diff to do one thing and one thing only.
| src/avalanche/processor.h | ||
|---|---|---|
| 98 ↗ | (On Diff #30148) | This can go in its own header, with the implementation inlined. There is no good reason to make this call opaque. Note that this can even be its own diff, with a test for it. |
| 120 ↗ | (On Diff #30148) | This name is clearly not accurate. |
| 196 ↗ | (On Diff #30148) | This is enough of an API to check for the behavior of the processor. If you split the proof comparator, you end up with 3 diffs already. |
Comment Actions
Remove the handlers for now and use addProofToReconcile to unit tests the invs are added as expected.
Comment Actions
I already updated the summary but forgot to remove the dependency in phab, good catch