Page MenuHomePhabricator

[avalanche] Add conflicting proofs to the vote
ClosedPublic

Authored by Fabien on Sep 2 2021, 13:45.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Restricted Maniphest Task
Commits
rABCbabaeea2cf14: [avalanche] Add conflicting proofs to the vote
Summary

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.

Test Plan

ninja check-avalanche-processor_tests

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 Difflint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang · build-clang-tidy
Build 33492: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.Sep 2 2021, 13:45
deadalnix requested changes to this revision.Sep 2 2021, 14:44
deadalnix added a subscriber: deadalnix.

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.

This revision now requires changes to proceed.Sep 2 2021, 14:44
Mengerian added a task: Restricted Maniphest Task.Sep 14 2021, 16:49
Mengerian removed a task: Restricted Maniphest Task.
Fabien added a task: Restricted Maniphest Task.
  • 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
Fabien edited the summary of this revision. (Show Details)
Fabien edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Sep 27 2021, 21:55

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.

This revision now requires changes to proceed.Sep 27 2021, 21:55
Fabien marked an inline comment as done.Sep 30 2021, 08:29

Rebase on top of D10219 and D10222 to keep it small enough.
Rename the conflicting_proofs map to proofVoteRecords. The vote_records will be renamed to blockVoteRecords in another diff.

deadalnix requested changes to this revision.Sep 30 2021, 15:45

Remove the whole proof handler thing. Test from addProofToReconciledirectly.

This revision now requires changes to proceed.Sep 30 2021, 15:45

Remove the handlers for now and use addProofToReconcile to unit tests the invs are added as expected.

Fabien edited the test plan for this revision. (Show Details)
Fabien edited the summary of this revision. (Show Details)

I don't think this depends on D10222 anymore.

This revision is now accepted and ready to land.Oct 1 2021, 14:57

I already updated the summary but forgot to remove the dependency in phab, good catch