Page MenuHomePhabricator

Remove Schnorr activation
ClosedPublic

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

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

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

rebase for D3262 & tweak accordingly

remove newline from timing.json

deadalnix requested changes to this revision.Jun 12 2019, 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.Jun 12 2019, 17:18
markblundeberg edited the summary of this revision. (Show Details)
markblundeberg edited the test plan for this revision. (Show Details)

rework into first of multiple parts

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

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

jasonbcox requested changes to this revision.Jun 15 2019, 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.Jun 15 2019, 00:09
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

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

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.

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.

deadalnix requested changes to this revision.Jun 17 2019, 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.Jun 17 2019, 00:23
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.

deadalnix added inline comments.
test/functional/abc-schnorr-activation.py
97 ↗(On Diff #9566)

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.

test/functional/abc-schnorr-activation.py
97 ↗(On Diff #9566)

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

test/functional/abc-schnorr-activation.py
97 ↗(On Diff #9566)

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

test/functional/abc-schnorr-activation.py
97 ↗(On Diff #9566)

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.

IBD with checkpoints=0 and assumevalid=0 passed ^

test/functional/abc-schnorr-activation.py
97 ↗(On Diff #9566)

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.Jun 23 2019, 16:07
test/functional/abc-schnorr-activation.py
97 ↗(On Diff #9566)

Yes, a couple days ago I was experimenting with writing up another test for something else and I tried the setup_clean_chain=False thing with generate(1) at the start. It is not only nicer, but it also is apparently faster!

This test doesn't need to be done the way it's done now, especially since it's not activation-related anymore.

test/functional/abc-schnorr-activation.py
97 ↗(On Diff #9566)

Yes it's definitely faster, because the "unclean" chain first start the nodes and build the blocks, then restart the nodes before running the test. These stop/start are wasting time.