Page MenuHomePhabricator

Remove Schnorr activation
AcceptedPublic

Authored by markblundeberg on Fri, Jun 7, 03:02.

Details

Reviewers
Mengerian
jasonbcox
Fabien
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
T653: Clean up past upgrades
Summary

This sets the SCRIPT_ENABLE_SCHNORR flag to be on for all time, retroactively.
The Schnorr activation test is simplified to a feature test.

(effectively this is a backing-out of D2483)

Test Plan

make check
test_runner.py --extended

  • IBD with -checkpoints=0 and -assumevalid=0 on testnet
  • IBD with -checkpoints=0 and -assumevalid=0 on mainnet

Diff Detail

Repository
rABC Bitcoin ABC
Branch
remschnorr
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6427
Build 10901: Bitcoin ABC Teamcity Staging
Build 10900: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
markblundeberg added inline comments.Sat, Jun 8, 03:05
src/script/interpreter.cpp
1464 ↗(On Diff #9239)

Ah ok yeah, that's in D3254 though I guess I could squash all that in if people want.

Mengerian accepted this revision as: Mengerian.Sat, Jun 8, 04:17

Did another pass over the Diff today, it looks good to me.

src/script/interpreter.cpp
1464 ↗(On Diff #9239)

Oh, I see... didn't realize that was also in D3254, in addition to the sigcache stuff.

I'm fine with doing it in a subsequent Diffs.

markblundeberg edited the test plan for this revision. (Show Details)Sat, Jun 8, 15:05

IBD successful on both nets.

markblundeberg added inline comments.Sat, Jun 8, 15:17
test/functional/timing.json
414 ↗(On Diff #9239)

note to self: drop this from diff.

note: will probably rebase this onto D3262, supposing people are in for that one.

markblundeberg updated this revision to Diff 9278.Sun, Jun 9, 23:55

rebase for D3262 & tweak accordingly

remove newline from timing.json

deadalnix requested changes to this revision.Wed, Jun 12, 17:18

It is very hard to ensure this is correct.

The first step here is to make sure the flag is a standard and mandatory flag and check for this. Once this is the case, each use of the flag can be removed one by one.

This revision now requires changes to proceed.Wed, Jun 12, 17:18
markblundeberg edited the summary of this revision. (Show Details)Fri, Jun 14, 03:14
markblundeberg edited the test plan for this revision. (Show Details)
markblundeberg updated this revision to Diff 9424.

rework into first of multiple parts

markblundeberg added a comment.EditedFri, Jun 14, 04:26

Now split out into three parts: D3253 (this) and D3331 and then D3332. The final result after D3332 is identical to unsplit version.

markblundeberg added a comment.EditedFri, Jun 14, 04:53

(Note that the functional test is identical to the prior version of this diff. This diff changes all externally-observable behaviour, and the follow ups just perform internal cleanup.)

markblundeberg edited the test plan for this revision. (Show Details)Fri, Jun 14, 16:30
jasonbcox requested changes to this revision.Sat, Jun 15, 00:09
jasonbcox added inline comments.
test/functional/abc-schnorr.py
1 ↗(On Diff #9234)

It's hard to tell if you made edits to this file. Can you hold the rename until a later diff and just apply any changes you made? That way, we can review the edits and then the name change is a simple, no-line-changes diff.

test/functional/timing.json
55 ↗(On Diff #9424)

ditto

This revision now requires changes to proceed.Sat, Jun 15, 00:09
jasonbcox added inline comments.Sat, Jun 15, 00:20
src/script/standard.h
53 ↗(On Diff #9424)

Reviewer note: this is cleaned up in D3332

src/validation.cpp
1596 ↗(On Diff #9424)

Reviewer note: This is cleaned up in D3332

markblundeberg added inline comments.Sat, Jun 15, 00:32
test/functional/abc-schnorr.py
1 ↗(On Diff #9234)

Sure, I'll give that a shot!

keep functional test same name to allow diff to show properly

markblundeberg added inline comments.Sat, Jun 15, 01:52
test/functional/abc-schnorr-activation.py
66 ↗(On Diff #9472)

Note, this whole fakeDER64 thing is now a bit unnecessary due to D3262 which does a proper test of this stuff. May be worth taking this fakeDER64 out too, but at least it serves as an invalid Schnorr signature.

markblundeberg updated this revision to Diff 9479.EditedSun, Jun 16, 00:53

update the functional test per my last comment; we can't properly test 64 byte ECDSA here,
but we can test the multisig length check banning.

markblundeberg edited the test plan for this revision. (Show Details)Sun, Jun 16, 00:57

IBD testing done

deadalnix requested changes to this revision.Mon, Jun 17, 00:23
deadalnix added inline comments.
src/validation.cpp
1591 ↗(On Diff #9479)

likestamp

test/functional/abc-schnorr-activation.py
52 ↗(On Diff #9479)

Don't make test dependent on when they run.

This revision now requires changes to proceed.Mon, Jun 17, 00:23

remove mocktiming

markblundeberg added inline comments.Mon, Jun 17, 03:08
test/functional/abc-schnorr-activation.py
52 ↗(On Diff #9479)

OK, makes sense, the mocktime thing is removed now. And test now uses block_time = int(time.time()) + 1 below inside next_block, which seems to be a very common pattern in other python tests.

simple rebase

deadalnix accepted this revision.Sat, Jun 22, 20:46
deadalnix added inline comments.
test/functional/abc-schnorr-activation.py
97

This should probably be avoided as well. I know it is a common pattern, but this is a bad one.

Wait, does that pass IBD? There may be some problem with the sigops count. Please double check before landing.

jasonbcox added inline comments.Sun, Jun 23, 00:15
test/functional/abc-schnorr-activation.py
97

Wouldn't it be better to just pick a time in the past and hardcode that here?

markblundeberg added inline comments.Sun, Jun 23, 01:08
test/functional/abc-schnorr-activation.py
97

Hmm, I'll give that past-time thing a try.

markblundeberg added inline comments.Sun, Jun 23, 05:35
test/functional/abc-schnorr-activation.py
97

Interesting, it doesn't work. What happens is that node0 never relays the blocks to node1. The test then fails on sync_blocks(self.nodes), line 262.

A brief look in the debug logs shows that node0 never leaves IBD, presumably since it's getting ancient blocks.

jasonbcox accepted this revision.Sun, Jun 23, 16:07

IBD with checkpoints=0 and assumevalid=0 passed ^

test/functional/abc-schnorr-activation.py
97

You can node.generate(1) at the very beginning of the test to bring it out of IBD. Alternatively, don't set this line self.setup_clean_chain = True and just extend the test chain from the latest block time. Both of these approaches require some tweaking of values elsewhere in the test though.

As Amaury said, this pattern is common, but bad. I think we should move on from this diff and work to solve this problem holistically in the future.

This revision is now accepted and ready to land.Sun, Jun 23, 16:07