aboutsummaryrefslogtreecommitdiff
path: root/src/validation.cpp
AgeCommit message (Collapse)Author
2023-11-13Merge bitcoin/bitcoin#28391: refactor: Simplify CTxMempool/BlockAssembler ↵fanquake
fields, remove some external mapTx access 4dd94ca18f6fc843137ffca3e6d3e97e4f19377b [refactor] remove access to mapTx in validation_block_tests (TheCharlatan) d0cd2e804ec9b278ed9699c2ae48574b1c1613b1 [refactor] rewrite BlockAssembler inBlock and failedTx as sets of txids (glozow) 55b0939cab49d50ca5bc59105b669e379d5e7f6c scripted-diff: rename vTxHashes to txns_randomized (TheCharlatan) a03aef9cec35b0d03aa63d7e8093f0420cd4b40b [refactor] rewrite vTxHashes as a vector of CTransactionRef (glozow) 938643c3b2b8e7b9aec1df34a2f8a95d616d8dd5 [refactor] remove access to mapTx in validation.cpp (glozow) 333367a9407701b5077e2457b1a6aa8ff5e4934b [txmempool] make CTxMemPoolEntry::lockPoints mutable (glozow) 1bf4855016e777dd8b424fe01750f9e3e97931a2 [refactor] use CheckPackageLimits for checkChainLimits (glozow) dbc5bdbf595e9dd0330493645ebff0b8696192a3 [refactor] remove access to mapTx.find in mempool_tests.cpp (glozow) f80909e7a31523d8f197fd650b138b9f228cd13f [refactor] remove access to mapTx in blockencodings_tests.cpp (glozow) 8892d6b744e3cbda2cf93721f573ffa7017bd898 [refactor] remove access to mapTx from rpc/mempool.cpp (glozow) fad61aa56189f98d97af1d6f70c4eb46b8f98bf0 [refactor] get wtxid from entry instead of vTxHashes (glozow) 9cd8cafb77563243af19c95e396c2fb4fc3758df [refactor] use exists() instead of mapTx.find() (glozow) 14804699e59794e61dcfb02ff1971db96e9a06ce [refactor] remove access to mapTx from policy/rbf.cpp (glozow) 1c6a73abbd1fb773c7d0036beb952b95dde8e38b [refactor] Add helper for retrieving mempool entry (TheCharlatan) 453b4813ebc74859864803e9972b58e4be76a4d6 [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 4dd94ca18f6fc843137ffca3e6d3e97e4f19377b 👝 stickies-v: re-ACK 4dd94ca18f6fc843137ffca3e6d3e97e4f19377b Tree-SHA512: c4d043f2186e4fde337591883fac66cade3058173987b49502bd65cecf69207a3df1077f6626809652ab63230013167b7f39a2b39f1c5166959e5495df57065f
2023-11-10[refactor] remove access to mapTx in validation.cppglozow
2023-11-10[refactor] use exists() instead of mapTx.find()glozow
2023-11-10[refactor] Add helper for retrieving mempool entryTheCharlatan
In places where the iterator is only needed for accessing the actual entry, it should not be required to first retrieve the iterator.
2023-11-07[validation] change package-fee-too-low, return wtxid(s) and effective feerateglozow
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.
2023-11-06[validation] add TxValidationResult::TX_RECONSIDERABLE, TX_UNKNOWNglozow
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.
2023-11-06[refactor] use Wtxid for m_wtxids_fee_calculationsglozow
2023-11-04snapshots: don't core dump when running -checkblockindex after `loadtxoutset`Mark Friedenbach
2023-11-03Merge bitcoin/bitcoin#28758: refactors for subpackage evaluationfanquake
b5a60abe8783852f5b31bc1e63b5836530410e65 MOVEONLY: CleanupTemporaryCoins into its own function (glozow) 10c0a8678cd28e7f0715e6cfa3e651903e4ad4aa [test util] CreateValidTransaction multi-in/out, configurable feerate, signal BIP125 (glozow) 6ff647a7e0d85040a6033047c5cf84f8f22b1c65 scripted-diff: rename CheckPackage to IsWellFormedPackage (glozow) da9aceba217bbded6909f06144eaa1e1a4ebcb69 [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 b5a60abe8783852f5b31bc1e63b5836530410e65 instagibbs: ACK b5a60abe8783852f5b31bc1e63b5836530410e65 Tree-SHA512: 39d67a5f0041e381f0d0f802a98ccffbff11e44daa3a49611189d6306b03f18613d5ff16c618898d490c97a216753e99e0db231ff14d327f92c17ae4d269cfec
2023-11-02Merge bitcoin/bitcoin#28530: tests, bug fix: DisconnectedBlockTransactions ↵glozow
rewrite followups 9b3da70bd06b45482e7211aa95637a72bd115553 [test] DisconnectedBlockTransactions::DynamicMemoryUsage (glozow) b2d04479647af64ad7cf5ebfb6175251b2f6b72e bugfix: correct DisconnectedBlockTransactions memory usage (stickies-v) f4254e209801d6a790b5f0c251c0b32154a4e3cc assume duplicate transactions are not added to `iters_by_txid` (ismaelsadeeq) 29eb219c1247993378fce06c8f71aab20736c237 move only: move implementation code to disconnected_transactions.cpp (ismaelsadeeq) 81dfeddea70ae5feeaf79062585c2ff9f33c0ca3 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)`](https://github.com/bitcoin/bitcoin/blob/6e721c923c87abdb8d99674093d08d8c17bc52c2/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)`](https://github.com/bitcoin/bitcoin/blob/6e721c923c87abdb8d99674093d08d8c17bc52c2/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 9b3da70bd06b45482e7211aa95637a72bd115553 - nice work! BrandonOdiwuor: re ACK 9b3da70bd06b45482e7211aa95637a72bd115553 glozow: ACK 9b3da70bd06b45482e7211aa95637a72bd115553 Tree-SHA512: 69b9595d09f4d0209038f97081d790cea92ccf63efb94e9e372749979fcbe527f7f17a8e454720cedd12021be0c8e11cf99874625d3dafd9ec602b12dbeb4098
2023-11-01MOVEONLY: CleanupTemporaryCoins into its own functionglozow
Avoid duplicate code. This will be used at the end of every AcceptSubPackage and after PreChecks loop in AcceptPackage.
2023-11-01scripted-diff: rename CheckPackage to IsWellFormedPackageglozow
-BEGIN VERIFY SCRIPT- sed -i 's/CheckPackage(/IsWellFormedPackage(/g' $(git grep -l CheckPackage) -END VERIFY SCRIPT-
2023-11-01[refactor] move package checks into helper functionsglozow
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.
2023-11-01refactor: Remove unused circular include dependency from validation.cppMarcoFalke
2023-10-30Merge bitcoin/bitcoin#28737: doc: Fix bugprone-lambda-function-name errorsfanquake
faa769db5a4c16fd171e9a39c33e245db4e7c134 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 faa769db5a4c16fd171e9a39c33e245db4e7c134 darosior: utACK faa769db5a4c16fd171e9a39c33e245db4e7c134 Tree-SHA512: 0b562bd4ebd7f46ca3ebabeee67851ad30bd522fa57e5010e833b163664e51f5df645ff9ca35d22c3479fb27d9267d4e5d0d417d42729bf3ccf80d7944970e4e
2023-10-26Merge bitcoin/bitcoin#28107: util: Type-safe transaction identifiersAndrew Chow
940a49978c70453e1aaf2c4a0bcb382872b844a5 Use type-safe txid types in orphanage (dergoegge) ed70e6501648466b9ca91a39b83775363e9a726d Introduce types for txids & wtxids (dergoegge) cdb14d79e809bf7d1612b21b554a9fcfb2ab1c91 [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 940a49978c70453e1aaf2c4a0bcb382872b844a5 stickies-v: ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5 hebasto: ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5, I have reviewed the code and it looks OK. instagibbs: re-ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5 BrandonOdiwuor: re-ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5 glozow: reACK 940a49978c Tree-SHA512: 55298d1c2bb82b7a6995e96e554571c22eaf4a89fb2a4d7a236d70e0f625e8cca62ff2490e1c179c47bd93153fe6527b56870198f026f5ee7753d64d7a424c92
2023-10-26Fix bugprone-lambda-function-name errorsMarcoFalke
Can be reviewed with --color-moved=dimmed-zebra
2023-10-20assumeutxo: Check deserialized coins for out of range valuesFabian Jahr
2023-10-20Merge bitcoin/bitcoin#28569: log: Don't log cache rebalancing in absense of ↵fanquake
a snapshot chainstate ec84f999f1408b7f1ff4498f78c33b34c30e934c 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 ec84f999f1408b7f1ff4498f78c33b34c30e934c glozow: concept ACK ec84f999f1408b7f1ff4498f78c33b34c30e934c, don't have opinions other than removing confusing log theStack: utACK ec84f999f1408b7f1ff4498f78c33b34c30e934c Tree-SHA512: 30bbfc648e7c788106f78d52e47a3aa1e1874f65d13743643dc50bcf7f450d8330711ff9fdeac361722542da6051533153829c6d49033227ed315e111afc899f
2023-10-20log: Don't log cache rebalancing in absense of a snapshot chainstateFabian Jahr
2023-10-13refactor: update `MAX_DISCONNECTED_TX_POOL` from kb to bytesismaelsadeeq
2023-10-12Introduce types for txids & wtxidsdergoegge
2023-10-12tidy: modernize-use-emplaceMarcoFalke
2023-10-06chain: Rename HaveTxsDownloaded to HaveNumChainTxsFabian Jahr
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
2023-10-06validation, test: Improve and document nChainTx check for testabilityFabian Jahr
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
2023-10-06doc: Add and edit some comments around assumeutxoFabian Jahr
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
2023-10-06validation: remove unused mempool param in DetectSnapshotChainstateFabian Jahr
2023-10-05Merge bitcoin/bitcoin#27609: rpc: allow submitpackage to be called outside ↵Andrew Chow
of regtest 5b878be742dbfcd232d949d2df1fff4743aec3d8 [doc] add release note for submitpackage (glozow) 7a9bb2a2a59ba49f80519c8435229abec2432486 [rpc] allow submitpackage to be called outside of regtest (glozow) 5b9087a9a7da2602485e85e0b163dc3cbd2daf31 [rpc] require package to be a tree in submitpackage (glozow) e32ba1599c599e75b1da3393f71f633de860505f [txpackages] IsChildWithParentsTree() (glozow) b4f28cc345ef9c5261c4a8d743654a44784c7802 [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 5b878be742dbfcd232d949d2df1fff4743aec3d8 achow101: ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8 dergoegge: Code review ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8 ajtowns: ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8 ariard: Code Review ACK 5b878be742. Though didn’t manually test the PR. Tree-SHA512: 610365c0b2ffcccd55dedd1151879c82de1027e3319712bcb11d54f2467afaae4d05dca5f4b25f03354c80845fef538d3938b958174dda8b14c10670537a6524
2023-10-04Remove duplicate NDEBUG check from compat/assumptions.hMarcoFalke
The check is already done in util/check.h, which is more widely included.
2023-10-03refactor: Make `CCheckQueue` constructor start worker threadsHennadii Stepanov
2023-10-03Make `CCheckQueue` destructor stop worker threadsHennadii Stepanov
2023-10-03Move global `scriptcheckqueue` into `ChainstateManager` classHennadii Stepanov
2023-10-02[doc] parent pay for child in aggregate CheckFeeRateglozow
2023-09-30refuse to activate a UTXO snapshot if mempool not emptyJames O'Beirne
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`.
2023-09-30validation: do not activate snapshot if behind active chainJames O'Beirne
Most easily reviewed with git show --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
2023-09-30validation: assumeutxo: swap m_mempool on snapshot activationJames O'Beirne
Otherwise we will not receive transactions during background sync until restart.
2023-09-30blockstorage: segment normal/assumedvalid blockfilesJames O'Beirne
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.
2023-09-30validation: populate nChainTx value for assumedvalid chainstatesJames O'Beirne
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.
2023-09-30test: adjust chainstate tests to use recognized snapshot baseJames O'Beirne
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>
2023-09-30validation: pruning for multiple chainstatesJames O'Beirne
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`.
2023-09-30validation: indexing changes for assumeutxoJames O'Beirne
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.
2023-09-30validation: pass ChainstateRole for validationinterface callsJames O'Beirne
This allows consumers to decide how to handle events from background or assumedvalid chainstates.
2023-09-30validation: only call UpdatedBlockTip for active chainstateJames O'Beirne
This notification isn't needed for background chainstates. `kernel::Notifications::blockTip` are also skipped.
2023-09-30validation: add ChainstateRoleJames O'Beirne
2023-09-30validation: MaybeRebalanceCaches when chain leaves IBDJames O'Beirne
Check to see if we need to rebalance caches across chainstates when a chain leaves IBD.
2023-09-30chainparams: add blockhash to AssumeutxoDataJames O'Beirne
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.
2023-09-30assumeutxo: remove snapshot during -reindex{-chainstate}James O'Beirne
Removing a snapshot chainstate from disk (and memory) is consistent with existing reindex operations.
2023-09-30bugfix: correct is_snapshot_cs in VerifyDBJames O'Beirne
2023-09-30net_processing: Request assumeutxo background chain blocksSuhas Daftuar
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>
2023-09-29Merge bitcoin/bitcoin#27866: blockstorage: Return on fatal flush errorsRyan Ofsky
d8041d4e042957660827313951b18c8dd9a99a16 blockstorage: Return on fatal undo file flush error (TheCharlatan) f0207e00303a1030eca795ede231e3c0d94df061 blockstorage: Return on fatal block file flush error (TheCharlatan) 5671c15f4520c6dc20e0805fd0b06157ff94bcd7 blockstorage: Mark FindBlockPos as nodiscard (TheCharlatan) Pull request description: The goal of this PR is to establish that fatal blockstorage flush errors should be treated as errors at their call site. Prior to this patch `FlushBlockFile` may have failed without returning in `Chainstate::FlushStateToDisk`, leading to a potential write from `WriteBlockIndexDB` that may refer to a block that is not fully flushed to disk yet. By returning if either `FlushUndoFile` or `FlushBlockFile` fail, we ensure that no further write operations take place that may lead to an inconsistent database when crashing. Add `[[nodiscard]]` annotations to them such that they are not ignored in future. Functions that call either `FlushUndoFile` or `FlushBlockFile`, need to handle these extra abort cases properly. Since `Chainstate::FlushStateToDisk` already produces an abort error in case of `WriteBlockIndexDB` failing, no extra logic for functions calling `Chainstate::FlushStateToDisk` is required. Besides `Chainstate::FlushStateToDisk`, `FlushBlockFile` is also called by `FindBlockPos`, while `FlushUndoFile` is only called by `FlushBlockFile` and `WriteUndoDataForBlock`. For both these cases, the flush error is not further bubbled up. Instead, the error is logged and a comment is provided why bubbling up an error would be less desirable in these cases. --- This pull request is part of a larger effort towards improving the shutdown / abort / fatal error handling in validation code. It is a first step towards implementing proper fatal error return type enforcement similar as proposed by theuni in this pull request [comment](https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1563561502). For ease of review of these critical changes, a first step would be checking that `AbortNode` leads to early and error-conveying returns at its call site. Further work for enforcing returns when `AbortNode` is called is done in https://github.com/bitcoin/bitcoin/pull/27862. ACKs for top commit: stickies-v: re-ACK d8041d4 ryanofsky: Code review ACK d8041d4e042957660827313951b18c8dd9a99a16 Tree-SHA512: 47ade9b873b15e567c8f60ca538d5a0daf32163e1031be3212a3a45eb492b866664b225f2787c9e40f3e0c089140157d8fd1039abc00c7bdfeec1b52ecd7e219
generated by cgit v1.2.3 (git 2.26.2) at 2025-03-19 10:02:54 +0000