Page MenuHomePhabricator

[avalanche] Fix a data race where updating availability scores can coincide with initialization of state
ClosedPublic

Authored by sdulfari on Oct 21 2022, 17:53.

Details

Summary

There is a data race where UpdateAvalancheStatistics() may be called while
m_avalanche_state is being initialized. This patch refactors out the
AvalancheState container which is not that useful since it only saves a few
bytes per peer that does not respond to avalanche polls. The resulting design
is simpler and less error prone.

Depends on D12325

Test Plan
ninja check check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Tail of the build log:

    ~~~~~~~^
1 error generated.
[178/477] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/i2p_tests.cpp.o
[179/477] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/__/__/wallet/test/wallet_test_fixture.cpp.o
[180/477] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/message_writer_tests.cpp.o
[181/477] Linking CXX executable src/seeder/test/test-seeder
[182/477] seeder: testing options_tests
[183/477] Running utility command for check-seeder-options_tests
[184/477] seeder: testing message_writer_tests
[185/477] Running utility command for check-seeder-message_writer_tests
[186/477] seeder: testing p2p_messaging_tests
[187/477] Running utility command for check-seeder-p2p_messaging_tests
[188/477] seeder: testing parse_name_tests
[189/477] Running utility command for check-seeder-parse_name_tests
[190/477] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/net_peer_eviction_tests.cpp.o
[191/477] seeder: testing write_name_tests
[192/477] Running utility command for check-seeder-write_name_tests
[193/477] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_tests.cpp.o
[194/477] Running seeder test suite
PASSED: seeder test suite
[195/477] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/denialofservice_tests.cpp.o
[196/477] secp256k1: testing secp256k1-exhaustive_tests
[197/477] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/apptests.cpp.o
[198/477] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_main.cpp.o
[199/477] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/txrequest_tests.cpp.o
[200/477] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/rpcnestedtests.cpp.o
[201/477] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/paymentservertests.cpp.o
[202/477] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/addressbooktests.cpp.o
[203/477] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
[204/477] Linking CXX executable src/qt/test/test_bitcoin-qt
[205/477] bitcoin-qt: testing test_bitcoin-qt
[206/477] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[207/477] Building CXX object src/avalanche/test/CMakeFiles/test-avalanche.dir/processor_tests.cpp.o
[208/477] Linking CXX executable src/avalanche/test/test-avalanche
[209/477] avalanche: testing init_tests
[210/477] Running utility command for check-avalanche-init_tests
[211/477] avalanche: testing delegation_tests
[212/477] Running utility command for check-avalanche-delegation_tests
[213/477] avalanche: testing proofcomparator_tests
[214/477] Running utility command for check-avalanche-proofcomparator_tests
[215/477] avalanche: testing proofpool_tests
[216/477] Running utility command for check-avalanche-proofpool_tests
[217/477] avalanche: testing proof_tests
[218/477] Running utility command for check-avalanche-proof_tests
[219/477] avalanche: testing compactproofs_tests
[220/477] Running utility command for check-avalanche-compactproofs_tests
[221/477] avalanche: testing processor_tests
[222/477] Running utility command for check-avalanche-processor_tests
[223/477] avalanche: testing voterecord_tests
[224/477] Running utility command for check-avalanche-voterecord_tests
[225/477] avalanche: testing peermanager_tests
[226/477] Running utility command for check-avalanche-peermanager_tests
[227/477] Running avalanche test suite
PASSED: avalanche test suite
[228/477] secp256k1: testing secp256k1-tests
[229/477] Running secp256k1 test suite
PASSED: secp256k1 test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang failed with exit code 1

Tail of the build log:

OK
[170/470] Building CXX object src/pow/test/CMakeFiles/test-pow.dir/daa_tests.cpp.o
[171/470] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/parse_name_tests.cpp.o
[172/470] Building CXX object src/pow/test/CMakeFiles/test-pow.dir/eda_tests.cpp.o
[173/470] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/options_tests.cpp.o
[174/470] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/write_name_tests.cpp.o
[175/470] Building CXX object src/pow/test/CMakeFiles/test-pow.dir/grasberg_tests.cpp.o
[176/470] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/p2p_messaging_tests.cpp.o
[177/470] avalanche: testing peermanager_tests
[178/470] Running utility command for check-avalanche-peermanager_tests
[179/470] Running avalanche test suite
PASSED: avalanche test suite
[180/470] Linking CXX executable src/pow/test/test-pow
[181/470] Automatic MOC for target test_bitcoin-qt
[182/470] pow: testing daa_tests
[183/470] Running utility command for check-pow-daa_tests
[184/470] pow: testing eda_tests
[185/470] Running utility command for check-pow-eda_tests
[186/470] Test Bitcoin utilities...
[187/470] pow: testing grasberg_tests
[188/470] Running utility command for check-pow-grasberg_tests
[189/470] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_tests.cpp.o
[190/470] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/txrequest_tests.cpp.o
[191/470] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/util.cpp.o
[192/470] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/bitcoinaddressvalidatortests.cpp.o
[193/470] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/compattests.cpp.o
[194/470] pow: testing aserti32d_tests
[195/470] Running utility command for check-pow-aserti32d_tests
[196/470] Running pow test suite
PASSED: pow test suite
[197/470] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/guiutiltests.cpp.o
[198/470] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_bitcoin-qt_autogen/mocs_compilation.cpp.o
[199/470] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/uritests.cpp.o
[200/470] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/__/__/wallet/test/wallet_test_fixture.cpp.o
[201/470] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/message_writer_tests.cpp.o
[202/470] Linking CXX executable src/seeder/test/test-seeder
[203/470] seeder: testing message_writer_tests
[204/470] seeder: testing parse_name_tests
[205/470] seeder: testing p2p_messaging_tests
[206/470] seeder: testing options_tests
[207/470] Running utility command for check-seeder-message_writer_tests
[208/470] seeder: testing write_name_tests
[209/470] Running utility command for check-seeder-parse_name_tests
[210/470] Running utility command for check-seeder-p2p_messaging_tests
[211/470] Running utility command for check-seeder-options_tests
[212/470] Running utility command for check-seeder-write_name_tests
[213/470] Running seeder test suite
PASSED: seeder test suite
[214/470] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/rpcnestedtests.cpp.o
[215/470] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_main.cpp.o
[216/470] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/apptests.cpp.o
[217/470] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/paymentservertests.cpp.o
[218/470] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/addressbooktests.cpp.o
[219/470] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
[220/470] Linking CXX executable src/qt/test/test_bitcoin-qt
[221/470] bitcoin-qt: testing test_bitcoin-qt
[222/470] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1

Tail of the build log:

wallet_listreceivedby.py                  | ○ Skipped | 0 s
wallet_listsinceblock.py                  | ○ Skipped | 0 s
wallet_listsinceblock.py --descriptors    | ○ Skipped | 0 s
wallet_listtransactions.py                | ○ Skipped | 0 s
wallet_listtransactions.py --descriptors  | ○ Skipped | 0 s
wallet_multiwallet.py                     | ○ Skipped | 0 s
wallet_multiwallet.py --usecli            | ○ Skipped | 0 s
wallet_reorgsrestore.py                   | ○ Skipped | 0 s
wallet_resendwallettransactions.py        | ○ Skipped | 0 s
wallet_send.py                            | ○ Skipped | 0 s
wallet_startup.py                         | ○ Skipped | 0 s
wallet_txn_clone.py                       | ○ Skipped | 0 s
wallet_txn_clone.py --mineblock           | ○ Skipped | 0 s
wallet_txn_doublespend.py                 | ○ Skipped | 0 s
wallet_txn_doublespend.py --mineblock     | ○ Skipped | 0 s
wallet_watchonly.py                       | ○ Skipped | 0 s
wallet_watchonly.py --usecli              | ○ Skipped | 0 s

ALL                                       | ✓ Passed  | 687 s (accumulated) 
Runtime: 138 s

[28/438] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.005s

OK
[30/438] cd /work/contrib/devtools/chainparams && /usr/bin/python3.9 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.004s

OK
[114/438] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
In file included from /usr/include/boost/test/unit_test.hpp:19,
                 from ../../src/test/script_tests.cpp:30:
../../src/test/script_tests.cpp: In member function ‘void script_tests::script_build::test_method()’:
../../src/test/script_tests.cpp:540:22: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without
  540 | BOOST_AUTO_TEST_CASE(script_build) {
      |                      ^~~~~~~~~~~~
[159/438] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/net_tests.cpp.o
FAILED: src/test/CMakeFiles/test_bitcoin.dir/net_tests.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DBOOST_AC_USE_STD_ATOMIC -DBOOST_ALL_NO_LIB -DBOOST_ATOMIC_DYN_LINK -DBOOST_FILESYSTEM_DYN_LINK -DBOOST_SP_USE_STD_ATOMIC -DBOOST_TEST_DYN_LINK -DBOOST_THREAD_DYN_LINK -DBOOST_UNIT_TEST_FRAMEWORK_DYN_LINK -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/univalue/include -I../../src/. -Isrc -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIE -fvisibility=hidden -fstack-reuse=none -fstack-protector-all -Wstack-protector -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -pthread -std=gnu++17 -MD -MT src/test/CMakeFiles/test_bitcoin.dir/net_tests.cpp.o -MF src/test/CMakeFiles/test_bitcoin.dir/net_tests.cpp.o.d -o src/test/CMakeFiles/test_bitcoin.dir/net_tests.cpp.o -c ../../src/test/net_tests.cpp
../../src/test/net_tests.cpp: In member function ‘void net_tests::avalanche_statistics::test_method()’:
../../src/test/net_tests.cpp:924:12: error: ‘AvalancheState’ is not a member of ‘CNode’
  924 |     CNode::AvalancheState avastats;
      |            ^~~~~~~~~~~~~~
../../src/test/net_tests.cpp:926:28: error: ‘avastats’ was not declared in this scope
  926 |     double previousScore = avastats.getAvailabilityScore();
      |                            ^~~~~~~~
[163/438] Running pow test suite
PASSED: pow test suite
[182/438] Running seeder test suite
PASSED: seeder test suite
[186/438] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[207/438] Running avalanche test suite
PASSED: avalanche test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1

Tail of the build log:

wallet_keypool.py                         | ✓ Passed  | 3 s
wallet_keypool_topup.py                   | ✓ Passed  | 4 s
wallet_keypool_topup.py --descriptors     | ✓ Passed  | 4 s
wallet_labels.py                          | ✓ Passed  | 2 s
wallet_labels.py --descriptors            | ✓ Passed  | 2 s
wallet_listreceivedby.py                  | ✓ Passed  | 9 s
wallet_listsinceblock.py                  | ✓ Passed  | 7 s
wallet_listsinceblock.py --descriptors    | ✓ Passed  | 10 s
wallet_listtransactions.py                | ✓ Passed  | 5 s
wallet_listtransactions.py --descriptors  | ✓ Passed  | 4 s
wallet_multiwallet.py                     | ✓ Passed  | 52 s
wallet_multiwallet.py --usecli            | ✓ Passed  | 17 s
wallet_reorgsrestore.py                   | ✓ Passed  | 4 s
wallet_resendwallettransactions.py        | ✓ Passed  | 7 s
wallet_send.py                            | ✓ Passed  | 9 s
wallet_startup.py                         | ✓ Passed  | 3 s
wallet_txn_clone.py                       | ✓ Passed  | 3 s
wallet_txn_clone.py --mineblock           | ✓ Passed  | 5 s
wallet_txn_doublespend.py                 | ✓ Passed  | 3 s
wallet_txn_doublespend.py --mineblock     | ✓ Passed  | 4 s
wallet_watchonly.py                       | ✓ Passed  | 1 s
wallet_watchonly.py --usecli              | ✓ Passed  | 2 s

ALL                                       | ✓ Passed  | 1817 s (accumulated) 
Runtime: 364 s

[164/478] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/net_tests.cpp.o
FAILED: src/test/CMakeFiles/test_bitcoin.dir/net_tests.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DABORT_ON_FAILED_ASSUME -DBOOST_AC_USE_STD_ATOMIC -DBOOST_ALL_NO_LIB -DBOOST_ATOMIC_DYN_LINK -DBOOST_FILESYSTEM_DYN_LINK -DBOOST_SP_USE_STD_ATOMIC -DBOOST_TEST_DYN_LINK -DBOOST_THREAD_DYN_LINK -DBOOST_UNIT_TEST_FRAMEWORK_DYN_LINK -DBUILD_BITCOIN_INTERNAL -DDEBUG -DDEBUG_LOCKORDER -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/univalue/include -I../../src/. -Isrc -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -O0 -fPIE -fvisibility=hidden -g3 -ftrapv -fstack-reuse=none -fstack-protector-all -Wstack-protector -U_FORTIFY_SOURCE -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Wredundant-move -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -pthread -std=gnu++17 -MD -MT src/test/CMakeFiles/test_bitcoin.dir/net_tests.cpp.o -MF src/test/CMakeFiles/test_bitcoin.dir/net_tests.cpp.o.d -o src/test/CMakeFiles/test_bitcoin.dir/net_tests.cpp.o -c ../../src/test/net_tests.cpp
../../src/test/net_tests.cpp: In member function ‘void net_tests::avalanche_statistics::test_method()’:
../../src/test/net_tests.cpp:924:12: error: ‘AvalancheState’ is not a member of ‘CNode’
  924 |     CNode::AvalancheState avastats;
      |            ^~~~~~~~~~~~~~
../../src/test/net_tests.cpp:926:28: error: ‘avastats’ was not declared in this scope
  926 |     double previousScore = avastats.getAvailabilityScore();
      |                            ^~~~~~~~
[180/478] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.020s

OK
[181/478] cd /work/contrib/devtools/chainparams && /usr/bin/python3.9 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.017s

OK
[196/478] Running seeder test suite
PASSED: seeder test suite
[209/478] Running pow test suite
PASSED: pow test suite
[225/478] Running avalanche test suite
PASSED: avalanche test suite
[227/478] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[229/478] Running secp256k1 test suite
PASSED: secp256k1 test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-debug failed with exit code 1

Tail of the build log:

rpc_users.py                              | ✓ Passed  | 5 s
rpc_whitelist.py                          | ✓ Passed  | 1 s
tool_wallet.py                            | ✓ Passed  | 4 s
tool_wallet.py --descriptors              | ✓ Passed  | 2 s
wallet_abandonconflict.py                 | ✓ Passed  | 5 s
wallet_address_types.py                   | ✓ Passed  | 12 s
wallet_address_types.py --descriptors     | ✓ Passed  | 8 s
wallet_avoidreuse.py                      | ✓ Passed  | 4 s
wallet_avoidreuse.py --descriptors        | ✓ Passed  | 4 s
wallet_backup.py                          | ✓ Passed  | 29 s
wallet_balance.py                         | ✓ Passed  | 6 s
wallet_balance.py --descriptors           | ✓ Passed  | 7 s
wallet_basic.py                           | ✓ Passed  | 17 s
wallet_coinbase_category.py               | ✓ Passed  | 1 s
wallet_create_tx.py                       | ✓ Passed  | 6 s
wallet_createwallet.py                    | ✓ Passed  | 2 s
wallet_createwallet.py --descriptors      | ✓ Passed  | 2 s
wallet_createwallet.py --usecli           | ✓ Passed  | 3 s
wallet_descriptor.py                      | ✓ Passed  | 6 s
wallet_disable.py                         | ✓ Passed  | 0 s
wallet_dump.py                            | ✓ Passed  | 5 s
wallet_encryption.py                      | ✓ Passed  | 5 s
wallet_encryption.py --descriptors        | ✓ Passed  | 5 s
wallet_groups.py                          | ✓ Passed  | 6 s
wallet_hd.py                              | ✓ Passed  | 6 s
wallet_hd.py --descriptors                | ✓ Passed  | 5 s
wallet_import_rescan.py                   | ✓ Passed  | 5 s
wallet_import_with_label.py               | ✓ Passed  | 1 s
wallet_importdescriptors.py               | ✓ Passed  | 3 s
wallet_importmulti.py                     | ✓ Passed  | 3 s
wallet_importprunedfunds.py               | ✓ Passed  | 2 s
wallet_importprunedfunds.py --descriptors | ✓ Passed  | 2 s
wallet_keypool.py                         | ✓ Passed  | 3 s
wallet_keypool_topup.py                   | ✓ Passed  | 3 s
wallet_keypool_topup.py --descriptors     | ✓ Passed  | 3 s
wallet_labels.py                          | ✓ Passed  | 2 s
wallet_labels.py --descriptors            | ✓ Passed  | 2 s
wallet_listreceivedby.py                  | ✓ Passed  | 6 s
wallet_listsinceblock.py                  | ✓ Passed  | 4 s
wallet_listsinceblock.py --descriptors    | ✓ Passed  | 6 s
wallet_listtransactions.py                | ✓ Passed  | 4 s
wallet_listtransactions.py --descriptors  | ✓ Passed  | 3 s
wallet_multiwallet.py                     | ✓ Passed  | 39 s
wallet_multiwallet.py --usecli            | ✓ Passed  | 10 s
wallet_reorgsrestore.py                   | ✓ Passed  | 3 s
wallet_resendwallettransactions.py        | ✓ Passed  | 4 s
wallet_send.py                            | ✓ Passed  | 7 s
wallet_startup.py                         | ✓ Passed  | 2 s
wallet_txn_clone.py                       | ✓ Passed  | 2 s
wallet_txn_clone.py --mineblock           | ✓ Passed  | 3 s
wallet_txn_doublespend.py                 | ✓ Passed  | 1 s
wallet_txn_doublespend.py --mineblock     | ✓ Passed  | 3 s
wallet_watchonly.py                       | ✓ Passed  | 1 s
wallet_watchonly.py --usecli              | ✓ Passed  | 1 s

ALL                                       | ✓ Passed  | 1216 s (accumulated) 
Runtime: 244 s

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
Fabien requested changes to this revision.Oct 22 2022, 09:03
Fabien added a subscriber: Fabien.

Back on your queue

This revision now requires changes to proceed.Oct 22 2022, 09:03

Fix missing unit test changes

Fabien requested changes to this revision.Oct 25 2022, 12:35
Fabien added inline comments.
src/net.h
782 ↗(On Diff #35969)
src/rpc/net.cpp
325 ↗(On Diff #35969)

you don't want to display availability score if avalanche is not enabled, the stat should only exist if avalanche_enabled is set

src/test/net_tests.cpp
929 ↗(On Diff #35969)

?

test/functional/abc_p2p_getavaaddr.py
239 ↗(On Diff #35969)

See other comment, that test should not change

This revision now requires changes to proceed.Oct 25 2022, 12:35

Revert returning score of 0 for avalanche-disabled nodes

src/test/net_tests.cpp
929 ↗(On Diff #35969)

AvalancheState is no longer its own class to test on so I make a dummy node for this test.

This revision is now accepted and ready to land.Oct 25 2022, 17:10