chore: make sure rules validation returns a structured result #186
Conversation
WalkthroughAlright mate, here's the lowdown: This batch of changes is a big ol' refactor of the block validation logic across the ledger and related bits. The validation flow now uses explicit enums— Changes
Sequence Diagram(s)sequenceDiagram
participant Worker
participant LedgerStage
participant BlockValidator
participant Context
Worker->>LedgerStage: roll_forward(point, raw_block)
LedgerStage->>LedgerStage: parse raw_block -> MintedBlock
LedgerStage->>LedgerStage: create_validation_context(MintedBlock)
LedgerStage->>BlockValidator: validate_block(context, MintedBlock)
BlockValidator->>BlockValidator: run header/body/ex_units checks
BlockValidator->>BlockValidator: for each tx: execute(context, tx)
BlockValidator-->>LedgerStage: BlockValidation::{Valid|Invalid}
alt Valid
LedgerStage->>Context: convert to VolatileState
LedgerStage->>Worker: return BlockValidation::Valid
else Invalid
LedgerStage->>Worker: return BlockValidation::Invalid
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
I'm not sure I agree with this statement. Perhaps it's not a system failure, but it's a failure to satisfy a predicate, which is a part of the happy-path. I have less rust experience than others working on Amaru so I may be wrong, but it feels very standard to use a I'll need some convincing to agree that this PR is needed at all. |
|
Errors accessing the db must be treated differently than failure to validate a block. Failure to validate a block is part of the nominal running of amaru, while Errors accessing the db will probably lead to process crash and require manual intervention. It also feels more functional to deal with validation errors this way. We then have a pure function calls returning |
I understand the idea here, it just feels unnatural to me. The Or, perhaps even better, instead of returning a I was just looking through some other rust implementations of blockchains (notably reth as we learned about it from an Ethereum dev in Paris), and they use a mix of basic All this to say that I get the idea and will use it if it's what we want throughout the rest of Amaru, it just feels a bit strange to me. |
| if let Err(err) = | ||
| header_size::block_header_size_valid(block.header.raw_cbor(), &protocol_params) | ||
| { | ||
| return BlockValidation::Invalid(err.into()); | ||
| }; |
There was a problem hiding this comment.
I'd eventually bite the bullet and do it for all rules. The inconsistency (some rules returning errors, and some returning results) is going to drive us crazy in the long run 😰.
|
@yHSJ this comes from a couple of discussions we had with @jeluard and @rkuhn about error handling. While we still don't generally agree about everything, I think the ledger rules is one particular location where the distinction between errors (as in, an application failure) and "invalid user inputs" is worth separating. One way to look at it is that from the perspective of Amaru (a block producer), an invalid block is part of the normal flow of execution and is just bound to a different logic flow. This is unlike a chain-indexer, who can reasonably expect all blocks it sees to be valid and if presented with an invalid one, it truly is an exception case. Amaru is different since we expect to sometimes run into invalid blocks if only because they are invalid with respects to our local state, but aren't "generally invalid". On the other hand, there are cases in the validation sequence that are truly invalid. For example, being unable to decode raw stake addresses bytes into a proper stake address. This error only occurs because of the partial de-serialization that happens in Pallas; and it is to be treated as an exception in this scenario. |
I totally get it and agree that this makes sense. I just think the way we're wrapping stuff and handling the results is strange now. If we want to have three different variants, we should create a new enum that has three variants, no? |
I do agree. I see mostly two points of improvements from here:
|
jeluard
force-pushed
the
jeluard/rules-validation-errors
branch
3 times, most recently
from
7d901ad to
bf33a12
Compare
|
One very important about So, please never use On the topic at hand: it would also be sensible to return |
| @@ -124,21 +137,22 @@ pub fn execute<C: ValidationContext<FinalState = S>, S: From<C>>( | |||
| transaction_index: i as usize, // From u32 | |||
| }; | |||
|
|
|||
| transaction::execute( | |||
| &mut context, | |||
| if let Err(err) = transaction::execute( | |||
There was a problem hiding this comment.
transaction rules haven't been changed; this could be done in a follow up PR.
| let bh_size = block_header.block_body_size as usize; | ||
| let actual_block_size = calculate_block_body_size(block); | ||
|
|
||
| if bh_size != actual_block_size { | ||
| Err(InvalidBlockSize::SizeMismatch { | ||
| BlockValidation::Invalid(InvalidBlock::Size(InvalidBlockSize::SizeMismatch { |
There was a problem hiding this comment.
Surely we can do better
jeluard
force-pushed
the
jeluard/rules-validation-errors
branch
2 times, most recently
from
6110b65 to
777740d
Compare
| @@ -1,3 +1,3 @@ | |||
| [toolchain] | |||
| channel = "1.84" # ⚠️ Also change in top-level Cargo.toml | |||
| channel = "nightly-2025-04-16" # ⚠️ Also change in top-level Cargo.toml | |||
There was a problem hiding this comment.
Doesn't match Cargo.toml's ?
There was a problem hiding this comment.
In Cargo.toml you can't use the nightly syntax so I used the closest stable version.
jeluard
force-pushed
the
jeluard/rules-validation-errors
branch
2 times, most recently
from
6173c21 to
6867dd5
Compare
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
jeluard
force-pushed
the
jeluard/rules-validation-errors
branch
from
e6939e3 to
f769bd4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
crates/amaru-consensus/src/consensus/chain_selection.rs (1)
500-504: Ah, a potential side-effect conundrum in the test!You've added the
clippy::double_ended_iterator_lastsuppression and a helpful TODO comment. This is a fair call-out - like that scene in "Inception" where actions in one layer affect all the others. The test might be relying onselect_roll_forwardbeing called for every element, not just to get the last result.Consider using
.for_each()followed by getting the last result, or.fold()to ensure all operations are performed:let result = chain2.iter().fold(None, |_, header| { Some(chain_selector.select_roll_forward(&bob, *header)) }).unwrap();examples/shared/src/lib.rs (2)
72-80: Don’tpanic!on user input – bubble it up for better UXIn this sample we slam a
panic!("Failed to validate block")the moment a block is dodgy.
That’s fine for a throw‑away example, but for real callers you probably want to propagate theInvalidBlockDetailsso higher layers (CLI, web, whatever) can display a friendly message instead of an abrupt stack trace.Something like:
-if let BlockValidation::Invalid(err) = rules::validate_block( - &mut DefaultRuleValidationExecutor::new(), - &mut context, - ProtocolParameters::default(), - &block, -) { - panic!("Failed to validate block") -}; +match rules::validate_block( + &mut DefaultRuleValidationExecutor::new(), + &mut context, + ProtocolParameters::default(), + &block, +) { + BlockValidation::Valid => {} + BlockValidation::Invalid(detail) => { + eprintln!("⚠️ Block rejected: {detail:?}"); + return; // or Result::Err(detail) + } +}Keeps the example snappy yet shows best practice.
82-85:unwrap()here could still kaboom – maybe swap for a?
state.forward(...).unwrap()will nuke the process if the ledger refuses to roll forward (storage error, unexpected era, etc.).
Because the surrounding function already returns(), consider convertingforward_ledgerinto-> anyhow::Result<()>(or similar) and usestate.forward(...)?;.That lets callers decide whether to retry or bail out in style.
crates/amaru-ledger/src/rules/block/header_size.rs (1)
31-35: Infallibleu32 → usizeconversion doesn’t need a panic cloakOn 32‑ and 64‑bit targets
usize::try_from(u32)cannot fail, so the panic arm is dead code and triggers Clippy’sunnecessary_lazy_evaluations.
A simple cast keeps things tidy:- let max_header_size = protocol_params - .max_header_size - .try_into() - .unwrap_or_else(|_| panic!("Failed to convert u32 to usize")); + let max_header_size = protocol_params.max_header_size as usize;Less noise, same safety.
crates/amaru-ledger/src/rules/block/body_size.rs (1)
42-43: Consider optimizing serialization in calculate_block_body_sizeG'day, this FIXME looks like a golden opportunity for a performance boost!
Calculating block body size by re-serializing the block is a bit like rebuilding your entire Minecraft house just to count how many blocks you used. The comment suggests relying on the original bytes instead, which would be much more efficient.
Would you like me to look into a more efficient implementation that doesn't require re-serialization?
crates/amaru-ledger/src/rules/block.rs (3)
31-57: AddClone/PartialEqderives & mark enum as#[non_exhaustive]for future‑proofingRight now
InvalidBlockDetails/BlockValidationcan’t be compared or cheaply copied, and any new variant addition will be a breaking change.
A tiny derive + attribute buys us ergonomics (unit tests, pattern‑matching comparisons) and semver wiggle‑room, with zero runtime cost.-#[derive(Debug)] -pub enum InvalidBlockDetails { +#[non_exhaustive] +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum InvalidBlockDetails { … } -#[derive(Debug)] -pub enum BlockValidation { +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum BlockValidation { … }
167-176: Promote “missing witness set” to a dedicated enum variantMapping a protocol‑level invariant breach to
UncategorizedErrorloses valuable signal.
Consider a specific variant, e.g.:MissingWitnessSet { transaction_index: u32 }That way consumers can match on it explicitly instead of string‑parsing.
198-213: Leveragemap_errergonomics to trim boilerplateThe manual
if let Err(err)dance works but feels a tad clunky.
You could compress it with:transaction::execute(…) .map_err(|violation| InvalidBlockDetails::Transaction { transaction_hash, transaction_index: i, violation, })?;The
?will bubble upBlockValidation::Invalidthanks to yourTryimpl, keeping the happy‑path tidy like Mario’s mustache.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.gitignore(1 hunks)Cargo.toml(1 hunks)crates/amaru-consensus/src/consensus/chain_selection.rs(2 hunks)crates/amaru-ledger/src/lib.rs(1 hunks)crates/amaru-ledger/src/rules.rs(3 hunks)crates/amaru-ledger/src/rules/block.rs(2 hunks)crates/amaru-ledger/src/rules/block/body_size.rs(3 hunks)crates/amaru-ledger/src/rules/block/ex_units.rs(3 hunks)crates/amaru-ledger/src/rules/block/header_size.rs(2 hunks)crates/amaru/src/stages/ledger.rs(4 hunks)examples/ledger-in-nodejs/rust-toolchain.toml(1 hunks)examples/shared/src/lib.rs(3 hunks)rust-toolchain.toml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/amaru-ledger/src/rules/block/header_size.rs (3)
crates/amaru-ledger/src/rules/block.rs (2)
new(93-95)default(99-101)crates/amaru-ledger/src/context/default/validation.rs (1)
new(40-47)crates/amaru-kernel/src/protocol_parameters.rs (1)
default(101-283)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Build on ubuntu-24.04 with target aarch64-unknown-linux-musl
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on ubuntu-latest with target x86_64-unknown-linux-gnu
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
- GitHub Check: coverage
- GitHub Check: Sanity
- GitHub Check: Snapshots (preprod, 10.1.4)
🔇 Additional comments (21)
.gitignore (1)
41-43: Mask llvm-cov coverage file
Top-notch move, mate! By adding the# llvm-cov generated filecomment and ignoringlcov.info, you’re keeping those bulky coverage artifacts out of commits—like slipping past security in Deus Ex.crates/amaru-ledger/src/lib.rs (1)
15-15: You've enabled thetry_trait_v2unstable feature - nice one!This is a solid move to support implementing the
Trytrait for your customBlockValidationenum. With this feature, you'll be able to use the?operator with your new validation result type, creating smoother control flow for validation chains. The changes align perfectly with the PR's goal of distinguishing validation failures from runtime errors.Cargo.toml (1)
10-10: Updated the rust-version requirement - fair dinkum!You've bumped the minimum Rust version from 1.84 to 1.88, which makes perfect sense given you're implementing the
Trytrait for your new validation types. This aligns with the toolchain updates elsewhere. Reminds me of upgrading gear in an RPG - gotta keep those dependencies fresh!crates/amaru-consensus/src/consensus/chain_selection.rs (1)
538-538: Good catch replacing.last()with.next_back()!You've swapped
.last()for.next_back()which is more explicit about how you're accessing the final element from the double-ended iterator. It's the same effect but avoids the Clippy warning. This change is like choosing the stiletto over the broadsword - more precise for the job at hand.rust-toolchain.toml (1)
2-2: Nightly surf’s up – double‑check the board wax, mateSwitching the whole repo to
nightly-2025‑04‑16is grand for unlockingtry_trait_v2, but nightly waves can dump breaking changes on you without warning.
👀 Make sure:
- Every workspace
Cargo.tomlnow opts into the same toolchain (or has an explicitrust-versiongate).- CI caches are flushed so you don’t keep compiling with a stale nightly.
rustfmt.toml/ Clippy lints still behave – some rules drift between nightlies.If any downstream crates must stay on stable you might need a
rust-toolchainfile per crate orrustup overridedance.
Just a gentle heads‑up before the build pipeline goes full “Mad Max”.crates/amaru-ledger/src/rules/block/ex_units.rs (4)
15-21: Added imports to support validation restructuringNice one updating the imports to align with the new structured validation approach. The shift from error handling to the explicit
BlockValidationenum makes your code's intent crystal clear - reminds me of how Dead Space separates the health bar from the suit integrity meter rather than smooshing them together.
23-27: Function signature refactored for validation clarityCrikey, this is a proper improvement! Taking a mutable validation context and returning the new
BlockValidationenum makes the distinction between validation failures and runtime errors dead obvious. It's like how in Fallout games, failing a speech check isn't an error - it's just one of the expected outcomes of the conversation.
36-46: Return structured validation results explicitlyBrilliant work here, mate! The explicit
BlockValidation::ValidandBlockValidation::Invalidreturns make the code flow dead obvious - it's not trying to shoehorn validation failures into error types anymore. This matches perfectly with the PR objective of distinguishing between validation failures (expected workflow) and runtime errors (exceptional cases).
51-52: Test updates reflect new validation patternGrand job updating the tests to match the new validation pattern. Tests are now properly checking for specific validation results rather than generic errors, which makes them much more meaningful - like how good unit tests in The Last of Us Part II test specific gameplay mechanics rather than just "did the game crash?".
Also applies to: 80-87
crates/amaru-ledger/src/rules/block/body_size.rs (4)
15-20: Reorganized imports to support validation restructuringSpot on with the imports updates here - pulling in the
ValidationContextand the newBlockValidationenum. Clean as a whistle!
23-29: Function signature refactored for validation clarityAce work on the signature change! Taking a mutable validation context and returning the new
BlockValidationenum is a slick move. Getting the block header directly from the block parameter is also cleaner than having it passed separately.
32-39: Return structured validation results explicitlyBeauty! The explicit
BlockValidation::ValidandBlockValidation::Invalidreturns make this function's intentions crystal clear. No more wondering if an error is a validation failure or a runtime issue - it's as clear as the difference between a parry and a miss in Dark Souls.
57-58: Test updates reflect new validation patternDeadly work on the test updates! I especially like how the assertions are now checking for specific validation details rather than just generic errors. Makes the tests much more meaningful - like checking if your Pokémon fainted from poison versus a critical hit, rather than just "did it faint?".
Also applies to: 60-61, 72-79
crates/amaru/src/stages/ledger.rs (4)
1-11: Updated imports to support new validation flowProper job with the imports! Bringing in
BlockValidationandDefaultRuleValidationExecutorsets the stage for the cleaner validation approach throughout the code.
62-92: Extracted validation context creation into a dedicated methodSmart move breaking out the validation context creation into its own method! Makes the code much more readable and maintainable - like how Breaking Bad separated its storylines into focused episodes rather than trying to cram everything into a single narrative thread.
99-123: Refactored roll_forward to use new validation flowThis refactoring is absolutely brilliant! The function now:
- Returns a meaningful
BlockValidationresult instead of()- Handles validation failures explicitly
- Has a clear separation between validation and state updates
It's like how Mad Max: Fury Road clearly distinguished the "everything is grand" path from the "we've got baddies on our tail" path. Very clean implementation!
171-182: Execution result mapping updated to handle validation outcomesNice update to the execute method! Mapping the
BlockValidationresults to the appropriateBlockValidationResultvariants is clean and expressive. The pattern match makes it impossible to forget handling either case - a classic example of making illegal states unrepresentable, like how Breath of the Wild won't let you climb when it's raining.crates/amaru-ledger/src/rules.rs (3)
131-135: Updated test imports to support new validation patternTop-notch import updates! Bringing in the
BlockValidationandDefaultRuleValidationExecutortypes sets up the tests to work with the new validation pattern.
178-186: Test updated to use new validation executor patternDeadly work updating the test to use the new validation approach! The mutable executor and context pattern is consistently applied, and the assertion checks specifically for
BlockValidation::Validinstead of a generic success result. Makes the test much more expressive - like how a good unit test in The Witcher 3 would check if Geralt successfully cast Igni rather than just "did something happen?".
206-217: Error case test updated to use new validation approachBeauty! The error test case is properly updated to use the new validation approach and assert on the specific
InvalidBlockDetailsvariant. This makes the test much more robust, as it's checking not just that validation failed, but how it failed - like checking if you died in Dark Souls because you ran out of stamina during a roll, not just that you died.crates/amaru-ledger/src/rules/block.rs (1)
59-79: NightlyTryusage – double‑check CI toolchain & gate featuresThe custom
Tryimpl is slick, mate, but it relies on#![feature(try_trait_v2)].
Make sure:
cargo +nightly checkis part of the CI matrix.- All downstream crates either gate the feature behind
cfg(feature = "nightly")or have a clear MSRV policy inREADME.md.Otherwise stable consumers will get a face‑full of compiler errors faster than Han shot Greedo.
| toolchain = { channel = "nightly-2025-04-16"} |
There was a problem hiding this comment.
Crikey, mate! You're time traveling to fetch this toolchain!
You've pinned the Rust toolchain to a specific nightly version dated "2025-04-16", which is in the future (as of April 2024). While pinning to a specific nightly is a grand idea for stability, especially when using unstable features like try_trait_v2, the date should probably be a valid past or current date.
-toolchain = { channel = "nightly-2025-04-16"}
+toolchain = { channel = "nightly-2024-04-16"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| toolchain = { channel = "nightly-2025-04-16"} | |
| toolchain = { channel = "nightly-2024-04-16"} |
There was a problem hiding this comment.
Looks like you are in the past, mate!
There was a problem hiding this comment.
@jeluard Strewth, you caught me time traveling in the wrong direction! You're absolutely right, cobber - I was living in 2024 while reviewing code from 2025. That's more confusing than the timeline in "Back to the Future"!
The "nightly-2025-04-16" is bang on for the current date. Thanks for the reality check - I'll be sure to check my temporal coordinates next time before making suggestions. My DeLorean must've had a dodgy flux capacitor!
| #[derive(Debug)] | ||
| pub enum InvalidBlockHeader { | ||
| #[error("block header size too big: supplied {supplied}, max {max}")] | ||
| SizeTooBig { supplied: usize, max: usize }, | ||
| } |
There was a problem hiding this comment.
InvalidBlockHeader is now a ronin – delete or reintegrate
The enum InvalidBlockHeader isn’t used anywhere after the refactor (the function returns InvalidBlockDetails instead).
Leaving it orphaned will trigger a dead‑code warning and confuse future spelunkers.
Either wire it back into InvalidBlockDetails or cull it altogether.
| // TODO: rewrite this to use iterators defined on `Redeemers` and `MaybeIndefArray`, ideally | ||
| let ex_units = block.ex_units(); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
TODO: Consider optimizing execution units extraction
Fair play flagging this with a TODO. Using iterators for the execution units extraction would make the code more idiomatic and potentially more efficient.
🏁 Script executed:
#!/bin/bash
# Let's see if there are any other similar patterns in the codebase
rg -A 2 "TODO: rewrite this to use iterators" --type rustLength of output: 338
Buff up ex_units extraction with iterators
Ahoy! I spotted the lone TODO at crates/amaru-ledger/src/rules/block/ex_units.rs:28—time to level up this code with a slick iterator combo. Zip together your block.redeemers() and block.maybe_indef_array() iterators, map them into ExUnits, then collect. It’s more idiomatic Rust and might shave off a few cycles, like landing a perfect dodge in your fave action RPG.
Locations to tweak:
- crates/amaru-ledger/src/rules/block/ex_units.rs:28
Suggested diff:
- // TODO: rewrite this to use iterators defined on `Redeemers` and `MaybeIndefArray`, ideally
- let ex_units = block.ex_units();
+ let ex_units = block.redeemers()
+ .zip(block.maybe_indef_array())
+ .map(|(redeemer, data)| {
+ // construct ExUnit from `redeemer` and `data`
+ })
+ .collect::<Vec<_>>();Committable suggestion skipped: line range outside the PR's diff.
| let witness_sets = block.transaction_witness_sets.deref().to_vec(); | ||
|
|
||
| let transactions = block.transaction_bodies.to_vec(); | ||
| let transactions = block.transaction_bodies.deref().to_vec(); | ||
|
|
||
| // using `zip` here instead of enumerate as it is safer to cast from u32 to usize than usize to u32 | ||
| // Realistically, we're never gonna hit the u32 limit with the number of transactions in a block (a boy can dream) | ||
| for (i, transaction) in (0u32..).zip(transactions.into_iter()) { | ||
| let transaction_hash = transaction.original_hash(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid full Vec clones – iterate by reference
to_vec() allocates & copies every transaction/witness set, which is a bit of a wallet‑drain when blocks get chunky.
-let witness_sets = block.transaction_witness_sets.deref().to_vec();
-let transactions = block.transaction_bodies.deref().to_vec();
+let witness_sets = &block.transaction_witness_sets;
+let transactions = &block.transaction_bodies;Then rewrite the loop:
-for (i, transaction) in (0u32..).zip(transactions.into_iter()) {
+for (i, (transaction, witness_set)) in transactions.iter().zip(witness_sets).enumerate() {
+ let i = i as u32;This keeps memory usage lean and sidesteps needless copies like Neo dodging bullets.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let witness_sets = block.transaction_witness_sets.deref().to_vec(); | |
| let transactions = block.transaction_bodies.to_vec(); | |
| let transactions = block.transaction_bodies.deref().to_vec(); | |
| // using `zip` here instead of enumerate as it is safer to cast from u32 to usize than usize to u32 | |
| // Realistically, we're never gonna hit the u32 limit with the number of transactions in a block (a boy can dream) | |
| for (i, transaction) in (0u32..).zip(transactions.into_iter()) { | |
| let transaction_hash = transaction.original_hash(); | |
| let witness_sets = &block.transaction_witness_sets; | |
| let transactions = &block.transaction_bodies; | |
| // using `zip` here instead of enumerate as it is safer to cast from u32 to usize than usize to u32 | |
| // Realistically, we're never gonna hit the u32 limit with the number of transactions in a block (a boy can dream) | |
| for (i, (transaction, witness_set)) in transactions.iter().zip(witness_sets).enumerate() { | |
| let i = i as u32; | |
| let transaction_hash = transaction.original_hash(); |
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
This reverts commit 3b31683. Signed-off-by: jeluard <[email protected]>
jeluard
force-pushed
the
jeluard/rules-validation-errors
branch
from
cb57f6a to
a45baef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/amaru-ledger/src/rules/block.rs (1)
111-113:⚠️ Potential issuePerformance hit with vector cloning
Crikey! You're still doing full vector clones here with
to_vec(), which can be a real resource drain for large blocks. This was flagged in a previous review but hasn't been addressed.Let's fix this to avoid unnecessary copying:
-let witness_sets = block.transaction_witness_sets.deref().to_vec(); -let transactions = block.transaction_bodies.deref().to_vec(); +let witness_sets = block.transaction_witness_sets.deref(); +let transactions = block.transaction_bodies.deref();Then update the loop to match:
-for (i, transaction) in (0u32..).zip(transactions.into_iter()) { +for (i, transaction) in (0u32..).zip(transactions.iter()) {
🧹 Nitpick comments (1)
crates/amaru-ledger/src/rules/block.rs (1)
31-51: Top-notch structured error type!Beauty of a job replacing the old error type with this more detailed
InvalidBlockDetailsenum. Each variant gives specific context about what went wrong, which is pure gold for debugging. This reminds me of how Witcher 3's quest system gives you just the right amount of info without overwhelming you.One small suggestion though - might be worth adding
Clone,PartialEqand maybeEqderives to make testing easier down the road.-#[derive(Debug)] +#[derive(Debug, Clone, PartialEq, Eq)] pub enum InvalidBlockDetails {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (3)
crates/amaru-ledger/src/rules.rs(3 hunks)crates/amaru-ledger/src/rules/block.rs(2 hunks)crates/amaru/src/stages/ledger.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/amaru/src/stages/ledger.rs
- crates/amaru-ledger/src/rules.rs
🔇 Additional comments (12)
crates/amaru-ledger/src/rules/block.rs (12)
25-26: Clean import update, nice one!The import structure is nicely aligned with the new approach, bringing in
ProtocolParametersdirectly. Tidy organization is the foundation of a good codebase, like how a neat inventory system makes RPGs so much more satisfying.
28-28: Added imports for theTrytrait - deadly choice!Good on ya for bringing in the necessary imports to support the new control flow approach. The
Trytrait is a ripper feature that makes the code flow as smooth as butter in a hot pan.
53-57: Crisp newBlockValidationenum - absolute winner!This new enum makes the distinction between valid and invalid blocks crystal clear. It's like when Neo finally sees the Matrix - suddenly everything makes sense! This aligns perfectly with separating validation failures from runtime errors.
59-73: Clever implementation of theTrytrait!You've done a brilliant job implementing the
Trytrait here, mate! This enables the?operator to work with your custom validation type, making the control flow as elegant as a parkour sequence in Mirror's Edge. Thebranchmethod creates a clean separation between the success and failure paths.
75-79: SolidFromResidualimplementation!This implementation completes the
Trytrait requirement and allows for smooth propagation of validation errors. Nice and concise - does exactly what it needs to without any faff.
81-88: Handy conversion to standard Result!This conversion function is a deadly idea - provides interoperability with the rest of the Rust ecosystem that expects standard
Results. It's like having that universal adapter when traveling abroad - works with everything!
91-95: Better ownership model in function signature - good thinking!Taking references instead of ownership is a top improvement! This change should reduce unnecessary cloning and make the code more efficient. It's like upgrading from a rusty sword to a legendary blade in Skyrim - same function but much more effective.
99-107: Slick validation functions collection!Collecting validation functions in a vector and applying them sequentially is a stroke of genius! This approach makes the code more maintainable and extensible. The TODO about applying them in parallel or priority order shows good forward thinking too.
This pattern opens the door to more advanced execution strategies, just like the PR description mentioned. Dead on!
120-128: Clearer error handling for witness sets!This explicit match statement is more readable than the previous approach with
ok_orand?. It provides a clear path for both success and failure cases, making the code as easy to follow as a well-designed quest marker.
151-165: Explicit pattern matching for transaction validation errors!The switch from
map_errand?to explicitif let Errmakes the error handling more direct and readable. It's like when a game tutorial shows you exactly what button to press instead of being all cryptic - much appreciated by everyone involved!
168-168: Clean return of validation result!Returning
BlockValidation::Validdirectly is much clearer than the previous approach. It's like the satisfying "quest completed" notification - you know exactly what happened!
90-169:✅ Verification successful
Overall code structure improvement
The whole
executefunction has had a ripper of a makeover! The new structure with consistent error handling, borrowing instead of taking ownership, and the collection of validation functions makes this function much more maintainable.Just one thing to note - there's an inconsistency mentioned in previous reviews about some rules returning errors and others returning results. As KtorZ said, might be worth "biting the bullet" and updating all rules to follow this pattern.
🏁 Script executed:
#!/bin/bash # Check if there are still inconsistencies in rule return types echo "Searching for transaction rule functions that might still return Result..." rg "pub fn .+ -> (Result|anyhow::Result)" crates/amaru-ledger/src/rules/transaction/ -A 1Length of output: 250
Alright, mate—let’s take another peek at those transaction rules and list out every function signature to spot any odd ducks still returning the wrong type. 🕵️♂️
🏁 Script executed:
#!/bin/bash echo "Listing all transaction rule functions and their return types..." rg -n "pub fn .+->" crates/amaru-ledger/src/rules/transaction/ -A 1Length of output: 208
Sure thing, mate—let’s give those transaction rules a proper once‑over, like tracing loot in a Diablo dungeon. We’ll list every
pub fnsignature and then sniff out any stragglers still returningResult.
🏁 Script executed:
#!/bin/bash echo "1) Listing all transaction rule function signatures with a bit of context..." rg -n -t rust "pub fn" crates/amaru-ledger/src/rules/transaction/ -A 2 echo -e "\n2) Checking specifically for any lingering '-> ...Result' return types..." rg -n -t rust "->.*Result" crates/amaru-ledger/src/rules/transaction/ -A 1Length of output: 2568
No worries, let’s refine our grep spell to dodge that leading hyphen ghost and sniff out any stragglers returning
Resultin the transaction rules.
🏁 Script executed:
#!/bin/bash echo "Looking for any `pub fn` in transaction rules that return a *Result..." rg -n -t rust -e 'pub fn .*->.*Result' crates/amaru-ledger/src/rules/transaction/ -A2Length of output: 326
Alright, let’s dodge those pesky backticks and hyphens—no more shell-level gremlins. We’ll do a quick two‑step scan: first list any
pub fnwith an arrow, then filter those forResultreturns.
🏁 Script executed:
#!/bin/bash echo '1) Find all transaction rule functions with a return arrow…' rg -n -t rust 'pub fn .*->' crates/amaru-ledger/src/rules/transaction/ -A2 echo -e '\n2) From those arrows, filter for Result return types…' rg -n -t rust -F '->' crates/amaru-ledger/src/rules/transaction/ -A1 \ | grep -R 'Result' || echo '✔ No Result<> return types found'Length of output: 44563
No return type inconsistency found in transaction rules
All
pub fn executefunctions undercrates/amaru-ledger/src/rules/transaction/now uniformly returnResult<(), Invalid*>. The earlier note about mixed “errors vs. results” is outdated—no further changes needed here.
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
Makefile(1 hunks)examples/shared/src/lib.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/shared/src/lib.rs
🔇 Additional comments (1)
Makefile (1)
113-113: Love the lean exit hack!
Swapping out the old multi‑line check for$(MAKE) -C $$dir || exitis slick—your CI pipeline now halts on the first stumble, just like hitting the eject button when a B‑movie goes off the rails.
Makefile
Outdated
| all-core-ci-checks: | ||
| @cargo fmt-amaru | ||
| @cargo clippy-amaru | ||
| @cargo test-amaru | ||
| @$(MAKE) build-examples | ||
| @$(MAKE) coverage-lconv | ||
|
|
||
| all-ci-checks: all-core-ci-checks build-examples coverage-lconv ## Run all CI checks |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mark new CI targets as phony & document them
The fresh all-core-ci-checks and all-ci-checks chores need a touch of makefile love—declare them in .PHONY and give all-core-ci-checks its own ## docstring. Also, that stray blank at line 121 can vanish quicker than DLC vaporware.
# At top of file, extend the .PHONY list
.PHONY: help bootstrap run import-snapshots import-headers import-nonces download-haskell-config coverage-html coverage-lconv check-llvm-cov
+.PHONY: all-core-ci-checks all-ci-checks
# Add a description on the new target
-all-core-ci-checks:
+all-core-ci-checks: ## Run core CI checks
# Delete the extra blank line at 121
- 📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| all-core-ci-checks: | |
| @cargo fmt-amaru | |
| @cargo clippy-amaru | |
| @cargo test-amaru | |
| @$(MAKE) build-examples | |
| @$(MAKE) coverage-lconv | |
| all-ci-checks: all-core-ci-checks build-examples coverage-lconv ## Run all CI checks | |
| # At the top of the Makefile, extend the .PHONY list | |
| .PHONY: help bootstrap run import-snapshots import-headers import-nonces download-haskell-config coverage-html coverage-lconv check-llvm-cov | |
| .PHONY: all-core-ci-checks all-ci-checks | |
| # Lines ~117–122: document and tighten up | |
| all-core-ci-checks: ## Run core CI checks | |
| @cargo fmt-amaru | |
| @cargo clippy-amaru | |
| @cargo test-amaru | |
| all-ci-checks: all-core-ci-checks build-examples coverage-lconv ## Run all CI checks |
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
| let result = chain2 | ||
| .iter() | ||
| // TODO looks like this test relies on the fact that `select_roll_forward` is called on every element. `map` might not be correct then |
There was a problem hiding this comment.
I do not understand this comment about map ?
There was a problem hiding this comment.
last can be replaced by next_back in theory here but in practice it fails some test. This implies that this call expects map to traverse all element by calling a function with side-effects (select_roll_forward). map should be called with pure functions as a way to construct another iterator. for_each might help clarify the expectation here (but at the price of a more contrived usage as the last element should be tracked).
Let
rules::validate_blockreturn validation results as structured data and not as an error.Result::Errshould be used only for runtime errors and not validation failures.Before
After
A common signature has been introduced for
blocklevel rule validation. This allows to share common logic to deal with theinvalidcase.Note that introducing a trait to have stronger guarantees could be considered.
Execution of block validation fun is now abstracted in a trait:
This open the door down the line for more advanced way to deal with their execution than the simplistic linear execution (e.g. parallel execution, ordered execution, ...).
Next steps
The same changes could be applied to
transactionlevel rule validations.Summary by CodeRabbit
New Features
Refactor
Chores