Page MenuHomePhabricator
Differential D2483

Schnorr signature activation
ClosedPublic
Actions

Authored by markblundeberg on Feb 2 2019, 20:25.

Details

Reviewers
deadalnix
jasonbcox
Fabien
Group Reviewers
Restricted Project
Maniphest Tasks
T527: Add Schnorr support to OP_CHECKSIG and OP_CHECKDATASIG
Commits
rSTAGING6bb69585f326: Schnorr signature activation
rABC6bb69585f326: Schnorr signature activation
Summary

Includes python activation tests based on replay protection activation.

Features:

  • Tests that when upgrade / downgrade happens, newly-invalid transactions are dumped from mempool (but not valid ones).
  • Don't ban peers who send a tx that would be valid under the mandatory flags on other side of upgrade/downgrade.
  • Tests of 64-byte DER signatures
Test Plan

run functional tests
uncomment fakeDER64 and patch VerifySignature (see comment in file about fakeDER64)
run abc-schnorr-activation.py again

(I have also run these tests with the various critical bits of mainline code cancelled out, and verified the tests fail as expected.)

Diff Detail

Repository
rABC Bitcoin ABC
Branch
schnorr_activation
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4937
Build 7937: Bitcoin ABC Buildbot (legacy)
Build 7936: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
markblundeberg added a comment.Feb 10 2019, 17:16

address comments; fix up variable names

Also changed upgrade-mismatch mempool acceptance error from standardness to a more appropriate error message.

teamcity added a comment.Feb 10 2019, 17:17

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

markblundeberg marked 8 inline comments as done.Feb 10 2019, 17:17
teamcity added a comment.Feb 10 2019, 17:32

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B4952: Diff 7290!Feb 10 2019, 17:32
teamcity added a comment.Feb 10 2019, 17:34

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

markblundeberg edited the summary of this revision. (Show Details)Feb 10 2019, 17:39
markblundeberg edited the test plan for this revision. (Show Details)
teamcity added a comment.Feb 10 2019, 17:48

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4952: Diff 7290.Feb 10 2019, 17:48
jasonbcox added inline comments.Feb 11 2019, 22:15
test/functional/abc-schnorr-activation.py
286 ↗(On Diff #7290)

The commend lines with GREAT_WALL_START_TIME + x fill up reading space. I think removing them is best. Readers can easily refer to next_block in this same file to see what the timestamps are.

3

Mengerian is correct here

13

I see. thanks.

37

got it. and yes, the names could improve :P

340

👍

markblundeberg updated this revision to Diff 7306.Feb 12 2019, 00:34

rebased; shorten comment

markblundeberg marked an inline comment as done.Feb 12 2019, 00:34
markblundeberg added inline comments.
test/functional/abc-schnorr-activation.py
37

let me know what you think of the new error names

teamcity added a comment.Feb 12 2019, 00:34

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Feb 12 2019, 00:48

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4961: Diff 7306.Feb 12 2019, 00:48
Fabien requested changes to this revision.Feb 12 2019, 11:32
Fabien added a subscriber: Fabien.

Bunch of nits in the test

test/functional/abc-schnorr-activation.py
30 ↗(On Diff #7306)

Add spaces: GREAT_WALL_START_TIME * 2

73 ↗(On Diff #7290)

Move the comment above

146 ↗(On Diff #7290)

This is not necessary, as it is done by make_conform_to_ctor

164 ↗(On Diff #7290)

Nit: self.nodes[0] is not a function

185 ↗(On Diff #7290)

Use spaces b"Schnorr!" * 4

188 ↗(On Diff #7290)

Move comment above

235 ↗(On Diff #7290)

This is convention only, but _ is usually the name of an unused variable, which is not the case here

This revision now requires changes to proceed.Feb 12 2019, 11:32
markblundeberg updated this revision to Diff 7310.Feb 12 2019, 15:18

fix nits that Fabien pointed out

teamcity added a comment.Feb 12 2019, 15:19

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Feb 12 2019, 15:33

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4965: Diff 7310.Feb 12 2019, 15:33
markblundeberg mentioned this in D2539: nits in abc-replay-protection.Feb 12 2019, 16:07
Diffusion mentioned this in rABC43e73d0dfed6: nits in abc-replay-protection.Feb 12 2019, 16:58
Fabien added inline comments.Feb 12 2019, 17:32
test/functional/abc-schnorr-activation.py
58 ↗(On Diff #7310)

Thinking out loud : maybe a debug flag could be added to bypass the script evaluation, allowing for testing the fakeDER64 signature in the test in a permanent manner ? Maybe only allowed if the node is running on regtest.
Thought ?

jasonbcox accepted this revision.Feb 12 2019, 23:36
jasonbcox added inline comments.
test/functional/abc-schnorr-activation.py
37

My critique of the naming is it sounds like "before an error" and "after an error" because it doesn't say what it's before and after. just a nit though

Fabien added inline comments.Feb 13 2019, 09:09
test/functional/abc-schnorr-activation.py
85 ↗(On Diff #7310)

Please use format()instead

86 ↗(On Diff #7310)

dito

markblundeberg updated this revision to Diff 7322.Feb 13 2019, 16:28

rebase after D2527 landing

teamcity added a comment.Feb 13 2019, 16:29

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

markblundeberg removed a parent revision: D2527: Improved mempool handling when changing validation rulesets.Feb 13 2019, 16:36
teamcity added a comment.Feb 13 2019, 16:43

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4974: Diff 7322.Feb 13 2019, 16:43
markblundeberg added inline comments.Feb 13 2019, 17:36
test/functional/abc-schnorr-activation.py
26 ↗(On Diff #7322)

Now that the replay protection has been postponed to the graviton upgrade date, does it make sense to use the real Great Wall time for this test?

markblundeberg added inline comments.Feb 13 2019, 17:42
test/functional/abc-schnorr-activation.py
58 ↗(On Diff #7310)

Seems a bit too complicated for what we want, though indeed such a test would be more robust.

Note that this will anyway be removed in May. It's only because of that that I'm comfortable having it done in this awkward way.

85 ↗(On Diff #7310)

👍 will do

86 ↗(On Diff #7310)

👍 will do

37

yeah the same thing was bugging at me, so I think it should be changed. :-)

markblundeberg mentioned this in D2479: Add activation code for SEGWIT_RECOVERY.Feb 13 2019, 21:55
markblundeberg planned changes to this revision.Feb 14 2019, 00:17
markblundeberg marked 3 inline comments as done.Feb 14 2019, 01:59
markblundeberg marked an inline comment as done.
markblundeberg updated this revision to Diff 7334.Feb 14 2019, 02:13
markblundeberg added a parent revision: D2527: Improved mempool handling when changing validation rulesets.

various:

  • rebase on now-landed D2479 (+ address conflicts in overlap)
  • now fully mock-timed
  • added actual mininode testing of non-banning (borrowed from D2479)
  • added a reorg test at end, for good measure
  • nits
teamcity added a comment.Feb 14 2019, 02:13

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

markblundeberg edited the summary of this revision. (Show Details)Feb 14 2019, 02:15
markblundeberg edited the test plan for this revision. (Show Details)
teamcity added a comment.Feb 14 2019, 02:27

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4983: Diff 7334.Feb 14 2019, 02:27
Fabien accepted this revision.Feb 14 2019, 20:26
deadalnix requested changes to this revision.Feb 14 2019, 22:57

Ok the general flow of the code and test looks good. However I'm not convinced that the check for banning actually checks anything. Can you remove the whitelist and then make sure to test manually that the node gets banned when sending a transaction that is actually invalid, for instance a schnorr multisig transaction?

test/functional/abc-schnorr-activation.py
96 ↗(On Diff #7334)

If 127.0.0.1 is whitelisted, does the banning test actually test anything ? I'm not confident it does.

200 ↗(On Diff #7334)

:)

296 ↗(On Diff #7334)

Please add a list of transaction as a parameter to block(). That would be better than reproducing the same pattern all the time.

462 ↗(On Diff #7334)

Well, as long as things do not get too deep :)

This revision now requires changes to proceed.Feb 14 2019, 22:57
markblundeberg updated this revision to Diff 7354.Feb 15 2019, 03:01
  • add extra node for doing actual ban-checking (whitelist was stopping this)
  • include positive ban tests (for schnorr multisig)
  • get rid of update_block
teamcity added a comment.Feb 15 2019, 03:02

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

deadalnix accepted this revision.Feb 15 2019, 03:06
This revision is now accepted and ready to land.Feb 15 2019, 03:06
teamcity added a comment.Feb 15 2019, 03:16

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4995: Diff 7354.Feb 15 2019, 03:16
Closed by commit rABC6bb69585f326: Schnorr signature activation (authored by Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com>). · Explain WhyFeb 15 2019, 03:19
This revision was automatically updated to reflect the committed changes.
markblundeberg mentioned this in D3253: Remove Schnorr activation.Jun 8 2019, 01:41
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> mentioned this in rABCfbcfe01efdec: Remove Schnorr activation.Jun 26 2019, 00:51
markblundeberg mentioned this in D3736: New Schnorr multisig activation.Jul 25 2019, 05:29
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> mentioned this in rABC2a1e1d244b1b: New Schnorr multisig activation.Aug 2 2019, 01:03
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> mentioned this in rSTAGING43e73d0dfed6: nits in abc-replay-protection.Sep 18 2019, 22:22
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> added a commit: rSTAGING6bb69585f326: Schnorr signature activation.Sep 18 2019, 22:25
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> mentioned this in rSTAGINGfbcfe01efdec: Remove Schnorr activation.Sep 19 2019, 00:33
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> mentioned this in rSTAGING2a1e1d244b1b: New Schnorr multisig activation.Sep 19 2019, 01:33

Revision Contents
Changeset List

PathSize
src/
35 lines
test/
functional/
381 lines

Diff 7265

View Options

src/validation.cpp

Loading...
View Options

test/functional/abc-schnorr-activation.py

Loading...