Do not store witness txn in rejection cache #8525

1. 
   sipa commented at 3:42 pm on August 16, 2016: member

   As proposed in the IRC meeting from august 4th (see http://www.erisian.com.au/meetbot/bitcoin-core-dev/2016/bitcoin-core-dev.2016-08-04-19.00.log.html), this changes the logic to never store witness transactions (or transactions whose witness data was stripped) in the rejection cache.

   As an optimization, only double check for witness invalidity if the transaction has no witness. This changes the meaning for state.CorruptionPossible() for transactions from “witness possibly invalidated” to “witness stripped”. This is faster (no need for 3 script checks) and simpler for the DoS logic.

   This also removes the special logic to not assign DoS for witness failures for non-witness peers. That logic already exists in more generic form, through the non-mandatory script verify flags.

   See also #8279.

2. sipa force-pushed on Aug 16, 2016
3. MarcoFalke added this to the milestone 0.13.1 on Aug 16, 2016
4. MarcoFalke added the label Needs backport on Aug 16, 2016
5. in src/main.cpp: in 49bf913fb0 outdated

   5487@@ -5488,7 +5488,10 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
   5488                         // Probably non-standard or insufficient fee/priority
   5489                         LogPrint("mempool", "   removed orphan tx %s\n", orphanHash.ToString());
   5490                         vEraseQueue.push_back(orphanHash);
   5491-                        if (!stateDummy.CorruptionPossible()) {
   5492+                        if (orphanTx.wit.IsNull() && !state.CorruptionPossible()) {
   

   ---

   ---

   

   instagibbs commented at 4:57 pm on August 16, 2016:

   s/state/stateDummy/ ?

   ---

   

   sipa commented at 4:59 pm on August 16, 2016:

   Nice catch, fixed!
6. sipa force-pushed on Aug 16, 2016
7. 
   instagibbs commented at 5:04 pm on August 16, 2016: member

   code review ACK https://github.com/bitcoin/bitcoin/pull/8525/commits/59d0df0193181a95f76f0d1c13f311461457aa03
8. 
   laanwj commented at 10:49 am on August 17, 2016: member

   concept ACK https://github.com/bitcoin/bitcoin/pull/8525/commits/59d0df0193181a95f76f0d1c13f311461457aa03
9. 
   sipa commented at 10:13 am on August 18, 2016: member

   @sdaftuar @morcos Comments?
10. 
    instagibbs commented at 4:06 pm on August 18, 2016: member

    created a tiny test for the witness-stuffing case that previous blinded nodes: https://github.com/instagibbs/bitcoin/commit/62778b20a971653cd84d2797adf51720985f55a6
11. Do not store witness txn in rejection cache 34521e4d7d
12. Add basic test for IsStandard witness transaction blinding ca10a03add
13. sipa force-pushed on Sep 5, 2016
14. 
    sipa commented at 3:57 pm on September 5, 2016: member

    Rebased, and incorporated @instagibbs’ test.
15. 
    laanwj commented at 5:47 am on September 9, 2016: member

    utACK ca10a03
16. laanwj merged this on Sep 9, 2016
17. laanwj closed this on Sep 9, 2016

    ---

18. laanwj referenced this in commit 80a4f21d37 on Sep 9, 2016
19. 
    sdaftuar commented at 6:26 pm on September 13, 2016: member

    @sipa Sorry for the late review! One question about this:

    As an optimization, only double check for witness invalidity if the transaction has no witness. This changes the meaning for state.CorruptionPossible() for transactions from “witness possibly invalidated” to “witness stripped”. This is faster (no need for 3 script checks) and simpler for the DoS logic.

    We still have one place in AcceptToMemoryPoolWorker where we return a state with CorruptionPossible set to true, even when there is a witness:

    https://github.com/sipa/bitcoin/blob/ca10a03addf70421893791c2c499e82fc494d60b/src/main.cpp#L1147

    This doesn’t seem to be a problem at all, because the caller is checking to see if the witness is NULL before checking CorruptionPossible, but I was wondering if the intention was to eliminate this type of usage?

20. laanwj referenced this in commit 1672225670 on Sep 26, 2016
21. laanwj referenced this in commit b394a96396 on Sep 26, 2016
22. laanwj removed the label Needs backport on Sep 26, 2016
23. 
    laanwj commented at 2:57 pm on September 26, 2016: member

    This is backported in #8815, removing tag
24. codablock referenced this in commit 7b95d5bde3 on Apr 4, 2018
25. gladcow referenced this in commit 67910590a1 on Apr 4, 2018
26. UdjinM6 referenced this in commit bc45a2f87a on Apr 11, 2018
27. andvgal referenced this in commit fd5c50bc2b on Jan 6, 2019
28. CryptoCentric referenced this in commit dd3fd51204 on Feb 28, 2019
29. instagibbs referenced this in commit bcf5d91568 on Aug 3, 2020
30. MarcoFalke locked this on Sep 8, 2021

Contributors
sipa instagibbs laanwj sdaftuar

Milestone
0.13.1