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

Revert to the Zebra sighash implementation #2468

Closed
conradoplg opened this issue Jul 8, 2021 · 2 comments
Closed

Revert to the Zebra sighash implementation #2468

conradoplg opened this issue Jul 8, 2021 · 2 comments
Labels
C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement

Comments

@conradoplg
Copy link
Collaborator

conradoplg commented Jul 8, 2021

Motivation

In #2183 we deleted our sighash implementation in order to use the librustzcash one, since we found a couple of bug in ours.

However, we may want to revert to our own implementation (and maybe implement our own ZIP-244 sighash). One possible reason for doing that would be efficiency (since we need to reserialize the transaction and deserialize it with librustzcash), but there could be others.

We must decide if we actually want to do this.

Known Issues

The Zebra sighash implementation changes the sighash based on the current network upgrade. But the specification uses the transaction version to select the sighash. We need to fix this bug before we can re-use the Zebra implementation, because it impacts NU5, which allows both V4 and V5 transactions.

We might want to remove some unimplemented! placeholders in the original code after it is reverted. See for example this previous attempt.

Specifications

ZIPs 143, 243, and 244.

Related Tickets

If we use Zebra's implementation, we might also want to Move duplicate Transaction code into methods

@conradoplg conradoplg added C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage labels Jul 8, 2021
@jvff
Copy link
Contributor

jvff commented Jul 9, 2021

If this is done, it might be interesting to remove some unimplemented! placeholders in the original code after it is reverted. See for example this previous attempt.

@mpguerra mpguerra added P-Low C-cleanup Category: This is a cleanup labels Jul 9, 2021
@conradoplg
Copy link
Collaborator Author

Closing this since performance doesn't seem to be an issue. If we decide it should be done we can reopen it.

@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement
Projects
None yet
Development

No branches or pull requests

3 participants