eafd530d20 kernel: avoid potential duplicate object in shared library/binary (Cory Fields)
24c3b47010 build: add kernel-specific warnings (Cory Fields)
Pull request description:
This is a revival of https://github.com/bitcoin/bitcoin/pull/31807
Introduces the [-Wunique-object-duplication](https://clang.llvm.org/docs/DiagnosticsReference.html#wunique-object-duplication) warning flag available in clang-21 for usage when building the kernel library. It warns of potential duplicate objects in shared libraries. REDUCE_EXPORTS needs to be ON to trigger it.
Though we have a C API now that manages exporting symbols, I think it is prudent to also avoid any duplicate symbols on the internal c++ side in case we ever to decide to expose some of its headers. It also not clear that all linkers would handle these cases correctly even in the current internal usage.
ACKs for top commit:
fanquake:
ACK eafd530d20
hebasto:
ACK eafd530d20.
Tree-SHA512: 81961b50f0268dbe076497e130857f5b4b9151c748d107ec15158d1511dd25bce745e0beeb127b9cea51cb2edd78032735600606a75f7ff8a3fd572acced42e0
In some cases, we'll want to be more aggressive or care about different things
when building the kernel. In this case, a warning is added for symbols which
may be duplicated between the kernel and downstream users.
This warning was introduced in clang 21, which is not yet the minimum
supported compiler version. REDUCE_EXPORTS needs to be ON to trigger it.
6f113cb184 txgraph: use fallback order to sort chunks (feature) (Pieter Wuille)
0a3351947e txgraph: use fallback order when linearizing (feature) (Pieter Wuille)
fba004a3df txgraph: pass fallback_order to TxGraph (preparation) (Pieter Wuille)
941c432a46 txgraph test: subclass TxGraph::Ref like mempool does (preparation) (Pieter Wuille)
39d0052cbf clusterlin: make optimal linearizations deterministic (feature) (Pieter Wuille)
8bfbba3207 txgraph: sort distinct-cluster chunks by equal-feerate-prefix size (feature) (Pieter Wuille)
e0bc73ba92 clusterlin: sort tx in chunk by feerate and size (feature) (Pieter Wuille)
6c1bcb2c7c txgraph: clear cluster's chunk index in ~Ref (preparation) (Pieter Wuille)
7427c7d098 txgraph: update chunk index on Compact (preparation) (Pieter Wuille)
3ddafceb9a txgraph: initialize Ref in AddTransaction (preparation) (Pieter Wuille)
Pull request description:
Part of #30289.
TxGraph's fundamental responsibility is deciding the order of transactions in the mempool. It relies on the `cluster_linearize.h` code to optimize it, but there can and often will be many different orderings that are essentially equivalent from a quality perspective, so we have to pick one. At a high level, the solution will involve one or more of:
* Deciding based on **internal identifiers** (`Cluster::m_sequence`, `DepGraphIndex`). This is very simple, but risks leaking information about transaction receive order.
* Deciding **randomly**, which is private, but may interfere with relay expectations, block propagation, and ability to monitor network behavior.
* Deciding **based on txid**, which is private and deterministic, but risks incentivizing grinding to get an edge (though we haven't really seen such behavior).
* Deciding **based on size** (e.g. prefer smaller transactions), which is somewhat related to quality, but not unconditionally (depending on mempool layout, the ideal ordering might call for smaller transactions first, last, or anywhere in between). It's also not a strong ordering as there can be many identically-sized transactions. However, if it were to encourage grinding behavior, incentivizing smaller transactions is probably not a bad thing.
As of #32545, the current behavior is primarily picking randomly, though inconsistently, as some code paths also use internal identifiers and size. #33335 sought to change it to use random (preferring size in a few places), with the downsides listed above.
This PR is an alternative to that, which changes the order to tie-break based on size everywhere possible, and use lowest-txid-first as final fallback. This is fully deterministic: for any given set of mempool transactions, if all linearized optimally, the transaction order exposed by TxGraph is deterministic.
The transactions within a chunk are sorted according to:
1. `PostLinearize` (which improves sub-chunk order), using an initial linearization created using the rules 2-5 below.
2. Topology (parents before children).
3. Individual transaction feerate (high to low)
4. Individual transaction weight (small to large)
5. Txid (low to high txid)
The chunks within a cluster are sorted according to:
1. Topology (chunks after their dependencies)
2. Chunk feerate (high to low)
3. Chunk weight (small to large)
4. Max-txid (chunk with lowest maximum-txid first)
The chunks across clusters are sorted according to:
1. Feerate (high to low)
2. Equal-feerate-chunk-prefix weight (small to large)
3. Max-txid (chunk with lowest maximum-txid first)
The equal-feerate-chunk-prefix weight of a chunk C is defined as the sum of the weights of all chunks in the same cluster as C, with the same feerate as C, up to and including C itself, in linearization order (but excluding such chunks that appear after C). This is a well-defined approximation of sorting chunks from small to large across clusters, while remaining consistent with intra-cluster linearization order.
ACKs for top commit:
ajtowns:
reACK 6f113cb184 it was good before and now it's better
instagibbs:
ACK 6f113cb184
marcofleon:
light crACK 6f113cb184
Tree-SHA512: 16dc43c62b7e83c81db1ee14c01e068ae2f06c1ffaa0898837d87271fa7179dd98baeb74abc9fe79220e01fdba6876defe60022c2b72badc21d770644a0fe0ac
Instead of returning a TxGraph::Ref from TxGraph::AddTransaction(),
pass in a TxGraph::Ref& which is updated to refer to the new transaction
in that graph.
This cleans up the usage somewhat, avoiding the need for dummy Refs in
CTxMemPoolEntry constructor calls, but the motivation is that a future
commit will allow a callback to passed to MakeTxGraph to define a
fallback order on the transaction objects. This does not work when a
Ref is created separately from the CTxMemPoolEntry it ends up living in,
as passing the newly-created Ref to the callback would be UB before it's
emplaced in its final CTxMemPoolEntry.
37cc2a2d95 logging: use util/log.h where possible (stickies-v)
bb8e9e7c4c logging: Move message formatting to util/log.h (stickies-v)
001f0a428e move-only: Move logging macros to util/log.h (stickies-v)
94c0adf4e8 move-onlyish: Move logging levels to util/log.h (stickies-v)
56d113cab0 move-only: move logging categories to logging/categories.h (stickies-v)
f5233f7e98 move-only: Move SourceLocation to util/log.h (stickies-v)
Pull request description:
This is a mostly move-only change. It's a small refactoring that allows logging macros to be used by including a simple `util/log.h` header instead of the full `logging.h` logging implementation. Most of the changes here were cherry-picked from #34374.
Original motivation for this change was to reduce the size and complexity of #34374 (kernel structured logging PR) and reduce the number of conflicts it causes with other PRs. But this should also make sense as a standalone change to have a clearer separation of concerns between log generation and log handling, and avoid needing to depend on the whole logging framework in call sites that only emit log messages.
Recommended to review with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
ACKs for top commit:
l0rinc:
diff ACK 37cc2a2d95
stickies-v:
re-ACK 37cc2a2d95
optout21:
crACK 37cc2a2d95
sedited:
ACK 37cc2a2d95
Tree-SHA512: c7a761323ae63f07ad290d4e3766ba1348a132c8cc68a9895fa9ae5c89206599c00646c42ef77223ac757b9d8bfe6c181bead15e7058e4d8966b3bac88a8f950
fa0677d131 refactor: Use SpanReader over DataStream (MarcoFalke)
fad3eb3956 refactor: Use SpanReader over DataStream (MarcoFalke)
fa06e26764 refactor: [qt] Use SpanReader to avoid two vector copies (MarcoFalke)
fabd4d2e2e refactor: Avoid UB in SpanReader::ignore (MarcoFalke)
fa20bc2ec2 refactor: Use empty() over eof() in the streams interface (MarcoFalke)
fa879db735 test: Read debug log for self-checking comment (MarcoFalke)
Pull request description:
This changes all places, where possible, to use SpanReader over DataStream. This makes the code easier to read and reason about, because `SpanReader` can never write data. Also, the code should be minimally faster, because it avoids a full redundant copy of the whole vector of bytes.
ACKs for top commit:
stickies-v:
re-ACK fa0677d131
achow101:
ACK fa0677d131
janb84:
re ACK fa0677d131
sipa:
crACK fa0677d131
Tree-SHA512: 1d9f43fc6e71d481cf7b8f8457f479745ee331734649e9e2c2ab00ce5d317112796c77afc328612ed004e65ac5c16fc92279d760cfb012cfddce9098c4af810f
This refactor does not change behavior. However, it avoids a vector
copy, which can lead to a minimal speed-up of 1%-5%, depending on the
call-site. This is mostly relevant for the fuzz tests and utils that
read large blobs of data (like a full block).
a50d0b6720 build: don't pass on boost dependency to kernel consumers (Cory Fields)
Pull request description:
This is unnecessary now that the kernel now exports a (boost-less) API.
Noticed while slimming down boost dependencies in #34495.
ACKs for top commit:
stickies-v:
ACK a50d0b6720
hebasto:
ACK a50d0b6720, I have reviewed the code and it looks OK. I tested it by applying the Boost-specifc commits from https://github.com/bitcoin/bitcoin/pull/34143 and building with depends.
Tree-SHA512: e2d12356f41dd51dd729362121a33bd4f395821d53dd9a0bb0d5d6a53aba2ca2064e0709d9799dc6751b3d61ea576d2efc0e28296fdba26f2809dbcb0feabe44
Preparation for a future commit where kernel's dependency
on logging.cpp is removed completely.
Replace usage of logging\.h with util/log\.h where it
suffices, and fix wrong includes according to iwyu.
40735450c0 Remove unused epochguard.h (Suhas Daftuar)
1a8494d16c Rework CTxMemPool::GetChildren() to not use epochs (Suhas Daftuar)
Pull request description:
Since #33591, the epoch-based graph traversal optimization logic is only used for `CTxMempool::GetChildren()`, a function that is only used in RPC code and tests. Rewrite it without epochs, and remove `util/epochguard.h` itself, as that was its last use.
This allows us to reduce per-transaction memory usage by 8 bytes, for no material loss. With the new TxGraph-based mempool implementation, I also don't foresee future uses for it, as TxGraph can do even better by using BitSet-based traversal tracking.
ACKs for top commit:
ajtowns:
ACK 40735450c0
instagibbs:
ACK 40735450c0
l0rinc:
code review ACK 40735450c0
Tree-SHA512: 7ce7c04835cd2425a71c4fd47f316b6fb7381caa27383de7ecc4aa81100fcf7bc5e062699b307c08e0b853b35f06710d9ac761d6e660af9f9331e708d36f2fe0
This is likely slightly slower, but this was the last place we were using
epochs instead of sets to deduplicate, and this is only used by the RPC code
and in tests, and should not be CPU-performance critical. Eliminating this
allows us to save 8 bytes in CTxMemPoolEntry.
Co-Authored-By: Pieter Wuille <bitcoin-dev@wuille.net>
Introduces btck_BlockHeader type with accessor methods and btck_chainstate_manager_process_block_header() for validating headers without full blocks. Also, adds btck_chainstate_manager_get_best_entry() to query the header with most cumulative proof-of-work.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
Add C API functions for managing BlockValidationState lifecycle:
- btck_block_validation_state_create()
- btck_block_validation_state_copy()
- btck_block_validation_state_destroy()
Introduce BlockValidationStateApi<> template to share common getter methods between BlockValidationState (Handle) and BlockValidationStateView (View) classes in the C++ wrapper. This enables external code to create and own BlockValidationState objects needed for the new process_block_header() API.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
a5a8c4139c ci, iwyu: Fix warnings in `src/kernel` and treat them as errors (Hennadii Stepanov)
Pull request description:
Now seems like a good time to update the includes in `src/kernel`.
ACKs for top commit:
maflcko:
review ACK a5a8c4139c🍱
purpleKarrot:
ACK a5a8c4139c
sedited:
ACK a5a8c4139c
Tree-SHA512: ba401b27b03dee66d52d0b348972268e162506c4bafa40f408349173b68c40a11f20ca24f46c98945515e1d5c84f740d6e6784f7e4c799df46ab816cf5d11483
fa64d8424b refactor: Enforce readability-avoid-const-params-in-decls (MarcoFalke)
faf0c2d942 refactor: Avoid copies by using const references or by move-construction (MarcoFalke)
Pull request description:
Top level `const` in declarations is problematic for many reasons:
* It is often a typo, where one wanted to denote a const reference. For example `bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, ...` is missing the `&`. This will create a redundant copy of the value.
* In constructors it prevents move construction.
* It can incorrectly imply some data is const, like in an imaginary example `std::span<int> Shuffle(const std::span<int>);`, where the `int`s are *not* const.
* The compiler ignores the `const` from the declaration in the implementation.
* It isn't used consistently anyway, not even on the same line.
Fix some issues by:
* Using a const reference to avoid a copy, where read-only of the value is intended. This is only done for values that may be expensive to copy.
* Using move-construction to avoid a copy
* Applying `readability-avoid-const-params-in-decls` via clang-tidy
ACKs for top commit:
l0rinc:
diff reACK fa64d8424b
hebasto:
ACK fa64d8424b, I have reviewed the code and it looks OK.
sedited:
ACK fa64d8424b
Tree-SHA512: 293c000b4ebf8fdcc75259eb0283a2e4e7892c73facfb5c3182464d6cb6a868b7f4a6682d664426bf2edecd665cf839d790bef0bae43a8c3bf1ddfdd3d068d38
7b5d256af4 test: Add bitcoin-chainstate test for assumeutxo functionality (stringintech)
2bc3265649 Fix `ChainstateManager::AddChainstate()` assertion crash (stringintech)
5f3d6bdb66 Add regtest support to bitcoin-chainstate tool (stringintech)
Pull request description:
This PR adds functional test coverage for the bitcoin-chainstate tool loading a datadir initialized with an assumeutxo snapshot.
The PR also includes:
- Fix for assertion crash in `ChainstateManager::AddChainstate()` when `prev_chainstate` has no initialized mempool (required for the test to pass)
- `-regtest` flag support for bitcoin-chainstate to enable the testing
This work started while experimenting with the bitcoin-chainstate tool and how the kernel API (#30595) behaved when loading a datadir containing assumeutxo data, during the time that PR was still under review. sedited suggested opening a PR to add this test coverage.
ACKs for top commit:
achow101:
ACK 7b5d256af4
theStack:
Concept and code-review ACK 7b5d256af4
sedited:
Re-ACK 7b5d256af4
Tree-SHA512: 5d3b0050cf2d53144b5f65451c991d5e212117b4541ae1368ecf58fde5f3cca4f018aad6ae32257b9ebb1c28b926424fbcff496ba5487cdc4eb456cea6db8b24
`ChainstateManager::IsInitialBlockDownload()` is queried on hot paths and previously acquired `cs_main` internally, contributing to lock contention.
Cache the IBD status in `m_cached_is_ibd`, and introduce `ChainstateManager::UpdateIBDStatus()` to latch it once block loading has finished and the current chain tip has enough work and is recent.
Call the updater after tip updates and after `ImportBlocks()` completes.
Since `IsInitialBlockDownload()` no longer updates the cache, drop `mutable` from `m_cached_is_ibd` and only update it from `UpdateIBDStatus()` under `cs_main`.
Update the new unit test to showcase the new `UpdateIBDStatus()`.
Co-authored-by: Patrick Strateman <patrick.strateman@gmail.com>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Factor the chain tip work/recency check out of `ChainstateManager::IsInitialBlockDownload()` into a reusable `CChain::IsTipRecent()` helper, and annotate it as requiring `cs_main` since it's reading mutable state.
Also introduce a local `chainman_ref` in the kernel import-blocks wrapper to reduce repetition and keep follow-up diffs small.
`IsInitialBlockDownload` returns were also unified to make the followup move clean.
Co-authored-by: Patrick Strateman <patrick.strateman@gmail.com>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
44e006d438 [kernel] Expose reusable PrecomputedTransactionData in script valid (Josh Doman)
Pull request description:
This PR exposes a reusable `PrecomputedTransactionData` object in script validation using libkernel.
Currently, libkernel computes `PrecomputedTransactionData` each time `btck_script_pubkey_verify` is called, exposing clients to quadratic hashing when validating a transaction with multiple inputs. By externalizing `PrecomputedTransactionData` and making it reusable, libkernel can eliminate this attack vector.
I discussed this problem in [this issue](https://github.com/TheCharlatan/rust-bitcoinkernel/issues/46). The design of this PR is inspired by @sedited's comments.
The PR introduces three new APIs for managing the `btck_PrecomputedTransactionData` object:
```c
/**
* @brief Create precomputed transaction data for script verification.
*
* @param[in] tx_to Non-null.
* @param[in] spent_outputs Nullable for non-taproot verification. Points to an array of
* outputs spent by the transaction.
* @param[in] spent_outputs_len Length of the spent_outputs array.
* @return The precomputed data, or null on error.
*/
btck_PrecomputedTransactionData* btck_precomputed_transaction_data_create(
const btck_Transaction* tx_to,
const btck_TransactionOutput** spent_outputs, size_t spent_outputs_len) BITCOINKERNEL_ARG_NONNULL(1);
/**
* @brief Copy precomputed transaction data.
*
* @param[in] precomputed_txdata Non-null.
* @return The copied precomputed transaction data.
*/
btck_PrecomputedTransactionData* btck_precomputed_transaction_data_copy(
const btck_PrecomputedTransactionData* precomputed_txdata) BITCOINKERNEL_ARG_NONNULL(1);
/**
* Destroy the precomputed transaction data.
*/
void btck_precomputed_transaction_data_destroy(btck_PrecomputedTransactionData* precomputed_txdata);
```
The PR also modifies `btck_script_pubkey_verify` so that it accepts `precomputed_txdata` instead of `spent_outputs`:
```c
/**
* @brief Verify if the input at input_index of tx_to spends the script pubkey
* under the constraints specified by flags. If the
* `btck_ScriptVerificationFlags_WITNESS` flag is set in the flags bitfield, the
* amount parameter is used. If the taproot flag is set, the precomputed data
* must contain the spent outputs.
*
* @param[in] script_pubkey Non-null, script pubkey to be spent.
* @param[in] amount Amount of the script pubkey's associated output. May be zero if
* the witness flag is not set.
* @param[in] tx_to Non-null, transaction spending the script_pubkey.
* @param[in] precomputed_txdata Nullable if the taproot flag is not set. Otherwise, precomputed data
* for tx_to with the spent outputs must be provided.
* @param[in] input_index Index of the input in tx_to spending the script_pubkey.
* @param[in] flags Bitfield of btck_ScriptVerificationFlags controlling validation constraints.
* @param[out] status Nullable, will be set to an error code if the operation fails, or OK otherwise.
* @return 1 if the script is valid, 0 otherwise.
*/
int btck_script_pubkey_verify(
const btck_ScriptPubkey* script_pubkey,
int64_t amount,
const btck_Transaction* tx_to,
const btck_PrecomputedTransactionData* precomputed_txdata,
unsigned int input_index,
btck_ScriptVerificationFlags flags,
btck_ScriptVerifyStatus* status) BITCOINKERNEL_ARG_NONNULL(1, 3);
```
As before, an error is thrown if the taproot flag is set and `spent_outputs` is not provided in `precomputed_txdata` (or `precomputed_txdata` is null). For simple single-input non-taproot verification, `precomputed_txdata` may be null, and the kernel will construct the precomputed data on-the-fly.
Both the C++ wrapper and the test suite are updated with the new API. Tests cover both `precomputed_txdata` reuse and nullability.
Appreciate feedback on this concept / approach!
ACKs for top commit:
sedited:
Re-ACK 44e006d438
stringintech:
ACK 44e006d
Tree-SHA512: 1ed435173e6ff4ec82bc603194cf182c685cb79f167439a442b9b179a32f6c189c358f04d4cb56d153fab04e3424a11b73c31680e42b87b8a6efcc3ccefc366c
This should avoid having to include interfaces/chain.h from a kernel
module. interfaces/chain.h in turn includes a bunch of non-kernel
headers, that break the desired library topology and might introduce
entanglement regressions.
Deduplicate code looping over chainstate objects and calling
ActivateBestChain() and avoid need for code outside ChainstateManager to use
the GetAll() method.
Change ChainstateRole parameter passed to wallets and indexes. Wallets and
indexes need to know whether chainstate is historical and whether it is fully
validated. They should not be aware of the assumeutxo snapshot validation
process.
b0c706795c Remove unreliable seed from chainparams.cpp, and the associated README (SatsAndSports)
Pull request description:
The DNS seed `dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us.` is not returning a representative sample of bitcoin nodes. It currently returns nothing later than 28.1.0, breaching the policy.
This PR removes that seed from the list of DNS seeds
### Rationale
The [policy for seeds](https://github.com/bitcoin/bitcoin/blob/master/doc/dnsseed-policy.md) includes this:
> The DNS seed results must consist exclusively of fairly selected and functioning Bitcoin nodes from the public network
A number of comments below, in response to this PR, include apparent breaches of this policy: [1](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458071231) [2](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3457655364), [3](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3457712557), in particular the first linked comment ([1](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458071231)) comparing the distribution at this seed to other seeds. This seed is not including anything later than 28.2.0, breaching this policy.
To ensure the policy is followed, and the seeds include a representative sample of Bitcoin nodes, this PR removes this seed from the list
### Data
I ran this:
```
# Get some ip address from that seed:
# Repeated multiple times, to get many different IPs:
dig +short dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us >> dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us
# For each distinct ip gathered from the seed, get basic info about the node, including it's User Agent string:
cat dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us | sort -u | while read ip; do echo ===; echo $ip; nmap -p 8333 --script bitcoin-info "$ip"; done > seed_versions.txt
```
and then summarized the agents with `egrep 'User Agent' seed_versions.txt | sort | uniq -c` and got:
```
1 User Agent: /Satoshi:22.0.0/
1 User Agent: /Satoshi:22.1.0/
5 User Agent: /Satoshi:24.0.1/
1 User Agent: /Satoshi:25.1.0/
30 User Agent: /Satoshi:27.0.0/
1 User Agent: /Satoshi:27.1.0/
1 User Agent: /Satoshi:27.1.0/Knots:20240801/
1 User Agent: /Satoshi:28.0.0/
7 User Agent: /Satoshi:28.1.0/
2 User Agent: /Satoshi:28.1.0/Knots:20250305/
```
ACKs for top commit:
l0rinc:
reACK b0c706795c
delta1:
reACK b0c706795c
Crypt-iQ:
crACK b0c706795c
laanwj:
ACK b0c706795c
murchandamus:
ACK b0c706795c
RandyMcMillan:
ACK b0c7067
wiz:
ACK b0c706795c
dergoegge:
ACK b0c706795c
stickies-v:
re-ACK b0c706795c
mzumsande:
ACK b0c706795c
instagibbs:
ACK b0c706795c
Tree-SHA512: 7230b8dd24560ce6f8247e2e82ae7846ded8b91e230c59cc3643da3f5b9c12b5f025c1bb14490c19ca55f3794e81ce08106b31b3bf883d5c2dced05017123ac4
096924d39d kernel: add btck_block_tree_entry_equals (stickies-v)
Pull request description:
`BlockTreeEntry` objects are often compared. This happens frequently in our own codebase and seems likely to be the case for clients, too. Users can already work around this by comparing based on block hash (and optionally height as belt-and-suspenders), but I think this should be part of the interface for performance and consistency reasons.
Note: perhaps this is too ad-hoc, and we should extend this PR to add the operator for more types? `BlockTreeEntry` is the main one I've needed this for in developing `py-bitcoinkernel`, though.
ACKs for top commit:
maflcko:
review ACK 096924d39d📓
TheCharlatan:
ACK 096924d39d
yuvicc:
Code Review ACK 096924d39d
Tree-SHA512: a0c08c01ab6c855aec4e2b2b898e9550493cd4cf8c6e1fe9e4fe5039d0d9ef3bffb2f2ab0454c7cc449b9deedd7889f5fd7b5f100fa706a855023af4adb803c6
17cf9ff7ef Use cluster size limit for -maxmempool bound, and allow -maxmempool=0 in general (Suhas Daftuar)
315e43e5d8 Sanity check `GetFeerateDiagram()` in CTxMemPool::check() (Suhas Daftuar)
de2e9a24c4 test: extend package rbf functional test to larger clusters (Suhas Daftuar)
4ef4ddb504 doc: update policy/packages.md for new package acceptance logic (Suhas Daftuar)
79f73ad713 Add check that GetSortedScoreWithTopology() agrees with CompareMiningScoreWithTopology() (Suhas Daftuar)
a86ac11768 Update comments for CTxMemPool class (Suhas Daftuar)
9567eaa66d Invoke TxGraph::DoWork() at appropriate times (Suhas Daftuar)
6c5c44f774 test: add functional test for new cluster mempool RPCs (Suhas Daftuar)
72f60c877e doc: Update mempool_replacements.md to reflect feerate diagram checks (Suhas Daftuar)
21693f031a Expose cluster information via rpc (Suhas Daftuar)
72e74e0d42 fuzz: try to add more code coverage for mempool fuzzing (Suhas Daftuar)
f107417490 bench: add more mempool benchmarks (Suhas Daftuar)
7976eb1ae7 Avoid violating mempool policy limits in tests (Suhas Daftuar)
84de685cf7 Stop tracking parents/children outside of txgraph (Suhas Daftuar)
88672e205b Rewrite GatherClusters to use the txgraph implementation (Suhas Daftuar)
1ca4f01090 Fix miniminer_tests to work with cluster limits (Suhas Daftuar)
1902111e0f Eliminate CheckPackageLimits, which no longer does anything (Suhas Daftuar)
3a646ec462 Rework RBF and TRUC validation (Suhas Daftuar)
19b8479868 Make getting parents/children a function of the mempool, not a mempool entry (Suhas Daftuar)
5560913e51 Rework truc_policy to use descendants, not children (Suhas Daftuar)
a4458d6c40 Use txgraph to calculate descendants (Suhas Daftuar)
c8b6f70d64 Use txgraph to calculate ancestors (Suhas Daftuar)
241a3e666b Simplify ancestor calculation functions (Suhas Daftuar)
b9cec7f0a1 Make removeConflicts private (Suhas Daftuar)
0402e6c780 Remove unused limits from CalculateMemPoolAncestors (Suhas Daftuar)
08be765ac2 Remove mempool logic designed to maintain ancestor/descendant state (Suhas Daftuar)
fc4e3e6bc1 Remove unused members from CTxMemPoolEntry (Suhas Daftuar)
ff3b398d12 mempool: eliminate accessors to mempool entry ancestor/descendant cached state (Suhas Daftuar)
b9a2039f51 Eliminate use of cached ancestor data in miniminer_tests and truc_policy (Suhas Daftuar)
ba09fc9774 mempool: Remove unused function CalculateDescendantMaximum (Suhas Daftuar)
8e49477e86 wallet: Replace max descendant count with cluster_count (Suhas Daftuar)
e031085fd4 Eliminate Single-Conflict RBF Carve Out (Suhas Daftuar)
cf3ab8e1d0 Stop enforcing descendant size/count limits (Suhas Daftuar)
89ae38f489 test: remove rbf carveout test from mempool_limit.py (Suhas Daftuar)
c0bd04d18f Calculate descendant information for mempool RPC output on-the-fly (Suhas Daftuar)
bdcefb8a8b Use mempool/txgraph to determine if a tx has descendants (Suhas Daftuar)
69e1eaa6ed Add test case for cluster size limits to TRUC logic (Suhas Daftuar)
9cda64b86c Stop enforcing ancestor size/count limits (Suhas Daftuar)
1f93227a84 Remove dependency on cached ancestor data in mini-miner (Suhas Daftuar)
9fbe0a4ac2 rpc: Calculate ancestor data from scratch for mempool rpc calls (Suhas Daftuar)
7961496dda Reimplement GetTransactionAncestry() to not rely on cached data (Suhas Daftuar)
feceaa42e8 Remove CTxMemPool::GetSortedDepthAndScore (Suhas Daftuar)
21b5cea588 Use cluster linearization for transaction relay sort order (Suhas Daftuar)
6445aa7d97 Remove the ancestor and descendant indices from the mempool (Suhas Daftuar)
216e693729 Implement new RBF logic for cluster mempool (Suhas Daftuar)
ff8f115dec policy: Remove CPFP carveout rule (Suhas Daftuar)
c3f1afc934 test: rewrite PopulateMempool to not violate mempool policy (cluster size) limits (Suhas Daftuar)
47ab32fdb1 Select transactions for blocks based on chunk feerate (Suhas Daftuar)
dec138d1dd fuzz: remove comparison between mini_miner block construction and miner (Suhas Daftuar)
6c2bceb200 bench: rewrite ComplexMemPool to not create oversized clusters (Suhas Daftuar)
1ad4590f63 Limit mempool size based on chunk feerate (Suhas Daftuar)
b11c89cab2 Rework miner_tests to not require large cluster limit (Suhas Daftuar)
95a8297d48 Check cluster limits when using -walletrejectlongchains (Suhas Daftuar)
95762e6759 Do not allow mempool clusters to exceed configured limits (Suhas Daftuar)
edb3e7cdf6 [test] rework/delete feature_rbf tests requiring large clusters (glozow)
435fd56711 test: update feature_rbf.py replacement test (Suhas Daftuar)
34e32985e8 Add new (unused) limits for cluster size/count (Suhas Daftuar)
838d7e3553 Add transactions to txgraph, but without cluster dependencies (Suhas Daftuar)
d5ed9cb3eb Add accessor for sigops-adjusted weight (Suhas Daftuar)
1bf3b51396 Add sigops adjusted weight calculator (Suhas Daftuar)
c18c68a950 Create a txgraph inside CTxMemPool (Suhas Daftuar)
29a94d5b2f Make CTxMemPoolEntry derive from TxGraph::Ref (Suhas Daftuar)
92b0079fe3 Allow moving CTxMemPoolEntry objects, disallow copying (Suhas Daftuar)
6c73e47448 mempool: Store iterators into mapTx in mapNextTx (Suhas Daftuar)
51430680ec Allow moving an Epoch::Marker (Suhas Daftuar)
Pull request description:
[Reopening #28676 here as a new PR, because GitHub is slow to load the page making it hard to scroll through and see comments. Also, that PR was originally opened with a prototype implementation which has changed significantly with the introduction of `TxGraph`.]
This is an implementation of the [cluster mempool proposal](https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393).
This branch implements the following observable behavior changes:
- Maintains a partitioning of the mempool into connected clusters (via the `txgraph` class), which are limited in vsize to 101 kvB by default, and limited in count to 64 by default.
- Each cluster is sorted ("linearized") to try to optimize for selecting highest-feerate-subsets of a cluster first
- Transaction selection for mining is updated to use the cluster linearizations, selecting highest feerate "chunks" first for inclusion in a block template.
- Mempool eviction is updated to use the cluster linearizations, selecting lowest feerate "chunks" first for removal.
- The RBF rules are updated to: (a) drop the requirement that no new inputs are introduced; (b) change the feerate requirement to instead check that the feerate diagram of the mempool will strictly improve; (c) replace the direct conflicts limit with a directly-conflicting-clusters limit.
- The CPFP carveout rule is eliminated (it doesn't make sense in a cluster-limited mempool)
- The ancestor and descendant limits are no longer enforced.
- New cluster count/cluster vsize limits are now enforced instead.
- Transaction relay now uses chunk feerate comparisons to determine the order that newly received transactions are announced to peers.
Additionally, the cached ancestor and descendant data are dropped from the mempool, along with the multi_index indices that were maintained to sort the mempool by ancestor and descendant feerates. For compatibility (eg with wallet behavior or RPCs exposing this), this information is now calculated dynamically instead.
ACKs for top commit:
instagibbs:
reACK 17cf9ff7ef
glozow:
reACK 17cf9ff7ef
sipa:
ACK 17cf9ff7ef
Tree-SHA512: bbde46d913d56f8d9c0426cb0a6c4fa80b01b0a4c2299500769921f886082fb4f51f1694e0ee1bc318c52e1976d7ebed8134a64eda0b8044f3a708c04938eee7
An empty path may be represented with a nullptr. For example,
std::string_view::data() may return nullptr.
Removes the BITCOINKERNEL_ARG_NONNULL attribute for data_directory,
and instead handles such null arguments in the implementation.
Also documents how BITCOINKERNEL_ARG_NONNULL should be used.
BlockTreeEntry objects are often compared. By exposing an equality
function, clients don't have to implement more expensive
comparisons based on height and block hash.
An empty span constructed from an empty vector may have a null data
pointer depending on the implementation. Remove the
BITCOINKERNEL_ARG_NONNULL requirement for these arguments and instead
handle such null arguments in the implementation.
66978a1a95 kernel: remove btck_chain_get_tip (stickies-v)
4dd7e6dc48 kernel: remove btck_chain_get_genesis (stickies-v)
Pull request description:
Removes `btck_chain_get_genesis` and `btck_chain_get_tip`.
They are trivially replaced with `btck_chain_get_by_height` (as indicated in the updated `bitcoinkernel_wrapper.h`), so I think it makes sense to trim the interface.
For `btck_chain_get_tip`: on `master` we don't provide any guarantees that the returned block index still corresponds to the actual tip, so the extra call doesn't seem like a regression to me.
ACKs for top commit:
TheCharlatan:
ACK 66978a1a95
janb84:
ACK 66978a1a95
Tree-SHA512: f583fbb7f2e3f8f23afb57732b2cbe9e1d550bfc43c9a2619895ee30c27f5f3c5cd9e4ecb7e05b1f6ab9e11c368596ec9b733d67e06cfafb12326d88e8e4dd7d
