Page MenuHomePhabricator

move sigops counting from CheckBlock to ContextualCheckBlock
ClosedPublic

Authored by markblundeberg on Jan 24 2020, 12:40.

Details

Summary

Upgrade features can't live in CheckBlock (which doesn't know anything
about the ancestors of the block), and have to live in ContextualCheckBlock.
So in order to deactivate sigops limits, it has to move.

(while we're at it, this simplifies a rather weird loop)

Test Plan

ninja check-extended

Diff Detail

Repository
rABC Bitcoin ABC
Branch
sigops_contextual
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9129
Build 16217: Default Diff Build & Tests
Build 16216: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Jan 25 2020, 13:06

The use of STANDARD_SCRIPT_VERIFY_FLAGS is a problem, everything else is all good.

src/validation.cpp
3758

Wow, this is a pretty big deal. Changing the standard flags shouldn't change this as it is consensus related. I know you did not introduce that problem, but please do not perpetuate it.

This revision now requires changes to proceed.Jan 25 2020, 13:06
src/validation.cpp
3758

Yeah actually in CheckBlock it sort of 'made sense' because it lacks context, so it at least needed hardcoded flags (though probably it should have been just hardcoded to either SCRIPT_VERIFY_NONE, or better SCRIPT_ENABLE_CHECKDATASIG_SIGOPS as we can get away with that). Here we can be contextual and consult the block script flags.

Either way it's no devastating mistake since this is just a prelim check that misses P2SH sigops, so missing some more sigops isn't hurting.

But, here's a funny one to think about: if we introduce multibyte opcodes, that changes the parsing of scripts, and therefore changes sigops count and would even increase the count for some scripts. There could be some real disasters in such a scenario.

src/validation.cpp
3758

It is a big problem, because someone modifying STANDARD_SCRIPT_VERIFY_FLAGS do not expect to have any impact on the consensus code, and the person reviewing it either.

It's like having a rake somewhere in your garden. You can argue that it's not in the path so nobody will walks on it, so it's not a problem and to some extent you'd be right. But leave it there long enough, and the probability that someone steps on it gets closer and closer to 1.

src/validation.cpp
3731 ↗(On Diff #15790)

I am thinking maybe GetNextBlockScriptFlags should accept nullptr and just return SCRIPT_VERIFY_NONE, instead of catching it here.

It seems that is a safe assumption in all cases: pindexPrev is null only when it's the genesis block.

src/validation.cpp
3758

Well in that case, grep for STANDARD_SCRIPT_VERIFY_FLAGS and see there are a couple more rakes waiting to be stepped on ;-P

deadalnix added inline comments.
src/validation.cpp
3731 ↗(On Diff #15790)

Interesting. IMO, you could also just pass down a static set of flags that contain the checkdatasig one, because I don't think any txn reached the limit, but this is "more correct".

Also, you should make this const.

This revision is now accepted and ready to land.Jan 27 2020, 01:02