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
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.
66978a1a95379a2fe5d41032682dedfaddc99db9 kernel: remove btck_chain_get_tip (stickies-v)
4dd7e6dc48ed3a97856a19ca15078366cd0b8056 kernel: remove btck_chain_get_genesis (stickies-v)
Pull request description:
Removes `btck_chain_get_genesis` and `btck_chain_get_tip`.
They are trivially replaced with `btck_chain_get_by_height` (as indicated in the updated `bitcoinkernel_wrapper.h`), so I think it makes sense to trim the interface.
For `btck_chain_get_tip`: on `master` we don't provide any guarantees that the returned block index still corresponds to the actual tip, so the extra call doesn't seem like a regression to me.
ACKs for top commit:
TheCharlatan:
ACK 66978a1a95379a2fe5d41032682dedfaddc99db9
janb84:
ACK 66978a1a95379a2fe5d41032682dedfaddc99db9
Tree-SHA512: f583fbb7f2e3f8f23afb57732b2cbe9e1d550bfc43c9a2619895ee30c27f5f3c5cd9e4ecb7e05b1f6ab9e11c368596ec9b733d67e06cfafb12326d88e8e4dd7d
743abbcbde9e5a2db489bca461c98df461eff7d0 refactor: inline constant return value of `BlockTreeDB::WriteBatchSync` and `BlockManager::WriteBlockIndexDB` and `BlockTreeDB::WriteFlag` (Lőrinc)
e030240e909493549e24aa8bcd5b382cab6e2c79 refactor: inline constant return value of `CDBWrapper::Erase` and `BlockTreeDB::WriteReindexing` (Lőrinc)
cdab9480e9e35656f490878f92dab5427b36f21d refactor: inline constant return value of `CDBWrapper::Write` (Lőrinc)
d1847cf5b5af232ad180f5d302361b72334952b2 refactor: inline constant return value of `TxIndex::DB::WriteTxs` (Lőrinc)
50b63a5698e533376ef7a20bc0c440d3d6bf7a9f refactor: inline constant return value of `CDBWrapper::WriteBatch` (Lőrinc)
Pull request description:
Related to https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223587480
### Summary
`WriteBatch` always returns `true` - the errors are handled by throwing `dbwrapper_error` instead.
### Context
This boolean return value of the `Write` methods is confusing because it's inconsistent with `CDBWrapper::Read`, which catches exceptions and returns a boolean to indicate success/failure. It's bad that `Read` returns and `Write` throws - but it's a lot worse that `Write` advertises a return value when it actually communicates errors through exceptions.
### Solution
This PR removes the constant return values from write methods and inlines `true` at their call sites. Many upstream methods had boolean return values only because they were propagating these constants - those have been cleaned up as well.
Methods that returned a constant `true` value that now return `void`:
- `CDBWrapper::WriteBatch`, `CDBWrapper::Write`, `CDBWrapper::Erase`
- `TxIndex::DB::WriteTxs`
- `BlockTreeDB::WriteReindexing`, `BlockTreeDB::WriteBatchSync`, `BlockTreeDB::WriteFlag`
- `BlockManager::WriteBlockIndexDB`
### Note
`CCoinsView::BatchWrite` (and transitively `CCoinsViewCache::Flush` & `CCoinsViewCache::Sync`) were intentionally not changed here. While all implementations return `true`, the base `CCoinsView::BatchWrite` returns `false`. Changing this would cause `coins_view` tests to fail with:
> terminating due to uncaught exception of type std::logic_error: Not all unspent flagged entries were cleared
We can fix that in a follow-up PR.
ACKs for top commit:
achow101:
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
janb84:
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
TheCharlatan:
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
sipa:
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
Tree-SHA512: b2a550bff066216f1958d2dd9a7ef6a9949de518cc636f8ab9c670e0b7a330c1eb8c838e458a8629acb8ac980cea6616955cd84436a7b8ab9096f6d648073b1e
060bb55508245776bb6a39c8b7849769ee588d69 rpc: add decoded tx details to gettransaction with extra wallet fields (Matthew Zipkin)
ad1c3bdba547685ca4163316017ab78e965c7ad1 [move only] move DecodeTxDoc() to a common util file for sharing (Matthew Zipkin)
d633db54166497685b80a12c51db6772982e01fe rpc: add "ischange: true" in wallet gettransaction decoded tx output (Matthew Zipkin)
Pull request description:
This change is motivated by external RBF clients like https://github.com/CardCoins/additive-rbf-batcher/. It saves the user a redundant re-looping of tx outputs, calling `getaddressinfo` on each one, looking for the change output in order to adjust the fee.
The field `"ischange"` only appears when `gettransaction` is called on a wallet, and is either `true` or not present at all. I chose not to include `ischange: false` because it is confusing to see that on *received* transactions.
Example of the new field:
```
"vout": [
{
"value": 1.00000000,
"n": 0,
"scriptPubKey": {
"asm": "0 5483235e05c76273b3b50af62519738781aff021",
"desc": "addr(bcrt1q2jpjxhs9ca388va4ptmz2xtns7q6lupppkw7wu)#d42g84j6",
"hex": "00145483235e05c76273b3b50af62519738781aff021",
"address": "bcrt1q2jpjxhs9ca388va4ptmz2xtns7q6lupppkw7wu",
"type": "witness_v0_keyhash"
}
},
{
"value": 198.99859000,
"n": 1,
"scriptPubKey": {
"asm": "0 870ab1ab58632b05a417d5295f4038500e407592",
"desc": "addr(bcrt1qsu9tr26cvv4stfqh65547spc2q8yqavj7fnlju)#tgapemkv",
"hex": "0014870ab1ab58632b05a417d5295f4038500e407592",
"address": "bcrt1qsu9tr26cvv4stfqh65547spc2q8yqavj7fnlju",
"type": "witness_v0_keyhash"
},
"ischange": true
}
]
```
ACKs for top commit:
furszy:
ACK [060bb55](060bb55508)
maflcko:
review ACK 060bb55508245776bb6a39c8b7849769ee588d69 🌛
achow101:
ACK 060bb55508245776bb6a39c8b7849769ee588d69
rkrux:
lgtm ACK 060bb55508245776bb6a39c8b7849769ee588d69
Tree-SHA512: aae4854d2bb4e9a7bc1152691ea90e594e8da8a63c9c7fda72a504fb6a7e54ae274ed5fa98d35d270e0829cc8f8d2fd35a5fc9735c252a10aa42cc22828930e7
01cc20f3307c532f402cdf5dc17f2f14920b725b test: improve coverage for a resolved stalling situation (Martin Zumsande)
9af6daf07ed0a82386c1930e67683af5f2090e8b test: remove magic number when checking for blocks that have arrived (Martin Zumsande)
3069d66dcac0e1eeca2142a2d72d3d010335346f p2p: During block download, adjust pindexLastCommonBlock better (Martin Zumsande)
Pull request description:
As described in #32179, `pindexLastCommonBlock` is updated later than necessary
in master.
In case of a linear chain with no forks, it can be moved forward at the beginning of
`FindNextBlocksToDownload`, so that the updated value can be used to better estimate `nWindowEnd`.
This helps the node to request all blocks from peers within the correct 1024-block-window and avoids peers being incorrectly marked as stallers.
I also changed `p2p_ibd_stalling.py` to cover the situation after a resolved situation, making sure that no additional peers are marked for stalling.
Fixes#32179
ACKs for top commit:
Crypt-iQ:
crACK 01cc20f3307c532f402cdf5dc17f2f14920b725b
stringintech:
re-ACK 01cc20f
achow101:
ACK 01cc20f3307c532f402cdf5dc17f2f14920b725b
sipa:
utACK 01cc20f3307c532f402cdf5dc17f2f14920b725b
Tree-SHA512: a97f7a7ef5ded538ee35576e04b3fbcdd46a6d0189c7ba3abacc6e0d81e800aac5b0c2d2565d0462ef6fd4acc751989f577fd6adfd450171a7d6ab26f437df32
1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596 log: reduce excessive messages during block replay (Lőrinc)
Pull request description:
### Summary
After an incomplete reindex the blocks will need to be replayed.
This results in excessive `Rolling back` and `Rolling forward` messages which quickly triggers the recently introduced log rate limiter.
Change the logging strategy to:
- Add single `LogInfo` messages showing the full range being replayed for both rollback and roll forward;
- Log progress at `LogInfo` level only every 10,000 blocks to track the long operations.
### Reproducer:
* Start a normal ibd, stop after some progress
* Do a reindex, stop before it finishes
* Restart the node normally without specifying the reindex parameter
It should start rolling the blocks forward.
Before this change the excessive logging would show:
```
[*] Rolling forward 000000002f4f55aecfccc911076dc3f73ac0288c83dc1d79db0a026441031d40 (46245)
[*] Rolling forward 0000000017ffcf34c8eac010c529670ba6745ea59cf1edf7b820928e3b40acf6 (46246)
```
After the change it shows:
```
Replaying blocks
Rolling forward to 00000000000000001034012d7e4facaf16ca747ea94b8ea66743086cfe298ef8 (326223 to 340991)
Rolling forward 00000000000000000faabab19f17c0178c754dbed023e6c871dcaf74159c5f02 (330000)
Rolling forward 00000000000000000d9b2508615d569e18f00c034d71474fc44a43af8d4a5003 (340000)
...
Rolled forward to 00000000000000001034012d7e4facaf16ca747ea94b8ea66743086cfe298ef8
```
(similarly to rolling back)
ACKs for top commit:
Crypt-iQ:
crACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596
stickies-v:
ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596
achow101:
ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596
vasild:
ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596
hodlinator:
Concept ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596
Tree-SHA512: 44ed1da8336de5a3d937e11a13e6f1789064e23eb70640a1c406fbb0074255344268f6eb6b06f036ca8d22bfeb4bdea319c3085a2139d848f6d36a4f8352b76a
4543a3bde26ff2440c16b06cc1dcf1994dc85720 Squashed 'src/minisketch/' changes from ea8f66b1ea..d1bd01e189 (Hennadii Stepanov)
Pull request description:
This PR updates the `minisketch` subtree to latest upstream, which includes:
- https://github.com/bitcoin-core/minisketch/pull/75
- https://github.com/bitcoin-core/minisketch/pull/98
ACKs for top commit:
fanquake:
ACK c235aa468b0dcc67b49340dbe9b675c513cec7bf
Tree-SHA512: 856fb8b7dc2e743c9c67164023bf53faf8766079aeccc82a30c8b90c85920b31977b6a8b26e51e5485b20e445a3ca6ff806e701a53e95f70181ea30055e3528c
It is equivalent to calling btck_chain_get_by_height with the
height obtained from btck_chain_get_height. In neither case do we
provide guarantees that the returned block index still corresponds
to the actual tip.
It does not make sense to use a pointer, when a reference is more
appropriate, especially given that nullptr has been ruled out.
This is also allows to remove the CI workaround to avoid warnings:
```
C++ compiler .......................... GNU 13.0.0, /bin/x86_64-w64-mingw32-g++-posix
...
/ci_container_base/src/test/blockmanager_tests.cpp: In member function ‘void blockmanager_tests::blockmanager_scan_unlink_already_pruned_files::test_method()’:
/ci_container_base/src/test/blockmanager_tests.cpp:63:17: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
63 | const auto& chainman = Assert(m_node.chainman);
| ^~~~~~~~
In file included from /ci_container_base/src/streams.h:13,
from /ci_container_base/src/dbwrapper.h:11,
from /ci_container_base/src/node/blockstorage.h:10,
from /ci_container_base/src/test/blockmanager_tests.cpp:8:
/ci_container_base/src/util/check.h:116:49: note: the temporary was destroyed at the end of the full expression ‘inline_assertion_check<true, std::unique_ptr<ChainstateManager>&>(((blockmanager_tests::blockmanager_scan_unlink_already_pruned_files*)this)->blockmanager_tests::blockmanager_scan_unlink_already_pruned_files::<anonymous>.TestChain100Setup::<anonymous>.TestingSetup::<anonymous>.ChainTestingSetup::<anonymous>.BasicTestingSetup::m_node.node::NodeContext::chainman, std::source_location{(& *.Lsrc_loc27)}, std::basic_string_view<char>(((const char*)"m_node.chainman")))’
116 | #define Assert(val) inline_assertion_check<true>(val, std::source_location::current(), #val)
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/ci_container_base/src/test/blockmanager_tests.cpp:63:28: note: in expansion of macro ‘Assert’
63 | const auto& chainman = Assert(m_node.chainman);
| ^~~~~~
cc1plus: all warnings being treated as errors
gmake[2]: Leaving directory '/ci_container_base/ci/scratch/build-x86_64-w64-mingw32'
gmake[2]: *** [src/test/CMakeFiles/test_bitcoin.dir/build.make:382: src/test/CMakeFiles/test_bitcoin.dir/blockmanager_tests.cpp.obj] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:1810: src/test/CMakeFiles/test_bitcoin.dir/all] Error 2
gmake[1]: Leaving directory '/ci_container_base/ci/scratch/build-x86_64-w64-mingw32'
gmake: *** [Makefile:146: all] Error 2
```
This false-positive warning is also fixed in later GCC versions.
See also https://godbolt.org/z/fjc6be65M
fad6efd3bef1d123f806d492f019e29530b03a5e refactor: Use STR_INTERNAL_BUG macro where possible (MarcoFalke)
fada379589a17e86396aa7c2ce458ff2ff602b84 doc: Remove unused bugprone-lambda-function-name suppression (MarcoFalke)
fae1d99651e29341e486a10e6340335c71a2144e refactor: Use const reference to std::source_location (MarcoFalke)
fa5fbcd61563942122623fe2840a677853081990 util: Allow Assert() in contexts without __func__ (MarcoFalke)
Pull request description:
Without this, compile warnings could be hit about `__func__` being only valid inside functions.
```
warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function] note: expanded from macro Assert
115 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
| ^
```
Ref https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486258473
This also introduces a slight behaviour change, because `std::source_location::function_name` usually includes the entire function signature instead of just the name.
ACKs for top commit:
l0rinc:
Code review ACK fad6efd3bef1d123f806d492f019e29530b03a5e
stickies-v:
ACK fad6efd3bef1d123f806d492f019e29530b03a5e
hodlinator:
re-ACK fad6efd3bef1d123f806d492f019e29530b03a5e
Tree-SHA512: e78a2d812d5ae22e45c93db1661dafbcd22ef209b3d8d8d5f2ac514e92fd19a17c3f0a5db2ef5e7748aa2083b10c0465326eb36812e6a80e238972facd2c7e98
dcb56fd4cb59e6857c110dd87019459989dc1ec3 interfaces: add interruptWait method (ismaelsadeeq)
Pull request description:
This is an attempt to fix#33575 see the issue for background and the usefulness of this feature.
This PR uses one of the suggested approaches: adding a new `interruptWaitNext()` method to the mining interface.
It introduces a new boolean variable, `m_interrupt_wait`, which is set to `false` when the thread starts waiting. The `interruptWaitNext()` method wakes the thread and sets `m_interrupt_wait` to `true`.
Whenever the thread wakes up, it checks whether the wait was aborted; if so, it simply set ` m_interrupt_wait ` to false and return`nullptr`.
This PR also adds a functional test for the new method. The test uses `asyncio` to spawn two tasks and attempts to ensure that the wait is executed before the interrupt by using an event monitor. It adds a 0.1-second buffer to ensure the wait has started executing.
If that buffer elapses without `waitNext` executing, the test will fail because a transaction is created after the buffer.
ACKs for top commit:
furszy:
Code ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
ryanofsky:
Code review ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3, just tweaking semantics slightly since last review so if an `interruptWait` call is made shortly after a `waitNext` call it will reliably cause the `waitNext` call to return right away without blocking, even if the `waitNext` call had not begun to execute or wait yet.
Sjors:
tACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
TheCharlatan:
ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
Tree-SHA512: a03f049e1f303b174a9e5d125733b6583dfd8effa12e7b6c37bd9b2cff9541100f5f4514e80f89005c44a57d7e47804afe87aa5fdb6831f3b0cd9b01d83e42be
24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7 refactor: remove dead code in `CountWitnessSigOps` (Lőrinc)
Pull request description:
Found while reviewing #32840
The `nullptr` witness path was dead in normal code paths: replacing it with reference enables us deleting unreachable logic.
Code coverage proof:
https://maflcko.github.io/b-c-cov/total.coverage/src/script/interpreter.cpp.gcov.html#L2135
ACKs for top commit:
kevkevinpal:
ACK [24bcad3](24bcad3d4d)
maflcko:
review ACK 24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7 🐏
darosior:
Neat. utACK 24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7.
stickies-v:
ACK 24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7
Tree-SHA512: 92c87e431f06a15d8eeb02e20e9154b272c4586ddacf77c8d83783091485fb82c24ecbd711db7043a92cf6169746db24ad46a5904d694aea9d3c3aa96da725f0
2d23820ee11678d567c75f94c40011ed9f0e274f refactor: remove dead branches in `SingletonClusterImpl` (Lőrinc)
Pull request description:
Found during review: [cluster mempool: control/optimize TxGraph memory usage](https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423058928)
### Fixes
`SplitAll()` always calls `ApplyRemovals()` first, for a singleton, it empties the cluster, therefore any `SingletonClusterImpl` passed to `Split()` must be empty.
`TxGraphImpl::ApplyDependencies()` first merges each dependency group and asserts the group has at least one dependency. Since `parent` != `child`, `TxGraphImpl::Merge()` upgrades the merge target to `GenericClusterImpl`, therefore the `ApplyDependencies()` is never dispatched to `SingletonClusterImpl`.
### Coverage proof:
* https://maflcko.github.io/b-c-cov/fuzz.coverage/src/txgraph.cpp.gcov.html#L1446
* https://storage.googleapis.com/oss-fuzz-coverage/bitcoin-core/reports/20251103/linux/src/bitcoin-core/src/txgraph.cpp.html#L1446
ACKs for top commit:
instagibbs:
ACK 2d23820ee11678d567c75f94c40011ed9f0e274f
sipa:
ACK 2d23820ee11678d567c75f94c40011ed9f0e274f
Tree-SHA512: 5135913206c800d5344df61c6654f00917cb85567bc5b821576c7891805cf7689bf47968434a06517d09183dadfefc257d24c42b55a7b99486a4c9b11fc523af
Now that the __func__ is no longer used, the
NOLINTBEGIN(bugprone-lambda-function-name) can be removed.
Also, re-format the NONFATAL_UNREACHABLE macro, while touching the
adjacent line.
038849e2e09bb9f4ce1fb5a1f291745506c6a52d clang-tidy: Remove no longer needed NOLINT (Hennadii Stepanov)
Pull request description:
From https://github.com/bitcoin/bitcoin/pull/33714/files#r2491476516:
> Actually, the `NOLINT` was fixed and can be removed? You've confirmed that it is undeclared on the listed platforms, so it can't be hit by `readability-redundant-declaration`
ACKs for top commit:
maflcko:
lgtm ACK 038849e2e09bb9f4ce1fb5a1f291745506c6a52d
l0rinc:
I wanted to ask the same on the original PR but forgot - ACK 038849e2e09bb9f4ce1fb5a1f291745506c6a52d
Tree-SHA512: c0b24235425e80baeac3158c7169122364f31140367bc289430d34f01cd38f9f6a3931319f6fe4e1dc86bc4d87e21a5b4b8a2263c199e8083593f89ce592a177
Performance likely does not matter here, but from a perspective of
code-readablilty, a const reference should be preferred for read-only
access.
So use it here.
This requires to set -Wno-error=dangling-reference for GCC 13.1
compilations, but this false-positive is fixed in later GCC versions.
See also https://godbolt.org/z/fjc6be65M
Without this, compile warnings could be hit about __func__ being only
valid inside functions.
warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function]
note: expanded from macro Assert
115 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
| ^
Ref https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486258473
