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

Generalize/deduplicate async batch verification services / create a derive-able proc macro #1951

Closed
dconnolly opened this issue Mar 26, 2021 · 5 comments
Assignees
Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup C-design Category: Software design work C-enhancement Category: This is an improvement

Comments

@dconnolly
Copy link
Contributor

dconnolly commented Mar 26, 2021

We now have three async batch verifiers in zebra-consensus::primitives, and they won't be the last (Orchard will bring halo2::Verifier and redpallas::Verifier eventually). There is a lot of boilerplate being copy-pasted around, which may be ok for easy deletion, but when it's duplicated amongst ~5 variants, it's a signal to revisit where we can DRY this up or make a more generic async batch Verifier, which can then be instantiated with different Items/ItemWrappers and some call method/function.

Or we could create a derive-able proc macro: https://github.com/imbolc/rust-derive-macro-guide

@dconnolly dconnolly added C-design Category: Software design work A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement C-cleanup Category: This is a cleanup P-Low labels Mar 26, 2021
@dconnolly dconnolly self-assigned this Mar 26, 2021
@dconnolly dconnolly changed the title Generalize/deduplicate async batch verification services Generalize/deduplicate async batch verification services / create a derive-able proc macro Jun 15, 2021
@jvff
Copy link
Contributor

jvff commented Jun 24, 2021

I ended up looking at the code as part of working on #2371 (and investigating what became #2390), and I stumbled upon an idea. I'll post it as a comment here before I forget it and just in case it's useful in the future.

I thought about creating a trait that resembles something like:

pub trait BatchVerifier {
    type Item;
    type Error;

    const MAX_BATCH_SIZE: usize;
    const MAX_BATCH_LATENCY: usize;

    fn enqueue(&mut self, item: Self::Item) -> Result<(), Self::Error>;
    fn verify(&mut self) -> Result<(), Self::Error>;

    fn verify_single(item: Self::Item) -> Result<(), Self::Error>;
}

and also a wrapper type to have the actual implementation of the service:

pub struct Verifier<V>(V);

impl<V: BatchVerifier> Service<BatchControl<V::Item>> for Verifier<V> {
    // ...
}

The remaining piece would be to have the actual batch and fallback wrapping code. This could be done by having a single constructor exposed for the Verifier<V> wrapper type:

impl<V: BatchVerifier> Verifier<V> {
    pub fn new() -> impl Service<V::Item, Response = (), Error = BoxError> {
        Fallback::new(
            Batch::new(
                Verifier(V::default()),
                V::MAX_BATCH_SIZE,
                V::MAX_BATCH_LATENCY,
            ),
            tower::service_fn(|item| future::ready(V::verify_single(item))),
        )
    }
}

With this trait and this wrapper type, it's possible to only implement the BatchVerifier trait for each verifier, which should be simpler than reimplementing the service for every verifier.

Not actually sure any of this works, so more research will be needed in the future.

@teor2345
Copy link
Contributor

const MAX_BATCH_SIZE: usize;
const MAX_BATCH_LATENCY: usize;

I think trait constants might have to be const generics, but they actually work in recent rust versions. Or they could just be functions.

@jvff
Copy link
Contributor

jvff commented Jun 25, 2021

Associated constants should work since Rust 1.20. Or do you mean it should be configurable when instantiated?

@teor2345
Copy link
Contributor

Ah you're right, I just haven't used them with traits before:
https://doc.rust-lang.org/edition-guide/rust-2018/trait-system/associated-constants.html

@dconnolly
Copy link
Contributor Author

Nice to have, but I think redundant now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup C-design Category: Software design work C-enhancement Category: This is an improvement
Projects
None yet
Development

No branches or pull requests

3 participants