When setting both a new index and prune size and restarting
an unpruned node, the node will prune the block store first
and then the index will fail to start syncing.
Fix this by setting the prune lock to 0 if the index does not
yet have a best block.
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
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.
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
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
7fcaccd9d0 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 7fcaccd9d0
sedited:
ACK 7fcaccd9d0
janb84:
ACK 7fcaccd9d0
Tree-SHA512: 3d439774d394f081b8107f8131963f7aa23ed048b0d6d349a80f9b3481fefeef7b5ce239fbd33606ad1f4960e6bfd899f968c50febc70d38b2fe731c6049583f
1ad8641278 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 1ad8641278
Tree-SHA512: d63d2f5aeac487f01012b8802aff32eb53a8b5d53b8a6c8ece2a40b8c9603402f4f9a20c0918b3007ffaee1ed38a1009698e7b80be29c6a5175517e3279db952
`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.
bed46bd16c build: check for SetThreadDescription() at configure time (ViniciusCestarii)
Pull request description:
SetThreadDescription() is missing from mingw-w64 headers before 12.0.0, so the Windows cross-compile fails on distro toolchains, e.g. Ubuntu 24.04. Reported by hebasto in https://github.com/bitcoin/bitcoin/pull/35884#issuecomment-5379490348.
Check for the symbol at configure time and guard its use with a new `HAVE_SETTHREADDESCRIPTION` guard, as cmake/introspection.cmake already does for other optional symbols. This avoids having to declare a minimum mingw-w64 version: toolchains that have the symbol get OS-level thread names, older ones build fine without them.
ACKs for top commit:
fanquake:
utACK bed46bd16c - could be reverted + docs updated post branch-off.
hebasto:
re-ACK bed46bd16c.
Tree-SHA512: 3edbbd252fc68e976d930a8a6124746b3ba586ea58dc0720a67f8975e935057e2838bb7484d6b789771a327d2b69a092d64f335dd483f90b84a3ef290c138bfb
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.
b3ff9c4d68 iwyu: Fix warnings in `src/interfaces` and treat them as errors (Hennadii Stepanov)
d564b0255f 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 b3ff9c4d68 🖋
Tree-SHA512: 9359854c7b6e6ec93022947e78ef57fc68728326df874371028997a550fd1b6ef08e4b09271163b9d5c921c9e01deadf92b0eb5ae3f620d797e4f9bdea8f4727
d5e64b01e1 doc: note -blocknotify is not run during IBD/reindex in help text (Guillermo Fernandes)
Pull request description:
Adds a note to the `-blocknotify` help output that it is not executed for blocks connected during initial block download or reindexing (it only fires once the node reaches the post-init sync state). Suggested by sedited in review of #35518.
ACKs for top commit:
sedited:
ACK d5e64b01e1
Tree-SHA512: 06b4a3c6354689e9846b5af6c210c3bb360b917477ac9506814c0238a1afb78392c4b6a6bbd51a4b9d98c841a1bc86686e90072155ce9a4f5e8f875cfe288c0c
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
2777300c68 fuzz: Implement connect_block harness (Robin David)
40add915be test: Add reset to CuckooCache (Eugene Siegel)
Pull request description:
Adds a fuzz target that directly calls `ConnectBlock` with `fJustCheck` set to true, so it hits block/transaction validation without writing undo data or updating the chainstate.
This PR is essentially https://github.com/bitcoin/bitcoin/pull/34651 with some minor tweaks and style cleanups. Additional validation harnesses (e.g. https://github.com/bitcoin/bitcoin/pull/34895) could build on this test's setup.
ACKs for top commit:
Crypt-iQ:
ACK 2777300c68
nervana21:
tACK 2777300c68
Tree-SHA512: e2dc74154a6e29e0f3eaec9caeeec53d64bcc96adb0d1739281da97712dd931c3937eaf71f977bfc9c9330f26e35b3633f72788143b76e24e17c37b0a4258ba4
Add a method that clears and resets CuckooCache, intended for
use in tests only. Without this, fuzz tests may reuse the cache
across iterations, resulting in instability.
576a0ebb53 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 576a0ebb53
marcofleon:
ACK 576a0ebb53
jeanpablojp:
tACK 576a0ebb53
Tree-SHA512: a7533e68a95b2f0200abdcf08ae72a7f3654db03cffe642ed39b0a5aa48d932a8b07ee4e4477b591a488b5358dd19fe85a03015642b663fd41a771b3090bc8a3
fa72de78a9 util: Set Univalue to null after read failure (MarcoFalke)
fa7786592d test: Add UniValue failed read test (MarcoFalke)
Pull request description:
Currently, `UniValue::read()` may leave the value in a dirty/corrupt state after a read failure.
This is perfectly fine, because all production code-paths check the read return value and exit early.
However, it seems nicer and safer to discard the dirty and corrupt state. So do that here.
This refactor doesn't change any production behavior. However, it fixes a fuzz failure in the `rpc` target, which was recently reworked in commit fa895bb77a. Later, adding new fuzz inputs (e.g. `fuzz_corpora/rpc/fa1b0eeaa948a091f022c1ff2d0002a3fa6a631f `) and commit 747cff8424 made it hit this invalid UniValue code path.
ACKs for top commit:
rustaceanrob:
ACK fa72de78a9
hodlinator:
re-ACK fa72de78a9
jeanpablojp:
tACK fa72de78a9
Sjors:
ACK fa72de78a9
l0rinc:
code review ACK fa72de78a9
Tree-SHA512: 6c0597a5ab558dc7d22e1742e89078e07a59a87185228114db4381c3381e19b668b95e42cb7ff80288fa56fb15ea1e0e181f59ba3ceaea8a2a3bff12899c7821
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.
```
