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
de0675f9de refactor: Move `transaction_identifier.h` to primitives (marcofleon)
6f068f65de Remove implicit uint256 conversion and comparison (marcofleon)
9c24cda72e refactor: Convert remaining instances from uint256 to Txid (marcofleon)
d2ecd6815d policy, refactor: Convert uint256 to Txid (marcofleon)
f6c0d1d231 mempool, refactor: Convert uint256 to Txid (marcofleon)
aeb0f78330 refactor: Convert `mini_miner` from uint256 to Txid (marcofleon)
326f244724 refactor: Convert RPCs and `merkleblock` from uint256 to Txid (marcofleon)
49b3d3a92a Clean up `FindTxForGetData` (marcofleon)
Pull request description:
This is the final leg of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
All of these changes are straightforward `uint256` --> `Txid` along with any necessary explicit conversions. Also, `transaction_identifier.h` is moved to primitives in the last commit, as `Txid` and `Wtxid` become fundamental types after this PR.
ACKs for top commit:
stickies-v:
re-ACK de0675f9de, no changes since a20724d926d5844168c6a13fa8293df8c8927efe except address review nits.
janb84:
re ACK de0675f9de
dergoegge:
re-ACK de0675f9de
theStack:
Code-review ACK de0675f9de
Tree-SHA512: 2413160fca7ab146a8d79d18ce3afcf7384cacc73c513d41928904aa453b4dd7a350064cee71e9c5d015da5904c7c81ac17603e50a47441ebc5b0c653235dd08
3543bfdfec test: Fix 'getdescriptoractivity' RPCHelpMan, add test to verify 'spend_vin' is the correct field (Chris Stewart)
Pull request description:
Fixes bug in `getdescriptoractivity` RPC help manual.
Here is the line that pushes `spend_vin` field, there is no `spend_vout` json field.
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/blockchain.cpp#L2757
ACKs for top commit:
nervana21:
tACK 3543bfd
luke-jr:
utACK 3543bfdfec
jonatack:
ACK 3543bfdfec
Tree-SHA512: 2cd543569a87261d8d804d9afe36f8e8ead55839c01da9c4831aea3ced7d1251e6885621e628898105700aae4d76cbb8a682f518f33c1c52163e66f75ec87a61
1252eeb997 rpc: fix getpeerinfo ping duration unit docs (0xb10c)
Pull request description:
The docs have been incorrect since a3789c700b (released in v25; master since Sept. 2022). Noticed while setting up monitoring using getpeerinfo.
0cb1ed2b7c/src/rpc/net.cpp (L249-L257)
ACKs for top commit:
luke-jr:
utACK 1252eeb997
maflcko:
lgtm ACK 1252eeb997
jonatack:
ACK 1252eeb997
theStack:
ACK 1252eeb997
janb84:
ACK 1252eeb997
Tree-SHA512: 33f576336b2a4d9533f51f4641d564ee59ef692c5fa9a3cad239fc31465883d5da534bfd0e069be1e1d688e5f0dea3fe6850be19bf35335041b8f414d08f7f09
The getpeerinfo docs incorrectly specified the ping durations as
milliseconds. This was incorrectly changed in a3789c700b
(released in v25; master since Sept. 2022). The correct duration unit
is seconds.
Also, remove the documentation of the getpeerinfo RPC response from the
ping RPC since it's incomplete. Better to just reference the getpeerinfo
RPC and it's documenation for this.
Currently this code is not called in unit tests. Calling should make it
possible to write tests for things like IPC exceptions being thrown during
shutdown.
c0642e558a [fuzz] fix latency score check in txorphan_protected (glozow)
3d4d4f0d92 scripted-diff: rename "ann" variables to "latency_score" (monlovesmango)
3b92448923 [doc] comment fixups for orphanage changes (glozow)
1384dbaf6d [config] emit warning for -maxorphantx, but allow it to be set (glozow)
b10c55b298 fix up TxOrphanage lower_bound sanity checks (glozow)
cfd71c6704 scripted-diff: rename TxOrphanage outpoints index (glozow)
edb97bb3f1 [logging] add logs for inner loop of LimitOrphans (glozow)
8a58d0e87d scripted-diff: rename OrphanTxBase to OrphanInfo (glozow)
cc50f2f0df [cleanup] replace TxOrphanage::Size() with CountUniqueOrphans (glozow)
ed24e01696 [optimization] Maintain at most 1 reconsiderable announcement per wtxid (Pieter Wuille)
af7402ccfa [refactor] make TxOrphanage keep itself trimmed (glozow)
d1fac25ff3 [doc] 31829 release note (glozow)
Pull request description:
Followup to #31829:
- Release notes
- Have the orphanage auto-trim itself whenever necessary (and test changes) https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169508690
- Reduce duplicate reconsiderations by keeping track of which txns are already reconsiderable so we only mark it for reconsideration for 1 peer at a time https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3001627814
- Rename `OrphanTxBase` to `OrphanInfo`
- Get rid of `Size()` method by replacing all calls with `CountUniqueOrphans`
- Rename outpoints index since they point to wtxids, not iterators https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205557613
- Add more logging in the `LimitOrphans` inner loop to make it easy to see which peers are being trimmed https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3074385460
ACKs for top commit:
sipa:
utACK c0642e558a
marcofleon:
Nice, ACK c0642e558a
Tree-SHA512: f298eae92cf906ed5e4f15a24eeffa7b9e620bcff457772cd77522dd9f0b3b183ffc976871b1b0e6fe93009e64877d518e53d4b9e186e0df58fc16d17f6de90a
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.
c6e2c31c55 rpc: unhide waitfor{block,newblock,blockheight} (Sjors Provoost)
0786b7509a rpc: add optional blockhash to waitfornewblock (Sjors Provoost)
Pull request description:
The `waitfornewblock` is inherently racy as the tip may have changed since the last RPC call, and can even change during initial processing of this call.
Add an optional `blockhash` argument so the caller can specify their current tip. Return immediately if our tip is different.
I've made it fail if `LookupBlockIndex` fails. This should never happen if the user got the block hash from our RPC in the first place.
Finally, the `waitfor{block,newblock,blockheight}` RPC methods are no longer hidden in `help`:
- the changes in #30409 ensured these methods _could_ work in the GUI
- #31785 removed the guards that prevented GUI users from using them
- this PR makes `waitfornewblock` reliable
So there's no more reason to hide them.
ACKs for top commit:
TheCharlatan:
Re-ACK c6e2c31c55
ryanofsky:
Code review ACK c6e2c31c55. Just rebased and tweaked documentation since last review.
glozow:
utACK c6e2c31c55
Tree-SHA512: 84a0c94cb9a2e4449e7a395cf3dce1650626bd852e30e0e238a1aafae19d57bf440bfac226fd4da44eaa8d1b2fa4a8c1177b6c716235ab862a72ff5bf8fc67ac
c5c1960f93 doc: Add release notes for changes in RPCs (pablomartin4btc)
90fd5acbe5 rpc, test: Fix error message in getdescriptoractivity (pablomartin4btc)
39fef1d203 test: Add missing logging info for each test (pablomartin4btc)
53ac704efd rpc, test: Fix error message in unloadwallet (pablomartin4btc)
1fc3a8e8e7 rpc, test: Add EnsureUniqueWalletName tests (pablomartin4btc)
b635bc0896 rpc, util: Add EnsureUniqueWalletName (pablomartin4btc)
Pull request description:
Currently, `unloadwallet` RPC call fails with a JSON parsing error when no `wallet_name` argument is provided. This behavior is misleading because the error originates from a low-level JSON type mismatch, rather than clearly indicating that the wallet name or RPC endpoint (`-rpcwallet=...`) is missing. Also, found out that the [issue](https://github.com/bitcoin/bitcoin/pull/13111#issuecomment-398831543) was noticed during its implementation but never addressed.
In addition, I've verified all RPC commands calls finding that `getdescriptoractivity` had the same problem, but related to the array input types (blockhashes & descriptors), so I've corrected that RPC as well. For consistency I've added the missing logging info for each test case in `test/functional/rpc_getdescriptoractivity.py` in preparation for the new test.
**_-Before_**
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc unloadwallet
error code: -3
error message:
JSON value of type number is not of expected type string
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity
error code: -3
error message:
JSON value of type null is not of expected type array
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity '[]'
error code: -3
error message:
JSON value of type null is not of expected type array
```
**_-After_**
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc unloadwallet
error code: -8
error message:
Either the RPC endpoint wallet or the wallet name parameter must be provided
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity
error code: -1
error message:
getdescriptoractivity ["blockhash",...] [scanobjects,...] ( include_mempool )
Get spend and receive activity associated with a set of descriptors for a set of blocks. This command pairs well with the `relevant_blocks` output of `scanblocks()`.
This call may take several minutes. If you encounter timeouts, try specifying no RPC timeout (bitcoin-cli -rpcclienttimeout=0)
Arguments:
1. blockhashes (json array, required) The list of blockhashes to examine for activity. Order doesn't matter. Must be along main chain or an error is thrown.
[
"blockhash", (string) A valid blockhash
...
]
2. scanobjects (json array, required) Array of scan objects. Every scan object is either a string descriptor or an object:
[
"descriptor", (string) An output descriptor
{ (json object) An object with output descriptor and metadata
"desc": "str", (string, required) An output descriptor
"range": n or [n,n], (numeric or array, optional, default=1000) The range of HD chain indexes to explore (either end or [begin,end])
},
...
]
3. include_mempool (boolean, optional, default=true) Whether to include unconfirmed activity
...
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity '[]'
error code: -1
error message:
getdescriptoractivity ["blockhash",...] [scanobjects,...] ( include_mempool )
...
```
ACKs for top commit:
achow101:
ACK c5c1960f93
stickies-v:
re-ACK c5c1960f93
furszy:
ACK c5c1960f93
Tree-SHA512: e831ff1acbfd15d2ce3a69bb408cce94664c0b63b2aa2f4627a05c6c052241ae3b5cc238219ef1b30afb489a4a3f4c3030e2168b0c8f08b4d20805d050d810f5
1cb2399703 doc: clarify the GetAddresses/GetAddressesUnsafe documentation (Daniela Brozzoni)
e5a7dfd79f p2p: rename GetAddresses -> GetAddressesUnsafe (Daniela Brozzoni)
Pull request description:
Rename GetAddresses to GetAddressesUnsafe to make it clearer that this function should only be used in trusted contexts. This helps avoid accidental privacy leaks by preventing the uncached version from being used in non-trusted scenarios, like P2P.
Additionally, better reflect in the documentation that the two methods should be used in different contexts.
Also update the outdated "call the function without a parameter" phrasing in the cached version. This wording was accurate when the cache was introduced in #18991, but became outdated after later commits (f26502e9fc, 81b00f8780) added parameters to each
function, and the previous commit changed the function naming completely.
ACKs for top commit:
stickies-v:
re-ACK 1cb2399703
l0rinc:
ACK 1cb2399703
luisschwab:
ACK 1cb2399703
brunoerg:
ACK 1cb2399703
theStack:
Code-review ACK 1cb2399703
mzumsande:
Code review ACK 1cb2399703
Tree-SHA512: 02c05d88436abcdfabad994f47ec5144e9ba47668667a2c1818f57bf8710727505faf8426fd0672c63de14fcf20b96f17cea2acc39fe3c1f56abbc2b1a9e9c23
Mark blockhashes and scanobjects arguments as required, so the user receives
a clear help message when either is missing.
Added a new functional test for this use case.
Co-authored-by: stickies-v <stickies-v@users.noreply.github.com>
Rename GetAddresses to GetAddressesUnsafe to make it clearer that this
function should only be used in trusted contexts. This helps avoid
accidental privacy leaks by preventing the uncached version from being
used in non-trusted scenarios, like P2P.
Expiry is going away in a later commit.
This is only an RPC change. Behavior of the orphanage does not change.
Note that getorphantxs is marked experimental.
a60f863d3e scripted-diff: Replace GenTxidVariant with GenTxid (marcofleon)
c8ba199598 Remove old GenTxid class (marcofleon)
072a198ea4 Convert remaining instances of GenTxid to GenTxidVariant (marcofleon)
1b528391c7 Convert `txrequest` to GenTxidVariant (marcofleon)
bde4579b07 Convert `txdownloadman_impl` to GenTxidVariant (marcofleon)
c876a892ec Replace GenTxid with Txid/Wtxid overloads in `txmempool` (marcofleon)
de858ce2be move-only: make GetInfo a private CTxMemPool member (stickies-v)
eee473d9f3 Convert `CompareInvMempoolOrder` to GenTxidVariant (marcofleon)
243553d590 refactor: replace get_iter_from_wtxid with GetIter(const Wtxid&) (stickies-v)
fcf92fd640 refactor: make CTxMemPool::GetIter strongly typed (marcofleon)
11d28f21bb Implement GenTxid as a variant (marcofleon)
Pull request description:
Part of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
This PR changes the GenTxid class to a variant, which holds both Txids and Wtxids. This provides compile-time type safety and eliminates the manual type check (bool m_is_wtxid). Variables that can be either a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256.
ACKs for top commit:
w0xlt:
ACK a60f863d3e
dergoegge:
Code review ACK a60f863d3e
maflcko:
review ACK a60f863d3e🎽
theStack:
Code-review ACK a60f863d3e
Tree-SHA512: da9b73b7bdffee2eb9281a409205519ac330d3336094d17681896703fbca8099608782c9c85801e388e4d90af5af8abf1f34931f57bbbe6e9674d802d6066047
fcfd3db563 remove RPCTimerInterface and RPCRunLater (Matthew Zipkin)
8a1765795f use WalletContext scheduler for walletpassphrase callback (Matthew Zipkin)
Pull request description:
This removes the dependency on libevent for events scheduled by RPC commands, like re-locking a wallet some time after decryption with walletpassphrase. Since walletpassphrase is currently the only RPC that does this, `RPCRunLater`, `RPCTimerInterface` and all related methods are left unused, and deleted in the second commit. Any future RPC that needs to execute a callback in the future can follow the pattern in this PR and just use a scheduler from node or wallet context.
This is an alternative approach to #32796, described in https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3014309449
ACKs for top commit:
fjahr:
Code Review ACK fcfd3db563
achow101:
ACK fcfd3db563
furszy:
ACK fcfd3db563
Tree-SHA512: 04f5e9c3f73f598c3d41d6e35bb59c64c7b93b03ad9fce3c40901733147ce7764f41f475fef1527d44af18f722759996a31ca83b48cb52153795d5022fecfd14
fa946520d2 refactor: Use structured binding for-loop (MarcoFalke)
eeeec1579e rpc: Use type-safe exception to pass RPC help (MarcoFalke)
Pull request description:
The current "catch-all" `catch (const std::exception& e)` in `CRPCTable::help` is problematic, because it could catch exceptions unrelated to passing the help string up.
Fix this by using a dedicated exception type.
ACKs for top commit:
l0rinc:
tested ACK fa946520d2 (edited)
achow101:
ACK fa946520d2
rkrux:
re-ACK fa946520d2
Tree-SHA512: 23dac6e0fe925561bfbf421e6a7441d546eed8c1492ac41ca4ed7dfcd12f4d2ef39c35f105a0291aac511365d98f08fbdc9a4f0bf627172873b8f23c2be45e76
c10e382d2a flatfile: check whether the file has been closed successfully (Vasil Dimov)
4bb5dd78ea util: check that a file has been closed before ~AutoFile() is called (Vasil Dimov)
8bb34f07df Explicitly close all AutoFiles that have been written (Vasil Dimov)
a69c4098b2 rpc: take ownership of the file by WriteUTXOSnapshot() (Hodlinator)
Pull request description:
`fclose(3)` may fail to flush the previously written data to disk, thus a failing `fclose(3)` is as serious as a failing `fwrite(3)`.
Previously the code ignored `fclose(3)` failures. This PR improves that by changing all users of `AutoFile` that use it to write data to explicitly close the file and handle a possible error.
---
Other alternatives are:
1. `fflush(3)` after each write to the file (and throw if it fails from the `AutoFile::write()` method) and hope that `fclose(3)` will then always succeed. Assert that it succeeds from the destructor 🙄. Will hurt performance.
2. Throw nevertheless from the destructor. Exception within the exception in C++ I think results in terminating the program without a useful message.
3. (this is implemented in the latest incarnation of this PR) Redesign `AutoFile` so that its destructor cannot fail. Adjust _all_ its users 😭. For example, if the file has been written to, then require the callers to explicitly call the `AutoFile::fclose()` method before the object goes out of scope. In the destructor, as a sanity check, assume/assert that this is indeed the case. Defeats the purpose of a RAII wrapper for `FILE*` which automatically closes the file when it goes out of scope and there are a lot of users of `AutoFile`.
4. Pass a new callback function to the `AutoFile` constructor which will be called from the destructor to handle `fclose()` errors, as described in https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2243842400. My thinking is that if that callback is going to only log a message, then we can log the message directly from the destructor without needing a callback. If the callback is going to do more complicated error handling then it is easier to do that at the call site by directly calling `AutoFile::fclose()` instead of getting the `AutoFile` object out of scope (so that its destructor is called) and inspecting for side effects done by the callback (e.g. set a variable to indicate a failed `fclose()`).
ACKs for top commit:
l0rinc:
ACK c10e382d2a
achow101:
ACK c10e382d2a
hodlinator:
re-ACK c10e382d2a
Tree-SHA512: 3994ca57e5b2b649fc84f24dad144173b7500fc0e914e06291d5c32fbbf8d2b1f8eae0040abd7a5f16095ddf4e11fe1636c6092f49058cda34f3eb2ee536d7ba
This allows adding a GetIter(const Wtxid&) overload in a next
commit, making it easier to visit this function from a variant.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
a18e572328 test: more template verification tests (Sjors Provoost)
10c908808f test: move gbt proposal mode tests to new file (Sjors Provoost)
94959b8dee Add checkBlock to Mining interface (Sjors Provoost)
6077157531 ipc: drop BlockValidationState special handling (Sjors Provoost)
74690f4ed8 validation: refactor TestBlockValidity (Sjors Provoost)
Pull request description:
This PR adds the IPC equivalent of the `getblocktemplate` RPC in `proposal` mode.
In order to do so it has `TestBlockValidity` return error reasons as a string instead of `BlockValidationState`. This avoids complexity in IPC code for handling the latter struct.
The new Mining interface method is used in `miner_tests`.
It's not used by the `getblocktemplate` and `generateblock` RPC calls, see https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2096473337
The `inconclusive-not-best-prevblk` check is moved from RPC
code to `TestBlockValidity`.
Test coverage is increased by `mining_template_verification.py`.
Superseedes #31564
## Background
### Verifying block templates (no PoW)
Stratum v2 allows miners to generate their own block template. Pools may wish (or need) to verify these templates. This typically involves comparing mempools, asking miners to providing missing transactions and then reconstructing the proposed block.[^0] This is not sufficient to ensure a proposed block is actually valid. In some schemes miners could take advantage of incomplete validation[^1].
The Stratum Reference Implementation (SRI), currently the only Stratum v2 implementation, collects all missing mempool transactions, but does not yet fully verify the block.[^2]. It could use the `getblocktemplate` RPC in `proposal` mode, but using IPC is more performant, as it avoids serialising up to 4 MB of transaction data as JSON.
(although SRI could use this PR, the Template Provider role doesn't need it, so this is _not_ part of #31098)
[^0]: https://github.com/stratum-mining/sv2-spec/blob/main/06-Job-Declaration-Protocol.md
[^1]: https://delvingbitcoin.org/t/pplns-with-job-declaration/1099/45?u=sjors
[^2]: https://github.com/stratum-mining/stratum/blob/v1.1.0/roles/jd-server/src/lib/job_declarator/message_handler.rs#L196
ACKs for top commit:
davidgumberg:
reACK a18e572328
achow101:
ACK a18e572328
TheCharlatan:
ACK a18e572328
ryanofsky:
Code review ACK a18e572328 just adding another NONFATAL_UNREACHABLE since last review
Tree-SHA512: 1a6c29f45a1666114f10f55aed155980b90104db27761c78aada4727ce3129e6ae7a522d90a56314bd767bd7944dfa46e85fb9f714370fc83e6a585be7b044f1
There is no way to report a close error from `AutoFile` destructor.
Such an error could be serious if the file has been written to because
it may mean the file is now corrupted (same as if write fails).
So, change all users of `AutoFile` that use it to write data to
explicitly close the file and handle a possible error.
Comments are expanded.
Return BlockValidationState instead of passing a reference.
Lock Chainman mutex instead of cs_main.
Remove redundant chainparams and pindexPrev arguments.
Drop defaults for checking proof-of-work and merkle root.
The ContextualCheckBlockHeader check is moved to after CheckBlock,
which is more similar to normal validation where context-free checks
are done first.
Validation failure reasons are no longer printed through LogError(),
since it depends on the caller whether this implies an actual bug
in the node, or an externally sourced block that happens to be invalid.
When called from getblocktemplate, via BlockAssembler::CreateNewBlock(),
this method already throws an std::runtime_error if validation fails.
Additionally it moves the inconclusive-not-best-prevblk check from RPC
code to TestBlockValidity.
There is no behavior change when callling getblocktemplate with proposal.
Previously this would return a BIP22ValidationResult which can throw for
state.IsError(). But CheckBlock() and the functions it calls only use
state.IsValid().
The final assert is changed into Assume, with a LogError.
Co-authored-by: <Ryan Ofsky <ryan@ofsky.org>
fa9ca13f35 refactor: Sort includes of touched source files (MarcoFalke)
facb152697 scripted-diff: Bump copyright headers after include changes (MarcoFalke)
fae71d30f7 clang-tidy: Apply modernize-deprecated-headers (MarcoFalke)
Pull request description:
Bitcoin Core is written in C++, so it is confusing to sometimes use the deprecated C headers (with the `.h` extension). For example, it is less clear whether `string.h` refers to the file in this repo or the cstring stdlib header (https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121492797).
The check is currently disabled for headers, to exclude subtree headers.
ACKs for top commit:
l0rinc:
ACK fa9ca13f35
achow101:
ACK fa9ca13f35
janb84:
ACK fa9ca13f35
stickies-v:
ACK fa9ca13f35
Tree-SHA512: 6639608308c598d612e24435aa519afe92d71b955874b87e527245291fb874b67f3ab95d3a0a5125c6adce5eb41c0d62f6ca488fbbfd60a94f2063d734173f4d
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~0 )
-END VERIFY SCRIPT-
This can be reproduced according to the developer notes with something
like
( cd ./src/ && ../contrib/devtools/run-clang-tidy.py -p ../bld-cmake -fix -j $(nproc) )
Also, the header related changes were done manually.
ee045b61ef rpc, psbt: Require sighashes match for descriptorprocesspsbt (Ava Chow)
2b7682c372 psbt: use sighash type field to determine whether to remove non-witness utxos (Ava Chow)
28781b5f06 psbt: Add sighash types to PSBT when not DEFAULT or ALL (Ava Chow)
15ce1bd73f psbt: Enforce sighash type of signatures matches psbt (Ava Chow)
1f71cd337a wallet: Remove sighash type enforcement from FillPSBT (Ava Chow)
4c7d767e49 psbt: Check sighash types in SignPSBTInput and take sighash as optional (Ava Chow)
a118256948 script: Add IsPayToTaproot() (Ava Chow)
d6001dcd4a wallet: change FillPSBT to take sighash as optional (Ava Chow)
e58b680923 psbt: Return PSBTError from SignPSBTInput (Ava Chow)
2adfd81532 tests: Test PSBT sighash type mismatch (Ava Chow)
5a5d26d612 psbt: Require ECDSA signatures to be validly encoded (Ava Chow)
Pull request description:
Currently, we do not add the sighash field to PSBTs at all, even when we have signed with a non-default sighash. This PR changes the behavior such that when we (attempt to) sign with a sighash other than DEFAULT or ALL, the sighash type field will be added to the PSBT to inform the later signers that a different sighash type was used by a signer. Notably, this is necessary for MuSig2 support as all signers must sign using the same sighash type, but the sighash is not provided in partial signatures.
Furthermore, because the sighash type can also be provided on the command line, we require that if both a command line sighash type and the sighash field is present, they must specify the same sighash type. However, this was being checked by the wallet, rather than the signing code, so the `descriptorprocesspsbt` RPC was not enforcing this restriction at all, and in fact ignored the sighash field entirely. This PR refactors the checking code so that the underlying PSBT signing function `SignPSBTInput` does the check.
ACKs for top commit:
theStack:
re-ACK ee045b61ef
rkrux:
re-ACK ee045b61ef
fjahr:
Code review ACK ee045b61ef
Tree-SHA512: 4ead5be1ef6756251b827f594beba868a145d75bf7f4ef6f15ad21f0ae4b8d71b38c83494e5a6b75f37fadd097178cddd93d614b962a2c72fc134f00ba2f74ae
135a0f0aa7 doc: Add missing top-level description to pruneblockchain RPC (nervana21)
Pull request description:
Previously, the `pruneblockchain` RPC help output included only the method signature and arguments, with no top-level description explaining its purpose or constraints.
This PR adds a top-level description, improving documentation consistency and alerting users to the potential impacts of using the command.
ACKs for top commit:
maflcko:
lgtm ACK 135a0f0aa7
yancyribbens:
cr ACK 135a0f0aa7
achow101:
ACK 135a0f0aa7
janb84:
re ACK [135a0f0](135a0f0aa7)
Tree-SHA512: e51475238e779555315668b7389ed312a5d2c4ad1c0b251f2314895ac473092fa458b6f931f70385e14047adb7e340e44fe2198643603da9e129f1c874578a28
