Policy: Report reason inputs are non standard #29060

pull ismaelsadeeq wants to merge 2 commits into bitcoin:master from ismaelsadeeq:11-2023-non-standard-inputs-error-messages changing 9 files +136 −30
  1. ismaelsadeeq commented at 1:16 pm on December 12, 2023: member

    This PR is another attempt at #13525.

    Transactions that fail PreChecks Validation due to non-standard inputs now returns invalid validation stateTxValidationResult::TX_INPUTS_NOT_STANDARD along with a debug error message.

    Previously, the debug error message for non-standard inputs do not specify why the inputs were considered non-standard. Instead, the same error string, bad-txns-nonstandard-inputs, used for all types of non-standard input scriptSigs.

    This PR updates the AreInputsStandard to include the reason why inputs are non-standard in the debug message. This improves the Precheck debug message to be more descriptive.

    Furthermore, I have addressed all remaining comments from #13525 in this PR.

  2. DrahtBot commented at 1:16 pm on December 12, 2023: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK stickies-v

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

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label TX fees and policy on Dec 12, 2023
  4. ismaelsadeeq force-pushed on Dec 12, 2023
  5. DrahtBot added the label CI failed on Dec 12, 2023
  6. DrahtBot removed the label CI failed on Dec 12, 2023
  7. in src/policy/policy.cpp:178 in 6ffb9569b7 outdated 
    174@@ -174,38 +175,49 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
175  *
176  * Note that only the non-witness portion of the transaction is checked here.
177  */
178-bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
179+std::optional<NonStandardInputsReason> AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
    stickies-v commented at 1:42 pm on December 13, 2023:

    A function called AreInputsStandard that returns a falsy value if inputs are standard is counterintuitive. Would suggest renaming to e.g. HasNonStandardInput.

      0diff --git a/src/bench/ccoins_caching.cpp b/src/bench/ccoins_caching.cpp
  1index c677fb1c16..9a31cecd7a 100644
  2--- a/src/bench/ccoins_caching.cpp
  3+++ b/src/bench/ccoins_caching.cpp
  4@@ -44,7 +44,7 @@ static void CCoinsCaching(benchmark::Bench& bench)
  5     // Benchmark.
  6     const CTransaction tx_1(t1);
  7     bench.run([&] {
  8-        auto result{AreInputsStandard(tx_1, coins)};
  9+        auto result{HasNonStandardInput(tx_1, coins)};
 10         assert(!result.has_value());
 11     });
 12     ECC_Stop();
 13diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
 14index c093cb3923..aada94d909 100644
 15--- a/src/policy/policy.cpp
 16+++ b/src/policy/policy.cpp
 17@@ -175,7 +175,7 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
 18  *
 19  * Note that only the non-witness portion of the transaction is checked here.
 20  */
 21-std::optional<NonStandardInputsReason> AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
 22+std::optional<NonStandardInputsReason> HasNonStandardInput(const CTransaction& tx, const CCoinsViewCache& mapInputs)
 23 {
 24     if (tx.IsCoinBase()) {
 25         return std::nullopt; // Coinbases don't use vin normally
 26@@ -187,32 +187,27 @@ std::optional<NonStandardInputsReason> AreInputsStandard(const CTransaction& tx,
 27         std::vector<std::vector<unsigned char> > vSolutions;
 28         TxoutType whichType = Solver(prev.scriptPubKey, vSolutions);
 29         if (whichType == TxoutType::NONSTANDARD) {
 30-            auto result = NonStandardInputsReason{"bad-txns-input-script-nonstandard", strprintf("input %u", i)};
 31-            return result;
 32+            return NonStandardInputsReason{"bad-txns-input-script-nonstandard", strprintf("input %u", i)};
 33         } else if (whichType == TxoutType::WITNESS_UNKNOWN) {
 34             // WITNESS_UNKNOWN failures are typically also caught with a policy
 35             // flag in the script interpreter, but it can be helpful to catch
 36             // this type of NONSTANDARD transaction earlier in transaction
 37             // validation.
 38-            auto result = NonStandardInputsReason{"bad-txns-input-witness-unknown", strprintf("input %u", i)};
 39-            return result;
 40+            return NonStandardInputsReason{"bad-txns-input-witness-unknown", strprintf("input %u", i)};
 41         } else if (whichType == TxoutType::SCRIPTHASH) {
 42             std::vector<std::vector<unsigned char> > stack;
 43             ScriptError serror;
 44             // convert the scriptSig into a stack, so we can inspect the redeemScript
 45             if (!EvalScript(stack, tx.vin[i].scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker(), SigVersion::BASE, &serror)) {
 46-                auto result = NonStandardInputsReason{"bad-txns-input-scriptsig-failure", strprintf("input %u: %s", i, ScriptErrorString(serror))};
 47-                return result;
 48+                return NonStandardInputsReason{"bad-txns-input-scriptsig-failure", strprintf("input %u: %s", i, ScriptErrorString(serror))};
 49             }
 50             if (stack.empty()) {
 51-                auto result = NonStandardInputsReason{"bad-txns-input-p2sh-no-redeemscript", strprintf("input %u", i)};
 52-                return result;
 53+                return NonStandardInputsReason{"bad-txns-input-p2sh-no-redeemscript", strprintf("input %u", i)};
 54             }
 55             CScript subscript(stack.back().begin(), stack.back().end());
 56             unsigned int sigop_count = subscript.GetSigOpCount(true);
 57             if (sigop_count > MAX_P2SH_SIGOPS) {
 58-                auto result = NonStandardInputsReason{"bad-txns-input-scriptcheck-sigops", strprintf("input %u: %u > %u", i, sigop_count, MAX_P2SH_SIGOPS)};
 59-                return result;
 60+                return NonStandardInputsReason{"bad-txns-input-scriptcheck-sigops", strprintf("input %u: %u > %u", i, sigop_count, MAX_P2SH_SIGOPS)};
 61             }
 62         }
 63     }
 64diff --git a/src/policy/policy.h b/src/policy/policy.h
 65index 3986f7d709..3d7d4b7e5c 100644
 66--- a/src/policy/policy.h
 67+++ b/src/policy/policy.h
 68@@ -149,7 +149,7 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
 69  * [@return](/bitcoin-bitcoin/contributor/return/) std::nullopt if all inputs (scriptSigs) use only standard transaction forms else returns
 70  * NonStandardInputsReason which states why an input is not standard.
 71  */
 72-std::optional<NonStandardInputsReason> AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs);
 73+std::optional<NonStandardInputsReason> HasNonStandardInput(const CTransaction& tx, const CCoinsViewCache& mapInputs);
 74 /**
 75 * Check if the transaction is over standard P2WSH resources limit:
 76 * 3600bytes witnessScript size, 80bytes per witness stack element, 100 witness stack elements
 77diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp
 78index 8f3e357a84..e210b2a13d 100644
 79--- a/src/test/fuzz/coins_view.cpp
 80+++ b/src/test/fuzz/coins_view.cpp
 81@@ -235,7 +235,7 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
 82                 assert(expected_code_path);
 83             },
 84             [&] {
 85-                (void)AreInputsStandard(CTransaction{random_mutable_transaction}, coins_view_cache);
 86+                (void)HasNonStandardInput(CTransaction{random_mutable_transaction}, coins_view_cache);
 87             },
 88             [&] {
 89                 TxValidationState state;
 90diff --git a/src/test/fuzz/transaction.cpp b/src/test/fuzz/transaction.cpp
 91index 2a043f7458..785c823e37 100644
 92--- a/src/test/fuzz/transaction.cpp
 93+++ b/src/test/fuzz/transaction.cpp
 94@@ -87,7 +87,7 @@ FUZZ_TARGET(transaction, .init = initialize_transaction)
 95 
 96     CCoinsView coins_view;
 97     const CCoinsViewCache coins_view_cache(&coins_view);
 98-    (void)AreInputsStandard(tx, coins_view_cache);
 99+    (void)HasNonStandardInput(tx, coins_view_cache);
100     (void)IsWitnessStandard(tx, coins_view_cache);
101 
102     if (tx.GetTotalSize() < 250'000) { // Avoid high memory usage (with msan) due to json encoding
103diff --git a/src/test/script_p2sh_tests.cpp b/src/test/script_p2sh_tests.cpp
104index 500ec0bd6b..32907bdd24 100644
105--- a/src/test/script_p2sh_tests.cpp
106+++ b/src/test/script_p2sh_tests.cpp
107@@ -265,7 +265,7 @@ BOOST_AUTO_TEST_CASE(switchover)
108     BOOST_CHECK_MESSAGE(err == SCRIPT_ERR_EQUALVERIFY, ScriptErrorString(err));
109 }
110 
111-BOOST_AUTO_TEST_CASE(AreInputsStandard)
112+BOOST_AUTO_TEST_CASE(GetFirstNonStandardInput)
113 {
114     CCoinsView coinsDummy;
115     CCoinsViewCache coins(&coinsDummy);
116@@ -366,7 +366,7 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
117     txTo.vin[3].scriptSig << OP_11 << OP_11 << std::vector<unsigned char>(oneAndTwo.begin(), oneAndTwo.end());
118     txTo.vin[4].scriptSig << std::vector<unsigned char>(fifteenSigops.begin(), fifteenSigops.end());
119 
120-    BOOST_CHECK(!::AreInputsStandard(CTransaction(txTo), coins).has_value());
121+    BOOST_CHECK(!::HasNonStandardInput(CTransaction(txTo), coins).has_value());
122     // 22 P2SH sigops for all inputs (1 for vin[0], 6 for vin[3], 15 for vin[4]
123     BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txTo), coins), 22U);
124 
125@@ -380,7 +380,7 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
126     txToNonStd1.vin[0].prevout.hash = txFrom.GetHash();
127     txToNonStd1.vin[0].scriptSig << std::vector<unsigned char>(sixteenSigops.begin(), sixteenSigops.end());
128 
129-    const auto txToNonStd1_res = ::AreInputsStandard(CTransaction(txToNonStd1), coins);
130+    const auto txToNonStd1_res = ::HasNonStandardInput(CTransaction(txToNonStd1), coins);
131     BOOST_CHECK(txToNonStd1_res.has_value());
132     BOOST_CHECK_EQUAL(txToNonStd1_res->reason, "bad-txns-input-scriptcheck-sigops");
133     BOOST_CHECK_EQUAL(txToNonStd1_res->debug, "input 0: 16 > 15");
134@@ -398,7 +398,7 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
135 
136     std::vector<std::vector<unsigned char>> vSolutions;
137     BOOST_CHECK_EQUAL(Solver(txFrom.vout[6].scriptPubKey, vSolutions), TxoutType::SCRIPTHASH);
138-    const auto txToNonStd2_res = ::AreInputsStandard(CTransaction(txToNonStd2), coins);
139+    const auto txToNonStd2_res = ::HasNonStandardInput(CTransaction(txToNonStd2), coins);
140     BOOST_CHECK(txToNonStd2_res.has_value());
141     BOOST_CHECK_EQUAL(txToNonStd2_res->reason, "bad-txns-input-scriptcheck-sigops");
142     BOOST_CHECK_EQUAL(txToNonStd2_res->debug, "input 0: 20 > 15");
143@@ -413,7 +413,7 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
144     txToNonStd2_no_scriptSig.vin[0].prevout.hash = txFrom.GetHash();
145 
146     BOOST_CHECK_EQUAL(Solver(txFrom.vout[6].scriptPubKey, vSolutions), TxoutType::SCRIPTHASH);
147-    const auto txToNonStd2_no_scriptSig_res = ::AreInputsStandard(CTransaction(txToNonStd2_no_scriptSig), coins);
148+    const auto txToNonStd2_no_scriptSig_res = ::HasNonStandardInput(CTransaction(txToNonStd2_no_scriptSig), coins);
149     BOOST_CHECK(txToNonStd2_no_scriptSig_res.has_value());
150     BOOST_CHECK_EQUAL(txToNonStd2_no_scriptSig_res->reason, "bad-txns-input-p2sh-no-redeemscript");
151     BOOST_CHECK_EQUAL(txToNonStd2_no_scriptSig_res->debug, "input 0");
152@@ -429,7 +429,7 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
153     txToNonStd3.vin[0].prevout.hash = txFrom.GetHash();
154 
155     BOOST_CHECK_EQUAL(Solver(txFrom.vout[7].scriptPubKey, vSolutions), TxoutType::NONSTANDARD);
156-    const auto txToNonStd3_res = ::AreInputsStandard(CTransaction(txToNonStd3), coins);
157+    const auto txToNonStd3_res = ::HasNonStandardInput(CTransaction(txToNonStd3), coins);
158     BOOST_CHECK(txToNonStd3_res.has_value());
159     BOOST_CHECK_EQUAL(txToNonStd3_res->reason, "bad-txns-input-script-nonstandard");
160     BOOST_CHECK_EQUAL(txToNonStd3_res->debug, "input 0");
161@@ -451,7 +451,7 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
162     BOOST_CHECK_EQUAL(Solver(txFrom.vout[8].scriptPubKey, vSolutions), TxoutType::SCRIPTHASH);
163     BOOST_CHECK(!EvalScript(stack, txToNonStd4.vin[0].scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker(), SigVersion::BASE, &serror));
164     BOOST_CHECK_EQUAL(serror, SCRIPT_ERR_OP_RETURN);
165-    const auto txToNonStd4_res = ::AreInputsStandard(CTransaction(txToNonStd4), coins);
166+    const auto txToNonStd4_res = ::HasNonStandardInput(CTransaction(txToNonStd4), coins);
167     BOOST_CHECK(txToNonStd4_res.has_value());
168     BOOST_CHECK_EQUAL(txToNonStd4_res->reason, "bad-txns-input-scriptsig-failure");
169     BOOST_CHECK_EQUAL(txToNonStd4_res->debug, "input 0: OP_RETURN was encountered");
170@@ -465,7 +465,7 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
171     txWitnessUnknown.vin[0].prevout.n = 9;
172     txWitnessUnknown.vin[0].prevout.hash = txFrom.GetHash();
173     BOOST_CHECK_EQUAL(Solver(txFrom.vout[9].scriptPubKey, vSolutions), TxoutType::WITNESS_UNKNOWN);
174-    const auto txWitnessUnknown_res = ::AreInputsStandard(CTransaction(txWitnessUnknown), coins);
175+    const auto txWitnessUnknown_res = ::HasNonStandardInput(CTransaction(txWitnessUnknown), coins);
176     BOOST_CHECK(txWitnessUnknown_res.has_value());
177     BOOST_CHECK_EQUAL(txWitnessUnknown_res->reason, "bad-txns-input-witness-unknown");
178     BOOST_CHECK_EQUAL(txWitnessUnknown_res->debug, "input 0");
179diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
180index e2443a2fce..05074eb631 100644
181--- a/src/test/transaction_tests.cpp
182+++ b/src/test/transaction_tests.cpp
183@@ -406,7 +406,7 @@ BOOST_AUTO_TEST_CASE(test_Get)
184     t1.vout[0].nValue = 90*CENT;
185     t1.vout[0].scriptPubKey << OP_1;
186 
187-    BOOST_CHECK(!AreInputsStandard(CTransaction(t1), coins).has_value());
188+    BOOST_CHECK(!HasNonStandardInput(CTransaction(t1), coins).has_value());
189 }
190 
191 static void CreateCreditAndSpend(const FillableSigningProvider& keystore, const CScript& outscript, CTransactionRef& output, CMutableTransaction& input, bool success = true)
192diff --git a/src/validation.cpp b/src/validation.cpp
193index 96204b2ab4..cdcf56860f 100644
194--- a/src/validation.cpp
195+++ b/src/validation.cpp
196@@ -822,7 +822,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
197         return false; // state filled in by CheckTxInputs
198     }
199 
200-    const auto inputs_standardness_result = AreInputsStandard(tx, m_view);
201+    const auto inputs_standardness_result = HasNonStandardInput(tx, m_view);
202     if (m_pool.m_require_standard && inputs_standardness_result.has_value()) {
203         return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, inputs_standardness_result.value().reason, inputs_standardness_result.value().debug);
204     }

    Alternatively, could use util::Result failure values after #25665 is merged.

    stickies-v commented at 3:09 pm on December 13, 2023:
    nit: requires #include <optional> (in both files)
    ismaelsadeeq commented at 11:45 am on December 14, 2023:
    Thank you, HasNonStandardInput makes more sense, rename to HasNonStandardInput in 94cd47f3460
    ismaelsadeeq commented at 11:47 am on December 14, 2023:
    Fixed
    ismaelsadeeq commented at 12:34 pm on December 25, 2023:

    Alternatively, could use util::Result failure values after #25665 is merged.

    will resolve this since I am now using TxValidationState

  8. in src/policy/policy.cpp:191 in 6ffb9569b7 outdated 
    189         std::vector<std::vector<unsigned char> > vSolutions;
190         TxoutType whichType = Solver(prev.scriptPubKey, vSolutions);
191-        if (whichType == TxoutType::NONSTANDARD || whichType == TxoutType::WITNESS_UNKNOWN) {
192+        if (whichType == TxoutType::NONSTANDARD) {
193+            auto result = NonStandardInputsReason{"bad-txns-input-script-nonstandard", strprintf("input %u", i)};
194+            return result;
    stickies-v commented at 1:43 pm on December 13, 2023:

    nit: No need for the intermediary var, here and in a bunch of other places

    0            return NonStandardInputsReason{"bad-txns-input-script-nonstandard", strprintf("input %u", i)};
    ismaelsadeeq commented at 11:45 am on December 14, 2023:
    Yes, fixed now
  9. in src/test/script_p2sh_tests.cpp:369 in 6ffb9569b7 outdated 
    365@@ -350,10 +366,11 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
366     txTo.vin[3].scriptSig << OP_11 << OP_11 << std::vector<unsigned char>(oneAndTwo.begin(), oneAndTwo.end());
367     txTo.vin[4].scriptSig << std::vector<unsigned char>(fifteenSigops.begin(), fifteenSigops.end());
368 
369-    BOOST_CHECK(::AreInputsStandard(CTransaction(txTo), coins));
370+    BOOST_CHECK(!::AreInputsStandard(CTransaction(txTo), coins).has_value());
    stickies-v commented at 1:44 pm on December 13, 2023:

    nit: no need for .has_value(), idiomatic enough as is imo (here and in a bunch of other places)

    0    BOOST_CHECK(!::AreInputsStandard(CTransaction(txTo), coins));
    ismaelsadeeq commented at 11:45 am on December 14, 2023:
    Fixed in latest push
  10. in src/validation.cpp:827 in 6ffb9569b7 outdated 
    821@@ -822,8 +822,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
822         return false; // state filled in by CheckTxInputs
823     }
824 
825-    if (m_pool.m_require_standard && !AreInputsStandard(tx, m_view)) {
826-        return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs");
827+    const auto inputs_standardness_result = AreInputsStandard(tx, m_view);
828+    if (m_pool.m_require_standard && inputs_standardness_result.has_value()) {
829+        return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, inputs_standardness_result.value().reason, inputs_standardness_result.value().debug);
    stickies-v commented at 1:45 pm on December 13, 2023:

    Since we’ve validated has_value() just above, can just derefence safely for conciseness:

    0        return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, inputs_standardness_result->reason, inputs_standardness_result->debug);
    ismaelsadeeq commented at 11:45 am on December 14, 2023:
    Fixed
  11. in src/bench/ccoins_caching.cpp:48 in 6ffb9569b7 outdated 
    43@@ -44,8 +44,8 @@ static void CCoinsCaching(benchmark::Bench& bench)
44     // Benchmark.
45     const CTransaction tx_1(t1);
46     bench.run([&] {
47-        bool success{AreInputsStandard(tx_1, coins)};
48-        assert(success);
49+        auto result{AreInputsStandard(tx_1, coins)};
50+        assert(!result.has_value());
    stickies-v commented at 1:46 pm on December 13, 2023:

    nit: more concise, easier to read (+ in other places)

    0        assert(!AreInputsStandard(tx_1, coins));
    ismaelsadeeq commented at 11:46 am on December 14, 2023:
    Taken
  12. in src/policy/policy.h:124 in 6ffb9569b7 outdated 
    120@@ -121,6 +121,11 @@ static constexpr unsigned int STANDARD_NOT_MANDATORY_VERIFY_FLAGS{STANDARD_SCRIP
121 /** Used as the flags parameter to sequence and nLocktime checks in non-consensus code. */
122 static constexpr unsigned int STANDARD_LOCKTIME_VERIFY_FLAGS{LOCKTIME_VERIFY_SEQUENCE};
123 
124+struct NonStandardInputsReason {
    stickies-v commented at 2:25 pm on December 13, 2023:

    nit: this is the reason for a single input

    0struct NonStandardInputReason {
    ismaelsadeeq commented at 11:46 am on December 14, 2023:
    Yes, taken
  13. in src/validation.cpp:825 in 6ffb9569b7 outdated 
    821@@ -822,8 +822,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
822         return false; // state filled in by CheckTxInputs
823     }
824 
825-    if (m_pool.m_require_standard && !AreInputsStandard(tx, m_view)) {
826-        return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs");
827+    const auto inputs_standardness_result = AreInputsStandard(tx, m_view);
    stickies-v commented at 3:07 pm on December 13, 2023:

    This causes unnecessary input validation if !m_require_standard, would suggest changing to e.g.

     0diff --git a/src/validation.cpp b/src/validation.cpp
 1index 96204b2ab4..bf1c578384 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -822,9 +822,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
 5         return false; // state filled in by CheckTxInputs
 6     }
 7 
 8-    const auto inputs_standardness_result = AreInputsStandard(tx, m_view);
 9-    if (m_pool.m_require_standard && inputs_standardness_result.has_value()) {
10-        return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, inputs_standardness_result.value().reason, inputs_standardness_result.value().debug);
11+    if (m_pool.m_require_standard) {
12+        if (const auto result{AreInputsStandard(tx, m_view)}) {
13+            return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, result->reason, result->debug);
14+        }
15     }
16 
17     // Check for non-standard witnesses.
    ismaelsadeeq commented at 11:47 am on December 14, 2023:
    Thank you fixed in 94cd47f3460
  14. stickies-v commented at 3:15 pm on December 13, 2023: contributor
    Concept ACK
  15. ismaelsadeeq force-pushed on Dec 14, 2023
  16. ismaelsadeeq commented at 11:49 am on December 14, 2023: member

    Thank you for review @stickies-v

    I Addressed your comments and taken suggestions. Force pushed from https://github.com/bitcoin/bitcoin/commit/6ffb9569b7f1c603ee3bfa3b56ee312d975fd8c9 to https://github.com/bitcoin/bitcoin/commit/e032462c44dba9b6961c1e18bd597d63c6afac02 , Compare diff

  17. in src/policy/policy.cpp:191 in e032462c44 outdated 
    189 
190         std::vector<std::vector<unsigned char> > vSolutions;
191         TxoutType whichType = Solver(prev.scriptPubKey, vSolutions);
192-        if (whichType == TxoutType::NONSTANDARD || whichType == TxoutType::WITNESS_UNKNOWN) {
193+        if (whichType == TxoutType::NONSTANDARD) {
194+            return NonStandardInputReason{"bad-txns-input-script-nonstandard", strprintf("input %u", i)};
    luke-jr commented at 6:09 pm on December 17, 2023: 
    0            return NonStandardInputReason{"bad-txns-input-script-unknown", strprintf("input %u", i)};
    ismaelsadeeq commented at 2:54 pm on December 18, 2023:
    Fixed thanks
  18. in src/policy/policy.cpp:206 in e032462c44 outdated 
    209-                return false;
210+            if (!EvalScript(stack, tx.vin[i].scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker(), SigVersion::BASE, &serror)) {
211+                return NonStandardInputReason{"bad-txns-input-scriptsig-failure", strprintf("input %u: %s", i, ScriptErrorString(serror))};
212+            }
213+            if (stack.empty()) {
214+                return NonStandardInputReason{"bad-txns-input-p2sh-no-redeemscript", strprintf("input %u", i)};
    luke-jr commented at 6:11 pm on December 17, 2023: 
    0                return NonStandardInputReason{"bad-txns-input-scriptcheck-missing", strprintf("input %u", i)};
    ismaelsadeeq commented at 2:54 pm on December 18, 2023:
    fixed
  19. luke-jr changes_requested
  20. ismaelsadeeq force-pushed on Dec 18, 2023
  21. ismaelsadeeq commented at 3:00 pm on December 18, 2023: member

    Thanks for reviewing @luke-jr Force pushed from https://github.com/bitcoin/bitcoin/commit/e032462c44dba9b6961c1e18bd597d63c6afac02 to https://github.com/bitcoin/bitcoin/pull/29060/commits/0a39b0708ec72292ab3e1dde643dedfbd5d20674

    Compare diff

    The changes in latest push are:

    • Rebased on master to fix CI
    • Updated the non-standardness reason to address @luke-jr comments
    • bad-txns-input-script-nonstandard –> bad-txns-input-script-unknown
    • bad-txns-input-scriptsig-failure –> bad-txns-input-p2sh-scriptsig-malformed
    • bad-txns-input-p2sh-no-redeemscript –> bad-txns-input-scriptcheck-missing
    • bad-txns-input-scriptcheck-sigops –> bad-txns-input-p2sh-redeemscript-sigops
  22. ismaelsadeeq force-pushed on Dec 18, 2023
  23. in src/policy/policy.h:128 in 0a39b0708e outdated 
    121@@ -121,6 +122,11 @@ static constexpr unsigned int STANDARD_NOT_MANDATORY_VERIFY_FLAGS{STANDARD_SCRIP
122 /** Used as the flags parameter to sequence and nLocktime checks in non-consensus code. */
123 static constexpr unsigned int STANDARD_LOCKTIME_VERIFY_FLAGS{LOCKTIME_VERIFY_SEQUENCE};
124 
125+struct NonStandardInputReason {
126+    std::string reason;
127+    std::string debug;
128+};
    glozow commented at 11:06 am on December 20, 2023:
    Maybe just use TxValidationState?
    ismaelsadeeq commented at 4:09 pm on December 24, 2023:
    Thanks updated
  24. ismaelsadeeq force-pushed on Dec 24, 2023
  25. ismaelsadeeq force-pushed on Dec 24, 2023
  26. DrahtBot added the label CI failed on Jan 16, 2024
  27. ismaelsadeeq force-pushed on Jan 25, 2024
  28. DrahtBot removed the label CI failed on Jan 26, 2024
  29. ismaelsadeeq commented at 12:36 pm on January 30, 2024: member
    Rebased on master for green CI.
  30. achow101 requested review from luke-jr on Apr 9, 2024
  31. achow101 requested review from glozow on Apr 9, 2024
  32. ismaelsadeeq requested review from stickies-v on Apr 24, 2024
  33. DrahtBot added the label Needs rebase on May 15, 2024
  34. ismaelsadeeq force-pushed on May 17, 2024
  35. DrahtBot removed the label Needs rebase on May 17, 2024
  36. in src/validation.cpp:906 in 101a30833b outdated 
    847@@ -848,8 +848,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
848         return false; // state filled in by CheckTxInputs
849     }
850 
851-    if (m_pool.m_opts.require_standard && !AreInputsStandard(tx, m_view)) {
852-        return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs");
853+    if (m_pool.m_opts.require_standard) {
854+        state = HasNonStandardInput(tx, m_view);
    luke-jr commented at 5:32 pm on May 31, 2024:
    Seems like it’d be nicer to just pass state into the function rather than creating a new one and copying it.
    ismaelsadeeq commented at 6:42 pm on May 31, 2024:

    Seems like it’d be nicer to just pass state into the function

    I am taking this review comment as a non-blocking (nit) suggestion based on the tone.

    I will update this when there are blocking comments that indicate a need to retouch the diffs.

    Also, I would like to know whether this PR is conceptually acknowledged, @luke-jr. Thanks for your review.

  37. DrahtBot added the label CI failed on Sep 11, 2024
  38. policy: update `AreInputsStandard` to return error string 
    This commit renames AreInputsStandard to HasNonStandardInput.

HasNonStandardInput now returns valid TxValidationState if all inputs
(scriptSigs) use only standard transaction forms else returns invalid
TxValidationState which states why an input is not standard.
    38a84db797
  39. test: ensure `HasNonStandardInput` optionally returns debug string 
    Test the various input scriptSig non standardness, and ensure HasNonStandardInput
Returns the resin why the input is nonstandard.

Co-authored-by: Ben Woosley <ben.woosley@gmail.com>
    3ef7ac6f47
  40. ismaelsadeeq force-pushed on Sep 14, 2024
  41. DrahtBot removed the label CI failed on Sep 14, 2024
  42. achow101 requested review from willcl-ark on Oct 15, 2024
  43. achow101 commented at 3:58 pm on October 15, 2024: member
    cc @tdb3
  44. in src/policy/policy.h:146 in 38a84db797 outdated 
    146-*/
147-bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs);
148+ * Check for standard transaction types
149+ * @param[in] mapInputs       Map of previous transactions that have outputs we're spending
150+ * @returns valid TxValidationState if all inputs (scriptSigs) use only standard transaction forms else returns
151+ * invalid TxValidationState which states why an input is not standard.
    tdb3 commented at 0:36 am on October 31, 2024:

    Could also clarify that invalid TxValidationState returned describes the first nonstandard input encountered.

    For example:

    0- * invalid TxValidationState which states why an input is not standard
1+ * invalid TxValidationState which states why the first invalid input is not standard
  45. in src/test/script_p2sh_tests.cpp:345 in 3ef7ac6f47 
    341+    txFrom.vout[7].scriptPubKey = no_sigops;
342+    txFrom.vout[7].nValue = 1000;
343+
344+    // vout [8] is non-standard because it contains OP_RETURN in its scriptSig.
345+    static const unsigned char op_return[] = {OP_RETURN};
346+    txFrom.vout[8].scriptPubKey = GetScriptForDestination(ScriptHash(CScript(op_return, op_return + sizeof(op_return))));
    tdb3 commented at 1:26 am on October 31, 2024:
    Instantiating the CScript here could allow re-use later (in txToNonStd4.vin[0].scriptSig = CScript(op_return, op_return + sizeof(op_return));)
  46. tdb3 commented at 1:33 am on October 31, 2024: contributor

    I’m a fan of the more descriptive errors.

    However, would some of these more descriptive errors flow down to RPC responses and cause issues for users? (e.g. testmempoolaccept).

    #30212 (comment)

    Does bitcoind consider this to be a breaking change? The new error message may be “more accurate”, but for clients that were matching on this error to figure out why a transaction was rejected, this breaks old behavior.


ismaelsadeeq DrahtBot stickies-v luke-jr glozow achow101 tdb3


willcl-ark glozow stickies-v

Labels
TX fees and policy

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: 2024-10-31 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me