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

Enable joinsplit and spend auth sighash verification #1940

Merged
merged 7 commits into from
Mar 25, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Mar 25, 2021

Motivation

In #1377, we thought our sighash implementation was broken. But investigation showed that it was actually the binding signature implementation.

Solution

  • re-enable joinsplit and sapling spend auth verification
  • add a metric for binding signature errors
  • add a warning log for binding signature errors
  • add an error grafana dashboard
  • minor cleanups

The code in this pull request has:

  • Documentation Comments
  • Sync Tests

Review

This is a routine review, @dconnolly just modified this code.

Related Issues

Closes #1377

Follow Up Work

#1939 Fix binding signature errors
#1944 Use Ed25519 async batch verification

@teor2345 teor2345 added C-bug Category: This is a bug A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup P-Medium I-consensus Zebra breaks a Zcash consensus rule labels Mar 25, 2021
@teor2345 teor2345 added this to the 2021 Sprint 6 milestone Mar 25, 2021
@teor2345 teor2345 requested a review from dconnolly March 25, 2021 01:25
@teor2345 teor2345 self-assigned this Mar 25, 2021
@teor2345 teor2345 marked this pull request as draft March 25, 2021 01:47
@teor2345
Copy link
Contributor Author

I've marked this PR as draft until a full sync passes on mainnet and testnet.

Feel free to review it, I just don't want to merge until I we're sure it works.

@teor2345 teor2345 marked this pull request as ready for review March 25, 2021 07:13
@teor2345
Copy link
Contributor Author

3 mainnet instances synced to tip, 3 testnet instances are lagging by about 130 blocks.

That's now #1941, which could be caused by network code hangs or peer issues (#1905), or the small number of peers on testnet (#704). We've had this bug before, but I haven't seen it happen on mainnet since the late 2020 / early 2021 network fixes.

So I don't think it's a blocking bug. (And it's unlikely to be related to these changes anyway.)

@mpguerra mpguerra removed this from the 2021 Sprint 6 milestone Mar 25, 2021
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕺💃

dconnolly
dconnolly previously approved these changes Mar 25, 2021
@dconnolly dconnolly merged commit c95716e into ZcashFoundation:main Mar 25, 2021
@dconnolly dconnolly added this to the 2021 Sprint 6 milestone Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-bug Category: This is a bug C-cleanup Category: This is a cleanup I-consensus Zebra breaks a Zcash consensus rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check sighash parameters in transaction verifier
3 participants