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
69a640e05e test: Add coverage for unsatisfiable locktime combination in PSBT ComputeTimeLock (nebula-21)
Pull request description:
This PR adds a test case to `psbt2_timelock_test` covering an unsatisfiable locktime combination in `PartiallySignedTransaction::ComputeTimeLock()`.
When different PSBT v2 inputs specify their own timelock requirement, `ComputeTimeLock()` needs to reconcile all of those into a single locktime for the whole transaction. To reconcile this locktime, all the inputs locktimes need to be height or time-based, but not a mix of them.
The existing test already covers this failure when the input #0 is height-based and a later input is time-based, returning `std::nullopt`.
This PR adds the other case when the input #0 is time-based and a later input is height-based, returning `std::nullopt`.
I've basically swapped the PSBT inputs from the already existing case to cover this one.
ACKs for top commit:
sedited:
ACK 69a640e05e
Tree-SHA512: e7a7556df3bd278686a2d53a11b228f6f8c0e8dda79f050bad83dea89824de2e3766518fa3450b3722be86927fb4e6b9061dcd2cdb45010080b37a4fa2baffe0
852f201e09 validation: refactor: encapsulate Chainstate::m_target_blockhash (stickies-v)
Pull request description:
`m_target_blockhash` is paired with a mutable `m_cached_target_block` that must be kept in sync whenever the hash changes.
Refactor, no behaviour change.
Addresses https://github.com/bitcoin/bitcoin/pull/36137#discussion_r3903670739
ACKs for top commit:
kevkevinpal:
ACK [852f201](852f201e09)
purpleKarrot:
ACK 852f201e09
l0rinc:
code review ACK 852f201e09
alexanderwiederin:
ACK 852f201e09
sedited:
ACK 852f201e09
Tree-SHA512: 6243ee9979a2493b4f495a0156a119814854d7d91c48bb18777afae928ee2c3b0280ecba3d7516ffef25d92eb15d0a3e369005d43e68afed1142161d6bf4eeda
8e4b7ab725 fuzz: use per-level fetch scopes in coinscache_sim (Andrew Toth)
5292386b78 doc: improve CoinsViewOverlay documentation (Andrew Toth)
d552c52b08 coins: log error reason when prevout fetch submission fails (Andrew Toth)
2ffaa6e6a7 coins: delete Sync and SetBackend on CoinsViewOverlay (Andrew Toth)
330022993f coins: filter coinbase txid from parallel input fetching (Andrew Toth)
Pull request description:
This addresses various follow-ups requested in https://github.com/bitcoin/bitcoin/pull/35295.
- add the coinbase txid to the filter so inputs spending the coinbase are not fetched.
- delete Sync and SetBackend from CoinsViewOverlay
- various logging and documentation improvements
- improve coinscache_sim fuzzing so we continue parallel fetching while more caches are added on to the cache stack
ACKs for top commit:
optout21:
reACK 8e4b7ab725
l0rinc:
ACK 8e4b7ab725
sedited:
ACK 8e4b7ab725
Tree-SHA512: 38001f96be6f893e2610bb81f379ecc0c40ffd39da5bfe1f5db47db1ef2f725d80ae3f9b5e25acd64e65013176ba3ba4e3e8585cb55420b2793845c292beda23
`Split` can return views into its input, but annotating its old reference warns for lvalue strings.
Take the span by value so Clang follows the backing storage.
`Split<std::string>` copies results but can still warn, while `SplitString` is unaffected.
The string-view helpers return views into their input, while `LineReader` stores one.
Annotate their inputs so Clang can warn when a returned or stored view outlives a temporary string.
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 change removes the never-documented `CoverageFuzz` script, which is
likely unused.
4550801058 validation: use unused SetTargetBlockHash (fanquake)
Pull request description:
This was pointed out as unused in #36103 by jeanpablojp, but that seems like a mistake from #30214, where it was introduced. See: https://github.com/bitcoin/bitcoin/pull/36137#discussion_r3906377189.
ACKs for top commit:
stickies-v:
ACK 4550801058
ryanofsky:
Code review ACK 4550801058
Tree-SHA512: 93ccac48855d384f0443b5a25c79c5e6d720b6b77ad7a2bb52989382666e4e5f32c76dd7473428d6bbb503307ada7213021591ad54e463d9f8034fe2da97d10c
5ba9af6b69 ci: pass LIBCXX_INCLUDE_TESTS=OFF to LLVM build (fanquake)
feb3bd46e4 clang-tidy: remove some performance-* options (fanquake)
b4bd12d3d5 ci: use LLVM 23 in *san, fuzz, *cross jobs (fanquake)
Pull request description:
LLVM 23.1.0 was recently released, switch to using it across sanitizer, fuzzer and cross-compilation jobs.
ACKs for top commit:
hebasto:
ACK 5ba9af6b69, I have reviewed the code and it looks OK.
willcl-ark:
ACK 5ba9af6b69
Tree-SHA512: 4d203bf1ec6100a21d9a185a37365d358859bbde79f44f93d2e4f5e3c9686f57ca06d6c73da7423eb234dad5b9501d7a09029a430d23b2bcf8aba95f2d88e66d
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
This was pointed out as unused in #36103, but that seems like a mistake
from #30214, where it was introduced.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
e85e27976b rpc: detail x-bitcoin-unit in openrpc help (will)
Pull request description:
Addresses review comment about clarifying this field: https://github.com/bitcoin/bitcoin/pull/36131#issuecomment-5480592255
ACKs for top commit:
sedited:
ACK e85e27976b
Tree-SHA512: 7fd0bef8a5d37cd9d2778463b2193c58ec7cced1aa790a0a5807ef093bd51e729b6c8880e1c12af78293eb68abe791f4ae6e96e0457ce85be717e3e776f5406d
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
cc577de954 net: align v2 message type validation with v1 range (Bruno Garcia)
Pull request description:
BIP324 specifies the 13-byte long-form message type encoding as "an ASCII message type (as in the v1 P2P protocol)", but V2Transport::GetMessageType() accepted bytes up to 0x7F, while for V1 it only accepts printable ASCII (0x20-0x7E).
This changes V2 to match V1 on it and add test coverage.
ACKs for top commit:
nervana21:
tACK cc577de954
ajtowns:
utACK cc577de954
w0xlt:
ACK cc577de954
sedited:
ACK cc577de954
Tree-SHA512: 8c97ee20df2311949bbe9655c7e04507c4b47d3b18766aa6ae51691d0870f8a5c25ea54d74c9afb797754572d057b4240533da6bf3c2e0435f3cb32c5fb1c3af
fab80e82c1 test: Avoid unsafe memory race in baseindex_no_commit_ahead_of_flush (MarcoFalke)
fa0f14ef5e test: Avoid unsafe memory race in index_reorg_crash shutdown (MarcoFalke)
faf9c8e8a1 test: Clarify index.GetSummary().synced state in index_reorg_crash (MarcoFalke)
Pull request description:
Currently, the `index_reorg_crash` test may rarely crash due to UB in sanitizers like TSan or ASan. This is perfectly fine, because it is just a rare test-only issue.
However, fix it nonetheless by adding a missing drain of the unused in-flight events. Also, add a small check about the synced state while touching this test.
ACKs for top commit:
arejula27:
ACK fab80e82c1
furszy:
ACK fab80e82c1
Tree-SHA512: 4423e420421aa37d8b59e053f44c455fafb676102866bdf23988cf72f3d3f265b996bd953583ea8208f1534defb0e16b13ef08644be97e61959dc777a2918e5a
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.
db39de5601 doc: add `-walletnotify` security note (Lőrinc)
1f9dfabef6 refactor: use string views in `ReplaceAll` (Lőrinc)
469b0e59a2 util: make `ReplaceAll` literal (Lőrinc)
604d7e8fdd test: characterize walletnotify shell injection (Lőrinc)
4efaa6763a test: simplify `ReplaceAll` coverage (Lőrinc)
Pull request description:
**Problem:** On non-Windows builds, operators can configure `-walletnotify` to run a command for wallet transactions, with `%w` replaced by the shell-escaped wallet name.
An authenticated RPC caller allowed to create wallets can supply a name containing `$'`, request an address, and send a transaction to it.
While replacing `%w`, `ReplaceAll()` passes the escaped wallet name to `std::regex_replace()` as replacement text.
There, `$'` copies the command suffix into the escaped name, breaking its quote accounting and allowing shell metacharacters in the wallet name to alter the command.
`runCommand()` passes the result to `system()`, so a suitable command template could execute additional shell commands as the node process account.
It is not reachable over P2P or by an unauthenticated network peer.
#25803 introduced this behavior in v24 when it replaced Boost's literal substitution with `std::regex_replace()`.
**Fix:** Restore the literal, non-recursive contract `ReplaceAll()` had before #25803, matching every current caller's literal search and replacement text, while the wallet notification test covers a wallet name containing `$'`.
**Related:** #35833 restricts control characters in new wallet names, while this change fixes replacement metacharacters in `ReplaceAll()`.
This was found and disclosed responsibly by the Red Team 🟥.
ACKs for top commit:
maflcko:
re-ACK db39de5601💈
jeanpablojp:
re-ACK db39de5601
stickies-v:
re-ACK db39de5601
Tree-SHA512: 0be4adecfee50cb4dab90ae3386079767694a6b1fa1d7bd1f10ef73de88707b232f1ba4975a723c465a4d34d12296d501986c657d93bd8ae0bdced16afad1b5e
