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
99d012ec80 refactor: return reference instead of pointer (Andrew Toth)
f743e6c5dd refactor: add missing LIFETIMEBOUND annotation for parameter (Andrew Toth)
141117f5e8 refactor: remove incorrect LIFETIMEBOUND annotations (Andrew Toth)
Pull request description:
The [developer-notes say](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lifetimebound):
> You can use the attribute by adding a `LIFETIMEBOUND`
annotation defined in `src/attributes.h`; please grep the codebase for examples.
While grepping, I found an incorrect usage of the `LIFETIMEBOUND` annotation on `BlockManager::CheckBlockDataAvailability`. This could be misleading about usage for other greppers. As I was looking, I also noticed a missing `LIFETIMEBOUND` on `BlockManager::GetFirstBlock`. While looking more closely at that method, it should return a reference instead of a pointer. The only reason to return a pointer is if it can be null.
ACKs for top commit:
maflcko:
review ACK 99d012ec80💧
l0rinc:
ACK 99d012ec80
stickies-v:
ACK 99d012ec80
optout21:
ACK 99d012ec80
vasild:
ACK 99d012ec80
Tree-SHA512: d6c56ee223d6614d52ee6cf5cd66e787125c98c6ae37705a17e51a6e15774e260ac55b3d60f2fc818132e766ad98dd94232d6c8829119f628498e9d0d2bd977f
f53dbbc505 test: Add functional tests for named argument parsing (zaidmstrr)
694f04e2bd rpc: Handle -named argument parsing where '=' character is used (zaidmstrr)
Pull request description:
Addresses [comment](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628) and [this](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2092039999).
The [PR #31375](https://github.com/bitcoin/bitcoin/pull/31375) got merged and enables `-named` by default in the `bitcoin rpc` interface; `bitcoin rpc` corresponds to `bitcoin-cli -named` as it's just a wrapper. Now, the problem arises when we try to parse the positional paramater which might contain "=" character. This splits the parameter into two parts first, before the "=" character, which treats this as the parameter name, but the other half is mostly passed as an empty string. Here, the first part of the string is an unknown parameter name; thus, an error is thrown. These types of errors are only applicable to those RPCs which might contain the `=` character as a parameter. Some examples are `finalizepsbt`, `decodepsbt`, `verifymessage` etc.
This is the one example of the error in `finalizepsbt` RPC:
```
./bitcoin-cli -named -regtest finalizepsbt cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O+g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA=
error code: -8
error message:
Unknown named parameter cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O+g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA
```
This PR fixes this by updating the `vRPCConvertParams` table that identifies parameters that need special handling in `-named` parameter mode. The parser now recognises these parameters and handles strings with "=" char correctly, preventing them from being incorrectly split as parameter assignments.
ACKs for top commit:
ryanofsky:
Code review ACK f53dbbc505. Just applied comment & test suggestions since last review
kannapoix:
Code review ACK: f53dbbc505
achow101:
ACK f53dbbc505
Tree-SHA512: 1b517144efeff45a4c4256c27a39ddf187f1d6189d133402a45171678214a10ff2925c31edcfd556d67f85bd26d42f63c528b941b68c9880eab443f2c883e681
Now that ancestor calculation never fails (due to ancestor/descendant limits
being eliminated), we can eliminate the error handling from
CalculateMemPoolAncestors.
The return value of BlockManager::GetFirstBlock must always be non-null. This
can be inferred by the implementation, which has an assertion that the return
value is not null. A raw pointer should only be returned if the result may be
null. In this case a reference is more appropriate.
fa6c0bedd3 refactor: Return uint64_t from GetSerializeSize (MarcoFalke)
fad0c8680e refactor: Use uint64_t over size_t for serialized-size values (MarcoFalke)
fa4f388fc9 refactor: Use fixed size ints over (un)signed ints for serialized values (MarcoFalke)
fa01f38e53 move-only: Move CBlockFileInfo to kernel namespace (MarcoFalke)
fa2bbc9e4c refactor: [rpc] Remove cast when reporting serialized size (MarcoFalke)
fa364af89b test: Remove outdated comment (MarcoFalke)
Pull request description:
Consensus code should arrive at the same conclusion, regardless of the architecture it runs on. Using architecture-specific types such as `size_t` can lead to issues, such as the low-severity [CVE-2025-46597](https://bitcoincore.org/en/2025/10/24/disclose-cve-2025-46597/).
The CVE was already worked around, but it may be good to still fix the underlying issue.
Fixes https://github.com/bitcoin/bitcoin/issues/33709 with a few refactors to use explicit fixed-sized integer types in serialization-size related code and concluding with a refactor to return `uint64_t` from `GetSerializeSize`. The refactors should not change any behavior, because the CVE was already worked around.
ACKs for top commit:
Crypt-iQ:
crACK fa6c0bedd3
l0rinc:
ACK fa6c0bedd3
laanwj:
Code review ACK fa6c0bedd3
Tree-SHA512: f45057bd86fb46011e4cb3edf0dc607057d72ed869fd6ad636562111ae80fea233b2fc45c34b02256331028359a9c3f4fa73e9b882b225bdc089d00becd0195e
060bb55508 rpc: add decoded tx details to gettransaction with extra wallet fields (Matthew Zipkin)
ad1c3bdba5 [move only] move DecodeTxDoc() to a common util file for sharing (Matthew Zipkin)
d633db5416 rpc: add "ischange: true" in wallet gettransaction decoded tx output (Matthew Zipkin)
Pull request description:
This change is motivated by external RBF clients like https://github.com/CardCoins/additive-rbf-batcher/. It saves the user a redundant re-looping of tx outputs, calling `getaddressinfo` on each one, looking for the change output in order to adjust the fee.
The field `"ischange"` only appears when `gettransaction` is called on a wallet, and is either `true` or not present at all. I chose not to include `ischange: false` because it is confusing to see that on *received* transactions.
Example of the new field:
```
"vout": [
{
"value": 1.00000000,
"n": 0,
"scriptPubKey": {
"asm": "0 5483235e05c76273b3b50af62519738781aff021",
"desc": "addr(bcrt1q2jpjxhs9ca388va4ptmz2xtns7q6lupppkw7wu)#d42g84j6",
"hex": "00145483235e05c76273b3b50af62519738781aff021",
"address": "bcrt1q2jpjxhs9ca388va4ptmz2xtns7q6lupppkw7wu",
"type": "witness_v0_keyhash"
}
},
{
"value": 198.99859000,
"n": 1,
"scriptPubKey": {
"asm": "0 870ab1ab58632b05a417d5295f4038500e407592",
"desc": "addr(bcrt1qsu9tr26cvv4stfqh65547spc2q8yqavj7fnlju)#tgapemkv",
"hex": "0014870ab1ab58632b05a417d5295f4038500e407592",
"address": "bcrt1qsu9tr26cvv4stfqh65547spc2q8yqavj7fnlju",
"type": "witness_v0_keyhash"
},
"ischange": true
}
]
```
ACKs for top commit:
furszy:
ACK [060bb55](060bb55508)
maflcko:
review ACK 060bb55508🌛
achow101:
ACK 060bb55508
rkrux:
lgtm ACK 060bb55508
Tree-SHA512: aae4854d2bb4e9a7bc1152691ea90e594e8da8a63c9c7fda72a504fb6a7e54ae274ed5fa98d35d270e0829cc8f8d2fd35a5fc9735c252a10aa42cc22828930e7
07a926474b node: change a tx-relay on/off flag to enum (Vasil Dimov)
Pull request description:
Previously the `bool relay` argument to `BroadcastTransaction()` designated:
```
relay=true: add to the mempool and broadcast to all peers
relay=false: add to the mempool
```
Change this to an `enum`, so it is more readable and easier to extend with a 3rd option. Consider these example call sites:
```cpp
Paint(true);
// Or
Paint(/*is_red=*/true);
```
vs
```cpp
Paint(RED);
```
The idea for putting `TxBroadcastMethod` into `node/types.h` by Ryan.
---
This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull/29415). Putting it in its own PR to reduce the size of #29415 and because it does not logically depend on the other commits from there.
ACKs for top commit:
optout21:
ACK 07a926474b
kevkevinpal:
ACK [07a9264](07a926474b)
laanwj:
Concept and code review ACK 07a926474b. Agree with the general reasoning and the change in #29415 is a valid motivation to change this interface.
glozow:
utACK 07a926474b
Tree-SHA512: ec8f6fa56a6d2422a0fbd5941ff2792685e8d8e7b9dd50bba9f3e21ed9b4a4a26c89b0d7e4895d48f30b7a635f2eddd894af26b5266410952cbdaf5c40b42966
1a1f46c228 refactor/doc: Add blockman param to `GetTransaction` doc comment and reorder out param (Musa Haruna)
Pull request description:
Follow-up to [#27125](https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190350876)
This PR addresses a minor documentation and style nit mentioned during review:
- Adds the missing `@param[in] blockman` line to the `GetTransaction()` doc comment.
- Moves the output parameter `hashBlock` to the end of both the function
declaration and definition, as suggested in the comment.
ACKs for top commit:
l0rinc:
ACK 1a1f46c228
maflcko:
re-lgtm-ut-cr-rfm-ACK 1a1f46c228
kevkevinpal:
reACK [1a1f46c](1a1f46c228)
Tree-SHA512: 5807a1ae6c383e691e948648dcb1e029620eaff3dcdff73d88c6dc268a7af5559a30c491b72f038b3f7e812e1845f4f063b49bd3671edfac1bb3a170c84be4f5
The values are small enough to fit in size_t, but to avoid having to
think about it, just use uint64_t consistently for all architectures.
On 64-bit systems, this refactor is a no-op. On 32-bit systems, it could
avoid bugs in the theoretical and unexpected case where a 32-bit size_t
is too small and overflows.
The values are small enough to fit in an int, so the cast is at best
redundant. However, UniValue can handle any integer type, so having to
think about the cast here is also confusing.
b63428ac9c rpc: refactor: use more (Maybe)Arg<std::string_view> (stickies-v)
037830ca0d refactor: increase string_view usage (stickies-v)
b3bf18f0ba rpc: refactor: use string_view in Arg/MaybeArg (stickies-v)
Pull request description:
The `RPCHelpMan::{Arg,MaybeArg}` helpers avoid copying (potentially) large strings by returning them as `const std::string*` (`MaybeArg`) or `const std::string&` (`Arg`). For `MaybeArg`, this has the not-so-nice effect that users need to deal with raw pointers, potentially also requiring new functions (e.g. [`EnsureUniqueWalletName` ](d127b25199 (diff-d8bfcfbdd5fa7d5c52d38c1fe5eeac9ce5c5a794cdfaf683585140fa70a32374R32))) with raw pointers being implemented.
This PR aims to improve on this by returning a trivially copyable `std::string_view` (`Arg`) or `std::optional<std::string_view>` (`MaybeArg`), modernizing the interface without introducing any additional copying overhead. In doing so, it also generalizes whether we return by value or by pointer/reference using `std::is_trivially_copyable_v` instead of defining the types manually.
In cases where functions currently take a `const std::string&` and it would be too much work / touching consensus logic to update them (`signmessage.cpp`), a `std::string` copy is made (which was already happening anyway).
The last 2 commits increase usage of the `{Arg,MaybeArg}<std::string_view>` helpers, and could be dropped/pruned if anything turns out to be controversial - I just think it's a nice little cleanup.
ACKs for top commit:
maflcko:
re-ACK b63428ac9c🎉
achow101:
ACK b63428ac9c
pablomartin4btc:
re-ACK [b63428a](b63428ac9c)
w0xlt:
reACK b63428ac9c
Tree-SHA512: b4942c353a1658c22a88d8c9b402c288ad35265a3b88aa2072b1f9b6d921cd073194ed4b00b807cb48ca440f47c87ef3d8e0dd1a5d814be58fc7743f26288277
c864a4c194 Simplify fs::path by dropping filename() and make_preferred() overloads (Ryan Ofsky)
b0113afd44 Fix windows libc++ fs::path fstream compile errors (Ryan Ofsky)
Pull request description:
Drop support for passing `fs::path` directly to `std::ifstream` and `std::ofstream` constructors and `open()` functions, because as reported by hebasto in https://github.com/bitcoin/bitcoin/issues/33545, after https://wg21.link/lwg3430 there is no way this can continue to work in windows builds, and there are already compile errors compiling for windows with newer versions of libc++.
Instead, add an `fs::path::std_path()` method that returns `std::filesystem::path` references and use it where needed.
ACKs for top commit:
hebasto:
ACK c864a4c194.
l0rinc:
Code review ACK c864a4c194
maflcko:
re-ACK c864a4c194 🌥
Tree-SHA512: d22372692ab86244e2b2caf4c5e9c9acbd9ba38df5411606b75e428474eabead152fc7ca1afe0bb0df6b818351211a70487e94b40a17b68db5aa757604a0ddf6
Previously the `bool relay` argument to `BroadcastTransaction()`
designated:
```
relay=true: add to the mempool and broadcast to all peers
relay=false: add to the mempool
```
Change this to an `enum`, so it is more readable and easier to extend
with a 3rd option. Consider these example call sites:
```cpp
Paint(true);
// Or
Paint(/*is_red=*/true);
```
vs
```cpp
Paint(RED);
```
The idea for putting `TxBroadcastMethod` into `node/types.h` by Ryan.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
652424ad16 test: additional test coverage for script_verify_flags (Anthony Towns)
417437eb01 script/verify_flags: extend script_verify_flags to 64 bits (Anthony Towns)
3cbbcb66ef script/interpreter: make script_verify_flag_name an ordinary enum (Anthony Towns)
bddcadee82 script/verify_flags: make script_verify_flags type safe (Anthony Towns)
a5ead122fe script/interpreter: introduce script_verify_flags typename (Anthony Towns)
4577fb2b1e rpc: have getdeploymentinfo report script verify flags (Anthony Towns)
a3986935f0 validation: export GetBlockScriptFlags() (Anthony Towns)
5db8cd2d37 Move mapFlagNames and FormatScriptFlags logic to script/interpreter.h (Anthony Towns)
Pull request description:
We currently use 21 of 32 possible bits for `SCRIPT_VERIFY_*` flags, with open PRs that may use 8 more (#29247, #31989, #32247, #32453). The mutinynet fork that has included many experimental soft fork features is [already reusing bits here](d4a86277ed/src/script/interpreter.h (L175-L195)). Therefore, bump this to 64 bits.
In order to make it easier to update this logic in future, this PR also introduces a dedicated type for the script flags, and disables implicit conversion between that type and the underlying integer type. To make verifying that this change doesn't cause flags to disappear, this PR also resurrects the changes from #28806 so that the script flags that are consensus enforced on each block can be queried via getdeploymentinfo.
ACKs for top commit:
instagibbs:
reACK 652424ad16
achow101:
ACK 652424ad16
darosior:
ACK 652424ad16
theStack:
Code-review ACK 652424ad16🎏
Tree-SHA512: 7b30152196cdfdef8b9700b571b7d7d4e94d28fbc5c26ea7532788037efc02e4b1d8de392b0b20507badfdc26f5c125f8356a479604a9149b8aae23a7cf5549f
These overloads were needed to allow passing `fs::path` objects directly to
libstdc++'s `fstream` constructors, but after the previous commit, there is no
longer any remaining code that does pass `fs::path` objects to `fstream`
constructors. Writing new code which does this is also discouraged because the
standard has been updated in https://wg21.link/lwg3430 to disallow it.
Dropping these also means its no longer possible to pass `fs::path` arguments
directly to `fstream::open` in libstdc++, which is somewhat unfortunate but not
a big loss because it is already not possible to pass them to the constructor.
So this commit updates `fstream::open` calls.
Additionally, this change required updates to src/bitcoin.cpp since it was
relying on the overloaded filename() method.
Use the {Arg,MaybeArg}<std::string_view> helper in all places where
it is a trivial change. In many places, this simplifies the logic
and reduces duplication of default values.
Modernizes interface by not forcing users to deal with raw pointers,
without adding copying overhead. Generalizes the logic of whether
we return by value or by optional/pointer.
In cases where functions take a `const std::string&` and it would
be too much work to update them, a string copy is made (which was
already happening anyway).
The returned value in `getpeerinfo/addr` could be a hostname as well as
an IP address and the `:port` part could be missing. It is displayed
from `CNode::m_addr_name` which could have been set from RPC `addnode`
where the argument is allowed to be a hostname and an optional port.
316a0c5132 rpc: addpeeraddress: throw on invalid IP (John Moffett)
Pull request description:
Right now we return an opaque `{"success" : false}` in `addpeeraddress` for an empty or invalid IP. This changes it to throw `RPC_CLIENT_INVALID_IP_OR_SUBNET` with the error message `Invalid IP address`. Tests updated to match.
ACKs for top commit:
sipa:
utACK 316a0c5132
achow101:
ACK 316a0c5132
vasild:
ACK 316a0c5132
pablomartin4btc:
tACK 316a0c5132
Tree-SHA512: 79a8ce127d0a24b2eb1f31bc3294b895d0c6424032a6b49168259e0e94aff69723d067adf1b4dc3c9b79e597531e5b65e4b8fc5a8e21fba0b81f99168de12b96
df67bb6fd8 test: Remove convert_to_json_for_cli (Ava Chow)
44a493e150 cli: Allow arguments to be both strings and json (Ava Chow)
Pull request description:
There are some RPCs where the argument can be either JSON that needs to be parsed, or a string that we can pass straight through. However, `bitcoin-cli` would always parse those arguments as JSON which makes for some cumbersome argument passing when using those RPCs. Notably, `hash_or_height` in `getblockstats` and `gettxoutsetinfo` do this, and results in a more cumbersome command of `bitcoin-cli getblockstats '"<hash>"'`. Otherwise, using a normal invocation of `bitcoin-cli getblockstats <hash>` results in `error: Error parsing JSON`. This PR marks those particular options as also being a string so that when `bitcoin-cli` fails to parse the argument as JSON, it will assume that the argument is a string and pass it straight through.
ACKs for top commit:
ryanofsky:
Code review ACK df67bb6fd8, just rebased since last review. I do still think it would be good to improve the test (https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2369570345)
rkrux:
Light code review, lgtm ACK df67bb6fd8
mzumsande:
Code Review ACK df67bb6fd8
Tree-SHA512: 6c488570fbb24d0cf10508416c56accfc7af5163b7a7187d22d78c812424a9e3ecc95906d3e295fbf6af54bf80903aa448fd879dd6a9944ba8b4d1a33eb29ef2
bf7996cbc3 rpc: fix getblock(header) returns target for tip (Sjors Provoost)
4c3c1f42cf test: add block 2016 to mock mainnet (Sjors Provoost)
Pull request description:
A `target` field was added to the `getblock` and `getblockheader` RPC calls in #31583, but it mistakingly always used the tip value.
This PR fixes it to return the target for the given block. Because regtest does not have difficulty adjustment, the mainnet test is expanded to cover the fix.
A preliminary commit deals with mining block 2016 that's needed for the test. It also:
- renames the `create_coinbase` `retarget_period` argument to `halving_period`. Before #31583 this was hardcoded for regtest where these values are the same.
- drops unused `fees` argument from `mine` helper
- expands the CPU miner instructions for generating the alternative mainnet chain
Fixes#33440
ACKs for top commit:
sipa:
utACK bf7996cbc3
luke-jr:
crACK bf7996cbc3
TheCharlatan:
ACK bf7996cbc3
ismaelsadeeq:
Code review ACK bf7996cbc3
Tree-SHA512: 2a2e11efd91f4aaccf9d2ec4dff9fd82c366b8a7e797ce5981dca2e6f08028f69154f4e6a27aef20d78b0e6c3304416789267c2fad42d7aa5072f8537d0c8b0d
2738b63e02 test: validate behaviour of getpeerinfo last_inv_sequence and inv_to_send (Anthony Towns)
77b2ebb811 rpc/net: report per-peer last_inv_sequence (Anthony Towns)
adefb51c54 rpc/net: add per-peer inv_to_send sizes (Anthony Towns)
Pull request description:
Adds per-peer entries to `getpeerinfo` for the size of the inv_to_send queue and the mempool sequence number as at the last INV. Can be helpful for debugging tx relay performance and privacy/fingerprinting issues.
ACKs for top commit:
sipa:
utACK 2738b63e02
instagibbs:
ACK 2738b63e02
Tree-SHA512: e3c9c52e8e38b099d405a177ffba6783c5821cc5ce1432b98218843e00906986ce2141dcd5b04a67006c328211a672e519fa3390e012688499bfc9ac99767599
A target field was added to the getblock and getblockheader RPC calls in bitcoin#31583, but it mistakingly always used the tip value.
Because regtest does not have difficulty adjustment, a test is added for mainnet instead.
When submitpackage produced no per-transaction result for a member,
the RPC previously set "error": "unevaluated" but then continued
without inserting the entry into tx-results, making it impossible for
callers to know which wtxids were unevaluated.
Insert the placeholder result before continuing, update help text, and
adjust functional tests to expect entries for all submitted wtxids.
Throw RPC_CLIENT_INVALID_IP_OR_SUBNET when LookupHost(addr, false) fails
in addpeeraddress. This aligns with setban/addconnection and avoids the
opaque {"success": false} result for input errors. The JSON {success,
error?} object remains for addrman outcomes only. Update test to match.
The index originally stored cumulative values in a CAmount type but this allowed for
potential overflow issues which were observed on Signet. Fix this by
storing the values that are in danger of overflowing in a arith_uint256.
Also turns an unnecessary copy into a reference in RevertBlock and
CustomAppend and gets
rid of the explicit total unspendable tracking which can be calculated
by adding the four categories of unspendables together.
1d9f1cb4bd kernel: improve BlockChecked ownership semantics (stickies-v)
9ba1fff29e kernel: refactor: ConnectTip to pass block pointer by value (stickies-v)
Pull request description:
Subscribers to the BlockChecked validation interface event may need access to the block outside of the callback scope. Currently, this is only possible by copying the block, which makes exposing this validation interface event publicly either cumbersome or with significant copy overhead.
By using shared_ptr, we make the shared ownership explicit and allow users to safely use the block outside of the callback scope. By using a const-ref shared_ptr, no atomic reference count cost is incurred if a subscriber does not require block ownership.
For example: in #30595, this would allow us to drop the `kernel_BlockPointer` handle entirely, and generalize everything into `kernel_Block`. This PoC is implemented in https://github.com/stickies-v/bitcoin/commits/kernel/remove-blockpointer/.
---
### Performance
I have added a benchmark in a [separate branch](https://github.com/stickies-v/bitcoin/commits/2025-07/validation-interface-ownership-benched/), to ensure this change does not lead to a problematic performance regression. Since most of the overhead comes from the subscribers, I have added scenarios for `One`, `Two`, and `Ten` subscribers. From these results, it appears there is no meaningful performance difference on my machine.
When `BlockChecked()` takes a `const CBlock&` reference _(master)_:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 170.09 | 5,879,308.26 | 0.3% | 0.01 | `BlockCheckedOne`
| 1,603.95 | 623,460.10 | 0.5% | 0.01 | `BlockCheckedTen`
| 336.00 | 2,976,173.37 | 1.1% | 0.01 | `BlockCheckedTwo`
When `BlockChecked()` takes a `const std::shared_ptr<const CBlock>&` _(this PR)_:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 172.20 | 5,807,155.33 | 0.1% | 0.01 | `BlockCheckedOne`
| 1,596.79 | 626,254.52 | 0.0% | 0.01 | `BlockCheckedTen`
| 333.38 | 2,999,603.17 | 0.3% | 0.01 | `BlockCheckedTwo`
ACKs for top commit:
achow101:
ACK 1d9f1cb4bd
w0xlt:
reACK 1d9f1cb4bd
ryanofsky:
Code review ACK 1d9f1cb4bd. These all seem like simple changes that make sense
TheCharlatan:
ACK 1d9f1cb4bd
yuvicc:
Code Review ACK 1d9f1cb4bd
Tree-SHA512: 7ed0cccb7883dbb1885917ef749ab7cae5d60ee803b7e3145b2954d885e81ba8c9d5ab1edb9694ce6b308235c621094c029024eaf99f1aab1b47311c40958095
5c8bf7b39e doc: add release notes for version 3 transactions (ishaanam)
4ef8065a5e test: add truc wallet tests (ishaanam)
5d932e14db test: extract `bulk_vout` from `bulk_tx` so it can be used by wallet tests (ishaanam)
2cb473d9f2 rpc: Support version 3 transaction creation (Bue-von-hon)
4c20343b4d rpc: Add transaction min standard version parameter (Bue-von-hon)
c5a2d08011 wallet: don't return utxos from multiple truc txs in AvailableCoins (ishaanam)
da8748ad62 wallet: limit v3 tx weight in coin selection (ishaanam)
85c5410615 wallet: mark unconfirmed v3 siblings as mempool conflicts (ishaanam)
0804fc3cb1 wallet: throw error at conflicting tx versions in pre-selected inputs (ishaanam)
cc155226fe wallet: set m_version in coin control to default value (ishaanam)
2e9617664e wallet: don't include unconfirmed v3 txs with children in available coins (ishaanam)
ec2676becd wallet: unconfirmed ancestors and descendants are always truc (ishaanam)
Pull request description:
This PR Implements the following:
- If creating a v3 transaction, `AvailableCoins` doesn't return unconfirmed v2 utxos (and vice versa)
- `AvailableCoins` doesn't return an unconfirmed v3 utxo if its transaction already has a child
- If a v3 transaction is kicked out of the mempool by a sibling, mark the sibling as a mempool conflict
- Throw an error if pre-selected inputs are of the wrong transaction version
- Allow setting version to 3 manually in `createrawtransaction` (uses commits from #31936)
- Limits a v3 transaction weight in coin selection
Closes#31348
To-Do:
- [x] Test a v3 sibling conflict kicking out one of our transactions from the mempool
- [x] Implement separate size limit for TRUC children
- [x] Test that we can't fund a v2 transaction when everything is v3 unconfirmed
- [x] Test a v3 sibling conflict being removed from the mempool
- [x] Test limiting v3 transaction weight in coin selection
- [x] Simplify tests
- [x] Add documentation
- [x] Test that user-input max weight is not overwritten by truc max weight
- [x] Test v3 in RPCs other than `createrawtransaction`
ACKs for top commit:
glozow:
reACK 5c8bf7b39e
achow101:
ACK 5c8bf7b39e
rkrux:
ACK 5c8bf7b39e
Tree-SHA512: da8aea51c113e193dd0b442eff765bd6b8dc0e5066272d3e52190a223c903f48788795f32c554f268af0d2607b5b8c3985c648879cb176c65540837c05d0abb5
