kernel: De-globalize signature cache
ClosedPublic
Actions

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

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABCba5f2d10c953: kernel: De-globalize signature cache

Summary

Move its ownership to the ChainstateManager class.

Next to simplifying usage of the kernel library by no longer requiring
manual setup of the cache prior to using validation code, it also slims
down the amount of memory allocated by BasicTestingSetup.

Use this opportunity to make SignatureCache RAII styled

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

This concludes backport of core#30141
https://github.com/bitcoin/bitcoin/pull/30141/commits/606a7ab862470413ced400aa68a94fd37c8ad3d3
Depends on D18859

Test Plan

ninja all check-all bitcoin-fuzzers

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 34931
Build 69324: Build Diff	lint-circular-dependencies · build-without-wallet · build-fuzzer · build-debug · build-diff · build-clang-tidy · build-clang
Build 69323: arc lint + arc unit

Event Timeline

PiRK created this revision.Fri, Oct 31, 15:15

Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, Oct 31, 15:15

teamcity edited the summary of this revision. (Show Details)Fri, Oct 31, 15:15

Harbormaster completed remote builds in B34926: Diff 56384.Fri, Oct 31, 15:31

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

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/util.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/uritests.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/__/__/wallet/test/wallet_test_fixture.cpp.o
[219/540] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/paymentservertests.cpp.o
[220/540] pow: testing aserti32d_tests
[221/540] Running utility command for check-pow-aserti32d_tests
[222/540] Running pow test suite
PASSED: pow test suite
[223/540] Test Bitcoin utilities...
[224/540] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/txpackage_tests.cpp.o
[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] Linking CXX executable src/qt/test/test_bitcoin-qt
[234/540] Building CXX object src/avalanche/test/CMakeFiles/test-avalanche.dir/processor_tests.cpp.o
[235/540] bitcoin-qt: testing test_bitcoin-qt
[236/540] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[237/540] Linking CXX executable src/avalanche/test/test-avalanche
[238/540] avalanche: testing init_tests
[239/540] Running utility command for check-avalanche-init_tests
[240/540] avalanche: testing delegation_tests
[241/540] Running utility command for check-avalanche-delegation_tests
[242/540] avalanche: testing stakingrewards_tests
[243/540] Running utility command for check-avalanche-stakingrewards_tests
[244/540] avalanche: testing proofcomparator_tests
[245/540] Running utility command for check-avalanche-proofcomparator_tests
[246/540] avalanche: testing stakecontendercache_tests
[247/540] Running utility command for check-avalanche-stakecontendercache_tests
[248/540] avalanche: testing proofpool_tests
[249/540] Running utility command for check-avalanche-proofpool_tests
[250/540] avalanche: testing proof_tests
[251/540] Running utility command for check-avalanche-proof_tests
[252/540] avalanche: testing compactproofs_tests
[253/540] Running utility command for check-avalanche-compactproofs_tests
[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

Build Bitcoin ABC Diffs / Diff Testing (lint-circular-dependencies) failed.

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

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

Tail of the build log:

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

rebase

PiRK published this revision for review.Fri, Oct 31, 16:10

Harbormaster completed remote builds in B34931: Diff 56389.Fri, Oct 31, 16:19

Fabien added a subscriber: Fabien.Fri, Oct 31, 16:34

Fabien added inline comments.

src/node/chainstatemanager_args.cpp
73	Can you why it differs from core ? It seems they have a single option for both caches

PiRK added inline comments.Fri, Oct 31, 17:22

src/node/chainstatemanager_args.cpp
73	We differ from core on this since the initial backport of the script cache: D530 So our default is 32MB per cache, when it is 16MB for Core. Not sure about the reason for this divergence, but the rationale for using a single value and split it in half was rather weak imo (from the PR#10192 description): This is somewhat duplicative with the sigcache, as entries in the new cache will also have several entries in the sigcache. However, the sigcache is kept both as ATMP relies on it and because it prevents malleability-based DoS attacks on the new higher-level cache. Instead, the -sigcachesize option is re-used - cutting the sigcache size in half and using the newly freed memory for the script execution cache.