Repository navigation
backport: v0.26 bitcoin#24148 - #56
Closed
DCG-Claude wants to merge 1 commit into
Closed
DCG-Claude wants to merge 1 commit into
DCG-Claude wants to merge 1 commit into
Conversation
DCG-Claude
force-pushed
the
backport-0.26-b033-src
branch
from
September 18, 2026 15:32
7485a01 to
edd20c1
Compare
Collaborator
Author
Dash's src/.clang-tidy enforces performance-no-automatic-move as a hard error (upstream did not at the time of bitcoin#24148), so the 🤖 backportsys, on behalf of the Dash backport pipeline. |
Collaborator
Author
The attached log contains only container/network teardown lines from the runner — no compiler diagnostic, no failing unit or functional test, and no TSan data-race report — so there is nothing in it that points at this branch. bitcoin#24148 touches only descriptor/miniscript parsing plus tests (the new wallet_miniscript.py is a watch-only import test and the framework here already provides add_wallet_options and skip_if_no_sqlite), and adds no threading or concurrency code that a TSan job could legitimately trip over. I read this as a job/infrastructure flake; please rerun linux64_tsan-test, and if it fails again please attach the portion of the log with the actual error. 🤖 backportsys, on behalf of the Dash backport pipeline. |
|
This pull request has conflicts, please rebase. |
8 tasks
ffc79b8 qa: functional test Miniscript watchonly support (Antoine Poinsot) bfb0367 Miniscript support in output descriptors (Antoine Poinsot) 4a08288 qa: better error reporting on descriptor parsing error (Antoine Poinsot) d25d58b miniscript: add a helper to find the first insane sub with no child (Antoine Poinsot) c38c7c5 miniscript: don't check for top level validity at parsing time (Antoine Poinsot) Pull request description: This adds Miniscript support for Output Descriptors without any signing logic (yet). See the OP of bitcoin#24147 for a description of Miniscript and a rationale of having it in Bitcoin Core. On its own, this PR adds "watchonly" support for Miniscript descriptors in the descriptor wallet. A follow-up adds signing support. A minified corpus of Miniscript Descriptors for the `descriptor_parse` fuzz target is available at bitcoin-core/qa-assets#92. The Miniscript descriptors used in the unit tests here and in bitcoin#24149 were cross-tested against the Rust implementation at https://github.com/rust-bitcoin/rust-miniscript. This PR contains code and insights from Pieter Wuille. ACKs for top commit: Sjors: re-utACK ffc79b8 achow101: ACK ffc79b8 w0xlt: reACK bitcoin@ffc79b8 Tree-SHA512: 02d919d38bb626d3c557eca3680ce71117739fa161b7a92cfdb6c9c432ed88870b1ed127ba24248574c40c7428217d7e9bdd986fd8cd7c51fae8c776e1271fb9 Dash adaptations: - src/script/descriptor.cpp: Dash has no segwit, so `ParseScriptContext::P2WSH` does not exist. Miniscript is wired to Dash's only hash-wrapped-script context, `ParseScriptContext::P2SH`, at all five upstream P2WSH sites: `KeyParser::FromString`, `KeyParser::FromPKBytes`, `KeyParser::FromPKHBytes`, the miniscript gate in `ParseScript`, and the miniscript gate in `InferScript`. Miniscript expressions therefore live inside `sh()` instead of `wsh()`. - src/script/descriptor.cpp: the user-visible error string was written as "Miniscript expressions can only be used in sh" instead of upstream's "... can only be used in wsh". - src/script/descriptor.cpp: upstream's new `InferPubkey` body uses `ConstPubkeyProvider(0, pubkey, /*xonly=*/false)` and a 3-arg `OriginPubkeyProvider`. Dash's `ConstPubkeyProvider` takes 2 args (no xonly flag) and `OriginPubkeyProvider` takes a trailing `apostrophe` flag, so the moved-up `InferPubkey` was written as `ConstPubkeyProvider(0, pubkey)` / `OriginPubkeyProvider(0, std::move(info), std::move(key_provider), /*apostrophe=*/false)`. - src/script/descriptor.cpp: `InferXOnlyPubkey` and upstream's `InferMultiA` were dropped rather than added — Dash has no `XOnlyPubKey` inference, no `MatchMultiA`, and no `MultiADescriptor`. - src/script/descriptor.cpp: the `DescriptorImpl` conflict was resolved keeping Dash's existing comment wording on `m_pubkey_args` while taking upstream's move of the `protected:` label above that member (so `MiniscriptDescriptor` can reach it). - src/test/descriptor_tests.cpp: the DoCheck hunks were resolved onto Dash's `Parse(prv, keys_priv, error)` / `UseHInsteadOfApostrophe(pub)` flow, keeping Dash's apostrophe-vs-h handling and taking upstream's `BOOST_CHECK_MESSAGE(EqualDescriptor(...))` upgrades. - src/test/descriptor_tests.cpp: upstream's negative Miniscript tests were rewritten from `wsh(...)` to `sh(...)`, and the expected error for the context check is "Miniscript expressions can only be used in sh". Upstream's private descriptors use Bitcoin mainnet WIF keys, which do not decode under Dash's base58 prefixes and could not be re-encoded in this environment, so the hex public keys are used for both the private and the public descriptor in those `CheckUnparsable` calls; a comment in the file records this. - test/functional/wallet_miniscript.py: every descriptor is wrapped in `sh(...)` instead of `wsh(...)` (the four `MINISCRIPTS` policies via `descsum_create(f"sh({ms})")`, and the insane-descriptor sanity check). All four policies compile to redeemScripts well under the 520-byte `MAX_SCRIPT_ELEMENT_SIZE` push limit. - test/functional/wallet_miniscript.py: added `add_options` calling `self.add_wallet_options(parser, legacy=False)`. Dash's test framework requires this declaration, otherwise `self.options.descriptors` is `None` and the node is started with `-disablewallet`. - test/functional/test_runner.py: the new entry is `'wallet_miniscript.py --descriptors'` — the explicit flag is needed because Dash's framework runs with `REQUIRE_WALLET_TYPE_SET`. Upstream's hunk also carried the context line `'feature_maxtipage.py'`; it was dropped because Dash already lists that test earlier in the file and re-adding it would duplicate the entry. Not applicable to Dash (intentionally omitted): - src/test/descriptor_tests.cpp: Two positive Miniscript Check() cases (wsh(...) and sh(wsh(...))). They expect segwit scriptPubKeys (0020..., P2SH-P2WSH), OutputType::BECH32/P2SH_SEGWIT and MIXED_PUBKEYS, none of which exist in Dash. The positive sh() path is covered by wallet_miniscript.py, which passes. (reviewer: not for Dash) - src/test/descriptor_tests.cpp: tr(miniscript) CheckUnparsable case and the long tr() fuzz-regression CheckUnparsable case. Dash has no taproot (no tr() descriptor, no P2TR context). (reviewer: not for Dash) - src/test/descriptor_tests.cpp: raw(miniscript) CheckUnparsable case whose public side is sh(miniscript) expecting 'can only be used in wsh'. In Dash sh(miniscript) is the valid context, so the pairing cannot carry over. The same error path is tested by the top-level (no wrapper) CheckUnparsable case. (reviewer: not for Dash) - src/test/descriptor_tests.cpp: 'Invalid checksum' CheckUnparsable case on a wsh(miniscript)#abcdef12 descriptor. Its expected computed checksum is for the wsh() string. This omission was carried over unchanged from the already-reviewed backport. It only exercises the generic checksum error path, which Dash's existing checksum tests already cover. No prerequisite blocks it, so an sh() variant could be added later as a test-only follow-up. (reviewer: not for Dash) Replayed onto a newer base. Dash adaptations: - src/script/descriptor.cpp: InferPubkey is moved above KeyParser as upstream 24148 does, but the moved body is develop's current one: it keeps the IsValidNonHybrid() and TOP/P2SH-only-uncompressed checks and the named `ctx` parameter that the partial bitcoin#28602 backport (13def7a) added - src/script/descriptor.cpp: KeyParser::FromPKBytes/FromPKHBytes check InferPubkey's nullable result (`if (auto pubkey_provider = InferPubkey(...))`), which is how upstream bitcoin#28602 rewrote these KeyParser methods (it landed after 24148). Develop's InferPubkey can now return nullptr, and leaving these unchanged would push null providers. This replaces the prior diff's `pubkey.IsValid()` check in FromPKBytes, because InferPubkey's IsValidNonHybrid() check covers it - src/script/descriptor.cpp: InferScript puts the P2SH Miniscript inference block before the 'top-level only descriptors' early return that bitcoin#28067 added (48a3b6c). This is the same order as upstream master (see 744157e), so sh(miniscript) inference is not cut off - src/script/miniscript.h: the Parse() conflict is resolved to upstream's `return std::move(constructed.front());`. Develop had already made tl_node non-const via bitcoin#26707, so the prior diff's DecodeScript `const NodeRef` -> `NodeRef` hunk is already on develop and is no longer in this diff - src/test/descriptor_tests.cpp: the Miniscript CheckUnparsable cases from the prior diff (sh() context, hex pubkeys instead of Bitcoin mainnet WIF keys) are appended after develop's CheckInferDescriptor cases from bitcoin#28602; both sets are kept - Carried over unchanged from the prior reviewed backport: Miniscript is only allowed inside sh() (Dash has no segwit), so the error text is 'Miniscript expressions can only be used in sh', and KeyParser and InferScript use ParseScriptContext::P2SH instead of P2WSH; wallet_miniscript.py wraps each miniscript in sh() instead of wsh() Not applicable to Dash (intentionally omitted): - src/test/descriptor_tests.cpp: Upstream's two positive Miniscript Check() cases are wsh()/sh(wsh()) descriptors with witness-program scripts and Bitcoin mainnet WIF keys. Dash has no segwit. This omission is carried over from the prior reviewed backport and is documented in a code comment; wallet_miniscript.py covers the sh() positive path
DCG-Claude
changed the title
DCG-Claude
force-pushed
the
backport-0.26-b033-src
branch
from
September 26, 2026 20:43
edd20c1 to
90ae269
Compare
Collaborator
Author
|
every backport on this branch is either on develop already or could not be carried onto the current develop; nothing left to carry - bitcoin#24148: could not be fixed: Confirmed: upstream relies on P2WSH limits inside IsValid()/CheckStackSize() (script size, stack items), which Dash's bitcoin#24147 backport dropped, and moving |
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
1 participant
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.
Automated Bitcoin Core v0.26 backports, batch
backport-0.26-b033-src.90ae2694b4Provenance
Each commit passed: cherry-pick (adapted by an Opus lane only where conflicts existed), build, touched tests, a mechanical diff-of-diffs check (every upstream hunk landed; no added line without an upstream counterpart), and an independent Opus verification lane where anything was adapted. Gate rows and lane artifacts are in the backportsys DB.