c6ca2b85a3e6e73674e210aee4ed69c4af2848e4 validation: do not wipe utxo cache for stats/scans/snapshots (Pieter Wuille)
7099e93d0a80c65a547131d7bab977b09573310c refactor: rename `FlushStateMode::ALWAYS` to `FORCE_FLUSH` (Lőrinc)
Pull request description:
Revival of https://github.com/bitcoin/bitcoin/pull/30610#issuecomment-3432564955 with the remaining comments applied on top
> Since #28280, the cost of a non-wiping sync of the UTXO cache is only proportional to the number of dirty entries, rather than proportional to the size of the entire cache. Because of that, there is no reason to perform a wiping flush in case the contents of the cache is still useful.
>
> Split the `FlushStateMode::ALWAYS` mode into a FORCE_SYNC (non-wiping) and a FORCE_FLUSH (wiping), and then use the former in `scantxoutset`, `gettxoutsetinfo`, snapshot creation.
(slightly updated after #30214)
ACKs for top commit:
optout21:
reACK c6ca2b85a3e6e73674e210aee4ed69c4af2848e4
cedwies:
reACK c6ca2b8 (trivial)
achow101:
ACK c6ca2b85a3e6e73674e210aee4ed69c4af2848e4
sedited:
ACK c6ca2b85a3e6e73674e210aee4ed69c4af2848e4
Tree-SHA512: f3525a85dc512db4a0a9c749ad47c0d3fa44085a121aa54cd77646260a719c71f754ec6570ae77779c0ed68a24799116f79c686e7a17ce57a26f6a598f7bf926
7b5d256af4a0f954a919604ed4346db3a814fb6d test: Add bitcoin-chainstate test for assumeutxo functionality (stringintech)
2bc32656498517fe58bd41dcbd0afd306d51d4b0 Fix `ChainstateManager::AddChainstate()` assertion crash (stringintech)
5f3d6bdb6659dba16941e6d6a05fd883d3f49a9d Add regtest support to bitcoin-chainstate tool (stringintech)
Pull request description:
This PR adds functional test coverage for the bitcoin-chainstate tool loading a datadir initialized with an assumeutxo snapshot.
The PR also includes:
- Fix for assertion crash in `ChainstateManager::AddChainstate()` when `prev_chainstate` has no initialized mempool (required for the test to pass)
- `-regtest` flag support for bitcoin-chainstate to enable the testing
This work started while experimenting with the bitcoin-chainstate tool and how the kernel API (#30595) behaved when loading a datadir containing assumeutxo data, during the time that PR was still under review. sedited suggested opening a PR to add this test coverage.
ACKs for top commit:
achow101:
ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d
theStack:
Concept and code-review ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d
sedited:
Re-ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d
Tree-SHA512: 5d3b0050cf2d53144b5f65451c991d5e212117b4541ae1368ecf58fde5f3cca4f018aad6ae32257b9ebb1c28b926424fbcff496ba5487cdc4eb456cea6db8b24
de4242f47476769d0a7f3e79e8297ed2dd60d9a4 refactor: Use reference for chain_start in HeadersSyncState (Daniela Brozzoni)
e37555e5401f9fca39ada0bd153e46b2c7ebd095 refactor: Use initializer list in CompressedHeader (Daniela Brozzoni)
0488bdfefe92b2c9a924be9244c91fe472462aab refactor: Remove unused parameter in ReportHeadersPresync (Daniela Brozzoni)
256246a9fa5b05141c93aeeb359394b9c7a80e49 refactor: Remove redundant parameter from CheckHeadersPoW (Daniela Brozzoni)
ca0243e3a6d77d2b218749f1ba113b81444e3f4a refactor: Remove useless CBlock::GetBlockHeader (Pieter Wuille)
45686522224598bed9923e60daad109094d7bc29 refactor: Use std::span in HasValidProofOfWork (Daniela Brozzoni)
4066bfe561a45f61a3c9bf24bec7f600ddcc7467 refactor: Compute work from headers without CBlockIndex (Daniela Brozzoni)
0bf6139e194f355d121bb2aea74715d1c4099598 p2p: Avoid an IsAncestorOfBestHeaderOrTip call (Pieter Wuille)
Pull request description:
This is a partial* revival of #25968
It contains a list of most-unrelated simplifications and optimizations to the code merged in #25717:
- Avoid an IsAncestorOfBestHeaderOrTip call: Just don't call this function when it won't have any effect.
- Compute work from headers without CBlockIndex: Avoid the need to construct a CBlockIndex object just to compute work for a header, when its nBits value suffices for that. Also use some Spans where possible.
- Remove useless CBlock::GetBlockHeader: There is no need for a function to convert a CBlock to a CBlockHeader, as it's a child class of it.
It also contains the following code cleanups, which were suggested by reviewers in #25968:
- Remove redundant parameter from CheckHeadersPoW: No need to pass consensusParams, as CheckHeadersPow already has access to m_chainparams.GetConsensus()
- Remove unused parameter in ReportHeadersPresync
- Use initializer list in CompressedHeader, also make GetFullHeader const
- Use reference for chain_start in HeadersSyncState: chain_start can never be null, so it's better to pass it as a reference rather than a raw pointer
*I decided to leave out three commits that were in #25968 (4e7ac7b94d04e056e9994ed1c8273c52b7b23931, ab52fb4e95aa2732d1a1391331ea01362e035984, 7f1cf440ca1a9c86085716745ca64d3ac26957c0), since they're a bit more involved, and I'm a new contributor. If this PR gets merged, I'll comment under #25968 to note that these three commits are still up for grabs :)
ACKs for top commit:
l0rinc:
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
polespinasa:
re-ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
sipa:
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
achow101:
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
hodlinator:
re-ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
Tree-SHA512: 1de4f3ce0854a196712505f2b52ccb985856f5133769552bf37375225ea8664a3a7a6a9578c4fd461e935cd94a7cbbb08f15751a1da7651f8962c866146d9d4b
- We no longer enforce ancestor/descendant count limit
in both PreChecks and PackageMempoolChecks.
- This commit fixes the incorrect comment by just renaming
`PackageMempoolChecks` to `PackageRBFChecks`
- The method name is self explanatory now; hence no need
for a description comment.
`ChainstateManager::IsInitialBlockDownload()` is queried on hot paths and previously acquired `cs_main` internally, contributing to lock contention.
Cache the IBD status in `m_cached_is_ibd`, and introduce `ChainstateManager::UpdateIBDStatus()` to latch it once block loading has finished and the current chain tip has enough work and is recent.
Call the updater after tip updates and after `ImportBlocks()` completes.
Since `IsInitialBlockDownload()` no longer updates the cache, drop `mutable` from `m_cached_is_ibd` and only update it from `UpdateIBDStatus()` under `cs_main`.
Update the new unit test to showcase the new `UpdateIBDStatus()`.
Co-authored-by: Patrick Strateman <patrick.strateman@gmail.com>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Factor the chain tip work/recency check out of `ChainstateManager::IsInitialBlockDownload()` into a reusable `CChain::IsTipRecent()` helper, and annotate it as requiring `cs_main` since it's reading mutable state.
Also introduce a local `chainman_ref` in the kernel import-blocks wrapper to reduce repetition and keep follow-up diffs small.
`IsInitialBlockDownload` returns were also unified to make the followup move clean.
Co-authored-by: Patrick Strateman <patrick.strateman@gmail.com>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Rename and invert the internal IBD latch so the cached value directly matches `IsInitialBlockDownload()` (true while in IBD, then latched to false).
This is a behavior-preserving refactor to avoid double negatives.
- No need to jump into the next subroutine when there is no conflict.
- This makes it clear why it is necessary to have two calls of
CheckMempoolPolicyLimts in both PackageMempoolChecks and after in
AcceptMultipleTransactionsInternal, there is a possibilty that we
we want to accept multiple transaction but they are not conflicting
with any in-mempool transaction, in that case also we want to check
that they do not bust the cluster limits.
Since #28280, the cost of a non-wiping sync of the UTXO cache is only proportional to the number of dirty entries, rather than proportional to the size of the entire cache. Because of that, there is no reason to perform a wiping flush in case the contents of the cache is still useful.
Split the FlushStateMode::ALWAYS mode into a FORCE_SYNC (non-wiping) and a FORCE_FLUSH (wiping), and then use the former in scantxoutset, gettxoutsetinfo, snapshot creation.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: cedwies <141683552+cedwies@users.noreply.github.com>
This prepares the addition of `FORCE_SYNC`.
`empty_cache` in `FlushStateToDisk` was moved up to be reusable and `FlushStateMode::FORCE_FLUSH` was used as a placeholder before we properly split the two new states.
`log_utxocache_flush.py` was regenerated and the alignment adjusted for the wider `FlushStateMode` values.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: optout <13562139+optout21@users.noreply.github.com>
6da6f503a6dd8de85780ca402f5202095aa8b6ea refactor: Let CCoinsViewCache::BatchWrite return void (TheCharlatan)
Pull request description:
CCoinsViewCache::BatchWrite always returns true if called from a backed cache, so just return void instead. Also return void from ::Sync and ::Flush.
This allows for dropping a FatalError condition and simplifying some dead error handling code a bit.
Since we now no longer exercise the "error path" when returning from `CCoinsView::BatchWrite`, make the method clear the cache instead. This should only be exercised by tests and not change production behaviour. This might slightly improve the coins_view fuzz test's ability to generate better coverage.
ACKs for top commit:
l0rinc:
ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea
andrewtoth:
re-ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea
achow101:
ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea
w0xlt:
ACK 6da6f503a6
Tree-SHA512: dfaa325b0cf8108910aebf1b27434aaddb639d10d860e96797c77ea42eca9035a54a7dc1d6a5d4eae2b75fcc9356206d3d5672243d2c906e80d19024c8b95408
e44dec027ceec2a5f74b65636689a51833d78a94 add release note about supporing non-TRUC <minrelay txns (Greg Sanders)
1488315d76ee40b9d021b7d0ecd01207eee4a426 policy: Allow any transaction version with < minrelay (Greg Sanders)
Pull request description:
Prior to cluster mempool, a policy was in place that
disallowed non-TRUC transactions from being
TX_RECONSIDERABLE in a package setting if it was below
minrelay. This was meant to simplify reasoning about mempool
trimming requirements with non-trivial transaction
topologies in the mempool. This is no longer a concern
post-cluster mempool, so this is relaxed.
In effect, this makes 0-value parent transactions relayable
through the network without the TRUC restrictions and
thus the anti-pinning protections.
ACKs for top commit:
ajtowns:
ACK e44dec027ceec2a5f74b65636689a51833d78a94 - lgtm
ismaelsadeeq:
ACK e44dec027ceec2a5f74b65636689a51833d78a94
Tree-SHA512: 6fd1a2429c55ca844d9bd669ea797e29eca3f544f0b5d3484743d3c1cdf4364f7c7a058aaf707bcfd94b84c621bea03228cb39487cbc23912b9e0980a1e5b451
d9319b06cf82664d55f255387a348135fd7f91c7 refactor: unify container presence checks - non-trivial counts (Lőrinc)
039307554eb311ce41648d1f9a12b543f480f871 refactor: unify container presence checks - trivial counts (Lőrinc)
8bb9219b6301215f53e43967d17445aaf1b81090 refactor: unify container presence checks - find (Lőrinc)
Pull request description:
### Summary
Instead of counting occurrences in sets and maps, the C++20 `::contains` method expresses the intent unambiguously and can return early on first encounter.
### Context
Applied clang‑tidy's [readability‑container‑contains](https://clang.llvm.org/extra/clang-tidy/checks/readability/container-contains.html) check, though many cases required manual changes since tidy couldn't fix them automatically.
### Changes
The changes made here were:
| From | To |
|------------------------|------------------|
| `m.find(k) == m.end()` | `!m.contains(k)` |
| `m.find(k) != m.end()` | `m.contains(k)` |
| `m.count(k)` | `m.contains(k)` |
| `!m.count(k)` | `!m.contains(k)` |
| `m.count(k) == 0` | `!m.contains(k)` |
| `m.count(k) != 1` | `!m.contains(k)` |
| `m.count(k) == 1` | `m.contains(k)` |
| `m.count(k) < 1` | `!m.contains(k)` |
| `m.count(k) > 0` | `m.contains(k)` |
| `m.count(k) != 0` | `m.contains(k)` |
> Note that `== 1`/`!= 1`/`< 1` only apply to simple [maps](https://en.cppreference.com/w/cpp/container/map/contains)/[sets](https://en.cppreference.com/w/cpp/container/set/contains) and had to be changed manually.
There are many other cases that could have been changed, but we've reverted most of those to reduce conflict with other open PRs.
-----
<details>
<summary>clang-tidy command on Mac</summary>
```bash
rm -rfd build && \
cmake -B build \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_OSX_SYSROOT="$(xcrun --show-sdk-path)" \
-DCMAKE_C_FLAGS="-target arm64-apple-macos11" \
-DCMAKE_CXX_FLAGS="-target arm64-apple-macos11" \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON
"$(brew --prefix llvm)/bin/run-clang-tidy" -quiet -p build -j$(nproc) -checks='-*,readability-container-contains' | grep -v 'clang-tidy'
```
</details>
Note: this is a take 2 of https://github.com/bitcoin/bitcoin/pull/33094 with fewer contentious changes.
ACKs for top commit:
optout21:
reACK d9319b06cf82664d55f255387a348135fd7f91c7
sedited:
ACK d9319b06cf82664d55f255387a348135fd7f91c7
janb84:
re ACK d9319b06cf82664d55f255387a348135fd7f91c7
pablomartin4btc:
re-ACK d9319b06cf82664d55f255387a348135fd7f91c7
ryanofsky:
Code review ACK d9319b06cf82664d55f255387a348135fd7f91c7. I manually reviewed the full change, and it seems there are a lot of positive comments about this and no more very significant conflicts, so I will merge it shortly.
Tree-SHA512: e4415221676cfb88413ccc446e5f4369df7a55b6642347277667b973f515c3c8ee5bfa9ee0022479c8de945c89fbc9ff61bd8ba086e70f30298cbc1762610fe1
CCoinsViewCache::BatchWrite always returns true if called from a backed
cache, so just return void instead. Also return void from ::Sync and
::Flush.
This allows for dropping a FatalError condition and simplifying some
dead error handling code a bit.
Since we now no longer exercise the "error path" when returning from
`CCoinsView::BatchWrite`, make the method clear the cache instead. This
should only be exercised by tests and not change production behaviour.
This might slightly improve the coins_view fuzz test's ability to
generate better coverage.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Deduplicate code looping over chainstate objects and calling
ActivateBestChain() and avoid need for code outside ChainstateManager to use
the GetAll() method.
Use to replace m_active_chainstate, m_ibd_chainstate, and m_snapshot_chainstate
members. This has several benefits:
- Ensures ChainstateManager treats chainstates instances equally, making
distinctions based on their attributes, not having special cases and making
assumptions based on their identities.
- Normalizes ChainstateManager representation so states that should be
impossible to reach and validation code has no handling for (like
m_snapshot_chainstate being set and m_ibd_chainstate being unset, or both
being set but m_active_chainstate pointing to the m_ibd_chainstate) can no
longer be represented.
- Makes ChainstateManager more extensible so new chainstates can be added for
different purposes, like indexing or generating and validating assumeutxo
snapshots without interrupting regular node operations. With the
m_chainstates member, new chainstates can be added and handled without needing
to make changes all over validation code or to copy/paste/modify the existing
code that's been already been written to handle m_ibd_chainstate and
m_snapshot_chainstate.
- Avoids terms that are confusing and misleading:
- The term "active chainstate" term is confusing because multiple chainstates
will be active and in use at the same time. Before a snapshot is validated,
wallet code will use the snapshot chainstate, while indexes will use the IBD
chainstate, and netorking code will use both chainstates, downloading
snapshot blocks at higher priority, but also IBD blocks simultaneously.
- The term "snapshot chainstate" is ambiguous because it could refer either
to the chainstate originally loaded from a snapshot, or to the chainstate
being used to validate a snapshot that was loaded, or to a chainstate being
used to produce a snapshot, but it is arbitrary used to refer the first
thing. The terms "most-work chainstate" or "assumed-valid chainstate" should
be less ambiguous ways to refer to chainstates loaded from snapshots.
- The term "IBD chainstate" is not just ambiguous but actively confusing
because technically IBD ends and the node is considered synced when the
snapshot chainstate finishes syncing, so in practice the IBD chainstate
will mostly by synced after IBD is complete. The term "fully-validated" is
a better way of describing the characteristics and purpose of this
chainstate.
SnapshotBlockhash() is only called two places outside of tests, and is used
redundantly in some tests, checking the same field as other checks. Simplify by
dropping the method and using the m_from_snapshot_blockhash field directly.
IsSnapshotValidated() is only called one place outside of tests, and is use
redundantly in some tests, asserting that a snapshot is not validated when a
snapshot chainstate does not even exist. Simplify by dropping the method and
checking Chainstate m_assumeutxo field directly.
IsSnapshotActive() method is only called one place outside of tests and
asserts, and is confusing because it returns true even after the snapshot is
fully validated.
The documentation which said this "implies that a background validation
chainstate is also in use" is also incorrect, because after the snapshot is
validated, the background chainstate gets disabled and IsUsable() would return
false.
Change ChainstateRole parameter passed to wallets and indexes. Wallets and
indexes need to know whether chainstate is historical and whether it is fully
validated. They should not be aware of the assumeutxo snapshot validation
process.
CurrentChainstate() is basically the same as ActiveChainstate() except it
requires cs_main to be locked when it is called, instead of locking cs_main
internally.
The name "current" should also be less confusing than "active" because multiple
chainstates can be active, and CurrentChainstate() returns the chainstate
targeting the current network tip, regardless of what chainstates are being
downloaded or how they are used.
Use to simplify code determining the chainstate leveldb paths. New method is
the now the only code that needs to figure out the storage path, so the path
doesn't need to be constructed multiple places and backed out of leveldb.
Remove hardcoded references to m_ibd_chainstate and m_snapshot_chainstate so
MaybeCompleteSnapshotValidation function can be simpler and focus on validating
the snapshot without dealing with internal ChainstateManager states.
This is a step towards being able to validate the snapshot outside of
ActivateBestChain loop so cs_main is not locked for minutes when the snapshot
block is connected.
Move duplicate code from ChainstateManager::ActivateSnapshot and
ChainstateManager::ActivateExistingSnapshot methods to a new
ChainstateManager::AddChainstate method.
The "AddChainstate" method name doesn't mention snapshots even though it is
only used to add snapshot chainstates now, because it becomes more generalized
in a later commit in this PR ("refactor: Add ChainstateManager::m_chainstates
member")
Get rid of m_disabled/IsUsable members. Instead of marking chains disabled for
different reasons, store chainstate assumeutxo status explicitly and use that
information to determine how chains should be treated.
Make Chainstate objects aware of what block they are targeting. This makes
Chainstate objects more self contained, so it's possible for validation code to
look at one Chainstate object and know what blocks to connect to it without
needing to consider global validation state or look at other Chainstate
objects.
The motivation for this change is to make validation and networking code more
readable, so understanding it just requires knowing about chains and blocks,
not reasoning about assumeutxo download states. This change also enables
simplifications to the ChainstateManager interface in subsequent commits, and
could make it easier to implement new features like creating new Chainstate
objects to generate UTXO snapshots or index UTXO data.
Note that behavior of the MaybeCompleteSnapshotValidation function is not
changing here but some checks that were previously impossible to trigger like
the BASE_BLOCKHASH_MISMATCH case have been turned into asserts.
c1e554d3e5834a140f2a53854018499a3bfe6822 refactor: consolidate 3 separate locks into one block (Andrew Toth)
41479ed1d23ea752d0ce14c2cf5627f43bceb722 test: add test for periodic flush inside ActivateBestChain (Andrew Toth)
84820561dcb2d156d1a1151a480fc1be6649cae4 validation: periodically flush dbcache during reindex-chainstate (Andrew Toth)
Pull request description:
After #30611 we periodically do a non-erasing flush of the dbcache to disk roughly every hour during IBD.
The intention was to also do this periodic flush during reindex-chainstate, so we would not risk losing progress during a system failure when reindexing with a high dbcache value.
It was discovered that reindex-chainstate does not perform a PERIODIC flush until it has already reached the tip. Since reindexing to tip usually happens within 24 hours, this behaviour was unnoticed with the previous periodic flush interval. Note that reindex-chainstate still does IF_NEEDED flushes during `ConnectBlock`, so this also would not be noticed when running with a lower dbcache value.
This patch moves the PERIODIC flush from after the outer loop in `ActivateBestChain` to inside the outer loop after we release `cs_main`. This will periodically flush during IBD, reindex-chainstate, and steady state.
ACKs for top commit:
l0rinc:
ACK c1e554d3e5834a140f2a53854018499a3bfe6822
achow101:
ACK c1e554d3e5834a140f2a53854018499a3bfe6822
sipa:
utACK c1e554d3e5834a140f2a53854018499a3bfe6822
Tree-SHA512: c447ad03e16c9978b8ed2c285b38e1b4c56e7778ab93b6f64435116f47b8931017f5f56ab53eb61656693146aaced776f666af573a41ab28e8f2b6d8657fa756
fa89f60e31d18b6c17d372f1cec26a4440e3d8b3 scripted-diff: LogPrintLevel(*,BCLog::Level::*,*) -> LogError()/LogWarning() (MarcoFalke)
fa6c7a1954eaaf60ba4c312508935868e3da51c8 scripted-diff: LogPrintLevel(*,BCLog::Level::Debug,*) -> LogDebug() (MarcoFalke)
Pull request description:
Errors and warnings should normally not happen. However, if they do happen, it is easier to spot them, if they are all logged in the same format via `LogError` or `LogWarning`.
So do that with a scripted-diff.
This is a minimal behavior change and unifies the log output from:
[net:error] Something bad happened
[net:warning] Something problematic happened
to either
[error] Something bad happened
[warning] Something problematic happened
or, when `-loglevelalways=1` is enabled:
[all:error] Something bad happened
[all:warning] Something problematic happened
Such a behavior change is desired, because all warning and error logs are written in the same style in the source code and they are logged in the same format for log consumers.
Removing the category should be harmless, because warning and error messages should be descriptive and unique anyway.
ACKs for top commit:
ajtowns:
ACK fa89f60e31d18b6c17d372f1cec26a4440e3d8b3
stickies-v:
ACK fa89f60e31d18b6c17d372f1cec26a4440e3d8b3
rkrux:
lgtm code review ACK fa89f60e31d18b6c17d372f1cec26a4440e3d8b3
Tree-SHA512: dafa47ab561609a79005faf008fe188dd714f6e07bb2dfbe4db49290d6636b12eb7ac4a18ed32bcc5526641a9f258dbc37c08e10c223ec068b97976590ff0b52
24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94 merkle: remove unused `mutated` arg from `BlockWitnessMerkleRoot` (Lőrinc)
63d640fa6a7090b4e615153b42ebf2e48d909db0 merkle: remove unused `proot` and `pmutated` args from `MerkleComputation` (Lőrinc)
be270551df30dd42b4f1b664234d0c22c09be625 merkle: migrate `path` arg of `MerkleComputation` to a reference (Lőrinc)
Pull request description:
### Summary
Simplifies merkle tree computation by removing dead code found through coverage analysis (following up on #33768 and #33786).
### History
#### BlockWitnessMerkleRoot
Original `MerkleComputation` was added in ee60e5625b (diff-706988c23877f8a557484053887f932b2cafb3b5998b50497ce7ff8118ac85a3R131) where it was called for either `&hash, mutated` or `position, &ret` args.
In 1f0e7ca09c (diff-706988c23877f8a557484053887f932b2cafb3b5998b50497ce7ff8118ac85a3L135-L165) the first usage was inlined in `ComputeMerkleRoot`, leaving the `proot` and , `pmutated` values unused in `MerkleComputation`.
Later in 4defdfab94 the method was moved to test and in 63d6ad7c89 (diff-706988c23877f8a557484053887f932b2cafb3b5998b50497ce7ff8118ac85a3R87-R95) was restored to the code, though with unused parameters again.
#### BlockWitnessMerkleRoot
`BlockWitnessMerkleRoot` was introduced in 8b49040854 where it was already called with `NULL` 8b49040854 (diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R3509) or an unused dummy 8b49040854 (diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R3598-R3599) for the `mutated` parameter.
### Fixes
#### BlockWitnessMerkleRoot
- Converts `path` parameter from pointer to reference (always non-null at call site)
- Removes `proot` and `pmutated` parameters (always `nullptr` at call site)
#### BlockWitnessMerkleRoot
- Removes unused `mutated` output parameter (always passed as `nullptr`)
The change is a refactor that shouldn't introduce *any* behavioral change, only remove dead code, leftovers from previous refactors.
### Coverage proof
https://maflcko.github.io/b-c-cov/total.coverage/src/consensus/merkle.cpp.gcov.html
ACKs for top commit:
optout21:
utACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94
Sjors:
utACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94
achow101:
ACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94
sedited:
ACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94
hodlinator:
ACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94
Tree-SHA512: 6960411304631bc381a3db7a682f6b6ba51bd58936ca85aa237c69a9109265b736b22ec4d891875bddfcbe8517bd3f014c44a4b387942eee4b01029c91ec93e1
0ac969cddfdba52f7947e9b140ef36e2b19c2c41 validation: don't reallocate cache for short-lived CCoinsViewCache (Lőrinc)
c8f5e446dc95712a63e4dd88786e2f7cb697b986 coins: reduce lookups in dbcache layer propagation (Lőrinc)
Pull request description:
This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043)
### Summary
Previously, when the parent coins cache had no entry and the child did, `BatchWrite` performed a find followed by `try_emplace`, which resulted in multiple `SipHash` computations and bucket traversals on the common insert path.
On a different path, these caches were recreated needlessly for every block connection.
### Fix for double fetch
This change uses a single leading `try_emplace` and branches on the returned `inserted` flag. In the `FRESH && SPENT` case (not used in production, only exercised by tests), we erase the just-inserted placeholder (which is constant time with no rehash anyway). Semantics are unchanged for all valid parent/child state combinations.
This change is a minimal version of [bitcoin/bitcoin@`723c49b` (#32128)](723c49b63b) and draws simplification ideas [bitcoin/bitcoin@`ae76ec7` (#30673)](ae76ec7bcf) and https://github.com/bitcoin/bitcoin/pull/30326.
### Fix for temporary cache recreation
Related to parent cache propagation, the second commit makes it possible to avoid destructuring-recreating-destructuring of these short-live parent caches created for each new block.
A few temporary `CCoinsViewCache`'s are destructed right after the `Flush()`, therefore it is not necessary to call `ReallocateCache` to recreate them right before they're killed anyway.
This change was based on a subset of https://github.com/bitcoin/bitcoin/pull/28945, the original authors and relevant commenters were added as coauthors to this version.
-----
Reindex-chainstate indicates ~1% speedup.
<details>
<summary>Details</summary>
```python
COMMITS="647cdb4f7e8041affed887e2325ee03a91078bb1 0b0c3293ffd75afb27dadc0b28426b40132a8c6b"; \
STOP=909090; DBCACHE=4500; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
(echo ""; for c in $COMMITS; do git fetch -q origin $c && git log -1 --pretty='%h %s' $c || exit 1; done; echo "") && \
hyperfine \
--sort command \
--runs 2 \
--export-json "$BASE_DIR/rdx-$(sed -E 's/(\w{8})\w+ ?/\1-/g;s/-$//'<<<"$COMMITS")-$STOP-$DBCACHE-$CC.json" \
--parameter-list COMMIT ${COMMITS// /,} \
--prepare "killall bitcoind 2>/dev/null; rm -f $DATA_DIR/debug.log; git checkout {COMMIT}; git clean -fxd; git reset --hard && \
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DENABLE_IPC=OFF && ninja -C build bitcoind && \
./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=1000 -printtoconsole=0; sleep 20" \
--cleanup "cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \
"COMPILER=$CC ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=$DBCACHE -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0"
647cdb4f7e Merge bitcoin/bitcoin#33311: net: Quiet down logging when router doesn't support natpmp/pcp
0b0c3293ff validation: don't reallocate cache for short-lived CCoinsViewCache
Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 647cdb4f7e8041affed887e2325ee03a91078bb1)
Time (mean ± σ): 16233.508 s ± 9.501 s [User: 19064.578 s, System: 951.672 s]
Range (min … max): 16226.790 s … 16240.226 s 2 runs
Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 0b0c3293ffd75afb27dadc0b28426b40132a8c6b)
Time (mean ± σ): 16039.626 s ± 17.284 s [User: 18870.130 s, System: 950.722 s]
Range (min … max): 16027.405 s … 16051.848 s 2 runs
Relative speed comparison
1.01 ± 0.00 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 647cdb4f7e8041affed887e2325ee03a91078bb1)
1.00 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 0b0c3293ffd75afb27dadc0b28426b40132a8c6b)
```
</details>
ACKs for top commit:
optout21:
utACK 0ac969cddfdba52f7947e9b140ef36e2b19c2c41
achow101:
ACK 0ac969cddfdba52f7947e9b140ef36e2b19c2c41
andrewtoth:
utACK 0ac969cddfdba52f7947e9b140ef36e2b19c2c41
sedited:
ACK 0ac969cddfdba52f7947e9b140ef36e2b19c2c41
Tree-SHA512: 9fcc3f1a8314368576a4fba96ca72665527eaa3a97964ab5b39491757f3527147d134f79a5c3456f76c1330c7ef862989d23f764236c5e2563be89a81c1cee47
4b4711369880369729893ba7baef11ba2a36cf4b validation: Reword CheckForkWarningConditions and call it also during IBD and at startup (Martin Zumsande)
2f51951d03cc1f8917e0fc931dce674f9bfedaf5 p2p: Add warning message when receiving headers for blocks cached as invalid (Martin Zumsande)
Pull request description:
In case of corruption that leads to a block being marked as invalid that is seen as valid by the rest of the network, the user currently doesn't receive good error messages, but will often be stuck in an endless headers-sync loop with no explanation (#26391).
This PR improves warnings in two ways:
- When we receive a header that is already saved in our disk, but invalid, add a warning. This will happen repeatedly during the headerssync loop (see https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-1291765534 on how to trigger it artificially).
- Removes the IBD check from `CheckForkWarningConditions` and adds a call to the function during init (`LoadChainTip()`). The existing check was added in 55ed3f1475 a long time ago when we had more sophisticated fork detection that could lead to false positives during IBD, but that logic was removed in fa62304c97 so that I don't see a reason to suppress the warning anymore.
Fixes#26391 (We'll still do the endless looping, trying to find a peer with a headers that we can use, but will now repeatedly log warnings while doing so).
ACKs for top commit:
glozow:
ACK `git range-diff 6d2c8ea9dbd77c71051935b5ab59224487509559...4b4711369880369729893ba7baef11ba2a36cf4b`
theStack:
re-ACK 4b4711369880369729893ba7baef11ba2a36cf4b
sedited:
ACK 4b4711369880369729893ba7baef11ba2a36cf4b
Tree-SHA512: 78bc53606374636d616ee10fdce0324adcc9bcee2806a7e13c9471e4c02ef00925ce6daef303bc153b7fcf5a8528fb4263c875b71d2e965f7c4332304bc4d922
This is a minimal behavior change and changes log output from:
[net:error] Something bad happened
[net:warning] Something problematic happened
to either
[error] Something bad happened
[warning] Something problematic happened
or, when -loglevelalways=1 is enabled:
[all:error] Something bad happened
[all:warning] Something problematic happened
Such a behavior change is desired, because all warning and error logs
are written in the same style in the source code and they are logged in
the same format for log consumers.
-BEGIN VERIFY SCRIPT-
sed --regexp-extended --in-place \
's/LogPrintLevel\((BCLog::[^,]*), BCLog::Level::(Error|Warning), */Log\2(/g' \
$( git grep -l LogPrintLevel ':(exclude)src/test/logging_tests.cpp' )
-END VERIFY SCRIPT-
The changes made here were:
| From | To |
|-------------------|------------------|
| `m.count(k)` | `m.contains(k)` |
| `!m.count(k)` | `!m.contains(k)` |
| `m.count(k) == 0` | `!m.contains(k)` |
| `m.count(k) != 0` | `m.contains(k)` |
| `m.count(k) > 0` | `m.contains(k)` |
The commit contains the trivial, mechanical refactors where it doesn't matter if the container can have multiple elements or not
Co-authored-by: Jan B <608446+janb84@users.noreply.github.com>
The existing IBD disable was added at a time when CheckForkWarningConditions
did also sophisticated fork detection that could lead to false positives
during IBD (55ed3f14751206fc87f0cbf8cb4e223efacef338).
The fork detection logic doesn't exist anymore
(since fa62304c9760f0de9838e56150008816e7a9bacb), so the IBD check is no
longer necessary.
Displaying the log at startup will help node operators diagnose the
problem better.
Also unify log message and alert warning text, since a long invalid chain
could be due to chainstate corruption or an actual consensus incompatibility
with peers. Previously the log assumed the former and the alert the latter.
Currently, if database corruption leads to a block being marked as
invalid incorrectly, we can get stuck in an infinite headerssync
loop with no indication what went wrong or how to fix it.
With the added log message, users will receive an explicit warning after each
failed headerssync attempt with an outbound peer.
Prior to cluster mempool, a policy was in place that
disallowed non-TRUC transactions from being
TX_RECONSIDERABLE in a package setting if it was below
minrelay. This was meant to simplify reasoning about mempool
trimming requirements with non-trivial transaction
topologies in the mempool. This is no longer a concern
post-cluster mempool, so this is relaxed.
In effect, this makes 0-value parent transactions relayable
through the network without the TRUC restrictions and
thus the anti-pinning protections.
17cf9ff7efdbab07644fc2f9017fcac1b0757c38 Use cluster size limit for -maxmempool bound, and allow -maxmempool=0 in general (Suhas Daftuar)
315e43e5d86c06b1e51b907f1942cab150205d24 Sanity check `GetFeerateDiagram()` in CTxMemPool::check() (Suhas Daftuar)
de2e9a24c40e1915827506250ed0bbda4009ce83 test: extend package rbf functional test to larger clusters (Suhas Daftuar)
4ef4ddb504e53cb148e8dd713695db37df0e1e4f doc: update policy/packages.md for new package acceptance logic (Suhas Daftuar)
79f73ad713a8d62a6172fbad228cbca848f9ff57 Add check that GetSortedScoreWithTopology() agrees with CompareMiningScoreWithTopology() (Suhas Daftuar)
a86ac117681727b6e72ab50ed751d0d3b0cdff34 Update comments for CTxMemPool class (Suhas Daftuar)
9567eaa66da88a79c54f7a77922d817862122af2 Invoke TxGraph::DoWork() at appropriate times (Suhas Daftuar)
6c5c44f774058bf2a0dfaaadc78347dcb5815f52 test: add functional test for new cluster mempool RPCs (Suhas Daftuar)
72f60c877e001bb8cbcd3a7fb7addfdaba149693 doc: Update mempool_replacements.md to reflect feerate diagram checks (Suhas Daftuar)
21693f031a534193cc7f066a5c6e23db3937bf39 Expose cluster information via rpc (Suhas Daftuar)
72e74e0d42284c712529bf3c619b1b740c070f1b fuzz: try to add more code coverage for mempool fuzzing (Suhas Daftuar)
f107417490ab5b81d3ec139de777a19db87845b6 bench: add more mempool benchmarks (Suhas Daftuar)
7976eb1ae77af2c88e1e61e85d4a61390b34b986 Avoid violating mempool policy limits in tests (Suhas Daftuar)
84de685cf7ee3baf3ca73087e5222411a0504df8 Stop tracking parents/children outside of txgraph (Suhas Daftuar)
88672e205ba1570fc92449b557fd32d836618781 Rewrite GatherClusters to use the txgraph implementation (Suhas Daftuar)
1ca4f01090cfa968c789fafde42054da3263a0e2 Fix miniminer_tests to work with cluster limits (Suhas Daftuar)
1902111e0f20fe6b5c12be019d24691d6b0b8d3e Eliminate CheckPackageLimits, which no longer does anything (Suhas Daftuar)
3a646ec4626441c8c2946598f94199a65d9646d6 Rework RBF and TRUC validation (Suhas Daftuar)
19b8479868e5c854d9268e3647b9488f9b23af0f Make getting parents/children a function of the mempool, not a mempool entry (Suhas Daftuar)
5560913e51af036b5e6907e08cd07488617b12f7 Rework truc_policy to use descendants, not children (Suhas Daftuar)
a4458d6c406215dccb31fd35e0968a65a3269670 Use txgraph to calculate descendants (Suhas Daftuar)
c8b6f70d6492a153b59697d6303fc0515f316f89 Use txgraph to calculate ancestors (Suhas Daftuar)
241a3e666b59abb695c9d0a13d7458a763c2c5a0 Simplify ancestor calculation functions (Suhas Daftuar)
b9cec7f0a1e089cd77bb2fa1c2b54e93442e594c Make removeConflicts private (Suhas Daftuar)
0402e6c7808017bf5c04edb4b68128ede7d1c1e7 Remove unused limits from CalculateMemPoolAncestors (Suhas Daftuar)
08be765ac26a3ae721cb3574d4348602a9982e44 Remove mempool logic designed to maintain ancestor/descendant state (Suhas Daftuar)
fc4e3e6bc12284d3b328c1ad19502294accfe5ad Remove unused members from CTxMemPoolEntry (Suhas Daftuar)
ff3b398d124b9efa49b612dbbb715bbe5d53e727 mempool: eliminate accessors to mempool entry ancestor/descendant cached state (Suhas Daftuar)
b9a2039f51226dce2c4e38ce5f26eefee171744b Eliminate use of cached ancestor data in miniminer_tests and truc_policy (Suhas Daftuar)
ba09fc9774d5a0eaa58d93a2fa20bef1efc74f1e mempool: Remove unused function CalculateDescendantMaximum (Suhas Daftuar)
8e49477e86b3089ea70d1f2659b9fd3a8a1f7db4 wallet: Replace max descendant count with cluster_count (Suhas Daftuar)
e031085fd464b528c186948d3cbf1c08a5a8d624 Eliminate Single-Conflict RBF Carve Out (Suhas Daftuar)
cf3ab8e1d0a2f2bdf72e61e2c2dcb35987e5b9bd Stop enforcing descendant size/count limits (Suhas Daftuar)
89ae38f48965ec0d6c0600ce4269fdc797274161 test: remove rbf carveout test from mempool_limit.py (Suhas Daftuar)
c0bd04d18fdf77a2f20f3c32f8eee4f1d71afd79 Calculate descendant information for mempool RPC output on-the-fly (Suhas Daftuar)
bdcefb8a8b0667539744eae63e9eb5b7dc1c51da Use mempool/txgraph to determine if a tx has descendants (Suhas Daftuar)
69e1eaa6ed22f542ab48da755fa63f7694a15533 Add test case for cluster size limits to TRUC logic (Suhas Daftuar)
9cda64b86c593f0d6ff8f17e483e6566f436b200 Stop enforcing ancestor size/count limits (Suhas Daftuar)
1f93227a84a54397699ca40d889f98913e4d5868 Remove dependency on cached ancestor data in mini-miner (Suhas Daftuar)
9fbe0a4ac26c2fddaa3201cdfd8b69bf1f5ffa01 rpc: Calculate ancestor data from scratch for mempool rpc calls (Suhas Daftuar)
7961496dda2eb24a3f09d661005f06611558a20a Reimplement GetTransactionAncestry() to not rely on cached data (Suhas Daftuar)
feceaa42e8eb43344ced33d94187e93268d45187 Remove CTxMemPool::GetSortedDepthAndScore (Suhas Daftuar)
21b5cea588a7bfe758a8d14efe90046b111db428 Use cluster linearization for transaction relay sort order (Suhas Daftuar)
6445aa7d97551ec5d501d91f6829071c67169122 Remove the ancestor and descendant indices from the mempool (Suhas Daftuar)
216e6937290338950215795291dbf0a533e234cf Implement new RBF logic for cluster mempool (Suhas Daftuar)
ff8f115dec6eb41f739e6e6738dd60becfa168fd policy: Remove CPFP carveout rule (Suhas Daftuar)
c3f1afc934e69a9849625924f72a5886a85eb833 test: rewrite PopulateMempool to not violate mempool policy (cluster size) limits (Suhas Daftuar)
47ab32fdb158069d4422e0f92078603c6df070a6 Select transactions for blocks based on chunk feerate (Suhas Daftuar)
dec138d1ddc79cc3a06e53ed255f0931ce46e684 fuzz: remove comparison between mini_miner block construction and miner (Suhas Daftuar)
6c2bceb200aa7206d44b551d42ad3e70943f1425 bench: rewrite ComplexMemPool to not create oversized clusters (Suhas Daftuar)
1ad4590f63855e856d59616d41a87873315c3a2e Limit mempool size based on chunk feerate (Suhas Daftuar)
b11c89cab210c87ebaf34fbd2a73d28353e8c7bd Rework miner_tests to not require large cluster limit (Suhas Daftuar)
95a8297d481e96d65ac81e4dac72b2ebecb9c765 Check cluster limits when using -walletrejectlongchains (Suhas Daftuar)
95762e6759597d201d685ed6bf6df6eedccf9a00 Do not allow mempool clusters to exceed configured limits (Suhas Daftuar)
edb3e7cdf63688058ad2b90bea0d4933d9967be8 [test] rework/delete feature_rbf tests requiring large clusters (glozow)
435fd5671116b990cf3b875b99036606f921a71d test: update feature_rbf.py replacement test (Suhas Daftuar)
34e32985e811607e7566ae7a6caeacdf8bd8384f Add new (unused) limits for cluster size/count (Suhas Daftuar)
838d7e3553661cb6ba0be32dd872bafb444822d9 Add transactions to txgraph, but without cluster dependencies (Suhas Daftuar)
d5ed9cb3eb52c33c5ac36421bb2da00290be6087 Add accessor for sigops-adjusted weight (Suhas Daftuar)
1bf3b513966e34b45ea359cbe7576383437f5d93 Add sigops adjusted weight calculator (Suhas Daftuar)
c18c68a950d3a17e80ad0bc11ac7ee3de1a87f6c Create a txgraph inside CTxMemPool (Suhas Daftuar)
29a94d5b2f26a4a8b7464894e4db944ea67241b7 Make CTxMemPoolEntry derive from TxGraph::Ref (Suhas Daftuar)
92b0079fe3863b20b71282aa82341d4b6ee4b337 Allow moving CTxMemPoolEntry objects, disallow copying (Suhas Daftuar)
6c73e4744837a7dc138a9177df3a48f30a1ba6c1 mempool: Store iterators into mapTx in mapNextTx (Suhas Daftuar)
51430680ecb722e1d4ee4a26dac5724050f41c9e Allow moving an Epoch::Marker (Suhas Daftuar)
Pull request description:
[Reopening #28676 here as a new PR, because GitHub is slow to load the page making it hard to scroll through and see comments. Also, that PR was originally opened with a prototype implementation which has changed significantly with the introduction of `TxGraph`.]
This is an implementation of the [cluster mempool proposal](https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393).
This branch implements the following observable behavior changes:
- Maintains a partitioning of the mempool into connected clusters (via the `txgraph` class), which are limited in vsize to 101 kvB by default, and limited in count to 64 by default.
- Each cluster is sorted ("linearized") to try to optimize for selecting highest-feerate-subsets of a cluster first
- Transaction selection for mining is updated to use the cluster linearizations, selecting highest feerate "chunks" first for inclusion in a block template.
- Mempool eviction is updated to use the cluster linearizations, selecting lowest feerate "chunks" first for removal.
- The RBF rules are updated to: (a) drop the requirement that no new inputs are introduced; (b) change the feerate requirement to instead check that the feerate diagram of the mempool will strictly improve; (c) replace the direct conflicts limit with a directly-conflicting-clusters limit.
- The CPFP carveout rule is eliminated (it doesn't make sense in a cluster-limited mempool)
- The ancestor and descendant limits are no longer enforced.
- New cluster count/cluster vsize limits are now enforced instead.
- Transaction relay now uses chunk feerate comparisons to determine the order that newly received transactions are announced to peers.
Additionally, the cached ancestor and descendant data are dropped from the mempool, along with the multi_index indices that were maintained to sort the mempool by ancestor and descendant feerates. For compatibility (eg with wallet behavior or RPCs exposing this), this information is now calculated dynamically instead.
ACKs for top commit:
instagibbs:
reACK 17cf9ff7efdbab07644fc2f9017fcac1b0757c38
glozow:
reACK 17cf9ff7efdbab07644fc2f9017fcac1b0757c38
sipa:
ACK 17cf9ff7efdbab07644fc2f9017fcac1b0757c38
Tree-SHA512: bbde46d913d56f8d9c0426cb0a6c4fa80b01b0a4c2299500769921f886082fb4f51f1694e0ee1bc318c52e1976d7ebed8134a64eda0b8044f3a708c04938eee7
Calculating mempool ancestors for a new transaction should not be done until
after cluster size limits have been enforced, to limit CPU DoS potential.
Achieve this by reworking TRUC and RBF validation logic:
- TRUC policy enforcement is now done using only mempool parents of
new transactions, not all mempool ancestors (note that it's fine to calculate
ancestors of in-mempool transactions, if the number of such calls is
reasonably bounded).
- RBF replacement checks are performed earlier (which allows for checking
cluster size limits earlier, because cluster size checks cannot happen until
after all conflicts are staged for removal).
- Verifying that a new transaction doesn't conflict with an ancestor now
happens later, in AcceptSingleTransaction() rather than in PreChecks(). This
means that the test is not performed at all in AcceptMultipleTransactions(),
but in package acceptance we already disallow RBF in situations where a
package transaction has in-mempool parents.
Also to ensure that all RBF validation logic is applied in both the single
transaction and multiple transaction cases, remove the optimization that skips
the PackageMempoolChecks() in the case of a single transaction being validated
in AcceptMultipleTransactions().
