Fix counting of sigops cost in mempool check #8364

1. 
   f139975 commented at 5:49 pm on July 18, 2016: none

   The new “bytespersigop” limit introduced the side effect of making most multisig transactions non-standard.

   This fixes #8079 by accurately counting the sigops cost, solely for the newly introduced check.

   As alternative I see two options:

   • revert #7081 which introduced this bug
   • raise DEFAULT_BYTES_PER_SIGOP significally so that there is no longer a side effect

   This should be backported to earlier versions, and especially for 0.13, as soon as possible the path is clear.

2. Fix counting of sigops cost in mempool check

   The new "bytespersigop" limit introduced the side effect of making most multisig transactions non-standard.
   
   This fixes the issue by accurately counting the sigops cost, solely for the newly introduced check.

   7f66701020
3. 
   sipa commented at 5:53 pm on July 18, 2016: member

   This does not prevent the vulnerability, I think? Someone can still create a large number of high-(legacy)sigops transactions, and the mining code will include them, resulting in a very small block.

   AFAIK the only workable solution is to treat transactions in the mempool as if their size was max(size, sigops * MAX_BLOCK_SIZE / MAX_SIGOPS).

4. 
   f139975 commented at 6:01 pm on July 18, 2016: none

   This does not prevent the vulnerability, I think?

   That’s not the objective of this submission, but to remove the side effect of making multisig transactions non-standard, which was introduced by #7081, while leaving #7081 as intact as possible.

   AFAIK the only workable solution is to scale up the mempool transaction size to max(size, sigops * MAX_BLOCK_SIZE / MAX_SIGOPS).

   I’d be happy, if there is a better solution, and I’d be very glad, if you could push one. My primary goal was to present something that may still be merged into 0.13.

5. 
   sipa commented at 6:02 pm on July 18, 2016: member

   @f139975 I’ll see what I can do. I understand the desire to have #8079 fixed, but not by reintroducing the attack that #7081 prevented.
6. 
   f139975 commented at 6:03 pm on July 18, 2016: none

   Thank you!
7. 
   btcdrak commented at 6:36 pm on July 18, 2016: contributor

   ping @rubensayshi @NicolasDorier
8. 
   luke-jr commented at 7:50 pm on July 18, 2016: member

   @sipa The DoS being addressed here was based on actual verification time, not the miner limit. So this fix (at least as described; have not reviewed the code) should be okay. Whether the mining code wants to demand a higher fee or not, is a separate (and perhaps reasonable) issue.

   Concept ACK. Would be nice if someone reopened and/or merged #5231 at the same time.

9. 
   sipa commented at 8:01 pm on July 18, 2016: member

   @luke-jr 20000 verification operations are allowed, and that is not currently changed by any of the proposals (neither #7081, this PR, or #8365). If that is a problem, we should propose a softfork to reduce the sigops limit, not change relay policy (which only prevents it in an easily-avoided case).
10. 
    gmaxwell commented at 8:02 pm on July 18, 2016: contributor

    I don’t see what use this serves. It makes the code more complex and doesn’t constrain anyone’s ability to cause computation. #8365 is sensible.
11. jonasschnelli added the label Mempool on Jul 19, 2016
12. 
    laanwj commented at 12:28 pm on July 26, 2016: member

    Closing in favor of #8365, which was merged today
13. laanwj closed this on Jul 26, 2016

    ---

14. MarcoFalke locked this on Sep 8, 2021

Contributors
f139975 sipa btcdrak luke-jr gmaxwell laanwj

Labels
Mempool