Page MenuHomePhabricator
Differential D12282

test: validation: add unittest for UpdateTip behavior
AbandonedPublic
Actions

Authored by PiRK on Oct 17 2022, 15:23.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Summary

Note: Use TestChain100DeterministicSetup instead of TestChain100Setup because CreateAndActivateUTXOSnapshot will check that the snapshot at height 110 matches a harcoded hash. There is likely a missing backport that makes TestChain100Setup a deterministic setup.

This concluces a backport of core#21526 [12/12]
https://github.com/bitcoin/bitcoin/pull/21526/commits/673a5bd3377929a0a6a62eda8b560e47bc2cca0c

Depends on D12281

Test Plan

ninja check

Diff Detail

Event Timeline

PiRK created this revision.Oct 17 2022, 15:23
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 17 2022, 15:23
PiRK requested review of this revision.Oct 17 2022, 15:23
teamcity edited the summary of this revision. (Show Details)Oct 17 2022, 15:23
Harbormaster completed remote builds in B20556: Diff 35763.Oct 17 2022, 16:19
teamcity added a comment.Oct 17 2022, 16:19
Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Tail of the build log:

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  | 642 s (accumulated) 
Runtime: 129 s

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

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

OK
[127/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) {
      |                      ^~~~~~~~~~~~
[184/438] Running seeder test suite
PASSED: seeder test suite
[190/438] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[197/438] Running pow test suite
PASSED: pow test suite
[202/438] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_chainstate_tests.cpp.o
FAILED: src/test/CMakeFiles/test_bitcoin.dir/validation_chainstate_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/validation_chainstate_tests.cpp.o -MF src/test/CMakeFiles/test_bitcoin.dir/validation_chainstate_tests.cpp.o.d -o src/test/CMakeFiles/test_bitcoin.dir/validation_chainstate_tests.cpp.o -c ../../src/test/validation_chainstate_tests.cpp
In file included from ../../src/./validation.h:18,
                 from ../../src/./test/util/chainstate.h:13,
                 from ../../src/test/validation_chainstate_tests.cpp:11:
../../src/./chain.h: In member function ‘void validation_chainstate_tests::chainstate_update_tip::test_method()’:
../../src/./chain.h:157:27: error: ‘background_cs’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  157 |         return vChain.size() > 0 ? vChain[vChain.size() - 1] : nullptr;
      |                ~~~~~~~~~~~^~
../../src/test/validation_chainstate_tests.cpp:113:18: note: ‘background_cs’ was declared here
  113 |     CChainState *background_cs;
      |                  ^~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
[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
teamcity added a comment.Oct 17 2022, 16:27
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

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  | 33 s
wallet_balance.py                         | ✓ Passed  | 4 s
wallet_balance.py --descriptors           | ✓ Passed  | 7 s
wallet_basic.py                           | ✓ Passed  | 19 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  | 2 s
wallet_descriptor.py                      | ✓ Passed  | 7 s
wallet_disable.py                         | ✓ Passed  | 0 s
wallet_dump.py                            | ✓ Passed  | 4 s
wallet_encryption.py                      | ✓ Passed  | 5 s
wallet_encryption.py --descriptors        | ✓ Passed  | 5 s
wallet_groups.py                          | ✓ Passed  | 8 s
wallet_hd.py                              | ✓ Passed  | 6 s
wallet_hd.py --descriptors                | ✓ Passed  | 5 s
wallet_import_rescan.py                   | ✓ Passed  | 6 s
wallet_import_with_label.py               | ✓ Passed  | 1 s
wallet_importdescriptors.py               | ✓ Passed  | 4 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  | 1 s
wallet_labels.py --descriptors            | ✓ Passed  | 2 s
wallet_listreceivedby.py                  | ✓ Passed  | 5 s
wallet_listsinceblock.py                  | ✓ Passed  | 5 s
wallet_listsinceblock.py --descriptors    | ✓ Passed  | 7 s
wallet_listtransactions.py                | ✓ Passed  | 4 s
wallet_listtransactions.py --descriptors  | ✓ Passed  | 3 s
wallet_multiwallet.py                     | ✓ Passed  | 41 s
wallet_multiwallet.py --usecli            | ✓ Passed  | 10 s
wallet_reorgsrestore.py                   | ✓ Passed  | 3 s
wallet_resendwallettransactions.py        | ✓ Passed  | 6 s
wallet_send.py                            | ✓ Passed  | 6 s
wallet_startup.py                         | ✓ Passed  | 2 s
wallet_txn_clone.py                       | ✓ Passed  | 1 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  | 1190 s (accumulated) 
Runtime: 238 s

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
PiRK planned changes to this revision.Oct 18 2022, 10:03
PiRK mentioned this in D12287: refactor: move update_* structs from txmempool.h to .cpp file.Oct 18 2022, 10:29
PiRK updated this revision to Diff 35789.Oct 18 2022, 12:12

workaround for false-positive [-Werror=maybe-uninitialized]

Harbormaster completed remote builds in B20574: Diff 35789.Oct 18 2022, 12:32
Fabien requested changes to this revision.Oct 18 2022, 14:14
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/test/validation_chainstate_tests.cpp
87 ↗(On Diff #35789)

BlockHash

123 ↗(On Diff #35789)

Just check for cs, I don't think any chain should be allowed to be null:

for (CChainState* cs : chainman.GetAll()) {
    BOOST_CHECK(cs);
    if (cs && cs != &chainman.ActiveChainstate()) {
        background_cs = cs;
    }
}
134 ↗(On Diff #35789)

Get the chainparams from the config below

This revision now requires changes to proceed.Oct 18 2022, 14:14
PiRK updated this revision to Diff 35801.Oct 18 2022, 14:36

address review

For an unknown reason I'm no longer to reproduce the [-Werror=maybe-uninitialized] error locally, so we will have to wait for the CI to tell if this is an acceptable solution.
Earlier today I was able to see the warning on my machine when compiling.

Harbormaster completed remote builds in B20585: Diff 35801.Oct 18 2022, 14:47
teamcity added a comment.Oct 18 2022, 14:47
Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Tail of the build log:

wallet_import_with_label.py               | ○ Skipped | 0 s
wallet_importdescriptors.py               | ○ Skipped | 0 s
wallet_importmulti.py                     | ○ Skipped | 0 s
wallet_importprunedfunds.py               | ○ Skipped | 0 s
wallet_importprunedfunds.py --descriptors | ○ Skipped | 0 s
wallet_keypool.py                         | ○ Skipped | 0 s
wallet_keypool_topup.py                   | ○ Skipped | 0 s
wallet_keypool_topup.py --descriptors     | ○ Skipped | 0 s
wallet_labels.py                          | ○ Skipped | 0 s
wallet_labels.py --descriptors            | ○ Skipped | 0 s
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  | 652 s (accumulated) 
Runtime: 131 s

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

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

OK
[180/438] Running pow test suite
PASSED: pow test suite
[189/438] Running avalanche test suite
PASSED: avalanche test suite
[203/438] Running seeder test suite
PASSED: seeder test suite
[206/438] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[207/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) {
      |                      ^~~~~~~~~~~~
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1
teamcity added a comment.Oct 18 2022, 14:53
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

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  | 14 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  | 31 s
wallet_balance.py                         | ✓ Passed  | 6 s
wallet_balance.py --descriptors           | ✓ Passed  | 7 s
wallet_basic.py                           | ✓ Passed  | 18 s
wallet_coinbase_category.py               | ✓ Passed  | 1 s
wallet_create_tx.py                       | ✓ Passed  | 5 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  | 10 s
wallet_disable.py                         | ✓ Passed  | 1 s
wallet_dump.py                            | ✓ Passed  | 4 s
wallet_encryption.py                      | ✓ Passed  | 5 s
wallet_encryption.py --descriptors        | ✓ Passed  | 5 s
wallet_groups.py                          | ✓ Passed  | 7 s
wallet_hd.py                              | ✓ Passed  | 7 s
wallet_hd.py --descriptors                | ✓ Passed  | 5 s
wallet_import_rescan.py                   | ✓ Passed  | 6 s
wallet_import_with_label.py               | ✓ Passed  | 1 s
wallet_importdescriptors.py               | ✓ Passed  | 5 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  | 1 s
wallet_labels.py --descriptors            | ✓ Passed  | 2 s
wallet_listreceivedby.py                  | ✓ Passed  | 5 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  | 40 s
wallet_multiwallet.py --usecli            | ✓ Passed  | 10 s
wallet_reorgsrestore.py                   | ✓ Passed  | 3 s
wallet_resendwallettransactions.py        | ✓ Passed  | 12 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  | 1214 s (accumulated) 
Runtime: 243 s

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
PiRK updated this revision to Diff 35802.EditedOct 18 2022, 14:54

revert to using the workaround

IMO the false-positive is caused by a compiler not knowing for sure if background_cs is initialized when initialization happens based on a condition. The compiler does not understand that at least one of the iterations will match the if statement, and seems unable to understand that BOOST_CHECK(background_cs); already guards against uninintialized pointer.

Also revert a debugging BOOST_CHECK that I added to make sure the uint256 vs BlockHash equality check really works.

Harbormaster completed remote builds in B20586: Diff 35802.Oct 18 2022, 15:07
PiRK updated this revision to Diff 35805.Oct 18 2022, 17:10

init background_cs to nullptr

Harbormaster completed remote builds in B20587: Diff 35805.Oct 18 2022, 17:21
Fabien accepted this revision.Oct 18 2022, 19:28
This revision is now accepted and ready to land.Oct 18 2022, 19:28
PiRK mentioned this in D12281: validation: fix CheckBlockIndex for multiple chainstates.Oct 19 2022, 08:12
PiRK abandoned this revision.Oct 19 2022, 08:55

squashed with D12281

Revision Contents
Changeset List

PathSize
src/
test/
84 lines

Diff 35801

View Options

src/test/validation_chainstate_tests.cpp

Loading...