Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABC8f770ea4e862: [avalanche] Use the number nodes from which we've received avaproofs as a…

Summary

This maintains a count of the successful compact proofs exchanges and enforce a minimum quantity of node that sent avaproofs message before the quorum is considered valid.
This is a best effort to give confidence that our proof set is in par with the other nodes from the network before we start polling.
The threshold is put to 8: because we need at least 8 nodes in our quorum to have converging votes there is no point using a lower value. This means that 1 out of 8 outbounds should be honest for us to get a good set of proofs.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.Jun 9 2022, 17:00

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 9 2022, 17:00

Harbormaster completed remote builds in B19331: Diff 33937.Jun 9 2022, 17:17

Don't count during IBD, lower the threshold

Harbormaster completed remote builds in B19366: Diff 33976.Jun 10 2022, 22:38

Make naming more consistent and improve comments

Fabien published this revision for review.Jun 11 2022, 19:53

Harbormaster completed remote builds in B19371: Diff 33983.Jun 11 2022, 20:08

sdulfari requested changes to this revision.Jun 12 2022, 05:34

sdulfari added a subscriber: sdulfari.

sdulfari added inline comments.

src/avalanche/avalanche.h
77 ↗	(On Diff #33983)	Maybe COUNT instead of NUMBER? It's a bit more self-explanatory this way.
src/avalanche/peermanager.h
227 ↗	(On Diff #33983)	To help differentiate this API from the sister one in Processor, maybe name this `latchAvaproofsSent()`? This makes the return value more intuitive.
src/avalanche/processor.cpp
292 ↗	(On Diff #33983)	non-negative
843 ↗	(On Diff #33983)	Make a comment why proofs received during IBD do not count. Update the diff summary with more context if necessary.
test/functional/abc_p2p_avalanche_quorum.py
31 ↗	(On Diff #33983)	Nit: This can be 8 and can stay that way even if we bump the default higher. This should make the test slightly faster.
127 ↗	(On Diff #33983)	This and the above comment no longer make sense with the reference to "half" since there are more than two peers in the quorum now.

This revision now requires changes to proceed.Jun 12 2022, 05:34

Fabien edited the summary of this revision. (Show Details)Jun 12 2022, 07:36

Fabien added inline comments.Jun 12 2022, 07:47

test/functional/abc_p2p_avalanche_quorum.py
127 ↗	(On Diff #33983)	This is still valid, each peers is worth half the minimum score for the quorum to be considered valid (`-avaminquorumstake`)

Fabien edited the summary of this revision. (Show Details)Jun 12 2022, 07:56

Feedback and more renaming to make it clear we are counting nodes and not message

Harbormaster completed remote builds in B19372: Diff 33984.Jun 12 2022, 08:25

sdulfari accepted this revision.Jun 12 2022, 17:17

This revision is now accepted and ready to land.Jun 12 2022, 17:17

Consider renaming the actual Diff to reflect that it's a count of nodes.

(also, singular of criteria should be "criterion")

Something such as: "Use the number of nodes from which we've received avaproofs as a criterion for quorum readiness"

Fabien retitled this revision from [avalanche] Use the number of received avaproofs as a criteria for quorum readiness to [avalanche] Use the number nodes that sent avaproofs as a criterion for quorum readiness.Jun 13 2022, 06:21

Fabien retitled this revision from [avalanche] Use the number nodes that sent avaproofs as a criterion for quorum readiness to [avalanche] Use the number nodes from which we received avaproofs as a criterion for quorum readiness.

Fabien retitled this revision from [avalanche] Use the number nodes from which we received avaproofs as a criterion for quorum readiness to [avalanche] Use the number nodes from which we've received avaproofs as a criterion for quorum readiness.

Closed by commit rABC8f770ea4e862: [avalanche] Use the number nodes from which we've received avaproofs as a… (authored by Fabien). · Explain WhyJun 13 2022, 06:41

This revision was automatically updated to reflect the committed changes.

Fabien added a commit: rABC8f770ea4e862: [avalanche] Use the number nodes from which we've received avaproofs as a….

[avalanche] Use the number nodes from which we've received avaproofs as a criterion for quorum readiness
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 33985

src/avalanche/avalanche.h

src/avalanche/node.h

src/avalanche/peermanager.h

src/avalanche/peermanager.cpp

src/avalanche/processor.h

src/avalanche/processor.cpp

src/avalanche/test/init_tests.cpp

src/avalanche/test/peermanager_tests.cpp

src/avalanche/test/processor_tests.cpp

src/init.cpp

src/net_processing.cpp

test/functional/abc_p2p_avalanche_quorum.py

[avalanche] Use the number nodes from which we've received avaproofs as a criterion for quorum readinessClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 33985

src/avalanche/avalanche.h

src/avalanche/node.h

src/avalanche/peermanager.h

src/avalanche/peermanager.cpp

src/avalanche/processor.h

src/avalanche/processor.cpp

src/avalanche/test/init_tests.cpp

src/avalanche/test/peermanager_tests.cpp

src/avalanche/test/processor_tests.cpp

src/init.cpp

src/net_processing.cpp

test/functional/abc_p2p_avalanche_quorum.py

[avalanche] Use the number nodes from which we've received avaproofs as a criterion for quorum readiness
ClosedPublic
Actions

Revision Contents
Changeset List