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
a0efe529e4 Fix outdated comments referring to ::ChainActive() (Samuel Dobson)
Pull request description:
After #21866 there are a few outdated comments referring to `::ChainActive()`, which should instead refer to `ChainstateManager::ActiveChain()`.
ACKs for top commit:
jamesob:
ACK a0efe529e4
Tree-SHA512: 80da19c105ed29ac247e6df4c8e916c3bf3f37230b63f07302114eef9c115add673e9649f0bbe237295be0c6da7b1030b5b93e14daf6768f17ce5de7cf2c9ff2
The tracepoint `validation:block_connected` was introduced in #22006.
The first argument was the hash of the connected block as a pointer
to a C-like String. The last argument passed the hash of the
connected block as a pointer to 32 bytes. The hash was only passed as
string to allow `bpftrace` scripts to print the hash. It was
(incorrectly) assumed that `bpftrace` cannot hex-format and print the
block hash given only the hash as bytes.
The block hash can be printed in `bpftrace` by calling
`printf("%02x")` for each byte of the hash in an `unroll () {...}`.
By starting from the last byte of the hash, it can be printed in
big-endian (the block-explorer format).
```C
$p = $hash + 31;
unroll(32) {
$b = *(uint8*)$p;
printf("%02x", $b);
$p -= 1;
}
```
See also: https://github.com/bitcoin/bitcoin/pull/22902#discussion_r705176691
This is a breaking change to the block_connected tracepoint API, however
this tracepoint has not yet been included in a release.
3b613722f6 Add release notes for fee est with replacement txs (Antoine Poinsot)
4556406562 qa: test fee estimation with replacement transactions (Antoine Poinsot)
053415b297 qa: split run_test into smaller parts (Antoine Poinsot)
06c5ce9714 Re-include RBF replacement txs in fee estimation (Antoine Poinsot)
Pull request description:
This effectively reverts #9519.
RBF is now largely in use on the network (signaled for by around 20% of
all transactions on average) and replacement logic is implemented in
most end-user wallets. The rate of replaced transactions is also
expected to rise as fee-bumping techniques are being developed for
pre-signed transaction ("L2") protocols.
ACKs for top commit:
prayank23:
reACK 3b613722f6
Zero-1729:
re-ACK 3b613722f6
benthecarman:
reACK 3b613722f6
glozow:
ACK 3b613722f6
theStack:
re-ACK 3b613722f6🍪
Tree-SHA512: a6146d15c80ff4ba9249314b0ef953a66a15673e61b8f98979642814f1b169b5695e330e3ee069fa9a7e4d1f8aa10e1dcb7f9aa79181cea5a4c4dbcaf5483023
UpdateCoins is an unnecessary dependency on validation. All we need to
do is add and remove coins to check inputs. We don't need the extra
logic for checking coinbases and handling TxUndos.
Also remove the wrapper function in validation.h which constructs a
throwaway TxUndo object before calling UpdateCoins because it is now
unused.
This effectively reverts de1ae324bf.
RBF is now largely in use on the network (signaled for by around 20% of
all transactions on average) and replacement logic is implemented in
most end-user wallets. The rate of replaced transactions is also
expected to rise as fee-bumping techniques are being developed for
pre-signed transaction ("L2") protocols.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Improve readability of code, simplify future scripted diff cleanup PRs, and be
more consistent with naming for GetBoolArg.
This will also be useful for replacing runtime settings type checking
with compile time checking.
-BEGIN VERIFY SCRIPT-
git grep -l GetArg | xargs sed -i 's/GetArg(\([^)]*\( [0-9]\+\|-1\|port\|BaseParams().RPCPort()\|Params().GetDefaultPort()\|_TIMEOUT\|Height\|_WORKQUEUE\|_THREADS\|_CONNECTIONS\|LIMIT\|SigOp\|Bytes\|_VERSION\|_AGE\|_CHECKS\|Checks() ? 1 : 0\|_BANTIME\|Cache\|BLOCKS\|LEVEL\|Weight\|Version\|BUFFER\|TARGET\|WEIGHT\|TXN\|TRANSACTIONS\|ADJUSTMENT\|i64\|Size\|nDefault\|_EXPIRY\|HEIGHT\|SIZE\|SNDHWM\|_TIME_MS\)\))/GetIntArg(\1)/g'
-END VERIFY SCRIPT-
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
faa9c19a4b doc: Add 23061 release notes (MarcoFalke)
faff17bbde Fix (inverse) meaning of -persistmempool (MarcoFalke)
Pull request description:
Passing `-persistmempool` is currently treated as `-nopersistmempool`
ACKs for top commit:
jnewbery:
reACK faa9c19a4b
hebasto:
ACK faa9c19a4b, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: f34a89a07745dabe340eb845b2a348b79c093e9056f7a21c17e1ba2e278177c9b4cf30e8095791fd645a7f90eb34850b2eee0c869b4f6ec02bf749c73b0e52ee
673a5bd337 test: validation: add unittest for UpdateTip behavior (James O'Beirne)
2705570109 test: refactor: separate CreateBlock in TestChain100Setup (James O'Beirne)
298bf5d563 test: refactor: declare NoMalleation const auto (James O'Beirne)
071200993f move-only: unittest: add test/util/chainstate.h (James O'Beirne)
8f5710fd0a validation: fix CheckBlockIndex for multiple chainstates (James O'Beirne)
5a807736da validation: insert assumed-valid block index entries into candidates (James O'Beirne)
01a9b8fe71 validation: set BLOCK_ASSUMED_VALID during snapshot load (James O'Beirne)
42b2520db9 chain: add BLOCK_ASSUMED_VALID for use with assumeutxo (James O'Beirne)
b217020df7 validation: change UpdateTip for multiple chainstates (James O'Beirne)
665072a36d doc: add comment for g_best_block (James O'Beirne)
ac4051d891 refactor: remove unused assumeutxo methods (James O'Beirne)
9f6bb53935 validation: add chainman ref to CChainState (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
Modify UpdateTip and CheckBlockIndex for use with multiple chainstates. Includes a new unittest verifying `g_best_block` behavior (previously untested at the unit level) and various changes necessary for running and testing `ProcessNewBlock()`-like behavior on the background validation chainstate.
This changeset introduces a new block index `nStatus` flag called `BLOCK_ASSUMED_VALID`, and it is applied to block index entries that are beneath the UTXO snapshot base block upon snapshot load. Once each block is validated (during async background validation), the flag is removed. This allows us to avoid (ab)using `BLOCK_VALID_*` flags for snapshot chain block entries, and preserves the original meaning of those flags.
Note: this PR previously incorporated changes to `LoadBlockIndex()` and `RewindBlockIndex()` as noted in Russ' comments below, but once I generated the changes necessary to test the UpdateTip change, I decided to split this changes out into another PR due to the size of this one.
ACKs for top commit:
achow101:
ACK 673a5bd337
jonatack:
Code-review re-ACK 673a5bd337 reviewed diff, rebased to master/debug build/ran unit+functional tests
naumenkogs:
ACK 673a5bd337
fjahr:
Code review ACK 673a5bd337
ariard:
utACK 673a5bd3
ryanofsky:
Code review ACK 673a5bd337. Just linker fix and split commit changes mentioned https://github.com/bitcoin/bitcoin/pull/21526#issuecomment-921064563 since last review
benthecarman:
ACK 673a5bd337
Tree-SHA512: 0a6dc23d041b27ed9fd0ee1f3e5971b92fb1d2df2fc9b655d5dc48594235321ab1798d06de2ec55482ac3966a9ed56de8d56e9e29cae75bbe8690bafc2dda383
ab27800799 log: Remove unnecessary timing logs for Callbacks bench (Douglas Chimento)
Pull request description:
Logging of Callbacks are no longer needed and records times that are not relevant for performance analysis.
resolves#23071
ACKs for top commit:
laanwj:
Thanks. re-ACK ab27800799
jonatack:
Code review ACK ab27800799
Tree-SHA512: be1ea780c4db9407a8799065a8824b9d3610abac72af5907809ed62d493d5a54e65735de45ec5fdd0edb85ef21ec6036105abe8ca00093942980f6f92e7fec50
0ef08f8bed add missing includes in policy/rbf (glozow)
c6abeb76fb make MAX_BIP125_RBF_SEQUENCE constexpr (glozow)
3cf46f6055 [doc] improve RBF documentation (glozow)
c78eb8651b [policy/refactor] pass in relay fee instead of using global (glozow)
Pull request description:
Followups to #22675 and documentation-only changes intended to clarify the code/logic concerning mempool Replace-by-Fee.
ACKs for top commit:
jnewbery:
utACK 0ef08f8bed
fanquake:
ACK 0ef08f8bed
Tree-SHA512: 6797ae758beca0c9673cb00ce85da48e9a4ac5cb5100074ca93e004cdb31d24d91a1a7721b57fc2f619addfeb4950d8caf45fee0f5b7528defbbd121eb4d271f
fa20f815a9 Remove txindex migration code (MarcoFalke)
fae8786033 doc: Fix validation typo (MarcoFalke)
fab89006d6 Add missing includes and forward declarations, remove unused ones (MarcoFalke)
Pull request description:
No supported version of Bitcoin Core used the legacy txindex, so all relevant nodes can be assumed to have upgraded. Thus, there is no need to keep this code any longer.
As a temporary courtesy, provide a one-time warning on how to free the disk space used by the legacy txindex.
Fixes#22615
ACKs for top commit:
laanwj:
Code review ACK fa20f815a9
hebasto:
ACK fa20f815a9, tested on Linux Mint 20.2 (x86_64).
Zero-1729:
crACK fa20f815a9
theStack:
Approach ACK fa20f815a9
Tree-SHA512: 68aa32d064d1e3932e6e382816a4b5de417bd7e82861fea1ee50660e8c397f4efeb88ae4ed54a8ad1952c3563eb0b8449d7ccf883c353cc4d4dc7e15c53d78e8
Adjust CheckBlockIndex to account for
- assumed-valid block indexes lacking transaction data, and
- setBlockIndexCandidates for the background chainstate not containing certain entries
which rely on assumed-valid ancestors.
Mark the block index entries that are beneath the snapshot base block as
assumed-valid. Subsequent commits will make use of this flag in other
parts of the system.
Only perform certain behavior (namely that related to servicing
the getblocktemplate RPC call) for the active chainstate when
calling UpdateTip.
Co-authored-by: Jon Atack <jon@atack.com>
32748da0f4 whitespace fixups after move and scripted-diff (glozow)
fa47622e8d scripted-diff: rename variables in policy/rbf (glozow)
ac761f0a23 MOVEONLY: fee checks (Rules 3 and 4) to policy/rbf (glozow)
9c2f9f8984 MOVEONLY: check that fees > direct conflicts to policy/rbf (glozow)
3f033f01a6 MOVEONLY: check for disjoint conflicts and ancestors to policy/rbf (glozow)
7b60c02b7d MOVEONLY: BIP125 Rule 2 to policy/rbf (glozow)
f8ad2a57c6 Make GetEntriesForConflicts return std::optional (glozow)
Pull request description:
This PR does not change behavior. It extracts the BIP125 logic into helper functions (and puts them in the policy/rbf* files). This enables three things - I think each one individually is pretty good:
- Implementation of package RBF (see #22290). I want it to be as close to BIP125 as possible so that it doesn't become a distinct fee-bumping mechanism. Doing these move-only commits first means the diff is mostly mechanical to review, and I just need to create a function that mirrors the single transaction validation.
- We will be able to isolate and test our RBF logic alone. Recently, there have been some discussions on discrepancies between our code and BIP125, as well as proposals for improving it. Generally, I think making this code more modular and de-bloating validation.cpp is probably a good idea.
- Witness Replacement (replacing same-txid-different-wtxid when the witness is significantly smaller and therefore higher feerate) in a BIP125-similar way. Hopefully it can just be implemented with calls to the rbf functions (i.e. `PaysForRBF`) and an edit to the relevant mempool entries.
ACKs for top commit:
mjdietzx:
ACK 32748da0f4
theStack:
Code-review ACK 32748da0f4📐
MarcoFalke:
review ACK 32748da0f4🦇
Tree-SHA512: d89985c8b4b42b54861018deb89468e04968c85a3fb1113bbcb2eb2609577bc4fd9bf254593b5bd0e7ab059a0fa8192d1a903b00f77e6f120c7a80488ffcbfc0
f293c68be0 MOVEONLY: getting mempool conflicts to policy/rbf (glozow)
8d71796335 [validation] quit RBF logic earlier and separate loops (glozow)
badb9b11a6 call SignalsOptInRBF instead of checking all inputs (glozow)
e0df41d7d5 [validation] default conflicting fees and size to 0 (glozow)
b001b9f6de MOVEONLY: BIP125 max conflicts limit to policy/rbf.h (glozow)
Pull request description:
See #22675 for motivation, this is one chunk of it. It extracts some BIP125 logic into policy/rbf:
- Defines a constant for specifying the maximum number of mempool entries we'd consider replacing by RBF
- Calls the available `SignalsOptInRBF` function instead of manually iterating through inputs
- Moves the logic for getting the list of conflicting mempool entries to a helper function
- Also does a bit of preparation for future moves - moving declarations around, etc
Also see #22677 for addressing the circular dependency.
ACKs for top commit:
jnewbery:
Code review ACK f293c68be0
theStack:
Code-review ACK f293c68be0📔
ariard:
ACK f293c68b
Tree-SHA512: a60370994569cfc91d4b2ad5e94542d4855a48927ae8b174880216074e4fa50d4523dd4ee36efdd6edf2bf7adb87a8beff9c3aaaf6dd323b286b287233e63790
