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

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.

@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.

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.

+1 @sipa

@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"

@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.

@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.

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.

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.

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.

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.

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).

@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.

Fixed s/INVALID/NON_STANDARD/ nit.

Should be ready for merging.

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.

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

OK - reverted again

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

posthumous ACK

ut ACK