*** Please remove the following help text before submitting: *** - #1
Open
Zaidalamari wants to merge 4552 commits into
Open
Zaidalamari wants to merge 4552 commits into
Zaidalamari wants to merge 4552 commits into
Conversation
`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.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Suggested as a followup for #36088: #36088 (comment)
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.
1ad8641 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 1ad8641 Tree-SHA512: d63d2f5aeac487f01012b8802aff32eb53a8b5d53b8a6c8ece2a40b8c9603402f4f9a20c0918b3007ffaee1ed38a1009698e7b80be29c6a5175517e3279db952
Noted in the last iwyu PR: #35900 (review)
7fcaccd 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 7fcaccd sedited: ACK 7fcaccd janb84: ACK 7fcaccd Tree-SHA512: 3d439774d394f081b8107f8131963f7aa23ed048b0d6d349a80f9b3481fefeef7b5ce239fbd33606ad1f4960e6bfd899f968c50febc70d38b2fe731c6049583f
…amples 21d4e0b 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 21d4e0b 🚝 sedited: ACK 21d4e0b Tree-SHA512: 2a8abc07d681b9dc81b8079a68421278da890049cea33a1561a48d53cbf919a30df588f559e9df94fa4a1ab7027f742f3b12c163afc25246a620340cb3522336
Read() returns false for both a missing key and a deserialization
failure, making it impossible for callers to distinguish between
them.
This commits adds TryRead() returning a ReadStatus struct that
discriminates between:
- true: record found, value deserialized
- false: record not found
- DatabaseError: levelDB threw during record read
- DeserializationError: key present, value incompatible with
expected format
An err_msg field preserves the original exception message for
diagnostic purposes.
This also makes Read() a thin wrapper over TryRead() to keep
existing call sites unchanged.
Note:
Key serialization is the only operation that may throw in
TryRead(), as callers are expected to provide well-formed keys.
This is why this function is not noexcept.
If a UTXO entry on disk can't be deserialized, the node treats it as if the coin doesn't exist. Any block that spends that coin is permanently rejected as invalid (BLOCK_FAILED_VALID), silently forking the node from the rest of the network. This can hardly be triggered in practice (details below), but it's still the wrong behavior that could affect us in the future. The root cause is that CDBWrapper::Read() returns false for both missing keys and deserialization failures, so the consensus class CCoinsViewDB::GetCoin() has no way to tell them apart. CCoinsViewErrorCatcher was built to catch database read errors and abort, but it never fires because CDBWrapper::Read() swallows the exception before it can propagate. In practice, this scenario isn't a latent risk at the moment. It requires either a bug in the coin serialization path, or memory corruption before the data reaches LevelDB (at which point we have bigger problems). Any random disk-level bit flips are caught earlier by LevelDB's verification (the verify_checksums=true option enabled by default), which surfaces as a DatabaseError rather than a deserialization failure. This commit switches CCoinsViewDB::GetCoin() to use CDBWrapper::TryRead(), which lets the caller discriminate between all possible outcomes. On deserialization error, the exception now propagates through CCoinsViewErrorCatcher to ExecuteBackedWrapper(), which invokes the shutdown callbacks and aborts the node accordantly. This also fixes PeekCoin(), which delegates to GetCoin() at the CCoinsViewDB level.
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.
4a12773 test: cover DERSIG rejects a non-compound signature type (ViniciusCestarii) 86c7fb9 test: cover OP_16 does not count towards the opcode limit (ViniciusCestarii) 331bf79 test: cover OP_WITHIN must pop all 3 elements (ViniciusCestarii) 3bb87bc test: cover OP_FROMALTSTACK must pop the altstack (ViniciusCestarii) Pull request description: Kills some live mutants on interpreter.cpp that affect consensus found by https://bitcoincore.space. They are: <details> <summary><a href="https://bitcoincore.space/src/script/interpreter.cpp#3951">interpreter.cpp#3951</a>: <code>OP_FROMALTSTACK</code>: removed <code>popstack(altstack)</code></summary> ```diff diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 98b16ec..68265d20b5 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -698,7 +698,7 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& if (altstack.size() < 1) return set_error(serror, SCRIPT_ERR_INVALID_ALTSTACK_OPERATION); stack.push_back(altstacktop(-1)); - popstack(altstack); + } break; ``` </details> <details> <summary><a href="https://bitcoincore.space/src/script/interpreter.cpp#4084">interpreter.cpp#4084</a>: <code>OP_WITHIN</code>: removed one <code>popstack(stack)</code></summary> ```diff diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 98b16ec..874cf5e1cf 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1018,7 +1018,7 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& CScriptNum bn2(stacktop(-2), fRequireMinimal); CScriptNum bn3(stacktop(-1), fRequireMinimal); bool fValue = (bn2 <= bn1 && bn1 < bn3); - popstack(stack); + popstack(stack); popstack(stack); stack.push_back(fValue ? vchTrue : vchFalse); ``` </details> <details> <summary><a href="https://bitcoincore.space/src/script/interpreter.cpp#3883">interpreter.cpp#3883</a>: opcode limit: <code>opcode > OP_16</code> → <code>opcode >= OP_16</code></summary> ```diff diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 98b16ec..e985643606 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -459,7 +459,7 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& if (sigversion == SigVersion::BASE || sigversion == SigVersion::WITNESS_V0) { // Note how OP_RESERVED does not count towards the opcode limit. - if (opcode > OP_16 && ++nOpCount > MAX_OPS_PER_SCRIPT) { + if (opcode >= OP_16 && ++nOpCount > MAX_OPS_PER_SCRIPT) { return set_error(serror, SCRIPT_ERR_OP_COUNT); } } ``` </details> <details> <summary><a href="https://bitcoincore.space/src/script/interpreter.cpp#3808">interpreter.cpp#3808</a>: <code>IsValidSignatureEncoding</code>: compound type check returns <code>true</code></summary> ```diff diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 98b16ec..b613a6ac19 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -133,7 +133,7 @@ bool static IsValidSignatureEncoding(const std::vector<unsigned char> &sig) { if (sig.size() > 73) return false; // A signature is of type 0x30 (compound). - if (sig[0] != 0x30) return false; + if (sig[0] != 0x30) return true; // Make sure the length covers the entire signature. if (sig[1] != sig.size() - 3) return false; ``` </details> Recommend reviewing per commit. ACKs for top commit: instagibbs: ACK 4a12773 brunoerg: ACK 4a12773 jeanpablojp: tACK 4a12773 Tree-SHA512: 5f53c733d11cb5d645f420d90ab626f894ef0bb155d01b9de0cae502109b2eaa46c072797d08df115da7a8738f01f31212a207a4d0e6f782128beb37332cf46e
The private method has no callers. It opened a `WalletBatch` and forwarded to `AddDescriptorKeyWithDB`, which is still called from two other places. Its last caller, in `CWallet::AddWalletDescriptor`, was replaced in aa4f782 ("wallet: include keys when constructing DescriptorSPKM during import"), which builds the manager with `CreateFromMigration` instead of adding the key afterwards.
No callers. It was used by `COutput::print()`, which was removed in 3802224 ("Remove all other print() methods").
`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 83af1a3 ("wallet: Delete LegacySPKM"). Neither appears in `SERIALIZE_METHODS` or in `SetNull`.
The counter is never incremented. Only the BDB implementation ever maintained it, and that went away in 04a7a7a ("build, wallet, doc: Remove BDB"). `AddRef()` and `RemoveRef()`, which the comment above the member describes as maintaining it, were removed in c0f3f32 ("wallet: Remove unused db functions"), leaving the member behind.
`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 7a41c93 ("wallet: Remove -format and bdb from wallet tool's createfromdump").
…ache_tests.cpp 3ba1bbf test: exercise Schnorr signature cache in txvalidationcache_tests.cpp (Sebastian Falbesoner) 198b36b test: respect "TAPROOT requires WITNESS" rule in `ValidateCheckInputsForAllFlags` (Sebastian Falbesoner) e78a2a0 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 c6fcc8f..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 3ba1bbf sedited: ACK 3ba1bbf instagibbs: ACK 3ba1bbf Tree-SHA512: e43f7077d9e9ab6f8b5e9e70f0187767d65f686ce24350ce5d61cc4cdf07d5eebdf5e4327ce665c212b7cc02be1e8632a6a9fbcf6be2916f8e058993e5fb2650
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.
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.
15630c7 validation: remove unused m_chainparams from ATMPArgs (fanquake) 84c5290 validation: remove unused args from PolicyScriptChecks (fanquake) a9d5cf7 validation: remove unused args from ConsensusScriptChecks (fanquake) d26dc09 validation: remove unused total_vsize arg from PackageRBFChecks (fanquake) 2cb6c15 validation: remove unused PackageMempoolAcceptResult constructor (fanquake) Pull request description: Remove some unused code from validation. ACKs for top commit: thomasbuilds: ACK 15630c7 sedited: ACK 15630c7 yuvicc: ACK 15630c7 hebasto: ACK 15630c7, completeness of removing unused parameters in the `validation` module verified by overriding the `-Wunused-parameter` compiler flag for `src/validation.cpp`. jeanpablojp: tACK 15630c7 Tree-SHA512: a01ff6ea758132d6ad4c163d51c36d9e2cfaf91e90ca6451323591341fefec23c875af26e0b66e6cdba87ae6cab1418048c8788361d9c62fb8e0400d4dcaeac7
…lock ff3e2e4 net: Trigger process abort when behind start block MTP (Hodlinator) 1883cec 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](#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 ff3e2e4 sedited: ACK ff3e2e4 mzumsande: Code Review ACK [ff3e2e4](ff3e2e4) Tree-SHA512: bdd82fd0609309aa4bea026db1b607ae856c53403ec01b2511fa2ccae9db4ff1bb9e39523b446583c09ae53823275b8a603050d9090b61fabb84fab35e458f28
8d93098 refactor: Replace !ContainsNoNUL() with ContainsNUL() (Hodlinator) Pull request description: Avoids frequent double negation. See also fa7078d when it was renamed from the previous name, "ValidAsCString()". Found while reviewing #35041. ACKs for top commit: maflcko: lgtm ACK 8d93098 l0rinc: code review ACK 8d93098 sedited: ACK 8d93098 janb84: ACK 8d93098 Tree-SHA512: 3ed1d264953f08272c115d760e8149c5985d63331404ac3b1017a277c4b3a61862915851fe45746e26ed46fe7478f851680d205e5ef83e727d061f2867fed99c
78e691e rpc: Change listunspent's ancestorfees type to NUM (sedited) 73fb9ce 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 78e691e stickies-v: ACK 78e691e musaHaruna: Tested ACK [78e691e](78e691e) Tree-SHA512: 121d80520a39738c1c7375a50bb552203fe2db403cb3414195e6a79142677ac3c3509ba5f18d4b1982a8e2872c73e47cf6e54b6acd66b1a71ddcbe335ea33f34
…ing clocks 55390d1 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 55390d1 jonatack: ACK 55390d1 Tree-SHA512: 244a0cb634a0ba67fa88fe83f73111e475f6fff258cd1783b9b0a39669eefdd3b2e739a0b761616972bc938216336e18b0c0322e1201c2e805767cb813ca616c
fa7be0a 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 fa7be0a Tree-SHA512: c533a8aebd92f3f1054563f20af438165632c98f7a2f189f3306420780468b143c24001f794a79ddfc0527c9605a4cfe59949648a9a7f41bbe138128b09f0a6e
fa39710 ci: Exclude subtrees from iwyu (MarcoFalke) fa85661 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 fa39710. Tree-SHA512: 9a555ab020f0f1a2bc4d70ea72011f8d42ba4bfe4a463947d31b0d208b4671b76b466f92a18b6295bc7a8c5bb67c6f697844f673fc02e18983b062d25bc0dc8c
f04b0c3 refactor(test): Make test method use Uppercase, shorten enum values (Hodlinator) 6eea8e3 refactor(test): Simplify HTTP response check (Hodlinator) Pull request description: Improves recently added unit tests. * Simplify HTTP response check in `http_server_socket_tests`, it was incorrectly referring to `unordered_map` * Make `http_request_state_tests` unit test method use Uppercase as per developer-notes.md, shorten `enum` values for readability ACKs for top commit: winterrdog: tACK f04b0c3 janb84: ACK f04b0c3 pinheadmz: ACK f04b0c3 sedited: ACK f04b0c3 Tree-SHA512: fd2ecb2f6803b2eafbcc9dd4881b8370be3b91ae6680a5d390af4da674b6c43fbc67ba4724762f9e71a5c455828ab07708ee880ca30f62947d6850dc8d9669cf
59a465c guix: Validate codesigned windows binaries (Ava Chow) 66eeac2 guix: Use osslsigncode 2.14 (Ava Chow) Pull request description: #34550 mistakenly removed the package `nss-certs`. This results in an error during codesigining: `Use the "-CAfile" option to add one or more trusted CA certificates to verify the signature.` However, introducing the package is not enough to make codesigning work. #34550 switched us to using osslsigncode 2.13 from upstream, but osslsigncode versions 2.6 through 2.13 all require network access as they validate CRLs. While the `verify` command has the ability to skip CRL validation, `attach-signature` does not. osslsigncode 2.14 resolves both issues for us by removing signature validation, instead it only checks that the hash in the signature matches the hash of the binary. But we still want to do a belt-and-suspenders signature validation, and we can achieve this by calling `verify` afterwards with CRL validation disabled to avoid the network access issue, along with including the `nss-certs` package. Lastly, guix upstream already has 2.14, so we could get it by bumping the time-machine commit. But it seems like bumping that is problematic for other packages, see #36233 and #35855. Instead, this PR copies in the package definition from upstream. ACKs for top commit: Sjors: re-utACK 59a465c hebasto: re-ACK 59a465c, tested the signed installer on Windows: Tree-SHA512: 4c5915aa23b5a9ace37003b72ce94a1b1711c39503e4793d4145fb99d96180329056902f94c29094986a4ea57217cffef6deb51e4f4bb4e9a7fcdc9bae38568a
…ions 4556ef6 torcontrol: Apply reconnect backoff after dropped connections (Fabian Jahr) Pull request description: Since #34158 the reconnect backoff was only applied when connecting to the Tor control port failed. When an established connection was dropped, for example by Tor closing it after a failed `AUTHENTICATE` because of a wrong password, the control thread reconnected immediately in a loop without any wait. This was resulting in us trying to make tons of connections to torcontrol and producing tons of log entries in very short time. The fix restores the pre-#34158 behavior where every reconnect waits for the backoff timeout by going through `disconnected_cb`, which now also does the waiting. Also adds a functional test to cover that the waiting behavior is actually applied. ACKs for top commit: willcl-ark: reACK 4556ef6 winterrdog: tACK 4556ef6 sedited: ACK 4556ef6 Tree-SHA512: bfc4cee5f2f5cd9d7c251ca4f1201a32bff05340986500a9b883dc80c73ddf313a4ae611cefcc3abf463ec6b729264d41794608175644d1d064d53985543e815
b078105 Remove MSVCRT-specific workarounds (Hennadii Stepanov) 7c86a37 doc: Update Windows build docs to reflect migration from MSVCRT to UCRT (Hennadii Stepanov) f5910d1 ci: Remove Windows + MSVCRT jobs for cross-compiling and native testing (Hennadii Stepanov) 55f86ab guix: Use UCRT runtime for Windows release binaries (Hennadii Stepanov) Pull request description: This PR: 1. Switches Windows release binaries to UCRT. 2. Update docs. 3. Drops MSVCRT workarounds. Closes #30210. ACKs for top commit: maflcko: review ACK b078105 🏦 fanquake: ACK b078105 - will followup with some docs / other related changes. This also doesn't add a check for the release builds. Tree-SHA512: 553e4befff6d79a22728ecaae104db00f350ebe1b560d19bdb820b41a242ea2b6a922a6988c98fc5904f7b4de4b799fd61d45b2d0645d0bba42b79dbba9fe044
Now that we target macOS >= 14.0 and use macOS SDK 14.0, and no-longer use objc_msgSend, this can be removed.
53a1914e00 Merge bitcoin-core/leveldb-subtree#65: refactor: use inline constexpr over static const in headers 811ba7c883 refactor: use inline constexpr over static const in headers git-subtree-dir: src/leveldb git-subtree-split: 53a1914e0040541e5ab6efc19748fc20d5c821ce
2c249db wallet: Add an importDescriptors() interface for the wallet (Pol Espinasa) 3be5f40 wallet: add CheckDescriptorRangeBounds (Pol Espinasa) a9cd985 wallet: Move ImportDescriptor and ProcessDescriptorsImport to imports.cpp (Pol Espinasa) c9650d8 wallet: rename ProcessDescriptorImport to ImportDescriptor and add ProcessDescriptorsImport (Pol Espinasa) a40ee4e wallet: rpc: refactor: ProcessDescriptorImport returns ImportDescriptorResult (Pol Espinasa) 48d3d71 wallet, util: Add HandleWalletErrorCode (Pol Espinasa) 04c73a9 wallet: Add ImportError struct and new WalletError codes (Pol Espinasa) 9752156 wallet: rpc: refactor: Extract UniValue processing from ProcessDescriptorImport (Pol Espinasa) f1f61af wallet: Add ImportDescriptorRequest structs (Pol Espinasa) 7375124 wallet: refactor: make is_ranged no longer an optional (Pol Espinasa) 9048510 wallet: lower the minimum timestamp to 0 (Pol Espinasa) 380d3ae wallet: rpc: Use std::optional in GetImportTimestamp (Pol Espinasa) Pull request description: This PR adds an interface for importing descriptors. The motivation behind this is that currently, importing descriptors is only possible via RPC. Bitcoin Core GUI doesn't use the RPC interface so it cannot offer descriptor import functionality, which is needed to support more complex wallet setups such as multisig. This PR also adds a refactor by moving the `importdescriptors` logic from the RPC layer into `CWallet::ImportDescriptor`, making it reusable by both the RPC and this new interface. The main changes are: - Introduces `CWallet::ImportDescriptor()` containing the core import logic, previously embedded in the RPC `ProcessDescriptorImport` function. - Introduces `wallet::ImportDescriptorResult`, a new result struct that carries success status, error message, warnings, and a `FailureReason` enum. The RPC layer uses `FailureReason` to map results back to the appropriate JSON-RPC error codes, keeping RPC concerns out of `CWallet`. - Updates `ProcessDescriptorImport` in `rpc/backup.cpp` to delegate to `CWallet::ImportDescriptor`. - Adds `interfaces::Wallet::importDescriptors()` as a new interface method, allowing the GUI to import descriptors without going through RPC. I have a GUI menu here: polespinasa#7 so it can be tested. I will open a PR against the main GUI repo, once this gets merged. ACKs for top commit: achow101: re-ACK 2c249db w0xlt: reACK 2c249db arejula27: reACK 2c249db Tree-SHA512: fa6fefc404c1015793cf5da53fe8a4352bbe46865eeb5c65d95afb3fa897667956ed9b912a11163dc7194b10bcb9f6b74211622c846e57908524e910a3c5a2ef
This partially reverts fa74f58 to avoid initialization dependencies, which the linker fails to handle properly.
4be88b9 Squashed 'src/leveldb/' changes from 13da2d6758..53a1914e00 (fanquake) Pull request description: Includes: * bitcoin-core/leveldb-subtree#65 Used in #36275. ACKs for top commit: l0rinc: Code review ACK 7528f79 hebasto: ACK 7528f79. sedited: ACK 7528f79 Tree-SHA512: 2dc6acb7a3b069e1d2f00992e830223f63edab2fb0c2e8b2ed3334199c95a7185515bed585131178f5bf08096e90483ab1a16d8c31be856993cfaf4544caf5d6
…ound ld64 bug fa8fefb refactor: Use static const over inline const to work around ld64 bug (MarcoFalke) Pull request description: This partially reverts fa74f58 to avoid initialization dependencies, which ld64 fails to handle properly. Works around #36281 for now. Obviously this will increase the bin size again (70kB for me), but this shouldn't matter much. ACKs for top commit: janb84: ACK fa8fefb sedited: ACK fa8fefb Tree-SHA512: a31f3f4503dbc1898f346a5ceb94de61d82fd5902feec65f3716dc785dc199223c66cf223dcbe8a434e0a05af04bc1b0e7a066c3361deded947691c501ab0228
This reverts commit 00a5f9b.
Drop the default constructor and add a factory function to construct a WalletDescriptor from stream
The canonical string comparison was slow because it would compute the canonical string for each comparison. This can be sped up by holding the canonical string in memory, computed upon construction of WalletDescriptor. To reduce memory usage, this string is further hashed so that the comparison operates over the hash of the canonical string.
The hardened indicator for Minscript expressions in CompatDescriptorHash uses whichever hardened indicator was originally given by the user.
0b353a9 netgroup: Cache asmap version (Fabian Jahr) c1e9d15 doc: Describe how to verify the asmap in use (Fabian Jahr) 87ce88c rpc: Add asmap_version to getnetworkinfo (Fabian Jahr) dd5a8a6 test: Check embedded asmap version log and addrman re-bucketing (Fabian Jahr) 0bc8736 init: Log asmap version from NetGroupManager (Fabian Jahr) fdfd196 rpc: Reuse AsmapVersion in exportasmap (Fabian Jahr) Pull request description: This is a follow-up to #36215. I noticed the possible improvement in `exportasmap` and went through everything to see where else we could make changes where we can use this as an advantage. - `rpc: Reuse AsmapVersion in exportasmap`: `exportasmap` hashed the file by hand to show the hash, instead we can use `AsmapVersion()` which is the same value. - `init: Log asmap version from NetGroupManager`: Drop the version variable in init and log the version by getting it from `NetGroupManager` instead - `test: Check embedded asmap version log and addrman re-bucketing`: We didn't cover rebucketing behavior in the functional test yet. Also gets rid of stale comment. - `rpc: Add asmap_version to getnetworkinfo`: Getting access to the asmap version from the logs may be a bit tedious for some users and now it may be even more interesting for them to compare the version to the hash attested to in `asmap-data`. So let them get it via `getnetworkinfo`. - `doc: Describe how to verify the asmap in use`: Just mention the latest changes from above and the opportunity to compare the version to the hash in seen in `asmap-data`. ACKs for top commit: sedited: Re-ACK 0b353a9 willcl-ark: ACK 0b353a9 Tree-SHA512: daed639f6cbec5ec3bbe04771970edae973e749ff15a24cf3f6d38e0991d4e26462fd4e49db78b75465623115ac967776626d2749f03344018cfb53658492909
b388f9b crypto: Fix MuHash3072 division by itself (Fabian Jahr) Pull request description: `MuHash3072::operator/=` multiplies the numerator by the divisor's denominator and then the denominator by the divisor's numerator. But as it is currently implemented the divisor could be the MuHash object itself. When that is the case, the second step reads the numerator that the first step already updated, so `x /= x` actually leaves `1/D` instead of the empty set. This only goes unnoticed when the denominator is 1, which is the case in our existing fuzz target and benchmark. No code in the node/index divides MuHash objects by themselves, so runnings nodes are not affected. Fixes the code by not using the potentially changed nominator, adds a test that reproduces the issue and updates the fuzz test to not always use denominator 1. ACKs for top commit: furszy: utACK b388f9b sedited: ACK b388f9b sipa: utACK b388f9b Tree-SHA512: ae2eb845db07fb140e7946dfc7d084766e5f6d06060b91da6acc3434fdf6b59ad1eb050b4be79bc4644dea032d1ca0d01dd9e9b366bd04a545cf5224e57526df
Correct six argument metadata entries that produce misleading or invalid OpenRPC defaults. The getdeploymentinfo blockhash fallback and four sighashtype fallbacks describe how omitted arguments are resolved. They are not literal values accepted by the RPCs, so mark them as DefaultHint values. The send include_watching option is boolean, but its string default makes the generated schema internally inconsistent. Use a boolean value, matching the analogous sendall option. Runtime behavior is unchanged.
b7f740c rpc: Correct OpenRPC default metadata (will) Pull request description: `getopenrpcinfo` emits two defaults that do not satisfy their schemas. This changes `getdeploymentinfo.blockhash` to a default hint, since its fallback describes the current chain tip, and makes `send.options.include_watching` default to boolean `false`. ACKs for top commit: nervana21: ACK b7f740c sedited: ACK b7f740c Tree-SHA512: fd99c3642ff39ef13116f7f18e2a93b398e347330ea24e5bfd01744528744e5c6fbff12a56213961f8703218a6100f0ea74a10fe3b1d9a221ba8b1fa307a907a
…ther canonical descriptor string followups 61edcf9 test: Add 31.1 to wallet back compat (Ava Chow) ebf2f69 test: Simplify miniscript descriptor check in wallet back compat (Ava Chow) f6cbcfd wallet: Document WalletDescriptor::UpdateFrom (Ava Chow) f0f6dce descriptor, doc: Clarify miniscript CompatDescriptorHash (Ava Chow) 405b1d6 descriptor: Explicitly handle use_apostrophe cases (Ava Chow) 64abb3e wallet: Compare descriptors by hash of canonical string (Ava Chow) bb5e832 wallet: Make WalletDescriptor's descriptor const (Ava Chow) 4cc00f7 wallet: Remove WalletDescriptor's default constructor (Ava Chow) Pull request description: Instead of re-computing the canonical descriptor string for every call to `HasWalletDescriptor`. `WalletDescriptor` will now compute it once upon construction and cache the hash of that string. The comparison uses a new `WalletDescriptor::Equals` function which compares the canonical string hashes. The hash is used to avoid holding possibly a large amount of memory for a string that is rarely used. This should fix the performance regression described in #35445 (comment) Also addresses several review comments related to documentation and code readability: - #35445 (comment) - #35445 (comment) - #35445 (comment) - #35445 (comment) - #35445 (comment) - #35445 (comment) ACKs for top commit: Sjors: ACK 61edcf9 polespinasa: ACK 61edcf9 Tree-SHA512: d13057cdfa89f9831950502f19159a40ca1da693f47d8ae63613fbc74b588994f6748b9124947b29ad1f035cff6650f8ae3d7396025a2363fcf3d32de002adf1
4f624bb Revert "build: Remove `cmake/script/CoverageFuzz.cmake`" (sedited) Pull request description: This reverts commit 00a5f9b as requested in #36161 (comment) . As noted there the script still has users, so should not be removed. ACKs for top commit: kevkevinpal: ACK [4f624bb](4f624bb) marcofleon: ACK 4f624bb Tree-SHA512: 9f79790d5ccc8787f9325fa46a6732ea99b393ac164a55850c4ee0cb6b0042148389876db6dab6b1cc724a8d2732fc8cacf5d1c383e2fbeb027173b0d3389979
0b46fc9 http: Add missing LIFETIMEBOUND annotations (Hodlinator) Pull request description: Helps Clang detect certain dangling reference issues, in a similar vein as #36164. ### Known limitations It doesn't catch invalidation nor brace-initialization. <details><summary>Diff illustrating limitations</summary> ```diff --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -80,6 +80,15 @@ BOOST_AUTO_TEST_CASE(test_query_parameters) BOOST_AUTO_TEST_CASE(http_headers_tests) { + auto foo = HTTPHeaders{}.FindAll("needle"); // Emits warning + (void)foo; + auto bar{HTTPHeaders{}.FindAll("needle")}; // No warning with Clang 22.1.8 :/ + (void)bar; + + HTTPHeaders test; + auto baz = test.FindAll("needle"); + test.Write("needle", "mutation"); // No warning with Clang 22.1.8 :/ + { // Writing response headers HTTPHeaders headers{}; ``` </details> Clang 24 has experimental invalidation detection so maybe that could be used in the far future: https://clang.llvm.org/docs/LifetimeSafety.html#use-after-invalidation-experimental ### Alternative solution A) Return by copy everywhere. Might introduce more heap activity, especially in the case of `HTTPRemoteClient::GetRequest()`. ### Alternative solution B) Refactor the methods to minimize copying while still making things more memory-safe. Replacing `HTTPHeaders::FindAll()` with an `Iterate()`-function taking a lambda which gets to process each header. Gets rid of the heap activity of building a `vector` but introduces copying of `first`. <details><summary>Diff of httpserver.cpp/h</summary> ```diff --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -272,15 +272,11 @@ std::optional<std::string> HTTPHeaders::FindFirst(const std::string_view key) co return std::nullopt; } -std::vector<std::string_view> HTTPHeaders::FindAll(const std::string_view key) const +void HTTPHeaders::Iterate(std::function<void(const std::string& key, const std::string& value)> fn) const { - std::vector<std::string_view> ret; for (const auto& item : m_headers) { - if (CaseInsensitiveEqual(key, item.first)) { - ret.push_back(item.second); - } + fn(item.first, item.second); } - return ret; } void HTTPHeaders::Write(std::string&& key, std::string&& value) @@ -504,18 +500,21 @@ bool HTTPRequest::LoadBody(LineReader& reader) // We read all the chunks but never got the last chunk, wait for client to send more return false; } else { + std::optional<std::string> first; + m_headers.Iterate([&first] (const std::string& key, const std::string& value) { + if (!CaseInsensitiveEqual(key, "Content-Length")) return; + if (!first.has_value()) { + first = value; + } else if (first != value) { + // Duplicate Content-Length headers are allowed only if they all have the same value + // https://www.rfc-editor.org/rfc/rfc7230#section-3.3.3 + throw std::runtime_error("Differing Content-Length values"); + } + }); // No Content-length or Transfer-Encoding header means no body, see libevent evhttp_get_body() - auto content_length_values{m_headers.FindAll("Content-Length")}; - if (content_length_values.empty()) return true; - - // Duplicate Content-Length headers are allowed only if they all have the same value - // https://www.rfc-editor.org/rfc/rfc7230#section-3.3.3 - const auto& first_content_length_value{content_length_values[0]}; - for (size_t i = 1; i < content_length_values.size(); ++i) { - if (content_length_values[i] != first_content_length_value) throw std::runtime_error("Differing Content-Length values"); - } + if (!first.has_value()) return true; - const auto content_length{ToIntegral<uint64_t>(first_content_length_value)}; + const auto content_length{ToIntegral<uint64_t>(first.value())}; if (!content_length) throw std::runtime_error("Cannot parse Content-Length value"); if (*content_length > MAX_BODY_SIZE) throw ContentTooLargeError("Max body size exceeded"); --- a/src/httpserver.h +++ b/src/httpserver.h @@ -97,10 +97,9 @@ public: */ std::optional<std::string> FindFirst(std::string_view key) const; /** - * @PARAM[in] key The field-name of the header to search for - * @returns Views into all values matching the provided key (valid while this object is alive) + * @PARAM[in] fn Receives each header as they are iterated through. */ - std::vector<std::string_view> FindAll(std::string_view key) const LIFETIMEBOUND; + void Iterate(std::function<void(const std::string& key, const std::string& value)> fn) const; void Write(std::string&& key, std::string&& value); /** * @PARAM[in] key The field-name of the header to search for and delete ``` </details> ### Rationale The methods are not called in many places so risk of misuse is low, and we avoid any risk of performance degradation (such as the one found in #35182 (comment)). Returning copies without adding mutexes or other thread safety measures does not considerably increase thread-safety. ACKs for top commit: maflcko: lgtm ACK 0b46fc9 l0rinc: code review ACK 0b46fc9 Tree-SHA512: d80670e3a832614f0b33b2c11e7ddd149b11f794e182c3dfb7c5c6045f23a7ea4a179da31c5fed94877a1df061968b65d13d47dcb7f858f853fa31e59497ee03
59f7f3d build: drop use of OBJC_OLD_DISPATCH_PROTOTYPES (fanquake) Pull request description: Now that we target macOS >= `14.0` and use macOS SDK `14.0`, and no-longer use `objc_msgSend`, this can be removed. Was added in #16720. ACKs for top commit: willcl-ark: ACK 59f7f3d Tree-SHA512: 020c66f9a1092301f2ae001186f6d3dbf5a05157d5d340d5072db1579c0c614ca1d305a54c3943d7a90df84ca62c665ac53981e550053135f0883943d08b7dfc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.