Page MenuHomePhabricator

make per-tx sigops limit contextual
ClosedPublic

Authored by markblundeberg on Jan 27 2020, 11:23.

Details

Summary

The per-tx sigops check in CheckTransactionCommon (used by CheckCoinbase
and CheckRegularTransaction) happens without any context. To deactivate
sigops, just as with D5061, this needs to be moved into a contextual zone
that is aware of block index.

This is a fairly delicate change, so please review it carefully (and I
will be taking another very careful look at this myself before landing):
Although for most transactions this check is in fact optional, since
they will be rechecked in ConnectBlock using the more strict P2SH
counting, it cannot be removed naively since it is the *only* place
where the coinbase sigops limit is enforced! I've added a test for
that in D5073, and put a second check in ConnectBlock just for
good measure.

Note that like D5061 this is a behaviour change for old blocks,
since it now only counts the checkdatasig sigops for transactions
in blocks after that upgrade.

Also, as for mempool behaviour, previously we would ban for txns
with >20000 sigops but only reject >4000 sigops as nonstandard.
Now, we just reject both without banning. I think that's fine.

Depends on D5061 D5062 D5072 D5073.

Test Plan

ninja check-extended

Diff Detail

Repository
rABC Bitcoin ABC
Branch
txsigops_contextual
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9166
Build 16286: Default Diff Build & Tests
Build 16285: arc lint + arc unit

Event Timeline

src/validation.cpp
1892 ↗(On Diff #15807)

It should be possible to merge the coinbase sigops checks and this one together in some clever way, but it's not as simple as it looks on first glance: for regular transactions we can only call GetTransactionSigOpCount (with P2SH) after calling CheckTxInputs, because we need to know that all the input coins exist. So, we would want to make CheckTxInputs have a special case when provided with a coinbase txn, or something along those lines.

deadalnix added a subscriber: deadalnix.

I think the change to accept to memory pool would be better with a static_assert but overall, LGTM.

src/validation.cpp
695 ↗(On Diff #15807)

A static_assert that both value are consistent with each other is better here.

This revision is now accepted and ready to land.Jan 27 2020, 14:03
src/validation.cpp
695 ↗(On Diff #15807)

👍

update the ContextualCheckBlock comment (this diff adds a second
check of coinbase in ConnectBlock, so scary warning is not needed)

(reviewed it again myself, am happy with this)