BIP-352: fix P2PKH pubkey extraction from malleated scriptSig #36338

theStack commented at 7:07 PM on September 25, 2026: contributor

This PR fixes a Silent Payments pubkey extraction issue for P2PKH inputs: a third party can malleate the scriptSig such that it contains an OP_CHECKSIG that fails in real validation but succeeds with the dummy signature checker currently used (which accepts any non-empty signature). E.g. for a P2PKH spend with scriptSig

<sig> <pubkey> <bogus_sig> OP_OVER OP_CHECKSIG OP_IF <wrong_pubkey> OP_ENDIF

the EvalScript call would currently result in wrong_pubkey as the top stack item instead of pubkey and thus lead to an incorrect extraction (see the test added in the first commit). Fix this by parsing the scriptSig manually via a .GetOp(...) iteration, looking at all 33-byte data pushes to find the one where the Hash160 (SHA-256 + RIPEMD-160) matches the one in the output script.

Note that an alternative fix would be to keep using EvalScript but use an actual signature checker instead of the dummy one, but for that we need more transaction data in order to enable sighash calculation. Also involving a full script interpreter run including signature checks merely for extracting a public key seems overblown.

This issue was reported by Project Loupe earlier this week (thanks!). From what I saw, none of the existing Silent Payments implementations are affected. I'm planning to add the test case to the BIP-352 test vectors in the BIPs repository as well, next to the existing ones that cover P2PKH malleation.

DrahtBot commented at 7:07 PM on September 25, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36338
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36338.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36338#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	achow101, fametrano, Yudis-bit, sedited
Concept ACK	fjahr
Approach ACK	jonatack
Stale ACK	nymius

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36417 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36417.svg"> (bip352: verify redeem script and pubkey hash in GetPubKeyFromInput by Yudis-bit)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

SignatureHash(spk, tx, 0, SIGHASH_ALL, /*amount=*/0, SigVersion::BASE) in src/test/bip352_tests.cpp

2026-09-30 13:43:34

theStack force-pushed on Sep 25, 2026

DrahtBot added the label CI failed on Sep 25, 2026

DrahtBot commented at 7:20 PM on September 25, 2026: contributor

🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/36177728667/job/108212317321 LLM reason (✨ experimental): CI failed because IWYU reported an include issue (generated failure) in src/common/bip352.cpp.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

theStack force-pushed on Sep 25, 2026

in src/common/bip352.cpp:209 in 76464fb137

jonatack commented at 7:46 PM on September 25, 2026:

        // P2SH-P2WPKH only. BIP141 requires scriptSig to be exactly one push of the
        // redeem script; extra opcodes make the spend invalid. EvalScript +
        // DUMMY_CHECKER is therefore safe here: it only peels that push. The pubkey
        // is witness.stack.back(), which is covered by the signature.
        // Contrast P2PKH above, where scriptSig is malleable and must not be evaluated.

theStack commented at 2:46 PM on September 26, 2026:

Thanks, makes sense, added a sentence explaining why evaluating scriptSig is safe for P2SH-P2WPKH. (Shower thought: maybe we could/should even get rid of EvalScript script as well here though for consistency, and for not having to worry about what soft-fork flags and signature checker to pass. A single .GetOp call should likely be sufficient here. Potentially something to explore in a refactoring follow-up I guess.)

in src/common/bip352.cpp:191 in 76464fb137

 189 | +        //
 190 | +        // Find the preimage of the public key hash by iterating through the scriptSig data
 191 | +        // pushes and compare the Hash160 of each 33-byte push against the output script.
 192 | +        // Note that in standard P2PKH spends the pubkey would always be the last data push,
 193 | +        // but due to malleability we can't rely on that, as the scriptSig may have been
 194 | +        // modified arbitrarily, e.g. by inserting `<dummy_push> OP_DROP` at any position.

jonatack commented at 7:55 PM on September 25, 2026:

Suggested comments so a future reader does not reintroduce EvalScript on P2PKH, (or below, so they do not remove it from P2SH-P2WPKH), feel free to ignore/adapt:

        // BIP-352: "The receiver MUST parse the scriptSig for the public key, even if
        // the scriptSig does not match the template specified." Bare P2PKH scriptSigs
        // are third-party malleable (not push-only, not covered by the signature).
        //
        // Do not EvalScript the scriptSig. A dummy signature checker accepts any
        // non-empty signature, so a third party can insert an OP_CHECKSIG that fails
        // in real validation but succeeds under the dummy checker and leave a different
        // item on top of the stack (e.g. <sig> <pubkey> <bogus_sig> OP_OVER OP_CHECKSIG
        // OP_IF <wrong_pubkey> OP_ENDIF).
        //
        // The spendable key is the compressed 33-byte Hash160 preimage of the
        // scriptPubKey hash, so search data pushes for that preimage. Position is not
        // trusted: the key need not be last (e.g. <dummy> OP_DROP <sig> <pubkey>).
        // GetOp failure (truncated script) skips this input, same as "no eligible key".

theStack commented at 2:46 PM on September 26, 2026:

Added a sentence with a similar meaning. The "truncated script" GetOp failure is more a theoretical possibility than a practical one, as the pubkey extraction function is not expected to be called for transactions that are not consensus-valid. Can still add it if other reviewers feel strongly.

jonatack commented at 7:57 PM on September 25, 2026: member

Approach ACK. Concept ACK on pending BIP updates.

In the OP, RIPEMD-160 probably should be Hash160.

Optional extra test coverage: BIP352's <dummy> OP_DROP <sig> <pubkey> example, and no-preimage → nullopt.

in src/common/bip352.cpp:183 in 76464fb137 outdated

 179 | @@ -179,14 +180,29 @@ std::optional<PubKey> GetPubKeyFromInput(const CTxIn& txin, const CScript& spk)
 180 |      }
 181 |  
 182 |      if (type == TxoutType::PUBKEYHASH) {
 183 | -        std::vector<std::vector<unsigned char>> stack;
 184 | -        if (!EvalScript(stack, txin.scriptSig, SCRIPT_VERIFY_NONE, DUMMY_CHECKER, SigVersion::BASE)) {
 185 | -            return std::nullopt;
 186 | +        // For P2PKH public key extraction, BIP-352 states: "The receiver MUST parse the scriptSig

nymius commented at 8:23 PM on September 25, 2026:

In parallel to bdk-sp implementation:

Pkh => txin
    .script_sig
    .into_bytes()
    .windows(33)
    .rev() // Ignore this, it isn't needed
    .find_map(|slice| {
        bitcoin::PublicKey::from_slice(slice)
            .ok()
            .filter(|pubkey| {
                <PubkeyHash as AsRef<[u8; 20]>>::as_ref(&pubkey.pubkey_hash())
                    == script_pubkey[3..23].as_bytes()
            })
            .map(|pk| (tag, pk.inner))
    }),

theStack commented at 2:59 PM on September 26, 2026:

Nice! This is slightly different from the PR implementation, as it looks at the bare scriptSig bytes with a sliding 33-byte window (as also done in the reference implementation) rather than extracting and iterating the data pushes. Either variant should be fine, I expect that the PR implementation here could be a bit faster for pathological cases (huge scriptSigs?) though.

in src/test/bip352_tests.cpp:299 in 76464fb137 outdated

 294 | +    CMutableTransaction tx;
 295 | +    tx.vin.emplace_back(COutPoint{Txid::FromHex("0000000000000000000000000000000000000000000000000000000000000001").value(), 0});
 296 | +    std::vector<unsigned char> sig;
 297 | +    BOOST_REQUIRE(key.Sign(SignatureHash(spk, tx, 0, SIGHASH_ALL, /*amount=*/0, SigVersion::BASE), sig));
 298 | +    sig.push_back(SIGHASH_ALL);
 299 | +    const std::vector<unsigned char> bogus_sig{ParseHex("300602010102010101")}; // r=1, s=1, SIGHASH_ALL

nymius commented at 8:29 PM on September 25, 2026:

In case is useful, bdk-sp tests a malleated but valid p2pkh sig with OP_0 OP_DROP <sig> <pubkey> shape, and then a malleated but invalid p2pkh (e.g. pub key hash doesn't match), to cover all code branches.

nymius commented at 8:30 PM on September 25, 2026: none

cACK 76464fb137665fb0c7538016152ade097903e887

DrahtBot requested review from jonatack on Sep 25, 2026

theStack force-pushed on Sep 26, 2026

theStack commented at 2:45 PM on September 26, 2026: contributor

Thanks for the reviews @jonatack and @nymius, much appreciated. Adapted/extended the comments slightly and initialized the serror variable in the test, which should hopefully fix CI (interestingly, only the Windows one failed). Decided to leave further test coverage for a follow-up, the most important ones that can be reached in practice should be covered by the newly introduced one and the existing ones in the BIP-352 test vectors.

DrahtBot removed the label CI failed on Sep 26, 2026

in src/common/bip352.cpp:203 in 09d063c563

 201 | +            if (!txin.scriptSig.GetOp(pc, opcode, pubkey_candidate)) {
 202 | +                return std::nullopt;
 203 | +            }
 204 | +            if (pubkey_candidate.size() == 33 && Hash160(pubkey_candidate) == pubkey_hash) {
 205 | +                CPubKey key{pubkey_candidate};
 206 | +                if (key.IsCompressed() && key.IsFullyValid()) return PubKey{key};

fjahr commented at 10:01 PM on September 26, 2026:

nit: I guess if IsFullyValid returns false there is no point in continuing the loop, but it's probably fine to keep the code simple in this case. I think this could get rid of IsCompressed though since the size is already checked above and then IsFullyValid does the rest.

theStack commented at 1:51 PM on September 30, 2026:

Good point, dropped the redundant IsCompressed check. Decided to keep continuing the loop if IsFullyValid is false to keep the code simple as you pointed out (AFAIU in theory there is only a point in continuing if we assume there could be a second Hash160 preimage, which shouldn't be possible in practice).

fjahr commented at 10:01 PM on September 26, 2026: contributor

Concept ACK

test: characterize buggy BIP-352 P2PKH extraction on malleated scriptSig

Add a unit test with a (third-party) malleated P2PKH scriptSig that
contains an OP_CHECKSIG with a bogus signature, followed by a
conditional push of a wrong pubkey. In real validation the OP_CHECKSIG
fails and the spend remains consensus-valid (which is verified in the
test as well), but since the scriptSig is evaluated with a dummy
signature checker that accepts any non-empty signature, the wrong
pubkey is extracted. Record this current (buggy) behavior; the test is
updated with the fix in the next commit.

c35b992514

BIP-352: fix P2PKH pubkey extraction from malleated scriptSig

The P2PKH pubkey was extracted by evaluating the scriptSig with
EvalScript and DUMMY_CHECKER, and taking the top stack element. As the
dummy checker accepts any non-empty signature, a third party can
malleate the scriptSig such that it contains an OP_CHECKSIG that fails
in real validation but succeeds with the dummy checker, resulting in a
different final stack and thus a wrong pubkey being extracted (see the
test added in the previous commit).

Fix this by not evaluating the scriptSig at all, and instead searching
its data pushes for the preimage of the pubkey hash in the
scriptPubKey, following BIP-352: "The receiver MUST parse the scriptSig
for the public key, even if the scriptSig does not match the template
specified."

Co-authored-by: Jon Atack <jon@atack.com>

cdfaa534d0

theStack force-pushed on Sep 30, 2026

theStack commented at 1:51 PM on September 30, 2026: contributor

Tackled review comment #36338 (review) and rebased on master (since there was at least one CI issue fixed within the last few days). Thanks!

DrahtBot added the label CI failed on Sep 30, 2026

achow101 commented at 5:36 PM on September 30, 2026: member

ACK cdfaa534d006f0effeb0ece3fb9aa5e53177a3fd

DrahtBot requested review from nymius on Sep 30, 2026

DrahtBot requested review from fjahr on Sep 30, 2026

fametrano commented at 9:28 PM on October 1, 2026: contributor

tACK cdfaa534d006f0effeb0ece3fb9aa5e53177a3fd

I ran 19 P2PKH scriptSigs (16 consensus-valid) through this PR, master and the BIP352 reference.py: the new test's scriptSig, other OP_CHECKSIG tricks, non-minimal pushes, extra 33-byte pushes before and after the key, and uncompressed spends. On all 16 valid ones the PR agrees with reference.py. Master gets three of them wrong: the new test's case, another OP_CHECKSIG trick where it finds no key, and an uncompressed spend it counts instead of skipping. The new test fails on master.

For the follow-up: the tests still pass with the Hash160 check removed, since no test has another valid 33-byte key before the spender's. jonatack's suggested <dummy> OP_DROP <sig> <pubkey> test would catch that if the dummy is a valid compressed key other than the spender's.

fametrano commented at 7:48 AM on October 2, 2026: contributor

@theStack I opened bitcoin/bips#2323 with BIP-352 test vectors for this case. They add your test's scriptSig and the <dummy> OP_DROP <sig> <pubkey> case jonatack suggested, with a valid pubkey as the dummy.

With the new file vendored on your branch (cdfaa534), both new vectors pass. On Core master, the first one fails.

If you prefer to take the commits into your own work, you are welcome to.

DrahtBot removed the label CI failed on Oct 5, 2026

sedited removed review request from nymius on Oct 6, 2026

sedited requested review from Eunovo on Oct 6, 2026

Yudis-bit commented at 2:58 PM on October 6, 2026: none

Tested ACK cdfaa534d006f0effeb0ece3fb9aa5e53177a3fd

Tested against several scriptSig malleation patterns including <dummy> OP_DROP <sig> <pubkey>, duplicate 33-byte pushes before and after the actual signer key, and uncompressed spends. Scanning correctly recovers the expected public key on valid preimages and returns std::nullopt on uncompressed keys and bogus pushes, resolving the divergence between DUMMY_CHECKER stack evaluation and consensus validation.

DrahtBot requested review from nymius on Oct 6, 2026

sedited approved

sedited commented at 9:21 AM on October 7, 2026: contributor

ACK cdfaa534d006f0effeb0ece3fb9aa5e53177a3fd

sedited merged this on Oct 7, 2026

sedited closed this on Oct 7, 2026

theStack deleted the branch on Oct 7, 2026