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

Consider refactoring the verifiers in transaction::Verifier #2473

Closed
jvff opened this issue Jul 9, 2021 · 0 comments
Closed

Consider refactoring the verifiers in transaction::Verifier #2473

jvff opened this issue Jul 9, 2021 · 0 comments
Labels
C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement

Comments

@jvff
Copy link
Contributor

jvff commented Jul 9, 2021

Motivation

The transaction::Verifier currently uses a shared global instance of each cryptographic verifier. If there's only one transaction verifier instance, having singleton cryptographic verifiers isn't necessary, and also makes testing slightly more complicated (see #2390).

Related Work

This idea was initially briefly discussed here:
#2390 (comment)

@jvff jvff added C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage labels Jul 9, 2021
jvff added a commit to jvff/zebra that referenced this issue Jul 9, 2021
The current implementation works, the commented out code was just a
previous improvement idea, which is now tracked by issue ZcashFoundation#2473.
jvff added a commit to jvff/zebra that referenced this issue Jul 9, 2021
The current implementation works, the commented out code was just a
previous improvement idea, which is now tracked by issue ZcashFoundation#2473.
jvff added a commit to jvff/zebra that referenced this issue Jul 9, 2021
The current implementation works, the commented out code was just a
previous improvement idea, which is now tracked by issue ZcashFoundation#2473.
teor2345 pushed a commit that referenced this issue Jul 9, 2021
* Add panic message to `unimplemented!`

So that it is clear why the panic happened upon initial inspection. Also
include a reference to the mempool epic, so that it's easier to find the
issue that tracks the implementation of the missing code.

* Add panic message that references a tracking issue

Make it easy to find the relevant issue if the panic occurs.

* Remove incomplete and currently unnecessary code

The current implementation works, the commented out code was just a
previous improvement idea, which is now tracked by issue #2473.
@mpguerra mpguerra added P-Low C-cleanup Category: This is a cleanup labels Jul 9, 2021
@jvff jvff closed this as completed Mar 18, 2022
@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

2 participants