84716b134e Add "index/blockfilterindex -> validation -> index/blockfilterindex" to expected circular dependencies (Jonas Schnelli)
ab3a0a2fb9 Add functional test for blockfilterindex in prune-mode (Jonas Schnelli)
c286a22f7b Add debug startup parameter -fastprune for more effective pruning tests (Jonas Schnelli)
5e112269c3 Avoid pruning below the blockfilterindex sync height (Jonas Schnelli)
00d57ff768 Avoid accessing nullpointer in BaseIndex::GetSummary() (Jonas Schnelli)
6abe9f5b11 Allow blockfilter in conjunction with prune (Jonas Schnelli)
Pull request description:
Maintaining the blockfilterindexes in prune mode is possible and may lead to efficient p2p based rescans of wallets (restore backups, import/sweep keys) beyond the prune height (rescans not part of that PR).
This PR allows running the blockfilterindex(es) in conjunction with pruning.
* Bitcoind/Qt will shutdown during startup when missing block data has been detected ([re]enable `-blockfilterindex` when we already have pruned)
* manual block pruning is disabled during blockfilterindex sync
* auto-pruning is delayed during blockfilterindex sync
ToDos:
* [x] Functional tests
ACKs for top commit:
fjahr:
Code review ACK 84716b1
ryanofsky:
Code review ACK 84716b134e. Only changes since last review were suggested new FindFilesToPrune argument and test.
benthecarman:
tACK 84716b134e
Tree-SHA512: 91d832c6c562c463f7ec7655c08956385413a99a896640b9737bda0183607fac530435d03d87c3c0e70c61ccdfe73fe8f3639bc7d26d33ca7e60925ebb97d77a
Values for mainnet and testnet will be specified in a follow-up PR that can be
scrutinized accordingly. This structure is required for use in snapshot activation
logic.
53e716ea11 [refactor] improve style for touched code (gzhao408)
174cb5330a [refactor] const ATMPArgs and non-const Workspace (gzhao408)
f82baf0762 [refactor] return MempoolAcceptResult (gzhao408)
9db10a5506 [refactor] clean up logic in testmempoolaccept (gzhao408)
Pull request description:
This is the first 4 commits of #20833, and does refactoring only. It should be relatively simple to review, and offers a few nice things:
- It makes accessing values that don't make sense (e.g. fee) when the tx is invalid an error.
- Returning `MempoolAcceptResult` from ATMP makes the interface cleaner. The caller can get a const instead of passing in a mutable "out" param.
- We don't have to be iterating through a bunch of lists for package validation, we can just return a `std::vector<MempoolAcceptResult>`.
- We don't have to refactor all ATMP call sites again if/when we want to return more stuff from it.
ACKs for top commit:
MarcoFalke:
ACK 53e716ea11💿
jnewbery:
Code review ACK 53e716ea11
ariard:
Code Review ACK 53e716e, I did tweak a bit the touched paths to see if we had good test coverage. Didn't find holes.
Tree-SHA512: fa6ec324a08ad9e6e55948615cda324cba176255708bf0a0a0f37cedb7a75311aa334ac6f223be7d8df3c7379502b1081102b9589f9a9afa1713ad3d9ab3c24f
This creates a cleaner interface with ATMP, allows us to make results const,
and makes accessing values that don't make sense (e.g. fee when tx is
invalid) an error.
Since these chainstates are:
1. Also vulnerable to the race condition described in the previous
commit
2. Documented as having similar semantics as m_active_chainstate
we should also protect them with ::cs_main.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
LoadExternalBlockFile mainly acts on CChainState.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
GetLastCheckPoint mainly acts on BlockManager.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
GetSpendHeight only acts on BlockManager.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
FindForkInGlobalIndex only acts on BlockManager.
Note to reviewers: Since FindForkInGlobalIndex is always called with
::ChainActive() as its first parameter, it is possible to move
FindForkInGlobalIndex to CChainState and remove this const CChain&
parameter to instead use m_chain. However, it seems like the original
intention was for FindForkInGlobalIndex to work with _any_ chain, not
just the current active chain. Let me know if this should be changed.
[META] This commit should be followed up by a scripted-diff commit which
fixes calls to LookupBlockIndex tree-wide.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
LookupBlockIndex only acts on BlockManager.
bb6fcc75d1 refactor: Drop boost::thread stuff in CCheckQueue (Hennadii Stepanov)
6784ac471b bench: Use CCheckQueue local thread pool (Hennadii Stepanov)
dba30695fc test: Use CCheckQueue local thread pool (Hennadii Stepanov)
01511776ac Add local thread pool to CCheckQueue (Hennadii Stepanov)
0ef938685b refactor: Use member initializers in CCheckQueue (Hennadii Stepanov)
Pull request description:
This PR:
- gets rid of `boost::thread_group` in the `CCheckQueue` class
- allows thread safety annotation usage in the `CCheckQueue` class
- is alternative to #14464 (https://github.com/bitcoin/bitcoin/pull/18710#issuecomment-616618525, https://github.com/bitcoin/bitcoin/pull/18710#issuecomment-617291612)
Also, with this PR (I hope) it could be easier to resurrect a bunch of brilliant ideas from #9938.
Related: #17307
ACKs for top commit:
laanwj:
Code review ACK bb6fcc75d1
LarryRuane:
ACK bb6fcc75d1
jonatack:
Code review ACK bb6fcc75d1 and verified rebase to master builds cleanly with unit/functional tests green
Tree-SHA512: fddeb720d5a391b48bb4c6fa58ed34ccc3f57862fdb8e641745c021841c8340e35c5126338271446cbd98f40bd5484f27926aa6c3e76fa478ba1efafe72e73c1
281fd1a4a0 Replace KeyIDHasher with SaltedSipHasher (Andrew Chow)
210b693db6 Add generic SaltedSipHasher (Andrew Chow)
95e61c1cf2 Move Hashers to util/hasher.{cpp/h} (Andrew Chow)
Pull request description:
There are existing `SaltedOutPointHasher` and `SaltedTxidHasher` classes used for `std::unordered_map` and `std::unordered_set` that could be useful in other places in the codebase. So we these to their own `saltedhash.{cpp/h}` file. An existing `KeyIDHasher` is moved there too. Additionally, `ScriptIDHasher`, `SaltedPubkeyHasher`, and `SaltedScriptHasher` are added so that they can be used in future work.
`KeyIDHasher` and `ScriptIDHasher` are not salted so that equality comparisons of maps and sets keyed by `CKeyID` and `CScriptID` will actually work.
Split from #19602 (and a few other PRs/branches I have).
ACKs for top commit:
laanwj:
Code review ACK 281fd1a4a0
jonatack:
ACK 281fd1a4a0, code review, debug build and ran bitcoind after rebasing to master @ dff0f6f753
fjahr:
utACK 281fd1a4a0
Tree-SHA512: bb03b231ccf3c9ecefc997b8da9c3770af4819f9be5b0a72997a103864e84046a2ac39b8eadf0dc9247bdccd53f86f433642e3a098882e6748341a9e7736271b
5021810650 Make CanFlushToDisk a const member function (practicalswift)
281cf99554 Do not run functions with necessary side-effects in assert() (practicalswift)
Pull request description:
Do not run functions with necessary side-effects in `assert()`.
ACKs for top commit:
laanwj:
Code review ACK 5021810650
sipa:
utACK 5021810650
theStack:
Code Review ACK 5021810650🟢
Tree-SHA512: 38b7faccc2f16a499f9b7b1b962b49eb58580b2a2bbf63ea49dcc418a5ecc8f21a0972fa953f66db9509c7239af67cfa2f9266423fd220963d091034d7332b96
This moves the CBlockPolicyEstimator to the NodeContext, which get rids
of two globals and allows us to conditionally create the
CBlockPolicyEstimator (and to remove a circular dep).
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Move the hashers that we use for hash tables to a common place.
Moved hashers:
- SaltedTxidHasher
- SaltedOutpointHasher
- FilterHeaderHasher
- SignatureCacheHasher
- BlockHasher
72a1d5c6f3 validation: Remove review-only comments + assertions (Carl Dong)
3756853b15 docs: Move FindFilesToPrune{,Manual} doxygen comment (Carl Dong)
485899a93c style: Make FindFilesToPrune{,Manual} match style guide (Carl Dong)
3f5b5f3f6d validation: Move FindFilesToPrune{,Manual} to BlockManager (Carl Dong)
f8d4975ab3 validation: Move PruneOneBlockFile to BlockManager (Carl Dong)
74f73c783d validation: Pass in chainman to UnloadBlockIndex (Carl Dong)
4668ded6d6 validation: Move ~CMainCleanup logic to ~BlockManager (Carl Dong)
Pull request description:
This PR paves the way for de-globalizing `g_chainman` entirely by removing the usage of `g_chainman` in the following functions/methods:
- `~CMainCleanup`
- `CChainState::FlushStateToDisk`
- `UnloadBlockIndex`
The remaining direct uses of `g_chainman` are as follows:
1. In initialization codepaths:
- `AppTests`
- `AppInitMain`
- `TestingSetup::TestingSetup`
2. `::ChainstateActive`
3. `LookupBlockIndex`
- Note: `LookupBlockIndex` is used extensively throughout the codebase and require a much larger set of changes, therefore I've left it out of this initial PR
ACKs for top commit:
MarcoFalke:
re-ACK 72a1d5c6f3👚
jnewbery:
utACK 72a1d5c6f3
Tree-SHA512: 944a4fa8405eecf39706ff944375d6824373aaeea849d11473f08181eff26b12f70043a8348a5b08e6e9021b243b481842fbdfbc7c3140ca795fce3688b7f5c3
[META] No behaviour change is intended in this commit.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
Also stop FindFilesToPrune{,Manual} from unnecessary reaching for
::ChainActive() by passing in the necessary information.
[META] This is a pure refactor commit.
Move PruneBlockFile to BlockManager because:
1. PruneOneBlockFile only acts on BlockManager
2. Eliminates the need for callers (FindFilesToPrune{,Manual}) to have a
reference to the larger ChainstateManager, just a reference to
BlockManager is enough. See following commits.
~CMainCleanup:
1. Is vestigial
2. References the g_chainman global (we should minimize g_chainman refs)
3. Only acts on g_chainman.m_blockman
4. Does the same thing as BlockManager::Unload
ea74e10acf doc: Add best practice for annotating/asserting locks (Hennadii Stepanov)
2ee7743fe7 sync.h: Make runtime lock checks require compile-time lock checks (Anthony Towns)
23d71d171e Do not hide compile-time thread safety warnings (Hennadii Stepanov)
3ddc150857 Add missed thread safety annotations (Hennadii Stepanov)
af9ea55a72 Use LockAssertion utility class instead of AssertLockHeld() (Hennadii Stepanov)
Pull request description:
On the way of transit from `RecursiveMutex` to `Mutex` (see #19303) it is crucial to have run-time `AssertLockHeld()` assertion that does _not_ hide compile-time Clang Thread Safety Analysis warnings.
On master (65e4ecabd5) using `AssertLockHeld()` could hide Clang Thread Safety Analysis warnings, e.g., with the following patch applied:
```diff
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -607,7 +607,7 @@ public:
void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs);
- void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
+ void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs);
void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs);
```
Clang compiles the code without any thread safety warnings.
See "Add missed thread safety annotations" commit for the actual thread safety warnings that are fixed in this PR.
ACKs for top commit:
MarcoFalke:
ACK ea74e10acf 🎙
jnewbery:
ACK ea74e10acf
ajtowns:
ACK ea74e10acf
Tree-SHA512: 8cba996e526751a1cb0e613c0cc1b10f027a3e9945fbfb4bd30f6355fd36b9f9c2e1e95ed3183fc254b42df7c30223278e18e5bdb5e1ef85db7fef067595d447
