Page MenuHomePhabricator
Differential D10788

[avalanche] Update our position according to the vote
ClosedPublic
Actions

Authored by Fabien on Jan 7 2022, 16:19.

Details

Reviewers
tyler-smith
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Commits
rABC5711d09db8c3: [avalanche] Update our position according to the vote
Summary

Accept or reject a proof being voted on according to the avalanche consensus.
There is still a missing cooldown feature (prevent proof replacement after a vote has been accepted and finalized, see D10714 summary) which will be added in a follow up. This is fine because the feature is disabled by default by the replacement option flag.

Ref T1854.

Depends on D10727.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
avalanche_answer_proof_responses
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 17964
Build 35750: Build Difflint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 35749: arc lint + arc unit

Event Timeline

Fabien created this revision.Jan 7 2022, 16:19
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 7 2022, 16:19
Fabien requested review of this revision.Jan 7 2022, 16:19
Fabien added a child revision: D10789: [avalanche] Add a peer replacement cooldown.Jan 7 2022, 16:23
Harbormaster completed remote builds in B17839: Diff 31700.Jan 7 2022, 16:38
Fabien updated this revision to Diff 31751.Jan 11 2022, 09:29

Rebase

Harbormaster completed remote builds in B17880: Diff 31751.Jan 11 2022, 09:43
tyler-smith added a subscriber: tyler-smith.Jan 21 2022, 07:05
tyler-smith added inline comments.
src/net_processing.cpp
4454 ↗(On Diff #31751)

Very small nit, but is there a reason to do this check instead of potentially trying to iterate over an empty proofUpdates?

Fabien added inline comments.Jan 21 2022, 08:08
src/net_processing.cpp
4454 ↗(On Diff #31751)

Indeed, since there is no action outside of the loop this check is not needed, good catch.

Fabien updated this revision to Diff 31886.Jan 21 2022, 08:13

Address feedback

Fabien marked an inline comment as done.Jan 21 2022, 08:14
Harbormaster completed remote builds in B17963: Diff 31886.Jan 21 2022, 08:28
Fabien updated this revision to Diff 31887.Jan 21 2022, 08:37

Add an invalidated proof to the recent reject filter

Harbormaster completed remote builds in B17964: Diff 31887.Jan 21 2022, 08:53
teamcity added a comment.Jan 21 2022, 08:53
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Failed tests logs:

====== Bitcoin ABC functional tests: abc_p2p_avalanche_proof_voting.py ======

------- Stdout: -------
2022-01-21T08:47:53.725000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20220121_084357/abc_p2p_avalanche_proof_voting_186
2022-01-21T08:47:56.018000Z TestFramework (INFO): Trigger polling from the node...
2022-01-21T08:47:56.130000Z TestFramework (INFO): Check we poll for valid proof
2022-01-21T08:47:56.251000Z TestFramework (INFO): Check we don't poll for subsequent proofs if the cooldown is not elapsed, proof not the favorite
2022-01-21T08:47:56.302000Z TestFramework (INFO): Check we don't poll for subsequent proofs if the cooldown is not elapsed, proof is the favorite
2022-01-21T08:47:56.354000Z TestFramework (INFO): Check we poll for conflicting proof if the proof is not the favorite
2022-01-21T08:47:56.517000Z TestFramework (INFO): Check we poll for conflicting proof if the proof is the favorite
2022-01-21T08:47:56.654000Z TestFramework (INFO): Check we don't poll for orphans
2022-01-21T08:47:56.705000Z TestFramework (INFO): Check we don't poll for proofs that get evicted
2022-01-21T08:47:56.757000Z TestFramework (INFO): Check we don't poll for invalid proofs and get banned
2022-01-21T08:47:56.820000Z TestFramework (INFO): Test proof acceptance
2022-01-21T08:47:58.369000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/work/test/functional/abc_p2p_avalanche_proof_voting.py", line 198, in run_test
    assert proofid_seq40 not in get_proof_ids(node)
AssertionError
2022-01-21T08:47:58.420000Z TestFramework (INFO): Stopping nodes
2022-01-21T08:47:58.622000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20220121_084357/abc_p2p_avalanche_proof_voting_186
2022-01-21T08:47:58.622000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20220121_084357/abc_p2p_avalanche_proof_voting_186/test_framework.log
2022-01-21T08:47:58.622000Z TestFramework (ERROR): 
2022-01-21T08:47:58.622000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20220121_084357/abc_p2p_avalanche_proof_voting_186' to consolidate all logs
2022-01-21T08:47:58.622000Z TestFramework (ERROR): 
2022-01-21T08:47:58.622000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2022-01-21T08:47:58.622000Z TestFramework (ERROR): https://github.com/Bitcoin-ABC/bitcoin-abc/issues
2022-01-21T08:47:58.622000Z TestFramework (ERROR):

Each failure log is accessible here:
Bitcoin ABC functional tests: abc_p2p_avalanche_proof_voting.py

Fabien planned changes to this revision.Jan 21 2022, 08:58

need to investigate the test failure

Fabien mentioned this in D10711: [avalanche] Add a method to reject a proof.Jan 21 2022, 11:33
Fabien added a comment.Jan 25 2022, 11:08

I ran a ton of time (e.g. over 1000 times on debian) on different platforms and was unable to reproduce the test failure. The failure is not in a code that is touched by this diff, never happened on CI, and can only happen under one of these 2 conditions:

  • There is a race in the RPC, which I checked several times and couldn't spot
  • The boost multi index failed to erase an iterator from the peer set, which is highly dubious

I restarted the tests on this diff, and submit for review again.

Fabien requested review of this revision.Jan 25 2022, 11:08
tyler-smith added inline comments.Jan 30 2022, 23:19
test/functional/test_framework/avatools.py
143

Should these changes be in their own diff? Also, I don't actually see these changes used in this diff.

Fabien updated this revision to Diff 32071.Jan 31 2022, 08:06

Remove unreleated avatools changes

test/functional/test_framework/avatools.py
143

Indeed, it's unrelated, I removed it

Harbormaster completed remote builds in B18070: Diff 32071.Jan 31 2022, 08:25
teamcity added a comment.Jan 31 2022, 08:25
Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Failed tests logs:

====== Bitcoin ABC functional tests: abc_p2p_avalanche_proof_voting.py ======

------- Stdout: -------
2022-01-31T08:24:53.567000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20220131_082306/abc_p2p_avalanche_proof_voting_186
2022-01-31T08:24:55.548000Z TestFramework (INFO): Trigger polling from the node...
2022-01-31T08:24:55.660000Z TestFramework (INFO): Check we poll for valid proof
2022-01-31T08:24:55.791000Z TestFramework (INFO): Check we don't poll for subsequent proofs if the cooldown is not elapsed, proof not the favorite
2022-01-31T08:24:55.843000Z TestFramework (INFO): Check we don't poll for subsequent proofs if the cooldown is not elapsed, proof is the favorite
2022-01-31T08:24:55.894000Z TestFramework (INFO): Check we poll for conflicting proof if the proof is not the favorite
2022-01-31T08:24:56.060000Z TestFramework (INFO): Check we poll for conflicting proof if the proof is the favorite
2022-01-31T08:24:56.177000Z TestFramework (INFO): Check we don't poll for orphans
2022-01-31T08:24:56.229000Z TestFramework (INFO): Check we don't poll for proofs that get evicted
2022-01-31T08:24:56.283000Z TestFramework (INFO): Check we don't poll for invalid proofs and get banned
2022-01-31T08:24:56.346000Z TestFramework (INFO): Test proof acceptance
2022-01-31T08:24:57.970000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/work/test/functional/abc_p2p_avalanche_proof_voting.py", line 198, in run_test
    assert proofid_seq40 not in get_proof_ids(node)
AssertionError
2022-01-31T08:24:58.020000Z TestFramework (INFO): Stopping nodes
2022-01-31T08:24:58.222000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20220131_082306/abc_p2p_avalanche_proof_voting_186
2022-01-31T08:24:58.223000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20220131_082306/abc_p2p_avalanche_proof_voting_186/test_framework.log
2022-01-31T08:24:58.223000Z TestFramework (ERROR): 
2022-01-31T08:24:58.223000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20220131_082306/abc_p2p_avalanche_proof_voting_186' to consolidate all logs
2022-01-31T08:24:58.223000Z TestFramework (ERROR): 
2022-01-31T08:24:58.223000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2022-01-31T08:24:58.223000Z TestFramework (ERROR): https://github.com/Bitcoin-ABC/bitcoin-abc/issues
2022-01-31T08:24:58.223000Z TestFramework (ERROR):

Each failure log is accessible here:
Bitcoin ABC functional tests: abc_p2p_avalanche_proof_voting.py

Fabien planned changes to this revision.Jan 31 2022, 08:40
Fabien updated this revision to Diff 32110.Feb 1 2022, 14:26

The rare test failure (I reproduced once after about 5000 runs) comes from the fact that we're voting yes on both conflicting proofs (seq 30 and 40).
So even if the final state is correct, if the quroum was slow to answer the polls at the beginning of the test, there might be a short switch of the node position occuring between the 2 consecutive calls to getpeerinfo and causing the test to fail.
The solution is to restart the node to reset the in flight requests and make sure we're testing what we want.

Harbormaster completed remote builds in B18098: Diff 32110.Feb 1 2022, 14:42
tyler-smith accepted this revision.Feb 2 2022, 15:18

The changes look correct and tests look sufficient. Discussed the intermittent test failure with Fabien and am comfortable with the cause.

This revision is now accepted and ready to land.Feb 2 2022, 15:18
Closed by commit rABC5711d09db8c3: [avalanche] Update our position according to the vote (authored by Fabien). · Explain WhyFeb 9 2022, 09:14
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC5711d09db8c3: [avalanche] Update our position according to the vote.

Revision Contents
Changeset List

PathSize
src/
46 lines
test/
functional/
78 lines
test_framework/
10 lines

Diff 31887

View Options

src/net_processing.cpp

Loading...
View Options

test/functional/abc_p2p_avalanche_proof_voting.py

Loading...
View Options

test/functional/test_framework/avatools.py

Loading...