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
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
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
`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
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
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
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
a51df9b0ec test: tolerate race condition in interface_http.py (Matthew Zipkin)
Pull request description:
Fixes#35632 by allowing both outcomes of a race condition. The server behavior is unchanged: in response to a malformed request we send an error code and disconnect. The issue is that sometimes on Windows the RST is caught by the platform and the receive buffer is discarded before the Python client can process it with recv().
We can also be much more polite to misbehaving clients by implementing a lingering close using SO_LINGER as suggested in #35780 but that will require more review.
The exact error in #35632 is hard to produce reliably but there are a few close options for reviewers. I tested this on windows native building with MSVC. In both of these cases the patch from this PR caught the error and passed the test.
**RemoteDisconnected: Remote end closed connection without response**
```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 9bb89863af..62324d3fea 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -1072,7 +1072,7 @@ std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequest(const std::shared_
e.what());
// We failed to read a complete request from the buffer
- WriteNoStoreErrorReply(*client->m_req, HTTP_BAD_REQUEST);
+ // WriteNoStoreErrorReply(*client->m_req, HTTP_BAD_REQUEST);
client->m_disconnect = true;
return nullptr;
}
```
**ConnectionResetError: [WinError 10054] An existing connection was forcibly closed by the remote host**
```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 9bb89863af..be52acb874 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -1154,6 +1154,11 @@ bool HTTPRemoteClient::MaybeDisconnect(std::chrono::time_point<SteadyClock> now,
"Disconnecting HTTP client %s (id=%llu)",
m_origin,
m_id);
+ auto sock{GetSock()};
+ linger opt{};
+ opt.l_onoff = 1; // enable SO_LINGER
+ opt.l_linger = 0; // zero timeout
+ sock->SetSockOpt(SOL_SOCKET, SO_LINGER, &opt, sizeof(opt));
return true;
}
```
ACKs for top commit:
jeanpablojp:
re-ACK a51df9b0ec
winterrdog:
tACK a51df9b0ec
janb84:
re ACK a51df9b0ec
hodlinator:
re-ACK a51df9b0ec
sedited:
ACK a51df9b0ec
Tree-SHA512: a6244581b2b51af647452e0dc8cd09cdc8d975dee6a0dc8b8064cad136023dad68b4af987303bced91a662bf5fae22871ea718a6a8e68024158a9aef6c5855ef
59ebf558f3 qa: Use IP_PORTRANGE_HIGH on OpenBSD for dynamic port allocation (Hennadii Stepanov)
Pull request description:
The default ephemeral port range on OpenBSD (1024-49151) overlaps with the test framework's static port range starting at `TEST_RUNNER_PORT_MIN`, the same way FreeBSD's does (see #34346).
Extend `set_ephemeral_port_range()` to OpenBSD. The socket option and its values are identical to FreeBSD's, so only the platform check changes.
ACKs for top commit:
maflcko:
lgtm ACK 59ebf558f3
theStack:
utACK 59ebf558f3
Tree-SHA512: 680235cf3e1799361796c0ff36d5f19bf74f79393057dbd7b38b0e92a7df3af669873c66ca1e82f1c78999c20351663b8960990f3f209af89ee18ce0773eb7de
Fixes#35632 by allowing both outcomes of a race condition.
The server behavior is unchanged: in response to a malformed request
we send an error code and disconnect. The issue is that sometimes
on Windows the RST is caught by the platform and the receive buffer
is discarded before the Python client can process it with recv().
We can also be much more polite to misbehaving clients by
implementing SO_LINGER as suggested in #35780 but that will require
more review.
The default ephemeral port range on OpenBSD (1024-49151) overlaps with
the test framework's static port range starting at TEST_RUNNER_PORT_MIN,
the same way FreeBSD's does (see #34346).
Extend `set_ephemeral_port_range()` to OpenBSD. The socket option and
its values are identical to FreeBSD's, so only the platform check
changes.
A client streaming pipelined requests into a busy connection
(or any connection whose replies are slower than the sender) could grow
server memory without limit, up to remote OOM.
Stop selecting RecvEvent for clients whose request is being processed;
pipelined data then backs up in the kernel socket buffer, applying TCP
backpressure to the sender. One request per connection is in flight
at a time.
Functional test streams pipelined submitblock requests into a connection
blocked on waitforblockheight. Unpatched builds continue draining the
socket buffer indefinitely, patched builds will stall.
`ReplaceAll()` substitutes fixed tokens in notification commands and other strings.
PR #25803 replaced the Boost helper with `std::regex_replace()`, treating searches as regular expressions and substitutes as replacement-format syntax.
Restore literal, non-recursive replacement so callers match fixed tokens and preserve replacement bytes exactly, while avoiding a new string when the search text is absent.
Co-authored-by: Rob Hamilton <6456095+Rob1Ham@users.noreply.github.com>
`-walletnotify` shell-escapes wallet names before substituting `%w` into the configured command.
`ReplaceAll()` uses `%w` as the regex pattern and the escaped wallet name as replacement text, where `$'` copies the command suffix into the escaped name and allows its shell metacharacters to alter the command.
Record the command execution, missing notification file, regex pattern matching, replacement expansion, and non-recursive replacement.
fa7be0a8df test: refactor: Remove confusing ignore_errors=True (MarcoFalke)
Pull request description:
There is an unexplained `ignore_errors=True` in the internal `_initialize_chain` helper:
```py
shutil.rmtree(cache_path('fees'), ignore_errors=True)
```
This is fine, because no error should happen. But it is a bit confusing, because an ignored error may lead to a later error anyway.
Fix that by failing early instead.
Also, re-write the simple block to `pathlib`.
ACKs for top commit:
willcl-ark:
ACK fa7be0a8df
Tree-SHA512: c533a8aebd92f3f1054563f20af438165632c98f7a2f189f3306420780468b143c24001f794a79ddfc0527c9605a4cfe59949648a9a7f41bbe138128b09f0a6e
ff3e2e4ebd net: Trigger process abort when behind start block MTP (Hodlinator)
1883cecb4d test: Characterize lagging-clock headers presync (Hodlinator)
Pull request description:
### Problem
Headers presync computes `m_max_commitments` from the elapsed time since the chain-start MTP plus `MAX_FUTURE_BLOCK_TIME`. When the local system clock is more than `MAX_FUTURE_BLOCK_TIME` behind the chain-start MTP, that elapsed value is negative, but it is used in arithmetic assigned to the unsigned commitment cap. This can turn the intended zero bound into a large cap, letting low-work headers presync continue instead of aborting when a reasonable commitment cap would have been exceeded.
### Fix
Instead of allowing an invalid `HeadersSyncState` object to be created, emit an error and **abort the node process**.
Typically, the node will detect that the system clock is set too far in the past when comparing it to the chain tip during chain state loading and shut down before we start syncing headers. So in practice this is very unlikely to make a difference (might be possible if the system clock jumps backwards after we loaded the chain state).
#### Commits
* Add functional and unit characterization tests [pinning the current behavior](https://github.com/bitcoin/bitcoin/pull/35260).
* The fix, along with corresponding test changes.
---
Replaces #35208 which was clamping `m_max_commitments` to zero and then letting the `HeadersSyncState` consume headers until the block height either reached the the next `commitment_period` point and aborted, or reached the minimum work threshold and succeeded (possible when having been offline for >144 blocks).
ACKs for top commit:
l0rinc:
diff and code review ACK ff3e2e4ebd
sedited:
ACK ff3e2e4ebd
mzumsande:
Code Review ACK [ff3e2e4](ff3e2e4ebd)
Tree-SHA512: bdd82fd0609309aa4bea026db1b607ae856c53403ec01b2511fa2ccae9db4ff1bb9e39523b446583c09ae53823275b8a603050d9090b61fabb84fab35e458f28
This ensures that UTXO unserialization errors abort the node, and does
not cause a consensus divergence.
A valid UTXO is created and shared between two nodes. The raw database
entry is then deliberately modified on one node so it can no longer be
deserialized. When the other node spends that UTXO and mines a block,
the node with the unserializable entry must abort during block connection
rather than silently treating the coin as absent and marking the block
BLOCK_FAILED_VALID, which would cause it to permanently diverge from the
network's best chain.
21d4e0ba75 rpc, wallet, test: fix invalid JSON in HelpExampleRpc curl examples (GuTS805)
Pull request description:
Several `HelpExampleRpc` call sites reused CLI-style argument strings
verbatim instead of valid JSON — missing commas, bare unquoted words, or
single backslashes that are not valid JSON escapes. As a result the
documented `curl` command for 14 RPCs (`getblockfrompeer`, `addnode`,
`addconnection`, `sendmsgtopeer`, `restorewallet`, `getmempoolcluster`,
`importmempool`, `getindexinfo`, `listlabels`, `unloadwallet`,
`createwalletdescriptor`, `addhdkey`, `loadwallet`, `listunspent`) fails
to parse as JSON if copy-pasted as-is. Also fixes a stray trailing quote
in the `restorewallet` named-argument examples.
This was previously raised in #31275, which sipa confirmed at runtime by
adding a `UniValue::read` check, but that PR was closed unmerged. Since
then two more examples broke the same way (`getmempoolcluster`,
`addhdkey`), which is why this adds a permanent regression check to
`rpc_help.py::dump_help()` instead of just fixing the current list.
Fixes#35864.
ACKs for top commit:
maflcko:
review ACK 21d4e0ba75🚝
sedited:
ACK 21d4e0ba75
Tree-SHA512: 2a8abc07d681b9dc81b8079a68421278da890049cea33a1561a48d53cbf919a30df588f559e9df94fa4a1ab7027f742f3b12c163afc25246a620340cb3522336
The PSBT sighash type field is a 32 bit unsigned integer in BIP 174,
signed in PSBTInput, and it is not validated when deserialized.
decodepsbt incorrectly truncates this field before looking up its
name. Fix that and add a test.
`LocateErrors()` returns multiple useful positions for character and checksum errors, but an overlength string has one structural error.
Every character from the limit onward is outside the permitted address, so listing each position adds no diagnostic value.
`validateaddress` converts every returned position into a `UniValue` number before serializing the response.
An authenticated request below the HTTP body limit can therefore require several gigabytes of memory.
Return only the first position beyond the length limit, which identifies where the violation begins.
Character and checksum errors continue to report multiple useful positions when they can be determined, and the existing unit and functional tests cover both behaviors.
Several HelpExampleRpc call sites reused CLI-style argument strings
verbatim (missing commas, bare unquoted words, or single backslashes
that are not valid JSON escapes), producing curl examples that fail
JSON parsing as documented. Also fixes a stray trailing quote in the
restorewallet named-argument examples, a missing comma in the
listunspent example, and a wrong-schema string-instead-of-array
listunspent argument caught in review.
Lines touched are converted to raw string literals (or strprintf with a
raw string template) throughout, for consistency and to avoid manual
quote/backslash escaping.
Adds a regression check to rpc_help.py::dump_help() so this class of
bug can't silently reappear.
558e26e66e test: cover OP_SUCCESSx bypassing the initial stack element size limit (ViniciusCestarii)
Pull request description:
BIP-342 specifies that the initial stack resource checks happen after OP_SUCCESSx processing, and explicitly notes the checks "can be bypassed using OP_SUCCESSx". Core implements this correctly, but there are currently no tests covering this behavior. This means a consensus-breaking change to the ordering could pass the test suite undetected. Verified this on local commit 68d24d7430, which mutates to incorrectly implement the order and CI still turns green.
Add a new test at feature_taproot.py to cover OP_SUCCESSx bypassing the initial stack element size limit.
Verified that the new test catches the mutant: f8f42a13a8.
ACKs for top commit:
instagibbs:
ACK 558e26e66e
Tree-SHA512: 66d7bbbf286bf7e5c5762704e8c0f835c6a8026d7d604263e9debcd7e71df80506c0238b8a15cda3d4ba245d9c2bc46e79a1986d66583e571e208c8dfbe66156
Since 31.0 has a compatibility issue with wallets containing miniscript
descriptors, this should be in the test, with a test for the failure
condition.
If a descriptor is being reimported, we should only update the metadata
and cache from the other one, rather than overwriting the entire thing.
This avoids a potential issue where the on-disk record is overwritten
with a backwards incompatible string.
On some systems, such as NetBSD, the non-default
`-rpcmaxconnections=128` is too high, so bitcoind refuses to start:
```
Error: Not enough file descriptors available. 256 available, 290 required.
```
The test only needs a value above the default of 16. Use 64 and lower
`-maxconnections` in that case so the total fits in 256.
Since bitcoin/bitcoin#35730 the HTTP server reserves file descriptors
for its listen sockets and for `-rpcmaxconnections` connected clients
(16 by default), so `min_required_fds` in init.cpp grew.
On select()-based platforms `available_fds` is capped at FD_SETSIZE,
which is 256 on NetBSD. The previous value of 94 no longer fits and
every node in the test suite started up with a warning, which the
framework treats as unexpected stderr and fails on.
Recompute the value with the new accounting (256 - 179 = 77) and
update the comment to match the current variable names in init.cpp.
e4d80e7001 test: close the loop after the network thread has completed (Vasil Dimov)
29fba5ddbb test: close the listeners before terminating the event loop (Vasil Dimov)
Pull request description:
Whenever a test creates a new `P2PInterface` object a new listener is
created inside `NetworkThread.create_listen_server()` by calling
`cls.network_event_loop.create_server()`.
These listeners are never closed which might result in:
```
2026-06-10T22:13:35.3934880Z Task was destroyed but it is pending!
2026-06-10T22:13:35.3936020Z task: <Task pending name='Task-54' coro=<BaseSelectorEventLoop._accept_connection2() done, defined at /opt/homebrew/Cellar/python@3.14/3.14.5/Frameworks/Python.framework/Versions/3.14/lib/python3.14/asyncio/selector_events.py:217> wait_for=<Future finished result=None>>
```
when the event loop is closed.
Fix that by closing the listeners.
Fixes: https://github.com/bitcoin/bitcoin/issues/35508
ACKs for top commit:
andrewtoth:
ACK e4d80e7001
sedited:
ACK e4d80e7001
Tree-SHA512: b93d06526b4eb31ac445a1a0e379e5ec947661f8ea29f2e07ac88b9e4760b0cc5348638b320ab8d60735f34fc163fcdd18eba42b20e0a7726a98a5136433bd64
1cb416397b psbt: avoid duplicate taproot leaf script keys when merging (Shuvam Pandey)
Pull request description:
Follow-up to #35665, which fixed the same combiner defect for `PSBT_GLOBAL_XPUB`. thomasbuilds
and winterrdog asked for this one as its own PR when I reported it there.
`m_tap_scripts` maps a leaf script to a set of control blocks, but is serialized as one record
per control block, keyed by the control block (`SerializeToVector(s, PSBT_IN_TAP_LEAF_SCRIPT,
std::span{control_block})`). `PSBTInput::Merge` unions it by the map key, so two PSBTs that map
the same control block to different leaf scripts merge into an input that serializes the `0x15`
key twice. Duplicate keys make a PSBT invalid, so it is the same `combinepsbt` then
`decodepsbt` failure as the xpub case, at the input level. Present since #22558 (v24.0).
Both decode on their own, and differ only in the leaf script the control block maps to, `OP_1`
against `OP_1 OP_1`:
```
$ A=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAIhXAUJKbdMGgSVS3i0tgNel6XgeKWg8o7JbVR7/ums6AOsACUcAAAA==
$ B=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAIhXAUJKbdMGgSVS3i0tgNel6XgeKWg8o7JbVR7/ums6AOsADUVHAAAA=
$ bitcoin-cli -regtest decodepsbt "$(bitcoin-cli -regtest combinepsbt "[\"$A\",\"$B\"]")"
error code: -22
error message:
TX decode failed Duplicate Key, input key "15c050929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0" already provided: unspecified iostream_category error
```
winterrdog reproduced it on the #35665 thread with another pair.
Merge the records rather than the map entries, keeping the leaf script already there. BIP 174
lets the combiner "pick arbitrarily when conflicts occur", and unknown and proprietary records
already resolve that way. Refusing to combine is the BIP's other option, but that would fail
`combinepsbt` on input it accepts today.
Merging by map key drops records as well. `std::map::insert` leaves existing keys alone, so
when both PSBTs carry the same leaf script with different control blocks, the incoming set was
dropped. Those keys do not conflict, so merging per record keeps them.
The control blocks already present are collected once per merge rather than searched for per
incoming record, which would be quadratic in the size of the two PSBTs `combinepsbt` takes from
the caller.
Since this is the second field with this shape I checked the rest. `m_xpubs` (#35665) and
`m_tap_scripts` are the only two whose record key comes from the value, so two map entries can
serialize the same key. `partial_sigs` is keyed by `CKeyID` and serialized under the pubkey,
but the pubkey determines the `CKeyID`, so those records stay distinct. The others key the
record by the map key, `m_proprietary` included, and `PSBTOutput` has no such field.
The test fails on master on both counts, and covers the merges that do not conflict as well.
I found this with a local assertion in the psbt fuzz target that a combined PSBT must
roundtrip. That assertion can go in a follow-up.
Tested:
```
./build/bin/test_bitcoin --run_test=psbt_tests
./build/bin/test_bitcoin --run_test=psbt_wallet_tests
./build/test/functional/test_runner.py rpc_psbt.py rpc_rawtransaction.py wallet_taproot.py wallet_signer.py feature_taproot.py wallet_basic.py
```
ACKs for top commit:
achow101:
ACK 1cb416397b
winterrdog:
re-ACK 1cb416397b
Tree-SHA512: 2599beffe701ba9b672e8dcc3d43844f3853ceeb3d86fc53798a428d3288aa8edd2e42f338b32a1046fa558d6cc5cfa5d55b3a0aaf960278b0f3002158381623
436921eb46 test: check joinpsbts preserves global xpubs and proprietary fields (Thomas)
011094b282 rpc: preserve global xpubs and proprietary fields in joinpsbts (Thomas)
Pull request description:
`joinpsbts` collects the global xpubs of all the joined PSBTs into `merged_psbt`, but returns a separately constructed `shuffled_psbt` into which only the inputs, outputs, and unknown fields are copied. The collected `PSBT_GLOBAL_XPUB` records are silently dropped, and `PSBT_GLOBAL_PROPRIETARY` records are not collected at all.
The xpub collection was added in #17034, which was written against a `joinpsbts` that still returned `merged_psbt`, but was merged after #16512 had introduced the `shuffled_psbt` rebuild, so the collected xpubs have never reached the result.
Shuffle the inputs and outputs of `merged_psbt` in place instead of rebuilding a new PSBT, so that all global data is preserved, and union the global proprietary records in the merge loop, matching the `combinepsbt` behavior from #34893.
ACKs for top commit:
jpk68:
ACK 436921eb46
achow101:
ACK 436921eb46
winterrdog:
tACK 436921eb46
Tree-SHA512: d9de34c25aecc29b6b4fb80d6584fa919cc5ff9b7ef2f4d8ce35c4043fe7638fefb8af10448f2cd14021f5d25e149f0efc8798c5b8c9bc8b5582c6152010e891
b42f7fade0 descriptor: don't prepend key origins twice (Shuvam Pandey)
7b15e2cb44 descriptor: fix duplicate check for hardened keys (Shuvam Pandey)
Pull request description:
Fixes#34273.
Importing a descriptor that uses the same `musig()` participants twice in one
tapleaf, with different musig subderivations, fails with
`is not sane: contains duplicate public keys`. It only fails when one of the
participants is a private key on a hardened path. The all-xpub version of the
same descriptor imports fine. That's what gave it away.
The duplicate check (`KeyCompare`) resolves each key expression to a pubkey and
compares the results. It does this at index 0, and the old code used an empty
signing provider. With that empty provider, a `musig()` expression can't resolve
when one of its participants is on a hardened path, because deriving that
participant needs its private key, so the whole aggregate key comes back empty.
Two different musig expressions both came back empty, so the check treated them
as duplicates. The fix derives against the signing provider populated during
parsing, which holds the private keys, and only compares the expression strings
when neither side resolves. 151henry151 had suggested looking at the empty
signing provider on the issue.
scgbckbone found a second, separate bug in the same descriptors. When another
expression that reuses those participants is handled in the same expansion, its
participant origin in the PSBT is added twice, so `m/86h/1h/0h` becomes
`m/86h/1h/0h/86h/1h/0h` in both the input and output Taproot BIP32 derivation
maps. `OriginPubkeyProvider::GetPubKey()` now derives into a temporary provider,
merges it, and writes the corrected origin once, so a later expression can't
prepend the same origin again.
Tested:
```
./build/bin/test_bitcoin --run_test=descriptor_tests
./build/bin/test_bitcoin --run_test=miniscript_tests
./build/bin/test_bitcoin --run_test=bip328_tests
./build/bin/test_bitcoin --run_test=psbt_wallet_tests
./build/test/functional/test_runner.py wallet_musig.py --jobs=1
```
ACKs for top commit:
achow101:
ACK b42f7fade0
scgbckbone:
ACK b42f7fade0
Tree-SHA512: ab36caa6bc484fa1fc3289c79e9a3d713278d82f80de478e53e1bdbe645037e07776ac798eba085733abc139c11a9dbf0d3f49d3c0eee9632e4ddf33d2242f92
m_tap_scripts maps a leaf script to a set of control blocks, but is serialized
one record per control block, keyed by the control block. PSBTInput::Merge
unions it by the map key, so two PSBTs that map the same control block to
different leaf scripts merge into an input serializing the 0x15 key twice.
Duplicate keys are invalid, so combinepsbt hands back a PSBT that can no longer
be decoded. Present since #22558 (v24.0).
Merge the records instead of the map entries, keeping the leaf script already
there, as BIP 174 lets the Combiner pick arbitrarily when conflicts occur. The
control blocks already present are collected once rather than searched for per
incoming record, which would be quadratic in the size of the PSBTs.
Control blocks under a leaf script that both PSBTs carry are now kept as well,
where the map level union dropped them.
The test covers conflicting and non-conflicting merges, including an incoming
leaf script whose control blocks only partly conflict, so records that do not
conflict are not dropped alongside those that do.
bd4b1524ea init: do not count file descriptors for HTTPServer if -server=0 (Matthew Zipkin)
b08662060d init: account for maximum file descriptors needed by HTTP (Matthew Zipkin)
cc2acebefb http: configure simultaneous connection limit with -rpcmaxconnections (Matthew Zipkin)
b3d6d2d1a7 http: limit connected clients to 16 (Matthew Zipkin)
86651d8197 scripted-diff: Rename nUserBind, nBind, nMaxConnections to snake_case (Matthew Zipkin)
Pull request description:
Introduces a new configuration option `-rpcmaxconnections` with default value `16`. This is used to limit the number of simultaneous `HTTPClient` connected to the `HTTPServer`. When the limit is reached, new pending connections remain queued in the kernel's socket buffer. Those connections have complete TCP handshakes with the kernel but do not occupy any application memory.
The previous libevent-based HTTP server had no limit on connections but it did have a limit on the kernel socket queue:
e7ff4ef2b4/http.c (L3510)
```c
if (listen(fd, 128) == -1) {
```
The current HTTP server, like the p2p server, uses a platform constant here:
b6becf3534/src/httpserver.cpp (L743)
(on my macOS `SOMAXCONN` is `128` but on my Debian machine it's `4096`)
The default of 16 was chosen as a reasonable upper bound for single-user RPC use cases. Systems designed to handle more simultaneous HTTP connections than this (previously relying on the absence of a limit) can adjust the setting.
## File descriptors
Because of the connection limit, we can now account for the maximum number of file descriptors needed by the HTTP server. This addresses several issues (#11368#11322 maybe #27732) that could have been fixed by a PR waiting in vain for a libevent release (#27731).
## Bonus performance improvement
The new limit is managed in a loop that drains the kernel's socket queue with `accept()`. All pending connections from the queue (up to the limit) are processed in one single call to `SocketHandlerListening()`. The previous code would only accept one connection from the queue on each I/O loop tick, with a `SELECT_TIMEOUT` (50ms) sleep between each.
ACKs for top commit:
fjahr:
tACK bd4b1524ea
janb84:
ACK bd4b1524ea
winterrdog:
tested ACK bd4b1524ea
hodlinator:
Concept ACK bd4b1524ea
willcl-ark:
ACK bd4b1524ea
Tree-SHA512: 2ef7a96da4d7037c7343ec0ea03fda5bb55d10c2a071fce4929141297515923b203d3d338dbcb6599849768f52aa3c9da509fb5d1d6f7c574a1d2034ea2a9e74
