50cf6838e6 wallet: rpc: manpage: fix example missing `fee_rate` argument (SomberNight)
Pull request description:
The function signature for the `send` RPC is:
```
send [{"address":amount,...},{"data":"hex"},...] ( conf_target "estimate_mode" fee_rate options version )
```
The last example in the manpage is missing the `fee_rate` arg, but is trying to specify the `options` arg, by index. The parser confuses the intended `options` arg as the missing `fee_rate` arg.
See:
```
$ bitcoin-cli -rpcuser=doggman -rpcpassword=donkey -rpcport=18554 -regtest send '{"bcrt1qusm48zmlzwr32csxdw4ar7atw260h22c9ten9l": 0.1}' 1 economical '{"add_to_wallet": false, "inputs": [{"txid":"0b7e1a471dc948b7a6187936b16e6d7d9833629b2f9dd8a392eb89928f63aaad", "vout":0}]}'
error code: -8
error message:
Cannot specify both conf_target and fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.
```
vs
```
$ bitcoin-cli -rpcuser=doggman -rpcpassword=donkey -rpcport=18554 -regtest send '{"bcrt1qusm48zmlzwr32csxdw4ar7atw260h22c9ten9l": 0.1}' 1 economical null '{"add_to_wallet": false, "inputs": [{"txid":"0b7e1a471dc948b7a6187936b16e6d7d9833629b2f9dd8a392eb89928f63aaad", "vout":0}]}'
{
"psbt": "cHNidP8BAHECAAAAAa2qY4+SieuSo9idL5tiM5h9bW6xNnkYprdIyR1HGn4LAAAAAAD9////AkR2DwQAAAAAFgAUpLDwJu+wFRHLQAgKAb0psk7UVd2AlpgAAAAAABYAFOQ3U4t/E4cVYgZrq9H7q3K0+6lYAAAAAAABAIUCAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/////wQC4wMA/////wLIF6gEAAAAABYAFLMY1zihXrefAA0DA5nld4MCPjkrAAAAAAAAAAAmaiSqIant4vYcP3HR3v0/qZnfo2lTdVxpBol5mWK0i+vYNpdOjPkAAAAAAQEfyBeoBAAAAAAWABSzGNc4oV63nwANAwOZ5XeDAj45KwEIawJHMEQCIElTV4pbUrsPR9qHWcioowVv3QVWHizxwevfD0u/I8YyAiBCY3OzF81PSLM00h4ueQkehYuxDFZu7Jk51iejphKnnwEhA0VKdYVSyBpWoxBwTDOupB58Fi3mEBs+u+OOqEYVd2sZACICA98YLWyH7dBCfXVxe7woiLSTgV1mJN8Zc8KgZ77pVSg+GNBMeT5UAACAAQAAgAAAAIABAAAAbAAAAAAA",
"txid": "625b71b314a6ac4f738634e29dc007cd5edc0427c1ae96ab706d06a62910cea2",
"hex": "02000000000101adaa638f9289eb92a3d89d2f9b6233987d6d6eb1367918a6b748c91d471a7e0b0000000000fdffffff0244760f0400000000160014a4b0f026efb01511cb40080a01bd29b24ed455dd8096980000000000160014e437538b7f13871562066babd1fbab72b4fba9580247304402204953578a5b52bb0f47da8759c8a8a3056fdd05561e2cf1c1ebdf0f4bbf23c6320220426373b317cd4f48b334d21e2e79091e858bb10c566eec9939d627a3a612a79f012103454a758552c81a56a310704c33aea41e7c162de6101b3ebbe38ea84615776b1900000000",
"complete": true
}
```
ACKs for top commit:
svanstaa:
tACK 50cf6838e6
kannapoix:
Tested ACK 50cf6838e6
achow101:
ACK 50cf6838e6
rkrux:
tACK 50cf6838e6
theStack:
Tested ACK 50cf6838e6
Tree-SHA512: 499701729038cd863b612698098a73ce995589fc5ab08a2962f8edf1ff3cb3de6f7090e04722ca13ba7707a566fa3750ae549b6ad55750a3d01127eb6b94a79f
Stop() has two windows where Start() could cause troubles:
1) m_workers is temporarily empty while workers are being joined,
this creates a window where Start() could slip through and reset
m_interrupt to false, preventing the old workers from exiting and
causing a deadlock.
2) Start() could be called after workers are joined but before the
empty() sanity check on m_work_queue, causing a crash.
Fix both races by keeping m_interrupt set for the entire duration
of Stop(), so any concurrent Start() call is rejected until all
workers have exited.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
e8f8b74a46 test: index, improve txospenderindex_initial_sync() test code (furszy)
ac3bea07cd test: improve rpc_gettxspendingprevout.py code (furszy)
Pull request description:
Fixes#34637.
Was reviewing #34637 and, while reading the new txospender index
test code for the first time, found it could use some cleanups. Finding
stuff in there is harder than it should be due to the amount of dup code.
The first commit cleans up `rpc_gettxspendingprevout.py` by introducing
helper functions to avoid repeating the same dicts everywhere, using
for-loops instead of duplicating the same checks for each node, and
renaming variables to better reflect what they actually represent.
The second commit reorganizes `txospenderindex_initial_sync()`
moving index initialization after the test setup phase, since the index
doesn't participate in it anyway. It adds a post-sync check to catch
cases where `Sync()` aborted prematurely.
Note:
This is just a pre-work for deeper index changes I'm cooking.
ACKs for top commit:
achow101:
ACK e8f8b74a46
sedited:
Re-ACK e8f8b74a46
w0xlt:
reACK e8f8b74a46
Tree-SHA512: 3f7026712ab20a43f376afa28c683dcd5daec8ed1bbf1c36d7ec6bbf231f468d4de74efae4aa8295ff3afb83986286ccaf31c03b34e45fc9971652f064791ed0
This fixes ``Assertion `m_node.chainman' failed`` errors first reported
https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602551596 when
IPC mining methods are called before ChainstateManager is loaded.
The fix works by making the `Init.makeMining` method block until chainstate
data is loaded.
Instead of having the InitAndLoadChainstate function delete and create the
KernelNotifications object each time it is called (it can be called twice when
reindexing) to clear cached state, create it just one time and add a
setChainstateLoaded() method to manage state as it is loaded and unloaded.
This refactoring should make sense by itself to be more explicit about how
KernelNotifications state is cleared, but it's also needed to make outside code
accessing KernelNotifications state (currently just mining code) safe during
node startup and shutdown so the KernelNofications mutex can be used for
synchronization and does not get recreated itself.
This parametrizes the cost model for the SFL algorithm with another
class. Right now, the behavior of that class matches the naive cost
model so far, but it will be replaced with a more advanced on in a
future commit.
The reason for abstracting this out is that it makes benchmarking for
creating such cost models easy, by instantiating the cost model class
with one that tracks time.
The index is now initialized after the setup phase (chain generation
and txs creation), since it doesn't participate on it at all.
This improves readability and splits setup from what we actually
want to check.
This also adds a check after Sync() to verify the index best block hash
matches the tip, so we know it fully synced before checking the
processed data. This will help catching errors as Sync() could have
aborted prematurely.
As a happy side effect, the SyncWithValidationInterfaceQueue() call at
the end of the test is no longer needed and has been removed.
There's no change in behavior. This is just a refactoring to avoid a minor
layer violation in init code. The node.init object is intended to return
interface pointers for code outside the node (like wallet and gui code), not
used by node itself to initialize its internal state.
(Motivation for this change is to introduce a MakeMining wait_loaded option in
an upcoming commit that can only be used internally and not set by external
clients.)
a28eedb8c2 ci: use LLVM 22 in sanitizer tasks (fanquake)
Pull request description:
Clang/LLVM 22 is out, and upstream apt issues seem to be resolved. Use it in the sanitizer related tasks in CI. Not changing tidy here (needs more changes).
ACKs for top commit:
maflcko:
lgtm ACK a28eedb8c2
hebasto:
ACK a28eedb8c2.
Tree-SHA512: 311cca7d1ebc5769812e63c1946e8bad1bd2da728bfe3a4b91182eca8c9a9036f7506aff4ea99665982f57c14e4523b674a8689a2390e30936c16ae2548bc759
The error was added in commit 1ea7e45a1f,
because there was an additional confusing `AssertionError: [node 0]
Error: no RPC connection` instead of just a single `FileNotFoundError:
[Errno 2] No such file or directory`.
This is no longer needed on current master.
Also, the test is incomplete, because it was just checking bitcoind and
bitcoin-cli, not any other missing binaries.
Also, after the previous commit, it would not work in combination with
--valgrind.
Instead of trying to make it complete, and work in all combinations,
just remove it, because the already existing error will be clear in any
case.
This can be tested via:
```sh
./test/get_previous_releases.py
mv releases releases_backup
# Confirm the test is skipped due to missing releases
./bld-cmake/test/functional/wallet_migration.py
# Confirm the test fails due to missing releases
./bld-cmake/test/functional/wallet_migration.py --previous-releases
mv releases_backup releases
mv ./releases/v28.2 ./releases/v28.2_backup
# Confirm the test fails with a single FileNotFoundError
./bld-cmake/test/functional/wallet_migration.py
mv ./releases/v28.2_backup ./releases/v28.2
# Confirm the test runs and passes
./bld-cmake/test/functional/wallet_migration.py
rm ./bld-cmake/bin/bitcoind
# Confirm the test fails with a single "No such file or directory",
# testing with and without --valgrind
./bld-cmake/test/functional/wallet_migration.py
./bld-cmake/test/functional/wallet_migration.py --valgrind
```
Prior to this commit, tool_bitcoin.py was failing:
```sh
$ ./bld-cmake/test/functional/tool_bitcoin.py --valgrind
TestFramework (ERROR): Unexpected exception
Traceback (most recent call last):
File "./test/functional/test_framework/test_framework.py", line 138, in main
self.setup()
~~~~~~~~~~^^
File "./test/functional/test_framework/test_framework.py", line 269, in setup
self.setup_network()
~~~~~~~~~~~~~~~~~~^^
File "./test/functional/tool_bitcoin.py", line 38, in setup_network
assert all(node.args[:len(node_argv)] == node_argv for node in self.nodes)
~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError
```
This commit fixes this issue by running `bitcoin` under valgrind. Also,
it comes with other improvements:
* Drop the outdated valgrind 3.14 requirement, because there is no
distro that ships a version that old anymore.
* Drop the VALGRIND_SUPPRESSIONS_FILE env var handling, because it was
presumably never used since it was introduced. Also, the use-case
seems limited.
Review note:
The set_cmd_args was ignoring the --valgrind test option.
In theory, this could be fixed by refactoring Binaries::node_argv() to
be used here. However, for now, just re-implement the node_argv logic in
set_cmd_args to prepend the valgrind cmd.
fcaec2544b doc: release note for IPC cooldown and interrupt (Sjors Provoost)
1e82fa498c mining: add interrupt() (Sjors Provoost)
a11297a904 mining: add cooldown argument to createNewBlock() (Sjors Provoost)
Pull request description:
As reported in #33994, connected mining clients will receive a flood of new templates if the node is still going through IBD or catching up on the last 24 hours. This PR fixes that using an _optional_ cooldown mechanism, only applied to `createNewBlock()`.
First, cooldown waits for IBD. Then, as the tip keeps moving forward, it waits a few seconds to see if the tip updated. If so, it restarts the timer and waits again. The trade-offs for this mechanism are explained below.
Because this PR changes `createNewBlock()` from a method that returns quickly to one that can block for minutes, we rely on #34568 to fix a bug in our `.capnp` definition, adding the missing `context` to `createNewBlock` (and `checkBlock`).
The second commit then adds an `interrupt()` method so that clients can cleanly disconnect.
---
## Rationale
The cooldown argument is optional, and not used by internal non-IPC code, for two reasons:
1. The mechanism wreaks havoc on the functional test suite, which would require very careful mock time handling to work around. But that's pointless, because only IPC clients need it.
2. It needs to be optional for IPC clients too, because in some situations, like a signet with only one miner, waiting for IBD can mean being stuck forever.
The reason it's only applied to `createNewBlock()` is that this is the first method called by clients; `waitNext()` is a method on the interface returned by `createNewBlock()`, at which point the cooldown is done.
After IBD, we wait N seconds if the header is N blocks ahead of the tip, with a minimum of 3 and a maximum of 20 seconds. The minimum waiting time is short enough that it shouldn't be annoying or confusing for someone manually starting up a client. While the maximum should be harmless if it happens spuriously (which it shouldn't).
If the minimum wait is too short, clients get a burst of templates, as observed in the original issue. We can't entirely rule this out without a lot of additional complexity (like scanning our own log file for heuristics). This PR should make it a lot less likely, and thanks to the IBD wait also limit it to one day worth of blocks (`-maxtipage`).
Some test runs on an M4 MacBook Pro, where I had a node catch up on the last few days worth of blocks:
<img width="872" height="972" alt="Schermafbeelding 2026-02-04 om 18 21 17" src="https://github.com/user-attachments/assets/7902a0f2-0e0b-4604-9688-cec2da073261" />
As the chart shows, sometimes it takes longer than 3 seconds. But it turns out that in all those cases there were quite a few headers ahead of the tip. It also demonstrates that it's important to first wait for IBD, because it's less likely a random tip update takes longer than 20 seconds.
- modified sv2-apps: https://github.com/Sjors/sv2-apps/tree/2026/02/cooldown
- test script: https://gist.github.com/Sjors/feb6122c97acc2b9e6d66b168614609c#file-run_mainnet_pool_loop-zsh
- chart script: https://gist.github.com/Sjors/feb6122c97acc2b9e6d66b168614609c#file-tip_interval_charts-py
ACKs for top commit:
ryanofsky:
Code review ACK fcaec2544b. Only changes since last review were removing two cooldown arguments from the mining IPC test to simplify it
enirox001:
ACK fcaec2544b
Tree-SHA512: 08b75470f7c5c80a583a2fdb918fad145e7d5377309e5c599f67fc0d0e3139d09881067ba50c74114f117e69da17ee50666838259491691c031b1feaf050853f
c462e54f9d test: don't always assert NUM_PRIVATE_BROADCAST_PER_TX broadcasts (Vasil Dimov)
3710566305 test: move abortprivatebroadcast test at the end (Vasil Dimov)
Pull request description:
_test: move abortprivatebroadcast test at the end_
The piece of `p2p_private_broadcast.py` which tests the correctness of
`abortprivatebroadcast` issues a new `sendrawtransaction` call. That
call schedules up to 3 new connections: peer=13, peer=14 and possibly
peer=15 before it gets aborted.
These up to 3 in-the-process-of-opening private broadcast connections
have `CNode::m_connected` set early - when the `CNode` object is
created. Later in the test the mock time is advanced by 20 minutes and
those "old" connections pick a transaction for rebroadcast but that
triggers `PRIVATE_BROADCAST_MAX_CONNECTION_LIFETIME` immediately:
```
2026-02-21T13:28:14.209766Z [privbcast] [net.cpp:4006] [CNode] [net] Added connection peer=20
2026-02-21T13:28:14.309792Z (mocktime: 2026-02-21T13:48:14Z) [msghand] [net.cpp:4074] [PushMessage] [net] sending inv (37 bytes) peer=20
2026-02-21T13:28:14.309801Z (mocktime: 2026-02-21T13:48:14Z) [msghand] [net_processing.cpp:5745] [SendMessages] [privatebroadcast] Disconnecting: did not complete the transaction send within 180 seconds, peer=20
```
This prematurely stops the private broadcast connection and results in
a failure like:
```
AssertionError: ... not({} == {'ping': 1, 'tx': 1})
```
---
_test: don't always assert NUM_PRIVATE_BROADCAST_PER_TX broadcasts_
In `p2p_private_broadcast.py` in the function `check_broadcasts()` we
should assert that the broadcast was done to `broadcasts_to_expect`
peers, not to `NUM_PRIVATE_BROADCAST_PER_TX`. This is because in the
"Basic" test we check the first broadcast manually because it is done to
`nodes[1]` and then check the other two by
`check_broadcasts(..., NUM_PRIVATE_BROADCAST_PER_TX - 1, ...)`.
The first broadcast might not have fully concluded by the time we call
`check_broadcasts()` to check the remaining 2.
Demanding always `NUM_PRIVATE_BROADCAST_PER_TX` can lead to:
```
Traceback (most recent call last):
File "/home/vd/gh/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 142, in main
self.run_test()
~~~~~~~~~~~~~^^
File "/tmp/build/clang22/test/functional/p2p_private_broadcast.py", line 347, in run_test
self.check_broadcasts("Basic", txs[0], NUM_PRIVATE_BROADCAST_PER_TX - 1, NUM_INITIAL_CONNECTIONS + 1)
~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/tmp/build/clang22/test/functional/p2p_private_broadcast.py", line 313, in check_broadcasts
assert_greater_than_or_equal(sum(1 for p in peers if "received" in p), NUM_PRIVATE_BROADCAST_PER_TX)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/vd/gh/bitcoin/bitcoin/test/functional/test_framework/util.py", line 94, in assert_greater_than_or_equal
raise AssertionError("%s < %s" % (str(thing1), str(thing2)))
AssertionError: 2 < 3
```
ACKs for top commit:
l0rinc:
ACK c462e54f9d
achow101:
ACK c462e54f9d
andrewtoth:
ACK c462e54f9d
Tree-SHA512: 0de8d0eae079eeedc3bfad39df8129a8fa0d7734bdc03b4fb3e520a2f13a187d68118ffc210556af125d634f0ff51a1b081b34a023ac68a1c6a0caf541cecb91
a9e59f7d95 rpc: add optimal result to getmempoolinfo (Greg Sanders)
a3fb3dd55c mempool: log if we detect a non-optimal mempool (Greg Sanders)
Pull request description:
Post-SFL #34023 I don't think we expect the mempool to be unordered for long periods of time. If we consider it likely to be a serious regression in production, it would be useful to expose the fact that the mempool is not known to be optimal.
1. do a MEMPOOL log after any `DoWork()` returns false, meaning non-optimal
2. expose it via getmempoolinfo, by calling `DoWork(0)`, which does nothing but return known-optimality
I'm not wedded to either approach, I just think something is better than nothing for the next release.
ACKs for top commit:
ajtowns:
ACK a9e59f7d95
ismaelsadeeq:
reACK a9e59f7d95 [c89b93b95..a9e59f7d95](c89b93b958..a9e59f7d95) fixed typo, added more logging for block/reorg additions to mempool, and fixed brittle test case.
sedited:
ACK a9e59f7d95
sipa:
ACK a9e59f7d95
Tree-SHA512: 1560ad21cc1606df7279c102f35f61d4555c0ac920f02208b2a6eb89b14d7e22befb6d7f510a00a9074c2f9931f32e9af86bcea3a8dd9a1d947b0398c84666dd
Modify fuzzer logic to avoid setting `FRESH` for an outpoint that already exists unspent in the parent view, and ensure `FRESH` implies `DIRTY`.
This keeps cursor invariants realistic and lets `BatchWrite` failures expose real bugs without resetting state.
The coins view fuzzer can call `AddCoin` with `possible_overwrite=false` for an outpoint that already exists unspent in the view, which violates the `AddCoin` caller contract.
Derive `possible_overwrite` from `PeekCoin` so `possible_overwrite=false` is only used when the outpoint is absent.
This matches the approach used by the `coinscache_sim` fuzzer, which derives the overwrite flag from simulated state.
In validation, `AddCoins(check_for_overwrite=false)` is only used after BIP30 has already ensured the transaction does not overwrite any unspent outputs in the UTXO view.
The coins view fuzz target can call `AddCoins` with arbitrary txids, so using the `check_for_overwrite=false` fast path on non-coinbase transactions may violate the `AddCoin` caller contract and trigger logic errors.
Only use `check_for_overwrite=false` when we have first confirmed that none of the outputs are currently unspent.
Otherwise, fall back to `check_for_overwrite=true` so `AddCoins` determines overwrites via the view.
Introduce `TrySub(T&, U)` which subtracts an unsigned integral `U` from an unsigned integral `T`, returning `false` on underflow.
Use with `Assume(TrySub(...))` at coins cache accounting decrement sites so invariant violations fail immediately rather than silently wrapping.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Grouped changes to improve the overall readability and maintainability of the test.
A lot more can be done, but this is a good first step.
1) Use for-loops instead of duplicating lines to perform the same checks for each
node.
2) The {'txid': x, 'vout': y} dict is repeated everywhere in the test, both as
input to gettxspendingprevout and as part of its result when an output has no
known spender, making the test tedious to read and maintain.
This introduces a prevout(txid, vout) query helper and an unspent_out(txid, vout)
result helper to reduce the repetition. These two helpers are intentionally kept
separate to make it immediately clear whether a dict is an input to
gettxspendingprevout or an assertion on its result.
3) The same repetition problem mentioned above applies to other gettxspendingprevout
possible results:
Spent outputs returns {'txid': x, 'vout': y, 'spendingtxid': z} and
Spent outputs when requesting spending tx returns {'txid': x, 'vout': y,
'spendingtxid': z, 'blockhash': w, 'spendingtx': v}
To fix it, this introduces:
- spent_out(txid, vout, spending_tx_id): for outputs with a known spender
- spent_out_in_block(txid, vout, spending_tx_id, blockhash, spending_tx): for
outputs spent in a confirmed block, when full tx data is requested
4) Rename overloaded confirmed_utxo variable (used in three different tests) to more
descriptive names: root_utxo, reorg_replace_utxo, reorg_cancel_utxo to clarify
their roles in each of the tests.
I2P addresses must use port=0, otherwise `bitcoind` refuses to connect.
The test `p2p_private_broadcast.py` cannot simulate connections to I2P
peers, so the I2P proxy is set to a dummy `127.0.0.1:1`. Still it is
good to pick I2P addresses and attempt connections to increase coverage.
However the test uses port=8333 for I2P addresses and thus the
connection attempts fail for the "wrong" reason:
```
Error connecting to ...i2p:8333, connection refused due to arbitrary port 8333
```
Using the proper port=0 makes the failures:
```
Error connecting to ...i2p:0: Cannot connect to 127.0.0.1:1
```
which will make possible simulated I2P connections once we have a test
I2P proxy.
If the following two events happen:
* (likely) the automatic 10 initial connections are not made to all
networks
* (unlikely) the network-specific logic kicks in almost immediately.
It is using exponential distribution with a mean of 5 minutes
(`rng.rand_exp_duration(EXTRA_NETWORK_PEER_INTERVAL)`).
So if both happen, then the 11th connection may not be the expected
private broadcast, but a network-specific connection.
Fix this by retrieving the connection type from
`destinations_factory()`. This is more flexible because it allows
connections to happen in any order and does not break if e.g. the 11th
connection is not the expected first private broadcast.
This also makes the test run faster:
before: 19-44 sec
now: 10-25 sec
because for example there is no need to wait for the initial 10
automatic outbound connections to be made in order to proceed.
Fixes: https://github.com/bitcoin/bitcoin/issues/34387
When `git range-diff` cannot match a commit between two versions of a branch, it shows the old commit as removed (`<`) and the new commit as added (`>`), and it does not show the patch contents.
This output is easy to misread as "no code changes" because the diff for that commit is effectively empty.
It really means the commits were considered unrelated and the new commit should be reviewed from scratch.
This is analogous to rename detection in `git diff`: if similarity is too low, a rename shows up as delete+add.
Example (exact SHAs from PR #34320):
B=ff338fdb53a66ab40a36e1277e7371941fc89840; A=dd76338a57b9b1169ac27f7b783d6d0d4c6e38ab; git fetch upstream $B $A
git range-diff 0ca4295f2e5f4443a1f8b3bae7cba0f6c054276f..$B 139aa4b27e4839291c83a04dcd1649c5595814ca..$A
This produced output like:
1: 4b32181dbb < -: ---------- test: add `HaveInputs` call-path unit tests
-: ---------- > 1: 277c57f0c5 test: add `HaveInputs` call-path unit tests
Even though the subject matches, the first commit had no matching diff shown.
That should be treated as "unmatched" rather than "unchanged".
If you expected a match, try increasing the creation factor so `git range-diff` searches harder:
git range-diff --creation-factor=95 <old_range> <new_range>
This avoids wasting work on calculating bloom filters that aren't
consumed during ibd and continuously re-calculated as now blocks get
validated.
Also update the functional test to document that transactions would now
be requested again once out of IBD.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
This documents existing behaviour before the change in the following
commit: The bloom filter maintained by the txdownload manager tracks
recently confirmed transasctions even during ibd. If a peer sends an INV
once IBD is over it does not re-request them.
Co-authored-by: sedited <seb.kung@gmail.com>
In `p2p_private_broadcast.py` in the function `check_broadcasts()` we
should assert that the broadcast was done to `broadcasts_to_expect`
peers, not to `NUM_PRIVATE_BROADCAST_PER_TX`. This is because in the
"Basic" test we check the first broadcast manually because it is done to
`nodes[1]` and then check the other two by
`check_broadcasts(..., NUM_PRIVATE_BROADCAST_PER_TX - 1, ...)`.
The first broadcast might not have fully concluded by the time we call
`check_broadcasts()` to check the remaining 2.
Demanding always `NUM_PRIVATE_BROADCAST_PER_TX` can lead to:
```
Traceback (most recent call last):
File "/home/vd/gh/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 142, in main
self.run_test()
~~~~~~~~~~~~~^^
File "/tmp/build/clang22/test/functional/p2p_private_broadcast.py", line 347, in run_test
self.check_broadcasts("Basic", txs[0], NUM_PRIVATE_BROADCAST_PER_TX - 1, NUM_INITIAL_CONNECTIONS + 1)
~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/tmp/build/clang22/test/functional/p2p_private_broadcast.py", line 313, in check_broadcasts
assert_greater_than_or_equal(sum(1 for p in peers if "received" in p), NUM_PRIVATE_BROADCAST_PER_TX)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/vd/gh/bitcoin/bitcoin/test/functional/test_framework/util.py", line 94, in assert_greater_than_or_equal
raise AssertionError("%s < %s" % (str(thing1), str(thing2)))
AssertionError: 2 < 3
```
The piece of `p2p_private_broadcast.py` which tests the correctness of
`abortprivatebroadcast` issues a new `sendrawtransaction` call. That
call schedules up to 3 new connections: peer=13, peer=14 and possibly
peer=15 before it gets aborted.
These up to 3 in-the-process-of-opening private broadcast connections
have `CNode::m_connected` set early - when the `CNode` object is
created. Later in the test the mock time is advanced by 20 minutes and
those "old" connections pick a transaction for rebroadcast but that
triggers `PRIVATE_BROADCAST_MAX_CONNECTION_LIFETIME` immediately:
```
2026-02-21T13:28:14.209766Z [privbcast] [net.cpp:4006] [CNode] [net] Added connection peer=20
2026-02-21T13:28:14.309792Z (mocktime: 2026-02-21T13:48:14Z) [msghand] [net.cpp:4074] [PushMessage] [net] sending inv (37 bytes) peer=20
2026-02-21T13:28:14.309801Z (mocktime: 2026-02-21T13:48:14Z) [msghand] [net_processing.cpp:5745] [SendMessages] [privatebroadcast] Disconnecting: did not complete the transaction send within 180 seconds, peer=20
```
This prematurely stops the private broadcast connection and results in
a failure like:
```
AssertionError: ... not({} == {'ping': 1, 'tx': 1})
```
4b53cbd692 test: Test for musig() in various miniscript expressions (Ava Chow)
ec0f47b15c miniscript: Using Func and Expr when parsing keys, hashes, and locktimes (Ava Chow)
6fd780d4fb descriptors: Increment key_exp_index in ParsePubkey(Inner) (Ava Chow)
b12281bd86 miniscript: Use a reference to key_exp_index in KeyParser (Ava Chow)
ce4c66eb7c test: Test that key expression indexes match key count (Ava Chow)
Pull request description:
The miniscript parser currently only looks for the next `)` when parsing key, hash, and locktime expressions. This fails to parse when the expressions contain a nested expression. Currently, this is only possible with `musig()` inside of key expressions. However, this pattern can be generalized to handling hashes and locktimes, so I implemented those too.
Fixes#34076
ACKs for top commit:
rkrux:
ACK 4b53cbd692
sipa:
ACK 4b53cbd692
darosior:
Other than that, Approach ACK 4b53cbd692. That makes sense to me but i have not closely reviewed the code.
Tree-SHA512: 01040c7b07a59d8e3725ff11ab9543b256aea22535fb94059f490a5bb45319e859666af04c2f0a4edcb8cf1e6dfc7bd8a8271b21ad81143bafccd4d0a39cae9c
fd06157d14 test: Add coverage for restarted node without any block sync (Fabian Jahr)
3d7ab7ecb7 rpc, test: Address feedback from #29668 (Fabian Jahr)
312919c9dd test: Indices can not start based on block data without undo data (Fabian Jahr)
a9a3b29dd6 index: Check availability of undo data for indices (Fabian Jahr)
881ab4fc82 support multiple block status checks in CheckBlockDataAvailability (furszy)
Pull request description:
Currently, we check that `BLOCK_HAVE_DATA` is available for all blocks an index needs to sync during startup. However, for `coinstatsindex` and `blockfilterindex` we also need the undo data for these blocks. If that data is missing in the blocks, we are currently still starting to sync each of these indices and then crash later when we encounter the missing data.
This PR adds explicit knowledge of which block data is needed for each index and then checks its availability during startup before initializing the sync process on them.
This also addresses a few open comments from #29668 in the last commit.
ACKs for top commit:
achow101:
ACK fd06157d14
furszy:
utACK fd06157d14
sedited:
Re-ACK fd06157d14
Tree-SHA512: e2ed81c93372b02daa8ddf2819df4164f96d92de05b1d48855410ecac78d5fcd9612d7f0e63a9d57d7e75a0b46e1bea278e43ea87f2693af0220d1f9c600e416
231dd04b8d build: define CMAKE_COMPILE_WARNING_AS_ERROR as a cache option (will)
Pull request description:
`CMAKE_COMPILE_WARNING_AS_ERROR` is not a cache variable by default in CMake, so it has no value in the configure summary when not set, and even when set cannot be toggled in `ccmake`. Define it as an `option()` to make it a cache BOOL with a default of OFF.
From the original MR to cmake, this was deliberately not set as a cache variable: https://gitlab.kitware.com/cmake/cmake/-/merge_requests/7187 (see Brad King's reply to the collapsed comments from Marc Chevrier).
Most `CMAKE_*` variables which are expected to be toggled by users are (as far as I can research) cache variables by default. Those that are considered likely to be set by the project (e.g. `CMAKE_CXX_STANDARD` or `CMAKE_POSITION_INDEPENDENT_CODE`) are not, along with read-only variables, script/internal variables, platform sppecific variables, template variables. `CMAKE_COMPILE_WARNING_AS_ERROR` may be a slight outlier here.
I count ~ 600 documented CMAKE_* variables, of which ~ 60 are default cache variables.
I could only see a few of these like:
- CMAKE_COMPILE_WARNING_AS_ERROR
- CMAKE_CXX_STANDARD,
- CMAKE_POSITION_INDEPENDENT_CODE
- CMAKE_INTERPROCEDURAL_OPTIMIZATION
...that we (or any project) _might_ want to expose as user-togglable, and would have to add as an `option()` in CMakeLists.txt.
ACKs for top commit:
ajtowns:
Yeah, seems to work for me. ACK 231dd04b8d
fanquake:
ACK 231dd04b8d
hebasto:
ACK 231dd04b8d, tested on Fedora 43.
Tree-SHA512: e380d79188859c97a9f7e45215a789daff55b8bff945a3d61fe4de3daae54bb7196aca0f6a6f1165979e787238dc26e94d217cff35f9642031ff72f4b39655e1
Both waitTipChanged() and createNewBlock() can take a long time to
return. Add a way for clients to interrupt them.
The new m_interrupt_mining is safely accessed with a lock on
m_tip_block_mutex, but it has no guard annotation. A more thorough
solution is discussed here:
https://github.com/bitcoin/bitcoin/pull/34184#discussion_r2743566474
At startup, if the needs to catch up, connected mining clients will
receive a flood of new templates as new blocks are connected.
Fix this by adding a cooldown argument to createNewBlock(). When set
to true, block template creation is briefly paused while the best
header chain is ahead of the tip.
This wait only happens when the best header extends the current tip,
to ignore competing branches.
Additionally, cooldown waits for isInitialBlockDownload() to latch to
false, which happens when there is less than a day of blocks left to sync.
When cooldown is false createNewBlock() returns immediately. The argument
is optional, because many tests are negatively impacted by this
mechanism, and single miner signets could end up stuck if no block
was mined for a day.
The getblocktemplate RPC also opts out, because it would add a delay
to each call.
Fixes#33994
We expect this to be rare in practice, and to not be the
usual state of the mempool. If we we detect non-optimal
ordering after a DoWork() invocation, allow this to be
observed in MEMPOOL logs.
f700609e8a doc: Release notes for mining IPC interface bump (Ryan Ofsky)
9453c15361 ipc mining: break compatibility with existing clients (version bump) (Sjors Provoost)
70de5cc2d2 ipc mining: pass missing context to BlockTemplate methods (incompatible schema change) (Sjors Provoost)
2278f017af ipc mining: remove deprecated methods (incompatible schema change) (Ryan Ofsky)
c6638fa7c5 ipc mining: provide default option values (incompatible schema change) (Ryan Ofsky)
a4603ac774 ipc mining: declare constants for default field values (Ryan Ofsky)
ff995b50cf ipc test: add workaround to block_reserved_weight exception test (Ryan Ofsky)
b970cdf20f test framework: expand expected_stderr, expected_ret_code options (Ryan Ofsky)
df53a3e5ec rpc refactor: stop using deprecated getCoinbaseCommitment method (Ryan Ofsky)
Pull request description:
This PR increments the field number of the `Init.makeMining` method and makes the old `makeMining` method return an error, so IPC mining clients not using the latest schema file will get an error and not be able to access the Mining interface.
Normally, there shouldn't be a need to break compatibility this way, but the mining interface has evolved a lot since it was first introduced, with old clients using the original methods less stable and performant than newer clients. So now is a good time to introduce a cutoff, drop deprecated methods, and stop supporting old clients which can't function as well.
Bumping the field number is also an opportunity to make other improvements that would be awkward to implement compatibly:
- Making Cap'n Proto default parameter and field values match default values of corresponding C++ methods and structs.
- Adding missing Context parameters to Mining.createNewBlock and checkBlock methods so these methods will be executed on separate execution threads and not block the Cap'n Proto event loop thread.
More details about these changes are in the commit messages.
ACKs for top commit:
Sjors:
ACK f700609e8a
enirox001:
ACK f700609e8a
ismaelsadeeq:
ACK f700609e8a
sedited:
ACK f700609e8a
Tree-SHA512: 0901886af00214c138643b33cec21647de5671dfff2021afe06d78dfd970664a844cde9a1e28f685bb27edccaf6e0c3f2d1e6bb4164bde6b84f42955946e366d
0b96b9c600 Minimize mempool lock, sync txo spender index only when and if needed (sstone)
3d82ec5bdd Add a "tx output spender" index (sstone)
Pull request description:
This PR adds a new "tx output spender" index, which allows users to query which tx spent a given outpoint with the `gettxspendingprevout` RPC call that was added by https://github.com/bitcoin/bitcoin/pull/24408.
Such an index would be extremely useful for Lightning, and probably for most layer-2 protocols that rely on chains of unpublished transactions.
UPDATE: this PR is ready for review and issues have been addressed:
- using a watch-only wallet instead would not work if there is a significant number of outpoints to watch (see https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-1276595646)
- this PR does not require `-txindex` anymore
We use a composite key with 2 parts (suggested by romanz): hash(spent outpoint) and tx position, with an empty value. Average composite key size is 15 bytes.
The spending tx can optionally be returned by `gettxspendingprevout` (even it `-txindex is not set`).
ACKs for top commit:
hodlinator:
re-ACK 0b96b9c600
sedited:
Re-ACK 0b96b9c600
fjahr:
ACK 0b96b9c600
w0xlt:
reACK 0b96b9c600
Tree-SHA512: 95c2c313ef4086e7d5bf1cf1a3c7b91cfe2bb1a0dcb4c9d3aa8a6e5bfde66aaca48d85a1f1251a780523c3e4356ec8a97fe6f5c7145bc6ccb6f820b26716ae01
