diff options
author | MarcoFalke <falke.marco@gmail.com> | 2021-07-28 18:19:38 +0200 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2021-07-28 18:19:50 +0200 |
commit | 4b1fb50def0dea0cd320bc43c12d9a12edde0390 (patch) | |
tree | a1f6074da1ec3c297bf380a15982538bd3d51eea | |
parent | 67b9416540566794c39425b38bc83ad138371ddf (diff) | |
parent | f685a13bef0418663015ea6d8f448f075510c0ec (diff) | |
download | bitcoin-4b1fb50def0dea0cd320bc43c12d9a12edde0390.tar.xz |
Merge bitcoin/bitcoin#22528: refactor: move GetTransaction to node/transaction.cpp
f685a13bef0418663015ea6d8f448f075510c0ec doc: GetTransaction()/getrawtransaction follow-ups to #22383 (John Newbery)
abc57e1f0882a1a2bb20474648419979af6e383d refactor: move `GetTransaction(...)` to node/transaction.cpp (Sebastian Falbesoner)
Pull request description:
~This PR is based on #22383, which should be reviewed first~ (merged by now).
In [yesterday's PR review club session to PR 22383](https://bitcoincore.reviews/22383), the idea of moving the function `GetTransaction(...)` from src/validation.cpp to src/node/transaction.cpp came up. With this, the circular dependency "index/txindex -> validation -> index/txindex" is removed (see change in `lint-circular-dependencies.sh`). Thanks to jnewbery for suggesting and to sipa for providing historical background.
Relevant IRC log:
```
17:52 <jnewbery> Was anyone surprised that GetTransaction() is in validation.cpp? It seems to me that node/transaction.cpp would be a more appropriate place for it.
17:53 <raj_> jnewbery, +1
17:53 <stickies-v> agreed!
17:54 <glozow> jnewbery ya
17:54 <jnewbery> seems weird that validation would call into txindex. I wonder if we remove this function, then validation would no longer need to #include txindex
17:54 <sipa> GetTransaction predates node/transaction.cpp, and even the generic index framework itself :)
17:55 <sipa> (before 0.8, validation itself used the txindex)
17:55 <jnewbery> (and GetTransaction() seems like a natural sibling to BroadcastTransaction(), which is already in node/transaction.cpp)
17:55 <jnewbery> sipa: right, this is not meant as a criticism of course. Just wondering if we can organize things a bit more rationally now that we have better separation between things.
17:55 <sipa> jnewbery: sure, just providing background
17:56 <sipa> seems very reasonable to move it elsewhere now
```
The commit should be trivial to review with `--color-moved`.
ACKs for top commit:
jnewbery:
Code review ACK f685a13bef0418663015ea6d8f448f075510c0ec
rajarshimaitra:
tACK https://github.com/bitcoin/bitcoin/pull/22528/commits/f685a13bef0418663015ea6d8f448f075510c0ec
mjdietzx:
crACK f685a13bef0418663015ea6d8f448f075510c0ec
LarryRuane:
Code review, test ACK f685a13bef0418663015ea6d8f448f075510c0ec
Tree-SHA512: 0e844a6ecb1be04c638b55bc4478c2949549a4fcae01c984eee078de74d176fb19d508fc09360a62ad130677bfa7daf703b67870800e55942838d7313246248c
-rw-r--r-- | src/node/transaction.cpp | 38 | ||||
-rw-r--r-- | src/node/transaction.h | 20 | ||||
-rw-r--r-- | src/rpc/rawtransaction.cpp | 4 | ||||
-rw-r--r-- | src/validation.cpp | 33 | ||||
-rw-r--r-- | src/validation.h | 15 | ||||
-rwxr-xr-x | test/lint/lint-circular-dependencies.sh | 1 |
6 files changed, 61 insertions, 50 deletions
diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index d3bce069b0..1861755aff 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -4,9 +4,12 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <consensus/validation.h> +#include <index/txindex.h> #include <net.h> #include <net_processing.h> +#include <node/blockstorage.h> #include <node/context.h> +#include <txmempool.h> #include <validation.h> #include <validationinterface.h> #include <node/transaction.h> @@ -119,3 +122,38 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t return TransactionError::OK; } + +CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock) +{ + LOCK(cs_main); + + if (mempool && !block_index) { + CTransactionRef ptx = mempool->get(hash); + if (ptx) return ptx; + } + if (g_txindex) { + CTransactionRef tx; + uint256 block_hash; + if (g_txindex->FindTx(hash, block_hash, tx)) { + if (!block_index || block_index->GetBlockHash() == block_hash) { + // Don't return the transaction if the provided block hash doesn't match. + // The case where a transaction appears in multiple blocks (e.g. reorgs or + // BIP30) is handled by the block lookup below. + hashBlock = block_hash; + return tx; + } + } + } + if (block_index) { + CBlock block; + if (ReadBlockFromDisk(block, block_index, consensusParams)) { + for (const auto& tx : block.vtx) { + if (tx->GetHash() == hash) { + hashBlock = block_index->GetBlockHash(); + return tx; + } + } + } + } + return nullptr; +} diff --git a/src/node/transaction.h b/src/node/transaction.h index 0c016ff04e..aed519cf7f 100644 --- a/src/node/transaction.h +++ b/src/node/transaction.h @@ -10,7 +10,12 @@ #include <primitives/transaction.h> #include <util/error.h> +class CBlockIndex; +class CTxMemPool; struct NodeContext; +namespace Consensus { +struct Params; +} /** Maximum fee rate for sendrawtransaction and testmempoolaccept RPC calls. * Also used by the GUI when broadcasting a completed PSBT. @@ -38,4 +43,19 @@ static const CFeeRate DEFAULT_MAX_RAW_TX_FEE_RATE{COIN / 10}; */ [[nodiscard]] TransactionError BroadcastTransaction(NodeContext& node, CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback); +/** + * Return transaction with a given hash. + * If mempool is provided and block_index is not provided, check it first for the tx. + * If -txindex is available, check it next for the tx. + * Finally, if block_index is provided, check for tx by reading entire block from disk. + * + * @param[in] block_index The block to read from disk, or nullptr + * @param[in] mempool If provided, check mempool for tx + * @param[in] hash The txid + * @param[in] consensusParams The params + * @param[out] hashBlock The block hash, if the tx was found via -txindex or block_index + * @returns The tx if found, otherwise nullptr + */ +CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock); + #endif // BITCOIN_NODE_TRANSACTION_H diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 6dfccd9023..c617b0389c 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -76,8 +76,8 @@ static RPCHelpMan getrawtransaction() "\nBy default, this call only returns a transaction if it is in the mempool. If -txindex is enabled\n" "and no blockhash argument is passed, it will return the transaction if it is in the mempool or any block.\n" - "If -txindex is not enabled and a blockhash argument is passed, it will return the transaction if\n" - "the specified block is available and the transaction is found in that block.\n" + "If a blockhash argument is passed, it will return the transaction if\n" + "the specified block is available and the transaction is in that block.\n" "\nHint: Use gettransaction for wallet transactions.\n" "\nIf verbose is 'true', returns an Object with information about 'txid'.\n" diff --git a/src/validation.cpp b/src/validation.cpp index 20d641bf40..1b3d00bc6d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -19,7 +19,6 @@ #include <flatfile.h> #include <hash.h> #include <index/blockfilterindex.h> -#include <index/txindex.h> #include <logging.h> #include <logging/timer.h> #include <node/blockstorage.h> @@ -1155,38 +1154,6 @@ PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTx return result; } -CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock) -{ - LOCK(cs_main); - - if (mempool && !block_index) { - CTransactionRef ptx = mempool->get(hash); - if (ptx) return ptx; - } - if (g_txindex) { - CTransactionRef tx; - uint256 block_hash; - if (g_txindex->FindTx(hash, block_hash, tx)) { - if (!block_index || block_index->GetBlockHash() == block_hash) { - hashBlock = block_hash; - return tx; - } - } - } - if (block_index) { - CBlock block; - if (ReadBlockFromDisk(block, block_index, consensusParams)) { - for (const auto& tx : block.vtx) { - if (tx->GetHash() == hash) { - hashBlock = block_index->GetBlockHash(); - return tx; - } - } - } - } - return nullptr; -} - CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams) { int halvings = nHeight / consensusParams.nSubsidyHalvingInterval; diff --git a/src/validation.h b/src/validation.h index d3e4f3b983..9d8d7c06a9 100644 --- a/src/validation.h +++ b/src/validation.h @@ -140,20 +140,7 @@ void UnloadBlockIndex(CTxMemPool* mempool, ChainstateManager& chainman); void StartScriptCheckWorkerThreads(int threads_num); /** Stop all of the script checking worker threads */ void StopScriptCheckWorkerThreads(); -/** - * Return transaction with a given hash. - * If mempool is provided and block_index is not provided, check it first for the tx. - * If -txindex is available, check it next for the tx. - * Finally, if block_index is provided, check for tx by reading entire block from disk. - * - * @param[in] block_index The block to read from disk, or nullptr - * @param[in] mempool If provided, check mempool for tx - * @param[in] hash The txid - * @param[in] consensusParams The params - * @param[out] hashBlock The block hash, if the tx was found via -txindex or block_index - * @returns The tx if found, otherwise nullptr - */ -CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock); + CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams); bool AbortNode(BlockValidationState& state, const std::string& strMessage, const bilingual_str& userMessage = bilingual_str{}); diff --git a/test/lint/lint-circular-dependencies.sh b/test/lint/lint-circular-dependencies.sh index f8f24bb1ff..df5051720b 100755 --- a/test/lint/lint-circular-dependencies.sh +++ b/test/lint/lint-circular-dependencies.sh @@ -10,7 +10,6 @@ export LC_ALL=C EXPECTED_CIRCULAR_DEPENDENCIES=( "chainparamsbase -> util/system -> chainparamsbase" - "index/txindex -> validation -> index/txindex" "node/blockstorage -> validation -> node/blockstorage" "index/blockfilterindex -> node/blockstorage -> validation -> index/blockfilterindex" "index/base -> validation -> index/blockfilterindex -> index/base" |