foster/bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-03-08 14:34:53 -05:00
Code Issues Activity
bitcoin-core/src/node
History
Andrew Chow 80fc1af096
Merge bitcoin/bitcoin#26289: Use util::Result in for calculating mempool ancestors 
47c4b1f52a mempool: log/halt when CalculateMemPoolAncestors fails unexpectedly (stickies-v)
5481f65849 mempool: add AssumeCalculateMemPoolAncestors helper function (stickies-v)
f911bdfff9 mempool: use util::Result for CalculateMemPoolAncestors (stickies-v)
66e028f739 mempool: use util::Result for CalculateAncestorsAndCheckLimits (stickies-v)

Pull request description:

  Upon reviewing the documentation for `CTxMemPool::CalculateMemPoolAncestors`, I noticed `setAncestors` was meant to be an `out` parameter but actually is an `in,out` parameter, as can be observed by adding `assert(setAncestors.empty());` as the first line in the function and running `make check`. This PR fixes this unexpected behaviour and introduces refactoring improvements to make intents and effects of the code more clear.

  ## Unexpected behaviour
  This behaviour occurs only in the package acceptance path, currently only triggered by `testmempoolaccept` and `submitpackage` RPCs.

  In `MemPoolAccept::AcceptMultipleTransactions()`, we first call `PreChecks()` and then `SubmitPackage()` with the same `Workspace ws` reference. `PreChecks` leaves `ws.m_ancestors` in a potentially non-empty state, before it is passed on to `MemPoolAccept::SubmitPackage`. `SubmitPackage` is the only place where `setAncestors` isn't guaranteed to be empty before calling `CalculateMemPoolAncestors`. The most straightforward fix is to just forcefully clear `setAncestors` at the beginning of CalculateMemPoolAncestors, which is done in the first bugfix commit.

  ## Improvements
  ### Return value instead of out-parameters
  This PR updates the function signatures for `CTxMemPool::CalculateMemPoolAncestors` and `CTxMemPool::CalculateAncestorsAndCheckLimits` to use a `util::Result` return type and eliminate both the `setAncestors` `in,out`-parameter as well as the error string. It simplifies the code and makes the intent and effects more explicit.

  ### Observability
  There are 7 instances where we currently call `CalculateMemPoolAncestors` without actually checking if the function succeeded because we assume that it can't fail, such as in [miner.cpp](69b10212ea/src/node/miner.cpp (L399)). This PR adds a new wrapper `AssumeCalculateMemPoolAncestors` function that logs such unexpected failures, or in case of debug builds even halts the program. It's not crucial to the objective, more of an observability improvement that seems sensible to add on here.

ACKs for top commit:
  achow101:
    ACK 47c4b1f52a
  w0xlt:
    ACK 47c4b1f52a
  glozow:
    ACK 47c4b1f52a
  furszy:
    light code review ACK 47c4b1f5
  aureleoules:
    ACK 47c4b1f52a

Tree-SHA512: d908dad00d1a5645eb865c4877cc0bae74b9cd3332a3641eb4a285431aef119f9fc78172d38b55c592168a73dae83242e6af3348815f7b37cbe2d448a3a58648
2023-01-03 16:30:55 -05:00
..
blockstorage.cpp scripted-diff: Bump copyright headers 2022-12-24 23:49:50 +00:00
blockstorage.h scripted-diff: Bump copyright headers 2022-12-24 23:49:50 +00:00
caches.cpp scripted-diff: Bump copyright headers 2022-12-24 23:49:50 +00:00
caches.h Add src/node/* code to node:: namespace 2022-01-06 22:14:16 -05:00
chainstate.cpp scripted-diff: Bump copyright headers 2022-12-24 23:49:50 +00:00
chainstate.h scripted-diff: Bump copyright headers 2022-12-24 23:49:50 +00:00
chainstatemanager_args.cpp Move ::fCheckBlockIndex into ChainstateManager 2022-10-18 14:11:48 +02:00
chainstatemanager_args.h Move ::nMinimumChainWork into ChainstateManager 2022-10-18 14:09:17 +02:00
coin.cpp Add src/node/* code to node:: namespace 2022-01-06 22:14:16 -05:00
coin.h scripted-diff: Bump copyright headers 2022-12-24 23:49:50 +00:00
connection_types.cpp [net] Move ConnectionType to its own file 2022-07-06 18:13:53 +02:00
connection_types.h [net] Move ConnectionType to its own file 2022-07-06 18:13:53 +02:00
context.cpp scripted-diff: Bump copyright headers 2022-12-24 23:49:50 +00:00
context.h scripted-diff: Bump copyright headers 2022-12-24 23:49:50 +00:00
eviction.cpp [net] Move eviction logic to its own file 2022-07-06 18:13:54 +02:00
eviction.h [net] Move eviction logic to its own file 2022-07-06 18:13:54 +02:00
interface_ui.cpp scripted-diff: Bump copyright headers 2022-12-24 23:49:50 +00:00
interface_ui.h scripted-diff: Bump copyright headers 2022-12-24 23:49:50 +00:00
interfaces.cpp Merge bitcoin/bitcoin#26289: Use util::Result in for calculating mempool ancestors 2023-01-03 16:30:55 -05:00
mempool_args.cpp Merge bitcoin/bitcoin#25872: Fix issues when calling std::move(const&) 2022-08-31 08:38:24 +01:00
mempool_args.h scripted-diff: Move mempool_args to src/node 2022-08-02 15:31:01 +02:00
mempool_persist_args.cpp DumpMempool: Pass in dump_path, stop using gArgs 2022-07-15 11:30:50 -04:00
mempool_persist_args.h Move DEFAULT_PERSIST_MEMPOOL out of libbitcoinkernel 2022-07-15 12:26:20 -04:00
miner.cpp Merge bitcoin/bitcoin#26289: Use util::Result in for calculating mempool ancestors 2023-01-03 16:30:55 -05:00
miner.h scripted-diff: Bump copyright headers 2022-12-24 23:49:50 +00:00
minisketchwrapper.cpp Add src/node/* code to node:: namespace 2022-01-06 22:14:16 -05:00
minisketchwrapper.h Add src/node/* code to node:: namespace 2022-01-06 22:14:16 -05:00
psbt.cpp scripted-diff: Bump copyright headers 2022-12-24 23:49:50 +00:00
psbt.h Add src/node/* code to node:: namespace 2022-01-06 22:14:16 -05:00
README.md doc: Remove irrelevant link to GitHub 2021-04-06 09:34:21 +02:00
transaction.cpp Add src/node/* code to node:: namespace 2022-01-06 22:14:16 -05:00
transaction.h scripted-diff: Bump copyright headers 2022-12-24 23:49:50 +00:00
txreconciliation.cpp p2p, test, refactor: Minor code improvements 2022-11-14 11:49:49 +02:00
txreconciliation.h p2p, refactor: Switch to enum class for ReconciliationRegisterResult 2022-11-14 11:37:28 +02:00
utxo_snapshot.cpp init: add utxo snapshot detection 2022-09-13 13:30:14 -04:00
utxo_snapshot.h scripted-diff: Bump copyright headers 2022-12-24 23:49:50 +00:00
validation_cache_args.cpp validationcaches: Use size_t for sizes 2022-08-03 12:03:28 -04:00
validation_cache_args.h validationcaches: Add and use ValidationCacheSizes 2022-08-03 12:03:27 -04:00

README.md

src/node/

The src/node/ directory contains code that needs to access node state (state in CChain, CBlockIndex, CCoinsView, CTxMemPool, and similar classes).

Code in src/node/ is meant to be segregated from code in src/wallet/ and src/qt/, to ensure wallet and GUI code changes don't interfere with node operation, to allow wallet and GUI code to run in separate processes, and to perhaps eventually allow wallet and GUI code to be maintained in separate source repositories.

As a rule of thumb, code in one of the src/node/, src/wallet/, or src/qt/ directories should avoid calling code in the other directories directly, and only invoke it indirectly through the more limited src/interfaces/ classes.

This directory is at the moment sparsely populated. Eventually more substantial files like src/validation.cpp and src/txmempool.cpp might be moved there.