cleanup: Use CommitSemanticallyVerifiedBlockRequest in block verifier#10284
cleanup: Use CommitSemanticallyVerifiedBlockRequest in block verifier#10284
Conversation
arya2
added
C-enhancement
There was a problem hiding this comment.
Pull request overview
This PR performs a small refactor across zebra-consensus and zebra-state to use the new MappedRequest-based request wrappers for committing blocks, and to simplify commit error handling by standardizing on CommitBlockError.
Changes:
- Switch semantic block verification to commit via
CommitSemanticallyVerifiedBlockRequest+MappedRequesthelpers. - Unify commit request error types across finalized/non-finalized commit paths to use
CommitBlockError. - Evolve
MappedRequestto rely onInto<Request>+ add helper mapping for tower-layer vs state errors.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
zebrad/tests/acceptance.rs |
Updates acceptance test to use CommitSemanticallyVerifiedBlockRequest::new(...).map_err(...). |
zebra-state/src/service/queued_blocks.rs |
Standardizes queued-block result channel error type to CommitBlockError. |
zebra-state/src/service/finalized_state/zebra_db/block.rs |
Switches finalized commit APIs to return CommitBlockError. |
zebra-state/src/service/finalized_state.rs |
Propagates CommitBlockError through finalized-state commit APIs. |
zebra-state/src/service.rs |
Updates state service request handling/comments to expect CommitBlockError. |
zebra-state/src/request.rs |
Updates MappedRequest API, adds map_err, and refactors commit request wrappers to use Into<Request>. |
zebra-state/src/lib.rs |
Changes public re-exports to expose LayeredStateError and removes older commit wrapper errors. |
zebra-state/src/error.rs |
Removes commit wrapper error types, adds LayeredStateError, and adjusts conversions. |
zebra-consensus/src/block.rs |
Uses CommitSemanticallyVerifiedBlockRequest in the block verifier and maps tower-layer errors into VerifyBlockError. |
zebra-consensus/Cargo.toml |
Adds derive-new dependency for new constructor generation usage. |
Cargo.lock |
Locks derive-new addition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Accepts a state service to call, maps this request to a [`Request`], waits for the state to be ready, | ||
| /// calls the state with the mapped request, then maps the success or error response to the expected response | ||
| /// or error type for this request. | ||
| /// | ||
| /// Returns a [`Result<MappedResponse, LayeredServicesError<RequestError>>`]. | ||
| #[allow(async_fn_in_trait)] | ||
| async fn mapped_oneshot<State>( | ||
| async fn run<State>( | ||
| self, | ||
| state: &mut State, | ||
| s: &mut State, | ||
| ) -> Result<Self::MappedResponse, LayeredStateError<Self::Error>> |
There was a problem hiding this comment.
The rustdoc for MappedRequest::run still says it returns LayeredServicesError<...>, but the function now returns Result<_, LayeredStateError<Self::Error>>. Please update the doc comment (and the referenced type name) so it matches the actual signature/output error semantics.
There was a problem hiding this comment.
This was already wrong, so feel free to fix it now or not
conradoplg
left a comment
conradoplg
left a comment
There was a problem hiding this comment.
Looks good, but I'd like to keep the MSRV 1.85 if possible
| /// Accepts a state service to call, maps this request to a [`Request`], waits for the state to be ready, | ||
| /// calls the state with the mapped request, then maps the success or error response to the expected response | ||
| /// or error type for this request. | ||
| /// | ||
| /// Returns a [`Result<MappedResponse, LayeredServicesError<RequestError>>`]. | ||
| #[allow(async_fn_in_trait)] | ||
| async fn mapped_oneshot<State>( | ||
| async fn run<State>( | ||
| self, | ||
| state: &mut State, | ||
| s: &mut State, | ||
| ) -> Result<Self::MappedResponse, LayeredStateError<Self::Error>> |
There was a problem hiding this comment.
This was already wrong, so feel free to fix it now or not
zebra-state/src/request.rs
Outdated
|
|
||
| /// Maps the expected [`Response`] variant for this request to the mapped response type. | ||
| fn map_response(response: Response) -> Self::MappedResponse; | ||
| fn map_rsp(response: Response) -> Self::MappedResponse; |
There was a problem hiding this comment.
Optional, but I'm not a fan of abbreviations
Cargo.toml
Outdated
| homepage = "https://zfnd.org/zebra/" | ||
| keywords = ["zebra", "zcash"] | ||
| rust-version = "1.85.0" | ||
| rust-version = "1.89.0" |
There was a problem hiding this comment.
We're trying to keep the MSRV 1.85 for crates, unless absolutely necessary. Can this PR be changed to not require this bump?
There was a problem hiding this comment.
The code on main is already using Result::flatten(self) from 1.89.0, we could change it back but there was a TODO waiting for that feature to stabilize for years.
There was a problem hiding this comment.
I know
There was a problem hiding this comment.
I see. I'll set it back to 1.85.0 for now and wait until it can be bumped without disrupting any active use cases. The ZIP 213 PR will also need some updates.
Motivation
This is just minor cleanup, it simplifies some code and serves as an example for how to use the new MappedRequest trait.
PR Checklist