Also improve test coverage for removeForReorg by creating a scenario where
there are in-mempool descendants that are only invalidated due to an in-mempool
parent no longer spending a mature coin.
8343a9ffcc test: add `-alertnotify` test for large work invalid chain warning (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `LARGE_WORK_INVALID_CHAIN` fork warning, checked with the `-alertnotify` option:
ead849c9f1/src/validation.cpp (L2033-L2040)
Found that this is missing during review of #32587. The test works by first creating a bunch of invalid blocks, that are first announced by headers and then submitted fully in reverse (invalid tip first), in order to set `m_best_invalid` to that value, finally leading to the best chain / invalid chain gap of >= 6 blocks. I'd be curious if there are other (more realistic?) ways to test this. One simple alternative is just to call `invalidateblock` twice (once at the tip, once at the base of the invalid chain).
Note that the written warning doesn't include the exclamation mark, as it is removed via `SanitizeString` in the `AlertNotify` function.
ACKs for top commit:
brunoerg:
reACK 8343a9ffcc
mzumsande:
re-ACK 8343a9ffcc
Tree-SHA512: d81e9ce7622026498cad5cdcdb867a22068670983737502888c72c72209ca6ff183e77d7429f758765a42c25cda439e01f795884864ac6fe6ff258a98d0bbcbc
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
Previously we would sanity check the -maxmempool configuration based on a
multiple of the descendant size limit, but with cluster mempool the maximum
evicted size is now the cluster size limit, so use that instead.
Also allow -maxmempool=0 in general (and not just if
-limitdescendantsize/-limitclustersize is set to 0).
288b8c30be doc: Drop (default: none) from -i2psam description (Ryan Ofsky)
f6ec3519a3 init: Require explicit -asmap filename (Ryan Ofsky)
Pull request description:
Currently, if `-asmap` is specified without a filename bitcoind tries to load `ip_asn.map` data file.
This change now requires `-asmap=ip_asn.map` or another filename to be specified explicitly.
The change is intended to make behavior of the option explicit and avoid confusion reported https://github.com/bitcoin/bitcoin/issues/33386 where documentation specifies a default file which is not actually loaded by default. It was originally implemented in
https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3410302383 and various alternatives are discussed there.
ACKs for top commit:
brunoerg:
reACK 288b8c30be
fjahr:
re-ACK 288b8c30be
vostrnad:
utACK 288b8c30be
achow101:
ACK 288b8c30be
Tree-SHA512: 11a38a03892a58d6ccc1505cfbf915f58a86df9891761d89dc54b92d40593ee3cbb2d7c7bdbb922b871b3529072ef7f34cc98393aff6e8f0633b56352315b27c
de7c3587cd doc: Update add checksum instructions in tutorial (Ben Westgate)
2a46e94a16 doc: Update multisig-tutorial.md to use multipath descriptors (Ben Westgate)
Pull request description:
### Summary
Update `doc/multisig-tutorial.md` to use multipath descriptor format
instead of separate external/internal descriptors. The tutorial now:
- extracts a single `xpub_n` per participant
- constructs a multipath `wsh(sortedmulti(...))` descriptor with `<0;1>`
change index semantics
- uses `getdescriptorinfo` to compute descriptor checksum
- explains that `importdescriptors` expands the multipath descriptor
into internal and external descriptors
- update `/test/functional/wallet_multisig_descriptor_psbt.py` functional test / documentation to use multi-path descriptors
---
### Motivation
A single multipath descriptor is the most convenient pattern for multisig; our documentation should use it.
---
### What changed
- replaced extraction of `external_xpub_n` and `internal_xpub_n` with
extraction of a single `xpub_n`
- removed instructions to create and import separate external/internal
descriptors
- added instructions to build a multipath `wsh(sortedmulti(...))`
descriptor and derive checksum with `getdescriptorinfo`
- checksum field is parsed and appended as the multipath descriptor is not the canonical "desc" output
- clarified that `importdescriptors` automatically expands multipath
descriptors into internal and external forms
- similar changes to the functional test: wallet_multisig_descriptor_psbt.
---
### Testing
I have run the updated shell snippets and confirmed the multipath descriptor produces the same `listdescriptors` output after importing as the two descriptor method in bitcoin:master.
---
### Related issues / PRs
This tutorial change references the multipath descriptor
consolidation (see commit / PR referenced in the change). The commit
message points to bitcoin#22838 as the upstream change that enables
this behavior.
---
### Release note (for changelog)
Documentation: update multisig tutorial and multisig functional test to use multipath descriptors
ACKs for top commit:
Sjors:
utACK de7c3587cd
kannapoix:
Core review ACK: de7c3587cd
achow101:
ACK de7c3587cd
rkrux:
crACK de7c3587cd
Tree-SHA512: ca7275d8ad04922b3fa8d2a3084ff96aa2104265f1fc2f749814dd16776351ab692d67e7e643d08052f7492e3eaa1a9a4dfe3470163e57939a49f782d3df511a
2578e6fc0f test: Fix race condition in IPC interface block propagation test (Fabian Jahr)
Pull request description:
CI failed on this condition here: https://github.com/bitcoin/bitcoin/actions/runs/19395398994/job/55494696022?pr=33878#step:9:3983
The check was added not too long ago in https://github.com/bitcoin/bitcoin/pull/33745 and the fix here switches the check to the node which actually produces the block. There are also some comments added to make the checks easier so understand.
Closes#33884
ACKs for top commit:
Sjors:
re-utACK 2578e6fc0f
maflcko:
lgtm ACK 2578e6fc0f
Tree-SHA512: bfb7ae44aede50a00d4096e1a9922f9b8df31ce4242e12863e329d0d1e714d8cb46c852f694c32314e4bd26b524535e3a6967b7c57861a9b00cf09831a950b99
28a4fcb03c test: check listdescriptors do not return a mix of hardened derivation marker (pythcoiner)
975783cb79 descriptor: account for all StringType in MiniscriptDescriptor::ToStringHelper() (pythcoiner)
Pull request description:
In `MiniscriptDescriptor::ToStringHelper()` only the `StringType::Private` variant of the `type` argument was handled. This PR implements serializing w/ all variants of `StringType` & add a functional test for the descriptor triggering the related issue.
Closes#31694: previously when calling `listdescriptors` RPC on a wallet containing a taproot descriptor w/ a (miniscript) taptree, origins of internal key & taptree were serialized w/ differents hardened derivation markers:
- origin of the internal key were serialized w/ `StringType::Normalized` type (using `h` as marker)
- origins of taptree keys were serialized w/ `StringType::Private` type (using `'` as marker)
Note: Origins in segwit (`wsh()`) miniscript descriptors were also serialized w/ `StringType::Private` type (`'` marker) and are now serialized w/ `StringType::Normalized` type (`h` marker).
ACKs for top commit:
sipa:
Code review ACK 28a4fcb03c
achow101:
ACK 28a4fcb03c
rkrux:
Concept ACK 28a4fcb03c
Tree-SHA512: 15d14000b5951ca69a64a05b9a0b138c48a07b81eaf2fa86b91ac20cc8735533355a787363c64ba88403dd8a56ef5232cba57d34bea80835a0f40774d62fbc2b
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
Previously, transaction batches were first sorted by ancestor count and then
feerate, to ensure transactions are announced in a topologically valid order,
while prioritizing higher feerate transactions. Ancestor count is a crude
topological sort criteria, so replace this with linearization order so that the
highest feerate transactions (as would be observed by the mining algorithm) are
relayed before lower feerate ones, in a topologically valid way.
This also fixes a test that only worked due to the ancestor-count-based sort
order.
With a total ordering on mempool transactions, we are now able to calculate a
transaction's mining score at all times. Use this to improve the RBF logic:
- we no longer enforce a "no new unconfirmed parents" rule
- we now require that the mempool's feerate diagram must improve in order
to accept a replacement
- the topology restrictions for conflicts in the package rbf setting have been
eliminated
Revert the temporary change to mempool_ephemeral_dust.py that were previously
made due to RBF validation checks being reordered.
Co-authored-by: Gregory Sanders <gsanders87@gmail.com>, glozow <gloriajzhao@gmail.com>
The addition of a cluster size limit makes the CPFP carveout rule useless,
because carveout cannot be used to bypass the cluster size limit. Remove this
policy rule and update tests to no longer rely on the behavior.
Include an adjustment to mempool_tests.cpp due to the additional memory used by
txgraph.
Includes a temporary change to the mempool_ephemeral_dust.py functional test,
due to validation checks being reordered. This change will revert once the RBF
rules are changed in a later commit.
b0a3887154 scripted-diff: fix leftover references to `policy/fees.h` (ismaelsadeeq)
Pull request description:
Fixes#33863
ryanofsky wrote
> I still see some references to the src/policy/fees.h file removed by this PR:
```
$ git grep -n policy/fees.h
src/wallet/rpc/spend.cpp:206: * @param[in] conf_target UniValue integer; confirmation target in blocks, values between 1 and 1008 are valid per policy/fees.h;
test/functional/rpc_estimatefee.py:39: # max value of 1008 per src/policy/fees.h
test/functional/rpc_psbt.py:604: assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", # max value of 1008 per src/policy/fees.h
test/functional/wallet_basic.py:337: assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", # max value of 1008 per src/policy/fees.h
test/functional/wallet_fundrawtransaction.py:851: assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", # max value of 1008 per src/policy/fees.h
test/functional/wallet_send.py:315: expect_error=(-8, "Invalid conf_target, must be between 1 and 1008")) # max value of 1008 per src/policy/fees.h
```
This is fixed in this PR by running a script that searches for what he greps and replaces it with the right reference.
```
git grep -l "policy\/fees\.h" | xargs sed -i "s/policy\/fees.h/policy\/fees\/block_policy_estimator.h/g"
```
ACKs for top commit:
kevkevinpal:
ACK [b0a3887](b0a3887154)
janb84:
ACK b0a3887154
rkrux:
lgtm ACK b0a3887154
Tree-SHA512: e24f2aaf18fcfb0ae047a53ed209135a644ff08f5a8bc162c1522be3f99d7d01d550fc2e73d8db5fec7b748902daf68e61e7a5624f5913b9824feba5641fc78c
c25a5e670b init: Signal m_tip_block_cv on Ctrl-C (Ryan Ofsky)
6a29f79006 test: Test SIGTERM handling during waitforblockheight call (Ryan Ofsky)
Pull request description:
Signal `m_tip_block_cv` when Ctrl-C is pressed or `SIGTERM` is received, the same way it is currently signaled when the `stop` RPC is called. This lets RPC calls like `waitforblockheight` and IPC calls like `waitTipChanged` be interrupted, instead of waiting for their original timeouts and delaying shutdown.
This issue was reported by plebhash in #33463. These hangs have been present since #30409. A similar bug was also fixed previously in Qt in #18452 and this PR simplifies that fix.
ACKs for top commit:
Sjors:
tACK c25a5e670b
TheCharlatan:
ACK c25a5e670b
enirox001:
Concept ACK c25a5e6
Tree-SHA512: 320aaa74fd308e826521c48c9a8aca4bd5f5530064cda2303d251d8e93e50c474bcd0db760ce04921928e73abefe4847aff797ac9ca7c89e74e5051bbed061cd
6eaa00fe20 test: clarify submitBlock() mutates the template (Sjors Provoost)
862bd43283 mining: ensure witness commitment check in submitBlock (Sjors Provoost)
00d1b6ef4b doc: clarify UpdateUncommittedBlockStructures (Sjors Provoost)
Pull request description:
When an IPC client requests a new block template via the Mining interface, we hold on to its `CBlock`. That way when they call `submitSolution()` we can modify it in place, rather than having to reconstruct the full block like the `submitblock` RPC does.
Before this commit however we forgot to invalidate `m_checked_witness_commitment`, which we should since the client brings a new coinbase.
This would cause us to accept an invalid chaintip.
Fix this and add a test to confirm that we now reject such a block. As a sanity check, we add a second node to the test and confirm that will accept our mined block.
As first noticed in #33374 the IPC code takes the coinbase as provided, unlike the `submitblock` RPC which calls `UpdateUncommittedBlockStructures()` and adds witness commitment to the coinbase if it was missing.
Although that could have been an alternative fix, we instead document that IPC clients are expected to provide the full coinbase including witness commitment.
Patch to produce the original issue:
```diff
diff --git a/src/node/miner.cpp b/src/node/miner.cpp
index b988e28a3f..28e9048a4d 100644
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -450,15 +450,10 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t
}
block.nVersion = version;
block.nTime = timestamp;
block.nNonce = nonce;
block.hashMerkleRoot = BlockMerkleRoot(block);
-
- // Reset cached checks
- block.m_checked_witness_commitment = false;
- block.m_checked_merkle_root = false;
- block.fChecked = false;
}
std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainman,
KernelNotifications& kernel_notifications,
CTxMemPool* mempool,
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index cce56e3294..bf1b7048ab 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -216,22 +216,22 @@ class IPCInterfaceTest(BitcoinTestFramework):
assert_equal(res.result, True)
# The remote template block will be mutated, capture the original:
remote_block_before = await self.parse_and_deserialize_block(template, ctx)
- self.log.debug("Submitted coinbase must include witness")
+ self.log.debug("Submitted coinbase with missing witness is accepted")
assert_not_equal(coinbase.serialize_without_witness().hex(), coinbase.serialize().hex())
res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize_without_witness())
- assert_equal(res.result, False)
+ assert_equal(res.result, True)
self.log.debug("Even a rejected submitBlock() mutates the template's block")
# Can be used by clients to download and inspect the (rejected)
# reconstructed block.
remote_block_after = await self.parse_and_deserialize_block(template, ctx)
assert_not_equal(remote_block_before.serialize().hex(), remote_block_after.serialize().hex())
- self.log.debug("Submit again, with the witness")
+ self.log.debug("Submit again, with the witness - does not replace the invalid block")
res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())
assert_equal(res.result, True)
self.log.debug("Block should propagate")
assert_equal(self.nodes[1].getchaintips()[0]["height"], current_block_height + 1)
```
ACKs for top commit:
ryanofsky:
Code review ACK 6eaa00fe20. Just documentation updates and test clarifications since last review, also splitting up a commit.
TheCharlatan:
Re-ACK 6eaa00fe20
ismaelsadeeq:
Code review and tested ACK 6eaa00fe20
Tree-SHA512: 3a6280345b0290fe8300ebc63c13ad4058d24ceb35b7d7a784b974d5f04f420860ac03a9bf2fc6a799ef3fc55552ce033e879fa369298f976b9a01d72bd55d9e
8810642b57 test: add option to skip large re-org test in feature_block (brunoerg)
Pull request description:
Fixes#32877
This PR adds a config flag `--skipreorg` which is used to skip the large re-org test. According to corecheck, `feature_block` is our slowest functional test and primarily because of this large re-org test. However, this test might not be useful for the mutation analysis of some files and could be skipped to save a huge amount of time.
```
time ./build/test/functional/feature_block.py --skipreorg
./build/test/functional/feature_block.py --skipreorg 11.38s user 0.33s system 37% cpu 31.422 total
time ./build/test/functional/feature_block.py
./build/test/functional/feature_block.py 25.87s user 3.53s system 56% cpu 52.317 total
```
ACKs for top commit:
maflcko:
review ACK 8810642b57🥁
enirox001:
tACK 8810642 – Ran tests with/without --skipreorg; saw ~40 % speedup; no regressions.
theStack:
Concept and code-review ACK 8810642b57
glozow:
lgtm ACK 8810642b57
Tree-SHA512: 4ef38bd32b8ad8ec2b7f30c96d2fe545d920759645ff52f632699f829b64f8d26fe878f3fdd255142235edd0a740a7feb64da8f5a10d0d740ebfa46c43ae60eb
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
01cc20f330 test: improve coverage for a resolved stalling situation (Martin Zumsande)
9af6daf07e test: remove magic number when checking for blocks that have arrived (Martin Zumsande)
3069d66dca p2p: During block download, adjust pindexLastCommonBlock better (Martin Zumsande)
Pull request description:
As described in #32179, `pindexLastCommonBlock` is updated later than necessary
in master.
In case of a linear chain with no forks, it can be moved forward at the beginning of
`FindNextBlocksToDownload`, so that the updated value can be used to better estimate `nWindowEnd`.
This helps the node to request all blocks from peers within the correct 1024-block-window and avoids peers being incorrectly marked as stallers.
I also changed `p2p_ibd_stalling.py` to cover the situation after a resolved situation, making sure that no additional peers are marked for stalling.
Fixes#32179
ACKs for top commit:
Crypt-iQ:
crACK 01cc20f330
stringintech:
re-ACK 01cc20f
achow101:
ACK 01cc20f330
sipa:
utACK 01cc20f330
Tree-SHA512: a97f7a7ef5ded538ee35576e04b3fbcdd46a6d0189c7ba3abacc6e0d81e800aac5b0c2d2565d0462ef6fd4acc751989f577fd6adfd450171a7d6ab26f437df32
79b4c276e7 Bugfix: QA: rpc_bind: Skip nonloopback test if no such address is found (Luke Dashjr)
Pull request description:
Without this, I get:
```
2025-09-19T03:14:05.157000Z TestFramework (INFO): PRNG seed is: 3218602557639511064
2025-09-19T03:14:05.158000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin-test/a
2025-09-19T03:14:05.158000Z TestFramework (INFO): Check for ipv6
2025-09-19T03:14:05.158000Z TestFramework (INFO): Check for non-loopback interface
2025-09-19T03:14:05.158000Z TestFramework (INFO): Bind test for []
2025-09-19T03:14:05.516000Z TestFramework (INFO): Bind test for []
2025-09-19T03:14:05.871000Z TestFramework (INFO): Bind test for ['[::1]']
2025-09-19T03:14:06.227000Z TestFramework (INFO): Bind test for ['127.0.0.1', '[::1]']
2025-09-19T03:14:06.583000Z TestFramework (INFO): Using interface None for testing
2025-09-19T03:14:06.583000Z TestFramework (INFO): Bind test for [None]
2025-09-19T03:14:06.583000Z TestFramework (ERROR): Unexpected exception
Traceback (most recent call last):
File "/Bitcoin/bitcoin/workingtree/test/functional/test_framework/test_framework.py", line 135, in main
self.run_test()
~~~~~~~~~~~~~^^
File "/Bitcoin/bitcoin/workingtree/test/functional/rpc_bind.py", line 126, in run_test
self._run_nonloopback_tests()
~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "/Bitcoin/bitcoin/workingtree/test/functional/rpc_bind.py", line 157, in _run_nonloopback_tests
self.run_bind_test([self.non_loopback_ip], self.non_loopback_ip, [self.non_loopback_ip],
~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[(self.non_loopback_ip, self.defaultport)])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Bitcoin/bitcoin/workingtree/test/functional/rpc_bind.py", line 38, in run_bind_test
expected = [(addr_to_hex(addr), port) for (addr, port) in expected]
~~~~~~~~~~~^^^^^^
File "/Bitcoin/bitcoin/workingtree/test/functional/test_framework/netutil.py", line 132, in addr_to_hex
if '.' in addr: # IPv4
^^^^^^^^^^^
TypeError: argument of type 'NoneType' is not iterable
```
ACKs for top commit:
maflcko:
review ACK 79b4c276e7🏑
theStack:
Tested ACK 79b4c276e7
Tree-SHA512: 2a723d9bc5d1d50a8321a4f8a8cac3da3125d373ea71e6cc9d03de07307008f58970e361490d4c34530a6a976cb078b62d0ef09b7fb321ca1cfb9249a70d99a5
dcb56fd4cb interfaces: add interruptWait method (ismaelsadeeq)
Pull request description:
This is an attempt to fix#33575 see the issue for background and the usefulness of this feature.
This PR uses one of the suggested approaches: adding a new `interruptWaitNext()` method to the mining interface.
It introduces a new boolean variable, `m_interrupt_wait`, which is set to `false` when the thread starts waiting. The `interruptWaitNext()` method wakes the thread and sets `m_interrupt_wait` to `true`.
Whenever the thread wakes up, it checks whether the wait was aborted; if so, it simply set ` m_interrupt_wait ` to false and return`nullptr`.
This PR also adds a functional test for the new method. The test uses `asyncio` to spawn two tasks and attempts to ensure that the wait is executed before the interrupt by using an event monitor. It adds a 0.1-second buffer to ensure the wait has started executing.
If that buffer elapses without `waitNext` executing, the test will fail because a transaction is created after the buffer.
ACKs for top commit:
furszy:
Code ACK dcb56fd4cb
ryanofsky:
Code review ACK dcb56fd4cb, just tweaking semantics slightly since last review so if an `interruptWait` call is made shortly after a `waitNext` call it will reliably cause the `waitNext` call to return right away without blocking, even if the `waitNext` call had not begun to execute or wait yet.
Sjors:
tACK dcb56fd4cb
TheCharlatan:
ACK dcb56fd4cb
Tree-SHA512: a03f049e1f303b174a9e5d125733b6583dfd8effa12e7b6c37bd9b2cff9541100f5f4514e80f89005c44a57d7e47804afe87aa5fdb6831f3b0cd9b01d83e42be
ec8516ceb7 test: remove obsolete `get_{key,multisig}` helpers from wallet_util.py (Sebastian Falbesoner)
Pull request description:
This small cleanup PR is a late follow-up to #31250 (commit c847dee148). These helpers are unused and wouldn't work anymore, as they call a legacy wallet RPC (`dumpprivkey`). They were only ever used for testing the `importmulti` RPC, which also doesn't exist anymore. Functional tests that need to create key pairs and derive various output script types from them can use `get_generate_key` (introduced in #16528, commit f193ea889d) instead, without involving the node.
ACKs for top commit:
rkrux:
crACK ec8516ceb7
brunoerg:
code review ACK ec8516ceb7
Tree-SHA512: cab3701f1a8fbcff0eecea4cfdc632ffac226afd2eefe3c9274a84ee1bb71fb231a57cd0876025c714be257a249157b048b67e309b3734442c425d85cf481cf6
These helpers use a legacy wallet RPC (`dumpprivkey`) and thus don't
work anymore. They were only ever used for testing the `importmulti`
RPC, which also doesn't exist anymore.
81e5c8385b test: cover invalid codesep positions for signature in taproot (Greg Sanders)
Pull request description:
There is some basic coverage, but I felt like adding some boundary conditions where the only issue is the codesep value would be nice.
ACKs for top commit:
ajtowns:
ACK 81e5c8385b
TheCharlatan:
ACK 81e5c8385b
Tree-SHA512: de74895c3bb49854987654720ebcefea2f47c4a55ba6ab4a52878f6a9a0bd8b3085afa3485101610327fa8d35c3d074542f58540e126460bd4bea918cb0054ee
Currently, if `-asmap` is specified without a filename, bitcoind tries to load
`ip_asn.map` data file.
This change now requires `-asmap=ip_asn.map` or another filename to be
specified explicitly.
The change is intended to make behavior of the option explicit avoid confusion
reported https://github.com/bitcoin/bitcoin/issues/33386 where documentation
specifies a default file which is not actually loaded by default. It was
originally implemented in
https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3410302383 and
various alternatives are discussed there.
Co-authored-by: Fabian Jahr <fjahr@protonmail.com>
PR #33374 proposed a new Mining IPC method applySolution() which
could be used by clients to obtain the reconstructed block for
inspection, especially in the case of a rejected block.
However it was pointed out during review that submitBlock() modified
the template CBlock in place, so the client can just call getBlock()
and no new method is needed.
This commit adds a test to document that (now intentional) behavior.
When an IPC client requests a new block template via the Mining interface,
we hold on to its CBlock. That way when they call submitSolution() we can
modify it in place, rather than having to reconstruct the full block like
the submitblock RPC does.
Before this commit however we forgot to invalidate
m_checked_witness_commitment, which we should since the client brings a
new coinbase.
This would cause us to accept an invalid chaintip.
Fix this and add a test to confirm that we now reject such a block.
As a sanity check, we add a second node to the test and confirm that will
accept our mined block.
Note that the IPC code takes the coinbase as provided, unlike the
submitblock RPC which calls UpdateUncommittedBlockStructures() and adds
witness commitment to the coinbase if it was missing.
Although that could have been an alternative fix, we instead document that
IPC clients are expected to provide the full coinbase including witness
commitment.
66667d6512 test: Use same rpc timeout for authproxy and cli (MarcoFalke)
Pull request description:
It seems odd to use different timeouts (and timeout factors) depending on whether the Python RPC proxy is used, or the bitcoin rpc command line interface.
Fix it by using the same timeout.
This can be tested by introducing a timeout error and checking it happens with and without `--usecli` after the exact same time.
Example timeout error:
```diff
diff --git a/test/functional/mining_template_verification.py b/test/functional/mining_template_verification.py
index de0833c596..e0f93a2b1e 100755
--- a/test/functional/mining_template_verification.py
+++ b/test/functional/mining_template_verification.py
@@ -173,7 +173,7 @@ class MiningTemplateVerificationTest(BitcoinTestFramework):
self.log.info("Submitting this block should succeed")
assert_equal(node.submitblock(block.serialize().hex()), None)
- node.waitforblockheight(2)
+ node.waitforblockheight(200000)
def transaction_test(self, node, block_0_height, tx):
self.log.info("make block template with a transaction")
```
Example cmd: `./bld-cmake/test/functional/mining_template_verification.py --timeout-factor=0.1 --usecli`.
ACKs for top commit:
brunoerg:
ACK 66667d6512
stickies-v:
tACK 66667d6512
Tree-SHA512: c8c21d8b9fb60ab192e3bbd45b317b96a40e10bf03704148613ac3cbdaae4abc2c03c4afbd504309ea0958201267c0d2a4bc5b40aa020917175c47e080ffe292
57f7c68821 test: add functional test for `TestShell` (matching doc example) (Sebastian Falbesoner)
53874f7934 doc: test: update TestShell example instructions/options (Sebastian Falbesoner)
Pull request description:
This PR adds a functional framework test for the `TestShell` class. The primary motivation for this is to avoid that the example instructions for the interactive Python shell in `test-shell.md` get outdated or broken without noticing, a problem we had already several times in the past (see #26520, #27906, #31415). Having a copy is still not perfect, as docs and functional test have to be kept in sync, but I don't expect this to be a problem in practice, assuming the hint in the functional test comment is hopefully noticed if changes are made.
Alternatively, the example instructions in `test-shell.md` could be removed with a hint to the functional test (I tend to prefer to keep the docs as-is though, showing the full REPL interaction).
The first commit contain two small removal fix-ups in `test-shell.md` regarding the result of the `createwallet` RPC call and the mentioning of the `noshutdown` option that was removed earlier (see #31620). Could be backported to v30.
ACKs for top commit:
brunoerg:
ACK 57f7c68821
stratospher:
ACK 57f7c68.
pinheadmz:
ACK 57f7c68821
Tree-SHA512: 25c35af46b742bbefce7838708437529bbf613fa3d1f0fba590cacef0acdde82b3a78c7a01459c73eaac26ce3f1041e54092d098f0fc94a8c76ac0b4f4970338
664657ed13 bugfix: disallow label for ranged descriptors & allow external non-ranged descriptors to have label (scgbckbone)
Pull request description:
Motivation:
* ranged descriptors MUST not be able to have label (current impl allows it)
* external non-ranged descriptor MUST be able to have label (current impl disallows it, **if** `internal=false` is provided via importdescriptor user data)
Repro steps:
* create blank wallet and import descriptors
* external has `label=test` (not internal)
```
conn = bitcoind.create_wallet(wallet_name=w_name, disable_private_keys=True, blank=True,
passphrase=None, avoid_reuse=False, descriptors=True)
descriptors = [
{
"timestamp": "now",
"label": "test",
"active": True,
"desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/0/*)#erexmnep",
"internal": False
},
{
"desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/1/*)#ghu8xxfe",
"active": True,
"internal": True,
"timestamp": "now"
},
]
r = conn.importdescriptors(descriptors)
print(r)
```
response:
```
[{'error': {'code': -8,
'message': 'Internal addresses should not have a label'},
'success': False,
'warnings': ['Range not given, using default keypool range']},
{'success': True,
'warnings': ['Range not given, using default keypool range']}]
```
But in above, ONLY external has a label.
If you remove `internal: False` from external descriptor import object - it will import no problem:
```
[{'success': True,
'warnings': ['Range not given, using default keypool range']},
{'success': True,
'warnings': ['Range not given, using default keypool range']}]
```
Even tho it should NOT, as the descriptor is ranged. Current implementation relies on checking user provided data to decide whether desc is ranged.
ACKs for top commit:
achow101:
ACK 664657ed13
rkrux:
lgtm crACK 664657ed13
Tree-SHA512: 9e70aea620019c29950ba417d4ae38d65cd94a4f6fcabbc021d67b031de1c44c27d6f6f5cb7e6950a099eb6e58bed9be764d4c6347195daeccb14a5d95c123b2
0465574c12 test: Fixes send_blocks_and_test docs (Sergi Delgado Segura)
09c95f21e7 test: Adds block tiebreak over restarts tests (Sergi Delgado Segura)
18524b072e Make nSequenceId init value constants (Sergi Delgado Segura)
8b91883a23 Set the same best tip on restart if two candidates have the same work (Sergi Delgado Segura)
5370bed21e test: add functional test for complex reorgs (Pieter Wuille)
ab145cb3b4 Updates CBlockIndexWorkComparator outdated comment (Sergi Delgado Segura)
Pull request description:
This PR grabs some interesting bits from https://github.com/bitcoin/bitcoin/pull/29284 and fixes some edge cases in how block tiebreaks are dealt with.
## Regarding #29284
The main functionality from the PR was dropped given it was not an issue anymore, however, reviewers pointed out some comments were outdated https://github.com/bitcoin/bitcoin/pull/29284#discussion_r1522023578 (which to my understanding may have led to thinking that there was still an issue) it also added test coverage for the aforementioned case which was already passing on master and is useful to keep.
## New functionality
While reviewing the superseded PR, it was noticed that blocks that are loaded from disk may face a similar issue (check https://github.com/bitcoin/bitcoin/pull/29284#issuecomment-1994317785 for more context).
The issue comes from how tiebreaks for equal work blocks are handled: if two blocks have the same amount of work, the one that is activatable first wins, that is, the one for which we have all its data (and all of its ancestors'). The variable that keeps track of this, within `CBlockIndex` is `nSequenceId`, which is not persisted over restarts. This means that when a node is restarted, all blocks loaded from disk are defaulted the same `nSequenceId`: 0.
Now, when trying to decide what chain is best on loading blocks from disk, the previous tiebreaker rule is not decisive anymore, so the `CBlockIndexWorkComparator` has to default to its last rule: whatever block is loaded first (has a smaller memory address).
This means that if multiple same work tip candidates were available before restarting the node, it could be the case that the selected chain tip after restarting does not match the one before.
Therefore, the way `nSequenceId` is initialized is changed to:
- 0 for blocks that belong to the previously known best chain
- 1 to all other blocks loaded from disk
ACKs for top commit:
sipa:
utACK 0465574c12
TheCharlatan:
ACK 0465574c12
furszy:
Tested ACK 0465574c12.
Tree-SHA512: 161da814da03ce10c34d27d79a315460a9c98d019b85ee35bc5daa991ed3b6a2e69a829e421fc70d093a83cf7a2e403763041e594df39ed1991445e54c16532a
45bd891465 log: split assumevalid ancestry-failure-reason message (Lőrinc)
6c13a38ab5 log: separate script verification reasons (Lőrinc)
f2ea6f04e7 refactor: untangle assumevalid decision branches (Lőrinc)
9bc298556c validation: log initial script verification state (Lőrinc)
4fad4e992c test: add assumevalid scenarios scaffold (Lőrinc)
91ac64b0a6 log: reword `signature validations` to `script verification` in `assumevalid` log (Lőrinc)
Pull request description:
### Summary
Users can encounter cases where script checks are unexpectedly enabled (e.g. after reindex, or when `assumevalid`/`minimumchainwork` gates fail). Without an explicit line, they must infer state from the absence of a message, which is incomplete and error-prone.
The existing "Assuming ancestors of block …" line does not reliably indicate whether script checks are actually enabled, which makes debugging/benchmarking confusing.
### What this changes
We make the initial **script-verification** state explicit and log **why** checks are enabled to avoid confusion.
* Always log the first script-verification state on startup, **before** the first `UpdateTip`.
* Flatten the nested `assumevalid` conditionals into a linear gating sequence for readability.
* Extend the functional test to assert the old behavior with the new reason strings.
This is a **logging-only** test change it shouldn't change any other behavior.
### Example output
The state (with reason) is logged at startup and whenever the reason changes, e.g.:
* `Disabling script verification at block #904336 (000000000000000000014106b2082b1a18aaf3091e8b337c6fed110db8c56620).`
* `Enabling script verification at block #912527 (000000000000000000010bb6aa3ecabd7d41738463b6c6621776c2e40dbe738a): block too recent relative to best header.`
* `Enabling script verification at block #912684 (00000000000000000001375cf7b90b2b86e559d05ed92ca764d376702ead3858): block height above assumevalid height.`
------
Follow-up to https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2329269037
ACKs for top commit:
Eunovo:
re-ACK 45bd891465
achow101:
ACK 45bd891465
hodlinator:
re-ACK 45bd891465
yuvicc:
ACK 45bd891465
andrewtoth:
ACK 45bd891465
ajtowns:
ACK 45bd891465
Tree-SHA512: 58328d7c418a6fe18f1c7fe1dd31955bb6fce8b928b0df693f6200807932eb5933146300af886a80a1d922228d93faf531145186dae55ad4ad1f691970732eca
