Page MenuHomePhabricator

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

Authored by Fabien on Jan 16 2023, 21:07.

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

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.

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

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

sdulfari requested changes to this revision.Jan 17 2023, 18:14

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
test/functional/test_framework/test_node.py
577 ↗(On Diff #37552)

TIL

Don't pick a reserved keyword

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