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
a34fc8b11a wallet: handle disabled startup settings (Robert Hamilton)
b7113e6f42 test: characterize disabled wallet settings (Robert Hamilton)
Pull request description:
I hit a crash while creating a new wallet in Bitcoin-Qt 31.1 on an Apple silicon Mac with `nosettings=1`. After looking through the crash report and code, I traced it to saving the wallet's load-on-startup setting: the settings writer throws when dynamic settings are disabled.
Wallet RPCs report errors with `-nosettings` after changing wallet state. In Qt, the same settings write causes an uncaught exception.
Return a persistence failure when dynamic settings are disabled so wallet operations finish with their existing startup-setting warning. This avoids an uncaught exception in Qt and RPC errors after the wallet state has already changed. Keep in-memory and no-op updates unchanged.
The first commit adds functional coverage for the current behavior. The second adds the fix, updates the assertions to expect success with warnings, and documents that failed settings writes keep the in-memory changes.
### Manual Reproduction
Run on the parent commit and the fixed commit, using a fresh temporary regtest data directory each time:
```sh
{ cmake -B build-wallet-review -DBUILD_GUI=ON && cmake --build build-wallet-review -j --target bitcoin-qt; } >/dev/null 2>&1
build-wallet-review/bin/bitcoin-qt -regtest -datadir="$(mktemp -d)" -nosettings -noconnect
```
Choose `File` > `Create Wallet...`, enter `repro`, leave the defaults unchanged, and click `Create`.
Before the fix, the application terminates with:
```text
libc++abi: terminating due to uncaught exception of type std::logic_error: Attempt to write settings file when dynamic settings are disabled.
```
After the fix, the wallet is created and the application displays:
```text
Wallet load on startup setting could not be updated, so wallet may not be loaded next node startup.
```
ACKs for top commit:
l0rinc:
tested ACK a34fc8b11a
kevkevinpal:
tACK a34fc8b11a
achow101:
ACK a34fc8b11a
jeanpablojp:
tACK a34fc8b11a
Tree-SHA512: 5e43028200478f89e71ebe7e0fc28c559f15e713226124899a69eb90d413d8ecaaaca02267d5a848068d70555b3e4334993f414de2debf6a22d73a51a71d1acd
1fca81960a psbt: fix rendering for invalid long sighash type field (Sjors Provoost)
Pull request description:
The `decodepsbt` incorrectly truncates the (32 bit) sighash type field before looking up its human friendly name. It's not dangerous, as such a signature would be invalid, but potentially confusing.
Fix that and add a test.
I plan to use `SighashToStr` in another pull request to render an error message for invalid sighash type field values, but it seemed worth fixing in a standalone PR.
ACKs for top commit:
jeanpablojp:
ACK 1fca81960a
achow101:
ACK 1fca81960a
winterrdog:
tACK 1fca81960a
rkrux:
lgtm ACK 1fca81960a
Tree-SHA512: 74f9206e53f7b72f251f9e0feab68abd8e1a1d99c976314675aba8231fb7c36ee484f21f443b8fee4a7518c9c5125f03c7121b6d9aa94ebb293271b0cc90cc0f
e014e5bb61 miner: Enforce murch-zawy rule (BIP54) (Fabian Jahr)
Pull request description:
Opened separate from #35793 as [requested by darosior](https://github.com/bitcoin/bitcoin/pull/35793#discussion_r3704817804). This makes the miner enforce the murch-zawy rule for which #35793 adds the validation part.
A node whose clock is behind the first block of the difficulty period currently reports a mintime below the consensus floor in getblocktemplate and fails to build a valid template for the last block of the period so it can't mine until its clock catches up. This is mostly a theoretical concern on mainnet because it would require a huge system clock misconfiguration. It might be a bigger concern on test networks with volatile hashrates. But generally, I think our miner should be able to create valid templates in any situation.
ACKs for top commit:
kevkevinpal:
crACK [e014e5b](e014e5bb61)
darosior:
ACK e014e5bb61
sedited:
ACK e014e5bb61
Tree-SHA512: 299f83459e92654e028ce1c27470e1dcaae4a58de10687659860cdb425d3af330d3b8da6d8ca8f727c9eaebb2f0a3f3af5185f9daa57e92133435cb80a9e365c
d180b891a2 test: add mixed P2SH/witness sigop accounting (Lőrinc)
6e60c362bc test: add P2SH sigop counting coverage (Musa Haruna)
Pull request description:
Add test coverage for sigop counting in P2SH spends in `test_witness_sigops()`, addressing the existing TODO.
The new cases mirror the existing P2WSH sigop tests by constructing transactions that:
- remain below the block sigop limit (accepted),
- exceed the limit (rejected with bad-blk-sigops)
Since P2SH sigops are accounted as legacy sigops, the expected sigop cost accounts for the 4× legacy weighting applied during consensus validation.
The added coverage verifies the enforcement of the block sigop limit for both witness and P2SH spends, including mixed P2SH/witness transactions.
**Acknowledgement:** During review ([comment](https://github.com/bitcoin/bitcoin/pull/35164#pullrequestreview-4769420630)), **l0rinc** demonstrated, using mutation testing on his branch [here](https://github.com/l0rinc/bitcoin/pull/248), that the original test suite would not detect two consensus sigop undercounting bugs. Those experiments helped validate the coverage added by this PR and motivated the inclusion of the mixed P2SH/witness regression test.
ACKs for top commit:
l0rinc:
reACK d180b891a2
Bicaru20:
ACK d180b891a2
sedited:
ACK d180b891a2
Tree-SHA512: 795923f56316c3cad4d02a572ed6486a0f3f62bd524fb162d2bcd884108485974e96f38c08b5ff0a8659feecfc208c8d7e93dde2b9e0517c70f6343c2063d9b5
bcb09b3f4a qa: Verify HTTP listen port exclusivity (Hodlinator)
af65069fd1 windows: Use SO_EXCLUSIVEADDRUSE over SO_REUSEADDR (Hodlinator)
Pull request description:
#### Problem
`HTTPServer::BindAndStartListening()` unconditionally enables `SO_REUSEADDR` before binding the RPC listener. On Windows, a reuse-enabled listener does not reserve the port exclusively: another local process can request `SO_REUSEADDR` and bind to the same port (see https://learn.microsoft.com/en-us/windows/win32/winsock/using-so-reuseaddr-and-so-exclusiveaddruse).
If the competing socket receives a new connection, it can capture the HTTP Basic `Authorization` header (including the cookie credential) and proxy or issue privileged RPC calls as the victim. This crosses a local-user boundary and can expose wallet-controlling RPC credentials.
#### Fix
Have Windows use `SO_EXCLUSIVEADDRUSE` instead which makes the port exclusive to the process which first requests it, while retaining the restart-friendly behavior which `SO_REUSEADDR` enabled. Abort if another process is already bound to the port.
#### Further context & rationale
This issue is new in our homegrown HTTP server implementation, since libevent had a guard against setting `SO_REUSEADDR` on Windows, see `evutil_make_listen_socket_reuseable()` d82464a277/evutil.c (L483). libevent does not reference `SO_EXCLUSIVEADDRUSE`.
Why should we not just avoid `SO_REUSEADDR` on Windows and skip `SO_EXCLUSIVEADDRUSE` like the libevent approach?
Because setting either option makes the process less prone to failing to bind to a port after having been restarted. Not sure why this wasn't an issue before, maybe the node startup was usually slow enough to time out the port before we tried to re-bind it on Windows.
---
Discovered by Project Loupe.
ACKs for top commit:
pinheadmz:
ACK bcb09b3f4a
sedited:
utACK bcb09b3f4a
jeanpablojp:
tACK bcb09b3f4a
Tree-SHA512: 7f2362cc8399e8c4e95b27b39066d3e591b5aebfc2b562aba10786609456f526f562394818a3d1042d64dabf497548ffabf0757322cfb201454a134321108cf5
Return a persistence failure when dynamic settings are disabled so
wallet operations finish with their existing startup-setting warning.
This avoids an uncaught exception in Qt and RPC errors after the wallet
state has already changed. Keep in-memory and no-op updates unchanged.
Wallet RPCs report errors with -nosettings after changing wallet state.
Check these results alongside wallet usability, unchanged settings.json,
and restored startup preferences when settings are enabled again.
Reuse an existing wallet for loading and check explicit unloading last,
so the sequence does not depend on the skipped unload completion wait.
3d1004cb9b http: throttle per-connection reads while a request is in flight (Matthew Zipkin)
Pull request description:
This patches a memory exhaustion scenario found while auditing the new http server with kimi-k3. A shallow version of this scenario was addressed in #35735 (See https://github.com/bitcoin/bitcoin/pull/35735#discussion_r3720177656 and https://github.com/bitcoin/bitcoin/pull/35735#issuecomment-5217000202) but a OOM vector still remained.
On master when the sever is busy handling a request from a client, it will still read data from that client and "queue up" the next request. In #35735 we handled the scenario where that additional incoming data was an invalid HTTP request by not attempting to parse the data. However, we didn't add a size limit.
A misbehaving client could block its request queue with something like `waitforblock` and then flood the server with nonsense data without any limit.
The solution in this patch is to not even read from the socket at all if we are busy with a request. Similar to the intent of #35735, the kernel will buffer incoming data until backpressure kicks in and the TCP window drops to 0.
If unaddressed, the attack vector is still limited to authenticated clients: unauthenticated REST requests don't block for very long, so the server *should* be able to drain the receive buffer.
ACKs for top commit:
jeanpablojp:
tACK 3d1004cb9b
frankomosh:
ACK 3d1004cb9b
hodlinator:
ACK 3d1004cb9b
winterrdog:
tACK 3d1004cb9b
sedited:
ACK 3d1004cb9b
Tree-SHA512: 56f7678a9ab6789aa542c1f252df0b6ccf9137cb426ff915a0a3fe8285200fdb62b7a47c476ed8617c3592e7a7eac18158cd8c0dac309cdcf4e5fd887e016209
a51df9b0ec test: tolerate race condition in interface_http.py (Matthew Zipkin)
Pull request description:
Fixes#35632 by allowing both outcomes of a race condition. The server behavior is unchanged: in response to a malformed request we send an error code and disconnect. The issue is that sometimes on Windows the RST is caught by the platform and the receive buffer is discarded before the Python client can process it with recv().
We can also be much more polite to misbehaving clients by implementing a lingering close using SO_LINGER as suggested in #35780 but that will require more review.
The exact error in #35632 is hard to produce reliably but there are a few close options for reviewers. I tested this on windows native building with MSVC. In both of these cases the patch from this PR caught the error and passed the test.
**RemoteDisconnected: Remote end closed connection without response**
```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 9bb89863af..62324d3fea 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -1072,7 +1072,7 @@ std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequest(const std::shared_
e.what());
// We failed to read a complete request from the buffer
- WriteNoStoreErrorReply(*client->m_req, HTTP_BAD_REQUEST);
+ // WriteNoStoreErrorReply(*client->m_req, HTTP_BAD_REQUEST);
client->m_disconnect = true;
return nullptr;
}
```
**ConnectionResetError: [WinError 10054] An existing connection was forcibly closed by the remote host**
```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 9bb89863af..be52acb874 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -1154,6 +1154,11 @@ bool HTTPRemoteClient::MaybeDisconnect(std::chrono::time_point<SteadyClock> now,
"Disconnecting HTTP client %s (id=%llu)",
m_origin,
m_id);
+ auto sock{GetSock()};
+ linger opt{};
+ opt.l_onoff = 1; // enable SO_LINGER
+ opt.l_linger = 0; // zero timeout
+ sock->SetSockOpt(SOL_SOCKET, SO_LINGER, &opt, sizeof(opt));
return true;
}
```
ACKs for top commit:
jeanpablojp:
re-ACK a51df9b0ec
winterrdog:
tACK a51df9b0ec
janb84:
re ACK a51df9b0ec
hodlinator:
re-ACK a51df9b0ec
sedited:
ACK a51df9b0ec
Tree-SHA512: a6244581b2b51af647452e0dc8cd09cdc8d975dee6a0dc8b8064cad136023dad68b4af987303bced91a662bf5fae22871ea718a6a8e68024158a9aef6c5855ef
59ebf558f3 qa: Use IP_PORTRANGE_HIGH on OpenBSD for dynamic port allocation (Hennadii Stepanov)
Pull request description:
The default ephemeral port range on OpenBSD (1024-49151) overlaps with the test framework's static port range starting at `TEST_RUNNER_PORT_MIN`, the same way FreeBSD's does (see #34346).
Extend `set_ephemeral_port_range()` to OpenBSD. The socket option and its values are identical to FreeBSD's, so only the platform check changes.
ACKs for top commit:
maflcko:
lgtm ACK 59ebf558f3
theStack:
utACK 59ebf558f3
Tree-SHA512: 680235cf3e1799361796c0ff36d5f19bf74f79393057dbd7b38b0e92a7df3af669873c66ca1e82f1c78999c20351663b8960990f3f209af89ee18ce0773eb7de
Fixes#35632 by allowing both outcomes of a race condition.
The server behavior is unchanged: in response to a malformed request
we send an error code and disconnect. The issue is that sometimes
on Windows the RST is caught by the platform and the receive buffer
is discarded before the Python client can process it with recv().
We can also be much more polite to misbehaving clients by
implementing SO_LINGER as suggested in #35780 but that will require
more review.
The default ephemeral port range on OpenBSD (1024-49151) overlaps with
the test framework's static port range starting at TEST_RUNNER_PORT_MIN,
the same way FreeBSD's does (see #34346).
Extend `set_ephemeral_port_range()` to OpenBSD. The socket option and
its values are identical to FreeBSD's, so only the platform check
changes.
A client streaming pipelined requests into a busy connection
(or any connection whose replies are slower than the sender) could grow
server memory without limit, up to remote OOM.
Stop selecting RecvEvent for clients whose request is being processed;
pipelined data then backs up in the kernel socket buffer, applying TCP
backpressure to the sender. One request per connection is in flight
at a time.
Functional test streams pipelined submitblock requests into a connection
blocked on waitforblockheight. Unpatched builds continue draining the
socket buffer indefinitely, patched builds will stall.
`ReplaceAll()` substitutes fixed tokens in notification commands and other strings.
PR #25803 replaced the Boost helper with `std::regex_replace()`, treating searches as regular expressions and substitutes as replacement-format syntax.
Restore literal, non-recursive replacement so callers match fixed tokens and preserve replacement bytes exactly, while avoiding a new string when the search text is absent.
Co-authored-by: Rob Hamilton <6456095+Rob1Ham@users.noreply.github.com>
`-walletnotify` shell-escapes wallet names before substituting `%w` into the configured command.
`ReplaceAll()` uses `%w` as the regex pattern and the escaped wallet name as replacement text, where `$'` copies the command suffix into the escaped name and allows its shell metacharacters to alter the command.
Record the command execution, missing notification file, regex pattern matching, replacement expansion, and non-recursive replacement.
fa7be0a8df test: refactor: Remove confusing ignore_errors=True (MarcoFalke)
Pull request description:
There is an unexplained `ignore_errors=True` in the internal `_initialize_chain` helper:
```py
shutil.rmtree(cache_path('fees'), ignore_errors=True)
```
This is fine, because no error should happen. But it is a bit confusing, because an ignored error may lead to a later error anyway.
Fix that by failing early instead.
Also, re-write the simple block to `pathlib`.
ACKs for top commit:
willcl-ark:
ACK fa7be0a8df
Tree-SHA512: c533a8aebd92f3f1054563f20af438165632c98f7a2f189f3306420780468b143c24001f794a79ddfc0527c9605a4cfe59949648a9a7f41bbe138128b09f0a6e
ff3e2e4ebd net: Trigger process abort when behind start block MTP (Hodlinator)
1883cecb4d test: Characterize lagging-clock headers presync (Hodlinator)
Pull request description:
### Problem
Headers presync computes `m_max_commitments` from the elapsed time since the chain-start MTP plus `MAX_FUTURE_BLOCK_TIME`. When the local system clock is more than `MAX_FUTURE_BLOCK_TIME` behind the chain-start MTP, that elapsed value is negative, but it is used in arithmetic assigned to the unsigned commitment cap. This can turn the intended zero bound into a large cap, letting low-work headers presync continue instead of aborting when a reasonable commitment cap would have been exceeded.
### Fix
Instead of allowing an invalid `HeadersSyncState` object to be created, emit an error and **abort the node process**.
Typically, the node will detect that the system clock is set too far in the past when comparing it to the chain tip during chain state loading and shut down before we start syncing headers. So in practice this is very unlikely to make a difference (might be possible if the system clock jumps backwards after we loaded the chain state).
#### Commits
* Add functional and unit characterization tests [pinning the current behavior](https://github.com/bitcoin/bitcoin/pull/35260).
* The fix, along with corresponding test changes.
---
Replaces #35208 which was clamping `m_max_commitments` to zero and then letting the `HeadersSyncState` consume headers until the block height either reached the the next `commitment_period` point and aborted, or reached the minimum work threshold and succeeded (possible when having been offline for >144 blocks).
ACKs for top commit:
l0rinc:
diff and code review ACK ff3e2e4ebd
sedited:
ACK ff3e2e4ebd
mzumsande:
Code Review ACK [ff3e2e4](ff3e2e4ebd)
Tree-SHA512: bdd82fd0609309aa4bea026db1b607ae856c53403ec01b2511fa2ccae9db4ff1bb9e39523b446583c09ae53823275b8a603050d9090b61fabb84fab35e458f28
This ensures that UTXO unserialization errors abort the node, and does
not cause a consensus divergence.
A valid UTXO is created and shared between two nodes. The raw database
entry is then deliberately modified on one node so it can no longer be
deserialized. When the other node spends that UTXO and mines a block,
the node with the unserializable entry must abort during block connection
rather than silently treating the coin as absent and marking the block
BLOCK_FAILED_VALID, which would cause it to permanently diverge from the
network's best chain.
21d4e0ba75 rpc, wallet, test: fix invalid JSON in HelpExampleRpc curl examples (GuTS805)
Pull request description:
Several `HelpExampleRpc` call sites reused CLI-style argument strings
verbatim instead of valid JSON — missing commas, bare unquoted words, or
single backslashes that are not valid JSON escapes. As a result the
documented `curl` command for 14 RPCs (`getblockfrompeer`, `addnode`,
`addconnection`, `sendmsgtopeer`, `restorewallet`, `getmempoolcluster`,
`importmempool`, `getindexinfo`, `listlabels`, `unloadwallet`,
`createwalletdescriptor`, `addhdkey`, `loadwallet`, `listunspent`) fails
to parse as JSON if copy-pasted as-is. Also fixes a stray trailing quote
in the `restorewallet` named-argument examples.
This was previously raised in #31275, which sipa confirmed at runtime by
adding a `UniValue::read` check, but that PR was closed unmerged. Since
then two more examples broke the same way (`getmempoolcluster`,
`addhdkey`), which is why this adds a permanent regression check to
`rpc_help.py::dump_help()` instead of just fixing the current list.
Fixes#35864.
ACKs for top commit:
maflcko:
review ACK 21d4e0ba75🚝
sedited:
ACK 21d4e0ba75
Tree-SHA512: 2a8abc07d681b9dc81b8079a68421278da890049cea33a1561a48d53cbf919a30df588f559e9df94fa4a1ab7027f742f3b12c163afc25246a620340cb3522336
The PSBT sighash type field is a 32 bit unsigned integer in BIP 174,
signed in PSBTInput, and it is not validated when deserialized.
decodepsbt incorrectly truncates this field before looking up its
name. Fix that and add a test.
`LocateErrors()` returns multiple useful positions for character and checksum errors, but an overlength string has one structural error.
Every character from the limit onward is outside the permitted address, so listing each position adds no diagnostic value.
`validateaddress` converts every returned position into a `UniValue` number before serializing the response.
An authenticated request below the HTTP body limit can therefore require several gigabytes of memory.
Return only the first position beyond the length limit, which identifies where the violation begins.
Character and checksum errors continue to report multiple useful positions when they can be determined, and the existing unit and functional tests cover both behaviors.
Several HelpExampleRpc call sites reused CLI-style argument strings
verbatim (missing commas, bare unquoted words, or single backslashes
that are not valid JSON escapes), producing curl examples that fail
JSON parsing as documented. Also fixes a stray trailing quote in the
restorewallet named-argument examples, a missing comma in the
listunspent example, and a wrong-schema string-instead-of-array
listunspent argument caught in review.
Lines touched are converted to raw string literals (or strprintf with a
raw string template) throughout, for consistency and to avoid manual
quote/backslash escaping.
Adds a regression check to rpc_help.py::dump_help() so this class of
bug can't silently reappear.
558e26e66e test: cover OP_SUCCESSx bypassing the initial stack element size limit (ViniciusCestarii)
Pull request description:
BIP-342 specifies that the initial stack resource checks happen after OP_SUCCESSx processing, and explicitly notes the checks "can be bypassed using OP_SUCCESSx". Core implements this correctly, but there are currently no tests covering this behavior. This means a consensus-breaking change to the ordering could pass the test suite undetected. Verified this on local commit 68d24d7430, which mutates to incorrectly implement the order and CI still turns green.
Add a new test at feature_taproot.py to cover OP_SUCCESSx bypassing the initial stack element size limit.
Verified that the new test catches the mutant: f8f42a13a8.
ACKs for top commit:
instagibbs:
ACK 558e26e66e
Tree-SHA512: 66d7bbbf286bf7e5c5762704e8c0f835c6a8026d7d604263e9debcd7e71df80506c0238b8a15cda3d4ba245d9c2bc46e79a1986d66583e571e208c8dfbe66156
On some systems, such as NetBSD, the non-default
`-rpcmaxconnections=128` is too high, so bitcoind refuses to start:
```
Error: Not enough file descriptors available. 256 available, 290 required.
```
The test only needs a value above the default of 16. Use 64 and lower
`-maxconnections` in that case so the total fits in 256.
Since bitcoin/bitcoin#35730 the HTTP server reserves file descriptors
for its listen sockets and for `-rpcmaxconnections` connected clients
(16 by default), so `min_required_fds` in init.cpp grew.
On select()-based platforms `available_fds` is capped at FD_SETSIZE,
which is 256 on NetBSD. The previous value of 94 no longer fits and
every node in the test suite started up with a warning, which the
framework treats as unexpected stderr and fails on.
Recompute the value with the new accounting (256 - 179 = 77) and
update the comment to match the current variable names in init.cpp.
e4d80e7001 test: close the loop after the network thread has completed (Vasil Dimov)
29fba5ddbb test: close the listeners before terminating the event loop (Vasil Dimov)
Pull request description:
Whenever a test creates a new `P2PInterface` object a new listener is
created inside `NetworkThread.create_listen_server()` by calling
`cls.network_event_loop.create_server()`.
These listeners are never closed which might result in:
```
2026-06-10T22:13:35.3934880Z Task was destroyed but it is pending!
2026-06-10T22:13:35.3936020Z task: <Task pending name='Task-54' coro=<BaseSelectorEventLoop._accept_connection2() done, defined at /opt/homebrew/Cellar/python@3.14/3.14.5/Frameworks/Python.framework/Versions/3.14/lib/python3.14/asyncio/selector_events.py:217> wait_for=<Future finished result=None>>
```
when the event loop is closed.
Fix that by closing the listeners.
Fixes: https://github.com/bitcoin/bitcoin/issues/35508
ACKs for top commit:
andrewtoth:
ACK e4d80e7001
sedited:
ACK e4d80e7001
Tree-SHA512: b93d06526b4eb31ac445a1a0e379e5ec947661f8ea29f2e07ac88b9e4760b0cc5348638b320ab8d60735f34fc163fcdd18eba42b20e0a7726a98a5136433bd64
1cb416397b psbt: avoid duplicate taproot leaf script keys when merging (Shuvam Pandey)
Pull request description:
Follow-up to #35665, which fixed the same combiner defect for `PSBT_GLOBAL_XPUB`. thomasbuilds
and winterrdog asked for this one as its own PR when I reported it there.
`m_tap_scripts` maps a leaf script to a set of control blocks, but is serialized as one record
per control block, keyed by the control block (`SerializeToVector(s, PSBT_IN_TAP_LEAF_SCRIPT,
std::span{control_block})`). `PSBTInput::Merge` unions it by the map key, so two PSBTs that map
the same control block to different leaf scripts merge into an input that serializes the `0x15`
key twice. Duplicate keys make a PSBT invalid, so it is the same `combinepsbt` then
`decodepsbt` failure as the xpub case, at the input level. Present since #22558 (v24.0).
Both decode on their own, and differ only in the leaf script the control block maps to, `OP_1`
against `OP_1 OP_1`:
```
$ A=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAIhXAUJKbdMGgSVS3i0tgNel6XgeKWg8o7JbVR7/ums6AOsACUcAAAA==
$ B=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAIhXAUJKbdMGgSVS3i0tgNel6XgeKWg8o7JbVR7/ums6AOsADUVHAAAA=
$ bitcoin-cli -regtest decodepsbt "$(bitcoin-cli -regtest combinepsbt "[\"$A\",\"$B\"]")"
error code: -22
error message:
TX decode failed Duplicate Key, input key "15c050929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0" already provided: unspecified iostream_category error
```
winterrdog reproduced it on the #35665 thread with another pair.
Merge the records rather than the map entries, keeping the leaf script already there. BIP 174
lets the combiner "pick arbitrarily when conflicts occur", and unknown and proprietary records
already resolve that way. Refusing to combine is the BIP's other option, but that would fail
`combinepsbt` on input it accepts today.
Merging by map key drops records as well. `std::map::insert` leaves existing keys alone, so
when both PSBTs carry the same leaf script with different control blocks, the incoming set was
dropped. Those keys do not conflict, so merging per record keeps them.
The control blocks already present are collected once per merge rather than searched for per
incoming record, which would be quadratic in the size of the two PSBTs `combinepsbt` takes from
the caller.
Since this is the second field with this shape I checked the rest. `m_xpubs` (#35665) and
`m_tap_scripts` are the only two whose record key comes from the value, so two map entries can
serialize the same key. `partial_sigs` is keyed by `CKeyID` and serialized under the pubkey,
but the pubkey determines the `CKeyID`, so those records stay distinct. The others key the
record by the map key, `m_proprietary` included, and `PSBTOutput` has no such field.
The test fails on master on both counts, and covers the merges that do not conflict as well.
I found this with a local assertion in the psbt fuzz target that a combined PSBT must
roundtrip. That assertion can go in a follow-up.
Tested:
```
./build/bin/test_bitcoin --run_test=psbt_tests
./build/bin/test_bitcoin --run_test=psbt_wallet_tests
./build/test/functional/test_runner.py rpc_psbt.py rpc_rawtransaction.py wallet_taproot.py wallet_signer.py feature_taproot.py wallet_basic.py
```
ACKs for top commit:
achow101:
ACK 1cb416397b
winterrdog:
re-ACK 1cb416397b
Tree-SHA512: 2599beffe701ba9b672e8dcc3d43844f3853ceeb3d86fc53798a428d3288aa8edd2e42f338b32a1046fa558d6cc5cfa5d55b3a0aaf960278b0f3002158381623
436921eb46 test: check joinpsbts preserves global xpubs and proprietary fields (Thomas)
011094b282 rpc: preserve global xpubs and proprietary fields in joinpsbts (Thomas)
Pull request description:
`joinpsbts` collects the global xpubs of all the joined PSBTs into `merged_psbt`, but returns a separately constructed `shuffled_psbt` into which only the inputs, outputs, and unknown fields are copied. The collected `PSBT_GLOBAL_XPUB` records are silently dropped, and `PSBT_GLOBAL_PROPRIETARY` records are not collected at all.
The xpub collection was added in #17034, which was written against a `joinpsbts` that still returned `merged_psbt`, but was merged after #16512 had introduced the `shuffled_psbt` rebuild, so the collected xpubs have never reached the result.
Shuffle the inputs and outputs of `merged_psbt` in place instead of rebuilding a new PSBT, so that all global data is preserved, and union the global proprietary records in the merge loop, matching the `combinepsbt` behavior from #34893.
ACKs for top commit:
jpk68:
ACK 436921eb46
achow101:
ACK 436921eb46
winterrdog:
tACK 436921eb46
Tree-SHA512: d9de34c25aecc29b6b4fb80d6584fa919cc5ff9b7ef2f4d8ce35c4043fe7638fefb8af10448f2cd14021f5d25e149f0efc8798c5b8c9bc8b5582c6152010e891
b42f7fade0 descriptor: don't prepend key origins twice (Shuvam Pandey)
7b15e2cb44 descriptor: fix duplicate check for hardened keys (Shuvam Pandey)
Pull request description:
Fixes#34273.
Importing a descriptor that uses the same `musig()` participants twice in one
tapleaf, with different musig subderivations, fails with
`is not sane: contains duplicate public keys`. It only fails when one of the
participants is a private key on a hardened path. The all-xpub version of the
same descriptor imports fine. That's what gave it away.
The duplicate check (`KeyCompare`) resolves each key expression to a pubkey and
compares the results. It does this at index 0, and the old code used an empty
signing provider. With that empty provider, a `musig()` expression can't resolve
when one of its participants is on a hardened path, because deriving that
participant needs its private key, so the whole aggregate key comes back empty.
Two different musig expressions both came back empty, so the check treated them
as duplicates. The fix derives against the signing provider populated during
parsing, which holds the private keys, and only compares the expression strings
when neither side resolves. 151henry151 had suggested looking at the empty
signing provider on the issue.
scgbckbone found a second, separate bug in the same descriptors. When another
expression that reuses those participants is handled in the same expansion, its
participant origin in the PSBT is added twice, so `m/86h/1h/0h` becomes
`m/86h/1h/0h/86h/1h/0h` in both the input and output Taproot BIP32 derivation
maps. `OriginPubkeyProvider::GetPubKey()` now derives into a temporary provider,
merges it, and writes the corrected origin once, so a later expression can't
prepend the same origin again.
Tested:
```
./build/bin/test_bitcoin --run_test=descriptor_tests
./build/bin/test_bitcoin --run_test=miniscript_tests
./build/bin/test_bitcoin --run_test=bip328_tests
./build/bin/test_bitcoin --run_test=psbt_wallet_tests
./build/test/functional/test_runner.py wallet_musig.py --jobs=1
```
ACKs for top commit:
achow101:
ACK b42f7fade0
scgbckbone:
ACK b42f7fade0
Tree-SHA512: ab36caa6bc484fa1fc3289c79e9a3d713278d82f80de478e53e1bdbe645037e07776ac798eba085733abc139c11a9dbf0d3f49d3c0eee9632e4ddf33d2242f92
m_tap_scripts maps a leaf script to a set of control blocks, but is serialized
one record per control block, keyed by the control block. PSBTInput::Merge
unions it by the map key, so two PSBTs that map the same control block to
different leaf scripts merge into an input serializing the 0x15 key twice.
Duplicate keys are invalid, so combinepsbt hands back a PSBT that can no longer
be decoded. Present since #22558 (v24.0).
Merge the records instead of the map entries, keeping the leaf script already
there, as BIP 174 lets the Combiner pick arbitrarily when conflicts occur. The
control blocks already present are collected once rather than searched for per
incoming record, which would be quadratic in the size of the PSBTs.
Control blocks under a leaf script that both PSBTs carry are now kept as well,
where the map level union dropped them.
The test covers conflicting and non-conflicting merges, including an incoming
leaf script whose control blocks only partly conflict, so records that do not
conflict are not dropped alongside those that do.
bd4b1524ea init: do not count file descriptors for HTTPServer if -server=0 (Matthew Zipkin)
b08662060d init: account for maximum file descriptors needed by HTTP (Matthew Zipkin)
cc2acebefb http: configure simultaneous connection limit with -rpcmaxconnections (Matthew Zipkin)
b3d6d2d1a7 http: limit connected clients to 16 (Matthew Zipkin)
86651d8197 scripted-diff: Rename nUserBind, nBind, nMaxConnections to snake_case (Matthew Zipkin)
Pull request description:
Introduces a new configuration option `-rpcmaxconnections` with default value `16`. This is used to limit the number of simultaneous `HTTPClient` connected to the `HTTPServer`. When the limit is reached, new pending connections remain queued in the kernel's socket buffer. Those connections have complete TCP handshakes with the kernel but do not occupy any application memory.
The previous libevent-based HTTP server had no limit on connections but it did have a limit on the kernel socket queue:
e7ff4ef2b4/http.c (L3510)
```c
if (listen(fd, 128) == -1) {
```
The current HTTP server, like the p2p server, uses a platform constant here:
b6becf3534/src/httpserver.cpp (L743)
(on my macOS `SOMAXCONN` is `128` but on my Debian machine it's `4096`)
The default of 16 was chosen as a reasonable upper bound for single-user RPC use cases. Systems designed to handle more simultaneous HTTP connections than this (previously relying on the absence of a limit) can adjust the setting.
## File descriptors
Because of the connection limit, we can now account for the maximum number of file descriptors needed by the HTTP server. This addresses several issues (#11368#11322 maybe #27732) that could have been fixed by a PR waiting in vain for a libevent release (#27731).
## Bonus performance improvement
The new limit is managed in a loop that drains the kernel's socket queue with `accept()`. All pending connections from the queue (up to the limit) are processed in one single call to `SocketHandlerListening()`. The previous code would only accept one connection from the queue on each I/O loop tick, with a `SELECT_TIMEOUT` (50ms) sleep between each.
ACKs for top commit:
fjahr:
tACK bd4b1524ea
janb84:
ACK bd4b1524ea
winterrdog:
tested ACK bd4b1524ea
hodlinator:
Concept ACK bd4b1524ea
willcl-ark:
ACK bd4b1524ea
Tree-SHA512: 2ef7a96da4d7037c7343ec0ea03fda5bb55d10c2a071fce4929141297515923b203d3d338dbcb6599849768f52aa3c9da509fb5d1d6f7c574a1d2034ea2a9e74
73a94b4545 psbt: avoid aborting on invalid MuSig2 derivations (Lőrinc)
e3d1e75a51 test: characterize MuSig2 derivation aborts (Lőrinc)
Pull request description:
**Problem:** A PSBT may contain MuSig2 derivation metadata with a hardened child index or a path that derives to a different key.
The hardened index aborts during public derivation, while the mismatched key aborts at the result assertion.
`analyzepsbt`, `finalizepsbt`, and `descriptorprocesspsbt` all reach this code without a wallet.
Even the read-only `analyzepsbt` can force a co-signer service to restart its node after unexpected input.
**Fix:** Return failure when a MuSig2 derivation path contains a hardened child index, and skip only the current aggregate when the path derives to a different key so another matching aggregate can still be tried.
This follows [#35154](https://github.com/bitcoin/bitcoin/pull/35154), with the related contributions credited in the commits.
ACKs for top commit:
jeanpablojp:
ACK 73a94b4545
achow101:
ACK 73a94b4545
andrewtoth:
ACK 73a94b4545
Tree-SHA512: d8e28c5a4184154a4427c644ce62423cbcccdc3d82a6293f36fe99055fa04714598bc92c43b526fbcc7c99b231140669c2d0f1b853999d7dc33f949564c90504
7f9c4e2928 doc: add release notes (ismaelsadeeq)
e18d392689 test: add mempool estimator i/o fuzz test (ismaelsadeeq)
970f02096d fees: persist mempool policy estimator data (ismaelsadeeq)
7dcb37989d fees: move fee_estimates.dat into fees directory (ismaelsadeeq)
0db2b69e6d rpc: add verbosity option to estimatesmartfee options (ismaelsadeeq)
06bb65730e fees: gate mempool estimates on recent block coverage (ismaelsadeeq)
cfe585df25 validation: emit block mempool removal signal from ConnectTip (ismaelsadeeq)
0d88558f95 fees: return mempool estimates when it's lower than block policy (ismaelsadeeq)
693b1351af fees: add caching to MemPoolFeeRateEstimator (ismaelsadeeq)
c9bb3df29f fees: add MemPoolFeeRateEstimator class (ismaelsadeeq)
9cacf677a9 rpc: add fee_rate_estimator option to estimatesmartfee (ismaelsadeeq)
ba6c61bbdd fees: add FeeRateEstimatorManager class (ismaelsadeeq)
2cb6b831e0 fees: add EstimateFeeRate and MaximumTarget to CBlockPolicyEstimator (ismaelsadeeq)
5adb2ab084 refactor: test block policy estimator directly (ismaelsadeeq)
9c8309a890 test: rename policy estimator tests to block policy estimator tests (ismaelsadeeq)
e3d5ef1b5f fees: move StringForBlockPolicyEstimateReason to block policy estimator (ismaelsadeeq)
74245c20e0 fees: split wallet and estimator fee reasons (ismaelsadeeq)
Pull request description:
This PR is another attempt to fix#27995 using a better approach.
For background and motivation, see #27995 and the discussion in the Delving Bitcoin post [Mempool Based Fee Estimation on Bitcoin Core](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703).
This PR is currently limited to using the mempool only to lower what is recommended by the Block Policy Estimator.
Accurate and safe fee estimation using the mempool is challenging. There are open questions about how to prevent mempool games that are theoretically possible for miners [(a variant of the Finney attack)](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703/6).
This is one reason this PR uses the mempool only to lower the Block Policy Estimator result. The Block Policy Estimator itself is not gameable in this way, so the combined estimate is not susceptible to this attack increasing the returned feerate.
The underlying assumption is that, with the current tools and work done to make RBF and CPFP feasible and reliable (TRUC transaction relay, ephemeral anchors, cluster size 2 package RBF), underestimation is safer than overestimation. We now assume it is relatively easy to fee-bump later if a transaction does not confirm, whereas once a fee is overestimated there is no way to recover from that.
Another open question when using the mempool for fee estimation is how to account for incoming transaction inflow.
[Bitcoin Augur](https://github.com/block/bitcoin-augur) does this by using past inflow plus a constant expected inflow to predict future inflow. I find this unconvincing for fee estimation and potentially prone to more overestimation, as past conditions are not always representative of the future. See my [review of the Augur fee rate estimator and open questions](https://github.com/block/bitcoin-augur/issues/3).
This PR uses a much simpler approach based on current user behavior, similar to the widely used method employed by mempool.space: looking at the top block of the mempool and selecting a percentile feerate depending on whether the user is economical or conservative.
Empirical data from both myself and Clara Shikhelman shows that the 75th percentile feerate for economical users and the 50th percentile feerate for conservative users provide positive confirmation guarantees, hence this is what is used in this PR.
Parallel research by Rene Pickhardt and his student suggests that using the [average fee per byte of the block template performs well](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703/12).
All of these are constants that can be adjusted. There is parallel work exploring these constants and running benchmarks across fee estimators to find a sweet spot.
See also work in LND, the [LND Budget Sweeper](https://delvingbitcoin.org/t/lnds-deadline-aware-budget-sweeper/1512), which applies this idea successfully. Their approach is to estimate fees initially with bitcoind, then increment gradually as the confirmation deadline approaches, using a fixed fee budget.
Historical data indicates that this PR's approach can [reduce overestimation quite significantly (~29%)](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703/8).
This is particularly useful in scenarios where the Block Policy Estimator recommends a high feerate while the mempool is empty.
<img width="1800" height="1090" alt="56f3ba26c0184521c42bb82ec9d8c9f2224d4f8e" src="https://github.com/user-attachments/assets/c035c40c-8ece-42a7-b290-d29f1ac9bf4d" />
As seen in the image above, there is only one remaining unfixed case: when there is a sudden inflow of transactions and the feerate rises, the Block Policy Estimator takes time to reflect this. In that case, users will continue to see a low feerate estimate until it slowly updates. From the historical data linked above, [this occurs about ~26% of the time](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703/8).
Overall, we observe a **73% success rate with 0% overestimation, and 26% underestimation** with this approach.
See https://bitcoincorefeerate.com/stats for recent running stats that have almost identical data.
This PR also includes refactors that enable this work. Rather than splitting the PR and implementing changes incrementally, I opted for an end-to-end implementation:
### 1. Refactors
* Split the mixed fee reason enum into separate wallet and block policy concepts. The wallet now has a `FeeReason` enum for why the wallet selected a fee rate (`FEE_RATE_ESTIMATOR`, `MEMPOOL_MIN`, `USER_SPECIFIED`, `FALLBACK`, `REQUIRED`), while the Block Policy Estimator uses `BlockPolicyEstimateReason` for its internal threshold details.
* Move `StringForBlockPolicyEstimateReason` to the Block Policy Estimator code, keeping the estimator-specific strings with the estimator.
* Move detailed Block Policy Estimator logging out of wallet transaction creation and into the estimator path. Wallet transaction creation now logs the selected fee and wallet fee reason instead of leaking estimator internals.
* Keep the wallet RPC `fee_reason` field name for compatibility, but update its meaning to report the wallet fee reason instead of the Block Policy Estimator's internal threshold reason.
* Rename policy estimator tests and files to block-policy-specific names where appropriate.
* Update Block Policy Estimator unit tests to be independent of the mempool and validation interface.
### 2. Introduce Mempool-Based Fee Estimator and Fee Estimator Manager
* Introduce `FeeRateEstimation` and `FeeRateEstimationError` as common estimator result types, avoiding new out-parameters for fee estimation results.
* Add `FeeRateEstimatorType` to identify the estimator that produced a result.
* Add `FeeRateEstimatorManager`, responsible for owning the Block Policy Estimator and Mempool Fee Rate Estimator.
* Update the node context to store a `std::unique_ptr` to `FeeRateEstimatorManager` instead of `CBlockPolicyEstimator`.
* Update `CBlockPolicyEstimator` to no longer subscribe directly to the validation interface; instead, `FeeRateEstimatorManager` subscribes and forwards relevant notifications.
* Add a mempool fee estimator that generates a block template when called, calculates a percentile feerate, and returns the 75th percentile for economical mode or the 50th percentile for conservative mode.
* When the selected estimate is below the node's fee floor, `estimatesmartfee` still returns at least the max of `mempoolminfee` and `minrelaytxfee`.
* Add caching to the mempool estimator so new estimates are generated at most every 7 seconds while the chain tip is unchanged, assuming enough [transactions have propagated](https://bitcoin.stackexchange.com/questions/125776/how-long-does-it-take-for-a-transaction-to-propagate-through-the-network/125777#125777) to make a meaningful difference.
This heuristic will likely be replaced by requesting block templates via the general-purpose block template cache proposed here: https://github.com/bitcoin/bitcoin/issues/33389
* Update `MempoolTransactionsRemovedForBlock` to receive the connected block as well as the transactions removed from the mempool.
* Track the weight of block transactions and mempool transactions removed due to block connection after each block connection.
This data is tracked for the last 6 mined blocks. A mempool feerate estimate is returned only when the ratio of mempool transaction weight removed due to block connection to block transaction weight is greater than 75% across the tracked window. This heuristic provides rough confidence that the node's mempool matches that of the majority of the hashrate. The 75% threshold is arbitrary and can be adjusted.
There is a caveat when transactions in the local mempool are consistently not mined by the network, as described in #27995 (e.g. due to filtering).
Accounting for these transactions during fee estimation is not necessary, as they should be evicted from the mempool itself (see #33510). Handling this again within fee estimation would be redundant.
* Persist statistics for the 6 most recent mined blocks to `fees/mempool_policy_estimator.dat` during periodic flushes and shutdown, so this data is available after restarts.
* Move Block Policy Estimator data from `fee_estimates.dat` to `fees/block_policy_estimates.dat`, migrating the legacy file during startup when needed.
* Add `fee_rate_estimator` to the `estimatesmartfee` options object. Supported values are `"none"` (default combined behavior), `"block_policy"` (use only the Block Policy Estimator), and `"mempool_policy"` (use only the Mempool Fee Rate Estimator). Unknown values are treated as `"none"`.
* Add `verbosity` to the `estimatesmartfee` options object. With `verbosity >= 2`, the RPC returns recent mempool health statistics.
* Expose the selected fee rate estimator in `estimatesmartfee` results when `fee_rate_estimator` is `"none"` and the estimate succeeds.
* Add unit, functional, and fuzz test coverage for the new estimator behavior, persistence, RPC options, and estimator I/O.
<details>
<summary>see example output</summary>
```bash
bitcoin-cli estimatesmartfee 1 economical '{"verbosity": 2, "fee_rate_estimator": "none"}'
```
```json
{
"feerate": 0.00002133,
"estimator": "mempool_policy",
"blocks": 2,
"mempool_health_statistics": [
{
"block_height": 927953,
"block_weight": 3991729,
"mempool_txs_weight": 3942409
}
]
}
```
</details>
ACKs for top commit:
willcl-ark:
reACK 7f9c4e2928
jsarenik:
Approach ACK 7f9c4e2
Tree-SHA512: c35b423eea0eb34524cf5ad07822c0ab8d53e2ab78965b58c8738044c61c77352184822360ed077a51bfbf83d0226d221e988f7156b1948023707c7e1fb31495
Persist MemPoolFeeRateEstimator's recent mined-block statistics
to fees/mempool_policy_estimator.dat and reload them at startup.
Without this, the mempool estimator starts cold after each restart
and treats the mempool as unhealthy until MEMPOOL_HEALTH_WINDOW_BLOCKS
blocks have been observed, causing the default combined estimatesmartfee
request to return a mempool fee rate estimator error.
Files with more stats than MEMPOOL_HEALTH_WINDOW_BLOCKS,
non-consecutive block heights, or a final block that does not match the
active chain tip are rejected on read, preserving the invariant that
loaded stats describe the current chain.
Add MempoolPolicyEstimatorPath(), pass the path through
FeeRateEstimatorManager, and flush both block-policy
and mempool-policy estimator files on interval and shutdown.
Move block policy fee estimates from fee_estimates.dat to
fees/block_policy_estimates.dat.
On startup, migrate the legacy file to the new path when only the legacy
file exists. If both files exist, keep the new file and remove the
legacy file.
Rename the block policy estimator args source files to the generic
estimator_args.{cpp,h} names and rename FeeestPath to
BlockPolicyFeeEstPath while the path helper is moved into the shared fee
estimator argument code.
Add a verbosity option to the existing estimatesmartfee options object.
The default verbosity remains 1.
When verbosity is at least 2 include mempool_health_statistics in the response.
The array reports the mined blocks tracked by the mempool fee rate estimator in
most-recent-first order, with each entry containing:
- block_height
- block_weight: total non-coinbase transaction weight in the block
- mempool_txs_weight: weight of transactions removed from our mempool
for that block
Expose these stats through the fee rate estimator manager so RPC users
can inspect the block coverage data used by the mempool health check.
Integrate MemPoolFeeRateEstimator into FeeRateEstimatorManager.
When both estimators succeed, select the lower of the block policy
and mempool estimates.
When either estimator fails, return its error instead of falling back
to the block policy estimate: if the mempool estimator cannot produce
an estimate, the combined estimate fails.
Callers that want a block-policy-only estimate can request it explicitly
via fee_rate_estimator option.
estimatesmartfee now emits the estimator field only for successful
manager-selected estimates.
Add a test that ensures estimatesmartfee returns the mempool fee rate
estimate when it is lower than the block policy estimate, and can request
the mempool policy estimator explicitly
Two wallet functional tests also need adjusting. When the mempool is
too sparse to fill its percentile buckets, MemPoolFeeRateEstimator
returns a relayable floor of max(min relay fee, mempool min fee), so in
regtest getFeeRateEstimate now returns the min relay fee where the
wallet previously had no estimate and fell back to a higher rate:
- wallet_taproot.py: the cleanup sendall used automatic fee estimation.
GetMinimumFeeRate previously fell back to the wallet fallback fee
(fallbackfee, 20 sat/vB in the test framework); it now uses the min
relay fee floor. At that lower feerate the wallet's underestimate of
the taproot script-path witness size drops the effective feerate
below min relay, so the transaction is rejected. Pin fee_rate=20 to
match the framework fallbackfee.
- wallet_bumpfee.py: GetDiscardRate() previously fell back to the
wallet discard rate (-discardfee); it now takes the minimum of that
and the estimate, so the min relay fee floor collapses the discard
rate down to the dust relay feerate. The lower discard rate reduces
the cost of change, so the ~614 sat leftover change in
test_dust_to_fee is now retained instead of being dropped to fee.
Rework the test to leave a sub-dust (20/270 sat) change that is
dropped regardless of the discard rate.
Co-authored-by: willcl-ark <will@256k1.dev>
Add MemPoolFeeRateEstimator, which calls Bitcoin Core's block
assembler with the mempool and chainstate to build a block template and
use its chunk fee rates for fee rate estimation.
Add CalculateMaxWeightPercentiles to return the 50th and 75th
percentile chunk feerates by cumulative block weight. If sparse,
EstimateFeeRate uses the higher of the minimum relay fee rate and the
current mempool minimum fee rate.
The 50th percentile is returned as the conservative estimate, and the
75th percentile as the economical estimate.
Wire MemPoolFeeRateEstimator into FeeRateEstimatorManager and add
FeeRateEstimatorType::MEMPOOL_POLICY for result attribution.
Add unit tests for the mempool fee rate estimator and fee estimator
string conversions, plus fuzz coverage for the string conversions.
Co-authored-by: willcl-ark <will@256k1.dev>
6d387af562 psbt: remove write-only global xpub tracking set (Thomas)
3b7051c7e3 test: check combinepsbt with conflicting global xpub origins (Thomas)
7c632c0e2a psbt: avoid duplicate global xpub keys when merging (Thomas)
Pull request description:
Global xpubs are stored in a map of key origin to set of xpubs, while the serialization writes one record per xpub, keyed by the xpub. `Merge` unions the map origin-by-origin, so when the combined PSBTs provide different key origins for the same xpub, the result serializes the same `PSBT_GLOBAL_XPUB` key twice. BIP 174 declares PSBTs with duplicate keys invalid and the deserializer rejects them, so `combinepsbt` returns a PSBT that no RPC can parse again. This affects all releases since the merge loop was added in #17034 (v23.0).
<details><summary>Reproduction on master</summary>
The PSBTs share the unsigned transaction and xpub, and differ only in the master fingerprint of the global xpub record (`00000000` vs `11111111`):
```
$ A=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAD/////AQAAAAAAAAAAAAAAAABPAQQ1h88AAAAAAAAAAACHPf+BwC9SViP9H+UWfqw6VaBJ3j0xS7Qu4if/7TfVCAM5o2ATMBWX2u9B++WToCzFE9C1VSfsLfEFDi6P9JyFwgQAAAAAAAAA
$ B=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAD/////AQAAAAAAAAAAAAAAAABPAQQ1h88AAAAAAAAAAACHPf+BwC9SViP9H+UWfqw6VaBJ3j0xS7Qu4if/7TfVCAM5o2ATMBWX2u9B++WToCzFE9C1VSfsLfEFDi6P9JyFwgQRERERAAAA
$ bitcoin-cli -regtest decodepsbt "$(bitcoin-cli -regtest combinepsbt "[\"$A\",\"$B\"]")"
error code: -22
error message:
TX decode failed Duplicate Key, global key "01043587cf00...9c85c2" already provided: iostream error
```
</details>
Deduplicate by xpub when merging, keeping the origin that is already present: BIP 174 lets the Combiner "pick arbitrarily when conflicts occur", and conflicting unknown and proprietary records are already resolved the same way. The logic is shared between `combinepsbt` and `joinpsbts` through a new `MergeGlobalXPubs` helper. The second commit adds a test that fails on master with the error above, and the last commit removes the `global_xpubs` tracking set in `Unserialize`, write-only since the generic duplicate key check introduced in #21283 (1e2d146b47) replaced the explicit one.
Note: the xpub loop in `joinpsbts` currently has no observable effect, since the collected xpubs never reach the returned PSBT. My #35516 fixes that, so this PR should land first: on its own, #35516 would make the same duplicate key issue reachable through `joinpsbts`, while with the shared helper in place it never becomes reachable. I will rebase #35516 on top afterwards.
ACKs for top commit:
Bicaru20:
tACK 6d387af562.
achow101:
ACK 6d387af562
winterrdog:
tACK 6d387af562
Tree-SHA512: e2a9e02617eeec22a9240d7cf9386ee880a5f3639b143df7de4d8ea3e7b808f8c123f0b0410ff4a22e9a564bd86b2335a5c4aa3b2281af47d111484a6f1fd108
8454fb2bd7 test: sync funding block before isolating nodes (shaurya2k06)
Pull request description:
Fixes#35967
test_alternate_witness_tx mines the taproot funding output on node0 with
sync_fun=self.no_op and immediately disconnects. node1 later includes the
script-path spend via generateblock. If the funding block has not reached
node1, that call fails with bad-txns-inputs-missingorspent.
Drop the no_op so generate() uses the default sync_all before the partition.
Later generate* calls keep no_op because the nodes are then disconnected.
Seen twice this week in hebasto bitcoin-core-nightly NetBSD jobs:
https://github.com/hebasto/bitcoin-core-nightly/actions/runs/31350308484/job/93339698854https://github.com/hebasto/bitcoin-core-nightly/actions/runs/31765546925/job/94660585799
The modified test is test/functional/wallet_listtransactions.py. I ran it
locally three times with build/test/functional/wallet_listtransactions.py.
ACKs for top commit:
achow101:
ACK 8454fb2bd7
furszy:
utACK 8454fb2bd7
Tree-SHA512: 6b8fdcdc9ce9c57c2939caff34850e88450864c909fe226ba9b6e02ffcefa3625623589b8ecd2f09b9ca16bcaec3a64bd07642ff2c47ae6e8d250de478d34b53
