Reject transactions with excessive numbers of sigops

petertodd commented at 4:23 am on May 8, 2014: contributor

What it says on the tin.

in src/main.cpp: in b89179b43b outdated

900-        // reasonable number of ECDSA signature verifications.
901+        // Check that the transaction doesn't have an excessive number of
902+        // sigops, making it impossible to mine. Since the coinbase transaction
903+        // itself can contain sigops MAX_TX_SIGOPS is less than
904+        // MAX_BLOCK_SIGOPS; we still consider this an invalid rather than
905+        // merely non-standard transaction.

luke-jr commented at 4:52 am on May 8, 2014:

How is this considered invalid? This code path is inherently about non-IsStandard.

laanwj commented at 3:15 pm on June 24, 2014:

I agree with @luke-jr here. INVALID is the wrong terminology. It doesn’t fail any consensus validation check, we just deny it into the mempool because of DoS risk.

in src/main.cpp: in b89179b43b outdated

903+        // itself can contain sigops MAX_TX_SIGOPS is less than
904+        // MAX_BLOCK_SIGOPS; we still consider this an invalid rather than
905+        // merely non-standard transaction.
906+        unsigned int nSigOps = GetLegacySigOpCount(tx);
907+        nSigOps += GetP2SHSigOpCount(tx, view);
908+        if (nSigOps > MAX_TX_SIGOPS)

luke-jr commented at 4:52 am on May 8, 2014:

I’m not sure a fixed limit makes sense here?

petertodd commented at 5:09 am on May 8, 2014:

Worrying about the exact difference between “invalid” and “non-standard” when the boundry is fuzzy seemed like quibbling to me. MAX_TX_SIGOPS is defined as 1/5th of MAX_BLOCK_SIGOPS; it’s already about three orders of magnitude greater than a normal transaction, and less than an order of magnitude away from the absolute limit. (which is impractical to actually use anyway - as I say coinbases use up sigops)

rebroad commented at 5:46 am on May 8, 2014:

Does this mean someone only needs to create five transactions with the max operations to cause the block limit to be reached?

gmaxwell commented at 5:51 am on May 8, 2014:

@rebroad Yes, and what about it? How transactions are prioritized is a separate matter from what should be considered at all.

petertodd commented at 5:51 am on May 8, 2014:

@rebroad Yup. OTOH the cost difference for the attacker isn’t huge, about an order of magnitude better than just buying p2p resources with fees, so it’s not an attack I’m very worried about. I actually did write and test up some quick code that just adjusts nSize so the fee calculations end up fair, but there isn’t a pressing need for it yet. Creating transactions that simply can’t be mined on the other hand is approximately ∞ cheaper than buying mempool RAM and network bandwidth with fees!

petertodd commented at 5:55 am on May 8, 2014:

Oh, and that’s also why I chose 1/5th of the total block sigops rather than trying to pick a more exact number closer to the absolute limit: Why invite attackers to exploit edge cases I didn’t think of? Five transactions is a lot more margin than trying to get exactly one.

petertodd commented at 5:36 am on May 8, 2014: contributor

To save you some time testing: https://github.com/petertodd/tx-flood-attack

gmaxwell commented at 5:39 am on May 8, 2014: contributor

I think we should probably introduce a new term for things which are not invalid but which no sane policy difference should change e.g.: E.g. version/nops/sane limits on SIGOPS count, canonization of pushes. We need to be careful about “invalid” since someone may accidentally incorporate it in a block validity rule.

petertodd commented at 5:40 am on May 8, 2014: contributor

@gmaxwell Agreed, especially given that IsStandard() may very well turn into a very permissive whitelist allowing everything but some carefully controlled edge cases and NOP functionality used for safe soft-forking.

sipa commented at 10:58 am on May 9, 2014: member

I have previous suggested using a more generic “resource usage” metric, defined as max(bytes / max_block_bytes, sigops / max_block_sigops), and use that for prioritization and fee calculations instead of just size. That would naturally limit standardness of high-sigop transactions, make mining code require appropriate fees for it, and make it compete fairly with others for “sigop space” in blocks.

jgarzik commented at 3:15 pm on May 9, 2014: contributor

+1 @sipa

petertodd commented at 11:44 pm on May 9, 2014: contributor

@jgarzik @sipa Agreed re: sigop space… but remember I deliberately didn’t include that in this pull-req to keep it simple and focused on solving the immediate problem of “don’t accept tx’s that simply can’t be mined”

sipa commented at 11:32 am on May 11, 2014: member

@petertodd Right; not a criticism on this pull request. As far as I can tell, it has the exact same effect, but on more than just mempool acceptance.

petertodd commented at 5:43 pm on May 11, 2014: contributor

@sipa Belt-and-suspenders; easy to write a resource usage metric that fails to notice a tx uses more resources than a block actually has!

But anyway, rewriting IsStandard() to IsSoftForkSafe() is on my todo list, and will need a sigops resource usage metric thing.

ashleyholman commented at 3:58 pm on May 12, 2014: contributor

I have done the following testing on testnet only. The patch appears to work as intended.

On unpatched master branch:
- TX with 39,060 sigops was accepted into the mempool, via RPC as well as via network peer.
- TX was not mineable
On patched master branch:
- TX with 39,060 sigops was rejected (“too many sigops” error logged, and reject message sent to peer / RPC client)
- TX’s with exactly 4000 (MAX_TX_SIGOPS) sigops or less were accepted.
- TX with 4020 sigops rejected
- All accepted TX’s were mineable

I used @petertodd’s tx-flood-attack script to generate the transactions.

As an aside, -debug=mempool does not log an “AcceptToMemoryPool: accepted” message for transactions accepted via RPC, but it does when received via a network tx message, so this was a bit confusing at first.

jgarzik commented at 2:10 am on June 7, 2014: contributor

I like the concept, and we really need to address sigops.

I don’t like making an arbitrary limit which is technically below that which could be mined. Thus it is a soft fork, one likely to escape notice. mempool accept includes local TXs as well as remote ones.

As such, leaning towards NAK. Want to find a better way to do the same thing.

petertodd commented at 2:26 am on June 7, 2014: contributor

Huh? This isn’t a soft-fork at all; AcceptToMemoryPool() isn’t called in the block validation code.

Anyway, it may be an arbitrary limit, but it’s a very high limit. Remember that we’re fixing an actual real-world vulnerability that is getting some exploitation attempts; we can create a better fix later with more permissive rules. The only transactions on mainnet that would have actually run into this limit are some data insertion ones from the wikileaks cables stuff.

petertodd commented at 2:29 am on June 7, 2014: contributor

Note BTW the similarities to the other arbitrary limit on max transaction size. Again it’s a limit almost no legit transaction runs into in practice, so essentially zero impact on actual users.

laanwj commented at 10:10 am on June 25, 2014: member

It is indeed not a softfork or any fork - it doesn’t touch the consensus code. It checks transactions before going into the mempool not transactions in blocks.

IMO these kinds of belt-and-suspenders DoS limits make sense.

ACK (except from returning INVALID which I think is confusing as it implies that a deeper validation limit is hit).

petertodd commented at 3:36 pm on June 25, 2014: contributor

@laanwj I can change the INVALID, although I’m out of the country again for another two weeks. If you want to do it yourself sooner feel free.

Reject transactions with excessive numbers of sigops 4fad8e6d83

petertodd commented at 2:21 pm on July 13, 2014: contributor

Fixed s/INVALID/NON_STANDARD/ nit.

Should be ready for merging.

BitcoinPullTester commented at 3:13 pm on July 13, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4150_4fad8e6d831729efa1965fa2034e7e51d3d0a1be/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj added the label TX scripts on Jul 31, 2014

laanwj merged this on Aug 12, 2014

laanwj closed this on Aug 12, 2014

laanwj referenced this in commit 8ebe42435a on Aug 12, 2014

jgarzik commented at 1:05 pm on August 12, 2014: contributor

EPROCESS. This appears to have been merged with only one ACK.

laanwj commented at 1:06 pm on August 12, 2014: member

OK - reverted again

jgarzik commented at 1:11 pm on August 12, 2014: contributor

Quickly poking people for ACKs on IRC seems like a reasonable alternative to reverting, if you want to go that route.

sipa commented at 1:12 pm on August 12, 2014: member

posthumous ACK

jgarzik commented at 1:17 pm on August 12, 2014: contributor

ut ACK

laanwj referenced this in commit 9ee09dc64f on Aug 13, 2014

jnewbery referenced this in commit 203a230565 on Apr 10, 2019

DrahtBot locked this on Sep 8, 2021

Reject transactions with excessive numbers of sigops #4150