The `gcov`-based `CoverageFuzz` script was introduced in
8b6f1c4353, as a CMake's replacement for
the legacy `cov_fuzz` target. However, neither `cov_fuzz` nor
`CoverageFuzz` has a documented usage.
Instead, #32206 documented compiling for fuzz coverage using the
LLVM/Clang toolchain, which does not involve the `CoverageFuzz` script.
This change removes the never-documented `CoverageFuzz` script, which is
likely unused.
4550801058 validation: use unused SetTargetBlockHash (fanquake)
Pull request description:
This was pointed out as unused in #36103 by jeanpablojp, but that seems like a mistake from #30214, where it was introduced. See: https://github.com/bitcoin/bitcoin/pull/36137#discussion_r3906377189.
ACKs for top commit:
stickies-v:
ACK 4550801058
ryanofsky:
Code review ACK 4550801058
Tree-SHA512: 93ccac48855d384f0443b5a25c79c5e6d720b6b77ad7a2bb52989382666e4e5f32c76dd7473428d6bbb503307ada7213021591ad54e463d9f8034fe2da97d10c
5ba9af6b69 ci: pass LIBCXX_INCLUDE_TESTS=OFF to LLVM build (fanquake)
feb3bd46e4 clang-tidy: remove some performance-* options (fanquake)
b4bd12d3d5 ci: use LLVM 23 in *san, fuzz, *cross jobs (fanquake)
Pull request description:
LLVM 23.1.0 was recently released, switch to using it across sanitizer, fuzzer and cross-compilation jobs.
ACKs for top commit:
hebasto:
ACK 5ba9af6b69, I have reviewed the code and it looks OK.
willcl-ark:
ACK 5ba9af6b69
Tree-SHA512: 4d203bf1ec6100a21d9a185a37365d358859bbde79f44f93d2e4f5e3c9686f57ca06d6c73da7423eb234dad5b9501d7a09029a430d23b2bcf8aba95f2d88e66d
a51df9b0ec test: tolerate race condition in interface_http.py (Matthew Zipkin)
Pull request description:
Fixes#35632 by allowing both outcomes of a race condition. The server behavior is unchanged: in response to a malformed request we send an error code and disconnect. The issue is that sometimes on Windows the RST is caught by the platform and the receive buffer is discarded before the Python client can process it with recv().
We can also be much more polite to misbehaving clients by implementing a lingering close using SO_LINGER as suggested in #35780 but that will require more review.
The exact error in #35632 is hard to produce reliably but there are a few close options for reviewers. I tested this on windows native building with MSVC. In both of these cases the patch from this PR caught the error and passed the test.
**RemoteDisconnected: Remote end closed connection without response**
```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 9bb89863af..62324d3fea 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -1072,7 +1072,7 @@ std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequest(const std::shared_
e.what());
// We failed to read a complete request from the buffer
- WriteNoStoreErrorReply(*client->m_req, HTTP_BAD_REQUEST);
+ // WriteNoStoreErrorReply(*client->m_req, HTTP_BAD_REQUEST);
client->m_disconnect = true;
return nullptr;
}
```
**ConnectionResetError: [WinError 10054] An existing connection was forcibly closed by the remote host**
```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 9bb89863af..be52acb874 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -1154,6 +1154,11 @@ bool HTTPRemoteClient::MaybeDisconnect(std::chrono::time_point<SteadyClock> now,
"Disconnecting HTTP client %s (id=%llu)",
m_origin,
m_id);
+ auto sock{GetSock()};
+ linger opt{};
+ opt.l_onoff = 1; // enable SO_LINGER
+ opt.l_linger = 0; // zero timeout
+ sock->SetSockOpt(SOL_SOCKET, SO_LINGER, &opt, sizeof(opt));
return true;
}
```
ACKs for top commit:
jeanpablojp:
re-ACK a51df9b0ec
winterrdog:
tACK a51df9b0ec
janb84:
re ACK a51df9b0ec
hodlinator:
re-ACK a51df9b0ec
sedited:
ACK a51df9b0ec
Tree-SHA512: a6244581b2b51af647452e0dc8cd09cdc8d975dee6a0dc8b8064cad136023dad68b4af987303bced91a662bf5fae22871ea718a6a8e68024158a9aef6c5855ef
This was pointed out as unused in #36103, but that seems like a mistake
from #30214, where it was introduced.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
e85e27976b rpc: detail x-bitcoin-unit in openrpc help (will)
Pull request description:
Addresses review comment about clarifying this field: https://github.com/bitcoin/bitcoin/pull/36131#issuecomment-5480592255
ACKs for top commit:
sedited:
ACK e85e27976b
Tree-SHA512: 7fd0bef8a5d37cd9d2778463b2193c58ec7cced1aa790a0a5807ef093bd51e729b6c8880e1c12af78293eb68abe791f4ae6e96e0457ce85be717e3e776f5406d
59ebf558f3 qa: Use IP_PORTRANGE_HIGH on OpenBSD for dynamic port allocation (Hennadii Stepanov)
Pull request description:
The default ephemeral port range on OpenBSD (1024-49151) overlaps with the test framework's static port range starting at `TEST_RUNNER_PORT_MIN`, the same way FreeBSD's does (see #34346).
Extend `set_ephemeral_port_range()` to OpenBSD. The socket option and its values are identical to FreeBSD's, so only the platform check changes.
ACKs for top commit:
maflcko:
lgtm ACK 59ebf558f3
theStack:
utACK 59ebf558f3
Tree-SHA512: 680235cf3e1799361796c0ff36d5f19bf74f79393057dbd7b38b0e92a7df3af669873c66ca1e82f1c78999c20351663b8960990f3f209af89ee18ce0773eb7de
cc577de954 net: align v2 message type validation with v1 range (Bruno Garcia)
Pull request description:
BIP324 specifies the 13-byte long-form message type encoding as "an ASCII message type (as in the v1 P2P protocol)", but V2Transport::GetMessageType() accepted bytes up to 0x7F, while for V1 it only accepts printable ASCII (0x20-0x7E).
This changes V2 to match V1 on it and add test coverage.
ACKs for top commit:
nervana21:
tACK cc577de954
ajtowns:
utACK cc577de954
w0xlt:
ACK cc577de954
sedited:
ACK cc577de954
Tree-SHA512: 8c97ee20df2311949bbe9655c7e04507c4b47d3b18766aa6ae51691d0870f8a5c25ea54d74c9afb797754572d057b4240533da6bf3c2e0435f3cb32c5fb1c3af
fab80e82c1 test: Avoid unsafe memory race in baseindex_no_commit_ahead_of_flush (MarcoFalke)
fa0f14ef5e test: Avoid unsafe memory race in index_reorg_crash shutdown (MarcoFalke)
faf9c8e8a1 test: Clarify index.GetSummary().synced state in index_reorg_crash (MarcoFalke)
Pull request description:
Currently, the `index_reorg_crash` test may rarely crash due to UB in sanitizers like TSan or ASan. This is perfectly fine, because it is just a rare test-only issue.
However, fix it nonetheless by adding a missing drain of the unused in-flight events. Also, add a small check about the synced state while touching this test.
ACKs for top commit:
arejula27:
ACK fab80e82c1
furszy:
ACK fab80e82c1
Tree-SHA512: 4423e420421aa37d8b59e053f44c455fafb676102866bdf23988cf72f3d3f265b996bd953583ea8208f1534defb0e16b13ef08644be97e61959dc777a2918e5a
Fixes#35632 by allowing both outcomes of a race condition.
The server behavior is unchanged: in response to a malformed request
we send an error code and disconnect. The issue is that sometimes
on Windows the RST is caught by the platform and the receive buffer
is discarded before the Python client can process it with recv().
We can also be much more polite to misbehaving clients by
implementing SO_LINGER as suggested in #35780 but that will require
more review.
db39de5601 doc: add `-walletnotify` security note (Lőrinc)
1f9dfabef6 refactor: use string views in `ReplaceAll` (Lőrinc)
469b0e59a2 util: make `ReplaceAll` literal (Lőrinc)
604d7e8fdd test: characterize walletnotify shell injection (Lőrinc)
4efaa6763a test: simplify `ReplaceAll` coverage (Lőrinc)
Pull request description:
**Problem:** On non-Windows builds, operators can configure `-walletnotify` to run a command for wallet transactions, with `%w` replaced by the shell-escaped wallet name.
An authenticated RPC caller allowed to create wallets can supply a name containing `$'`, request an address, and send a transaction to it.
While replacing `%w`, `ReplaceAll()` passes the escaped wallet name to `std::regex_replace()` as replacement text.
There, `$'` copies the command suffix into the escaped name, breaking its quote accounting and allowing shell metacharacters in the wallet name to alter the command.
`runCommand()` passes the result to `system()`, so a suitable command template could execute additional shell commands as the node process account.
It is not reachable over P2P or by an unauthenticated network peer.
#25803 introduced this behavior in v24 when it replaced Boost's literal substitution with `std::regex_replace()`.
**Fix:** Restore the literal, non-recursive contract `ReplaceAll()` had before #25803, matching every current caller's literal search and replacement text, while the wallet notification test covers a wallet name containing `$'`.
**Related:** #35833 restricts control characters in new wallet names, while this change fixes replacement metacharacters in `ReplaceAll()`.
This was found and disclosed responsibly by the Red Team 🟥.
ACKs for top commit:
maflcko:
re-ACK db39de5601💈
jeanpablojp:
re-ACK db39de5601
stickies-v:
re-ACK db39de5601
Tree-SHA512: 0be4adecfee50cb4dab90ae3386079767694a6b1fa1d7bd1f10ef73de88707b232f1ba4975a723c465a4d34d12296d501986c657d93bd8ae0bdced16afad1b5e
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.
PR #25803 changed these parameters to `const std::string&` for `std::regex_replace()`.
The literal implementation no longer needs owned strings, so restore the original `std::string_view` interface.
`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.
RPC responses that build objects from `std::map` or `std::set` keys already have unique keys.
Use `pushKVEnd()` for direct map and set loops in `decodepsbt` and verbose mempool ancestor and descendant results.
Do the same for `getpeerinfo` message counters and selected `getblockstats` results.
`use_shared_memory` and `max_log_mb` are never read or written; they
only ever hold their initializers. Their last readers, in
`GetBerkeleyEnv()` and the `BerkeleyDatabase` constructor, were removed
in 04a7a7a28c ("build, wallet, doc: Remove BDB"), along with the
`-privdb` and `-dblogsize` options that set them.
Co-authored-by: pablomartin4btc <pablomartin4btc@gmail.com>
fa3971011d ci: Exclude subtrees from iwyu (MarcoFalke)
fa8566152a refactor: Bump old copyright header in univalue (MarcoFalke)
Pull request description:
The iwyu CI may modify subtrees when iwyu thinks a header inside a subtree is "associated" (due to the naming).
This happens to not be a problem on current master, but can become a problem if an iwyu-enforced file is renamed or a file is iwyu-enforced in the future.
Fix this by excluding subtrees.
Can be tested by running the iwyu CI on `src/test/fuzz/minisketch.cpp` and seeing a change in `minisketch.h` before this CI fix.
ACKs for top commit:
hebasto:
re-ACK fa3971011d.
Tree-SHA512: 9a555ab020f0f1a2bc4d70ea72011f8d42ba4bfe4a463947d31b0d208b4671b76b466f92a18b6295bc7a8c5bb67c6f697844f673fc02e18983b062d25bc0dc8c
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
55390d1827 doc: Correct comment about which subsystem detects lagging clocks (Hodlinator)
Pull request description:
Turns out a completely fresh datadir means there is no chain state to load and hence no detection of a lagging clock occurs in that subsystem. Instead we do proceed into attempting to start a headers sync.
<details><summary>Diff to repro with fresh -datadir</summary>
```diff
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1499,6 +1499,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
const ArgsManager& args = *Assert(node.args);
const CChainParams& chainparams = Params();
+ SetMockTime(chainparams.GenesisBlock().Time() - 3h);
+
auto opt_max_upload = ParseByteUnits(args.GetArg("-maxuploadtarget", DEFAULT_MAX_UPLOAD_TARGET), ByteUnit::M);
if (!opt_max_upload) {
return InitError(strprintf(_("Unable to parse -maxuploadtarget: '%s'"), args.GetArg("-maxuploadtarget", "")));
```
</details>
Follow-up to #35351
ACKs for top commit:
sedited:
ACK 55390d1827
jonatack:
ACK 55390d1827
Tree-SHA512: 244a0cb634a0ba67fa88fe83f73111e475f6fff258cd1783b9b0a39669eefdd3b2e739a0b761616972bc938216336e18b0c0322e1201c2e805767cb813ca616c
78e691ea10 rpc: Change listunspent's ancestorfees type to NUM (sedited)
73fb9ced56 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 78e691ea10
stickies-v:
ACK 78e691ea10
musaHaruna:
Tested ACK [78e691e](78e691ea10)
Tree-SHA512: 121d80520a39738c1c7375a50bb552203fe2db403cb3414195e6a79142677ac3c3509ba5f18d4b1982a8e2872c73e47cf6e54b6acd66b1a71ddcbe335ea33f34
8d930981e9 refactor: Replace !ContainsNoNUL() with ContainsNUL() (Hodlinator)
Pull request description:
Avoids frequent double negation. See also fa7078d84f when it was renamed from the previous name, "ValidAsCString()".
Found while reviewing #35041.
ACKs for top commit:
maflcko:
lgtm ACK 8d930981e9
l0rinc:
code review ACK 8d930981e9
sedited:
ACK 8d930981e9
janb84:
ACK 8d930981e9
Tree-SHA512: 3ed1d264953f08272c115d760e8149c5985d63331404ac3b1017a277c4b3a61862915851fe45746e26ed46fe7478f851680d205e5ef83e727d061f2867fed99c
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 seems to be the only place where a STR_AMOUNT is used for a sats
denominated fee amount. Many other places use the raw NUM type for a fee
amount, for example getblockstats and getblocktemplate. This doesn't
change the actual result of the RPC call.
The change is motivated by OpenRPC, where the field was previously given
a 'x-bitcoin-unit' tag. This usually describes a decimal amount, and may
be confusing for consumers applying this tag.
It is base58, so shouldn't be qualified with STR_HEX. Similarly,
signmessagewithprivkey also declares the argument as a STR.
This fix is motivated by the OpenRPC dump, where fields tagged with STR_HEX are
described with a restricting regex that would make its correct usage a
violation against the unpatched schema.
3ba1bbfa3f test: exercise Schnorr signature cache in txvalidationcache_tests.cpp (Sebastian Falbesoner)
198b36bc85 test: respect "TAPROOT requires WITNESS" rule in `ValidateCheckInputsForAllFlags` (Sebastian Falbesoner)
e78a2a0d00 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 3ba1bbfa3f
instagibbs:
ACK 3ba1bbfa3f
Tree-SHA512: e43f7077d9e9ab6f8b5e9e70f0187767d65f686ce24350ce5d61cc4cdf07d5eebdf5e4327ce665c212b7cc02be1e8632a6a9fbcf6be2916f8e058993e5fb2650
`CreateFromDump` never writes to the vector, so the loop that prints it
in wallet-tool cannot produce output. `tool_wallet.py` already asserts
empty output for `createfromdump`.
The only `warnings.push_back()` was removed in 7a41c939f0 ("wallet:
Remove -format and bdb from wallet tool's createfromdump").
The counter is never incremented. Only the BDB implementation ever
maintained it, and that went away in 04a7a7a28c ("build, wallet, doc:
Remove BDB"). `AddRef()` and `RemoveRef()`, which the comment above the
member describes as maintaining it, were removed in c0f3f3264f
("wallet: Remove unused db functions"), leaving the member behind.
`m_next_external_index` and `m_next_internal_index` are declared and
initialized and never read or written afterwards. Their last uses were
removed in 83af1a3cca ("wallet: Delete LegacySPKM"). Neither appears in
`SERIALIZE_METHODS` or in `SetNull`.
