Details

Reviewers

sdulfari

Group Reviewers

Restricted Project

Commits

rABC23e36b757a56: [avalanche] Don't hold cs_main when adding a block to reconcile

Summary

This has 2 benefits:

It prevents a recursive lock of cs_main
It will prevent a deadlock after we switched to a single vote records structure

Note that this also fixes a subtle bug as a side effect: before this patch the node could poll for a block before it gets added to the chain, leading to a spurious initial "no" vote.
As a consequence the avalanche voting functional test needs a slight adjustment, which also makes it more correct.
A unit test is added to exibit the bug, and demonstrates that it's fixed with this patch.

Test Plan

With Clang and Debug:

ninja check-all

ninja check-extended
./contrib/teamcity/build-configurations.py build-tsan

Diff Detail

Repository

rABC Bitcoin ABC

Branch

avalanche_reconcile_blocks_no_recursive_lock

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 21758
Build 43151: Build Diff	build-diff · lint-circular-dependencies · build-without-wallet · build-clang-tidy · build-clang · build-debug
Build 43150: arc lint + arc unit

Event Timeline

Fabien created this revision.Jan 16 2023, 21:07

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 16 2023, 21:07

Fabien requested review of this revision.Jan 16 2023, 21:07

Harbormaster completed remote builds in B21750: Diff 37542.Jan 16 2023, 21:13

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

Tail of the build log:

[398/473] Running utility command for check-bitcoin-blockfilter_tests
[399/473] bitcoin: testing coinstatsindex_tests
[400/473] bitcoin: testing script_tests
[401/473] Running utility command for check-bitcoin-coinstatsindex_tests
[402/473] bitcoin: testing scheduler_tests
[403/473] bitcoin: testing sigencoding_tests
[404/473] Running utility command for check-bitcoin-script_tests
[405/473] Running utility command for check-bitcoin-scheduler_tests
[406/473] Running utility command for check-bitcoin-sigencoding_tests
[407/473] bitcoin: testing walletdb_tests
[408/473] bitcoin: testing txvalidationcache_tests
[409/473] Running utility command for check-bitcoin-walletdb_tests
[410/473] Running utility command for check-bitcoin-txvalidationcache_tests
[411/473] bitcoin: testing blockindex_tests
[412/473] Running utility command for check-bitcoin-blockindex_tests
[413/473] bitcoin: testing miner_tests
[414/473] bitcoin: testing merkle_tests
[415/473] Running utility command for check-bitcoin-merkle_tests
[416/473] bitcoin: testing mempool_tests
[417/473] Running utility command for check-bitcoin-miner_tests
[418/473] Running utility command for check-bitcoin-mempool_tests
[419/473] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
[420/473] bitcoin: testing denialofservice_tests
[421/473] bitcoin: testing crypto_tests
[422/473] bitcoin: testing rcu_tests
[423/473] Running utility command for check-bitcoin-denialofservice_tests
[424/473] Running utility command for check-bitcoin-crypto_tests
[425/473] Running utility command for check-bitcoin-rcu_tests
[426/473] bitcoin: testing init_tests
[427/473] Running utility command for check-bitcoin-init_tests
[428/473] bitcoin: testing uint256_tests
[429/473] bitcoin: testing bitmanip_tests
[430/473] bitcoin: testing scriptnum_tests
[431/473] Running utility command for check-bitcoin-uint256_tests
[432/473] Running utility command for check-bitcoin-bitmanip_tests
[433/473] Running utility command for check-bitcoin-scriptnum_tests
[434/473] bitcoin: testing cuckoocache_tests
[435/473] Linking CXX executable src/qt/test/test_bitcoin-qt
[436/473] Running utility command for check-bitcoin-cuckoocache_tests
[437/473] bitcoin: testing wallet_crypto_tests
[438/473] Running utility command for check-bitcoin-wallet_crypto_tests
[439/473] bitcoin: testing radix_tests
[440/473] Running utility command for check-bitcoin-radix_tests
[441/473] bitcoin: testing txrequest_tests
[442/473] Running utility command for check-bitcoin-txrequest_tests
[443/473] bitcoin-qt: testing test_bitcoin-qt
[444/473] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[445/473] bitcoin: testing coinselector_tests
[446/473] Running utility command for check-bitcoin-coinselector_tests
[447/473] bitcoin: testing wallet_tests
[448/473] Running utility command for check-bitcoin-wallet_tests
[449/473] bitcoin: testing transaction_tests
[450/473] Running utility command for check-bitcoin-transaction_tests
[451/473] bitcoin: testing coins_tests
[452/473] Running utility command for check-bitcoin-coins_tests
[453/473] Running bitcoin test suite
PASSED: bitcoin test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1

sdulfari requested changes to this revision.Jan 16 2023, 21:33

sdulfari added a subscriber: sdulfari.

sdulfari added inline comments.

src/avalanche/test/processor_tests.cpp
1793–1794 ↗	(On Diff #37542)	cleanup
test/functional/abc_p2p_avalanche_voting.py
211 ↗	(On Diff #37542)	We should answer all polls as it says... (see next comment)
232 ↗	(On Diff #37542)	We don't want to subtly change this part of the test. It's more important to make sure that we can unpark an invalidated block. As the previous comment indicates, we should continue to vote after the initial rejection until the block is invalidated so that we cover the entire range of behavior.

This revision now requires changes to proceed.Jan 16 2023, 21:33

Remove leftover, make clang tidy happy

Fabien planned changes to this revision.Jan 17 2023, 08:10

Fabien added inline comments.

test/functional/abc_p2p_avalanche_voting.py
232 ↗	(On Diff #37542)	So I generally agree, but please note that this doesn't change anything in the following test, that checks that we unfinalize the previous block if this one is unparked (independently of it being invalidated or simply rejected). The problem here is that there is no way to wait for a block to be finalized, apart from looping with sleeps and bumping the log parsing timeout accordingly, so I'll need to develop something new and more efficient to do so.

Harbormaster completed remote builds in B21756: Diff 37548.Jan 17 2023, 08:24

Fabien added inline comments.Jan 17 2023, 08:29

test/functional/abc_p2p_avalanche_voting.py
232 ↗	(On Diff #37542)	Answering to myself: there is now the `wait_for_debug_log` function: D12624

Avoid massaging the test by expanding the wait_for_debug_log function and use it to check the block is invalidated

Harbormaster completed remote builds in B21757: Diff 37550.Jan 17 2023, 10:11

Sync the validation interface queue before destructing the processor to prevent a data race in the test

Harbormaster completed remote builds in B21758: Diff 37551.Jan 17 2023, 17:03

Name improvement

Harbormaster completed remote builds in B21759: Diff 37552.Jan 17 2023, 17:43

patch looks good otherwise

test/functional/test_framework/test_node.py
577 ↗	(On Diff #37552)	callable is a python built-in so this should be renamed

This revision now requires changes to proceed.Jan 17 2023, 18:14

Fabien added inline comments.Jan 17 2023, 19:04

test/functional/test_framework/test_node.py
577 ↗	(On Diff #37552)	TIL

Don't pick a reserved keyword

Harbormaster completed remote builds in B21762: Diff 37555.Jan 17 2023, 19:26

sdulfari accepted this revision.Jan 17 2023, 19:30

This revision is now accepted and ready to land.Jan 17 2023, 19:30

Closed by commit rABC23e36b757a56: [avalanche] Don't hold cs_main when adding a block to reconcile (authored by Fabien). · Explain WhyJan 17 2023, 19:31

This revision was automatically updated to reflect the committed changes.

Fabien added a commit: rABC23e36b757a56: [avalanche] Don't hold cs_main when adding a block to reconcile.

[avalanche] Don't hold cs_main when adding a block to reconcile
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 37551

src/avalanche/test/processor_tests.cpp

src/validation.h

src/validation.cpp

test/functional/abc_p2p_avalanche_voting.py

test/functional/test_framework/test_node.py

[avalanche] Don't hold cs_main when adding a block to reconcileClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 37551

src/avalanche/test/processor_tests.cpp

src/validation.h

src/validation.cpp

test/functional/abc_p2p_avalanche_voting.py

test/functional/test_framework/test_node.py

[avalanche] Don't hold cs_main when adding a block to reconcile
ClosedPublic
Actions

Revision Contents
Changeset List