Details

Reviewers

Group Reviewers

Restricted Project

Maniphest Tasks

Commits

rSTAGING0cfa675d41f9: deactivate sigops limits in phonon upgrade
rABC0cfa675d41f9: deactivate sigops limits in phonon upgrade

Summary

There are four sigops limits in validation, which all get deactivated here:

consensus per-block limit of 20000 per MB
consensus per-tx limit of 20000
standard per-tx limit of 4000
standard per-input limit of 15 for P2SH spends (only when -acceptnonstdtxn=0)

Additionally there is sigops tracking in mempool / mining:

mining respects the consensus per-block limit
virtualsize (priority) uses sigops

This Diff zeroes ALL sigops counts which indirectly means that all
limits are disabled. This is done using a flag analogous to the
checkdatasig counting flag, which is put into the consensus block
flags:

all validations after phonon upgrade will see 0 sigops, and
the mempool will be reprocessed, so all sigops counts will be 0 in mempool / mining.

Depends on D5060 D5061 D5062 D5076 D5077 D5160 D5168

Test Plan

ninja check-extended
ninja check-upgrade-activated-extended

Rerun ninja check-all with cmake .. -DBUILD_BITCOIN_WALLET=OFF.

Diff Detail

Repository

rABC Bitcoin ABC

Branch

deactivate_sigops

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 9275
Build 16491: Default Diff Build & Tests
Build 16490: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9131: Diff 15772.Jan 24 2020, 14:30

(this was actually missing one deactivation -- the per-tx limit, see D5076)

rebase, still need a proper activation test

Build Bitcoin-ABC / Diffs / Diff Testing started.

markblundeberg planned changes to this revision.Jan 30 2020, 10:48

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9195: Diff 15886.Jan 30 2020, 10:59

also deactivate P2SH sigops limit; added a test for all of this

Build Bitcoin-ABC / Diffs / Diff Testing started.

markblundeberg retitled this revision from [wip] deactivate sigops limits in phonon upgrade to deactivate sigops limits in phonon upgrade.Feb 3 2020, 02:01

markblundeberg edited the summary of this revision. (Show Details)

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9236: Diff 15951.Feb 3 2020, 02:06

markblundeberg added inline comments.Feb 3 2020, 02:07

src/policy/policy.cpp
164 ↗	(On Diff #15951)	Note that this other stuff gets bypassed too -- if we didn't have sigops limit then there would never have been a need to parse scriptsig like this. This does mean that seriously invalid txes (missing redeemscript) will be caught later during CheckInputs and likely be identified as banworthy instead of nonstandard, which is fine.

markblundeberg added a task: T704: sigChecks implementation.Feb 3 2020, 02:43

The approach is needlessly branchy. You can simplify this.

src/miner.cpp
261 ↗	(On Diff #15951)	Passing a bool al over the place is messy and not really good. IMO you are better off creating a member function that return either `GetMaxBlockSigOpsCount` or the sigcheck limit depending on activation state.
src/policy/policy.cpp
164 ↗	(On Diff #15951)	This is not good and open DoS, for instance using the NOPs opcodes. The only way you fix this is by making signature validation lazy. In the meantime, this stays.
src/validation.cpp
1868 ↗	(On Diff #15951)	You can have the proper flag make this return 0, which avoid a bunch of logic.
3795 ↗	(On Diff #15951)	dito

This revision now requires changes to proceed.Feb 3 2020, 18:27

Yeah the specific boolean-based thing is going to be reworked. For the most part I just want to get review on which things are being deactivated (have I missed any), and the content of the deactivation tests.

src/miner.cpp
261 ↗	(On Diff #15951)	True, BlockAssembler already has chain context variables so it can go there.
src/policy/policy.cpp
164 ↗	(On Diff #15951)	I don't understand, can you be more specific? It's not possible for OP_NOP to be in scriptSig, remember this function only gets called in ATMP after `IsStandardTx` which already enforced push-only scriptSig.

deadalnix requested changes to this revision.Feb 4 2020, 15:14

deadalnix added inline comments.

src/policy/policy.cpp
164 ↗	(On Diff #15951)	Ok then it happens to work in that case, but it is still really bad an this discussion is the confirmation it is. This is a form of temporal coupling: https://hackernoon.com/api-design-temporal-coupling-2c1687173c7c This function as changed expects to be called in a specific order with other functions, effectively baking in knowledge about the whole software, and changing the rest of the software can introduce a DoS vector in there. This is pretty much bound to happen because this code you assume the behavior of is completely invisible here - and vice versa. You know it because you just worked on that part of the software, nobody else knows it, and you won't remember it in 6 month.

This revision now requires changes to proceed.Feb 4 2020, 15:14

markblundeberg added inline comments.Feb 4 2020, 15:37

src/policy/policy.cpp
164 ↗	(On Diff #15951)	Hmm I think you might have it backwards ... the function as it works today assumes temporal coupling: EvalScript is run assuming it is safe to do so. Once fLimitSigOpCount has been activated to false, then the coupling disappears and this part never runs. In 6 months this EvalScript won't be here, assuming these reviews go smoothly.

updated to use a script flag instead.

Two more functional tests had to be disabled post-upgrade since now
the in-mempool sigops count has been nuked too:

abc-magnetic-anomaly-mining which wants to track ordering of fee / sigops information (and expects nonzero)
abc-sigops-mempool-mining which looks at intricate virtualsize mechanics, using high-sigop transactions.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9275: Diff 16028.Feb 5 2020, 12:42

markblundeberg edited the summary of this revision. (Show Details)Feb 5 2020, 16:07

markblundeberg added inline comments.

test/functional/abc-magnetic-anomaly-mining.py
32	(Funnily enough, this test can get re-enabled as-is once sigchecks counting is in place, because it uses only transactions where sigops == sigchecks.)

markblundeberg edited the summary of this revision. (Show Details)Feb 5 2020, 17:35

markblundeberg added a parent revision: D5162: Fix flakey feature_pruning.

While there is nothing wrong with that patch per se, it would be better to get the sigcheck nailed before landing this.

In D5029#124801, @deadalnix wrote:

While there is nothing wrong with that patch per se, it would be better to get the sigcheck nailed before landing this.

Yep, these will be landed together or not at all. (The block sigchecks one does depend on this for weird testing reasons though, so I can't do it the other way around.)

markblundeberg added inline comments.Feb 6 2020, 01:13

test/functional/abc-sigops-mempool-mining.py
109	This test does deserve a counterpart in sigchecks land, probing how sigchecks virtual sizes work out. It's going to be quite different beast though.