bip352: verify redeem script and pubkey hash in GetPubKeyFromInput #36417

In GetPubKeyFromInput(), evaluating TxoutType::SCRIPTHASH (P2SH-P2WPKH) inputs runs EvalScript under SCRIPT_VERIFY_NONE and passes the resulting redeem script to Solver(redeem, solutions). Because Solver clears the passed solutions vector, the 20-byte script hash originally extracted from the UTXO's scriptPubKey is erased without checking that Hash160(redeem) matches it. Consequently, spending an arbitrary P2SH UTXO (such as a multisig or timelock) with a scriptSig that pushes a valid P2WPKH redeem script causes an unintended public key to be extracted and summed into A_sum, silently corrupting the Silent Payments input hash and derived shared secret.

Additionally, GetPubKeyFromInput() previously accepted malleated or non-standard witness stacks and scriptSigs by grabbing stack.back() without enforcing the BIP-16 push-only requirement (!txin.scriptSig.IsPushOnly()), the BIP-141 single-element redeem script constraint (stack.size() == 1), the standard two-element P2WPKH witness stack depth (txin.scriptWitness.stack.size() == 2), or verifying that the extracted compressed public key actually hashes to the target witness program (Hash160(key) == solutions[0]).

This change preserves the expected 20-byte script hash prior to solver invocation, enforces BIP-16 push-only and single-element constraints, verifies both redeem script and witness pubkey preimage hashes, and validates exact witness stack depths for both native P2WPKH and P2SH-P2WPKH inputs. Targeted unit test coverage in src/test/bip352_tests.cpp verifies rejection of redeem script mismatches, non-push opcodes, multi-push scriptSigs, pubkey hash mismatches, uncompressed keys, and invalid stack sizes.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

External sites

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

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

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #36521 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36521.svg">} (Keep witness stacks in serialized form in CScriptWitness by sipa)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Concept ACK

Tested ACK d48b8af

bip352_tests passes, and five of the new assertions fail with master's bip352.cpp.

Every input this PR newly rejects already fails consensus: a P2SH scriptSig that is not push-only, a P2SH-P2WPKH scriptSig that is not a single push, a P2WPKH witness without exactly two items, or a redeem script or pubkey that does not match its hash. The multisig spend in the description is invalid, and the BIP-352 test vectors still pass. So this changes nothing for valid transactions: it is a robustness fix, not a fix for an exploitable bug.

The new stack.size() != 2 checks are not tested on their own. The one- and three-item witnesses in the tests are rejected on master too, because their last item is not a valid pubkey. A three-item witness ending with the matching pubkey would show the difference.

Good catch. Updated both the P2SH-P2WPKH and native P2WPKH test suites so the 3-item and 1-item witness stacks end with the valid matching compressed pubkey. On master, both cases now fail as expected since master extracts stack.back() whenever it is a valid pubkey regardless of witness depth.

re-ACK c751d92d39c02f48f363c91ac7c27b4b7937ea4c

I checked that the new witness tests cover the stack.size() != 2 checks, which my last review said were untested.

The range-diff from d48b8af changes only bip352_tests.cpp. The 3-item and 1-item witnesses now end with the matching pubkey.

On this head, bip352_tests, script_tests and transaction_tests pass. With only the two size() != 2 checks put back to empty(), exactly those four new assertions fail. With master's bip352.cpp, nine assertions fail.

Concept NACK adding this. GetPubKeyFromInput assumes inputs have been validated,. This adds pointless hashing to an operation that will be used extensively in scanning.

Fair point on confirmed blocks—for transactions already connected to the chain, VerifyScript has already verified the script and witness program hashes.

The motivation was whether GetPubKeyFromInput in src/common/ should be a self-contained parser safe for unconfirmed transactions (mempool/0-conf) or light-client contexts where VerifyScript hasn't run. In those cases, omitting the hash check allows an invalid P2SH input to inject an arbitrary public key into A_sum, corrupting the shared secret and causing the wallet to miss incoming payments.

On performance, #36475 showed GetPubKeyFromInput on P2WPKH takes ~2,757 ns (dominated by secp256k1_ec_pubkey_parse), where Hash160 is ~50 ns (<0.1% of the overall scanning loop with ecmult). But if the intended architecture is for GetPubKeyFromInput to strictly assume pre-validated inputs to keep the rescan path minimal, that's completely reasonable.

Happy to close this if that's the preferred contract, or scope the check strictly to P2SH (where SCRIPT_VERIFY_NONE is used) if unconfirmed robustness is desired.

light-client contexts

Not relevant here

unconfirmed transactions (mempool/0-conf)

Scripts are validated before TransactionAddedToMempool

I don't think this is in scope for the module

Makes sense. Closing.