Use thiserror in block validation #145
Conversation
Signed-off-by: yHSJ <[email protected]>
Signed-off-by: yHSJ <[email protected]>
Signed-off-by: yHSJ <[email protected]>
Signed-off-by: yHSJ <[email protected]>
Signed-off-by: yHSJ <[email protected]>
Signed-off-by: yHSJ <[email protected]>
Signed-off-by: yHSJ <[email protected]>
Signed-off-by: yHSJ <[email protected]>
Signed-off-by: yHSJ <[email protected]>
WalkthroughThis pull request introduces dependency updates and public export modifications in the amaru-kernel crate by adding the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant BV as Block Validator
participant TV as Transaction Validator
participant DI as Disjoint Checker
participant MV as Metadata Validator
participant OS as Output Size Validator
Caller->>BV: validate_block(block)
BV->>BV: block_header_size_valid(header)
BV->>BV: block_body_size_valid(block)
BV->>BV: block_ex_units_valid(ex_units)
BV->>TV: validate_transaction(tx)
TV->>DI: disjoint_ref_inputs(tx)
TV->>MV: validate_metadata(tx, aux_data)
TV->>OS: validate_output_size(tx, protocol_params)
TV-->>BV: tx validation result
BV-->>Caller: block validation result
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
crates/amaru-ledger/src/rules/transaction/disjoint_ref_inputs.rs (1)
5-20: Solid implementation for checking disjoint inputs!The function correctly identifies any overlap between reference inputs and normal inputs. However, it could benefit from some documentation to explain its purpose.
Consider adding documentation and an explicit type annotation for clarity:
+/// Validates that reference inputs are disjoint from normal inputs. +/// +/// Reference inputs should not appear in the normal inputs list to prevent +/// double-counting of the same input. +/// +/// # Arguments +/// * `transaction` - The transaction body to validate +/// +/// # Returns +/// * `Ok(())` if all reference inputs are disjoint from normal inputs +/// * `Err` with the overlapping inputs if any are found pub fn disjoint_ref_inputs(transaction: &TransactionBody) -> Result<(), TransactionRuleViolation> { let intersection = match &transaction.reference_inputs { Some(ref_inputs) => ref_inputs .iter() .filter(|ref_input| transaction.inputs.contains(ref_input)) .cloned() - .collect(), + .collect::<Vec<_>>(), None => Vec::new(), };crates/amaru-ledger/src/rules/block/body_size.rs (2)
5-6: New doc lines
These doc comments provide a helpful summary of the validation logic. Cheers for the clarity!Maybe add a cheeky mention of the expected data bounds for completeness.
24-43: calculate_block_body_size function
This is a neat little function! If you fancy more robust handling of encoding mishaps, you might consider returning aResultin future, but no rush if it stays squeaky clean.crates/amaru-ledger/src/rules/transaction/metadata.rs (1)
17-17: Mind that clone usage.
There’s a grand old “TODO” referencing a plan to clean up clones and introduce lifetimes. If time allows, reducing excessive clones might improve memory usage, especially if transactions are big.crates/amaru-ledger/src/rules.rs (2)
13-14: Handy new transaction validations.
Bringing indisjoint_ref_inputsandvalidate_metadatabroadens your quality checks. The single line import is tidy, though watch out for the “kitchen sink” effect if you add more functions.
75-90: ExUnits extraction logic.
Nicely done flattening and collecting those redeemers. If performance ever becomes a wee bit of a bottleneck, you might consider an iterator-based or streaming approach. For now, it’s perfectly readable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
crates/amaru-kernel/Cargo.toml(1 hunks)crates/amaru-kernel/src/lib.rs(1 hunks)crates/amaru-ledger/src/rules.rs(3 hunks)crates/amaru-ledger/src/rules/block/body_size.rs(1 hunks)crates/amaru-ledger/src/rules/block/ex_units.rs(2 hunks)crates/amaru-ledger/src/rules/block/header_size.rs(1 hunks)crates/amaru-ledger/src/rules/transaction/disjoint_ref_inputs.rs(1 hunks)crates/amaru-ledger/src/rules/transaction/metadata.rs(1 hunks)crates/amaru-ledger/src/rules/transaction/mod.rs(1 hunks)crates/amaru-ledger/src/rules/transaction/output_size.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Snapshots (preprod, 10.1.4)
🔇 Additional comments (27)
crates/amaru-kernel/Cargo.toml (1)
23-23: Looking good mate - adding the pallas-traverse dependency.This change properly adds the
pallas-traversecrate as a workspace dependency, consistent with how other dependencies are managed in this project.crates/amaru-ledger/src/rules/transaction/mod.rs (1)
1-3: Top-notch module structure!Good job organizing these transaction validation rules into separate modules. This structure will make the codebase more maintainable and easier to navigate.
crates/amaru-ledger/src/rules/block/ex_units.rs (2)
8-8: Neat shift to RuleViolation
This line neatly aligns the function with the unified error approach. Great call tidying up the error types!
17-23: Direct usage of TooManyExUnitsBlock
Nicely done returningRuleViolation::TooManyExUnitsBlockdirectly. Keeps the code breezy as a sea breeze in Southend.crates/amaru-ledger/src/rules/block/header_size.rs (2)
8-8: Function signature refactor
Swapping inResult<(), RuleViolation>spotlights a more streamlined approach. Top-notch consistency there, mate!
15-18: BlockHeaderSizeTooBig variant
Brilliant use ofRuleViolation::BlockHeaderSizeTooBig; top marks for placing all errors under one tidy umbrella.crates/amaru-ledger/src/rules/block/body_size.rs (4)
1-3: Import adjustments
Good show removing the old error struct references and roping in the newRuleViolation. Simpler than tea and biscuits.
9-10: Signature changes
Passingblockdirectly is ace for checking actual sizes. Good move toward more robust validation, chum.
11-13: Calculating block body size
Readingblock_header.block_body_sizeasusizethen pulling an actual size is a fair approach. No pressing concerns here.
14-17: BlockBodySizeMismatch
Well played returning an error variant that captures both supplied and actual sizes. Super handy for debugging!crates/amaru-ledger/src/rules/transaction/metadata.rs (3)
1-3: Nice use ofthiserrorand consistent imports.
It’s great to see you tidying up error handling by leveragingthiserror. This approach makes your errors more uniform around the project. Well done!
4-15: Spot-on enumeration of metadata errors.
Defining clear variants such asMissingTransactionMetadataandConflictingMetadataHashis top-drawer, ensuring that any future debugging is a piece of cake. The error messages are descriptive, which will help maintainers and users quickly figure out the cause of any validation errors.
18-45: Clear and cogent metadata validation.
The function neatly handles all combinations of missing or present auxiliary data and hash. Great job ensuring the mismatch scenario raises a distinct error. Consider adding a unit test to ensure all error paths are covered, especially around that mismatch.Would you like me to generate a quick script to locate potential test coverage references for
validate_metadataacross your test suite?crates/amaru-kernel/src/lib.rs (3)
31-31: Splendid re-export ofBytes.
Centralising re-exports keeps usage consistent throughout the codebase. Nicely done!
38-42: Extended re-exports for conway types.
Re-exportingTransactionBodyin place ofMintedTransactionBodyis a good move, presumably to unify logic across modules. Just be sure to update all references in the ledger code to prevent any mix-ups with minted vs. regular transactions.
46-46: Handy hashing utilities added.
ComputeHashandOriginalHashwill certainly come in handy. This approach matches the rest of the ledger’s design. Cheers for that!crates/amaru-ledger/src/rules.rs (11)
2-2: Transaction module reveal.
Introducingmod transaction;signals your new validation structure. Watch out for cyclical dependencies if you expand the module further.
5-7: Expanded imports for block and transaction elements.
Looks like you’re usingAuxiliaryData,TransactionBody, and others. Centralising them here helps keep references neat as a pin. Good show.
10-10: Modular block size validations.
Importing these helpers in a straightforward manner is delightful. Helps keep block validations well-organised.
16-24: NewBlockValidationErrorenumerations.
Your error definitions, especially the descriptiveCascading rule violationsvariant, provide clarity for diagnosing tricky multi-level errors. Perfect for sleuthing out complicated ledger issues.
26-41: ConciseRuleViolationcoverage.
Capturing violations such asTooManyExUnitsBlockpresents a thorough approach to ledger constraints. The structured message format helps ensure quick debugging.
42-55: Transaction-level rule checking.
TransactionRuleViolationelegantly enumerates concerns like referencing inputs twice (NonDisjointRefInputs) or small outputs. This kind of specificity is invaluable in diagnosing subtle transaction issues.
71-71: Seamless injection ofblock_body_size_valid.
You’re hooking the function in properly. Keep an eye on potential synergy withblock_header_size_validto maintain consistent checking.
95-101: Transacting with care.
Assigningtransactions,raw_transactions,witness_sets, andinvalid_transactionsin separate lines is easy for the eyes. Maintaining clarity in these crucial sets helps keep the ledger code tidy.
106-138: Snake-charming iteration over transactions.
Neatly iterating over transactions and witness sets while accounting for invalid ones is well done. The typed variant referencing the original hash is an excellent detail for debugging.It may be worth verifying if the block can contain zero witness sets or zero transactions for an edge case. Shall I prepare a script to cross-check your tests for this scenario?
143-155:validate_transactionwith layered checks.
The structure is quite comprehensive, testing metadata, reference inputs, and output sizes. It might be wise to add further checks or guidance around_is_valid, if some transactions are expected to pass partial validations. But overall, smashing job!
203-209: Test coverage forBlockHeaderSizeTooBig.
The step where you confirm the rule violation is spot-on. This helps ensure your validation logic stays consistent if max header size changes in the future.
| // clean up clones, replace with lifetimes | ||
| #[allow(clippy::panic)] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
TODO comment needs addressing
There's a comment indicating that clones should be replaced with lifetimes. This should be addressed before finalizing the PR.
🏁 Script executed:
#!/bin/bash
# Check if this comment appears elsewhere in the codebase
rg "clean up clones, replace with lifetimes" --type rustLength of output: 158
Heads up – the "clean up clones, replace with lifetimes" comment still needs your attention!
It appears that the TODO comment remains in the file at crates/amaru-ledger/src/rules/transaction/output_size.rs (around lines 7-8). There’s no sign elsewhere in the codebase, confirming that the issue hasn't been touched on in this PR.
- Please either implement the recommended refactoring to replace the clones with lifetimes or remove the comment if it’s no longer relevant.
Thanks for giving it a proper once-over, mate!
| pub fn validate_output_size( | ||
| transaction: &TransactionBody, | ||
| protocol_parameters: &ProtocolParameters, | ||
| ) -> Result<(), TransactionRuleViolation> { | ||
| let coins_per_utxo_byte = protocol_parameters.coins_per_utxo_byte; | ||
| let outputs_too_small = transaction | ||
| .outputs | ||
| .clone() | ||
| .into_iter() | ||
| .filter(|output| { | ||
| let mut bytes = Vec::new(); | ||
| cbor::encode(output, &mut bytes) | ||
| .unwrap_or_else(|_| panic!("Failed to serialize output")); | ||
|
|
||
| let lovelace = output_lovelace(output); | ||
|
|
||
| lovelace <= bytes.len() as u64 * coins_per_utxo_byte | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| if outputs_too_small.is_empty() { | ||
| Ok(()) | ||
| } else { | ||
| Err(TransactionRuleViolation::OutputTooSmall { outputs_too_small }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function works but needs some polish
The function correctly validates output sizes, but there are a few issues to address:
- The
.clone()operation is inefficient - The unwrap_or_else with panic isn't ideal for error handling
- The comparison could be clearer with parentheses
Let's spruce this up a bit:
// clean up clones, replace with lifetimes
-#[allow(clippy::panic)]
+/// Validates that transaction outputs contain enough lovelace based on their size.
+///
+/// According to the protocol parameters, each output must have at least
+/// (size_in_bytes * coins_per_utxo_byte) lovelace.
pub fn validate_output_size(
transaction: &TransactionBody,
protocol_parameters: &ProtocolParameters,
) -> Result<(), TransactionRuleViolation> {
let coins_per_utxo_byte = protocol_parameters.coins_per_utxo_byte;
let outputs_too_small = transaction
.outputs
- .clone()
- .into_iter()
+ .iter()
.filter(|output| {
let mut bytes = Vec::new();
- cbor::encode(output, &mut bytes)
- .unwrap_or_else(|_| panic!("Failed to serialize output"));
+ // This serialization should never fail for valid outputs
+ if let Err(e) = cbor::encode(output, &mut bytes) {
+ // Log the error or handle it more gracefully
+ return false; // Skip this output if we can't serialize it
+ }
let lovelace = output_lovelace(output);
- lovelace <= bytes.len() as u64 * coins_per_utxo_byte
+ lovelace <= (bytes.len() as u64 * coins_per_utxo_byte)
})
+ .cloned() // Only clone the outputs that fail validation
.collect::<Vec<_>>();
if outputs_too_small.is_empty() {
Ok(())
} else {
Err(TransactionRuleViolation::OutputTooSmall { outputs_too_small })
}
}This change is like changing from a rusty old spanner to a shiny new wrench - it'll do the same job but more efficiently and with less chance of breaking!
📝 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.
| pub fn validate_output_size( | |
| transaction: &TransactionBody, | |
| protocol_parameters: &ProtocolParameters, | |
| ) -> Result<(), TransactionRuleViolation> { | |
| let coins_per_utxo_byte = protocol_parameters.coins_per_utxo_byte; | |
| let outputs_too_small = transaction | |
| .outputs | |
| .clone() | |
| .into_iter() | |
| .filter(|output| { | |
| let mut bytes = Vec::new(); | |
| cbor::encode(output, &mut bytes) | |
| .unwrap_or_else(|_| panic!("Failed to serialize output")); | |
| let lovelace = output_lovelace(output); | |
| lovelace <= bytes.len() as u64 * coins_per_utxo_byte | |
| }) | |
| .collect::<Vec<_>>(); | |
| if outputs_too_small.is_empty() { | |
| Ok(()) | |
| } else { | |
| Err(TransactionRuleViolation::OutputTooSmall { outputs_too_small }) | |
| } | |
| } | |
| /// Validates that transaction outputs contain enough lovelace based on their size. | |
| /// | |
| /// According to the protocol parameters, each output must have at least | |
| /// (size_in_bytes * coins_per_utxo_byte) lovelace. | |
| pub fn validate_output_size( | |
| transaction: &TransactionBody, | |
| protocol_parameters: &ProtocolParameters, | |
| ) -> Result<(), TransactionRuleViolation> { | |
| let coins_per_utxo_byte = protocol_parameters.coins_per_utxo_byte; | |
| let outputs_too_small = transaction | |
| .outputs | |
| .iter() | |
| .filter(|output| { | |
| let mut bytes = Vec::new(); | |
| // This serialization should never fail for valid outputs | |
| if let Err(e) = cbor::encode(output, &mut bytes) { | |
| // Log the error or handle it more gracefully | |
| return false; // Skip this output if we can't serialize it | |
| } | |
| let lovelace = output_lovelace(output); | |
| lovelace <= (bytes.len() as u64 * coins_per_utxo_byte) | |
| }) | |
| .cloned() // Only clone the outputs that fail validation | |
| .collect::<Vec<_>>(); | |
| if outputs_too_small.is_empty() { | |
| Ok(()) | |
| } else { | |
| Err(TransactionRuleViolation::OutputTooSmall { outputs_too_small }) | |
| } | |
| } |
This PR updates the error handling to use thiserror, making it more in line with error handling in the rest of Amaru.
This PR relies on #129 being merged first
Summary by CodeRabbit
New Features
Refactor