Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Project

Commits

rABC6979db2ce876: [avalanche] Move block-vote decision into getAvalancheVoteForBlock.

Summary

Abstract block-vote logic into its own function to prepare for adding other types of vote
logic.

Depends on D10685.

Test Plan

python test/functional/abc_p2p_avalanche_voting.py

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tyler-smith created this revision.Dec 15 2021, 16:05

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 15 2021, 16:05

tyler-smith requested review of this revision.Dec 15 2021, 16:05

Harbormaster completed remote builds in B17650: Diff 31426.Dec 15 2021, 16:33

tyler-smith added a child revision: D10387: [avalanche] Add proof-poll responses.Dec 15 2021, 16:38

deadalnix added a subscriber: deadalnix.Dec 15 2021, 17:12

deadalnix added inline comments.

src/net_processing.cpp
4356 ↗	(On Diff #31426)	Th code would really benefit from having a default clause here, and maybe a log?

deadalnix requested changes to this revision.Dec 16 2021, 11:44

deadalnix added inline comments.

src/net_processing.cpp
5144 ↗	(On Diff #31426)	It doesn't looks like it needs to be a method of the peer manager, is it? A static method seems like it would be enough.
5145 ↗	(On Diff #31426)	This will lock and unlock main in a loop. I don't think we want this here. The time where cs_main is unlocked will be very minimal, and the cost we pay is that we lose consistency.

This revision now requires changes to proceed.Dec 16 2021, 11:44

tyler-smith added inline comments.Dec 20 2021, 00:03

src/net_processing.cpp
5145 ↗	(On Diff #31426)	@deadalnix Is it reasonable to expect the caller to hold cs_main, with a note in the method doc, or should the entire abstracted function be re-inlined so it's local to where the local is obtained?

tyler-smith added inline comments.Dec 20 2021, 00:06

src/net_processing.cpp
4356 ↗	(On Diff #31426)	@deadalnix Just a log for the default case, or a log for each case denoting which inv type happened? The former seems sufficient but I wanted to be clear.

Addressing some review feedback.

Harbormaster completed remote builds in B17673: Diff 31457.Dec 20 2021, 00:24

Fabien added a subscriber: Fabien.Dec 20 2021, 08:34

Fabien added inline comments.

src/net_processing.cpp
2878 ↗	(On Diff #31457)	We have 2 methods to make sure locks are held when needed: Using the clang thread safety annotations: `EXCLUSIVE_LOCKS_REQUIRED(cs_main)` Using a runtime assertion: `AssertLockHeld(cs_main)` This makes sure your callsite is holding the lock.

Move lock out of loop.

Harbormaster completed remote builds in B17719: Diff 31524.Dec 22 2021, 06:50

Build Bitcoin ABC Diffs / Diff Testing (build-clang-tidy) failed.

Tail of the build log:

[389/448] bitcoin: testing bip32_tests
[390/448] Running utility command for check-bitcoin-scheduler_tests
[391/448] Running utility command for check-bitcoin-merkleblock_tests
[392/448] Running utility command for check-bitcoin-bip32_tests
[393/448] bitcoin: testing sync_tests
[394/448] bitcoin: testing torcontrol_tests
[395/448] Running utility command for check-bitcoin-sync_tests
[396/448] Running utility command for check-bitcoin-torcontrol_tests
[397/448] bitcoin: testing settings_tests
[398/448] Running utility command for check-bitcoin-settings_tests
[399/448] bitcoin: testing streams_tests
[400/448] bitcoin: testing txvalidationcache_tests
[401/448] Running utility command for check-bitcoin-streams_tests
[402/448] Running utility command for check-bitcoin-txvalidationcache_tests
[403/448] bitcoin: testing timedata_tests
[404/448] bitcoin: testing serialize_tests
[405/448] Running utility command for check-bitcoin-timedata_tests
[406/448] bitcoin: testing validation_flush_tests
[407/448] Running utility command for check-bitcoin-serialize_tests
[408/448] Running utility command for check-bitcoin-validation_flush_tests
[409/448] bitcoin: testing op_reversebytes_tests
[410/448] bitcoin: testing radix_tests
[411/448] Running utility command for check-bitcoin-op_reversebytes_tests
[412/448] bitcoin: testing cuckoocache_tests
[413/448] Running utility command for check-bitcoin-radix_tests
[414/448] bitcoin: testing schnorr_tests
[415/448] bitcoin: testing compilerbug_tests
[416/448] Running utility command for check-bitcoin-cuckoocache_tests
[417/448] Running utility command for check-bitcoin-schnorr_tests
[418/448] bitcoin: testing checkpoints_tests
[419/448] Running utility command for check-bitcoin-compilerbug_tests
[420/448] bitcoin: testing script_standard_tests
[421/448] Running utility command for check-bitcoin-checkpoints_tests
[422/448] Running utility command for check-bitcoin-script_standard_tests
[423/448] bitcoin: testing validationinterface_tests
[424/448] Running utility command for check-bitcoin-validationinterface_tests
[425/448] bitcoin: testing cashaddr_tests
[426/448] bitcoin: testing util_tests
[427/448] Running utility command for check-bitcoin-cashaddr_tests
[428/448] bitcoin: testing script_tests
[429/448] Running utility command for check-bitcoin-util_tests
[430/448] Running utility command for check-bitcoin-script_tests
[431/448] bitcoin: testing transaction_tests
[432/448] Running utility command for check-bitcoin-transaction_tests
[433/448] bitcoin: testing versionbits_tests
[434/448] bitcoin: testing crypto_tests
[435/448] Running utility command for check-bitcoin-versionbits_tests
[436/448] Running utility command for check-bitcoin-crypto_tests
[437/448] bitcoin: testing blockcheck_tests
[438/448] bitcoin: testing intmath_tests
[439/448] Running utility command for check-bitcoin-blockcheck_tests
[440/448] bitcoin: testing monolith_opcodes_tests
[441/448] Running utility command for check-bitcoin-intmath_tests
[442/448] Running utility command for check-bitcoin-monolith_opcodes_tests
[443/448] bitcoin: testing coinselector_tests
[444/448] Running utility command for check-bitcoin-coinselector_tests
[445/448] bitcoin: testing coins_tests
[446/448] Running utility command for check-bitcoin-coins_tests
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1

Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Failed tests logs:

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

------- Stdout: -------
2021-12-22T09:12:27.142000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20211222_091154/rpc_net_25
2021-12-22T09:12:28.519000Z TestFramework (INFO): Test getconnectioncount
2021-12-22T09:12:28.521000Z TestFramework (INFO): Test getpeerinfo
2021-12-22T09:12:31.584000Z TestFramework (INFO): Test getnettotals
2021-12-22T09:12:37.803000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
            lambda:
'''
2021-12-22T09:12:37.803000Z 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/rpc_net.py", line 62, in run_test
    self.test_getnettotals()
  File "/work/test/functional/rpc_net.py", line 85, in test_getnettotals
    timeout=1)
  File "/work/test/functional/test_framework/test_framework.py", line 671, in wait_until
    timeout_factor=self.options.timeout_factor)
  File "/work/test/functional/test_framework/util.py", line 286, in wait_until_helper
    "Predicate {} not true after {} seconds".format(predicate_source, timeout))
AssertionError: Predicate ''''
            lambda:
''' not true after 1.0 seconds
2021-12-22T09:12:37.854000Z TestFramework (INFO): Stopping nodes
2021-12-22T09:12:51.167000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20211222_091154/rpc_net_25
2021-12-22T09:12:51.167000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20211222_091154/rpc_net_25/test_framework.log
2021-12-22T09:12:51.167000Z TestFramework (ERROR): 
2021-12-22T09:12:51.167000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20211222_091154/rpc_net_25' to consolidate all logs
2021-12-22T09:12:51.167000Z TestFramework (ERROR): 
2021-12-22T09:12:51.167000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2021-12-22T09:12:51.167000Z TestFramework (ERROR): https://github.com/Bitcoin-ABC/bitcoin-abc/issues
2021-12-22T09:12:51.167000Z TestFramework (ERROR):

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

Fabien accepted this revision.Dec 22 2021, 09:45

Fabien added inline comments.

src/net_processing.cpp
4417 ↗	(On Diff #31524)	You can keep this newline

I restarted the CI builds, you got failures unrelated to the diff that needs to be addresses separately.

Fix unintentional additions.

Harbormaster completed remote builds in B17736: Diff 31549.Dec 22 2021, 20:22

deadalnix accepted this revision.Jan 5 2022, 16:34

deadalnix added inline comments.

src/net_processing.cpp
2919 ↗	(On Diff #31549)	You don't need a semicolon.

This revision is now accepted and ready to land.Jan 5 2022, 16:34

Closed by commit rABC6979db2ce876: [avalanche] Move block-vote decision into getAvalancheVoteForBlock. (authored by tyler-smith). · Explain WhyJan 10 2022, 06:36

This revision was automatically updated to reflect the committed changes.

tyler-smith added a commit: rABC6979db2ce876: [avalanche] Move block-vote decision into getAvalancheVoteForBlock..

[avalanche] Move block-vote decision into getAvalancheVoteForBlock.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 31710

src/net_processing.cpp

[avalanche] Move block-vote decision into getAvalancheVoteForBlock.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 31710

src/net_processing.cpp

[avalanche] Move block-vote decision into getAvalancheVoteForBlock.
ClosedPublic
Actions

Revision Contents
Changeset List