Page MenuHomePhabricator

kernel: De-globalize script execution cache hasher
ClosedPublic

Authored by PiRK on Fri, Oct 31, 15:04.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC42e6cb1cbab9: kernel: De-globalize script execution cache hasher
Summary

Move it to the ChainstateManager class.

This is a partial backport of core#30141
https://github.com/bitcoin/bitcoin/pull/30141/commits/021d38822c0e6a1b9497bcb20401c5c37e1bb84d
Depends on D18856

Test Plan

ninja all check-all

Diff Detail

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

Event Timeline

Tail of the build log:

[207/540] Automatic MOC for target test_bitcoin-qt
[208/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/compattests.cpp.o
[209/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_bitcoin-qt_autogen/mocs_compilation.cpp.o
[210/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/bitcoinaddressvalidatortests.cpp.o
[211/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/guiutiltests.cpp.o
[212/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/rpcnestedtests.cpp.o
[213/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_main.cpp.o
[214/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/uritests.cpp.o
[215/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/optiontests.cpp.o
[216/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/util.cpp.o
[217/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/addressbooktests.cpp.o
[218/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/paymentservertests.cpp.o
[219/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/__/__/wallet/test/wallet_test_fixture.cpp.o
[220/540] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/txpackage_tests.cpp.o
[221/540] pow: testing aserti32d_tests
[222/540] Test Bitcoin utilities...
[223/540] Running utility command for check-pow-aserti32d_tests
[224/540] Running pow test suite
PASSED: pow test suite
[225/540] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_tests.cpp.o
[226/540] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_chainstatemanager_tests.cpp.o
[227/540] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validationinterface_tests.cpp.o
[228/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/apptests.cpp.o
[229/540] secp256k1: testing secp256k1-tests
[230/540] Running secp256k1 test suite
PASSED: secp256k1 test suite
[231/540] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_tests.cpp.o
[232/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
[233/540] Building CXX object src/avalanche/test/CMakeFiles/test-avalanche.dir/processor_tests.cpp.o
[234/540] Linking CXX executable src/qt/test/test_bitcoin-qt
[235/540] Linking CXX executable src/avalanche/test/test-avalanche
[236/540] avalanche: testing init_tests
[237/540] Running utility command for check-avalanche-init_tests
[238/540] avalanche: testing delegation_tests
[239/540] avalanche: testing stakingrewards_tests
[240/540] Running utility command for check-avalanche-delegation_tests
[241/540] Running utility command for check-avalanche-stakingrewards_tests
[242/540] avalanche: testing proofcomparator_tests
[243/540] Running utility command for check-avalanche-proofcomparator_tests
[244/540] avalanche: testing stakecontendercache_tests
[245/540] Running utility command for check-avalanche-stakecontendercache_tests
[246/540] avalanche: testing proofpool_tests
[247/540] Running utility command for check-avalanche-proofpool_tests
[248/540] avalanche: testing proof_tests
[249/540] Running utility command for check-avalanche-proof_tests
[250/540] avalanche: testing compactproofs_tests
[251/540] Running utility command for check-avalanche-compactproofs_tests
[252/540] bitcoin-qt: testing test_bitcoin-qt
[253/540] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[254/540] avalanche: testing voterecord_tests
[255/540] Running utility command for check-avalanche-voterecord_tests
[256/540] avalanche: testing peermanager_tests
[257/540] Running utility command for check-avalanche-peermanager_tests
[258/540] avalanche: testing processor_tests
[259/540] Running utility command for check-avalanche-processor_tests
[260/540] Running avalanche test suite
PASSED: avalanche 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:

/work /work/abc-ci-builds/lint-circular-dependencies
A new circular dependency in the form of "kernel/chainstatemanager_opts -> script/scriptcache -> validation -> kernel/chainstatemanager_opts" appears to have been introduced.

/work/abc-ci-builds/lint-circular-dependencies
Build lint-circular-dependencies failed with exit code 1

Tail of the build log:

PASSED          [100%]##teamcity[testFinished timestamp='2025-10-31T15:23:35.634' duration='28' flowId='tests.test_iguana.test_redeem_script_input_sigchecks' name='tests.test_iguana.test_redeem_script_input_sigchecks']


============================== 20 passed in 0.43s ==============================
[209/545] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/parse_name_tests.cpp.o
[210/545] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/fixture.cpp.o
[211/545] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/write_name_tests.cpp.o
[212/545] Building CXX object src/pow/test/CMakeFiles/test-pow.dir/grasberg_tests.cpp.o
[213/545] Automatic MOC for target test_bitcoin-qt
[214/545] Linking CXX executable src/pow/test/test-pow
[215/545] Test Bitcoin utilities...
[216/545] pow: testing daa_tests
[217/545] Running utility command for check-pow-daa_tests
[218/545] pow: testing eda_tests
[219/545] Running utility command for check-pow-eda_tests
[220/545] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/p2p_messaging_tests.cpp.o
[221/545] pow: testing grasberg_tests
[222/545] Running utility command for check-pow-grasberg_tests
[223/545] Linking CXX executable src/seeder/test/test-seeder
[224/545] seeder: testing db_tests
[225/545] Running utility command for check-seeder-db_tests
[226/545] seeder: testing message_writer_tests
[227/545] Running utility command for check-seeder-message_writer_tests
[228/545] seeder: testing options_tests
[229/545] Running utility command for check-seeder-options_tests
[230/545] seeder: testing p2p_messaging_tests
[231/545] Running utility command for check-seeder-p2p_messaging_tests
[232/545] seeder: testing parse_name_tests
[233/545] Running utility command for check-seeder-parse_name_tests
[234/545] seeder: testing write_name_tests
[235/545] Running utility command for check-seeder-write_name_tests
[236/545] Running seeder test suite
PASSED: seeder test suite
[237/545] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_bitcoin-qt_autogen/mocs_compilation.cpp.o
[238/545] pow: testing aserti32d_tests
[239/545] Running utility command for check-pow-aserti32d_tests
[240/545] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/bitcoinaddressvalidatortests.cpp.o
[241/545] Running pow test suite
PASSED: pow test suite
[242/545] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/compattests.cpp.o
[243/545] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/__/chronik/test/chronikbridge_tests.cpp.o
[244/545] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/util.cpp.o
[245/545] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/uritests.cpp.o
[246/545] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/guiutiltests.cpp.o
[247/545] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/optiontests.cpp.o
[248/545] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_main.cpp.o
[249/545] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/rpcnestedtests.cpp.o
[250/545] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/__/chronik/test/bridgeprimitives_tests.cpp.o
[251/545] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_tests.cpp.o
[252/545] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/__/__/wallet/test/wallet_test_fixture.cpp.o
[253/545] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/paymentservertests.cpp.o
[254/545] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/addressbooktests.cpp.o
[255/545] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/apptests.cpp.o
[256/545] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
[257/545] Linking CXX executable src/qt/test/test_bitcoin-qt
[258/545] bitcoin-qt: testing test_bitcoin-qt
[259/545] 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

resolve two circular dependencies

PiRK published this revision for review.Fri, Oct 31, 16:09
Fabien requested changes to this revision.Fri, Oct 31, 16:27
Fabien added a subscriber: Fabien.

wrong commit

This revision now requires changes to proceed.Fri, Oct 31, 16:27

It is the right commit but we are doing some extra stuff, like moving the ValidationCache ctor to validation.h after dropping the dependency on scriptcache.cpp globals. And we have to deal with an extra layer of wrapper around uint256 for the cache key: ScriptCacheKey, introduced in D4834. This class needs the hasher as a param in its ctor.

PiRK requested review of this revision.Fri, Oct 31, 17:30
Fabien requested changes to this revision.Fri, Oct 31, 17:40

Then you need to pass the hasher as a const ref and not copy it

This revision now requires changes to proceed.Fri, Oct 31, 17:40

force the ScriptCacheKey constructor to take the hasher as a rvalue, to avoid intermediate copies. Adjust the txvalidationcache_tests accordingly.

We do the salting only once when initializing the ValidationCache, then we copy the salted hasher for each tx

This revision is now accepted and ready to land.Mon, Nov 3, 09:01