validation: detect witness stripping without re-running Script checks #33105

pull darosior wants to merge 3 commits into bitcoin:master from darosior:2507_early_witstrip_detection changing 5 files +211 −7
  1. darosior commented at 8:13 PM on July 30, 2025: member

    Since it was introduced in 4eb515574e1012bc8ea5dafc3042dcdf4c766f26 (#18044), the detection of a stripped witness relies on running the Script checks 3 times. In the worst case, this consists in running Script validation for every single input 3 times.

    Detection of a stripped witness is necessary because in this case wtxid==txid, and the transaction's wtxid must not be added to the reject filter or it could allow a malicious peer to interfere with txid-based orphan resolution as used in 1p1c package relay.

    However it is not necessary to run Script validation to detect a stripped witness (much less so doing it 3 times in a row). There are 3 types of witness program: defined program types (Taproot, P2WPKH and P2WSH), undefined types, and the Pay-to-anchor carve-out.

    For defined program types, Script validation with an empty witness will always fail (by consensus). For undefined program types, Script validation is always going to fail regardless of the witness (by standardness). For P2A, an empty witness is never going to lead to a failure.

    Therefore it holds that we can always detect a stripped witness without re-running Script validation. However this might lead to more "false positives" (cases where we return witness stripping for an otherwise invalid transaction) than the existing implementation. For instance a transaction with one P2PKH input with an invalid signature and one P2WPKH input with its witness stripped. The existing implementation would treat it as consensus invalid while the implementation in this PR would always consider it witness stripped.

    h/t AJ: this essentially implements a variant of #33066 (comment).

  2. DrahtBot added the label Validation on Jul 30, 2025
  3. DrahtBot commented at 8:13 PM on July 30, 2025: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK sipa, Crypt-iQ, glozow
    Approach ACK ajtowns
    Stale ACK achow101, nervana21

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32317 (kernel: Separate UTXO set access from validation functions by TheCharlatan)
    • #31682 ([IBD] specialize CheckBlock's input & coinbase checks by l0rinc)
    • #28676 (Cluster mempool implementation by sdaftuar)

    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-->

  4. in src/validation.cpp:909 in 0e22a1401c outdated 
     906 | -        return state.Invalid(TxValidationResult::TX_WITNESS_MUTATED, "bad-witness-nonstandard");
 907 | +    if (tx.HasWitness()) {
 908 | +        if (m_pool.m_opts.require_standard && !IsWitnessStandard(tx, m_view)) {
 909 | +            return state.Invalid(TxValidationResult::TX_WITNESS_MUTATED, "bad-witness-nonstandard");
 910 | +        }
 911 | +    } else {
    ajtowns commented at 1:13 AM on July 31, 2025:

    Shouldn't this check also be gated by m_opts.require_standard ? I guess that could be done in #29843 presuming this is merged first.

    darosior commented at 1:19 PM on July 31, 2025:

    I considered it and decided against because 1) whether to add the txid to the reject filter seems unrelated to standardness (and surprising that -acceptnonstdtxn=1 would now break 1p1c in the adversarial case) and 2) a failure here means the witness is invalid by consensus, #29843 would already not allow it today on master.

  5. in src/validation.cpp:919 in 0e22a1401c outdated 
     916 | +        // more details see this brief historical summary: https://github.com/bitcoin/bitcoin/pull/32379#issuecomment-2985252920.
 917 | +        int version;
 918 | +        std::vector<uint8_t> program;
 919 | +        for (const auto& txin: tx.vin) {
 920 | +            const auto& prev_spk{m_view.AccessCoin(txin.prevout).out.scriptPubKey};
 921 | +            if (prev_spk.IsWitnessProgram(version, program) && !prev_spk.IsPayToAnchor(version, program)) {
    ajtowns commented at 1:17 AM on July 31, 2025:

    Maybe put this logic in policy.h and call if (RequiresNonEmptyWitness(prev_spk)) or if (RequiresNonEmptyWitness(tx))?

    darosior commented at 1:45 PM on July 31, 2025:

    I think this would be a bit confusing because it would need to return true for undefined program types, whereas we don't know yet if this type will require a witness or not (presumably yes, but hey). I also think it's brief enough to not warrant its own function. Happy to change it, but it seems cleaner this way.

    ajtowns commented at 6:20 AM on August 1, 2025:

    I think that's fine -- if require_standard is true, then undefined program types will have already given an INPUTS_NOT_STANDARD response; if it's not true and #29843 has been merged, then we'll accept any witness including a null witness for undefined program types, so RequiresNonEmptyWitness should return false for them. That can change to true when they're defined in a soft-fork, but that's soft-fork behaviour, so should be fine.

    But worrying about it in 29843 rather than here seems best.

    nervana21 commented at 8:34 AM on August 1, 2025:

    I followed ajtowns suggestion and moved the logic to policy.h as defined in the patch below. Instead of RequiresNonEmptyWitness, the function is named IsNonAnchorWitnessProgram. This way, returning true for undefined program types that are non-P2A witness programs still makes sense.

    <details> <summary>Expand patch</summary>

    diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
index 48f2a6a744..2b03a404a9 100644
--- a/src/policy/policy.cpp
+++ b/src/policy/policy.cpp
@@ -96,6 +96,14 @@ bool IsStandard(const CScript& scriptPubKey, TxoutType& whichType)
     return true;
 }
 
+bool IsNonAnchorWitnessProgram(const CScript& scriptPubKey)
+{
+    int version;
+    std::vector<uint8_t> program;
+    return scriptPubKey.IsWitnessProgram(version, program) &&
+           !scriptPubKey.IsPayToAnchor(version, program);
+}
+
 bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_datacarrier_bytes, bool permit_bare_multisig, const CFeeRate& dust_relay_fee, std::string& reason)
 {
     if (tx.version > TX_MAX_STANDARD_VERSION || tx.version < 1) {
diff --git a/src/policy/policy.h b/src/policy/policy.h
index f9a18561bc..f3982bfa9f 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -139,6 +139,23 @@ bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFee);
 
 bool IsStandard(const CScript& scriptPubKey, TxoutType& whichType);
 
+/**
+ * Check if the script is a witness program that is not P2A (Pay-to-Anchor).
+ *
+ * All standard Segwit programs but P2A require a non-empty witness.
+ * Detect if it is spending a non-P2A Segwit program with no witness.
+ * We specifically detect the case of stripped witness because in this
+ * case wtxid==txid and adding the wtxid to the reject filter upon Script
+ * validation failure could allow a malicious peer to interfere with orphan
+ * parent resolution (which is necessarily done by txid) as used in 1p1c
+ * package relay. For more details see this brief historical summary:
+ * [#32379 (comment)](/bitcoin-bitcoin/32379/#issuecomment-2985252920).
+ *
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] scriptPubKey The script to check.
+ * [@return](/bitcoin-bitcoin/contributor/return/) True if the script is a witness program that is not P2A.
+ */
+bool IsNonAnchorWitnessProgram(const CScript& scriptPubKey);
+
 /** Get the vout index numbers of all dust outputs */
 std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate);
 
diff --git a/src/validation.cpp b/src/validation.cpp
index ac4eac45e9..9813f1553e 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -901,22 +901,16 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
         return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs");
     }
 
-    // Check for non-standard witnesses.
+    // Check for witness-related policy violations.
     if (tx.HasWitness()) {
         if (m_pool.m_opts.require_standard && !IsWitnessStandard(tx, m_view)) {
             return state.Invalid(TxValidationResult::TX_WITNESS_MUTATED, "bad-witness-nonstandard");
         }
     } else {
-        // All standard Segwit programs but P2A require a non-empty witness. Detect if it is spending a non-P2A Segwit
-        // program with no witness. We specifically detect the case of stripped witness because in this case wtxid==txid
-        // and adding the wtxid to the reject filter upon Script validation failure could allow a malicious peer to
-        // interfere with orphan parent resolution (which is necessarily done by txid) as used in 1p1c package relay. For
-        // more details see this brief historical summary: [#32379 (comment)](/bitcoin-bitcoin/32379/#issuecomment-2985252920).
-        int version;
-        std::vector<uint8_t> program;
+        // Check if any input is spending a witness output without providing a witness
         for (const auto& txin: tx.vin) {
             const auto& prev_spk{m_view.AccessCoin(txin.prevout).out.scriptPubKey};
-            if (prev_spk.IsWitnessProgram(version, program) && !prev_spk.IsPayToAnchor(version, program)) {
+            if (IsNonAnchorWitnessProgram(prev_spk)) {
                 return state.Invalid(TxValidationResult::TX_WITNESS_STRIPPED, "bad-witness-stripped");
             }
         }

    </details>

    Please feel free to use if helpful — or disregard if not :)

    darosior commented at 6:23 PM on August 4, 2025:

    The latest version now uses a SpendsNonAnchorWitnessProg helper in policy.h.

  6. ajtowns commented at 1:23 AM on July 31, 2025: contributor

    Approach ACK

  7. in src/validation.cpp:902 in 0e22a1401c outdated 
     901 | @@ -902,8 +902,24 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
 902 |      }
    darosior commented at 1:46 PM on July 31, 2025:

    self nit: if i retouch, 0e22a1401c7edee8946f404dd0deb59a94231340's message is missing a word at the end of the second sentence.

    darosior commented at 6:23 PM on August 4, 2025:

    Done.

  8. in src/validation.cpp:904 in 0e22a1401c outdated 
     901 | @@ -902,8 +902,24 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
 902 |      }
 903 |  
 904 |      // Check for non-standard witnesses.
    nervana21 commented at 2:37 PM on July 31, 2025:

    Small nit. This comment could be updated now that we're checking for both nonstandard and stripped witnesses

        // Check for witness-related policy violations.
    darosior commented at 6:23 PM on August 4, 2025:

    Taken.

  9. achow101 commented at 9:39 PM on July 31, 2025: member

    ACK 0e22a1401c7edee8946f404dd0deb59a94231340

  10. DrahtBot requested review from ajtowns on Jul 31, 2025
  11. nervana21 commented at 8:46 AM on August 1, 2025: contributor

    tACK 0e22a14

    After understanding the larger context of this PR, I thoroughly reviewed all code. I also ran the updated tests locally and modified them such that they all passed and failed in the expected manner.

  12. in src/validation.cpp:914 in 0e22a1401c outdated 
     911 | +    } else {
 912 | +        // All standard Segwit programs but P2A require a non-empty witness. Detect if it is spending a non-P2A Segwit
 913 | +        // program with no witness. We specifically detect the case of stripped witness because in this case wtxid==txid
 914 | +        // and adding the wtxid to the reject filter upon Script validation failure could allow a malicious peer to
 915 | +        // interfere with orphan parent resolution (which is necessarily done by txid) as used in 1p1c package relay. For
 916 | +        // more details see this brief historical summary: https://github.com/bitcoin/bitcoin/pull/32379#issuecomment-2985252920.
    glozow commented at 6:17 PM on August 1, 2025:

    I don't think this comment belongs here. It has too many details about net_processing logic that shouldn't really be mentioned in validation / are already covered in the code that does rejection caching.

            // Detect a missing witness so that p2p code can handle rejection caching appropriately.
    darosior commented at 5:28 PM on August 4, 2025:

    Done.

    I somewhat disagree that this is already covered though, the rationale given in txdownloadman refers to an outdated reason (wtxid relay) rather than the contemporary reason for WITNESS_STRIPPED's existence (1p1c through orphan resolution). But improvements to existing code documentation can be done separately.

    glozow commented at 7:47 PM on August 4, 2025:

    We still support txid relay, so I don't think code implementing it is outdated.

    darosior commented at 7:55 PM on August 4, 2025:

    I am not saying code implementing txid relay is outdated, that seems unrelated.

  13. glozow commented at 6:37 PM on August 1, 2025: member

    approach ACK - I'm not super confident in my script knowledge but I can't think of why you couldn't detect witness stripped this way.

  14. Crypt-iQ commented at 2:03 AM on August 4, 2025: contributor

    I'm a little out of my depth and it is getting late so might be off-base, but reviewing this PR I think it's a slight behavior change in what we see as witness-stripped? P2SH-P2WSH are not caught by IsWitnessProgram in this PR, but they are caught as witness-stripped in master. I checked by running the test_p2sh_witness subtest in p2p_segwit.py and printing if witness-stripped was detected. I didn't test P2SH-P2WPKH. Not sure if it matters, figured I'd point it out.

  15. darosior commented at 4:42 AM on August 4, 2025: member

    Good catch.

  16. qa: test witness stripping in p2p_segwit
    A stripped witness is detected as a special case in mempool acceptance to make sure we do not add
the wtxid (which is =txid since witness is stripped) to the reject filter. This is because it may
interfere with 1p1c parent relay which currently uses orphan reconciliation (and originally it was
until wtxid-relay was widely adopted on the network.

This commit adds a test for this special case in the p2p_segwit function test, both when spending
a native segwit output and when spending a p2sh-wrapped segwit output.

Thanks to Eugene Siegel for pointing out the p2sh-wrapped detection did not have test coverage by
finding a bug in a related patch of mine.
    eb073209db
  17. darosior force-pushed on Aug 4, 2025
  18. darosior commented at 6:23 PM on August 4, 2025: member

    I fixed the bug @Crypt-iQ found (thanks!) and added a functional test that explicitly tests for witness-stripping detection for both native and p2sh-wrapped outputs. This test would have failed without the fix. In addition, i made the segwit-spend detection into its own helper function for which i added a unit test to sanity check detection against all defined output types.

  19. darosior force-pushed on Aug 4, 2025
  20. in test/functional/p2p_segwit.py:696 in eb073209db outdated 
     692 | @@ -693,6 +693,12 @@ def test_p2sh_witness(self):
 693 |                  expected_msgs=[spend_tx.txid_hex, 'was not accepted: mandatory-script-verify-flag-failed (Witness program was passed an empty witness)']):
 694 |              test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False)
 695 |  
 696 | +        # The transaction was detected as witness stripped above and not added to the reject
    glozow commented at 6:41 PM on August 4, 2025:

    Transactions received through RPC aren't checked against the reject filter, so I think this comment can be confusing

    darosior commented at 7:15 PM on August 4, 2025:

    How is this related to RPC?

    glozow commented at 7:19 PM on August 4, 2025:

    Oh woops, I thought these were being submitted through RPC because of the err strings. Nevermind.

  21. in src/policy/policy.cpp:364 in 9230de64dc outdated 
     359 | +        if (prev_spk.IsPayToScriptHash()) {
 360 | +            // If EvalScript fails or results in an empty stack, the transaction is invalid by consensus.
 361 | +            std::vector <std::vector<uint8_t>> stack;
 362 | +            if (!EvalScript(stack, txin.scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker{}, SigVersion::BASE)
 363 | +                || stack.empty()) {
 364 | +                return false;
    glozow commented at 6:46 PM on August 4, 2025:

    Is it right to return false early? You need to check all inputs, no?

    glozow commented at 6:53 PM on August 4, 2025:

    It might be a good idea to have the unit tests cover transactions with multiple input types

    darosior commented at 7:19 PM on August 4, 2025:

    Is it right to return false early? You need to check all inputs, no?

    It is intentional to conserve behaviour. Currently we check scripts sequentially from first input to last. If input N is consensus-invalid and input N+1 is witness stripped we will return a consensus-invalid error. In my opinion it would be better to continue here but i think it's better to not change behaviour in this PR.

    It might be a good idea to have the unit tests cover transactions with multiple input types

    I can add a case which illustrates what i just described above.

    glozow commented at 7:44 PM on August 4, 2025:

    Oh I see. I would prefer returning witness-stripped in that scenario. It feels like this function returns a not-quite-accurate result in order to accommodate a different function that must run after it to catch a different error... a bit brittle.

    ajtowns commented at 12:43 AM on August 5, 2025:

    Shouldn't this mimic VerifyScript more closely? ie:

    • check the scriptSig is push-only
    • check the redeemScript matches the hash in the p2sh prev_spk
    • check the scriptSig doesn't push other things on the stack (ie stack.size() == 1)

    All those are permanent errors for the txid (fixing them requires changing the scriptSig which changes the txid), so the rejection would be valid to cache.

    Currently we check scripts sequentially from first input to last. If input N is consensus-invalid and input N+1 is witness stripped we will return a consensus-invalid error.

    I don't think that's the right idea? We call:

    1. CheckInputScripts(tx, scriptVerifyFlags)
    2. if that fails, and we have no witness, we check CheckInputScripts(tx, scriptVerifyFlags - WITNESS,CLEANSTACK) succeeds
    3. if it does, we check `CheckInputScripts(tx, scriptVerifyFlags - CLEANSTACK) fails
    4. if so, we return WITNESS_STRIPPED

    But each of these consider every input; so consensus failures outside of witness validation will fail at step 2, so we'll return the error from step 1.

    Because this PR returns witness stripped results earlier, we should expect to get different results than the current code; eg, for a tx without witness data, where the first input is a p2wpkh spend and the second input is a p2sh script with a bad redeemScript, currently I think we'll see a script-verification-error due to the first input, while after this patch I'd expect to see a witness stripped error.

    darosior commented at 1:43 PM on August 5, 2025:

    I would prefer returning witness-stripped in that scenario.

    Sure, ok. That's my preference as well, so if people agree i'll do that then. EDIT: actually this cannot happen as it would have failed first in AreInputsStandard (and then in IsWitnessStandard). So i think it's fine to just use continue and that won't be a change in behaviour.

    Shouldn't this mimic VerifyScript more closely?

    Pushonly is already checked before in IsStandardTx(). Checking the redeemscript hash and clean stack make sense. But so would checking every other standardness / consensus rule first. In fact i think your point hints that we should instead check for this after every other check.

    What do you (both) think of changing the call site for SpendsNonAnchorWitnessProg to after encountering a Script failure (i.e. where it is already done today through more script checks)? This would make sure we don't weaken any existing standardness or consensus error into a witness-stripped one.

    I don't think that's the right idea?

    I don't see how what you describe differs from what i said.

    Because this PR returns witness stripped results earlier, we should expect to get different results than the current code

    Yeah. I think it's better to go with the minimal amount of change for now and move the Segwit-spend detection to after we encounter a Script failure, i.e. where it is today.

    darosior commented at 3:03 PM on August 5, 2025:

    Changing the call site to where we currently perform detection in master is not going to avoid all behaviour changes. Short of checking all consensus rules for all other inputs preceding the one that failed the check, we can't guarantee that we won't return a false-positive witness-stripped error. For instance if we receive a transaction with two inputs, the first a legacy P2PKH, the second a Segwit P2WPKH, then if the transaction comes in without witness but also fails the signature check for the P2PKH input we would currently (in master) return the consensus error and this PR would (in any reasonable shape) return a witness-stripped error. If we don't want behaviour changes here we should do #33066 instead.

    darosior commented at 7:30 PM on August 5, 2025:

    I updated the return false to continue because it makes more sense and is actually not a change in behaviour (see edit in #33105 (review)).

    I did not add the check to return false if redeem script mismatches, or EvalScript doesn't result in a clean stack. This is because those would be confusing to have in a function checking if inputs are Segwit while only partially reducing the change in behaviour (see #33105 (review)). Because we won't be able to detect all consensus errors in other inputs, i don't think we should add detection for only a few of those in a seemingly-unrelated helper function.

    ajtowns commented at 7:31 PM on August 5, 2025:

    The logic when we see "WITNESS_STRIPPED" is "this peer sent us garbage, that we have to ignore" -- to me, it's better to spend as little CPU as possible on garbage, so that means we should detect WITNESS_STRIPPED as soon and as cheaply as possible, then just ignore the tx. If we could have drawn some useful conclusions from the garbage (eg "INPUTS_NOT_STANDARD" or any other policy/consensus failure that different witness data couldn't fix), that's fine, but probably not at the cost of doing much extra processing -- caching the error will only help if we had another misbehaving peer wanting to send the same invalid tx.

    In fact i think your point hints that we should instead check for this after every other check.

    So I think my point is the opposite -- we should check for witness stripped early, and not bother very much with doing the other checks even if they're easy. Just identify that the node's giving us garbage asap and move on.

    Crypt-iQ commented at 1:47 PM on August 6, 2025:

    So I think my point is the opposite -- we should check for witness stripped early, and not bother very much with doing the other checks even if they're easy.

    I agree.

    I double-checked and the current code does not change behavior.

    The early check in an earlier iteration of this PR changed behavior in two ways:

    • consensus-invalid for non-witness reasons could be converted to witness-stripped, but this does not matter as they are consensus-invalid and a malicious peer can't interfere with honest relay.
    • a tx that is TX_RECONSIDERABLE that a peer relayed without the witness was reported as TX_RECONSIDERABLE. With the early check, the tx is instead (correctly imo) reported as WITNESS_STRIPPED. I also think this does not matter in practice. See comment.
    darosior commented at 9:07 PM on August 6, 2025:

    If we can cheaply detect this txid refers to an always-invalid transaction and cache it, that seems like a win. It might be fine to cache even less such errors, but i'd rather this was done separately. There is already enough happening in this PR 14 days from the feature freeze.

    I double-checked and the current code does not change behavior.

    It does, as far as i can tell. An example is given in OP / last commit's message. "For instance a transaction with one P2PKH input with an invalid signature and one P2WPKH input with its witness stripped. The existing implementation would treat it as witness stripped if the P2WPKH input appears first, and consensus invalid if it appears second. The implementation in this commit would always consider it witness stripped."

    That's why i say above there is already enough happening in this PR, i'd like to minimize the number of non-obvious behaviour changes.

    Crypt-iQ commented at 1:44 PM on August 7, 2025:

    It does, as far as i can tell.

    Oh ok, that was not obvious to me. I think the OP / last commit message needs to be modified: "The existing implementation would treat it as witness stripped if the P2WPKH input appears first, and consensus invalid if it appears second." is not true.

    I tested this in master and both orders of inputs give consensus invalid, this PR gives witness-stripped in both orders. If the P2WPKH input appears first, the first CheckInputScripts will fail with a consensus error since SCRIPT_VERIFY_WITNESS is on. Then, the second CheckInputScripts will fail due to the invalid signature for the P2PKH input. This will skip the third CheckInputScripts and return the consensus invalid error from the first CheckInputScripts.

    darosior commented at 2:01 PM on August 7, 2025:

    I tested this in master and both orders of inputs give consensus invalid

    Ah, yes, right! Thank you for testing, will modify the commit message / OP.

  22. in src/policy/policy.cpp:366 in 9230de64dc outdated 
     361 | +            std::vector <std::vector<uint8_t>> stack;
 362 | +            if (!EvalScript(stack, txin.scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker{}, SigVersion::BASE)
 363 | +                || stack.empty()) {
 364 | +                return false;
 365 | +            }
 366 | +            // P2SH-wrapped Taproot programs are invalid by consensus. P2SH-wrapped Anchor program is non-standard.
    Crypt-iQ commented at 10:57 PM on August 4, 2025:

    I think P2SH-wrapped taproot programs are non-standard according to BIP341 text (says they remain unencumbered) and here?

    darosior commented at 2:56 PM on August 5, 2025:

    Right, i should have double checked. Fixed in upcoming push, thanks.

  23. luke-jr commented at 1:40 AM on August 5, 2025: member

    Why would this case ever occur? Seems like a lot of code to handle an unexpected path. And would-be DoS attackers can defeat it with a dummy witness anyway, right?

  24. darosior commented at 1:55 PM on August 5, 2025: member

    Why would this case ever occur?

    Most likely never because we have the detection. If we didn't, someone may try to exploit this case to hinder propagation of 1p1c packages (see OP).

    Seems like a lot of code to handle an unexpected path.

    Yeah. Note however this is replacing expensive code. I think it's always fair to question adding more code, but i hope in this instance it is pretty clear that replacing the expensive code by an inexpensive, albeit slightly more verbose, one is worth it. Note also 80% of code added in this PR is tests, some for currently-untested behaviour.

    And would-be DoS attackers can defeat it with a dummy witness anyway, right?

    I think you are confusing the witness-stripping detection, which we do for "censorship resistance", with the rationale for this PR which is "DoS resistance" (by making said detection a lot cheaper).

  25. darosior force-pushed on Aug 5, 2025
  26. darosior commented at 7:29 PM on August 5, 2025: member

    I changed the call site for SpendsNonAnchorWitnessProg to be where we detect witness stripping today. Besides being a smaller diff this minimizes the potential for behaviour changes. Note this PR still slightly changes behaviour (see last commit's message and #33105 (review)).

  27. darosior renamed this:
    validation: detect witness stripping early on
    validation: detect witness stripping without re-running Script checks
    on Aug 5, 2025
  28. Crypt-iQ commented at 9:34 PM on August 5, 2025: contributor

    Replying out of thread to not derail the thread conversation.

    This would make sure we don't weaken any existing standardness or consensus error into a witness-stripped one.

    In current master, these are the consensus errors (I think?) that can be returned as witness-stripped:

    • SCRIPT_ERR_WITNESS_MALLEATED (changes txid)
    • SCRIPT_ERR_WITNESS_MALLEATED_P2SH (changes txid)
    • SCRIPT_ERR_WITNESS_PROGRAM_MISMATCH (for P2WPKH, not the case for P2WSH)
    • SCRIPT_ERR_WITNESS_PROGRAM_WRONG_LENGTH

    The other errors require a witness and would fail the CheckInputScripts logic early. I might be misunderstanding the comment / being a bit pedantic and maybe you're referring to not adding even more errors that can be converted into witness-stripped?

  29. sipa commented at 6:02 PM on August 6, 2025: member

    Concept ACK

  30. darosior commented at 9:00 PM on August 6, 2025: member

    maybe you're referring to not adding even more errors that can be converted into witness-stripped?

    Yes this is what i meant. There are cases today where we would return WITNESS_STRIPPED for an otherwise invalid transaction (such as it could only be made valid by changing its txid, and we are missing the opportunity of adding its txid to the reject filter). This PR adds more such cases. This is minimized in the latest iteration (https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3156334619) by leaving the witness detection be at the same site as today, since an early witness-stripped detection may overlook even more ways in which the transaction is invalid (for instance too many sigops).

  31. in src/test/transaction_tests.cpp:1214 in 38727a2900 outdated 
    1209 | +    tx_create.vout[0].scriptPubKey = GetScriptForDestination(WitnessV0KeyHash{pubkey});
1210 | +    tx_spend.vin[0].prevout.hash = tx_create.GetHash();
1211 | +    AddCoins(coins, CTransaction{tx_create}, 0, false);
1212 | +    BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
1213 | +
1214 | +    // P2SH-wrapped P2WSH
    Crypt-iQ commented at 2:52 PM on August 7, 2025:

    should be "P2SH-wrapped P2WPKH"

    darosior commented at 8:24 PM on August 7, 2025:

    Thanks, done.

  32. Crypt-iQ commented at 3:00 PM on August 7, 2025: contributor

    ACK 370770fb361fdb745cfa7bb9abc7ee4a7833b83b modulo commit message change and test comment fix

  33. DrahtBot requested review from nervana21 on Aug 7, 2025
  34. DrahtBot requested review from glozow on Aug 7, 2025
  35. DrahtBot requested review from achow101 on Aug 7, 2025
  36. DrahtBot requested review from sipa on Aug 7, 2025
  37. darosior force-pushed on Aug 7, 2025
  38. darosior commented at 8:26 PM on August 7, 2025: member

    Corrected commit message and OP as per @Crypt-iQ's point.

  39. in src/policy/policy.cpp:367 in 714c9160cd outdated 
     362 | +            if (!EvalScript(stack, txin.scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker{}, SigVersion::BASE)
 363 | +                || stack.empty()) {
 364 | +                continue;
 365 | +            }
 366 | +            const CScript redeem_script{stack.back().begin(), stack.back().end()};
 367 | +            if (redeem_script.IsWitnessProgram(version, program) && !redeem_script.IsPayToAnchor()) {
    sipa commented at 9:18 PM on August 7, 2025:

    I'm not convinced by the inclusion of the && !redeem_script.IsPayToAnchor() part inside the P2SH check here.

    P2SH-P2A is not a thing, as far as our codebase is concerned, just like P2SH-P2TR is not a thing. Highly unlikely of course, but P2SH-WITV1 could feasibly be assigned a consensus meaning in the future, which could require a witness. The code treating it as a P2A is incorrectly "guessing" that it will never need a witness to spend, and thus if we see it in a witness-stripped transaction, we can insert the failure in the txid cache.

    darosior commented at 9:28 PM on August 7, 2025:

    Right. Fixed, thanks.

  40. darosior force-pushed on Aug 7, 2025
  41. sipa commented at 10:35 PM on August 7, 2025: member

    ACK ffd82f3783d1af4f94f7dbd0af3a80d4591eb185

    While discussing possible alternatives to this with @darosior and @glozow, I feel I came to an understanding about the problem I didn't have before.

    Consider an approach where there are completely separate txid and wtxid rejection caches. The txid cache is used when one has a txid, the wtxid cache is used when one has a wtxid. Ideally then, we insert failed transactions:

    • Always into the wtxid cache (even when TX_WITNESS_STRIPPED, because it won't be used for txid lookups)
    • Into the txid cache whenever the failure is known to be independent of the witness. There are many potential reasons we can detect as such:
      • a. Script validation errors that occur before witness validation.
      • b. Base-weight variations of resource checks (e.g. base_size above 100000, fee/base_size is below mempool feerate, ...)
      • c. Miscellaneous non-witness related transaction failures (e.g. non-standard input/output script types, too large scriptSigs, dust, ...)

    We can see the current code as an approximation of this:

    • All failures are inserted into the wtxid cache except TX_WITNESS_STRIPPED.
    • Into the txid cache we insert:
      • TX_NONSTANDARD_INPUTS failures (one specific instance of (c) above).
      • All failed non-segwit transactions except TX_WITNESS_STRIPPED, which covers a subset of (a), (b), and (c), implicitly because txid=wtxid here and the tables are shared.

    Relying on txid=wtxid for non-segwit transactions kind of does the right thing, though it feels accidental. For example, a non-witness transaction failing due to low feerate is inserted into the txid cache (by virtue of its wtxid=txid), which is correct, and a special case of (b). However, it has one case where it blatantly does the wrong thing, which is exactly what TX_WITNESS_STRIPPED fixes by special-casing it and not inserting it at all.

    So I think there are two follow-ups possible after this:

    • Get rid of the txid rejection cache (i.e., #33066).
    • Make the txid rejection cache work correctly (i.e., split the caches into txid/wtxid, and annotate each failure with whether it's witness-dependent or witness-independent).

    In both cases, the TX_WITNESS_STRIPPED can go away. That means the SpendsNonAnchorWitnessProg() added here can go away too, but it could also be transformed into an independent optimization. We could choose to detect (very) early whether a transaction has any input type that necessarily needs a witness (i.e., P2WPKH, P2WSH, P2SH-P2WPKH, P2SH-P2WSH, P2TR) but lacks one. If so, transaction validation can instantly fail (with witness-dependent failure). Unlike now, where the location of calling SpendsNonAnchorWitnessProg() is a trade-off between fast failure and cache accuracy, it would be unambiguously better to do the check earlier (or not at all).

  42. DrahtBot requested review from Crypt-iQ on Aug 7, 2025
  43. ajtowns commented at 2:09 PM on August 8, 2025: contributor

    So I think there are two follow-ups possible after this:

    • ..
    • Make the txid rejection cache work correctly (i.e., split the caches into txid/wtxid, and annotate each failure with whether it's witness-dependent or witness-independent).

    In both cases, the TX_WITNESS_STRIPPED can go away.

    I don't think that's straightforwardly true: if we have separate caches working correctly, we still should not be adding failures that may be solely due to witness stripping to the txid cache, as that would (allow an attacker to) prevent us from doing orphan resolution/1p1c from our wtxidrelay peers, unless we also ignore the txid cache in that case, but that was the other follow-up.

  44. sipa commented at 2:15 PM on August 8, 2025: member

    I don't think that's straightforwardly true: if we have separate caches working correctly, we still should not be adding failures that may be solely due to witness stripping to the txid cache

    Of course! But that is captured by annotating each failure with whether it's witness-dependent or witness-independent. Any script failure that happens after witness validation begins would be witness-dependent, and therefore not eligible to enter the txid rejection cache.

    The difference is that this is just directly assessing what failure should go where, rather than the current patchwork of essentially inserting (([all txid=wtxid tx failures] + TX_NONSTANDARD_INPUTS) - TX_WITNESS_STRIPPED) into the txid cache.

    Let me maybe be more explicit about what I mean: in both cases, the concept of a special class of failure (TX_WITNESS_STRIPPED) which should not be inserted into any rejection filter can go away. In both approaches, all failures can be inserted into the wtxid rejection caches. Depending on the approach, either none, or all the appropriate, failures can be inserted into the txid rejection caches.

  45. in src/test/transaction_tests.cpp:1171 in ffd82f3783 outdated 
    1165 | +    // WitnessV1Taproot, PayToAnchor, WitnessUnknown.
1166 | +    static_assert(std::variant_size_v<CTxDestination> == 9);
1167 | +
1168 | +    // Go through all defined output types and sanity check SpendsNonAnchorWitnessProg.
1169 | +
1170 | +    // P2PK
    glozow commented at 2:47 PM on August 8, 2025:

    could add checks that the txout types are as labeled, e.g.

    diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
index fb1829db7e5..90a34aa778c 100644
--- a/src/test/transaction_tests.cpp
+++ b/src/test/transaction_tests.cpp
@@ -1171,12 +1171,22 @@ BOOST_AUTO_TEST_CASE(spends_witness_prog)
     tx_create.vout[0].scriptPubKey = GetScriptForDestination(PubKeyDestination{pubkey});
     tx_spend.vin[0].prevout.hash = tx_create.GetHash();
     AddCoins(coins, CTransaction{tx_create}, 0, false);
+    {
+        std::vector<std::vector<unsigned char>> vSolutions;
+        TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+        BOOST_CHECK_EQUAL(whichType, TxoutType::PUBKEY);
+    }
     BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
 
     // P2PKH
     tx_create.vout[0].scriptPubKey = GetScriptForDestination(PKHash{pubkey});
     tx_spend.vin[0].prevout.hash = tx_create.GetHash();
     AddCoins(coins, CTransaction{tx_create}, 0, false);
+    {
+        std::vector<std::vector<unsigned char>> vSolutions;
+        TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+        BOOST_CHECK_EQUAL(whichType, TxoutType::PUBKEYHASH);
+    }
     BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
 
     // P2SH
@@ -1185,6 +1195,11 @@ BOOST_AUTO_TEST_CASE(spends_witness_prog)
     tx_spend.vin[0].prevout.hash = tx_create.GetHash();
     tx_spend.vin[0].scriptSig = CScript{} << OP_0 << ToByteVector(redeem_script);
     AddCoins(coins, CTransaction{tx_create}, 0, false);
+    {
+        std::vector<std::vector<unsigned char>> vSolutions;
+        TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+        BOOST_CHECK_EQUAL(whichType, TxoutType::SCRIPTHASH);
+    }
     BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
     tx_spend.vin[0].scriptSig.clear();
 
@@ -1193,6 +1208,11 @@ BOOST_AUTO_TEST_CASE(spends_witness_prog)
     tx_create.vout[0].scriptPubKey = GetScriptForDestination(WitnessV0ScriptHash{witness_script});
     tx_spend.vin[0].prevout.hash = tx_create.GetHash();
     AddCoins(coins, CTransaction{tx_create}, 0, false);
+    {
+        std::vector<std::vector<unsigned char>> vSolutions;
+        TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+        BOOST_CHECK_EQUAL(whichType, TxoutType::WITNESS_V0_SCRIPTHASH);
+    }
     BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
 
     // P2SH-wrapped P2WSH
@@ -1201,6 +1221,11 @@ BOOST_AUTO_TEST_CASE(spends_witness_prog)
     tx_spend.vin[0].prevout.hash = tx_create.GetHash();
     tx_spend.vin[0].scriptSig = CScript{} << ToByteVector(redeem_script);
     AddCoins(coins, CTransaction{tx_create}, 0, false);
+    {
+        std::vector<std::vector<unsigned char>> vSolutions;
+        TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+        BOOST_CHECK_EQUAL(whichType, TxoutType::SCRIPTHASH);
+    }
     BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
     tx_spend.vin[0].scriptSig.clear();
     BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
@@ -1209,6 +1234,11 @@ BOOST_AUTO_TEST_CASE(spends_witness_prog)
     tx_create.vout[0].scriptPubKey = GetScriptForDestination(WitnessV0KeyHash{pubkey});
     tx_spend.vin[0].prevout.hash = tx_create.GetHash();
     AddCoins(coins, CTransaction{tx_create}, 0, false);
+    {
+        std::vector<std::vector<unsigned char>> vSolutions;
+        TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+        BOOST_CHECK_EQUAL(whichType, TxoutType::WITNESS_V0_KEYHASH);
+    }
     BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
 
     // P2SH-wrapped P2WSH
@@ -1217,6 +1247,11 @@ BOOST_AUTO_TEST_CASE(spends_witness_prog)
     tx_spend.vin[0].prevout.hash = tx_create.GetHash();
     tx_spend.vin[0].scriptSig = CScript{} << ToByteVector(redeem_script);
     AddCoins(coins, CTransaction{tx_create}, 0, false);
+    {
+        std::vector<std::vector<unsigned char>> vSolutions;
+        TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+        BOOST_CHECK_EQUAL(whichType, TxoutType::SCRIPTHASH);
+    }
     BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
     tx_spend.vin[0].scriptSig.clear();
     BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
@@ -1225,6 +1260,11 @@ BOOST_AUTO_TEST_CASE(spends_witness_prog)
     tx_create.vout[0].scriptPubKey = GetScriptForDestination(WitnessV1Taproot{XOnlyPubKey{pubkey}});
     tx_spend.vin[0].prevout.hash = tx_create.GetHash();
     AddCoins(coins, CTransaction{tx_create}, 0, false);
+    {
+        std::vector<std::vector<unsigned char>> vSolutions;
+        TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+        BOOST_CHECK_EQUAL(whichType, TxoutType::WITNESS_V1_TAPROOT);
+    }
     BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
 
     // P2SH-wrapped P2TR (undefined, non-standard)
@@ -1233,6 +1273,11 @@ BOOST_AUTO_TEST_CASE(spends_witness_prog)
     tx_spend.vin[0].prevout.hash = tx_create.GetHash();
     tx_spend.vin[0].scriptSig = CScript{} << ToByteVector(redeem_script);
     AddCoins(coins, CTransaction{tx_create}, 0, false);
+    {
+        std::vector<std::vector<unsigned char>> vSolutions;
+        TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+        BOOST_CHECK_EQUAL(whichType, TxoutType::SCRIPTHASH);
+    }
     BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
     tx_spend.vin[0].scriptSig.clear();
     BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
@@ -1241,6 +1286,11 @@ BOOST_AUTO_TEST_CASE(spends_witness_prog)
     tx_create.vout[0].scriptPubKey = GetScriptForDestination(PayToAnchor{});
     tx_spend.vin[0].prevout.hash = tx_create.GetHash();
     AddCoins(coins, CTransaction{tx_create}, 0, false);
+    {
+        std::vector<std::vector<unsigned char>> vSolutions;
+        TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+        BOOST_CHECK_EQUAL(whichType, TxoutType::ANCHOR);
+    }
     BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
 
     // P2SH-wrapped P2A (undefined, non-standard)
@@ -1249,6 +1299,11 @@ BOOST_AUTO_TEST_CASE(spends_witness_prog)
     tx_spend.vin[0].prevout.hash = tx_create.GetHash();
     tx_spend.vin[0].scriptSig = CScript{} << ToByteVector(redeem_script);
     AddCoins(coins, CTransaction{tx_create}, 0, false);
+    {
+        std::vector<std::vector<unsigned char>> vSolutions;
+        TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+        BOOST_CHECK_EQUAL(whichType, TxoutType::SCRIPTHASH);
+    }
     BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
     tx_spend.vin[0].scriptSig.clear();
 
@@ -1256,6 +1311,11 @@ BOOST_AUTO_TEST_CASE(spends_witness_prog)
     tx_create.vout[0].scriptPubKey = GetScriptForDestination(WitnessUnknown{1, {0x42, 0x42}});
     tx_spend.vin[0].prevout.hash = tx_create.GetHash();
     AddCoins(coins, CTransaction{tx_create}, 0, false);
+    {
+        std::vector<std::vector<unsigned char>> vSolutions;
+        TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+        BOOST_CHECK_EQUAL(whichType, TxoutType::WITNESS_UNKNOWN);
+    }
     BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
 
     // P2SH-wrapped undefined version 1 witness program
@@ -1264,6 +1324,11 @@ BOOST_AUTO_TEST_CASE(spends_witness_prog)
     tx_spend.vin[0].prevout.hash = tx_create.GetHash();
     tx_spend.vin[0].scriptSig = CScript{} << ToByteVector(redeem_script);
     AddCoins(coins, CTransaction{tx_create}, 0, false);
+    {
+        std::vector<std::vector<unsigned char>> vSolutions;
+        TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+        BOOST_CHECK_EQUAL(whichType, TxoutType::SCRIPTHASH);
+    }
     BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
     tx_spend.vin[0].scriptSig.clear();
     BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
@@ -1274,6 +1339,11 @@ BOOST_AUTO_TEST_CASE(spends_witness_prog)
         tx_create.vout[0].scriptPubKey = GetScriptForDestination(WitnessUnknown{i, program});
         tx_spend.vin[0].prevout.hash = tx_create.GetHash();
         AddCoins(coins, CTransaction{tx_create}, 0, false);
+        {
+            std::vector<std::vector<unsigned char>> vSolutions;
+            TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+            BOOST_CHECK_EQUAL(whichType, TxoutType::WITNESS_UNKNOWN);
+        }
         BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
 
         // It's also detected within P2SH.
@@ -1282,6 +1352,11 @@ BOOST_AUTO_TEST_CASE(spends_witness_prog)
         tx_spend.vin[0].prevout.hash = tx_create.GetHash();
         tx_spend.vin[0].scriptSig = CScript{} << ToByteVector(redeem_script);
         AddCoins(coins, CTransaction{tx_create}, 0, false);
+        {
+            std::vector<std::vector<unsigned char>> vSolutions;
+            TxoutType whichType = Solver(tx_create.vout[0].scriptPubKey, vSolutions);
+            BOOST_CHECK_EQUAL(whichType, TxoutType::SCRIPTHASH);
+        }
         BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
         tx_spend.vin[0].scriptSig.clear();
         BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins));
    darosior commented at 3:37 PM on August 8, 2025:

    Sure, done.

  46. glozow commented at 2:48 PM on August 8, 2025: member

    ACK ffd82f3783d1af4f94f7dbd0af3a80d4591eb185

  47. policy: introduce a helper to detect whether a transaction spends Segwit outputs
    We will use this helper in later commits to detect witness stripping without having
to execute every input Script three times in a row.
    2907b58834
  48. validation: detect witness stripping without re-running Script checks
    Since it was introduced in 4eb515574e1012bc8ea5dafc3042dcdf4c766f26 (#18044), the detection of a
stripped witness relies on running the Script checks 3 times. In the worst case, this consists in
running Script validation 3 times for every single input.

Detection of a stripped witness is necessary because in this case wtxid==txid, and the transaction's
wtxid must not be added to the reject filter or it could allow a malicious peer to interfere with
txid-based orphan resolution as used in 1p1c package relay.

However it is not necessary to run Script validation to detect a stripped witness (much less so
doing it 3 times in a row). There are 3 types of witness program: defined program types (Taproot,
P2WPKH, P2WSH), undefined types, and the Pay-to-anchor carve-out.

For defined program types, Script validation with an empty witness will always fail (by consensus).
For undefined program types, Script validation is always going to fail regardless of the witness (by
standardness). For P2A, an empty witness is never going to lead to a failure.

Therefore it holds that we can always detect a stripped witness without re-running Script validation.
However this might lead to more "false positives" (cases where we return witness stripping for an
otherwise invalid transaction) than the existing implementation. For instance a transaction with one
P2PKH input with an invalid signature and one P2WPKH input with its witness stripped. The existing
implementation would treat it as consensus invalid while the implementation in this commit would
always consider it witness stripped.
    27aefac425
  49. Crypt-iQ commented at 3:14 PM on August 8, 2025: contributor

    re-ACK ffd82f3783d1af4f94f7dbd0af3a80d4591eb185

  50. darosior force-pushed on Aug 8, 2025
  51. sipa commented at 3:40 PM on August 8, 2025: member

    re-ACK 27aefac42505e9c083fa131d3d7edbec7803f3c0

  52. DrahtBot requested review from glozow on Aug 8, 2025
  53. Crypt-iQ commented at 3:59 PM on August 8, 2025: contributor

    re-ACK 27aefac42505e9c083fa131d3d7edbec7803f3c0

  54. glozow commented at 6:15 PM on August 8, 2025: member

    reACK 27aefac42505e9c083fa131d3d7edbec7803f3c0

  55. glozow merged this on Aug 8, 2025
  56. glozow closed this on Aug 8, 2025
  57. ajtowns commented at 6:37 PM on August 8, 2025: contributor

    Let me maybe be more explicit about what I mean: in both cases, the concept of a special class of failure (TX_WITNESS_STRIPPED) which should not be inserted into any rejection filter can go away. In both approaches, all failures can be inserted into the wtxid rejection caches. Depending on the approach, either none, or all the appropriate, failures can be inserted into the txid rejection caches.

    Ah, in that case we could have an early check for "this utxo's scriptPubKey (which I understand and would accept spends of) requires witness data and you didn't provide it" and translate that to a TX_MISSING_EXPECTED_WITNESS_DATA error which would go into the wtxid reject cache, but not the txid reject cache, even in the case where all witness data was missing and wtxid=txid.

    I guess I don't mind it, but it seems like it wouldn't be very helpful over doing the same logic and just reporting TX_WITNESS_STRIPPED and having a shared cache -- it'd help you avoid requesting it if multiple buggy/adversarial peers announce the same wtxid which seems unlikely, but wouldn't improve orphan resolution.

  58. darosior deleted the branch on Aug 8, 2025
Contributors
darosiorDrahtBotajtownsnervana21achow101glozowCrypt-iQluke-jrsipa


ajtownsachow101Crypt-iQnervana21

Labels
Validation
Linked (view graph)
#18044 Use wtxid for transaction relay#33066 p2p: never check tx rejections by txid
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-28 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me