Bitcoin Core

Merge bitcoin/bitcoin#36127: wallet: remove unused code

9f0543d69a1357371017c8289d9addc457b6f132 wallet: remove unused DatabaseOptions members (jeanpablo)
bbed824a64f0a5335442261b3abd63db22453cd6 wallet: remove unused warnings parameter from CreateFromDump (jeanpablo)
2f6aa41d3d195b8a4413efd35d10fa6cbd6cf368 wallet: remove unused WalletDatabase::m_refcount (jeanpablo)
f64b3fa70f6a318a03cfadec240c40a2b478adf1 wallet: remove unused CHDChain keypool index members (jeanpablo)
4afc7bc40d4ff4dd8205c757e3e851d813e17ee5 wallet: remove unused COutput::ToString (jeanpablo)
a0e9aac42802f6e22c5a227abcd16784d94d881d 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 9f0543d69a1357371017c8289d9addc457b6f132
vicjuma:
ACK 9f0543d69a1357371017c8289d9addc457b6f132

Tree-SHA512: 0996043a116ee2c653b8c3e2987fbe6d5c4573db1477dfc1b06a433334af160c44382bb1ed0fe234dcaeda09f15b07c7e5d2c2f81fce63c780ef4cac1c30fb29

Merge bitcoin/bitcoin#35949: miner: Enforce Murch-Zawy rule (BIP54)

e014e5bb61c117f0230a092439e4bbc98cd383f5 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](https://github.com/bitcoin/bitcoin/pull/35949/commits/e014e5bb61c117f0230a092439e4bbc98cd383f5)
darosior:
ACK e014e5bb61c117f0230a092439e4bbc98cd383f5
sedited:
ACK e014e5bb61c117f0230a092439e4bbc98cd383f5

Tree-SHA512: 299f83459e92654e028ce1c27470e1dcaae4a58de10687659860cdb425d3af330d3b8da6d8ca8f727c9eaebb2f0a3f3af5185f9daa57e92133435cb80a9e365c

Merge bitcoin/bitcoin#36096: rpc: avoid quadratic JSON construction when keys are unique

74ddf1c0a0ef8447686f59044b2fc2ee8d78c0e4 refactor: use structured bindings for map entries (Lőrinc)
21d5d5cb739062d40182cab66abac8eaf87a3c25 rpc: append unique container keys directly (Lőrinc)
23e512a58e69e16ede660ea46d0f6e15408bb371 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 74ddf1c0a0ef8447686f59044b2fc2ee8d78c0e4
hodlinator:
re-ACK 74ddf1c0a0ef8447686f59044b2fc2ee8d78c0e4

Tree-SHA512: 0e9204a3dab448f370c37f668dc877c689b6cfd273242ab72fc54551717f58fec00ff6f199b009d8980a5549f1cea1607f91c6afc1e06e6aa86c154d2a15cb0d

Merge bitcoin/bitcoin#35164: test: cover P2SH sigop counting in test_witness_sigops

d180b891a2904fb787ac5fa1ed66f56c98496698 test: add mixed P2SH/witness sigop accounting (Lőrinc)
6e60c362bc1e373a284911381e2a513f57f5f26b 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 d180b891a2904fb787ac5fa1ed66f56c98496698
Bicaru20:
ACK d180b891a2904fb787ac5fa1ed66f56c98496698
sedited:
ACK d180b891a2904fb787ac5fa1ed66f56c98496698

Tree-SHA512: 795923f56316c3cad4d02a572ed6486a0f3f62bd524fb162d2bcd884108485974e96f38c08b5ff0a8659feecfc208c8d7e93dde2b9e0517c70f6343c2063d9b5

Merge bitcoin/bitcoin#36161: build: Remove `cmake/script/CoverageFuzz.cmake`

00a5f9b7375ec8dca944e200bd9bef6a29949409 build: Remove `cmake/script/CoverageFuzz.cmake` (Hennadii Stepanov)

Pull request description:

The `gcov`-based `CoverageFuzz` script was introduced in 8b6f1c4353836bae6aa683cbc65251165bd031ba, 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 00a5f9b7375ec8dca944e200bd9bef6a29949409
sedited:
ACK 00a5f9b7375ec8dca944e200bd9bef6a29949409

Tree-SHA512: a0932f717d9ddf2540634728ee26a0bdf843bdcff0885b94b2d5df9de7df023afa4d90d644b7cfac7b50b7e4343114e87b7906ca55419f7ca353019f9072b03e

Merge bitcoin/bitcoin#36164: util: diagnose dangling views of temporary strings

b57b0dbebd56dac69d702e508af188e059db9c19 util: annotate `Split` input lifetime (Lőrinc)
34c5dc0583ddd49aa985e4100f6d395b7974afd5 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 b57b0dbebd56dac69d702e508af188e059db9c19
hodlinator:
ACK b57b0dbebd56dac69d702e508af188e059db9c19
sedited:
ACK b57b0dbebd56dac69d702e508af188e059db9c19

Tree-SHA512: 892b4c386d19dd9d46b36223084751d4be370bc985ad83283f3a2bffdd3a19f95ad107f1bf7ee4a85f90129b29553175b4713eb1308ee5176dbfa64ecff7e435

Merge bitcoin/bitcoin#36169: http: Use SO_EXCLUSIVEADDRUSE on Windows

bcb09b3f4aec73b5e17d1266ec21983d7add118c qa: Verify HTTP listen port exclusivity (Hodlinator)
af65069fd15bbfca5c93b6099a9cc1d5b8de30bc 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()` https://github.com/libevent/libevent/blob/d82464a277d0f42703702c4dfd9af6af38595a83/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 bcb09b3f4aec73b5e17d1266ec21983d7add118c
sedited:
utACK bcb09b3f4aec73b5e17d1266ec21983d7add118c
jeanpablojp:
tACK bcb09b3f4aec73b5e17d1266ec21983d7add118c

Tree-SHA512: 7f2362cc8399e8c4e95b27b39066d3e591b5aebfc2b562aba10786609456f526f562394818a3d1042d64dabf497548ffabf0757322cfb201454a134321108cf5

Merge bitcoin/bitcoin#36123: http: throttle per-connection reads while a request is in flight

3d1004cb9b8f27bd328d95b4c7524e878c296d61 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 3d1004cb9b8f27bd328d95b4c7524e878c296d61
frankomosh:
ACK 3d1004cb9b8f27bd328d95b4c7524e878c296d61
hodlinator:
ACK 3d1004cb9b8f27bd328d95b4c7524e878c296d61
winterrdog:
tACK 3d1004cb9b8f27bd328d95b4c7524e878c296d61
sedited:
ACK 3d1004cb9b8f27bd328d95b4c7524e878c296d61

Tree-SHA512: 56f7678a9ab6789aa542c1f252df0b6ccf9137cb426ff915a0a3fe8285200fdb62b7a47c476ed8617c3592e7a7eac18158cd8c0dac309cdcf4e5fd887e016209

Merge bitcoin/bitcoin#36130: test: add tests in transaction_tests.cpp covering live mutants

5ce3a0b4aab5ad9ec710e803f88d79139b3b3c44 test: cover legacy sigops count CHECKMULTISIG inaccurately (ViniciusCestarii)
a5fc82e2b1403b7bf0f1ad494a62ccff793f99d0 test: cover enforce BIP68 to tx versions higher than 2 (ViniciusCestarii)
bba1d4150ee8d4d4b4df2b91166dff564a756c09 test: cover IsFinalTx requires every input to be SEQUENCE_FINAL (ViniciusCestarii)

Pull request description:

Kills some live mutants on tx_verify.cpp that affect consensus found with https://github.com/ViniciusCestarii/mutant-harness. They are:

<details>
<summary>tx_verify.cpp (killed by 5c35785d6ddda80d5147616342e42d759490e6b9): <code>IsFinalTx</code>: sequence loop returns on the first input instead of requiring all of them</summary>

```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index e580a9d..46009a6 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -35,11 +35,7 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)
// also check that the spending input's nSequence != SEQUENCE_FINAL,
// ensuring that an unsatisfied nLockTime value will actually cause
// IsFinalTx() to return false here:
- for (const auto& txin : tx.vin) {
- if (!(txin.nSequence == CTxIn::SEQUENCE_FINAL))
- return false;
- }
- return true;
+ return std::ranges::any_of(tx.vin, [](const CTxIn& txin) { return txin.nSequence == CTxIn::SEQUENCE_FINAL; });
}

std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags, std::vector<int>& prevHeights, const CBlockIndex& block)
```

</details>

<details>
<summary>tx_verify.cpp (killed by 3ef559d9a5cb79e4721b68427ad679d9f4f6392a): <code>CalculateSequenceLocks</code>: <code>tx.version >= 2</code> -> <code>tx.version == 2</code></summary>

```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index e580a9d..0faaa55 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -54,7 +54,7 @@ std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags
int nMinHeight = -1;
int64_t nMinTime = -1;

- bool fEnforceBIP68 = tx.version >= 2 && flags & LOCKTIME_VERIFY_SEQUENCE;
+ bool fEnforceBIP68 = tx.version == 2 && flags & LOCKTIME_VERIFY_SEQUENCE;

// Do not enforce sequence numbers as a relative lock time
// unless we have been instructed to
```

</details>

<details>
<summary>tx_verify.cpp (killed by 1944eb409055d88eeaf7888b18a75289c506a943): <code>GetLegacySigOpCount</code>: <code>scriptSig.GetSigOpCount(false)</code> -> <code>GetSigOpCount(true)</code></summary>

```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index e580a9d..0b98597 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -120,7 +120,7 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx)
unsigned int nSigOps = 0;
for (const auto& txin : tx.vin)
{
- nSigOps += txin.scriptSig.GetSigOpCount(false);
+ nSigOps += txin.scriptSig.GetSigOpCount(true);
}
for (const auto& txout : tx.vout)
{
```

</details>

Recommend reviewing per commit.

ACKs for top commit:
jeanpablojp:
tACK 5ce3a0b4aab5ad9ec710e803f88d79139b3b3c44
instagibbs:
ACK https://github.com/bitcoin/bitcoin/pull/36130/commits/5ce3a0b4aab5ad9ec710e803f88d79139b3b3c44
brunoerg:
ACK 5ce3a0b4aab5ad9ec710e803f88d79139b3b3c44
sedited:
ACK 5ce3a0b4aab5ad9ec710e803f88d79139b3b3c44

Tree-SHA512: 1f5c941638fc2907759e5a8d6faf0669b7b7d03d833b51ad675bed10585dd6b232999aa2a5ecb9e0b58db81b1ec44c9916c680e872608e4fe5ee50e71f6b82b4

Merge bitcoin/bitcoin#36163: test: Add coverage for unsatisfiable locktime combination in PSBT `ComputeTimeLock()`

69a640e05ec072c8bdce8d52dfd982df274e8262 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 69a640e05ec072c8bdce8d52dfd982df274e8262

Tree-SHA512: e7a7556df3bd278686a2d53a11b228f6f8c0e8dda79f050bad83dea89824de2e3766518fa3450b3722be86927fb4e6b9061dcd2cdb45010080b37a4fa2baffe0

Merge bitcoin/bitcoin#36166: validation: refactor: encapsulate Chainstate::m_target_blockhash

852f201e09cb98e98ad7418ae11933203cd57896 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](https://github.com/bitcoin/bitcoin/pull/36166/commits/852f201e09cb98e98ad7418ae11933203cd57896)
purpleKarrot:
ACK 852f201e09cb98e98ad7418ae11933203cd57896
l0rinc:
code review ACK 852f201e09cb98e98ad7418ae11933203cd57896
alexanderwiederin:
ACK 852f201e09cb98e98ad7418ae11933203cd57896
sedited:
ACK 852f201e09cb98e98ad7418ae11933203cd57896

Tree-SHA512: 6243ee9979a2493b4f495a0156a119814854d7d91c48bb18777afae928ee2c3b0280ecba3d7516ffef25d92eb15d0a3e369005d43e68afed1142161d6bf4eeda

Merge bitcoin/bitcoin#35738: coins: parallel input prevout fetching followups

8e4b7ab7258aa8497ef90847e495e81d984b99d4 fuzz: use per-level fetch scopes in coinscache_sim (Andrew Toth)
5292386b785a0a133368b5429a2363cf53e29e00 doc: improve CoinsViewOverlay documentation (Andrew Toth)
d552c52b081c4c0853086bcdce38d67fd02f2c67 coins: log error reason when prevout fetch submission fails (Andrew Toth)
2ffaa6e6a7db239306309a859b7e8aed478a810d coins: delete Sync and SetBackend on CoinsViewOverlay (Andrew Toth)
330022993fb96b3b776e562f1de6696d381e6524 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 8e4b7ab7258aa8497ef90847e495e81d984b99d4
l0rinc:
ACK 8e4b7ab7258aa8497ef90847e495e81d984b99d4
sedited:
ACK 8e4b7ab7258aa8497ef90847e495e81d984b99d4

Tree-SHA512: 38001f96be6f893e2610bb81f379ecc0c40ffd39da5bfe1f5db47db1ef2f725d80ae3f9b5e25acd64e65013176ba3ba4e3e8585cb55420b2793845c292beda23

Merge bitcoin/bitcoin#36137: validation: use unused SetTargetBlockHash

455080105835b07b3eaba7a309ec51abe566b379 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 455080105835b07b3eaba7a309ec51abe566b379
ryanofsky:
Code review ACK 455080105835b07b3eaba7a309ec51abe566b379

Tree-SHA512: 93ccac48855d384f0443b5a25c79c5e6d720b6b77ad7a2bb52989382666e4e5f32c76dd7473428d6bbb503307ada7213021591ad54e463d9f8034fe2da97d10c

Merge bitcoin/bitcoin#36100: ci: use LLVM 23 in *san, fuzz, *cross jobs

5ba9af6b6922c48567c0db5c7c03febb643b29d1 ci: pass LIBCXX_INCLUDE_TESTS=OFF to LLVM build (fanquake)
feb3bd46e4c8ca53d1d9e59cd032588a544b1393 clang-tidy: remove some performance-* options (fanquake)
b4bd12d3d5d63a0ef2fe47bfb66fd497326298b6 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 5ba9af6b6922c48567c0db5c7c03febb643b29d1, I have reviewed the code and it looks OK.
willcl-ark:
ACK 5ba9af6b6922c48567c0db5c7c03febb643b29d1

Tree-SHA512: 4d203bf1ec6100a21d9a185a37365d358859bbde79f44f93d2e4f5e3c9686f57ca06d6c73da7423eb234dad5b9501d7a09029a430d23b2bcf8aba95f2d88e66d

Merge bitcoin/bitcoin#36118: test: tolerate race condition in interface_http.py

a51df9b0ecf6ecab1a9eb7728a0b475be6eec3fd 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 a51df9b0ecf6ecab1a9eb7728a0b475be6eec3fd
winterrdog:
tACK a51df9b0ecf6ecab1a9eb7728a0b475be6eec3fd
janb84:
re ACK a51df9b0ecf6ecab1a9eb7728a0b475be6eec3fd
hodlinator:
re-ACK a51df9b0ecf6ecab1a9eb7728a0b475be6eec3fd
sedited:
ACK a51df9b0ecf6ecab1a9eb7728a0b475be6eec3fd

Tree-SHA512: a6244581b2b51af647452e0dc8cd09cdc8d975dee6a0dc8b8064cad136023dad68b4af987303bced91a662bf5fae22871ea718a6a8e68024158a9aef6c5855ef

Merge bitcoin/bitcoin#36144: rpc: detail x-bitcoin-unit in openrpc help

e85e27976bfb6399edc15dee3a676eeba48fc701 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 e85e27976bfb6399edc15dee3a676eeba48fc701

Tree-SHA512: 7fd0bef8a5d37cd9d2778463b2193c58ec7cced1aa790a0a5807ef093bd51e729b6c8880e1c12af78293eb68abe791f4ae6e96e0457ce85be717e3e776f5406d

Merge bitcoin/bitcoin#36145: qa: Use IP_PORTRANGE_HIGH on OpenBSD for dynamic port allocation

59ebf558f3458bbc9038c7bf2958f2f224485ee1 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 59ebf558f3458bbc9038c7bf2958f2f224485ee1
theStack:
utACK 59ebf558f3458bbc9038c7bf2958f2f224485ee1

Tree-SHA512: 680235cf3e1799361796c0ff36d5f19bf74f79393057dbd7b38b0e92a7df3af669873c66ca1e82f1c78999c20351663b8960990f3f209af89ee18ce0773eb7de

Merge bitcoin/bitcoin#35958: net: align v2 message type validation with v1 range

cc577de954671378268bab2a78714869b4f7388d 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 cc577de954671378268bab2a78714869b4f7388d
ajtowns:
utACK cc577de954671378268bab2a78714869b4f7388d
w0xlt:
ACK cc577de954671378268bab2a78714869b4f7388d
sedited:
ACK cc577de954671378268bab2a78714869b4f7388d

Tree-SHA512: 8c97ee20df2311949bbe9655c7e04507c4b47d3b18766aa6ae51691d0870f8a5c25ea54d74c9afb797754572d057b4240533da6bf3c2e0435f3cb32c5fb1c3af

Merge bitcoin/bitcoin#36148: test: Avoid unsafe memory race in index_reorg_crash shutdown

fab80e82c1087126477e07eda5f6e3a1f25ceb99 test: Avoid unsafe memory race in baseindex_no_commit_ahead_of_flush (MarcoFalke)
fa0f14ef5e76424ed7770936f7d053f27336a601 test: Avoid unsafe memory race in index_reorg_crash shutdown (MarcoFalke)
faf9c8e8a12cff5ef4f277d8c3f1035776e57c14 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 fab80e82c1087126477e07eda5f6e3a1f25ceb99

Tree-SHA512: 4423e420421aa37d8b59e053f44c455fafb676102866bdf23988cf72f3d3f265b996bd953583ea8208f1534defb0e16b13ef08644be97e61959dc777a2918e5a

indexes: set prune lock to genesis before first block

When setting both a new index and prune size and restarting
an unpruned node, the node will prune the block store first
and then the index will fail to start syncing.

Fix this by setting the prune lock to 0 if the index does not
yet have a best block.

test: Avoid unsafe memory race in baseindex_no_commit_ahead_of_flush

Without the drain, a BlockConnected event may execute during shutdown
and lead to memory races.

Merge bitcoin/bitcoin#36048: util: keep wallet names literal in notification commands

db39de5601094dc3f0b15ce4759e1b88025403c2 doc: add `-walletnotify` security note (Lőrinc)
1f9dfabef64121c5ea030f5dbaaaf2a6af706eef refactor: use string views in `ReplaceAll` (Lőrinc)
469b0e59a29ac48364c20268feeb4477b8ec9192 util: make `ReplaceAll` literal (Lőrinc)
604d7e8fdd95d22203120ca15d98d89f8668c240 test: characterize walletnotify shell injection (Lőrinc)
4efaa6763a7eb3614b78fa09e676fe0630c34678 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 db39de5601094dc3f0b15ce4759e1b88025403c2 💈
jeanpablojp:
re-ACK db39de5601094dc3f0b15ce4759e1b88025403c2
stickies-v:
re-ACK db39de5601094dc3f0b15ce4759e1b88025403c2

Tree-SHA512: 0be4adecfee50cb4dab90ae3386079767694a6b1fa1d7bd1f10ef73de88707b232f1ba4975a723c465a4d34d12296d501986c657d93bd8ae0bdced16afad1b5e

Merge bitcoin/bitcoin#35808: fuzz: reset connman state in p2p targets

d29b22d07897058eb69c862ecea8b00325229de2 fuzz: reset connman state in p2p targets (Hao Xu)

Pull request description:

Resets `ConnmanTestMsg` at the start of each input in `cmpctblock` and `p2p_handshake`, matching the other reused-connman fuzz targets and preventing sticky `CConnman` state from leaking between corpus inputs.

Before this, deterministic-fuzz-coverage showed single inputs were stable, but all-input directory runs were not:

```diff
cmpctblock, src/net.cpp:4172
- Branch (4172:9): [True: 1.21k, False: 33.0k]
+ Branch (4172:9): [True: 613, False: 33.6k]
- Branch (4172:72): [True: 901, False: 311]
+ Branch (4172:72): [True: 497, False: 116]
```

```diff
p2p_handshake, src/net.cpp:4172
- Branch (4172:9): [True: 98, False: 1.67k]
+ Branch (4172:9): [True: 743, False: 1.03k]
- Branch (4172:72): [True: 90, False: 8]
+ Branch (4172:72): [True: 612, False: 131]
```

With the resets, `deterministic-fuzz-coverage` passed for both `cmpctblock` and `p2p_handshake`.

ACKs for top commit:
nervana21:
re-tACK d29b22d07897058eb69c862ecea8b00325229de2
maflcko:
lgtm ACK d29b22d07897058eb69c862ecea8b00325229de2

Tree-SHA512: bd445ae33ab7f9850046e3de4e318bee9ae7b38ee77ee282d0f5c3a88a4b67610dd00faef2af913a4d7d42bfc42b957855b2d85f1849f77823149baa089afce2

Merge bitcoin/bitcoin#36112: ci: Exclude subtrees from iwyu

fa3971011d1eef383c5b4cc03cc6b278900c0920 ci: Exclude subtrees from iwyu (MarcoFalke)
fa8566152a057a585ea2f642b795756d87f2f38a refactor: Bump old copyright header in univalue (MarcoFalke)

Pull request description:

The iwyu CI may modify subtrees when iwyu thinks a header inside a subtree is "associated" (due to the naming).

This happens to not be a problem on current master, but can become a problem if an iwyu-enforced file is renamed or a file is iwyu-enforced in the future.

Fix this by excluding subtrees.

Can be tested by running the iwyu CI on `src/test/fuzz/minisketch.cpp` and seeing a change in `minisketch.h` before this CI fix.

ACKs for top commit:
hebasto:
re-ACK fa3971011d1eef383c5b4cc03cc6b278900c0920.

Tree-SHA512: 9a555ab020f0f1a2bc4d70ea72011f8d42ba4bfe4a463947d31b0d208b4671b76b466f92a18b6295bc7a8c5bb67c6f697844f673fc02e18983b062d25bc0dc8c

Merge bitcoin/bitcoin#36065: test: refactor: Remove confusing ignore_errors=True

fa7be0a8df9d99d4dd880afb4f48a9b197dca5b0 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 fa7be0a8df9d99d4dd880afb4f48a9b197dca5b0

Tree-SHA512: c533a8aebd92f3f1054563f20af438165632c98f7a2f189f3306420780468b143c24001f794a79ddfc0527c9605a4cfe59949648a9a7f41bbe138128b09f0a6e