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

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#36131: rpc: Improve two field's OpenRPC types

78e691ea10a17092dd65cf0b22c814e9fb3438f0 rpc: Change listunspent's ancestorfees type to NUM (sedited)
73fb9ced563cdf59bfee9462025e32e73f08ecd1 rpc: Fix private key type in signrawtransactionwithkey (sedited)

Pull request description:

This corrects the types for two fields in the OpenRPC dump. Both changes have no effect on the rpc help output. The changes to the schema's format are:

```diff
diff dump.json dump_new.json
11452d11451
< "x-bitcoin-unit": "amount",
13844,13845c13843
< "type": "string",
< "pattern": "^[0-9a-fA-F]+$"
---
> "type": "string"
```

I asked Claude to flag any inconsistencies in the dump and these were the two, out of many others, that I thought were worthwhile to fix.

ACKs for top commit:
maflcko:
lgtm ACK 78e691ea10a17092dd65cf0b22c814e9fb3438f0
stickies-v:
ACK 78e691ea10a17092dd65cf0b22c814e9fb3438f0
musaHaruna:
Tested ACK [78e691e](https://github.com/bitcoin/bitcoin/pull/36131/changes/78e691ea10a17092dd65cf0b22c814e9fb3438f0)

Tree-SHA512: 121d80520a39738c1c7375a50bb552203fe2db403cb3414195e6a79142677ac3c3509ba5f18d4b1982a8e2872c73e47cf6e54b6acd66b1a71ddcbe335ea33f34

Merge bitcoin/bitcoin#35477: test: exercise Schnorr signature cache in txvalidationcache_tests.cpp

3ba1bbfa3f85fae32c0b6614db9bb1a1b2db8ce7 test: exercise Schnorr signature cache in txvalidationcache_tests.cpp (Sebastian Falbesoner)
198b36bc85d54792af82ea9b9e58554608573cb6 test: respect "TAPROOT requires WITNESS" rule in `ValidateCheckInputsForAllFlags` (Sebastian Falbesoner)
e78a2a0d00cf67637eae4caf4db1735f110aff14 test: refactor: simplify tx vin/vout creation in txvalidationcache_tests.cpp (Sebastian Falbesoner)

Pull request description:

The Schnorr verification path of the signature cache is currently never hit in the unit tests, i.e. with the following patch they still pass:
```diff
diff --git a/src/script/sigcache.cpp b/src/script/sigcache.cpp
index c6fcc8f8eb..87688c1049 100644
--- a/src/script/sigcache.cpp
+++ b/src/script/sigcache.cpp
@@ -44,6 +44,7 @@ void SignatureCache::ComputeEntryECDSA(uint256& entry, const uint256& hash, cons

void SignatureCache::ComputeEntrySchnorr(uint256& entry, const uint256& hash, std::span<const unsigned char> sig, const XOnlyPubKey& pubkey) const
{
+ assert(false);
CSHA256 hasher = m_salted_hasher_schnorr;
hasher.Write(hash.begin(), 32).Write(pubkey.data(), pubkey.size()).Write(sig.data(), sig.size()).Finalize(entry.begin());
}
```
This PR adds missing coverage for that by adding a Taproot key-path spend to `checkinputs_test` in `txvalidationcache_tests.cpp`. Same as for the already-existing ECDSA spends, the caching is tested across a large number of flag combinations (using `ValidateCheckInputsForAllFlags`), both with an invalid Schnorr signature (-> should only fail if `SCRIPT_VERIFY_TAPROOT` is set) and a valid one (-> should pass for all flag combinations).

ACKs for top commit:
Bortlesboat:
tACK 3ba1bbfa3f
sedited:
ACK 3ba1bbfa3f85fae32c0b6614db9bb1a1b2db8ce7
instagibbs:
ACK 3ba1bbfa3f85fae32c0b6614db9bb1a1b2db8ce7

Tree-SHA512: e43f7077d9e9ab6f8b5e9e70f0187767d65f686ce24350ce5d61cc4cdf07d5eebdf5e4327ce665c212b7cc02be1e8632a6a9fbcf6be2916f8e058993e5fb2650

Merge bitcoin/bitcoin#36054: test: add script_tests cases covering interpreter mutants

4a12773f269742d2c655beb1b3f5ffe98e9beadb test: cover DERSIG rejects a non-compound signature type (ViniciusCestarii)
86c7fb910d674c757c4829cd006b3732d3a5e78b test: cover OP_16 does not count towards the opcode limit (ViniciusCestarii)
331bf798819ca4f2815450ba50d5920e2d2c2aa6 test: cover OP_WITHIN must pop all 3 elements (ViniciusCestarii)
3bb87bc61b489911cf36e50f74ac8b6ee201b82b test: cover OP_FROMALTSTACK must pop the altstack (ViniciusCestarii)

Pull request description:

Kills some live mutants on interpreter.cpp that affect consensus found by https://bitcoincore.space. They are:

<details>
<summary><a href="https://bitcoincore.space/src/script/interpreter.cpp#3951">interpreter.cpp#3951</a>: <code>OP_FROMALTSTACK</code>: removed <code>popstack(altstack)</code></summary>

```diff
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 98b16eca6b..68265d20b5 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -698,7 +698,7 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
if (altstack.size() < 1)
return set_error(serror, SCRIPT_ERR_INVALID_ALTSTACK_OPERATION);
stack.push_back(altstacktop(-1));
- popstack(altstack);
+
}
break;
```

</details>

<details>
<summary><a href="https://bitcoincore.space/src/script/interpreter.cpp#4084">interpreter.cpp#4084</a>: <code>OP_WITHIN</code>: removed one <code>popstack(stack)</code></summary>

```diff
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 98b16eca6b..874cf5e1cf 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -1018,7 +1018,7 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
CScriptNum bn2(stacktop(-2), fRequireMinimal);
CScriptNum bn3(stacktop(-1), fRequireMinimal);
bool fValue = (bn2 <= bn1 && bn1 < bn3);
- popstack(stack);
+
popstack(stack);
popstack(stack);
stack.push_back(fValue ? vchTrue : vchFalse);
```

</details>

<details>
<summary><a href="https://bitcoincore.space/src/script/interpreter.cpp#3883">interpreter.cpp#3883</a>: opcode limit: <code>opcode > OP_16</code> → <code>opcode >= OP_16</code></summary>

```diff
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 98b16eca6b..e985643606 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -459,7 +459,7 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&

if (sigversion == SigVersion::BASE || sigversion == SigVersion::WITNESS_V0) {
// Note how OP_RESERVED does not count towards the opcode limit.
- if (opcode > OP_16 && ++nOpCount > MAX_OPS_PER_SCRIPT) {
+ if (opcode >= OP_16 && ++nOpCount > MAX_OPS_PER_SCRIPT) {
return set_error(serror, SCRIPT_ERR_OP_COUNT);
}
}
```

</details>

<details>
<summary><a href="https://bitcoincore.space/src/script/interpreter.cpp#3808">interpreter.cpp#3808</a>: <code>IsValidSignatureEncoding</code>: compound type check returns <code>true</code></summary>

```diff
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 98b16eca6b..b613a6ac19 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -133,7 +133,7 @@ bool static IsValidSignatureEncoding(const std::vector<unsigned char> &sig) {
if (sig.size() > 73) return false;

// A signature is of type 0x30 (compound).
- if (sig[0] != 0x30) return false;
+ if (sig[0] != 0x30) return true;

// Make sure the length covers the entire signature.
if (sig[1] != sig.size() - 3) return false;
```

</details>

Recommend reviewing per commit.

ACKs for top commit:
instagibbs:
ACK 4a12773f269742d2c655beb1b3f5ffe98e9beadb
brunoerg:
ACK 4a12773f269742d2c655beb1b3f5ffe98e9beadb
jeanpablojp:
tACK 4a12773f269742d2c655beb1b3f5ffe98e9beadb

Tree-SHA512: 5f53c733d11cb5d645f420d90ab626f894ef0bb155d01b9de0cae502109b2eaa46c072797d08df115da7a8738f01f31212a207a4d0e6f782128beb37332cf46e

Merge bitcoin/bitcoin#35868: rpc, wallet: fix invalid JSON in HelpExampleRpc curl examples

21d4e0ba759bb1024c5cc14c76b4f2963f252007 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 21d4e0ba759bb1024c5cc14c76b4f2963f252007 🚝
sedited:
ACK 21d4e0ba759bb1024c5cc14c76b4f2963f252007

Tree-SHA512: 2a8abc07d681b9dc81b8079a68421278da890049cea33a1561a48d53cbf919a30df588f559e9df94fa4a1ab7027f742f3b12c163afc25246a620340cb3522336

Merge bitcoin/bitcoin#36111: rpc: bound memory for overlong Bech32 errors

7fcaccd9d0b7b9ec80b8224239aeb3c13b90593d bech32: bound overlength error locations (Lőrinc)

Pull request description:

**Problem:** `validateaddress` reports likely error positions for invalid Bech32 inputs, including multiple useful positions for character and checksum errors.
For an overlength input, `LocateErrors()` returns every position after the 90-character limit, which the RPC converts to a `UniValue` number before serializing the response.
A near-limit authenticated request therefore creates about 33 million `int` values and 33 million `UniValue` objects.

**Fix:** Return position 90 for an overlength input, which identifies where the single length violation begins.
Character and checksum errors continue to return multiple useful positions when they can be determined.
The tests now include an oversized example and pin the bounded result.

**Reproducer:** Peak memory usage for a near-limit authenticated request:

<details>
<summary>Linux reproducer</summary>

```bash
sed -i "/def test_validateaddress(self):/a\\
self.nodes[0].validateaddress('bcrt1' + 'q' * (2**25 - 100))\\
__import__('time').sleep(30)" test/functional/rpc_invalid_address_message.py
cmake -B build && cmake --build build -j2
build/test/functional/rpc_invalid_address_message.py >/dev/null 2>&1 &
sleep 20 && awk '/VmHWM/' /proc/$(pgrep bitcoind)/status
```
</details>

```text
Before ████████████████████████ 5.69 GiB
After █░░░░░░░░░░░░░░░░░░░░░░░ 240 MiB
```

ACKs for top commit:
maflcko:
lgtm ACK 7fcaccd9d0b7b9ec80b8224239aeb3c13b90593d
sedited:
ACK 7fcaccd9d0b7b9ec80b8224239aeb3c13b90593d
janb84:
ACK 7fcaccd9d0b7b9ec80b8224239aeb3c13b90593d

Tree-SHA512: 3d439774d394f081b8107f8131963f7aa23ed048b0d6d349a80f9b3481fefeef7b5ce239fbd33606ad1f4960e6bfd899f968c50febc70d38b2fe731c6049583f

Merge bitcoin/bitcoin#36107: iwyu: Fix warnings in `src/init` and treat them as errors

1ad86412783225776c53fb702982f5882f99fcca iwyu: Fix warnings in `src/init` and treat them as errors (Hennadii Stepanov)

Pull request description:

This PR continues the ongoing effort to enforce IWYU warnings.

See [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#using-iwyu).

ACKs for top commit:
maflcko:
lgtm ACK 1ad86412783225776c53fb702982f5882f99fcca

Tree-SHA512: d63d2f5aeac487f01012b8802aff32eb53a8b5d53b8a6c8ece2a40b8c9603402f4f9a20c0918b3007ffaee1ed38a1009698e7b80be29c6a5175517e3279db952

Merge bitcoin/bitcoin#35900: iwyu: Fix warnings in `src/interfaces` and treat them as errors

b3ff9c4d683fdcd0530b7c76c408b4a6e9e0830e iwyu: Fix warnings in `src/interfaces` and treat them as errors (Hennadii Stepanov)
d564b0255f7ec984b1c788910a113a2533dd4d6a iwyu: Add temporary mapping to work around upstream bug (Hennadii Stepanov)

Pull request description:

This PR continues the ongoing effort to enforce IWYU warnings.

See [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#using-iwyu).

ACKs for top commit:
maflcko:
review ACK b3ff9c4d683fdcd0530b7c76c408b4a6e9e0830e 🖋

Tree-SHA512: 9359854c7b6e6ec93022947e78ef57fc68728326df874371028997a550fd1b6ef08e4b09271163b9d5c921c9e01deadf92b0eb5ae3f620d797e4f9bdea8f4727

Merge bitcoin/bitcoin#36092: fix: UB sanitizer in mempool estimator logging

576a0ebb536f93469a1ced6d497e11adf7c6a74d fix: UB sanitizer in mempool estimator logging (rustaceanrob)

Pull request description:

The following is failing in CI, when a block has `m_height` of 64 bit max:
```
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /home/runner/work/_temp/src/policy/fees/mempool_estimator.cpp:210:62
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x1,0x0,0x0,0x0,0x3,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x1,0x0,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x0,0x0,0x26,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x2f,0x0,0x3,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x7a,0x3f,0x3f,0x0,0x0,0x2f,0x0,0x3,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x7a,0x3f,0x3f,0x3f,0xff,0xff,0xff,0xff,0xff,0x18,0x0,0x0,0x85,0x3f,0xff,0xff,0xff,0xff,0xff,0x18,0x0,0x0,0x85,0xd6,0x1,0x0,0x86,0x0,0x0,0x0,0x2a,0x0,0xff,0xff,0xff,
\001\000\000\000\003\377\377\377\377\377\377\377\377\001\000\377\377\377\377\377\377\377\377\000\000&\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000/\000\003\002\002\002\002\002\002\002\000\000\000\000\000\000\000z??\000\000/\000\003\002\002\002\002\002\002\002\000\000\000\000\000\000\000z???\377\377\377\377\377\030\000\000\205?\377\377\377\377\377\030\000\000\205\326\001\000\206\000\000\000*\000\377\377\377
artifact_prefix='./'; Test unit written to ./crash-b4333d1fe3993fe8610b86654e385682c23050b9
Base64: AQAAAAP//////////wEA//////////8AACYAAAAAAAAAAAAAAAAAAAAAAAAvAAMCAgICAgICAAAAAAAAAHo/PwAALwADAgICAgICAgAAAAAAAAB6Pz8///////8YAACFP///////GAAAhdYBAIYAAAAqAP///w==

⚠️ Failure generated from target with exit code 1: ['/home/runner/work/_temp/build_ ₿🧪_/bin/fuzz', '-runs=1', PosixPath('/home/runner/work/_temp/ci/scratch_ ₿🧪_/qa-assets/fuzz_corpora/policy_estimator_io')]
Check if using libFuzzer ... True
Command '['docker', 'exec', '--env', 'DANGER_RUN_CI_ON_HOST=1', '8100bf684275e706787e07f8ab94431926ba5184562c52bce95c932210f6f38f', '/home/runner/work/_temp/ci/test/03_test_script.sh']' returned non-zero exit status 1.

```

ACKs for top commit:
maflcko:
lgtm ACK 576a0ebb536f93469a1ced6d497e11adf7c6a74d
marcofleon:
ACK 576a0ebb536f93469a1ced6d497e11adf7c6a74d
jeanpablojp:
tACK 576a0ebb536f93469a1ced6d497e11adf7c6a74d

Tree-SHA512: a7533e68a95b2f0200abdcf08ae72a7f3654db03cffe642ed39b0a5aa48d932a8b07ee4e4477b591a488b5358dd19fe85a03015642b663fd41a771b3090bc8a3

fix: UB sanitizer in mempool estimator logging

The following is failing in CI, when a block has `m_height` of 64 bit
max:
```
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /home/runner/work/_temp/src/policy/fees/mempool_estimator.cpp:210:62
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x1,0x0,0x0,0x0,0x3,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x1,0x0,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x0,0x0,0x26,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x2f,0x0,0x3,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x7a,0x3f,0x3f,0x0,0x0,0x2f,0x0,0x3,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x7a,0x3f,0x3f,0x3f,0xff,0xff,0xff,0xff,0xff,0x18,0x0,0x0,0x85,0x3f,0xff,0xff,0xff,0xff,0xff,0x18,0x0,0x0,0x85,0xd6,0x1,0x0,0x86,0x0,0x0,0x0,0x2a,0x0,0xff,0xff,0xff,
\001\000\000\000\003\377\377\377\377\377\377\377\377\001\000\377\377\377\377\377\377\377\377\000\000&\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000/\000\003\002\002\002\002\002\002\002\000\000\000\000\000\000\000z??\000\000/\000\003\002\002\002\002\002\002\002\000\000\000\000\000\000\000z???\377\377\377\377\377\030\000\000\205?\377\377\377\377\377\030\000\000\205\326\001\000\206\000\000\000*\000\377\377\377
artifact_prefix='./'; Test unit written to ./crash-b4333d1fe3993fe8610b86654e385682c23050b9
Base64: AQAAAAP//////////wEA//////////8AACYAAAAAAAAAAAAAAAAAAAAAAAAvAAMCAgICAgICAAAAAAAAAHo/PwAALwADAgICAgICAgAAAAAAAAB6Pz8///////8YAACFP///////GAAAhdYBAIYAAAAqAP///w==

⚠️ Failure generated from target with exit code 1: ['/home/runner/work/_temp/build_ ₿🧪_/bin/fuzz', '-runs=1', PosixPath('/home/runner/work/_temp/ci/scratch_ ₿🧪_/qa-assets/fuzz_corpora/policy_estimator_io')]
Check if using libFuzzer ... True
Command '['docker', 'exec', '--env', 'DANGER_RUN_CI_ON_HOST=1', '8100bf684275e706787e07f8ab94431926ba5184562c52bce95c932210f6f38f', '/home/runner/work/_temp/ci/test/03_test_script.sh']' returned non-zero exit status 1.

```

Merge bitcoin/bitcoin#36077: bugfix: give TxDownloadManager its own RNG

80eaa6cabf28b7d68c61fc5aced189b36970f17e bugfix: give TxDownloadManager its own RNG (Greg Sanders)

Pull request description:

TxDownloadManagerImpl retains a reference to PeerManagerImpl::m_rng,
which is non-thread-safe and guarded by g_msgproc_mutex.

BlockConnected runs on the validation background thread while holding
only m_tx_download_mutex. Reconsidering an orphan with multiple
announcers could therefore use m_rng concurrently with message
processing.

Regression introduced in #35986

Added a regression test on second commit, can remove it from the PR if deemed superfluous.

This is a Project Loupe find.

ACKs for top commit:
maflcko:
review ACK 80eaa6cabf28b7d68c61fc5aced189b36970f17e 🐓
hodlinator:
ACK 80eaa6cabf28b7d68c61fc5aced189b36970f17e
sedited:
ACK 80eaa6cabf28b7d68c61fc5aced189b36970f17e

Tree-SHA512: 2dbc4a9298bfa1375dc364ead4b1ec74c2ebe54fb7c311180fa06fc32240406be2979a2dd6ae0e7a23b099ddcd63f5c76c126d84a1405fc7eacd337eb009dd88

Merge bitcoin/bitcoin#36032: rpc: avoid quadratic output lookups

747cff842481153357199bf9a81b5a4d82ea91fb rpc: avoid quadratic output lookups (Lőrinc)

Pull request description:

**Problem:** Transaction-creation RPCs currently take quadratic time to parse outputs.
An authenticated RPC client can therefore tie up a worker with a large request.
`sendmany` also holds the wallet lock while parsing, delaying other operations on the same wallet.

**Fix:** Parse transaction outputs in linear time by reading corresponding keys and values by index instead of looking up each value by key.

**Reproducer:** Run `time build/bin/test_bitcoin --run_test=rpc_tests/parse_outputs` before and after the fix:
<details>
<summary>parse_outputs test in `rpc_tests.cpp`</summary>

```cpp
BOOST_AUTO_TEST_CASE(parse_outputs)
{
constexpr size_t OUTPUT_COUNT{10'000};
UniValue outputs{UniValue::VOBJ};
for (size_t i{0}; i < OUTPUT_COUNT; ++i) {
auto destination{EncodeDestination(WitnessV0ScriptHash{CScript{} << i})};
outputs.pushKVEnd(destination, ValueFromAmount(i + 1));
}

const auto parsed_outputs{ParseOutputs(outputs)};
BOOST_REQUIRE_EQUAL(parsed_outputs.size(), OUTPUT_COUNT);
for (size_t i{OUTPUT_COUNT}; i > 0; --i) {
std::pair expected{CTxDestination{WitnessV0ScriptHash{CScript{} << (i - 1)}}, static_cast<CAmount>(i)};
BOOST_CHECK(parsed_outputs[i - 1] == expected);
}
}
```
</details>
E.g. on my M4 Max with `debug` build:

```python
Before ████████████████████ 1.80 s
After █████▒░░░░░░░░░░░░░░ 0.50 s -72%
```
Related to #35889

ACKs for top commit:
achow101:
ACK 747cff842481153357199bf9a81b5a4d82ea91fb
jonatack:
ACK 747cff842481153357199bf9a81b5a4d82ea91fb
jeanpablojp:
tACK 747cff842481153357199bf9a81b5a4d82ea91fb
hodlinator:
ACK 747cff842481153357199bf9a81b5a4d82ea91fb

Tree-SHA512: 154c9f583f6e7f4154882aeb1ae11c40b327d0ef04147e12a0fee749494ae95314cdfc56baad78723f3383f225d752654be0ae9ed7458b1c73f7a6a80922e3ef

Merge bitcoin/bitcoin#36025: psbt: avoid duplicate taproot leaf script keys when merging

1cb416397b7aa4addd714f9ab68ae0a861ff81c3 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 1cb416397b7aa4addd714f9ab68ae0a861ff81c3
winterrdog:
re-ACK 1cb416397b7aa4addd714f9ab68ae0a861ff81c3

Tree-SHA512: 2599beffe701ba9b672e8dcc3d43844f3853ceeb3d86fc53798a428d3288aa8edd2e42f338b32a1046fa558d6cc5cfa5d55b3a0aaf960278b0f3002158381623

Merge bitcoin/bitcoin#35516: rpc: preserve global xpubs and proprietary fields in joinpsbts

436921eb469a1c4dde9f46e2fbad231da0e17e7a test: check joinpsbts preserves global xpubs and proprietary fields (Thomas)
011094b282b22484090213a20a4c04b32c0d90fc 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 436921eb469a1c4dde9f46e2fbad231da0e17e7a
achow101:
ACK 436921eb469a1c4dde9f46e2fbad231da0e17e7a
winterrdog:
tACK 436921eb469a1c4dde9f46e2fbad231da0e17e7a

Tree-SHA512: d9de34c25aecc29b6b4fb80d6584fa919cc5ff9b7ef2f4d8ce35c4043fe7638fefb8af10448f2cd14021f5d25e149f0efc8798c5b8c9bc8b5582c6152010e891

Merge bitcoin/bitcoin#34993: wallet: `NotifyCanGetAddressesChanged` when advancing `next_index`

e2ab8ae55142370f31d8606531a065e098be6c77 wallet: spkm: Only notify CanGetAddressesChanged on change (David Gumberg)
0892f16f911d0e2ac7ebf40946b74c2cef485e2c refactor: moveonly: Pair CanGetAddressesChanged notifications with desc range. (David Gumberg)
e6adae3db242a2146504bc72cf18ae58bb73401e wallet: `NotifyCanGetAddressesChanged` when advancing `next_index` (David Gumberg)

Pull request description:

Even though `TopUp()` notifies, advancing `next_index` after can deplete available addresses, so make sure to notify any time it's changed.

This would manifest as users seeing a clickable `Receive` button in the GUI when in fact no address can be generated in some edge cases, e.g. when a user has a watch only wallet with a hardened derivation path and runs out of keys.

This feels like it's begging for:

1) a refactor to make it impossible to modify `next_index` or `range_end` without firing `CanGetAddressesChanged`
2) a test

I banged my head against the keyboard for a bit but I couldn't get either of these to fall out, I also tried massaging a few clankers into doing it but I couldn't get any results that seemed reasonable to me, still seems like a worthwhile fix so opening PR anyway.

I also included a moveonly commit to pair code that can change the result of `CanGetAddresses()` with the notification firing

ACKs for top commit:
achow101:
ACK e2ab8ae55142370f31d8606531a065e098be6c77
polespinasa:
ACK e2ab8ae55142370f31d8606531a065e098be6c77
furszy:
utACK e2ab8ae55142370f31d8606531a065e098be6c77

Tree-SHA512: 5bb00d1ef4909a3e55535d283e5995df75e4288a151647f4a368b2086e2f2f4140693f43cae4f72727eafb49c9050aea8604cd6ddddc8646f7ac47b1357ed287

Merge bitcoin/bitcoin#36059: test: make index crash test check saved state

7ea36e985a900b2291ce549e468f6baab5324dc6 test: preserve index crash test state (Lőrinc)
5aa15df60c49aacd3b3dafe13a4ceded9cec07cc test: expose missing index crash checkpoint (Lőrinc)

Pull request description:

**Problem:** #35847 moved the unclean-shutdown test into the shared base index tests, but it checked only that each index could reopen and start background sync.
Both checks also pass when the index reopens at height 0, so they do not verify that a height-100 checkpoint was saved before the simulated crash and reloaded afterward.

**Fix:** The first commit records the existing false positive by asserting that each index reopens at height 0 before background sync.
The second commit establishes a durable checkpoint at height 100, drains its setup notification, and changes the same assertion to the pre-crash height.

ACKs for top commit:
jeanpablojp:
tACK 7ea36e985a900b2291ce549e468f6baab5324dc6
mzumsande:
ACK 7ea36e985a900b2291ce549e468f6baab5324dc6

Tree-SHA512: 0dca2bdd978c5df4acbb01692bb2058e74efa70da3d7191687628075a680a57848ddda8087629b5d743d9a1648d7dc849fda9ff487252136a0c91cdfda33ba32