9c7748315d build: avoid pipe2 on Darwin (for now) (fanquake)
Pull request description:
macOS 27 will support `pipe2` at runtime, and Xcode 27 (and Command Line Tools) support it at compile time. This means a macOS < 27 system will detect support for `pipe2`, but then binaries will crash at runtime, as pipe2 is not available.
Just avoid `pipe2` on macOS for now, and continue using `pipe`. Note that the compilation also produces availability warnings:
```bash
[415/1121] Building CXX object src/util/CMakeFiles/bitcoin_util.dir/tokenpipe.cpp.o
../src/util/tokenpipe.cpp:89:9: warning: 'pipe2' is only available on macOS 27.0 or newer [-Wunguarded-availability-new]
89 | if (pipe2(fds, O_CLOEXEC) != 0) {
| ^~~~~
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/sys/unistd.h:219:9: note: 'pipe2' has been marked as being introduced in macOS 27.0 here, but the deployment target is macOS 26.0.0
219 | int pipe2(int [2], int);
| ^
../src/util/tokenpipe.cpp:89:9: note: enclose 'pipe2' in a __builtin_available check to silence this warning
89 | if (pipe2(fds, O_CLOEXEC) != 0) {
| ^~~~~
90 | return std::nullopt;
91 | }
```
and this will need to be backported. When macOS 27 is released, we could change approach, but wanted to PR something straightforward (and backportable) for `32.x`.
ACKs for top commit:
hebasto:
ACK 9c7748315d, tested on macOS Tahoe 26.6.2 with CLT 27.0:
willcl-ark:
ACK 9c7748315d
Tree-SHA512: 5aa97b93e7038344a5118eef2660b101e443eff52ab94fb4b8eb88ec4e09b8a3000b9b4e2aa59b406d8acbb6d9add8f0c3a2dd5e1db53b1dfc97a0fcbaf48995
28b69e2988 http: stop processing requests from a client when send buffer is full (Matthew Zipkin)
Pull request description:
This is a follow-up to #36123 and applies a second throttle mechanism to the send-side. If a misbehaving client refuses to read data from the socket, the server will now stop processing requests instead of packing more and more responses to `m_send_buffer` without bound.
After we parse a complete request from a client, **before** we dispatch it to a worker, we quickly lock and check the size of `m_send_buffer`. If there's already 32MiB of data there (reusing `MAX_BODY_SIZE` here, open for bikeshedding...) we do not dispatch the request to a worker, leaving it in place as `m_req`.
This was found and disclosed responsibly by the Red Team 🟥.
ACKs for top commit:
hodlinator:
ACK 28b69e2988
janb84:
re ACK 28b69e2988
sedited:
ACK 28b69e2988
Tree-SHA512: ecefdf6406fa40aa28fe284fd4b5a148a0e4ef84b857e373cc7a55c84bac5c7a928b09c088d9ebe5a2adadfbd95a5ae1644f024947ed891da067130d1aa84ae6
macOS 27 will support pipe2 at runtime, and Xcode 27 (and Command Line
Tools) support it at compile time. This means a macOS < 27 system will
detect support for pipe2, but then binaries will crash at runtime, as
pipe2 is not available.
Just avoid pipe2 on macOS for now, and continue using pipe. Note that
the compilation also produces availability warnings, and this will need
to be backported.
ecdf9db906 test: get_previous_releases.py use `PREVIOUS_RELEASES_DIR` (David Gumberg)
Pull request description:
`test_framework.py` already uses `PREVIOUS_RELEASES_DIR` to set a default directory to find previous releases, so should `get_previous_releases.py` when fetching them.
This is more convenient for local development when using multiple worktrees where having `$PREVIOUS_RELEASES_DIR` set is useful.
Also allows some CI code to get deleted.
_LLM Usage Disclosure: Qwen3.8-0.5B generated the next 800,000 pages of my epistolary novel while I made this PR._
ACKs for top commit:
hodlinator:
ACK ecdf9db906
willcl-ark:
ACK ecdf9db906
Tree-SHA512: cd99ff118bdf0425b286ad1efb99be3b53997e4f0915722923e79081ee0820c23a2177b2b4a45153d1d5e2963363b5b59d7840e203d7722e46204f9bee2f6485
b3a9b84b37 doc: Correct upstream-pull reference (littleyier)
Pull request description:
Notice that the refspec in `doc/productivity.md` maps to `upstream-pull/NUMBER`, but the doc mentions `upstream-pull/NUMBER/head` right below it.
Dropping the trailing `/head` so the `git show` / `git chekcout` examples actually match up. Tested both locally in a scratch repo and they work fine now.
ACKs for top commit:
sedited:
ACK b3a9b84b37
Tree-SHA512: e5bb1b093d6cc1445988c5c74c7282efa39edf599093719ccb31a8ac11ab9bf4f4857cfb8fe48af558fd99ac25e2b5a106f487a8d022bdb3074cc1bc3824fa88
3c91db262b net: Update embedded asmap to 1788801420 (Fabian Jahr)
Pull request description:
Depends on #36199 and https://github.com/bitcoin-core/asmap-data/pull/70 getting merged and receiving enough attestations. The run just happened so I hope we'll get these ACKs before the end of the week.
ACKs for top commit:
hodlinator:
ACK 3c91db262b
Tree-SHA512: a93aa1e25a7484e79eb7f8fa1d4871ee5a1356c2d9638a503729e31e629ae9448d4c0b82d8016cf8b0c1f7ec2d86ee03a9b14b27a5c3e12b39fb7cff4232086b
8976ba50a2 rpc: require connman in getnetworkinfo (Ruslan Kasheparov)
975229580f rpc: Fix inaccuracies in RPC help docs (Ruslan Kasheparov)
Pull request description:
Align RPC help documentation with actual implementation.
ACKs for top commit:
sedited:
ACK 8976ba50a2
willcl-ark:
ACK 8976ba50a2
Tree-SHA512: 9cea335aa1a74e3c598ce43cfa9a99b992b4436203e9b5e90a9e8b3bbd3cbf7ef3d5f112f426c7d13ba7b0ff9723f84be8fbfff23bdf940de51ef15708ea5ac4
`test_framework.py` already uses `PREVIOUS_RELEASES_DIR` to set a
default directory to find previous releases, so should
`get_previous_releases.py` when fetching them.
This is more convenient for local development when using multiple
worktrees where having `$PREVIOUS_RELEASES_DIR` set is useful.
Also allows some CI code to get deleted.
Prevents a memory exhaustion case where a misbehaving client
refuses to read responses and drain the socket buffer. Instead of
packing more data on to the server-side m_send_buffer, stop
dispatching requests from the client to workers
7d54a88b44 iwyu: Fix warnings in `src/rpc` and treat them as errors (Hennadii Stepanov)
462ff18d79 iwyu, refactor: Fix includes in some source files (Hennadii Stepanov)
Pull request description:
This PR continues the ongoing effort to enforce IWYU warnings.
See [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#using-iwyu).
ACKs for top commit:
stickies-v:
re-ACK 7d54a88b44
Tree-SHA512: aeeb8e2851e4012c4e8474c524fafae01f6c323537d533c693e9b018ee2752cedf19b35b3ac0433a287b2ad7953ae0a466d20ddd3a00e854985d2404abce2512
b48a27691d fuzz: assert invalid addresses are not routable (Fabian Jahr)
e6c775c6d6 net: treat RFC 9637 new IPv6 documentation range as invalid (Fabian Jahr)
Pull request description:
[RFC 9637](https://www.rfc-editor.org/rfc/rfc9637.html) was newly introduced in 2024 but we don't handle this yet. This pull handles it in the same way as RFC 3849 of which 9637 is an extension.
This came up in #36196 because the new `GetMappedAS()` benchmark from https://github.com/bitcoin/bitcoin/pull/35285 asserts on these addresses being unmapped. New maps using `--fill` couldn't be embedded because the filling assigns these addresses. Making this range invalid in the code fixes the benchmarks.
ACKs for top commit:
0xB10C:
ACK b48a27691d
willcl-ark:
ACK b48a27691d
hodlinator:
re-ACK b48a27691d
sedited:
ACK b48a27691d
Tree-SHA512: d28552c0620f2b4c7de311d9ddf90bffb0f2dfdac4d76e305fc6b8138c8f3f070958a8a50679b45e1875cb5b24f6a7a10297ad327200b3ed5cd5ab5338187bcc
a7f264373e Merge bitcoin-core/secp256k1#1923: group: VERIFY input/output ge/gej/fe exhaustively
3f6ee3e989 Merge bitcoin-core/secp256k1#1931: tests: cover rejection of invalid plain seckey alongside a valid one
05f9289984 tests: silentpayments: cover rejection of invalid plain seckey alongside a valid one
978d3fa7f9 Merge bitcoin-core/secp256k1#1932: silentpayments: drop "empty key arrays must be NULL" requirement
ad3688d1e5 changelog: mention dropped NULL requirement for silentpayments key arrays
89e3a70662 silentpayments: drop "empty key arrays must be NULL" requirement
c36269ef95 group/refactor: Use constant expression for readability
031dbae659 group: Export secp256k1_ge_set_ge(j)_zinv properly in group.h
4f3bd158d5 group: Cover all input/output ge/gej/fe with VERIFY calls
0711443e6d group: Add missing FE_VERIFY checks on rzr outputs
1bd7492181 group: Remove redundant SECP256K1_GEJ_VERIFY
cddd125f72 group: Split functions into pre/post VERIFY and _impl
aa0af2fac4 Merge bitcoin-core/secp256k1#1928: tests: add coverage for exact-size DER signature serialization
500ccb60f7 tests: add coverage for exact-size DER signature serialization
a9a61831bd Merge bitcoin-core/secp256k1#1924: tests: add coverage for the DER long form length encoding
ddb1dfa770 tests: add coverage for the DER long form length encoding
1c8babcd6c Merge bitcoin-core/secp256k1#1922: field: Check that argument of _fe_set_int() is a constant
bae6b89a20 Merge bitcoin-core/secp256k1#1908: refactor: replace `_get_hash_context` with direct `->hash_ctx` access
f6d821db03 field: Fix typo in comment
1ce0066e60 field: Check that argument of _fe_set_int() is a constant
c8745f6b3a refactor: replace `_get_hash_context` with direct `->hash_ctx` access
3397f3cb28 Merge bitcoin-core/secp256k1#1878: field: correct `_fe_half` docs (output is not normalized, input requires magnitude <= 31)
86c7cbaf5a Merge bitcoin-core/secp256k1#1915: refactor: Move (de)ser helpers from musig and eckey to group
bfd40bd0ce refactor: Rename privkey to seckey in eckey helpers
d8ee6e6cf6 refactor: Move parsing helpers from musig to group
a37d7cc064 Merge bitcoin-core/secp256k1#1916: ecdh/ellswift: simplify seckey loading with `_scalar_set_b32_seckey`
3d69f4ce1c Merge bitcoin-core/secp256k1#1911: refactor: rename `ctx` param to `ecmult_gen_ctx` where applicable
c00dc81810 ecdh/ellswift: simplify seckey loading with `_scalar_set_b32_seckey`
81a5a7567d refactor: rename `ctx` param to `ecmult_gen_ctx` where applicable
2577fb1888 refactor: Move pubkey parsing and serialization from eckey to group
439278a649 Merge bitcoin-core/secp256k1#1910: scratch: reject sizes that overflow when added to header
3d4340d173 scratch: reject sizes that overflow when added to header
56e031a148 field: correct `_fe_half` docs (output is not normalized, input requires magnitude <= 31)
git-subtree-dir: src/secp256k1
git-subtree-split: a7f264373e5b187536c21e434f632b571b3ab178
a31eb8bfa4 ci, iwyu: Request IPC file generation explicitly (Hennadii Stepanov)
Pull request description:
Fixes a part of https://github.com/bitcoin/bitcoin/issues/35361:
> ... other sporadic error output:
>
> ```
> [1](https://github.com/bitcoin/bitcoin/actions/runs/26292243180/job/77395542650#step:11:11661)
> error: no such file or directory: '/home/admin/actions-runner/_work/_temp/build/src/ipc/capnp/common.capnp.c++'
> error: no input files
> error: unable to handle compilation, expected exactly one compiler job in ''
> error: no such file or directory: '/home/admin/actions-runner/_work/_temp/build/src/ipc/capnp/common.capnp.proxy-client.c++'
> error: no input files
> error: unable to handle compilation, expected exactly one compiler job in ''
> error: no such file or directory: '/home/admin/actions-runner/_work/_temp/build/src/ipc/capnp/common.capnp.proxy-server.c++'
> error: no input files
> error: unable to handle compilation, expected exactly one compiler job in ''
> error: no such file or directory: '/home/admin/actions-runner/_work/_temp/build/src/ipc/capnp/common.capnp.proxy-types.c++'
> error: no input files
> ```
ACKs for top commit:
ryanofsky:
Code review ACK a31eb8bfa4. Thanks for the updates! This is a minimal fix getting rid of IWYU "fatal error" output from IPC files in the iwyu CI job.
willcl-ark:
ACK a31eb8bfa4
Tree-SHA512: 7059c3adbb1f8cb5f69149b45e180b01753dc93c852829eccbc70e6ddcfa212dec45a0e8625c6cb0ea3b8ee05f8f17181f36fd4d277e8f6cc9fe4578b0228166
46654094be lint: Use C.UTF-8 locale only in shell scripts (Hennadii Stepanov)
982ee64938 lint: Skip `libmultiprocess` subtree in `lint-shell-locale.py` (Hennadii Stepanov)
1194918a5d scripted-diff: Use C.UTF-8 locale in all shell scripts (Hennadii Stepanov)
Pull request description:
This unifies the used locales across the entire codebase.
Additionally, the `test/lint/lint-shell-locale.py` linter has been adjusted accordingly.
Also see https://github.com/bitcoin/bitcoin/pull/35775#issuecomment-5047323736.
ACKs for top commit:
fanquake:
ACK 46654094be
Tree-SHA512: e72e076614602937c5fe6ca27d0bb7bebe4464ef28455c43a1bd1d700ffeeea684365fa5749cb1e5fdad56178a1e88a544b5854783b57aef468efb105a03af57
4200f8163a policy: fix negative CFeeRate::ToString() formatting (joaonevess)
Pull request description:
`CFeeRate` may represent modified/effective fee rates, for example after a negative `prioritisetransaction` fee delta.
Previously, `CFeeRate::ToString()` formatted the quotient and remainder directly. For negative values, C++ `%` produces a negative remainder, which could result in malformed strings such as `0.-01 sat/vB`.
This changes formatting to emit the sign once and format non-negative quotient/remainder parts. Positive fee rate formatting is unchanged.
This is a display-only change and does not affect fee calculation or policy behavior.
ACKs for top commit:
polespinasa:
ACK 4200f8163a
winterrdog:
Re-ACK 4200f8163a
sedited:
ACK 4200f8163a
Tree-SHA512: bf499078040154ac7828d0a58248b725156b7cfd763a8d6a2e48bcbd77f195a5e0ca9d3a973becdc7436e0bafb46a22cb49dd7ae947f98b434bed7d048d18397
ec2adf3c51 test: Check miniscript descriptor h and apostrophe equivalence (w0xlt)
a2d001b57c test: Enforce descriptor reimport is an update (Ava Chow)
e2b2f1c5c6 descriptor: Rename DescriptorID to CompatDescriptorHash (Ava Chow)
6ad31c062c test: Add 31.0 to wallet backwards compatibility test (Ava Chow)
2a6c53371b wallet, spkm: Treat Descriptor ID as an opaque SPKM ID (Ava Chow)
62e826fa76 wallet: Update WalletDescriptor from another one instead of overwriting (Ava Chow)
1113f7590e wallet, export: Include descriptor cache when exporting descriptors (Ava Chow)
9fc7b2618b spkm: Remove DescriptorSPKM constructor that doesn't take a descriptor (Ava Chow)
770ff64bd7 test: Add v30.2 and Miniscript to wallet backwards compatibility test (Ava Chow)
35d6a60dbf descriptor: Add ToCanonicalString (Ava Chow)
1d87af26ce descriptors: Remove default StringType from PubkeyProvider::ToString() (Ava Chow)
1c7f9aaf75 miniscript: Don't use StringType::COMPAT (Ava Chow)
Pull request description:
Since keys in Miniscript expressions were not correctly handling `StringType::COMPAT` when generating the Descriptor ID, in order to keep compatibility with previous versions, we need to continue to handle that enum incorrectly when computing the ID.
Given that this it the second time that we have had this issue, this PR also drops the concept of Descriptor ID being something that we can validate. Instead, the ID read in from the database is treated as an opaque blob that is used only to tie together the records related to a particular SPKM. It is instead treated as a ScriptPubKeyMan ID and users of it must be retrieving the ID from somewhere rather than computing it from a descriptor. The check of comparing the read ID to the computed ID is removed so that all previously created wallets can be read.
To clarify that the ID is not actually an ID, the function `DescriptorID` is renamed to `CompatDescriptorHash` and it is still used to generate the SPKM ID that is written to the database.
The ID was additionally being used to determine whether a descriptor is equal to another descriptor. This was used only by `importdescriptors` and `createwalletdescriptor`. These uses have been changed to do a string comparison rather than computing a hash and comparing the hashes. This removes the need to rely on `CompatDescriptorHash`.
The only caveat is that previously the hash was being used to do a map lookup in `m_spk_managers`, but this is now changed to use `std::find_if`. The lookup complexity changes from logarithmic to linear, which may be really bad for wallets with a lot of descriptors, e.g. migrated formerly non-HD wallets. I think in general though, the tradeoff is okay, and neither of these functions purport to be performant, especially as `importdescriptors` may also do a rescan which can take a long time. However, if that is a concern, an additional map of `CompatDescriptorHash` to DescriptorSPKM can be added.
Lastly, the wallet backwards compatibility test is updated to have 30.2 and 31.0 nodes, and a wallet with miniscript expressions. This exercises both creating wallets in previous versions and making sure they load in master, and making new wallets on master and checking whether they load, depending on the version.
Fixes#35432
ACKs for top commit:
pseudoramdom:
ACK ec2adf3c51
davidgumberg:
crACK ec2adf3c51
w0xlt:
ACK ec2adf3c51
Tree-SHA512: a32995c171b829a874cfd1bb03adde46fd8737322d5c44bc2ff27eff1ea8742c16c7ea1bb6fdc0fb2b89d0f11919850383799c3af126e7f55fe0878a8f1a7024
e07224f2d5 depends: fix IPC listeners on macOS dying when accepting a dead socket (xyzconstant)
Pull request description:
This PR adds a depends patch to fix a Cap'n Proto bug present in macOS that throws the following exception after accepting a disconnected client:
```
mp/proxy.cpp:48: error: Uncaught exception in daemonized task.; exception = kj/async-io-unix.c++:1365: failed: setsocketopt(IPPROTO_TCP, TCP_NODELAY): Invalid argument
```
This goes uncaught killing `libmultiprocess`'s accept loop (`mp::ListenConnections()`) in the process.
### Root cause
After `accept()`, kj unconditionally calls `setsockopt(TCP_NODELAY)` on the connection socket. For Unix sockets, kj handles the usual "not supported" error codes, however, on macOS EINVAL is returned when the client has already disconnected and kj only tolerates that code when built for FreeBSD. The unexpected error then goes uncaught in the accept loop, causing the listener to stop accepting new connections.
See https://github.com/bitcoin-core/libmultiprocess/issues/319 for more details and instructions on how to reproduce it in Bitcoin Core using socat.
### Fix
The patch is based on the EINVAL handling from capnproto/capnproto@7df5bd078, which is only available on the not yet released v2 branch. It's applied to `capnp.mk` as a temporary workaround until capnproto/capnproto#2748 is available in a v1 release.
ACKs for top commit:
hebasto:
ACK e07224f2d5, I have reviewed the code and it looks OK.
ryanofsky:
Code review ACK e07224f2d5. Confirmed patch matches what was committed upstream (85d8e47fcb).
Tree-SHA512: 7d3b3dd6cabdc658971e0f20159ad36594759b3686d2e5dc74fb21900ccec0c637980e36fa71fbd808c642ce83f89c73f5bc788eb42644bceed67acb78e8eee5
ea785a31f7 psbt: preserve sighash type when merging inputs (Thomas)
Pull request description:
`PSBTInput::Merge` copies every optional input field from the other input when it is absent locally, except `PSBT_IN_SIGHASH_TYPE`. So `combinepsbt` silently drops the sighash type whenever the first PSBT does not carry it, making the result depend on the argument order.
The field is what lets finalizers enforce the sighash type of existing signatures (BIP 174). When it is lost, `FinalizePSBT` falls back to the default type (`SIGHASH_ALL`, or `SIGHASH_DEFAULT` for taproot inputs), rejects signatures made with any other type as a sighash mismatch, and the PSBT can no longer be finalized. Combining a PSBT signed with `ALL|ANYONECANPAY` after a merely updated copy of the same PSBT reproduces this: `finalizepsbt` reports it as incomplete, while the reverse order finalizes and broadcasts fine.
Merge the sighash type like the other optional fields, keeping the one already present, and test both combine orders.
ACKs for top commit:
achow101:
ACK ea785a31f7
winterrdog:
Re-ACK ea785a31f7
vicjuma:
ACK ea785a31f7
rkrux:
lgtm ACK ea785a31f7
Tree-SHA512: 3487368509926c3dc0218dfab2e08273676504ad5ed4635e12e56c0484bda4cd94f4ba6f2df26ee4a902ce9a274546f727f8eca4a62eb0add3a700b2141eb272
75f64e50c6 test: exercise node abort on UTXO deserialization failure (furszy)
4652cd0d82 txdb: detect UTXO deserialization errors via CDBWrapper::TryRead() (furszy)
5dfbb91b6c dbwrapper: add TryRead() to distinguish errors from valid outcomes (furszy)
f78834fac9 test: add missing coverage for CDBWrapper::Read() errors (furszy)
Pull request description:
Early note: the majority of this PR consists of test coverage. The changes per se are small.
If a UTXO entry on disk can't be deserialized, the node currently treats it as if the coin
wouldn't exist instead of aborting with an error. A non-existing coin has a very specific
meaning for consensus: any block that spends it would be permanently rejected as invalid
(`BLOCK_FAILED_VALID`), silently forking the node from the rest of the network. This can't
currently be triggered in practice (details below), but it's still the wrong behavior.
The root cause is that `CDBWrapper::Read()` returns `false` for both missing entries and
deserialization failures, so `CCoinsViewDB::GetCoin()` has no way to tell them apart.
`CCoinsViewErrorCatcher` was built to catch database read errors and abort, but it never
fires during deserialization errors because `CDBWrapper::Read()` swallows the exception
before it can propagate. This [comment](8a8edc8d88/src/coins.cpp (L398-L411)) in `ExecuteBackedWrapper()` spells out the code
intent very clearly.
As mentioned initially, this can't happen in practice today. It would require either a bug
in the coin serialization path, or a memory corruption before the data reaches LevelDB
(at which point we have bigger problems). Random disk-level bit flips are caught earlier
by LevelDB's verification (`verify_checksums=true`, enabled by default), which already
propagates correctly as `DB_INTERNAL_ERROR`. Regardless, a db read issue should
never be silently misinterpreted as a consensus violation.
This PR adds `CDBWrapper::TryRead()`, which returns a `ReadStatus` that lets callers
discriminate between all possible outcomes. `CCoinsViewDB::GetCoin()` switches on the
result and throws on any error, letting `ExecuteBackedWrapper()` do what it was designed
to do. `CDBWrapper::Read()` becomes a thin wrapper over `TryRead()`, preserving backward
compatibility for all other callers (so we don't have to change non-consensus code here).
`PeekCoin()` is also covered, as it delegates to `CCoinsViewDB::GetCoin()` at the database
level.
The idea of the PR is to go slowly over the code changes, first commit locks-in the current
`CDBWrapper::Read()` behavior . The second adds `TryRead()` with tests for all four
status codes. The third is the `CCoinsViewDB::GetCoin()` fix. The fourth is a functional
that ensures the node aborts correctly instead of silently diverging.
Testing Notes:
Cherry-picking the functional test commit on master demonstrates the consensus split
when the coin entry fails to deserialize.
Extra Note:
`CDBIterator::GetValue()` has the same silent-swallow pattern. Not consensus-critical.
Should be addressed in a follow-up.
ACKs for top commit:
ajtowns:
reACK 75f64e50c6
sedited:
ACK 75f64e50c6
mzumsande:
Code Review ACK [75f64e5](75f64e50c6)
Tree-SHA512: 51b0114ea443544a2f1fbb8e63be6e1dff94d6f287221d566dbc98d666784a2b4c486acfb87eea5392bc1d092fb6d6dc0ff6782bcdccdcf15939281c895e384d
82deb69111 PSBT: Make input/output `Merge()` methods return void (nebula-21)
Pull request description:
PSBT input/output `Merge()` methods always return `true` unconditionally and have no failure paths. As a result, the return value checks in `PartiallySignedTransaction::Merge()` can never fail and are dead code.
This makes the `bool` return type and `[[nodiscard]]` misleading.
This PR changes both methods to return `void` and remove the return value checks. If at some point in the future the failure logic is needed, it can be introduced again. For now using `void` makes the current behavior easier to understand.
ACKs for top commit:
achow101:
ACK 82deb69111
polespinasa:
ACK 82deb69111
sedited:
ACK 82deb69111
Tree-SHA512: 694fdf19292d2f3627c90c5f111d7ecf5ad4f933d69eebf7c9ef37adee19be7de44002030ed4539aa5a28910acb751822b443f7ee22c80614296ef40d21c60da
`PSBTInput::Merge` copies every optional input field from the other
input when it is absent locally, except `PSBT_IN_SIGHASH_TYPE`. So
`combinepsbt` silently drops the sighash type whenever the first PSBT
does not carry it, making the result depend on the argument order.
The field is what lets finalizers enforce the sighash type of existing
signatures (BIP 174). When it is lost, `FinalizePSBT` falls back to the
default type (`SIGHASH_ALL`, or `SIGHASH_DEFAULT` for taproot inputs),
rejects signatures made with any other type as a sighash mismatch, and
the PSBT can no longer be finalized. Combining a PSBT signed with
`ALL|ANYONECANPAY` after a merely updated copy of the same PSBT
reproduces this: `finalizepsbt` reports it as incomplete, while the
reverse order finalizes and broadcasts fine.
Merge the sighash type like the other optional fields, keeping the one
already present, and test both combine orders.
a34fc8b11a wallet: handle disabled startup settings (Robert Hamilton)
b7113e6f42 test: characterize disabled wallet settings (Robert Hamilton)
Pull request description:
I hit a crash while creating a new wallet in Bitcoin-Qt 31.1 on an Apple silicon Mac with `nosettings=1`. After looking through the crash report and code, I traced it to saving the wallet's load-on-startup setting: the settings writer throws when dynamic settings are disabled.
Wallet RPCs report errors with `-nosettings` after changing wallet state. In Qt, the same settings write causes an uncaught exception.
Return a persistence failure when dynamic settings are disabled so wallet operations finish with their existing startup-setting warning. This avoids an uncaught exception in Qt and RPC errors after the wallet state has already changed. Keep in-memory and no-op updates unchanged.
The first commit adds functional coverage for the current behavior. The second adds the fix, updates the assertions to expect success with warnings, and documents that failed settings writes keep the in-memory changes.
### Manual Reproduction
Run on the parent commit and the fixed commit, using a fresh temporary regtest data directory each time:
```sh
{ cmake -B build-wallet-review -DBUILD_GUI=ON && cmake --build build-wallet-review -j --target bitcoin-qt; } >/dev/null 2>&1
build-wallet-review/bin/bitcoin-qt -regtest -datadir="$(mktemp -d)" -nosettings -noconnect
```
Choose `File` > `Create Wallet...`, enter `repro`, leave the defaults unchanged, and click `Create`.
Before the fix, the application terminates with:
```text
libc++abi: terminating due to uncaught exception of type std::logic_error: Attempt to write settings file when dynamic settings are disabled.
```
After the fix, the wallet is created and the application displays:
```text
Wallet load on startup setting could not be updated, so wallet may not be loaded next node startup.
```
ACKs for top commit:
l0rinc:
tested ACK a34fc8b11a
kevkevinpal:
tACK a34fc8b11a
achow101:
ACK a34fc8b11a
jeanpablojp:
tACK a34fc8b11a
Tree-SHA512: 5e43028200478f89e71ebe7e0fc28c559f15e713226124899a69eb90d413d8ecaaaca02267d5a848068d70555b3e4334993f414de2debf6a22d73a51a71d1acd
1fca81960a psbt: fix rendering for invalid long sighash type field (Sjors Provoost)
Pull request description:
The `decodepsbt` incorrectly truncates the (32 bit) sighash type field before looking up its human friendly name. It's not dangerous, as such a signature would be invalid, but potentially confusing.
Fix that and add a test.
I plan to use `SighashToStr` in another pull request to render an error message for invalid sighash type field values, but it seemed worth fixing in a standalone PR.
ACKs for top commit:
jeanpablojp:
ACK 1fca81960a
achow101:
ACK 1fca81960a
winterrdog:
tACK 1fca81960a
rkrux:
lgtm ACK 1fca81960a
Tree-SHA512: 74f9206e53f7b72f251f9e0feab68abd8e1a1d99c976314675aba8231fb7c36ee484f21f443b8fee4a7518c9c5125f03c7121b6d9aa94ebb293271b0cc90cc0f
1dad06eff3 remove stale canonical form claim from getdescriptorinfo help (Craig Raw)
Pull request description:
`getdescriptorinfo` describes its `descriptor` result as:
> The descriptor in canonical form, without private keys.
The returned string is a re-serialisation of the parsed descriptor with private keys removed. It is not a canonical form: descriptors that describe the same wallet routinely come back as different strings with different checksums, and this is deliberate.
Three things the RPC does not canonicalise:
- **The hardened derivation marker.** #26076 added `m_apostrophe` so that the marker the caller used is preserved rather than rewritten (`src/script/descriptor.cpp:262`, `:512`); it first shipped in v26.0. Before that, `FormatHDKeypath()` emitted `'` unconditionally (v25.0, `src/util/bip32.cpp:54`), so `h` supplied by the caller was rewritten — the behaviour #15740 objected to under the name "canonicalize". Where a single key expression mixes both markers, the style of its last hardened element is applied to the whole expression, which is still input-dependent.
- **Key order in `multi()` / `sortedmulti()`.** The order is preserved as given. For `sortedmulti()` the written order carries no meaning at all, since BIP 383 sorts the derived keys when the output script is built, so the same wallet has n! equally valid descriptors.
- **The checksum.** The `checksum` field is computed over the *input* string (`src/rpc/output_script.cpp:215`), so it can differ from the checksum embedded in the returned `descriptor` — e.g. when a private key was supplied.
```
$ bitcoin-cli getdescriptorinfo "sortedmulti(1,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)" | jq -r .descriptor
sortedmulti(1,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)#fne5696l
$ bitcoin-cli getdescriptorinfo "sortedmulti(1,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)" | jq -r .descriptor
sortedmulti(1,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)#w5gau8hw
```
Both produce the same output script; neither is more canonical than the other. The same applies to the marker:
```
$ bitcoin-cli getdescriptorinfo "wpkh([f6bb4c63/0h/0h/30h]028429a37c3f09c8c5cc1fab58df32d1a7da7616c748a40eeb1aae1d64acb9c5cc)" | jq -r .descriptor
wpkh([f6bb4c63/0h/0h/30h]028429a37c3f09c8c5cc1fab58df32d1a7da7616c748a40eeb1aae1d64acb9c5cc)#vk9vfu0h
$ bitcoin-cli getdescriptorinfo "wpkh([f6bb4c63/0'/0'/30']028429a37c3f09c8c5cc1fab58df32d1a7da7616c748a40eeb1aae1d64acb9c5cc)" | jq -r .descriptor
wpkh([f6bb4c63/0'/0'/30']028429a37c3f09c8c5cc1fab58df32d1a7da7616c748a40eeb1aae1d64acb9c5cc)#5wdxpxcx
```
The wording dates from v0.18.0 (`src/rpc/misc.cpp:153`), where it did describe the behaviour, and has been carried forward unchanged since. Whether the RPC should canonicalise was settled in #15740 in favour of round-tripping what the caller supplied; this only brings the description into line with that outcome.
I have deliberately not replaced the phrase with "normal form" or "normalized". Those terms already denote a different transformation in this codebase — BIP 380's "Normalization of Key Expressions with Hardened Derivation", implemented as `Descriptor::ToNormalizedString()` ("Normalized descriptors have the xpub at the last hardened step", `src/script/descriptor.h:140`) — which `getdescriptorinfo` does not perform. Dropping the qualifier avoids the collision.
Documentation only; no behaviour change. The phrase occurs nowhere else in the repo.
```diff
-{RPCResult::Type::STR, "descriptor", "The descriptor in canonical form, without private keys. For a multipath descriptor, only the first will be returned."},
+{RPCResult::Type::STR, "descriptor", "The descriptor, without private keys. For a multipath descriptor, only the first will be returned."},
```
If reviewers would rather the help positively state what is preserved (hardened marker and key order as supplied, checksum recomputed), I am happy to expand it; I have kept the change minimal.
ACKs for top commit:
l0rinc:
ACK 1dad06eff3
Eunovo:
ACK 1dad06eff3:
rkrux:
lgtm ACK 1dad06eff3
Tree-SHA512: 7b03100fdbc71c867d26094eb3975dfe967875a977ef3c7264b8bb3b920db319d6f11dbde1d39bb70fff2b2288eca42aadffc215ab7503dcc7b3bf62fc2692e5
9f0543d69a wallet: remove unused DatabaseOptions members (jeanpablo)
bbed824a64 wallet: remove unused warnings parameter from CreateFromDump (jeanpablo)
2f6aa41d3d wallet: remove unused WalletDatabase::m_refcount (jeanpablo)
f64b3fa70f wallet: remove unused CHDChain keypool index members (jeanpablo)
4afc7bc40d wallet: remove unused COutput::ToString (jeanpablo)
a0e9aac428 wallet: remove unused DescriptorScriptPubKeyMan::AddDescriptorKey (jeanpablo)
Pull request description:
Six unused items in src/wallet, one per commit.
`DescriptorScriptPubKeyMan::AddDescriptorKey`, a private wrapper that
lost its caller in #28333.
`COutput::ToString`, no callers. It was used by `COutput::print()`,
which went away with the other `print()` methods in wallet.
The two `CHDChain` keypool index members, whose last uses went away with
`LegacySPKM` in #28710.
`WalletDatabase::m_refcount`. Only BDB ever maintained it, and BDB went
away in #28710.
The `warnings` parameter of `CreateFromDump`, never written, along with
the loop that printed it in wallet-tool. The `push_back` went away with
the `-format` option in #31250.
The two BDB-only members of `DatabaseOptions`, `use_shared_memory` and
`max_log_mb`. Their last readers went away with BDB in #28710, along
with the `-privdb` and `-dblogsize` options that set them.
ACKs for top commit:
pablomartin4btc:
ACK 9f0543d69a
vicjuma:
ACK 9f0543d69a
Tree-SHA512: 0996043a116ee2c653b8c3e2987fbe6d5c4573db1477dfc1b06a433334af160c44382bb1ed0fe234dcaeda09f15b07c7e5d2c2f81fce63c780ef4cac1c30fb29
e014e5bb61 miner: Enforce murch-zawy rule (BIP54) (Fabian Jahr)
Pull request description:
Opened separate from #35793 as [requested by darosior](https://github.com/bitcoin/bitcoin/pull/35793#discussion_r3704817804). This makes the miner enforce the murch-zawy rule for which #35793 adds the validation part.
A node whose clock is behind the first block of the difficulty period currently reports a mintime below the consensus floor in getblocktemplate and fails to build a valid template for the last block of the period so it can't mine until its clock catches up. This is mostly a theoretical concern on mainnet because it would require a huge system clock misconfiguration. It might be a bigger concern on test networks with volatile hashrates. But generally, I think our miner should be able to create valid templates in any situation.
ACKs for top commit:
kevkevinpal:
crACK [e014e5b](e014e5bb61)
darosior:
ACK e014e5bb61
sedited:
ACK e014e5bb61
Tree-SHA512: 299f83459e92654e028ce1c27470e1dcaae4a58de10687659860cdb425d3af330d3b8da6d8ca8f727c9eaebb2f0a3f3af5185f9daa57e92133435cb80a9e365c
74ddf1c0a0 refactor: use structured bindings for map entries (Lőrinc)
21d5d5cb73 rpc: append unique container keys directly (Lőrinc)
23e512a58e rpc: avoid quadratic prioritised transaction JSON (Lőrinc)
Pull request description:
**Problem:** `getprioritisedtransactions` lets node operators inspect fee adjustments.
While building the response, the RPC checks each transaction ID against all previous IDs, even though duplicates are impossible.
The same unnecessary search appears in a few other RPC responses built directly from `std::map` or `std::set` keys.
**Fix:** Each changed response key comes from a `std::map` or `std::set`, where keys are unique, so insertion can skip the linear `findKey()` call.
**Reproducer:** On a RPi 4, the test below took almost a minute before the fix and about half that time after.
The other changed map and set loops perform the same per-key search, so their response construction has the same quadratic-to-linear scaling as the number of entries grows.
<details>
<summary>Reproducer commands</summary>
```patch
diff --git a/test/functional/mining_prioritisetransaction.py b/test/functional/mining_prioritisetransaction.py
--- a/test/functional/mining_prioritisetransaction.py
+++ b/test/functional/mining_prioritisetransaction.py
@@ -11,6 +11,7 @@ from test_framework.blocktools import NORMAL_GBT_REQUEST_PARAMS
from test_framework.messages import (
COIN,
MAX_BLOCK_WEIGHT,
+ ser_uint256,
)
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
@@ -215,4 +216,10 @@ class PrioritiseTransactionTest(BitcoinTestFramework):
assert_raises_rpc_error(-1, "getprioritisedtransactions",
self.nodes[0].getprioritisedtransactions, True)
+ self.log.info("Test getprioritisedtransactions order")
+ txids = [ser_uint256(i).hex() for i in range(20_000, 0, -1)]
+ self.nodes[0].batch([self.nodes[0].prioritisetransaction.get_request(txid, 0, 1) for txid in txids])
+ assert_equal(list(self.nodes[0].getprioritisedtransactions()), txids[::-1])
+ self.clear_prioritisation(self.nodes[0])
+
# Test `prioritisetransaction` invalid `txid`
```
</details>
ACKs for top commit:
sedited:
ACK 74ddf1c0a0
hodlinator:
re-ACK 74ddf1c0a0
Tree-SHA512: 0e9204a3dab448f370c37f668dc877c689b6cfd273242ab72fc54551717f58fec00ff6f199b009d8980a5549f1cea1607f91c6afc1e06e6aa86c154d2a15cb0d
d180b891a2 test: add mixed P2SH/witness sigop accounting (Lőrinc)
6e60c362bc test: add P2SH sigop counting coverage (Musa Haruna)
Pull request description:
Add test coverage for sigop counting in P2SH spends in `test_witness_sigops()`, addressing the existing TODO.
The new cases mirror the existing P2WSH sigop tests by constructing transactions that:
- remain below the block sigop limit (accepted),
- exceed the limit (rejected with bad-blk-sigops)
Since P2SH sigops are accounted as legacy sigops, the expected sigop cost accounts for the 4× legacy weighting applied during consensus validation.
The added coverage verifies the enforcement of the block sigop limit for both witness and P2SH spends, including mixed P2SH/witness transactions.
**Acknowledgement:** During review ([comment](https://github.com/bitcoin/bitcoin/pull/35164#pullrequestreview-4769420630)), **l0rinc** demonstrated, using mutation testing on his branch [here](https://github.com/l0rinc/bitcoin/pull/248), that the original test suite would not detect two consensus sigop undercounting bugs. Those experiments helped validate the coverage added by this PR and motivated the inclusion of the mixed P2SH/witness regression test.
ACKs for top commit:
l0rinc:
reACK d180b891a2
Bicaru20:
ACK d180b891a2
sedited:
ACK d180b891a2
Tree-SHA512: 795923f56316c3cad4d02a572ed6486a0f3f62bd524fb162d2bcd884108485974e96f38c08b5ff0a8659feecfc208c8d7e93dde2b9e0517c70f6343c2063d9b5
00a5f9b737 build: Remove `cmake/script/CoverageFuzz.cmake` (Hennadii Stepanov)
Pull request description:
The `gcov`-based `CoverageFuzz` script was introduced in 8b6f1c4353, as a CMake's replacement for the legacy `cov_fuzz` target. However, neither `cov_fuzz` nor `CoverageFuzz` has a documented usage.
Instead, #32206 documented compiling for fuzz coverage using the LLVM/Clang toolchain, which does not involve the `CoverageFuzz` script.
This PR removes the never-documented `CoverageFuzz` script, which is likely unused.
ACKs for top commit:
Crypt-iQ:
crACK 00a5f9b737
sedited:
ACK 00a5f9b737
Tree-SHA512: a0932f717d9ddf2540634728ee26a0bdf843bdcff0885b94b2d5df9de7df023afa4d90d644b7cfac7b50b7e4343114e87b7906ca55419f7ca353019f9072b03e
b57b0dbebd util: annotate `Split` input lifetime (Lőrinc)
34c5dc0583 util: annotate string view input lifetimes (Lőrinc)
Pull request description:
**Problem:** Several string utilities return or store views into their input.
A temporary `std::string` can leave these views dangling, although no current caller does this.
**Fix:** Add `LIFETIMEBOUND` so Clang diagnoses the misuse while preserving immediate use.
Pass the `Split` span by value so lvalue strings do not trigger false warnings.
ACKs for top commit:
kevkevinpal:
crACK b57b0db
stickies-v:
ACK b57b0dbebd
hodlinator:
ACK b57b0dbebd
sedited:
ACK b57b0dbebd
Tree-SHA512: 892b4c386d19dd9d46b36223084751d4be370bc985ad83283f3a2bffdd3a19f95ad107f1bf7ee4a85f90129b29553175b4713eb1308ee5176dbfa64ecff7e435
bcb09b3f4a qa: Verify HTTP listen port exclusivity (Hodlinator)
af65069fd1 windows: Use SO_EXCLUSIVEADDRUSE over SO_REUSEADDR (Hodlinator)
Pull request description:
#### Problem
`HTTPServer::BindAndStartListening()` unconditionally enables `SO_REUSEADDR` before binding the RPC listener. On Windows, a reuse-enabled listener does not reserve the port exclusively: another local process can request `SO_REUSEADDR` and bind to the same port (see https://learn.microsoft.com/en-us/windows/win32/winsock/using-so-reuseaddr-and-so-exclusiveaddruse).
If the competing socket receives a new connection, it can capture the HTTP Basic `Authorization` header (including the cookie credential) and proxy or issue privileged RPC calls as the victim. This crosses a local-user boundary and can expose wallet-controlling RPC credentials.
#### Fix
Have Windows use `SO_EXCLUSIVEADDRUSE` instead which makes the port exclusive to the process which first requests it, while retaining the restart-friendly behavior which `SO_REUSEADDR` enabled. Abort if another process is already bound to the port.
#### Further context & rationale
This issue is new in our homegrown HTTP server implementation, since libevent had a guard against setting `SO_REUSEADDR` on Windows, see `evutil_make_listen_socket_reuseable()` d82464a277/evutil.c (L483). libevent does not reference `SO_EXCLUSIVEADDRUSE`.
Why should we not just avoid `SO_REUSEADDR` on Windows and skip `SO_EXCLUSIVEADDRUSE` like the libevent approach?
Because setting either option makes the process less prone to failing to bind to a port after having been restarted. Not sure why this wasn't an issue before, maybe the node startup was usually slow enough to time out the port before we tried to re-bind it on Windows.
---
Discovered by Project Loupe.
ACKs for top commit:
pinheadmz:
ACK bcb09b3f4a
sedited:
utACK bcb09b3f4a
jeanpablojp:
tACK bcb09b3f4a
Tree-SHA512: 7f2362cc8399e8c4e95b27b39066d3e591b5aebfc2b562aba10786609456f526f562394818a3d1042d64dabf497548ffabf0757322cfb201454a134321108cf5
Return a persistence failure when dynamic settings are disabled so
wallet operations finish with their existing startup-setting warning.
This avoids an uncaught exception in Qt and RPC errors after the wallet
state has already changed. Keep in-memory and no-op updates unchanged.
Wallet RPCs report errors with -nosettings after changing wallet state.
Check these results alongside wallet usability, unchanged settings.json,
and restored startup preferences when settings are enabled again.
Reuse an existing wallet for loading and check explicit unloading last,
so the sequence does not depend on the skipped unload completion wait.
3d1004cb9b http: throttle per-connection reads while a request is in flight (Matthew Zipkin)
Pull request description:
This patches a memory exhaustion scenario found while auditing the new http server with kimi-k3. A shallow version of this scenario was addressed in #35735 (See https://github.com/bitcoin/bitcoin/pull/35735#discussion_r3720177656 and https://github.com/bitcoin/bitcoin/pull/35735#issuecomment-5217000202) but a OOM vector still remained.
On master when the sever is busy handling a request from a client, it will still read data from that client and "queue up" the next request. In #35735 we handled the scenario where that additional incoming data was an invalid HTTP request by not attempting to parse the data. However, we didn't add a size limit.
A misbehaving client could block its request queue with something like `waitforblock` and then flood the server with nonsense data without any limit.
The solution in this patch is to not even read from the socket at all if we are busy with a request. Similar to the intent of #35735, the kernel will buffer incoming data until backpressure kicks in and the TCP window drops to 0.
If unaddressed, the attack vector is still limited to authenticated clients: unauthenticated REST requests don't block for very long, so the server *should* be able to drain the receive buffer.
ACKs for top commit:
jeanpablojp:
tACK 3d1004cb9b
frankomosh:
ACK 3d1004cb9b
hodlinator:
ACK 3d1004cb9b
winterrdog:
tACK 3d1004cb9b
sedited:
ACK 3d1004cb9b
Tree-SHA512: 56f7678a9ab6789aa542c1f252df0b6ccf9137cb426ff915a0a3fe8285200fdb62b7a47c476ed8617c3592e7a7eac18158cd8c0dac309cdcf4e5fd887e016209
