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.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36338.

Reviews

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

Type	Reviewers
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.

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. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/36177728667/job/108212317321</sub> <sub>LLM reason (✨ experimental): CI failed because IWYU reported an include issue (generated failure) in src/common/bip352.cpp.</sub>

<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

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.

34dee4231b

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>

09d063c563

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.

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

Concept ACK