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

1. 
   jl2012 commented at 3:47 pm on August 25, 2016: contributor

   1. 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.
   2. 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
   3. The tx weight with witness is checked after we have confirmed that the witness is valid.

   This should obsolete #8499

2. jl2012 force-pushed on Aug 28, 2016
3. 
   jl2012 commented at 2:42 pm on August 28, 2016: contributor

   Added: Not verify if witness_stripped_size * accurately_counted_base_sigops > 10MB
4. jl2012 renamed this:
   Verify all incoming txs unless the witness stripped size is >100kB
   Verify all incoming txs unless too big or too much hashing
   on Aug 28, 2016
5. jonasschnelli added the label Mempool on Aug 29, 2016
6. jonasschnelli added this to the milestone 0.13.1 on Aug 29, 2016
7. in src/policy/policy.h: in 91330e08c6 outdated

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

   ---

   ---

   

   instagibbs commented at 2:41 pm on August 29, 2016:

   in bytes?

   ---

   

   jl2012 commented at 2:53 pm on August 29, 2016:

   yes, 10MB

   ---

   

   instagibbs commented at 2:57 pm on August 29, 2016:

   please add units in comment thanks
8. in src/primitives/transaction.cpp: in 91330e08c6 outdated

   152@@ -153,3 +153,8 @@ int64_t GetTransactionWeight(const CTransaction& tx)
   153 {
   154     return ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * (WITNESS_SCALE_FACTOR -1) + ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION);
   155 }
   156+
   157+int64_t GetWitnessStrippedTransactionWeight(const CTransaction& tx)
   158+{
   159+    return ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR;
   160+}
   

   ---

   ---

   

   instagibbs commented at 2:42 pm on August 29, 2016:

   nit: add newline here
9. in src/main.cpp: in 91330e08c6 outdated

   1294+            }
   1295+            return false;
   1296+        }
   1297+
   1298+        // Now we know the witness is valid. We could check the size with witness
   1299+        if (fRequireStandard && GetTransactionWeight(tx) >= MAX_STANDARD_TX_WEIGHT)
   

   ---

   ---

   

   instagibbs commented at 3:48 pm on August 29, 2016:

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

   ---

   

   sipa commented at 4:02 pm on August 29, 2016:

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

   ---

   

   jl2012 commented at 4:06 pm on August 29, 2016:

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

   ---

   

   dcousens commented at 1:33 am on August 30, 2016:

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

   ---

   

   sipa commented at 9:27 am on August 30, 2016:

   @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 :)
10. jonasschnelli added the label Needs backport on Aug 30, 2016
11. 
    instagibbs commented at 1:40 pm on September 1, 2016: member

    needs rebase
12. Verify all txs unless too big or too much hashing b50cf347ad
13. jl2012 force-pushed on Sep 1, 2016
14. 
    jl2012 commented at 6:36 pm on September 1, 2016: contributor

    rebased. @sipa has a better estimation of sighash size by removing the scriptSig
15. laanwj removed this from the milestone 0.13.1 on Sep 1, 2016
16. laanwj removed the label Needs backport on Sep 1, 2016
17. 
    laanwj commented at 7:21 pm on September 1, 2016: member

    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).
18. 
    TheBlueMatt commented at 8:39 pm on July 11, 2017: member

    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?
19. 
    jl2012 commented at 7:11 am on October 6, 2017: contributor

    close due to lack of interests
20. jl2012 closed this on Oct 6, 2017

    ---

21. DrahtBot locked this on Sep 8, 2021

Contributors
jl2012 instagibbs sipa dcousens laanwj TheBlueMatt

Labels
Mempool