fad0c76d0a109ab7b063f0d405588cf1e6c15e4d clang-format: Set PackConstructorInitializers: CurrentLine (MarcoFalke)
Pull request description:
Now that the minimum supported clang version is larger than 14, the `PackConstructorInitializers` setting can be set to `CurrentLine` in the clang-format file. (This option was added in clang 14. Ref: https://releases.llvm.org/17.0.1/tools/clang/docs/ClangFormatStyleOptions.html#packconstructorinitializers)
The `CurrentLine` option will either put all constructor initializers on the current line if they fit. Otherwise, it will put each one on its own line.
The `CurrentLine` option is desirable over the current `BinPack` option, because:
* It is what the majority of the codebase is currently using.
* It makes it easier to skim the lines to ensure all fields are properly initialized, without having to parse bin-packed constructor initializers, possibly with nested initializer lists, function calls, or ternary operators.
* It makes diffs smaller when an initializer is added or removed, because only a single line is touched. Otherwise, the whole bin-packed block could re-flow, making the diff harder to parse.
Note: The previous `BinPack` option allows any formatting, due to the current `ColumnLimit: 0`. I presume developers manually formatted most constructor initializers to be on separate lines? With the new `CurrentLine` setting, one has to only put the first initializer on a separate line, and clang-format will take care of the rest.
For example:
```sh
echo 'A::A(O o)
: m_first{o.a, o.b},
m_second{fun(o)}, m_third{o.c?o.d:o.e} {}' | clang-format --style=file:./src/.clang-format
```
Will put each on a separate line. Previously, it was left as-is.
ACKs for top commit:
l0rinc:
ACK fad0c76d0a109ab7b063f0d405588cf1e6c15e4d
TheCharlatan:
ACK fad0c76d0a109ab7b063f0d405588cf1e6c15e4d
hebasto:
ACK fad0c76d0a109ab7b063f0d405588cf1e6c15e4d.
Tree-SHA512: f26a0980ecfa01b2a5279561e3df316c10241f8e67830034d493d70a6d0baae8831498233e8986cfa8f3b434cb9bc1e7e525b3d4587dca66b2d609ddae522a88
99d012ec80a4415e1a37218fb4933550276b9a0a refactor: return reference instead of pointer (Andrew Toth)
f743e6c5dd386b7535e6c9442923a6ee54341994 refactor: add missing LIFETIMEBOUND annotation for parameter (Andrew Toth)
141117f5e8b41eb27539d217aa4e6c407c067d90 refactor: remove incorrect LIFETIMEBOUND annotations (Andrew Toth)
Pull request description:
The [developer-notes say](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lifetimebound):
> You can use the attribute by adding a `LIFETIMEBOUND`
annotation defined in `src/attributes.h`; please grep the codebase for examples.
While grepping, I found an incorrect usage of the `LIFETIMEBOUND` annotation on `BlockManager::CheckBlockDataAvailability`. This could be misleading about usage for other greppers. As I was looking, I also noticed a missing `LIFETIMEBOUND` on `BlockManager::GetFirstBlock`. While looking more closely at that method, it should return a reference instead of a pointer. The only reason to return a pointer is if it can be null.
ACKs for top commit:
maflcko:
review ACK 99d012ec80a4415e1a37218fb4933550276b9a0a 💧
l0rinc:
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
stickies-v:
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
optout21:
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
vasild:
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
Tree-SHA512: d6c56ee223d6614d52ee6cf5cd66e787125c98c6ae37705a17e51a6e15774e260ac55b3d60f2fc818132e766ad98dd94232d6c8829119f628498e9d0d2bd977f
fa1bf6818f0910f997e5235a197ff51f2e18780d clang-format: Set InsertNewlineAtEOF: true (MarcoFalke)
Pull request description:
Now that the minimum supported clang version is 17, the `InsertNewlineAtEOF` setting can be set to `true` in the clang-format file. (https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#insertnewlineateof)
This is in line with the already existing newline linter. Can be tested via:
```
truncate --size=-1 src/init.cpp
git diff
# Should fail:
cargo run --manifest-path ./test/lint/test_runner/Cargo.toml -- --lint=trailing_newline
# Restore newline:
git diff -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i -v
```
ACKs for top commit:
l0rinc:
ACK fa1bf6818f0910f997e5235a197ff51f2e18780d
achow101:
ACK fa1bf6818f0910f997e5235a197ff51f2e18780d
hodlinator:
ACK fa1bf6818f0910f997e5235a197ff51f2e18780d
stickies-v:
ACK fa1bf6818f0910f997e5235a197ff51f2e18780d
hebasto:
ACK fa1bf6818f0910f997e5235a197ff51f2e18780d.
janb84:
ACK fa1bf6818f0910f997e5235a197ff51f2e18780d
Tree-SHA512: 6443f0f9e2710fb31c85e4ece5deb9fa25bf22ef2d5cadad2d54a239194860c3a785ffa5dd97be942e42139336575425499f085aa2e3f29003e529fdc20fce20
6657bcbdb4d0359c1843ca31fb3670c7c0c260d5 kernel: allow null data_directory (stickies-v)
Pull request description:
An empty path may be represented with a `nullptr`. For example, `std::string_view{}.data()` may return nullptr.
Removes the `BITCOINKERNEL_ARG_NONNULL` attribute for `btck_chainstate_manager_options_create` 's `data_directory` parameter, and instead handles such null arguments in the implementation. [Because an empty path is meaningless](https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523930442), `btck_chainstate_manager_options_create` now treats both null and empty directories as invalid, tightening the interface.
Also documents how `BITCOINKERNEL_ARG_NONNULL` should be used.
Follow-up to https://github.com/bitcoin/bitcoin/pull/33853#pullrequestreview-3454620265
ACKs for top commit:
stringintech:
ACK 6657bcb
maflcko:
review ACK 6657bcbdb4d0359c1843ca31fb3670c7c0c260d5 🐪
achow101:
ACK 6657bcbdb4d0359c1843ca31fb3670c7c0c260d5
TheCharlatan:
ACK 6657bcbdb4d0359c1843ca31fb3670c7c0c260d5
janb84:
ACK 6657bcbdb4d0359c1843ca31fb3670c7c0c260d5
Tree-SHA512: 11c02b221ff19a5357e94355808e3b503b3a336c16fc5186c9c9137931709e880383ed1f4990fc4cc6b0e23961e2e1e03fc90154a3b546b9490ef66bd63688b7
7f318e1dd0496384e7bc6d8754c5a2a618a14b2a test: Add better coverage for Autofile size() (Fabian Jahr)
b7af960eb82f5e3af530014a4f59c48faa3a4109 refactor: Add AutoFile::size (Fabian Jahr)
ec0f75862e6791eade487f0dc7d4d0a8c91b9ba5 refactor: Modernize logging in util/asmap.cpp (Fabian Jahr)
606a251e0a3146eb2d2beb7cef486b2d46a37b40 tests: add unit test vectors for asmap interpreter (Pieter Wuille)
Pull request description:
This contains some commits from #28792 that can be easily reviewed and merged independently. I hope splitting this change off can make this part move a bit faster and reduce frequency of needed rebases for #28792.
The commits in order:
- Add additional unit test vectors to the asmap interpreter (written by sipa). This helps to ensure that the further refactors in #28792 don't change behavior.
- Modernizes the logging in `util/asmap.cpp`, I added this while touching the rest of the file all over anyway.
- Adds an `AutoFile::size` helper function with some additional test coverage in a separate commit
ACKs for top commit:
maflcko:
review ACK 7f318e1dd0496384e7bc6d8754c5a2a618a14b2a 🏀
hodlinator:
tACK 7f318e1dd0496384e7bc6d8754c5a2a618a14b2a
laanwj:
Code review ACK 7f318e1dd0496384e7bc6d8754c5a2a618a14b2a
Tree-SHA512: 45156b74e4bd9278a7ec24521dfdafe4dab1ba3384243c7d589ef17e16ca374ee2af7178c86b7229e80ca262dbe78c4d456d80a6ee742ec31d2ab5243dac8b57
310e4979b36cbcf1e9e01dd90c14e2e9997343a0 qt: Added test coverage for qt gui#901 console history filter (WakeTrainDev)
Pull request description:
Add test coverage for the QT rpc console updated filtered commands in gui#901
ACKs for top commit:
pablomartin4btc:
ACK 310e4979b36cbcf1e9e01dd90c14e2e9997343a0
hebasto:
ACK 310e4979b36cbcf1e9e01dd90c14e2e9997343a0, tested on Fedora 42 by reverting 4e352efa2ce756c668664486c99d003eef530e0c.
Tree-SHA512: 45bb8583311f145353d8265d28f220d2a318c701346f147979c5d33b27811276d5e18586bf58f35e455701495d2cb87ec54dd78f4ca8631a0c7bd2c1d7fe640c
929f69d0ff29cb803769a423035fdcf675f40b78 qt: Remove HD seed reference from blank wallet tooltip (John Moffett)
Pull request description:
Blank descriptor wallets currently do not have HD seeds and none can be added (or 'set') by the user, so remove the reference in the tooltip.
As I understand it, descriptor wallets don't have a global HD seed and don't store the HD seeds for keys they generate. Currently, no new HD seeds can be added by the user (even for old wallets since `sethdseed` was removed), though it may be possible in the future, eg - https://github.com/bitcoin/bitcoin/pull/33043
ACKs for top commit:
maflcko:
lgtm ACK 929f69d0ff29cb803769a423035fdcf675f40b78
Tree-SHA512: 85e9c8e18a92b3da6fd62b70570cef58e03843633300b65aee5789d38c7bcaa46738970f0aea63f4e9b3e8814abb5bf1e1aa93f568a875ad1e0443d4dafb0aab
28a4fcb03c0fb1cd5112eca1eb36dcb13e0b4ff2 test: check listdescriptors do not return a mix of hardened derivation marker (pythcoiner)
975783cb79e929260873c1055d4b415cd33bb6b9 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 28a4fcb03c0fb1cd5112eca1eb36dcb13e0b4ff2
achow101:
ACK 28a4fcb03c0fb1cd5112eca1eb36dcb13e0b4ff2
rkrux:
Concept ACK 28a4fcb03c0fb1cd5112eca1eb36dcb13e0b4ff2
Tree-SHA512: 15d14000b5951ca69a64a05b9a0b138c48a07b81eaf2fa86b91ac20cc8735533355a787363c64ba88403dd8a56ef5232cba57d34bea80835a0f40774d62fbc2b
f53dbbc5057b6f676db4be9bc720898149f293fc test: Add functional tests for named argument parsing (zaidmstrr)
694f04e2bd34f994d81e27b68e4d7466a9a319f8 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 f53dbbc5057b6f676db4be9bc720898149f293fc. Just applied comment & test suggestions since last review
kannapoix:
Code review ACK: f53dbbc5057b6f676db4be9bc720898149f293fc
achow101:
ACK f53dbbc5057b6f676db4be9bc720898149f293fc
Tree-SHA512: 1b517144efeff45a4c4256c27a39ddf187f1d6189d133402a45171678214a10ff2925c31edcfd556d67f85bd26d42f63c528b941b68c9880eab443f2c883e681
4d893c0f46055218d6a2b3d24fbce9f0fb6ddc92 net: Remove unused `local_socket_bytes` variable in `CConnman::GetAddresses()` (WakeTrainDev)
Pull request description:
The local_socket_bytes variable was never used. Removed it to clean up dead code.
ACKs for top commit:
mzumsande:
ACK 4d893c0f46055218d6a2b3d24fbce9f0fb6ddc92
theStack:
ACK 4d893c0f46055218d6a2b3d24fbce9f0fb6ddc92
Tree-SHA512: f423bcf975aa2602464fcb96db323cbd6007a7491ddbe119f1d20e890c883dd351a55976151c5d25f5d26267b0efe1f0836fbd65e540c920dac931ed8d67846a
0aebdac95da9a7d476264424c0107bd806ce5362 init: completely remove `-maxorphantx` option (Sebastian Falbesoner)
Pull request description:
This is a small follow-up for #32941 (commit 1384dbaf6d0bfcdb05f97e1e3cb3d5e498bee505), removing the `-maxorphantx` option completely, now that v30 has been released. If removing it for v31 is seen as controversial/premature (I personally don't think it is), the merge can be delayed for a future release.
ACKs for top commit:
maflcko:
lgtm ACK 0aebdac95da9a7d476264424c0107bd806ce5362
achow101:
ACK 0aebdac95da9a7d476264424c0107bd806ce5362
w0xlt:
ACK 0aebdac95d
rkrux:
lgtm ACK 0aebdac95da9a7d476264424c0107bd806ce5362
stickies-v:
ACK 0aebdac95da9a7d476264424c0107bd806ce5362
Tree-SHA512: 818633b903174387ae259acb1d1e8ce07f78e158de2c150742ef0950b0f5d62af553e4e35ab962432306e04e07c45b1be11dbae459a8b62c4b9a6b5ef1746d26
d31158d3646f3c7e4832b9ca50f6ffe02800ff4c psbt: clarify PSBT, PSBTInput, PSBTOutput unserialization flows (rkrux)
Pull request description:
The unserialization flows of the PSBT types work based on few underlying assumptions of functions from `serialize.h` & `stream.h` that takes some to understand when read the first time.
Add few comments that highlight these assumptions hopefully making it easier to grasp. Also, mention key/value format types as per BIP 174.
ACKs for top commit:
achow101:
ACK d31158d3646f3c7e4832b9ca50f6ffe02800ff4c
theStack:
ACK d31158d3646f3c7e4832b9ca50f6ffe02800ff4c
Tree-SHA512: 45111ef7f0258ebbc41d058b3ef2a72472774ab2878caf2d71d7b57b27549c46a51ccbeda5fe164bcf4f7ec10627bbae6e7763aa80b1e66912703a2088682817
fa95353902b7a6f73f094e78106088ab3c16ce14 ci: Run macos tasks in a git archive, not git checkout (MarcoFalke)
faf99ae379636d6ef8316ffb2efaa61896343de9 refactor: Avoid -W*-whitespace in git archive (MarcoFalke)
Pull request description:
Otherwise, compilation with GCC-15+ will warn about it:
```
src/clientversion.cpp:33:79: error: trailing whitespace [-Werror=trailing-whitespace=]
33 | //! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.
```
Follow-up to https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3522280482
Can be tested via `git archive --output=/tmp/a.tar HEAD`
ACKs for top commit:
fanquake:
ACK fa95353902b7a6f73f094e78106088ab3c16ce14
Tree-SHA512: 73940ffc0fd83db557275bd5e993a3c47c5397682a1188447c48e077ead597ba0fc3e5ef9da7b746746ff04a26022ce35ac10768888bbd4707f25b799af43e45
2594d5a189e52052c2019faccaa47f2affdc48e1 build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings (Henry Romp)
Pull request description:
Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings that are no longer needed after reordering the Guix build script to perform binary checks after installation.
This PR also removes the unused CMake maintenance targets (`check-security` and `check-symbols`) and updates the Guix security checks to include binaries in the `libexec/` directory (added in PR #31679).
ACKs for top commit:
purpleKarrot:
ACK 2594d5a189e52052c2019faccaa47f2affdc48e1
hebasto:
ACK 2594d5a189e52052c2019faccaa47f2affdc48e1.
Tree-SHA512: ed451a298f5aae05c177b0033b092faaa7536caeaa3d84da9b8b611e2aa905e1dd337e57aef0efd69ce6ce6ac0cf77dc57adf175079b95bf53dd96d5d0c8118b
0dd8d5c237e2fdb0168d9ca644e7f2f5a8b6ed72 cmake: Specify Windows plugin path in `test_bitcoin-qt` property (Hennadii Stepanov)
Pull request description:
This PR simplifies testing on Windows by removing the need to set the `QT_PLUGIN_PATH` environment variable for different build configurations. For example, the paths might otherwise be:
- `C:/Users/hebasto/dev/bitcoin/build/vcpkg_installed/x64-windows/Qt6/plugins/` for "Release"
- `C:/Users/hebasto/dev/bitcoin/build/vcpkg_installed/x64-windows/debug/Qt6/plugins/` for "Debug"
ACKs for top commit:
purpleKarrot:
ACK 0dd8d5c237e2fdb0168d9ca644e7f2f5a8b6ed72
Tree-SHA512: 0418b8fa4d74ca500aae9e36e56ebcefb566d2ac04a9d22e17d309400ad38dd5a6e75f0195c680796b761fb145444c33336b64180f15c6b1125fe190d58396b6
An empty path may be represented with a nullptr. For example,
std::string_view::data() may return nullptr.
Removes the BITCOINKERNEL_ARG_NONNULL attribute for data_directory,
and instead handles such null arguments in the implementation.
Also documents how BITCOINKERNEL_ARG_NONNULL should be used.
The return value of BlockManager::GetFirstBlock must always be non-null. This
can be inferred by the implementation, which has an assertion that the return
value is not null. A raw pointer should only be returned if the result may be
null. In this case a reference is more appropriate.
The BlockManager::GetFirstBlock lower_block parameter can have its lifetime
extended by the return parameter. In the case where lower_block is returned,
its lifetime will be bound to the return value. A LIFETIMEBOUND annotation is
appropriate here.
The return value of CheckBlockDataAvailability does not extend the lifetime of
the input parameters, nor does BlockManager instance retain references to the
parameters. The LIFETIMEBOUND annotations are misleading here since the lifetime
of the parameters are not extended past the method call.
b0a38871546dfcdd3a578c1ae4c28a88b6ee32d5 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 b0a38871546dfcdd3a578c1ae4c28a88b6ee32d5
rkrux:
lgtm ACK b0a38871546dfcdd3a578c1ae4c28a88b6ee32d5
Tree-SHA512: e24f2aaf18fcfb0ae047a53ed209135a644ff08f5a8bc162c1522be3f99d7d01d550fc2e73d8db5fec7b748902daf68e61e7a5624f5913b9824feba5641fc78c
Remove CMake settings that are no longer needed after reordering Guix build script to perform binary checks after installation.
Also removes unused CMake maintenance targets (check-security and check-symbols) and updates security checks to include libexec/ directory binaries (see PR #31679).
c25a5e670b27d3b6eb958ce437dbe89678bd1511 init: Signal m_tip_block_cv on Ctrl-C (Ryan Ofsky)
6a29f79006a9d60b476893dface5eea8f9bf271c 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 c25a5e670b27d3b6eb958ce437dbe89678bd1511
TheCharlatan:
ACK c25a5e670b27d3b6eb958ce437dbe89678bd1511
enirox001:
Concept ACK c25a5e6
Tree-SHA512: 320aaa74fd308e826521c48c9a8aca4bd5f5530064cda2303d251d8e93e50c474bcd0db760ce04921928e73abefe4847aff797ac9ca7c89e74e5051bbed061cd
6eaa00fe20206baedc0d8ade5bb8d066ea615704 test: clarify submitBlock() mutates the template (Sjors Provoost)
862bd432837efeb6ab1435f75493501618ab3190 mining: ensure witness commitment check in submitBlock (Sjors Provoost)
00d1b6ef4b1203e80271c16c0d5b179525de1913 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 6eaa00fe20206baedc0d8ade5bb8d066ea615704. Just documentation updates and test clarifications since last review, also splitting up a commit.
TheCharlatan:
Re-ACK 6eaa00fe20206baedc0d8ade5bb8d066ea615704
ismaelsadeeq:
Code review and tested ACK 6eaa00fe20206baedc0d8ade5bb8d066ea615704
Tree-SHA512: 3a6280345b0290fe8300ebc63c13ad4058d24ceb35b7d7a784b974d5f04f420860ac03a9bf2fc6a799ef3fc55552ce033e879fa369298f976b9a01d72bd55d9e
fa6c0bedd33ac7ad27454adaf9522fd27bef6ea3 refactor: Return uint64_t from GetSerializeSize (MarcoFalke)
fad0c8680ea7ef433c2d6e7c0d5799f81fd861b9 refactor: Use uint64_t over size_t for serialized-size values (MarcoFalke)
fa4f388fc99c9ec7c3cf2bac3863c7b3004bb2ae refactor: Use fixed size ints over (un)signed ints for serialized values (MarcoFalke)
fa01f38e53cfda4155d0ea09ca8b1291b7001fe8 move-only: Move CBlockFileInfo to kernel namespace (MarcoFalke)
fa2bbc9e4cfe017436a5167ab5c443f4412efa3c refactor: [rpc] Remove cast when reporting serialized size (MarcoFalke)
fa364af89bd914ea7cd0d4a5470e0a502e0a2075 test: Remove outdated comment (MarcoFalke)
Pull request description:
Consensus code should arrive at the same conclusion, regardless of the architecture it runs on. Using architecture-specific types such as `size_t` can lead to issues, such as the low-severity [CVE-2025-46597](https://bitcoincore.org/en/2025/10/24/disclose-cve-2025-46597/).
The CVE was already worked around, but it may be good to still fix the underlying issue.
Fixes https://github.com/bitcoin/bitcoin/issues/33709 with a few refactors to use explicit fixed-sized integer types in serialization-size related code and concluding with a refactor to return `uint64_t` from `GetSerializeSize`. The refactors should not change any behavior, because the CVE was already worked around.
ACKs for top commit:
Crypt-iQ:
crACK fa6c0bedd33ac7ad27454adaf9522fd27bef6ea3
l0rinc:
ACK fa6c0bedd33ac7ad27454adaf9522fd27bef6ea3
laanwj:
Code review ACK fa6c0bedd33ac7ad27454adaf9522fd27bef6ea3
Tree-SHA512: f45057bd86fb46011e4cb3edf0dc607057d72ed869fd6ad636562111ae80fea233b2fc45c34b02256331028359a9c3f4fa73e9b882b225bdc089d00becd0195e
a3ac59a4316305fb38a5338b48940682889d0dc2 ci: Enable experimental kernel stuff in ASan task (MarcoFalke)
5b89956eeb76cf8c9717152fbb0928e026fc0087 kernel: Allow null arguments for serialized data (TheCharlatan)
Pull request description:
An empty span constructed from an empty vector may have a null data pointer depending on the implementation. Remove the BITCOINKERNEL_ARG_NONNULL requirement for these arguments and instead handle such null arguments in the implementation.
Also cherry-picked from #33845 to show that CI task passing now.
ACKs for top commit:
yuvicc:
Code review ACK a3ac59a4316305fb38a5338b48940682889d0dc2
maflcko:
review ACK a3ac59a4316305fb38a5338b48940682889d0dc2 🥈
laanwj:
code review ACK a3ac59a4316305fb38a5338b48940682889d0dc2
Tree-SHA512: 629e463796f2f057df5be8e8981a45751c578ed0021be731c1d57fe849a539fe38b0a445914b0fc48f32f0408ad6d566984bd7f3a68797fcfdf1c6889e316a08
40dcbf580d8eb31a067b62bf9676099919b9841e build: add -Wtrailing-whitespace=any (fanquake)
d7659cd7e6f883088081c9e782d8a3fa40da210a build: add -Wleading-whitespace=spaces (fanquake)
d86650220a16075f7739a9ae0a017df4477a4541 cmake: Disable `-Wtrailing-whitespace` warnings for RCC-generated files (Hennadii Stepanov)
aabc5ca6ed6e15e1f5c805b0e14c0c701b2b1824 cmake: Switch from AUTORCC to `qt6_add_resources` (Hennadii Stepanov)
25ae14c3391a813cdf78fb067693be0c4db06bd2 subprocess: replace tab with space (fanquake)
0c2b9dadd55453e7e730c361f88b3cae12f969cc scripted-diff: remove whitespace in sha256_sse4.cpp (fanquake)
4da084fbc93374ed07bca6d10f42a8c6aa73f3f3 scripted-diff: change whitespace to spaces in univalue (fanquake)
e6caf150b309a576ce016b589cea203c871866bc ci: add moreutils to lint job (fanquake)
Pull request description:
GCC 15 now has options to turn leading & trailing whitespace into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable `-Wleading-whitespace` and `-Wtrailing-whitespace`.
We currently get PRs that are opened with various whitespace, i.e #33822, so turning that into compile-time failure where possible, seems useful, to avoid a CI roundtrip.
ACKs for top commit:
ajtowns:
utACK 40dcbf580d8eb31a067b62bf9676099919b9841e
hebasto:
re-ACK 40dcbf580d8eb31a067b62bf9676099919b9841e.
Tree-SHA512: a128001ab2abb41cd6d249dcf46be4167ebd608d6b0f1452212a3ec9a383747bea623ab0382ec7bc0ac7a232a47cca5174e1cd73d4eda6751aa3cb2365ad2ede
fa9f29a4a79944f6ffbb58eab0ac41e243fbeb97 doc: Recommend latest Debian stable or Ubuntu LTS (MarcoFalke)
fa1711ee0d3bac12daa7fdac04af448b69cc257a doc: Add GCC-12 min release notes (MarcoFalke)
faa8be75c9470d7d28b0993b723ef1a36a6f58cd ci: Enable experimental kernel stuff in G++-12 task (previous releases) (MarcoFalke)
fabce97b303bd4aafa98ceb11c63800e7f4f11cd test: Remove gccbug_90348 test case (MarcoFalke)
fa3854e43295f71f5dad8557dd621f0f799b0ee0 test: Remove unused fs::create_directories test (MarcoFalke)
fa9dacdbde7dc18d134019bdad24f47e4dea1dda util: [refactor] Remove unused create_directories workaround (MarcoFalke)
fa807f78aede4bc59a75366899fd5752ce6a66f8 build: Bump g++ minimum supported version to 12 (MarcoFalke)
Pull request description:
All supported operating systems that previously came with at least g++-11, also come with at least g++-12, so bumping the minimum should be fine.
For reference:
* https://packages.ubuntu.com/jammy/g++-12
* https://packages.ubuntu.com/noble/g++ (g++-13)
* https://packages.debian.org/bookworm/g++ (g++-12)
* FreeBSD Ports ship a recent GCC
* RHEL-based 8, and 9 ship with g++-14 via appstream (`dnf install gcc-toolset-14` -> `/opt/rh/gcc-toolset-14/`)
* RHEL-based 10 ships with g++ (14 by default)
* OpenSuse Leap and Tumbleweed ship with g++ 15 https://software.opensuse.org/package/gcc15-c++
Obviously, downloading pre-compiled releases or compiling previous release branches is unaffected by this change.
ACKs for top commit:
janb84:
re-ACK fa9f29a4a79944f6ffbb58eab0ac41e243fbeb97
TheCharlatan:
Re-ACK fa9f29a4a79944f6ffbb58eab0ac41e243fbeb97
hebasto:
ACK fa9f29a4a79944f6ffbb58eab0ac41e243fbeb97.
Tree-SHA512: ce14ecf78ccfe4f221dcbc9147dcfc00c0512b23a6fcda5ba71b62b4f5d39a5139f083d035113f189bfbd396d485e1ebc626a9a16b6fa0b74fd95aed2041c841
The test case no longer detects this specific issue for GCC versions
12.1+, as explained in the
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90348 thread and in this
compiler-explorer playground:
https://godbolt.org/z/Y48osrjM8
So remove the test case and update the -fstack-reuse=none cmake
docstring with the underlying affected GCC versions, and the bug URL.
The test was added in commit ddb75c2e87a60ed24065bdf0c3bfabf4e058cef1.
After the create_directories wrapper removal, the test is redundant with
the unit test in the upstream stdlib. Also, there is a Bitcoin Core
functional test that covers this behavior in
test/functional/feature_dirsymlinks.py
So remove this unit test.
Finally, I could not find a real system that still ships a buggy stdlib
(v11.2) in their package manager. A stand-alone test is also available
in compiler-explorer under https://godbolt.org/z/aeMKraYrT.
7a4901c9029687ad2b37f6d929d4c8fe96c15db3 test, refactor: Fix `-Warray-bounds` warning (Hennadii Stepanov)
faf2759c8c4507adacf2618f23c1f445f490c012 test: [refactor] Use reference over ptr to chainman (MarcoFalke)
Pull request description:
Just some minor test-only refactor commits to fix GCC false positive warnings, along with making the test code easier to read and understand:
* First change requested in https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2510727269
* Second change requested in commit 3b135a8fc4451c93b3ea50b3f4621e0d19f35daf
Those changes are required in a bunch of pulls touching the CI system, so merging them allows to drop them in all pulls.
ACKs for top commit:
l0rinc:
ACK 7a4901c9029687ad2b37f6d929d4c8fe96c15db3
hebasto:
ACK 7a4901c9029687ad2b37f6d929d4c8fe96c15db3, I have reviewed the code and it looks OK.
Tree-SHA512: 64dca52ec7b25078bf489e2d8b43e449f4968fbac14a09c66a60cdc75b513588403665f248368820694a6f72c4f7f465589d9306355239cffe35c38111929eff
An empty span constructed from an empty vector may have a null data
pointer depending on the implementation. Remove the
BITCOINKERNEL_ARG_NONNULL requirement for these arguments and instead
handle such null arguments in the implementation.
