Verify all incoming txs unless too big or too much hashing #8593

jl2012 · Aug 25, 2016

IsStandardTx now checks the witness stripped size, instead of transaction weight with witness size counted. Due to the ability of relay node to malleate witness, checking the tx weight with witness is not reliable before the witness is actually verified.
Script verification is done before all the resources limit policy checking. Relay nodes trying to malleate witness will be banned. We don't do this if the witness stripped size is >100kB, otherwise we have problems with O(n^2) hashing
The tx weight with witness is checked after we have confirmed that the witness is valid.

This should obsolete #8499

jl2012 · Aug 28, 2016

Added: Not verify if witness_stripped_size * accurately_counted_base_sigops > 10MB

in bytes?

yes, 10MB

please add units in comment thanks

nit: add newline here

can't a relay node stuff in a too-large valid witness causing this to enter rejection cache?

No, because the CheckInputs above will catch that as mauling.

@sipa: it is possible in some cases. That's why we need NULLDUMMY (#8533) and MINIMALIF (#8528)

@sipa in what context do you refer to mauling? Mutation/malleability?

@dcousens I've found some literature that uses 'to maul' as the verb related to malleability, and not 'to malleate', so I guess we should use that :)

instagibbs · Sep 1, 2016

needs rebase

jl2012 · Sep 1, 2016

rebased.

@sipa has a better estimation of sighash size by removing the scriptSig

laanwj · Sep 1, 2016

Untagging this for 0.13.1 and tagging #8499 instead, this has been deemed too large a change for a minor version in today's meeting (2016-09-01).

TheBlueMatt · Jul 11, 2017

This need a decent rebase. Do we still want this? I suppose we currently rely on "good" copies of txn which have been malleated in this way making it to us eventually? Is that good enough?

jl2012 changed the title from Verify all incoming txs unless the witness stripped size is >100kB to Verify all incoming txs unless too big or too much hashing Aug 28, 2016

jonasschnelli added the Mempool label Aug 29, 2016

jonasschnelli added this to the 0.13.1 milestone Aug 29, 2016

jonasschnelli added the Needs backport label Aug 30, 2016

jl2012 Verify all txs unless too big or too much hashing

b50cf34

laanwj removed this from the 0.13.1 milestone Sep 1, 2016

laanwj removed the Needs backport label Sep 1, 2016

This was referenced Sep 13, 2016

Closed

Mempool DoS risk in segwit due to malleated transactions #8279

Merged

Add several policy limits and disable uncompressed keys for segwit scripts #8499

@@ -30,6 +30,8 @@ static const unsigned int MAX_STANDARD_TX_SIGOPS_COST = MAX_BLOCK_SIGOPS_COST/5;
      static const unsigned int DEFAULT_MAX_MEMPOOL_SIZE = 300;
      /** Default for -bytespersigop */
      static const unsigned int DEFAULT_BYTES_PER_SIGOP = 20;
+    +/** Maximum amount of estimated hashing in base CHECKSIG operations */

@@ -153,3 +153,8 @@ int64_t GetTransactionWeight(const CTransaction& tx)
      {
          return ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * (WITNESS_SCALE_FACTOR -1) + ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION);
      }
+    +
+    +int64_t GetWitnessStrippedTransactionWeight(const CTransaction& tx)
+    +{
+    +    return ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR;
+    +}

+    +
+    +        // Check against previous transactions
+    +        if (!CheckInputs(tx, state, view, true, scriptVerifyFlags, true)) {
+    +            // SCRIPT_VERIFY_CLEANSTACK requires SCRIPT_VERIFY_WITNESS, so we
+    +            // need to turn both off, and compare against just turning off CLEANSTACK
+    +            // to see if the failure is specifically due to witness validation.
+    +            if (CheckInputs(tx, state, view, true, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true) &&
+    +                !CheckInputs(tx, state, view, true, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true)) {
+    +                // Only the witness is wrong, so the transaction itself may be fine.
+    +                state.SetCorruptionPossible();
+    +            }
+    +            return false;
+    +        }
+    +
+    +        // Now we know the witness is valid. We could check the size with witness
+    +        if (fRequireStandard && GetTransactionWeight(tx) >= MAX_STANDARD_TX_WEIGHT)

bitcoin/bitcoin

Verify all incoming txs unless too big or too much hashing #8593

jl2012 commented Aug 25, 2016

jl2012 commented Aug 28, 2016

jl2012 changed the title from Verify all incoming txs unless the witness stripped size is >100kB to Verify all incoming txs unless too big or too much hashing Aug 28, 2016

jonasschnelli added the Mempool label Aug 29, 2016

jonasschnelli added this to the 0.13.1 milestone Aug 29, 2016

instagibbs commented on the diff Aug 29, 2016

instagibbs commented on the diff Aug 29, 2016

instagibbs commented on the diff Aug 29, 2016

jonasschnelli added the Needs backport label Aug 30, 2016

instagibbs commented Sep 1, 2016

jl2012 commented Sep 1, 2016

laanwj removed this from the 0.13.1 milestone Sep 1, 2016

laanwj removed the Needs backport label Sep 1, 2016

laanwj commented Sep 1, 2016 • edited

This was referenced Sep 13, 2016

Mempool DoS risk in segwit due to malleated transactions #8279

Add several policy limits and disable uncompressed keys for segwit scripts #8499

TheBlueMatt commented Jul 11, 2017

bitcoin/bitcoin

Join GitHub today

Verify all incoming txs unless too big or too much hashing #8593

Conversation

jl2012 commented Aug 25, 2016

jl2012 commented Aug 28, 2016

jl2012 changed the title from Verify all incoming txs unless the witness stripped size is >100kB to Verify all incoming txs unless too big or too much hashing Aug 28, 2016

jonasschnelli added the Mempool label Aug 29, 2016

jonasschnelli added this to the 0.13.1 milestone Aug 29, 2016

instagibbs commented on the diff Aug 29, 2016

instagibbs commented on the diff Aug 29, 2016

instagibbs commented on the diff Aug 29, 2016

jonasschnelli added the Needs backport label Aug 30, 2016

instagibbs commented Sep 1, 2016

jl2012 commented Sep 1, 2016

laanwj removed this from the 0.13.1 milestone Sep 1, 2016

laanwj removed the Needs backport label Sep 1, 2016

laanwj commented Sep 1, 2016 • edited

This was referenced Sep 13, 2016

TheBlueMatt commented Jul 11, 2017