5736d1ddacc4019101e7a5170dd25efbc63b622a tracing: pass if replaced by tx/pkg to tracepoint (0xb10c)
a4ec07f1944999c2eead41d08d7dd4fc3aa71243 doc: add comments for CTxMemPool::ChangeSet (Suhas Daftuar)
83f814b1d1100baac9dca9c176f89b0ec2555dbc Remove m_all_conflicts from SubPackageState (Suhas Daftuar)
d3c8e7dfb63f7986a1f9654ea2393aabe3cd78da Ensure that we don't add duplicate transactions in rbf fuzz tests (Suhas Daftuar)
d7dc9fd2f7bc675256687b9c55fdbec9cc8ac781 Move CalculateChunksForRBF() to the mempool changeset (Suhas Daftuar)
284a1d33f1dcbc3b3404ea40a948ff6600239613 Move prioritisation into changeset (Suhas Daftuar)
446b08b599bc492bbec10ccc2292aee6f90c58e7 Don't distinguish between direct conflicts and all conflicts when doing cluster-size-2-rbf checks (Suhas Daftuar)
b53041021abc4f9ee7203341413e8676e2d5a7ca Duplicate transactions are not permitted within a changeset (Suhas Daftuar)
b447416fddcb8c8647391502cca3dbfd1552e02e Public mempool removal methods Assume() no changeset is outstanding (Suhas Daftuar)
2b30f4d36c86f775ac637b171d27d42a02309c5b Make RemoveStaged() private (Suhas Daftuar)
18829194ca68152ac0b38d34e94b9265ee74c410 Enforce that there is only one changeset at a time (Suhas Daftuar)
7fb62f7db60c7d793828ae45f87bc3f5c63cc989 Apply mempool changeset transactions directly into the mempool (Suhas Daftuar)
34b6c5833d11ea84fbd4b891e06408f6f4ca6fac Clean up FinalizeSubpackage to avoid workspace-specific information (Suhas Daftuar)
57983b8add72a04721d3f2050c063a3c4d8683ed Move LimitMempoolSize to take place outside FinalizeSubpackage (Suhas Daftuar)
01e145b9758f1df14a7ea18058ba9577bf88e459 Move changeset from workspace to subpackage (Suhas Daftuar)
802214c0832de00f24268183f7763fa984ba7903 Introduce mempool changesets (Suhas Daftuar)
87d92fa340195d9c87be3d023ca133b90b3b7d4e test: Add unit test coverage of package rbf + prioritisetransaction (Suhas Daftuar)
15d982f91e6b0f145c9dd4edf29827cfabb37a3f Add package hash to package-rbf log message (Suhas Daftuar)
Pull request description:
part of cluster mempool: #30289
It became clear while working on cluster mempool that it would be helpful for transaction validation if we could consider a full set of proposed changes to the mempool -- consisting of a set of transactions to add, and a set of transactions (ie conflicts) to simultaneously remove -- and perform calculations on what the mempool would look like if the proposed changes were to be applied. Two specific examples of where we'd like to do this:
- Determining if ancestor/descendant/TRUC limits would be violated (in the future, cluster limits) if either a single transaction or a package of transactions were to be accepted
- Determining if an RBF would make the mempool "better", however that idea is defined, both in the single transaction and package of transaction cases
In preparation for cluster mempool, I have pulled this reworking of the mempool interface out of #28676 so it can be reviewed on its own. I have not re-implemented ancestor/descendant limits to be run through the changeset, since with cluster mempool those limits will be going away, so this seems like wasted effort. However, I have rebased #28676 on top of this branch so reviewers can see what the new mempool interface could look like in the cluster mempool setting.
There are some minor behavior changes here, which I believe are inconsequential:
- In the package validation setting, transactions would be added to the mempool before the `ConsensusScriptChecks()` are run. In theory, `ConsensusScriptChecks()` should always pass if the `PolicyScriptChecks()` have passed and it's just a belt-and-suspenders for us, but if somehow they were to diverge then there could be some small behavior change from adding transactions and then removing them, versus never adding them at all.
- The error reporting on `CheckConflictTopology()` has slightly changed due to no longer distinguishing between direct conflicts and indirect conflicts. I believe this should be entirely inconsequential because there shouldn't be a logical difference between those two ideas from the perspective of this function, but I did have to update some error strings in some tests.
- Because, in a package setting, RBFs now happen as part of the entire package being accepted, the logging has changed slightly because we do not know which transaction specifically evicted a given removed transaction.
- Specifically, the "package hash" is now used to reference the set of transactions that are being accepted, rather than any single txid. The log message relating to package RBF that happen in the `TXPACKAGES` category has been updated as well to include the package hash, so that it's possible to see which specific set of transactions are being referenced by that package hash.
- Relatedly, the tracepoint logging in the package rbf case has been updated as well to reference the package hash, rather than a transaction hash.
ACKs for top commit:
naumenkogs:
ACK 5736d1ddac
instagibbs:
ACK 5736d1ddacc4019101e7a5170dd25efbc63b622a
ismaelsadeeq:
reACK 5736d1ddacc4019101e7a5170dd25efbc63b622a
glozow:
ACK 5736d1ddacc
Tree-SHA512: 21810872e082920d337c89ac406085aa71c5f8e5151ab07aedf41e6601f60a909b22fbf462ef3b735d5d5881e9b76142c53957158e674dd5dfe6f6aabbdf630b
Rather than individually calling addUnchecked for each transaction added in a
changeset (after removing all the to-be-removed transactions), instead we can
take advantage of boost::multi_index's splicing features to extract and insert
entries directly from the staging multi_index into mapTx.
This has the immediate advantage of saving allocation overhead for mempool
entries which have already been allocated once. This also means that the memory
locations of mempool entries will not change when transactions go from staging
to the main mempool.
Additionally, eliminate addUnchecked and require all new transactions to enter
the mempool via a CTxMemPoolChangeSet.
The *_RECENT_CONSENSUS_CHANGE variants in the validation result
enumerations were always unused. They seem to have been kept around
speculatively for a soft fork after segwit, however they were never used
for taproot either. This points at them not having a clear purpose.
Based on the original pull requests' comments their usage was never
entirely clear:
https://github.com/bitcoin/bitcoin/pull/11639#issuecomment-370234133https://github.com/bitcoin/bitcoin/pull/15141#discussion_r271039747
Since they are part of the validation interface and need to exposed by
the kernel library keeping them around may also be confusing to future
users of the library.
55eea003af24169c883e1761beb997e151845225 test: Make blockencodings_tests deterministic (AngusP)
4c99301220ab44e98d0d0e1cc8d774d96a25b7aa test: Add ReceiveWithExtraTransactions Compact Block receive test. (AngusP)
4621e7cc8f8e2b71393a2b30d5dbe56165bfb854 test: refactor: Rename extra_txn to const empty_extra_txn as it is empty in all test cases (AngusP)
Pull request description:
This test uses the `extra_txn` (`vExtraTxnForCompact`) vector of optional orphan/conflicted/etc. transactions to provide transactions to a PartiallyDownloadedBlock that are not otherwise present in the mempool, and check that they are used.
This also covers a former nullptr deref bug that was fixed in #29752 (bf031a517c79cec5b43420bcd40291ab0e9f68a8) where the `extra_txn` vec/circular-buffer was null-initialized and not yet filled when dereferenced in `PartiallyDownloadedBlock::InitData`.
ACKs for top commit:
marcofleon:
Code review ACK 55eea003af24169c883e1761beb997e151845225. I ran the `blockencodings` unit test and no issues with the new test case.
dergoegge:
Code review ACK 55eea003af24169c883e1761beb997e151845225
glozow:
ACK 55eea003af24169c883e1761beb997e151845225
Tree-SHA512: d7909c212bb069e1f6184b26390a5000dcc5f2b18e49b86cceccb9f1ec4f874dd43bc9bc92abd4207c71dd78112ba58400042c230c42e93afe55ba51b943262c
refactor: CBlockHeaderAndShortTxIDs constructor now always takes an explicit nonce.
test: Make blockencodings_tests deterministic using fixed seed providing deterministic
CBlockHeaderAndShortTxID nonces and dummy transaction IDs.
Fixes very rare flaky test failures, where the ShortIDs of test transactions collide, leading to
`READ_STATUS_FAILED` from PartiallyDownloadedBlock::InitData and/or `IsTxAvailable` giving `false`
when the transaction should actually be available.
* Use a new `FastRandomContext` with a fixed seed in each test, to ensure 'random' uint256s
used as fake prevouts are deterministic, so in-turn test txids and short IDs are deterministic
and don't collide causing very rare but flaky test failures.
* Add new test-only/internal initializer for `CBlockHeaderAndShortTxIDs` that takes a specified
nonce to further ensure determinism and avoid rare but undesireable short ID collisions.
In a test context this nonce is set to a fixed known-good value. Normally it is random, as
previously.
Flaky test failures can be reproduced with:
```patch
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index 695e8d806a..64d635a97a 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -44,7 +44,8 @@ void CBlockHeaderAndShortTxIDs::FillShortTxIDSelector() const {
uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const Wtxid& wtxid) const {
static_assert(SHORTTXIDS_LENGTH == 6, "shorttxids calculation assumes 6-byte shorttxids");
- return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0xffffffffffffL;
+ // return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0xffffffffffffL;
+ return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0x0f;
}
```
to increase the likelihood of a short ID collision; and running
```shell
set -e;
n=0;
while (( n++ < 5000 )); do
src/test/test_bitcoin --run_test=blockencodings_tests;
done
```
All `CTransactionRef` have `.GetWitnessHash()` that returns a cached `const Wtxid` (since fac1223a568fa1ad6dd602350598eed278d115e8),
so we don't need to pass transaction refs around with their IDs as they're easy to get from a ref.
