Skip to content

feat(via_verifier): separate the verifier task from the withdrawal task#301

Open
0xatomFusion wants to merge 3 commits intomainfrom
feat/via-split-verifier-and-verfier-withdrawal
Open

feat(via_verifier): separate the verifier task from the withdrawal task#301
0xatomFusion wants to merge 3 commits intomainfrom
feat/via-split-verifier-and-verfier-withdrawal

Conversation

@0xatomFusion
Copy link
Copy Markdown
Collaborator

@0xatomFusion 0xatomFusion commented Nov 24, 2025

What ❔

  • Separate ZK verification from withdrawal processing.
  • Introduce a VerifierAndProcessor role that handles both ZK verification and withdrawal processing.
  • Updated the Makefile to start a new verifier-1

Why ❔

This PR introduces a dedicated Verifier role that performs only ZK verification and participates in batch finalization. Additionally, a VerifierAndProcessor role is able to perform both verification and withdrawal processing.

Testing

  1. Start the sequencer
  2. Start the coordinator and the make via-verifier-1
  3. Execute a deposit and wait for the batch to be finalized
  4. Execute a withdrawal, the batch will be finalized bu the withdrawal not processed as the verifier-1 just verify the batch.
  5. Start the make via-verifier, then wait for it to sync and the withdrawal to be processed.
  6. Done :)

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zkstack dev fmt and zkstack dev lint.

@0xatomFusion 0xatomFusion added the enhancement New feature or request label Nov 24, 2025
@0xatomFusion 0xatomFusion self-assigned this Nov 24, 2025
@0xatomFusion 0xatomFusion force-pushed the feat/via-split-verifier-and-verfier-withdrawal branch from 3332eac to 32e58ba Compare November 27, 2025 15:29
@0xatomFusion 0xatomFusion marked this pull request as ready for review November 27, 2025 15:32
@romanornr
Copy link
Copy Markdown
Collaborator

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request effectively separates the ZK verification task from the withdrawal processing task by introducing a new VerifierAndProcessor role. This improves modularity and allows for more flexible deployment configurations. The changes are well-structured, with updates to configuration, roles, and the node builder logic.

I've identified a few issues in the configuration handling that could lead to panics or unexpected behavior at runtime. Specifically, making some configuration fields optional requires careful handling of the None case in accessor functions. My review includes suggestions to make this more robust by returning Results or using expect() for mandatory fields for certain roles. I also found a minor inconsistency in the Makefile for the new verifier instance.

Overall, this is a good change that enhances the verifier's architecture. Addressing the configuration issues will make it production-ready.

Comment on lines 56 to 58
pub fn wallet_address(&self) -> anyhow::Result<Address> {
Ok(Address::from_str(&self.wallet_address)?.assume_checked())
Ok(Address::from_str(&self.wallet_address.clone().unwrap())?.assume_checked())
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

Calling unwrap() on self.wallet_address will cause a panic if the value is None. Since wallet_address is now an Option, this case must be handled. Given that the function already returns a Result, it's more idiomatic to propagate an error if the address is not set.

    pub fn wallet_address(&self) -> anyhow::Result<Address> {
        let address = self.wallet_address.as_ref().context("wallet_address is not set in ViaVerifierConfig")?;
        Ok(Address::from_str(address)?.assume_checked())
    }

Comment on lines +78 to +80
pub fn coordinator_port(&self) -> u16 {
self.coordinator_port.unwrap_or_default()
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Using unwrap_or_default() here will cause coordinator_port() to return 0 if the port is not configured. When bind_addr() uses this, it will bind to a random ephemeral port. The Coordinator role, which is the one using this, should always have a specific port configured. Using expect() will cause a panic with a clear error message on startup if the configuration is missing, which is safer than silently using a random port.

    pub fn coordinator_port(&self) -> u16 {
        self.coordinator_port.expect("coordinator_port is not set in ViaVerifierConfig")
    }

Comment on lines +82 to +84
pub fn coordinator_http_url(&self) -> String {
self.coordinator_http_url.clone().unwrap_or_default()
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Using unwrap_or_default() will return an empty string if coordinator_http_url is None. This will lead to invalid URLs (e.g., "/session") being constructed, causing runtime errors in network requests. For roles that require this URL, it should be mandatory. Using expect() will ensure that the configuration is present, causing an early panic if it's missing.

    pub fn coordinator_http_url(&self) -> String {
        self.coordinator_http_url.clone().expect("coordinator_http_url is not set in ViaVerifierConfig")
    }

Comment on lines +114 to +116
# Run the basic setup workflow in verifier
.PHONY: via-restart-verifier-1
via-restart-verifier-1: base verifier
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The comment for via-restart-verifier-1 seems to be a copy-paste from via-verifier-1 and doesn't reflect that it's a restart command. Additionally, a restart command typically shouldn't run init (which is part of the base target). For consistency with via-restart-verifier, it would be better to use env-soft which is more appropriate for a restart operation.

# Restart the verifier-1
.PHONY: via-restart-verifier-1
via-restart-verifier-1: env-soft verifier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants