Page MenuHomePhabricator

add a flag that (if unset) zeroes sigchecks reported by VerifyScript
ClosedPublic

Authored by markblundeberg on Jan 14 2020, 12:34.

Details

Summary

We want all the sigchecks counting/caching machinery to be functioning and in
place before the upgrade activation, however we don't actually want sigchecks
to be nonzero until then:

  • prior to upgrade the sigchecks total could be too large for the cache, and we definitely want high-sigchecks txns to be cached.
  • it's easier to have the block sigchecks limit fully in place, but then have nothing counting towards it

Effectively, we will be able to simply flip on this flag to cause the
activation of sigchecks limits in consensus and in block template creation.

This flag can be removed (always-on) as soon as the upgrade is past and we
impose a retroactive consensus sigchecks limit.

Depends on D4920

Test Plan

ninja check

Diff Detail

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

Event Timeline

markblundeberg created this revision.Jan 14 2020, 12:34
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 14 2020, 12:34
deadalnix accepted this revision.Jan 15 2020, 15:40

Accepting, but consider doing the xor thing and that this may end up requiring changes due to changes requested in parents.

src/test/sigcheckcount_tests.cpp
322 ↗(On Diff #15442)

You can xor the expected result so that you know you always set to something different.

This revision is now accepted and ready to land.Jan 15 2020, 15:40
markblundeberg updated this revision to Diff 15681.EditedJan 20 2020, 06:13
markblundeberg edited the summary of this revision. (Show Details)

remove dependency on D4834 , use xor

markblundeberg requested review of this revision.Jan 20 2020, 06:14

@deadalnix returning for review due to slight changes. Note after our discussion that ScriptExecutionMetrics will remain a pure struct (as it is in this diff) since accumulation etc will happen elsewhere.

deadalnix requested changes to this revision.Jan 20 2020, 18:56

It would reverse the metric setting logic as to avoid a double negation. Everything else is good to go.

src/script/interpreter.cpp
1757 ↗(On Diff #15681)

I would reverse the logic as follow:

1/ Set metricsOut as 0.
2/ Compute things using metrics.
3/ If the flag is set, metricsOut = metrics; before success.

This is more straightforward.

src/test/sigcheckcount_tests.cpp
318 ↗(On Diff #15681)

The ^ 0 isn't really necessary, but it's no big deal either way.

323 ↗(On Diff #15681)

likestamp

This revision now requires changes to proceed.Jan 20 2020, 18:56
markblundeberg added inline comments.Jan 21 2020, 03:16
src/script/interpreter.cpp
1757 ↗(On Diff #15681)

If there were other things in metrics, I wouldn't be wanting to zero them all with this flag. It's the same for now yeah, but I think the current way is more clear.

markblundeberg requested review of this revision.Jan 21 2020, 03:16
deadalnix accepted this revision.Jan 22 2020, 02:05
deadalnix added inline comments.
src/script/interpreter.cpp
1757 ↗(On Diff #15681)

I won't block for this, but I still think the double negation is a bit confusing.

This revision is now accepted and ready to land.Jan 22 2020, 02:05