Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overwinter SignatureHash #2903

Merged
merged 18 commits into from Feb 20, 2018
Merged

Overwinter SignatureHash #2903

merged 18 commits into from Feb 20, 2018

Conversation

@str4d
Copy link
Contributor

str4d commented Jan 25, 2018

Implements ZIP 143.

Includes code cherry-picked from the following upstream PRs:

Part of #2074 and #2254. Closes #1408 and #2584.

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Jan 25, 2018

This is a rebased and cleaned-up version of #2896. Still todo:

  • Rebase on top of activation logic.
  • Implement version-handling (making parts of the transaction validation branch ID-aware).
  • Switch inner hashes to use BLAKE2b-256.
  • Rebase on top of transaction format changes.
  • Add / update tests.
  • Update zcbenchmark to use new transaction format with caching.
@str4d str4d force-pushed the str4d:1408-sighash branch 2 times, most recently from 74a57e1 to e92418b Feb 1, 2018
@str4d str4d self-assigned this Feb 2, 2018
@str4d str4d force-pushed the str4d:1408-sighash branch 3 times, most recently from 516a278 to 7eeb9fe Feb 2, 2018
@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Feb 3, 2018

Note to reviewers: do not trust the commit ordering on GitHub, it does not handle the rebasing well.

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Feb 3, 2018

Just to check that nothing was horribly broken, I adjusted zcbenchmark to use the more efficient sighash, and compared it to current master. Results (measured on my dev laptop, so take values with a grain of salt):

Current SigHash

[
  {
    "runningtime": 0.122434
  },
  {
    "runningtime": 0.117106
  },
  {
    "runningtime": 0.117861
  },
  {
    "runningtime": 0.120877
  },
  {
    "runningtime": 0.119742
  }
]

New SigHash

[
  {
    "runningtime": 0.118283
  },
  {
    "runningtime": 0.108241
  },
  {
    "runningtime": 0.106504
  },
  {
    "runningtime": 0.110444
  },
  {
    "runningtime": 0.109168
  }
]

New SigHash, with caching

[
  {
    "runningtime": 0.041709
  },
  {
    "runningtime": 0.042964
  },
  {
    "runningtime": 0.040389
  },
  {
    "runningtime": 0.04138
  },
  {
    "runningtime": 0.041503
  }
]
@str4d str4d force-pushed the str4d:1408-sighash branch 2 times, most recently from 9d5c135 to 44212dd Feb 3, 2018
@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Feb 5, 2018

Once I rebase this on #2925 (so all the underlying pieces are in place, and this code can be finished), I will collect benchmark data for both the old and new SignatureHash as a function of the number of inputs, to verify that this PR does indeed address the performance problem sufficiently.

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 7, 2018

☔️ The latest upstream changes (presumably #2898) made this pull request unmergeable. Please resolve the merge conflicts.

@str4d str4d force-pushed the str4d:1408-sighash branch from 44212dd to e86af3a Feb 8, 2018
str4d and others added 7 commits Feb 2, 2018
… parameter

We do not need to be able to calculate multiple SignatureHash versions for a
single transaction format; instead, we use the transaction format to determine
the SigVersion.

The consensus branch ID *does* need to be passed in from the outside, as only
the caller knows the context in which the SignatureHash is being calculated
(ie. mempool acceptance vs. block validation).

JoinSplit signature verification has been moved into ContextualCheckTransaction,
where the consensus branch ID can be obtained.

The argument to the sign command for zcash-tx has been modified to take a height
in addition to the optional sigtype flags.
with help from str4d
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 20, 2018

☔️ The latest upstream changes (presumably #2940) made this pull request unmergeable. Please resolve the merge conflicts.

@str4d str4d force-pushed the str4d:1408-sighash branch from 39fb225 to 4553901 Feb 20, 2018
@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Feb 20, 2018

Rebased on master to fix merge conflicts, namely removing the partial commit from bitcoin/bitcoin#6915 which was fully back-ported in #2940.

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 20, 2018

⌛️ Trying commit 4553901 with merge 6db1012...

zkbot added a commit that referenced this pull request Feb 20, 2018
Overwinter SignatureHash

Implements ZIP 143.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of #2074 and #2254. Closes #1408 and #2584.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 20, 2018

💔 Test failed - pr-try

@daira

This comment has been minimized.

@arielgabizon

This comment has been minimized.

Copy link
Contributor

arielgabizon commented Feb 20, 2018

image
why are we running this test that always fails?

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Feb 20, 2018

@arielgabizon so we know that the expected-failures builder is working. We added that builder so we had the abillity to merge known-failing tests before we merged something to make them pass. There have been several instances of wanting to do this, but obviously making them regular tests that fail would mean our CI system would refuse to merge the PR 🙃

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Feb 20, 2018

The failure in the try above was a spurious hang in the RPC tests.

Thanks all! Any further issues raised can be addressed in subsequent PRs.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 20, 2018

📌 Commit 4553901 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 20, 2018

⌛️ Testing commit 4553901 with merge 8487be8...

zkbot added a commit that referenced this pull request Feb 20, 2018
Overwinter SignatureHash

Implements ZIP 143.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of #2074 and #2254. Closes #1408 and #2584.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 20, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 8487be8 to master...

@zkbot zkbot merged commit 4553901 into zcash:master Feb 20, 2018
1 check passed
1 check passed
homu Test successful
Details
Network Upgrade 0 automation moved this from Work Queue to Complete Feb 20, 2018
@str4d str4d deleted the str4d:1408-sighash branch Feb 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.