f485a07454 Add missing thread safety lock assertions in validation.h (Jon Atack)
37af8a20cf Add missing thread safety lock assertions in validation.cpp (Jon Atack)
Pull request description:
A number of functions in validation.{h,cpp} have a thread safety lock annotation in the declaration but are missing the corresponding run-time lock assertion in the definition.
ACKs for top commit:
hebasto:
re-ACK f485a07454, only suggested change since my [previous](https://github.com/bitcoin/bitcoin/pull/24177#pullrequestreview-877810465) review.
vasild:
ACK f485a07454
Tree-SHA512: c86c0c0e8fe6ec7ae9ed9890f1dd7d042aa482ecf99feb6679a670aa004f6e9a99f7bc047205a34968fab7f1f841898c59b48c3ed6245c166e3b5abbf0867445
020acea99b refactor: replace RecursiveMutex m_chainstate_mutex with Mutex (w0xlt)
ddeefeef20 refactor: add negative TS annotations for `m_chainstate_mutex` (w0xlt)
1dfd31bc26 scripted-diff: rename m_cs_chainstate -> m_chainstate_mutex (w0xlt)
Pull request description:
This PR is related to #19303 and gets rid of the `RecursiveMutex m_cs_chainstate`.
`m_cs_chainstate` is only held in `ActivateBestChain()` and `InvalidateBlock()`.
So apparently there is no recursion involved, so the `m_cs_chainstate` can be a non-recursive mutex.
ACKs for top commit:
hebasto:
ACK 020acea99b, I have reviewed the code and it looks OK, I agree it can be merged.
theStack:
Code-review ACK 020acea99b🌴
shaavan:
reACK 020acea99b
Tree-SHA512: c7c16e727e326df3410514915ce753a2a5e1da78857ef965ef683e36251e1b73c9cced4cd5231b04dbe2be0ea14084f6731b4d7a4d9a8e086e982b985e37e4b4
fa5d2e678c Remove unused char serialize (MarcoFalke)
fa24493d63 Use spans of std::byte in serialize (MarcoFalke)
fa65bbf217 span: Add BytePtr helper (MarcoFalke)
Pull request description:
This changes the serialize code (`.read()` and `.write()` functions) to take a `Span` instead of a pointer and size. This is a breaking change for the serialize interface, so at no additional cost we can also switch to `std::byte` (instead of using `char`).
The benefits of using `Span`:
* Less verbose and less fragile code when passing an already existing `Span`(-like) object to or from serialization
The benefits of using `std::byte`:
* `std::byte` can't accidentally be mistaken for an integer
The goal here is to only change serialize to use spans of `std::byte`. If needed, `AsBytes`, `MakeUCharSpan`, ... can be used (temporarily) to pass spans of the right type.
Other changes that are included here:
* [#22167](https://github.com/bitcoin/bitcoin/pull/22167) (refactor: Remove char serialize by MarcoFalke)
* [#21906](https://github.com/bitcoin/bitcoin/pull/21906) (Preserve const in cast on CTransactionSignatureSerializer by promag)
ACKs for top commit:
laanwj:
Concept and code review ACK fa5d2e678c
sipa:
re-utACK fa5d2e678c
Tree-SHA512: 08ee9eced5fb777cedae593b11e33660bed9a3e1711a7451a87b835089a96c99ce0632918bb4666a4e859c4d020f88fb50f2dd734216b0c3d1a9a704967ece6f
This checks finality at the current Tip, so clarify this in its name.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/ ) ; }
ren CheckSequenceLocks CheckSequenceLocksAtTip
ren CheckFinalTx CheckFinalTxAtTip
-END VERIFY SCRIPT-
c5b36b1c1b Mempool Update Cut-Through Optimization (Jeremy Rubin)
c49daf9885 [TESTS] Increase limitancestorcount in tournament RPC test to showcase improved algorithm (Jeremy Rubin)
Pull request description:
Often when we're updating mempool entries we update entries that we ultimately end up removing the updated entries shortly thereafter. This patch makes it so that we filter for such entries a bit earlier in processing, which yields a mild improvement for these cases, and is negligible overhead otherwise.
There's potential for a better -- but more sophisticated -- algorithm that can be used taking advantage of epochs, but I figured it is better to do something that is simple and works first and upgrade it later as the other epoch mempool work proceeds as it makes the patches for the epoch algorithm simpler to understand, so you can consider this as preparatory work. It could either go in now if it is not controversial, or we could wait until the other patch is ready to go.
ACKs for top commit:
instagibbs:
reACK c5b36b1
sipa:
utACK c5b36b1c1b
mzumsande:
Code Review ACK c5b36b1c1b
Tree-SHA512: 78b16864f77a637d8a68a65e23c019a9757d8b2243486728ef601d212ae482f6084cf8e69d810958c356f1803178046e4697207ba40d6d10529ca57de647fae6
No behavior change.
This code was introduced in 5add7a7 before we required C++11, which is
why the struct was needed. As we are now using more modern C++ and this
is the only place where lockpoints are updated for mempool entries, it
is more idiomatic to call `modify` with a lambda.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Behavior change: don't quit right after LimitMempoolSize() when a
package is partially submitted. We should still send
TransactionAddedToMempool notifications for
transactions that were submitted.
Not behavior change: add a new package validation result for mempool logic errors.
The previous interface required callers to guess that the tx had been
swapped and look up the tx again by txid to find a `MEMPOOL_ENTRY`
result. This is a confusing interface.
Instead, explicitly tell the caller that this transaction was
`DIFFERENT_WITNESS` in the result linked to the mempool entry's wtxid.
This gives the caller all the information they need in 1 lookup, and
they can query the mempool for the other transaction if needed.
fac22fd36b log: Remove GetAdjustedTime from IBD header progress estimation (MarcoFalke)
Pull request description:
This is a "refactor" that shouldn't change behaviour, because the two times are most likely equal. A minimum of 5 outbound peers are needed to adjust the time. And if the time is adjusted, it will be by at most 70 minutes (`DEFAULT_MAX_TIME_ADJUSTMENT`). Thus, the progress estimate should differ by at most 7 blocks.
ACKs for top commit:
laanwj:
Code review ACK fac22fd36b
vincenzopalazzo:
ACK fac22fd36b
Tree-SHA512: bf9f5eef66db0110dd268cf6dbfab64b9c11ba776924f5b386ceae3f2d005272cceb87ebcc96e0c8b854c051514854a2a5af39ae43bad008fac685b5aafaabd0
Shave off an extra 100 or so instructions from the
validation:block_connected tracepoint by reusing a nearby
GetTimeMicros(). This brings the tracepoint down to 54 instructions.
Still high, but much better than the previous ~154 and 8000 instructions
which it was originally.
Signed-off-by: William Casarin <jb55@jb55.com>
In the validation:block_connected tracepoint, we call block->GetHash(),
which ends up calling CBlockHeader::GetHash(), executing around 8000
serialization instructions. We don't need to do this extra work, because
block->GetHash() is already called further up in the function. Let's
save that value as a local variable and re-use it in our tracepoint so
there is no unnecessary tracepoint overhead.
Signed-off-by: William Casarin <jb55@jb55.com>
BlockManager is a large data structure, and cs_main is not required to
take its address or access every part of it. Individual BlockManager
fields and methods which do require cs_main like m_block_index and
LookupBlockIndex are already annotated separately, and these other
annotations describe locking requirements more accurately and do a
better job enforcing thread safety.
Since cs_main is not needed to access the address of the m_block object,
this commit drops cs_main LOCK calls which were added pointlessly to
satisfy this annotation in the past.
Co-authored-by: Carl Dong <contact@carldong.me>
This is a refactor and safe to do because:
* UnloadBlockIndex calls ChainstateManager::Unload, which calls
BlockManager::Unload
* Only unit tests call Unload directly
fa7efc915b Fixup style of moved code (MarcoFalke)
fade2a44f4 Move BlockManager to node/blockstorage (MarcoFalke)
Pull request description:
`BlockManager` is responsible for reading and writing block(headers). So move it to the existing `blockstorage` module in `node`. Also, move validation code unrelated to block-storage out from `BlockManager`.
ACKs for top commit:
ryanofsky:
Code review obvious ACK fa7efc915b
Tree-SHA512: 0197943d818e5f59e743b07fbb92e7661bff90081127a41e35e5692ce49d6f6a7872448670b0da282f7714580a45c8d93e571a67177c8b5f785ce9edefe834c5
b4adc5ad67 [bugfix] update lockpoints correctly during reorg (glozow)
b6002b07a3 MOVEONLY: update_lock_points to txmempool.h (glozow)
Pull request description:
I introduced a bug in #22677 (sorry! 😅)
Mempool entries cache `LockPoints`, containing the first height/blockhash/`CBlockIndex*` at which the transaction becomes valid. During a reorg, we re-check timelocks on all mempool entries using `CheckSequenceLocks(useExistingLockPoints=false)` and remove any now-invalid entries. `CheckSequenceLocks()` also mutates the `LockPoints` passed in, and we update valid entries' `LockPoints` using `update_lock_points`. Thus, `update_lock_points(lp)` needs to be called right after `CheckSequenceLocks(lp)`, otherwise we lose the data in `lp`. I incorrectly assumed they could be called in separate loops.
The incorrect behavior introduced is: if we have a reorg in which a timelocked mempool transaction is still valid but becomes valid at a different block, the cached `LockPoints` will be incorrect.
This PR fixes the bug, adds a test, and adds an assertion at the end of `removeForReorg()` to check that all mempool entries' lockpoints are valid. You can reproduce the bug by running the test added in the [test] commit on the code before the [bugfix] commit.
ACKs for top commit:
jnewbery:
ACK b4adc5ad67
vasild:
ACK b4adc5ad67
mzumsande:
Code Review ACK b4adc5ad67
hebasto:
ACK b4adc5ad67
MarcoFalke:
re-ACK b4adc5ad67🏁
Tree-SHA512: 16b59f6ff8140d0229079ca1c6b04f2f4a00a2e49931275150e4f3fe5ac4ec109698b083fa6b223ba9511f328271cc1ab081263669d5da020af7fee83c13e401
fadd73037e refactor: Remove implicit-integer-sign-change suppressions in validation.cpp (MarcoFalke)
Pull request description:
A file-wide suppression is problematic because it will wave through future violations, potentially bugs.
Fix that by using per-statement casts.
ACKs for top commit:
shaavan:
ACK fadd73037e
theStack:
Code-review ACK fadd73037e
Tree-SHA512: a8a05613be35382b92d7970f958a4e8f4332432056eaa9d72f6719495134b93aaaeea692899d9035654d0e0cf56bcd759671eeeacfd0535582c0ea048ab58a56
fa1a51cbc1 doc: testnet3 was not reset and is doing BIP30 checks again (MarcoFalke)
Pull request description:
ACKs for top commit:
theStack:
ACK fa1a51cbc1
Tree-SHA512: 793eccda583a3edb056b142c36a09a5c867f61d90b96e15e6643417d62eb651eb2f3429c5f245bdb062d18ab9bb05b5048c0888aa5a492cb7bb21a2f3f52324e
Previously, the `utxocache:flush` tracepoint was in the wrong scope and
reached every time `CChainState::FlushStateToDisk` was called, even when
there was no flushing of the cache. The tracepoint is now properly scoped
and will be reached during a full flush.
Inside the scope, the `fDoFullFlush` value will always be `true`, so it
doesn't need to be logged separately. Hence, it's dropped from the
tracepoint arguments.
fab6d6b2d1 Move pindexBestInvalid to ChainstateManager (MarcoFalke)
facd2137ec Move m_failed_blocks to ChainstateManager (MarcoFalke)
fa47b5c100 Move AcceptBlockHeader to ChainstateManager (MarcoFalke)
fa3d62cf7b Move FindForkInGlobalIndex from BlockManager to CChainState (MarcoFalke)
Pull request description:
Move globals or members of the wrong class to the right class.
ACKs for top commit:
naumenkogs:
ACK fab6d6b2d1
Sjors:
ACK fab6d6b2d1
shaavan:
ACK fab6d6b2d1
Tree-SHA512: 926cbdfa22838517497bacb79ed5f521f64117c2aacf96a0176f62831b4713314a32abc0213df5ee067edf63e4a4300f752a26006d36e5aab415bb91209a271f
The member is unrelated to block storage (BlockManager). It is related
to validation.
Fix the confusion by moving it.
Can be reviewed with
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
The helper was moved in commit b026e318c3,
which also mentioned that it could be moved to CChainState. So do that,
as the functionality is not block-storage related.
This also allows to drop one function argument.
2283b9cd1e test: add tests for LoadBlockIndex when using multiple chainstates (James O'Beirne)
0fd599a51a validation: have LoadBlockIndex account for snapshot use (James O'Beirne)
d0c6e61f5d validation: don't modify genesis during snapshot load (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
Currently, `BlockManager::LoadBlockIndex` adds all blocks that have downloaded transactions to the active chain state's `setBlockIndexCandidates` set, ignoring the background chain state.
This PR changes ChainstateManager::LoadBlockIndex to update `setBlockIndexCandidates` in the background chain, not just the active chain. In the active chain, the same blocks are added as before. In the background chain, only blocks that have actually been validated, not blocks marked assumed-valid are added so the background chain will continue to download and validate assumed-valid blocks.
ACKs for top commit:
MarcoFalke:
Concept ACK 2283b9cd1e🤽
Sjors:
utACK 2283b9cd1e
Tree-SHA512: 7c9a80802df4722d85d12b78d2e7f628ac5f11cb8be66913d5c3230339bd1220c6723805509d4460826a17d1dc04b0ae172eb7d09ac0ea5dc5e41d77975cbd5e
Ensure that blocks past the snapshot base block (i.e. the end of the
assumed-valid region of the chain) are not included in
setBlockIndexCandidates for the background validation chainstate. These
blocks, while fully validated and lacking the BLOCK_ASSUMED_VALID flag,
*rely* on blocks which are assumed-valid, and so shouldn't be added to
the IBD chainstate.
Co-authored-by: Russ Yanofsky <russ@yanofsky.org>
Avoid modifying the genesis block index entry during snapshot load. This
is because, in a future change that fixes LoadBlockIndex for UTXO
snapshots, we detect block index entries that are reliant on
assumed-valid ancestors and treat them specially.
Since the genesis block doesn't have BLOCK_VALID_SCRIPTS, it would be
erroneously marked BLOCK_ASSUMED_VALID during snapshot load if we didn't
skip it here. This would cause a "setBlockIndexCandidates() empty"
assertion to be tripped since all block index entries would be marked
assume-valid due to genesis, which is never re-validated.
There's probably no good reason to modify the genesis block index entry
during snapshot load anyway...
It would make for sense for the TODO to be done in PR 17487
(or noted in the review feedback for a follow-up),
no need to continue maintaining the TODO in the codebase.
Use the `LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE` macro to improve the
logging of snapshot persistance and no longer manually track the duration.
before
[snapshot] flushing coins cache (0 MB)... done (0.00ms)
[snapshot] flushing snapshot chainstate to disk (0 MB)... done (0.00ms)
after
FlushSnapshotToDisk: flushing coins cache (0 MB) started
FlushSnapshotToDisk: completed (0.00ms)
FlushSnapshotToDisk: saving snapshot chainstate (0 MB) started
FlushSnapshotToDisk: completed (0.00ms)
The logging can be observed in the output of
./src/test/test_bitcoin -t validation_chainstate_tests -- DEBUG_LOG_OUT
7f15eff2dd style-only: Remove redundant scope in *Chainstate (Carl Dong)
89bec827fd Collapse the 2 cs_main locks in LoadChainstate (Carl Dong)
3b1584b794 Remove all #include // for * comments (Carl Dong)
9a5a5a3d08 test/setup: Use LoadChainstate (Carl Dong)
c541da0d62 node/chainstate: Add options for in-memory DBs (Carl Dong)
ceb9790341 node/caches: Remove intermediate variables (Carl Dong)
ac4bf138b8 node/caches: Extract cache calculation logic (Carl Dong)
15f2e33bb3 validation: VerifyDB only needs Consensus::Params (Carl Dong)
4da9c076d1 node/chainstate: Decouple from ShutdownRequested (Carl Dong)
05441c2dc5 node/chainstate: Decouple from GetTime (Carl Dong)
2414ebc18b init: Delay RPC block notif until warmup finished (Carl Dong)
8d466a8504 Move -checkblocks LogPrintf to AppInitMain (Carl Dong)
aad8d59789 node/chainstate: Reduce coupling of LogPrintf (Carl Dong)
b345979a2b node/chainstate: Decouple from concept of uiInterface (Carl Dong)
ca7c0b934d Split off VerifyLoadedChainstate (Carl Dong)
adf4912d77 node/chainstate: Remove do/while loop (Carl Dong)
975235ca0a Move init logistics message for BAD_GENESIS_BLOCK to init.cpp (Carl Dong)
8715658983 Move mempool nullptr Assert out of LoadChainstate (Carl Dong)
9162a4f93e node/chainstate: Decouple from concept of NodeContext (Carl Dong)
c7a5c46e6f node/chainstate: Decouple from ArgsManager (Carl Dong)
ae9121f958 node/chainstate: Decouple from stringy errors (Carl Dong)
cbac28b72f node/chainstate: Decouple from GetTimeMillis (Carl Dong)
cb64af9635 node: Extract chainstate loading sequence (Carl Dong)
Pull request description:
This PR:
1. Coalesce the Chainstate loading sequence between `AppInitMain` and `*TestingSetup` (which makes it more tested)
2. Makes the Chainstate loading sequence reusable in preparation for future work extracting out our consensus engine.
Code-wise, this PR:
1. Extracts `AppInitMain`'s Chainstate loading sequence into a `::LoadChainstateSequence` function
2. Makes this `::LoadChainstateSequence` function reusable by
1. Decoupling it from various concepts (`ArgsManager`, `uiInterface`, etc)
2. Making it report errors using an `enum` rather than by setting a `bilingual_str`
3. Makes `*TestingSetup` use this new `::LoadChainstateSequence`
Reviewers: Aside from commentary, I've also included `git diff` flags of interest in the commit messages which I hope will aid review!
ACKs for top commit:
ryanofsky:
Code review ACK 7f15eff2dd. Thanks for updates!
MarcoFalke:
review ACK 7f15eff2dd💳
Tree-SHA512: fb9a6cbd1c511a52b477c62a5e68e53a8be5dec2fff0e44a279966afb91efbab44bf1fe7c6b1519f8464ecc25f42dd4bae8e1efbf55ee91fc90fa0b92e3a83e2
Often when we're updating mempool entries we update entries that we
ultimately end up removing the updated entries shortly thereafter. This
patch makes it so that we filter for such entries a bit earlier in
processing, which yields a mild improvement for these cases, and is
negligible overhead otherwise.
dce8c4c381 rpc: getblockfrompeer (Sjors Provoost)
b884ababc2 rpc: move Ensure* helpers to server_util.h (Sjors Provoost)
Pull request description:
This adds an RPC method to fetch a block directly from a peer. This can used to fetch stale blocks with lower proof of work that are normally ignored by the node (`headers-only` in `getchaintips`).
Usage:
```
bitcoin-cli getblockfrompeer HASH peer_n
```
Closes#20155
Limitations:
* you have to specify which peer to fetch the block from
* the node must already have the header
ACKs for top commit:
jnewbery:
ACK dce8c4c381
fjahr:
re-ACK dce8c4c381
Tree-SHA512: 843ba2b7a308f640770d624d0aa3265fdc5c6ea48e8db32269b96a082b7420f7953d1d8d1ef2e6529392c7172dded9d15639fbc9c24e7bfa5cfb79e13a5498c8
f1f10c0514 Remove CTxMemPool params from ATMP (lsilva01)
Pull request description:
Remove `CTxMemPool` parameter from `AcceptToMemoryPool` function, as suggested in https://github.com/bitcoin/bitcoin/pull/23437#issuecomment-962536149 .
This requires that `CChainState` has access to `MockedTxPool` in `tx_pool.cpp` as mentioned https://github.com/bitcoin/bitcoin/pull/23173#discussion_r731895386. So the `MockedTxPool` is attributed to `CChainState::m_mempool` before calling `AcceptToMemoryPool`.
Requires #23437.
ACKs for top commit:
jnewbery:
utACK f1f10c0514
MarcoFalke:
review ACK f1f10c0514🔙
Tree-SHA512: 2a4885f4645014fc1fa98bb1090f13721c1a0796bc0021b9cb43bc8cc13920b6eaf057d1f5ed796e0a110e7813e41fe0196334ce7c80d1231fc057a9a3bdf349
During a reorg, we re-check timelocks on all mempool entries using
CheckSequenceLocks(useExistingLockPoints=false) and remove any
now-invalid entries. CheckSequenceLocks() also mutates the LockPoints
passed in, and we update valid entries' LockPoints using
update_lock_points. Thus, update_lock_points(lp) needs to be called
right after CheckSequenceLocks(lp), otherwise we lose the data in lp.
commit bedf246 introduced a bug by separating those two loops.
fa3942fc4c Remove GetSpendHeight (MarcoFalke)
Pull request description:
It is unclear what the goal of the helper is, as the caller already
knows the spend height before calling the helper.
Also, in case the coins view is corrupted, LookupBlockIndex will return
nullptr. Dereferencing a nullptr is UB.
Fix both issues by removing it. Also, add a sanity check, which aborts
if the coins view is corrupted.
ACKs for top commit:
laanwj:
Code review ACK fa3942fc4c
ryanofsky:
Code review ACK fa3942fc4c. I'm not aware of cases where coins GetBestBlock could be different from active chain tip, and asset seems sufficient to guarantee PR doesn't change behavior if that doesn't happen.
Tree-SHA512: 29f65d72e116ec5a4509e0947ceeaa5bb6b7dfd5d174d3c7945cb15fa266d590c4f8b48e6385de74ef7d7c84ebd2255de902ad9c87c24955348a91b12e5bffd5
It is unclear what the goal of the helper is, as the caller already
knows the spend height before calling the helper.
Also, in case the coins view is corrupted, LookupBlockIndex will return
nullptr. Dereferencing a nullptr is UB.
Fix both issues by removing it. Also, add a sanity check, which aborts
if the coins view is corrupted.
a64078e385 Break validation <-> txmempool circular dependency (glozow)
64e4963c63 [mempool] always assert coin spent (glozow)
bb9078ed51 [refactor] put finality and maturity checking into a lambda (glozow)
bedf246f1e [mempool] only update lockpoints for non-removed entries (glozow)
1b3a11e126 MOVEONLY: TestLockPointValidity to txmempool (glozow)
Pull request description:
Remove 2 circular dependencies: validation - txmempool and validation - policy/rbf - txmempool
Validation should depend on txmempool (e.g. `CChainstateManager` has a mempool and we often need to know what's in our mempool to validate transactions), but txmempool is a data structure that shouldn't really need to know about chain state.
- Changes `removeForReorg()` to be parameterized by a callable that returns true/false (i.e. whether the transaction should be removed due to being now immature or nonfinal) instead of a `CChainState`. The mempool really shouldn't need to know about coinbase maturity or lockpoints, it just needs to know which entries to remove.
ACKs for top commit:
laanwj:
Code review ACK a64078e385
mjdietzx:
reACK a64078e385
theStack:
re-ACK a64078e385
Tree-SHA512: f75995200569c09dfb8ddc09729da66ddb32167ff1e8a7e72f105ec062d2d6a9a390e6b4a2a115e7ad8ad3525f891ee1503f3cd2bed11773abcaf7c3230b1136
faad05c6d2 Crash debug builds when mempool ConsensusScriptChecks fails (MarcoFalke)
Pull request description:
Currently a bug in the function might sneak around our testing infrastructure.
Fix that by turning bugs into crashes during tests.
ACKs for top commit:
glozow:
utACK faad05c6d2, there's something seriously wrong with the code if this returns false, good to throw in debug mode
Tree-SHA512: dfea1cd9ce3f1c303f49cca1417cd5c77c6ed12849aaff7b6ab1b6060f2f0c9cf5d4689017355d11f66639bab35823f65f848e6979042fa875181509dfd5d3d7
No behavior change.
Parameterize removeForReorg using a CChain and callable that
encapsulates validation logic. The mempool shouldn't need to know a
bunch of details about coinbase maturity and lock finality. Instead,
just pass in a callable function that says true/false. Breaks circular
dependency by removing txmempool's dependency on validation.
As node operators are free to set their mempool policies however they
please, it's possible for package transaction(s) to already be in the
mempool. We definitely don't want to reject the entire package in that
case (as that could be a censorship vector).
We should still return the successful result to the caller, so add
another result type to MempoolAcceptResult.
0fdb619aaf [validation] Always call mempool.check() after processing a new transaction (John Newbery)
2c64270bbe [refactor] Don't call AcceptToMemoryPool() from outside validation.cpp (John Newbery)
92a3aeecf6 [validation] Add CChainState::ProcessTransaction() (John Newbery)
36167faea9 [logging/documentation] Remove reference to AcceptToMemoryPool from error string (John Newbery)
4c24142b1e [validation] Remove comment about AcceptToMemoryPool() (John Newbery)
5759fd12b8 [test] Don't set bypass_limits to true in txvalidation_tests.cpp (John Newbery)
497c9e2964 [test] Don't set bypass_limits to true in txvalidationcache_tests.cpp (John Newbery)
Pull request description:
Similarly to how #18698 added `ProcessNewBlock()` and `ProcessNewBlockHeaders()` methods to the `ChainstateManager` class, this PR adds a new `ProcessTransaction()` method. Code outside validation no longer calls `AcceptToMemoryPool()` directly, but calls through the higher-level `ProcessTransaction()` method. Advantages:
- The interface is simplified. Calling code no longer needs to know about the active chainstate or mempool object, since `AcceptToMemoryPool()` can only ever be called for the active chainstate, and that chainstate knows which mempool it's using. We can also remove the `bypass_limits` argument, since that can only be used internally in validation.
- responsibility for calling `CTxMemPool::check()` is removed from the callers, and run automatically by `ChainstateManager` every time `ProcessTransaction()` is called.
ACKs for top commit:
lsilva01:
tACK 0fdb619 on Ubuntu 20.04
theStack:
Code-review ACK 0fdb619aaf
ryanofsky:
Code review ACK 0fdb619aaf. Only changes since last review: splitting & joining commits, adding more explanations to commit messages, tweaking MEMPOOL_ERROR string, fixing up argument name comments.
Tree-SHA512: 0b395c2e3ef242f0d41d47174b1646b0a73aeece38f1fe29349837e6fb832f4bf8d57e1a1eaed82a97c635cfd59015a7e07f824e0d7c00b2bee4144e80608172
No change in behavior, because package transactions would not be going
through the rbf logic in PreChecks anyway (BIP125 is currently disabled
for package acceptance, see ATMPArgs).
We draw the line here because each individual transaction in package
validation still goes through all PreChecks. For example, checking that
one's own conflicts and dependencies are disjoint (a consensus check)
and individual transaction mempool ancestor/descendant limits.
The aliases are leftover from a previous MOVEONLY refactor - they are
unnecessary and removing them reduces the diff for splitting out mempool
Checks from PreChecks, making RBF variables MemPoolAccept-wide, etc.
-BEGIN VERIFY SCRIPT-
unalias() { sed -i "s:\<$1\>:$2:g" src/validation.cpp; sed -i "/$2 = $2/d" src/validation.cpp; }
unalias nModifiedFees ws.m_modified_fees
unalias nConflictingFees ws.m_conflicting_fees
unalias nConflictingSize ws.m_conflicting_size
unalias setConflicts ws.m_conflicts
unalias allConflicting ws.m_all_conflicting
unalias setAncestors ws.m_ancestors
-END VERIFY SCRIPT-
CTxMemPool::check() will carry out internal consistency checks 1/n times,
where n is set by the `-checkmempool` configuration option. By default,
mempool consistency checks are disabled entirely on mainnet.
Therefore, this change has no effect on mainnet nodes running with
default configuration. It simply removes the responsibility to trigger
mempool consistency checks from net_processing.
This just calls through to AcceptToMemoryPool() internally, and is currently unused.
Also add a new transaction validation failure reason TX_NO_MEMPOOL to
indicate that there is no mempool.
"This logic is not necessary for memory pool transactions, as
AcceptToMemoryPool already refuses previously-known transaction ids
entirely." refers to the logic at
a206b0ea12/src/main.cpp (L484-L486),
which was later removed in commit 450cbb0944.
This is not only cleaner but also helps make sure we are always using
the virtual size measure that includes the sigop weight heuristic (which
is the vsize the mempool would return).
This bool was originally part of Workspace and was removed in #22539
when it was no longer needed in Finalize(). Re-introducing it because,
once again, multiple functions will need to know whether we're doing an
RBF. Member of MemPoolAccept so that we can use this to inform package
RBF in the future.
No change in behavior.
ATMPArgs can continue to have granular rules like switching BIP125
on/off while we create an interface for the different sets of rules for
single transactions vs multiple-testmempoolaccept vs package validation.
This is a cleaner interface than manually constructing the args, which
makes it easy to mix up ordering, use the wrong default, etc. It also
means we don't need to edit ATMP/single transaction validation code
every time we update ATMPArgs for package validation.
082c5bf099 [refactor] pass coinsview and height to check() (glozow)
ed6115f1ea [mempool] simplify some check() logic (glozow)
9e8d7ad5d9 [validation/mempool] use Spend/AddCoin instead of UpdateCoins (glozow)
09d18916af MOVEONLY: remove single-use helper func CheckInputsAndUpdateCoins (glozow)
e8639ec26a [mempool] remove now-unnecessary code (glozow)
54c6f3c1da [mempool] speed up check() by using coins cache and iterating in topo order (glozow)
30e240f65e [bench] Benchmark CTxMemPool::check() (glozow)
cb1407196f [refactor/bench] make mempool_stress bench reusable and parameterizable (glozow)
Pull request description:
Remove the txmempool <-> validation circular dependency by removing txmempool's dependency on validation. There are two functions in txmempool that need validation right now: `check()` and `removeForReorg()`. This PR removes the dependencies in `check()`.
This PR also improves the performance of `CTxMemPool::check()` by walking through the entries exactly once, in ascending ancestorcount order, which guarantees that we see parents before children.
ACKs for top commit:
jnewbery:
reACK 082c5bf099
GeneFerneau:
tACK [082c5bf](082c5bf099)
rajarshimaitra:
tACK 082c5bf099
theStack:
Code-review ACK 082c5bf099
Tree-SHA512: 40ac622af1627b5c3e6abb4f0f035d833265a8c5e8dc88faf5354875dfb5137f137825e54bbd2a2668ed37b145c5d02285f776402629f58596e51853a9a79d29
