Page MenuHomePhabricator

deactivate sigops limits in phonon upgrade

Authored by markblundeberg on Jan 20 2020, 11:57.



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

  • 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

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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

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

rebase, still need a proper activation test

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

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

deadalnix requested changes to this revision.Feb 3 2020, 18:27

The approach is needlessly branchy. You can simplify this.

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.

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.

1868 ↗(On Diff #15951)

You can have the proper flag make this return 0, which avoid a bunch of logic.

3795 ↗(On Diff #15951)


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.

261 ↗(On Diff #15951)

True, BlockAssembler already has chain context variables so it can go there.

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

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

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

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.
markblundeberg added inline comments.
32 ↗(On Diff #16028)

(Funnily enough, this test can get re-enabled as-is once sigchecks counting is in place, because it uses only transactions where sigops == sigchecks.)

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

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

109 ↗(On Diff #16028)

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.

This revision is now accepted and ready to land.Feb 6 2020, 15:32

rebase with test lint changes

update functional test to work with nowallet build