-bytespersigop option to additionally limit sigops in transactions we relay and mine #7081

pull luke-jr wants to merge 2 commits into bitcoin:master from luke-jr:bytespersigop changing 3 files +14 −8

luke-jr commented at 3:09 am on November 23, 2015: member
NicolasDorier commented at 10:03 am on November 23, 2015: contributor

glad to see that at least we would have a mitigation path in case of sigop spam attack. The worried me a bit.
NicolasDorier commented at 10:16 am on November 23, 2015: contributor

What is the rational on the default of 20 though ?

During current rules, if all transactions max out the MAX_STANDARD_TX_SIGOPS (4000) we can get 5 transactions in the block.

In size, the maximum those 5 transactions would be is 20000 bytes. This is 5 bytesperop currently. (without this option)

Not sure if it is relevant though. Just interested to know why you chose 20.
jonasschnelli commented at 12:01 pm on November 23, 2015: contributor

Concept ACK
jgarzik commented at 12:59 pm on November 23, 2015: contributor

ut ACK
laanwj added the label Mempool on Nov 24, 2015
laanwj commented at 1:16 pm on November 27, 2015: member

Concept ACK

in src/init.cpp: in d0827044a7 outdated

448@@ -449,6 +449,7 @@ std::string HelpMessage(HelpMessageMode mode)
449     strUsage += HelpMessageGroup(_("Node relay options:"));
450     if (showDebug)
451         strUsage += HelpMessageOpt("-acceptnonstdtxn", strprintf("Relay and mine \"non-standard\" transactions (%sdefault: %u)", "testnet/regtest only; ", !Params(CBaseChainParams::TESTNET).RequireStandard()));
452+    strUsage += HelpMessageOpt("-bytespersigop", strprintf(_("Maximum rate of sigops per byte in transactions we relay and mine (default: %u)"), nBytesPerSigOp));

sipa commented at 12:50 pm on November 28, 2015:

Can you use a constant DEFAULT_MAX_BYTES_PER_SIGOP instead here?

sipa commented at 12:51 pm on November 28, 2015: member

utACK
sipa commented at 9:38 pm on November 28, 2015: member

Needs trivial rebase.
luke-jr force-pushed on Nov 29, 2015
luke-jr commented at 0:02 am on November 29, 2015: member

Nit addressed, and rebased.
osogama commented at 0:04 am on November 29, 2015: none

Ggggggbgh

On Saturday, November 28, 2015, Luke-Jr notifications@github.com wrote:

Nit addressed, and rebased.

— Reply to this email directly or view it on GitHub #7081 (comment).

Sent from Gmail Iphone
sdaftuar commented at 11:37 am on November 29, 2015: member

I haven’t thought hard enough yet about the value of 20 as a default, but conceptually I think something like this makes sense. Could you add a unit test or RPC test that exercises the new logic?
dcousens commented at 4:27 am on November 30, 2015: contributor

utACK
luke-jr force-pushed on Dec 1, 2015
gmaxwell commented at 11:06 am on December 1, 2015: contributor

ACK, with nit. Do something about that help message.
gmaxwell added this to the milestone 0.12.0 on Dec 1, 2015
gmaxwell added this to the milestone 0.11.0 on Dec 1, 2015
gmaxwell removed this from the milestone 0.12.0 on Dec 1, 2015
-bytespersigop option to additionally limit sigops in transactions we relay and mine 45b8e278fb

in src/init.cpp: in 90e7a8f360 outdated

474@@ -475,6 +475,7 @@ std::string HelpMessage(HelpMessageMode mode)
475     strUsage += HelpMessageGroup(_("Node relay options:"));
476     if (showDebug)
477         strUsage += HelpMessageOpt("-acceptnonstdtxn", strprintf("Relay and mine \"non-standard\" transactions (%sdefault: %u)", "testnet/regtest only; ", !Params(CBaseChainParams::TESTNET).RequireStandard()));
478+    strUsage += HelpMessageOpt("-bytespersigop", strprintf(_("Maximum rate of sigops per byte in transactions we relay and mine (default: %u)"), DEFAULT_BYTES_PER_SIGOP));

MarcoFalke commented at 12:22 pm on December 1, 2015:

Nit: per @gmaxwell @luke-jr Also needs rebase

luke-jr force-pushed on Dec 1, 2015
luke-jr commented at 8:58 pm on December 1, 2015: member

Done
morcos commented at 9:33 pm on December 1, 2015: member

utACK but agree with previous nit that it would be nice to have a brief explanation of the choice of default.
dcousens commented at 2:42 am on December 2, 2015: contributor

re-ACK
laanwj added this to the milestone 0.12.0 on Dec 3, 2015
laanwj removed this from the milestone 0.11.0 on Dec 3, 2015
laanwj added the label Needs backport on Dec 3, 2015
laanwj commented at 11:44 am on December 3, 2015: member

Changed milestone back to 0.12, added “Needs backport” label as it needs backport to 0.11
btcdrak commented at 3:21 pm on January 8, 2016: contributor

utACK 45b8e27 but would be nice to see some RPC tests.
laanwj commented at 3:36 pm on January 9, 2016: member

@luke-jr Not sure what you want to do here instead of rebasing, but this somehow needs to be made suitable for merge if you still want it included in 0.12
luke-jr force-pushed on Jan 9, 2016
Merge branch bytespersigop fdc202f4b0
luke-jr force-pushed on Jan 9, 2016
laanwj merged this on Jan 9, 2016
laanwj closed this on Jan 9, 2016
laanwj referenced this in commit dd1304ec21 on Jan 9, 2016
laanwj commented at 8:09 am on February 4, 2016: member

This has been backported to 0.12 via #7323, removing “needs backport” label
laanwj removed the label Needs backport on Feb 4, 2016
ScroogeMcDuckButWithBitcoin commented at 6:14 pm on May 28, 2016: none

So, are we metacoin guys supposed to go back to p2pkh encoding - or should we just fragment our user’s wallets into more inputs, before continuing to use op_multisig encodings?
morcos commented at 5:28 pm on June 8, 2016: member

There were repeated requests in this PR for a justification of the limit chosen which were ignored. It was a mistake to merge the PR without first understanding why 20 was the right number. I apologize for not being more forceful about it at the time.
sipa commented at 5:42 pm on June 8, 2016: member

I think we should change the behavior so that transactions with too high sigops are simply treated as if their corresponding size was larger too.
ScroogeMcDuckButWithBitcoin commented at 5:46 pm on June 8, 2016: none

For what it’s worth, I’ve reverted to pub key hash encoding for the time being: https://github.com/17Q4MX2hmktmpuUKHFuoRmS5MfB5XPbhod/dropzone_ruby/blob/master/lib/dropzone/connection.rb#L167-L173

I’d rather not engage in a cat and mouse game on this, and I would expect that doing so would probably just further waste the resources that you guys are generally try to protect.
MarcoFalke locked this on Sep 8, 2021

Contributors
luke-jr NicolasDorier jonasschnelli jgarzik laanwj sipa osogama sdaftuar dcousens gmaxwell MarcoFalke morcos btcdrak ScroogeMcDuckButWithBitcoin

Labels
Mempool

Milestone
0.12.0