Page MenuHomePhabricator

make per-tx sigops limit contextual

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


Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
rSTAGINGd6e89de94369: make per-tx sigops limit contextual
rABCd6e89de94369: make per-tx sigops limit contextual

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

rABC Bitcoin ABC
Lint Not Applicable
Tests Not Applicable

Event Timeline

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.

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
695 ↗(On Diff #15807)


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

markblundeberg added a task: Restricted Maniphest Task.Jan 28 2020, 09:31

(reviewed it again myself, am happy with this)