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

Check sighash parameters in transaction verifier #1377

Closed
hdevalence opened this issue Nov 24, 2020 · 4 comments · Fixed by #1940
Closed

Check sighash parameters in transaction verifier #1377

hdevalence opened this issue Nov 24, 2020 · 4 comments · Fixed by #1940
Assignees
Labels
C-bug Category: This is a bug S-needs-investigation Status: Needs further investigation
Milestone

Comments

@hdevalence
Copy link
Contributor

Version

Git

Description

The sighash parameters in the transaction verifier are possibly incorrect and need to be checked.

@hdevalence hdevalence added C-bug Category: This is a bug S-needs-triage Status: A bug report needs triage labels Nov 24, 2020
@hdevalence hdevalence self-assigned this Nov 24, 2020
@mpguerra mpguerra added this to the v1.0.0-alpha.1 milestone Dec 9, 2020
@teor2345 teor2345 added S-needs-design Status: Needs a design decision S-needs-investigation Status: Needs further investigation and removed S-needs-design Status: Needs a design decision labels Dec 17, 2020
@mpguerra mpguerra removed this from the v1.0.0-alpha.1 milestone Jan 11, 2021
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Feb 2, 2021
@teor2345 teor2345 added this to the 2021 Sprint 5 milestone Mar 3, 2021
@mpguerra mpguerra modified the milestones: 2021 Sprint 5, 2021 Sprint 6 Mar 17, 2021
@teor2345
Copy link
Contributor

I've done some investigation, but I haven't discovered the fix yet.

Here's what I know:

  • our sighash impl passes the zcashd test vectors
  • Jubjub verification fails using our sighash impl
  • transparent script verification succeeds
  • I don't know if script verification uses an alternate sighash impl internally
  • both batched and individual jubjub fail
  • the parameters passed to sighash seem to match the spec
  • I did a few quick checks, and it's not a simple output array reversal

Here's what we could check next:

  • check if our Jubjub verification passes on the test vectors
  • try to work out what Jubjub expects the hash to be
  • try to work out if there's a simpler version of the problem, or a very simple transaction we can check in zcashd and Zebra
  • work out if all 3 of our sighash usages are failing (there's 1 for sprout and 2 for sapling)

Here's what I think the bug could be:

  • a byte order issue
  • accidental trailing or truncated bytes
  • some kind of issue in the glue code
  • the test vectors don't cover the cases we're actually using (but they do cover Overwinter and Sapling)
  • the zcashd impl doesn't quite match the spec
  • an issue in the way we use the sighash in other code

@teor2345
Copy link
Contributor

* work out if all 3 of our sighash usages are failing (there's 1 for sprout and 2 for sapling)

It looks like only the Sapling binding signature is failing - which means the bug is probably in the Sapling binding signature code.

Screen Shot 2021-03-23 at 16 24 13

I'd like to run a full sync on mainnet and testnet to be sure.

I'm using this code and dashboard:
https://github.com/teor2345/zebra/tree/sighash-diagnosis

@teor2345
Copy link
Contributor

Notes:

  • the "tmp" instances haven't synced past Canopy yet, so they're only verifying gossiped blocks
  • some nodes could be sending blocks with bad binding signatures, but that seems unlikely

@teor2345
Copy link
Contributor

Just confirming that it's only the binding signature, after syncing to tip.

Notes:

  • this is showing up in 3 instances, but not in the other 3. That could be a port configuration issue on my end.

Screen Shot 2021-03-24 at 06 45 04

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug S-needs-investigation Status: Needs further investigation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants