fa1a384706 Move compat.h include from system.h to system.cpp (MarcoFalke)
88887531b7 Move compat/assumptions.h include to one place that actually needs it (MarcoFalke)
77774110f4 Remove __cplusplus from compat/assumptions.h (MarcoFalke)
faa3d4f1d8 Remove duplicate NDEBUG check from compat/assumptions.h (MarcoFalke)
Pull request description:
Generally, compile-time checks should be close to the code that use them. Especially, since `compat/assumptions.h` is only included in one place, where iwyu suggests to remove it.
Fix all issues:
* The `NDEBUG` check is used in `util/check`, so it is redundant in `compat/assumptions.h`.
* The `__cplusplus` check is redundant with `doc/dependencies.md` (see commit message).
* Add missing `// IWYU pragma: keep` to avoid removing the include by accident.
ACKs for top commit:
achow101:
ACK fa1a384706
TheCharlatan:
re-ACK fa1a384706
theuni:
ACK fa1a384706
Tree-SHA512: f8b6db84be5d8844a2267345c0b1405fcbc39b8b5eeaa24db5b8412a74145fe44cf188b6b0c39cc2b062690ed37ca5b4662473484afe28dbec6469e79961389b
9e58c5bcd9 Use Txid in COutpoint (dergoegge)
Pull request description:
This PR changes the type of the hash of a transaction outpoint from `uint256` to `Txid`.
ACKs for top commit:
Sjors:
ACK 9e58c5bcd9
stickies-v:
ACK 9e58c5bcd9. A sizeable diff, but very straightforward changes. Didn't see anything controversial. Left a few nits, but nothing blocking, only if you have to retouch.
TheCharlatan:
ACK 9e58c5bcd9
Tree-SHA512: 58f61ce1c58668f689513e62072a7775419c4d5af8f607669cd8cdc2e7be9645ba14af7f9e2d65da2670da3ec1ce7fc2a744037520caf799aba212fd1ac44b34
4dd94ca18f [refactor] remove access to mapTx in validation_block_tests (TheCharlatan)
d0cd2e804e [refactor] rewrite BlockAssembler inBlock and failedTx as sets of txids (glozow)
55b0939cab scripted-diff: rename vTxHashes to txns_randomized (TheCharlatan)
a03aef9cec [refactor] rewrite vTxHashes as a vector of CTransactionRef (glozow)
938643c3b2 [refactor] remove access to mapTx in validation.cpp (glozow)
333367a940 [txmempool] make CTxMemPoolEntry::lockPoints mutable (glozow)
1bf4855016 [refactor] use CheckPackageLimits for checkChainLimits (glozow)
dbc5bdbf59 [refactor] remove access to mapTx.find in mempool_tests.cpp (glozow)
f80909e7a3 [refactor] remove access to mapTx in blockencodings_tests.cpp (glozow)
8892d6b744 [refactor] remove access to mapTx from rpc/mempool.cpp (glozow)
fad61aa561 [refactor] get wtxid from entry instead of vTxHashes (glozow)
9cd8cafb77 [refactor] use exists() instead of mapTx.find() (glozow)
14804699e5 [refactor] remove access to mapTx from policy/rbf.cpp (glozow)
1c6a73abbd [refactor] Add helper for retrieving mempool entry (TheCharlatan)
453b4813eb [refactor] Add helper for iterating through mempool entries (stickies-v)
Pull request description:
Motivation
* It seems preferable to use stdlib data structures instead of boost if they can achieve close to the same thing.
* Code external to mempool should ideally use its public helper methods instead of accessing `mapTx` or its iterators directly.
* Reduce the number of complex boost multi index type interactions
* Also see #28335 for further context/motivation. This PR together with #28385 simplifies that one.
Overview of things done in this PR:
* Make `vTxHashes` a vector of transaction references instead of a pair of transaction hash and iterator. The trade off here is that the data is retrieved on the fly with `GetEntry` instead of being cached in `vTxHashes`.
* Introduce `GetEntry` helper method to replace the more involved `GetIter` where applicable
* Replace `mapTx` access with `CTxMemPool` helper methods
* Simplify `checkChainLimits` call in `node/interfaces.cpp`
* Make `CTxMemPoolEntry`s `lockPoints`mutable such that they can be changed with a const iterator directly instead of going through `mapTx`
* Make `BlockAssembler`'s `inBlock` and `failedTx` sets of transaction hashes.
ACKs for top commit:
glozow:
reACK 4dd94ca
maflcko:
re-ACK 4dd94ca18f👝
stickies-v:
re-ACK 4dd94ca18f
Tree-SHA512: c4d043f2186e4fde337591883fac66cade3058173987b49502bd65cecf69207a3df1077f6626809652ab63230013167b7f39a2b39f1c5166959e5495df57065f
With subpackage evaluation and de-duplication, it's not always the
entire package that is used in CheckFeerate. To be more helpful to the
caller, specify which transactions were included in the evaluation and
what the feerate was.
Instead of PCKG_POLICY (which is supposed to be for package-wide
errors), use PCKG_TX.
With package validation rules, transactions that fail individually may
sometimes be eligible for reconsideration if submitted as part of a
(different) package. For now, that includes trasactions that failed for
being too low feerate. Add a new TxValidationResult type to distinguish
these failures from others. In the next commits, we will abort package
validation if a tx fails for any other reason. In the future, we will
also decide whether to cache failures in recent_rejects based on this
result (we won't want to reject a package containing a transaction that
was rejected previously for being low feerate).
Package validation also sometimes elects to skip some transactions when
it knows the package will not be submitted in order to quit sooner. Add
a result to specify this situation; we also don't want to cache these
as rejections.
b5a60abe87 MOVEONLY: CleanupTemporaryCoins into its own function (glozow)
10c0a8678c [test util] CreateValidTransaction multi-in/out, configurable feerate, signal BIP125 (glozow)
6ff647a7e0 scripted-diff: rename CheckPackage to IsWellFormedPackage (glozow)
da9aceba21 [refactor] move package checks into helper functions (glozow)
Pull request description:
This is part of #27463. It splits off the more trivial changes from #26711 for ease of review, as requested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253.
- Split package sanitization in policy/packages.h into helper functions
- Add some tests for its quirks (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1340521597)
- Rename `CheckPackage` to `IsPackageWellFormed`
- Improve the `CreateValidTransaction` unit test utility to:
- Configure the target feerate and return the fee paid
- Signal BIP125 on transactions to enable RBF tests
- Allow the specification of multiple inputs and outputs
- Move `CleanupTemporaryCoins` into its own function to be reused later without duplication
ACKs for top commit:
dergoegge:
Code review ACK b5a60abe87
instagibbs:
ACK b5a60abe87
Tree-SHA512: 39d67a5f0041e381f0d0f802a98ccffbff11e44daa3a49611189d6306b03f18613d5ff16c618898d490c97a216753e99e0db231ff14d327f92c17ae4d269cfec
9b3da70bd0 [test] DisconnectedBlockTransactions::DynamicMemoryUsage (glozow)
b2d0447964 bugfix: correct DisconnectedBlockTransactions memory usage (stickies-v)
f4254e2098 assume duplicate transactions are not added to `iters_by_txid` (ismaelsadeeq)
29eb219c12 move only: move implementation code to disconnected_transactions.cpp (ismaelsadeeq)
81dfeddea7 refactor: update `MAX_DISCONNECTED_TX_POOL` from kb to bytes (ismaelsadeeq)
Pull request description:
This PR is a follow-up to fix review comments and a bugfix from #28385
The PR
- Updated `DisconnectedBlockTransactions`'s `MAX_DISCONNECTED_TX_POOL` from kb to bytes.
- Moved `DisconnectedBlockTransactions` implementation code to `kernel/disconnected_transactions.cpp`.
- `AddTransactionsFromBlock` now assume duplicate transactions are not passed by asserting after inserting each transaction to `iters_by_txid`.
- Included a Bug fix: In the current master we are underestimating the memory usage of `DisconnectedBlockTransactions`.
* When adding and subtracting `cachedInnerUsage` we call `RecursiveDynamicUsage` with `CTransaction` which invokes this [`RecursiveDynamicUsage(const CTransaction& tx)`](6e721c923c/src/core_memusage.h (L32)) version of `RecursiveDynamicUsage`, the output of that call only account for the memory usage of the inputs and outputs of the `CTransaction`, this omits the memory usage of the `CTransaction` object and the control block.
* This PR fixes this bug by calling `RecursiveDynamicUsage` with `CTransactionRef` when adding and subtracting `cachedInnerUsage` which invokes [`RecursiveDynamicUsage(const std::shared_ptr<X>& p)`](6e721c923c/src/core_memusage.h (L67)) version of `RecursiveDynamicUsage` the output of the calculation accounts for the` CTransaction` object, the control blocks, inputs and outputs memory usage.
* see [comment ](https://github.com/bitcoin/bitcoin/pull/28385#discussion_r1322948452)
- Added test for DisconnectedBlockTransactions memory limit.
ACKs for top commit:
stickies-v:
ACK 9b3da70bd0 - nice work!
BrandonOdiwuor:
re ACK 9b3da70bd0
glozow:
ACK 9b3da70bd0
Tree-SHA512: 69b9595d09f4d0209038f97081d790cea92ccf63efb94e9e372749979fcbe527f7f17a8e454720cedd12021be0c8e11cf99874625d3dafd9ec602b12dbeb4098
This allows IsSorted() and IsConsistent() to be used by themselves.
IsSorted() with a precomputed set is used so that we don't create this
set multiple times.
faa769db5a Fix bugprone-lambda-function-name errors (MarcoFalke)
Pull request description:
Inside a lambda, `__func__` will evaluate to something like `"operator()"`. Fix this by either removing it, or by using the real name.
https://clang.llvm.org/extra/clang-tidy/checks/bugprone/lambda-function-name.html
ACKs for top commit:
TheCharlatan:
ACK faa769db5a
darosior:
utACK faa769db5a
Tree-SHA512: 0b562bd4ebd7f46ca3ebabeee67851ad30bd522fa57e5010e833b163664e51f5df645ff9ca35d22c3479fb27d9267d4e5d0d417d42729bf3ccf80d7944970e4e
940a49978c Use type-safe txid types in orphanage (dergoegge)
ed70e65016 Introduce types for txids & wtxids (dergoegge)
cdb14d79e8 [net processing] Use HasWitness over comparing (w)txids (dergoegge)
Pull request description:
We currently have two different identifiers for transactions: `txid` (refering to the hash of a transaction without witness data) and `wtxid` (referring to the hash of a transaction including witness data). Both are typed as `uint256` which could lead to type-safety bugs in which one transaction identifier type is passed where the other would be expected.
This PR introduces explicit `Txid` and `Wtxid` types that (if used) would cause compilation errors for such type confusion bugs.
(Only the orphanage is converted to use these types in this PR)
ACKs for top commit:
achow101:
ACK 940a49978c
stickies-v:
ACK 940a49978c
hebasto:
ACK 940a49978c, I have reviewed the code and it looks OK.
instagibbs:
re-ACK 940a49978c
BrandonOdiwuor:
re-ACK 940a49978c
glozow:
reACK 940a49978c
Tree-SHA512: 55298d1c2bb82b7a6995e96e554571c22eaf4a89fb2a4d7a236d70e0f625e8cca62ff2490e1c179c47bd93153fe6527b56870198f026f5ee7753d64d7a424c92
ec84f999f1 log: Don't log cache rebalancing in absense of a snapshot chainstate (Fabian Jahr)
Pull request description:
I have noticed that this log now is always printed, even if there is no snapshot chainstate present or even was present. I think this is confusing to users that have never even thought about using assumeutxo since in that case the rebalancing is just ensuring the normal environment with one chainstate. So I suggest we don't log in absence of a snapshot chainstate. We could also think about rewording the message instead but I think this is simpler.
ACKs for top commit:
stickies-v:
utACK ec84f999f1
glozow:
concept ACK ec84f999f1, don't have opinions other than removing confusing log
theStack:
utACK ec84f999f1
Tree-SHA512: 30bbfc648e7c788106f78d52e47a3aa1e1874f65d13743643dc50bcf7f450d8330711ff9fdeac361722542da6051533153829c6d49033227ed315e111afc899f
5b878be742 [doc] add release note for submitpackage (glozow)
7a9bb2a2a5 [rpc] allow submitpackage to be called outside of regtest (glozow)
5b9087a9a7 [rpc] require package to be a tree in submitpackage (glozow)
e32ba1599c [txpackages] IsChildWithParentsTree() (glozow)
b4f28cc345 [doc] parent pay for child in aggregate CheckFeeRate (glozow)
Pull request description:
Permit (restricted topology) submitpackage RPC outside of regtest. Suggested in https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1510851570
This RPC should be safe but still experimental - interface may change, not all features (e.g. package RBF) are implemented, etc. If a miner wants to expose this to people, they can effectively use "package relay" before the p2p changes are implemented. However, please note **this is not package relay**; transactions submitted this way will not relay to other nodes if the feerates are below their mempool min fee. Users should put this behind some kind of rate limit or permissions.
ACKs for top commit:
instagibbs:
ACK 5b878be742
achow101:
ACK 5b878be742
dergoegge:
Code review ACK 5b878be742
ajtowns:
ACK 5b878be742
ariard:
Code Review ACK 5b878be742. Though didn’t manually test the PR.
Tree-SHA512: 610365c0b2ffcccd55dedd1151879c82de1027e3319712bcb11d54f2467afaae4d05dca5f4b25f03354c80845fef538d3938b958174dda8b14c10670537a6524
This ensures that we avoid any unexpected conditions inherent in
transferring non-empty mempools across chainstates.
Note that this should never happen in practice given that snapshot
activation will not occur outside of IBD, based upon the height checks
in `loadtxoutset`.
When using an assumedvalid (snapshot) chainstate along with a background
chainstate, we are syncing two very different regions of the chain
simultaneously. If we use the same blockfile space for both of these
syncs, wildly different height blocks will be stored alongside one
another, making pruning ineffective.
This change implements a separate blockfile cursor for the assumedvalid
chainstate when one is in use.
Use the expected AssumeutxoData in order to bootstrap nChainTx values
for assumedvalid blockindex entries in the snapshot chainstate. This
is necessary because nChainTx is normally built up from nTx values,
which are populated using blockdata which the snapshot chainstate
does not yet have.
In future commits, loading the block index while making use of a
snapshot is contingent on the snapshot being recognized by chainparams.
Ensure all existing unittests that use snapshots use a recognized
snapshot (at height 110).
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Introduces ChainstateManager::GetPruneRange().
The prune budget is split evenly between the number of chainstates,
however the prune budget may be exceeded if the resulting shares are
beneath `MIN_DISK_SPACE_FOR_BLOCK_FILES`.
When using an assumedvalid chainstate, only process validationinterface
callbacks from the background chainstate within indexes. This ensures
that all indexes are built in-order.
Later, we can possibly designate indexes which can be built out of order
and continue their operation during snapshot use.
Once the background sync has completed, restart the indexes so that
they continue to index the now-validated snapshot chainstate.
This allows us to reference assumeutxo configuration by blockhash as
well as height; this is helpful in future changes when we want to
reference assumeutxo configurations before the block index is loaded.
Add new PeerManagerImpl::TryDownloadingHistoricalBlocks method and use it to
request background chain blocks in addition to blocks normally requested by
FindNextBlocksToDownload.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: James O'Beirne <james.obeirne@gmail.com>
