Initial Transaction Validations #129
Conversation
Signed-off-by: yHSJ <[email protected]>
WalkthroughThe changes introduce enhancements to transaction validation across several modules. In the kernel crate, new types such as Changes
Sequence Diagram(s)sequenceDiagram
participant BV as BlockValidator
participant VT as validate_transaction
participant DRI as disjoint_ref_inputs
participant OS as validate_output_size
participant MD as validate_metadata
BV->>VT: validate_transaction(tx_body, witness_set, protocol_params)
VT->>DRI: disjoint_ref_inputs(tx_body)
DRI-->>VT: Return ok/error for disjoint inputs
VT->>OS: validate_output_size(tx_body, protocol_params)
OS-->>VT: Return ok/error for output size
VT->>MD: validate_metadata(tx_body, auxiliary_data)
MD-->>VT: Return ok/error for metadata
VT-->>BV: Return Ok / TransactionRuleViolation (InvalidTransaction)
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🧹 Nitpick comments (5)
crates/amaru-ledger/src/rules/transaction/disjoint_ref_inputs.rs (1)
9-13: Alternative Conversion Recommendation
Usingimpl From<NonDisjointRefInputs> for TransactionRuleViolationmight be more idiomatic thanimpl Into. Consider turning that commented code back on, dear chum!Here’s a potential tweak:
-impl Into<TransactionRuleViolation> for NonDisjointRefInputs { +impl From<NonDisjointRefInputs> for TransactionRuleViolation { fn from(value: NonDisjointRefInputs) -> Self { TransactionRuleViolation::NonDisjointRefInputs(value) } }crates/amaru-ledger/src/rules/transaction/output_size.rs (1)
17-53: Handle Serialization Error More Politely
Thepanic!on serialisation failure could rattle unsuspecting users if it’s triggered. Might be best to return a bespoke error. Otherwise, the rest is top-notch, especially the logic around coin size checks—cracking work!Here’s a possible approach for gentler error handling:
cbor::encode(output, &mut bytes) - .unwrap_or_else(|_| panic!("Failed to serialize output")); + .map_err(|_| { + // Return a descriptive error or perhaps wrap it in your custom type + OutputTooSmall { outputs_too_small: vec![output.clone()] } + })?;crates/amaru-ledger/src/rules.rs (3)
59-59: Consider avoiding unnecessary data cloning.You’re cloning
block.transaction_witness_setsbefore de-structuring it. If the structure’s not massive or if you only need read access, you could reference it directly, saving a few bytes.
97-116: Check those TODOs to shore up the logic.
- If a witness set is missing for a transaction, you skip it—are you sure no error is needed? Might be worth verifying.
- The placeholder zeroed-out
transaction_hashshould eventually be replaced by the real McCoy to make error reports more helpful.Happy to offer help implementing the transaction hash retrieval or refining the skip logic if needed—just let me know, mate.
121-131: Unused_is_validparameter.Since
_is_validis never used, it might puzzle future readers. You could remove it or incorporate a check to short-circuit on known invalid transactions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/amaru-kernel/src/lib.rs(1 hunks)crates/amaru-ledger/src/rules.rs(5 hunks)crates/amaru-ledger/src/rules/transaction/disjoint_ref_inputs.rs(1 hunks)crates/amaru-ledger/src/rules/transaction/mod.rs(1 hunks)crates/amaru-ledger/src/rules/transaction/output_size.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/amaru-ledger/src/rules/transaction/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
🔇 Additional comments (16)
crates/amaru-kernel/src/lib.rs (1)
40-42: Spiffing Re-exports!
Bringing inPseudoTransactionOutputandTransactionBodyin place ofMintedTransactionOutputlooks grand. It neatly aligns with the new transaction validation enhancements. Keep this approach up!crates/amaru-ledger/src/rules/transaction/disjoint_ref_inputs.rs (3)
1-3: Imports Look Spot-On!
Everything is hunky-dory here. Nothing to quibble over. Move along confidently!
5-7: Nicely Named Struct
NonDisjointRefInputsmay sound a tad negative, but it’s crystal clear in its intent. Good on you for clarity!
21-36: Intersection Detection Works a Treat
This function elegantly identifies intersecting inputs. Splendid job! Everything looks properly well-structured.crates/amaru-ledger/src/rules/transaction/output_size.rs (3)
1-3: Importing the Essentials
The necessary items for transaction output checks are graciously gathered here. No fuss whatsoever!
7-9: A Spot-On Struct
OutputTooSmallclarifies its purpose in a jiffy. Good naming, mate!
11-15: Consistent Error Conversion
YourInto<TransactionRuleViolation>implementation deftly parallels the pattern from other rule violations. Jolly good!crates/amaru-ledger/src/rules.rs (9)
2-2: Top-notch module separation, mate!Introducing the
transactionmodule here helps keep everything tidy and compartmentalised. No worries from my end.
6-6: Neat addition of these items from the kernel crate.Pulling in
MintedBlock, Redeemers, TransactionBody, WitnessSetis perfectly grand, as it sets the stage for your transaction logic.
15-16: Good on you for splitting up transaction checks into their own modules.This modular approach for
disjoint_ref_inputsandoutput_sizeis a beaut—makes the code a smidgen easier to organise and maintain.
18-22: Enum structure looks well-structured.Lovely to see
BlockValidationErrorenumerated so thoroughly. This is a pretty straightforward approach, helps keep error handling crisp.
28-32: Detailed error variant for invalid transactions.Storing the transaction hash, index, and the specific violation in
InvalidTransactionis a smashing idea. This will provide clarity in debugging.
35-37: Great approach to categorising transaction rule violations.Defining each specific error type (
NonDisjointRefInputsandOutputTooSmall) makes it practically impossible to mix them up. Right as rain!
79-82: Handy extraction of transactions.Matching on
MaybeIndefArrayto unify data into a simple vector is quite neat. Crikey, no problems spotted here.
84-87: Witness sets neatly retrieved.Again, the pattern matching approach is consistent, which keeps the code easy to follow.
89-95: Good fallback for when invalid transactions are absent.Skipping gracefully to an empty list prevents blow-ups in your loop below. Spot on!
| pub intersection: Vec<TransactionInput>, | ||
| } | ||
|
|
||
| impl Into<TransactionRuleViolation> for NonDisjointRefInputs { |
There was a problem hiding this comment.
I attempted to implement the From trait here instead, but I ran into a strange rustc error I'm still working on debugging.
Signed-off-by: yHSJ <[email protected]>
yHSJ
force-pushed
the
jshy/block-validation
branch
from
5fd7757 to
0f70c7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/amaru-ledger/src/rules.rs (2)
105-109: Consider error handling for missing witness setsThe current implementation simply skips transactions without a witness set. Is this the intended behavior? If a transaction is expected to have a witness set but doesn't, this might indicate a problem that should be reported rather than silently skipped.
Consider whether this warrants an error or at least a warning log.
- let witness_set = match witness_sets.get(i as usize) { - Some(witness_set) => witness_set, - None => continue, - }; + let witness_set = match witness_sets.get(i as usize) { + Some(witness_set) => witness_set, + None => { + // Only skip if this is expected behavior, otherwise consider returning an error + tracing::warn!("Transaction at index {} has no witness set", i); + continue; + } + };
106-106: Address TODO comment for type conversionThis TODO comment flagging the
ascast suggests potential issues with the type conversion. Worth addressing this to ensure proper error handling for potential overflows ifibecomes a large value.This could potentially lead to an integer overflow if the transaction index exceeds
usize::MAX, though that's unlikely in practice.- // TODO handle `as` correctly - let witness_set = match witness_sets.get(i as usize) { + let witness_set = match usize::try_from(i).map_err(|_| { + BlockValidationError::RuleViolations(vec![RuleViolation::InvalidTransaction { + transaction_hash: raw_transaction.original_hash(), + transaction_index: i, + violation: TransactionRuleViolation::IndexOutOfRange, // Would need to add this variant + }]) + }).and_then(|idx| Ok(witness_sets.get(idx))) {
📜 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 (3)
crates/amaru-kernel/Cargo.toml(1 hunks)crates/amaru-kernel/src/lib.rs(1 hunks)crates/amaru-ledger/src/rules.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/amaru-kernel/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- 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
🔇 Additional comments (10)
crates/amaru-kernel/src/lib.rs (2)
40-42: Updated the re-exports to support transaction validationGood job on updating the public re-exports to include
PseudoTransactionOutputand replaceMintedTransactionOutputwithTransactionBody. This is a sensible change that aligns with the PR objectives of introducing transaction validations.Just make sure all existing code that was using
MintedTransactionOutputhas been properly migrated to useTransactionBodyinstead. This change will ripple through anywhere that imported the old type.
46-47: Added OriginalHash from pallas_traverseNice addition of the
OriginalHashtrait which will be useful for getting transaction hashes in the validation logic. Good to see this being properly exported through the anti-corruption layer.crates/amaru-ledger/src/rules.rs (8)
2-2: Added new transaction moduleGood addition of the transaction module. This is a nice clean way to organize the new transaction validation functionality.
15-16: Imported transaction validation functionsGreat job importing the specific validation functions from the transaction submodules. This approach keeps the code organized while making the validation functions directly accessible in the rules module.
28-32: Added InvalidTransaction variant to RuleViolationNice implementation of the
InvalidTransactionvariant which captures all the necessary context (hash, index, and specific violation). This gives you plenty of information for debugging when validation fails.
35-38: Added TransactionRuleViolation enumGood job creating a specific enum for transaction rule violations. This gives you a nice extensible structure to add more validation rules in the future while keeping error reporting clean and specific.
79-101: Enhanced block validation with transaction processingWell-structured code to extract the relevant transaction information from the block. The pattern matching approach is consistent with the rest of the codebase.
127-137: New validate_transaction functionThe
validate_transactionfunction is a good start for transaction validation. The unused parameters (_witness_setand_is_valid) with leading underscores indicate planned future enhancements, which is fine for initial transaction validations.As you expand the validation rules in the future, you'll likely make use of these parameters. For now, it's clear these are intentionally unused.
113-122: Good error handling for transaction validationWell done on the comprehensive error handling that includes the transaction hash and index in the error. This will make debugging much easier when validation fails on a specific transaction.
133-134: Initial transaction validation rulesNice implementation of the first two transaction validation rules. You've created a good foundation to easily add more validation rules in the future. The implementation is clean and follows a consistent pattern.
For debugging purposes, you might want to consider adding a bit more context like specific values in the error types, but this is a solid start.
Signed-off-by: yHSJ <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/amaru-ledger/src/rules/transaction/metadata.rs (2)
20-24: Remove or document the commented-out From implementationYou've already got the
Intotrait implementation, which makes this commented-outFromimplementation redundant. Better to either remove it entirely or add a comment explaining why it's being kept for future reference.-// impl From<InvalidTransactionMetadata> for TransactionRuleViolation { -// fn from(value: InvalidTransactionMetadata) -> Self { -// TransactionRuleViolation::InvalidTransactionMetadata(value) -// } -// }
28-28: Fix typo in parameter name: "auxilary_data" → "auxiliary_data"There's a wee spelling mistake in the parameter name - missing an "i" in "auxiliary". Small thing, but worth fixing for consistency.
- auxilary_data: Option<AuxiliaryData>, + auxiliary_data: Option<AuxiliaryData>,crates/amaru-ledger/src/rules.rs (2)
132-139: Add documentation for the validate_transaction functionThis function would benefit from some documentation explaining its purpose, parameters, and return value. Especially since some parameters have leading underscores indicating they're not currently used.
Consider adding documentation like:
+/// Validates a transaction against the ledger rules. +/// +/// # Parameters +/// * `transaction_body` - The transaction body to validate +/// * `_witness_set` - The witness set (currently unused) +/// * `auxiliary_data` - Optional auxiliary data for the transaction +/// * `_is_valid` - Flag indicating if the transaction is valid (currently unused) +/// * `protocol_params` - Protocol parameters to validate against +/// +/// # Returns +/// * `Ok(())` if the transaction is valid +/// * `Err(TransactionRuleViolation)` with the specific violation otherwise pub fn validate_transaction( transaction_body: &TransactionBody, _witness_set: &WitnessSet, auxiliary_data: Option<AuxiliaryData>, _is_valid: bool, protocol_params: &ProtocolParameters, ) -> Result<(), TransactionRuleViolation> {
139-142: Simplify error mapping with the?operatorThe error mapping pattern of
.map_err(|err| err.into())?can be simplified by implementing theFromtrait for each error type and using the?operator directly.- validate_metadata(transaction_body, auxiliary_data).map_err(|err| err.into())?; + validate_metadata(transaction_body, auxiliary_data)?; - disjoint_ref_inputs(transaction_body).map_err(|err| err.into())?; + disjoint_ref_inputs(transaction_body)?; - validate_output_size(transaction_body, protocol_params).map_err(|err| err.into())?; + validate_output_size(transaction_body, protocol_params)?;This would require implementing
Fromtraits instead ofInto:impl From<InvalidTransactionMetadata> for TransactionRuleViolation { fn from(value: InvalidTransactionMetadata) -> Self { TransactionRuleViolation::InvalidTransactionMetadata(value) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/amaru-kernel/src/lib.rs(1 hunks)crates/amaru-ledger/src/rules.rs(3 hunks)crates/amaru-ledger/src/rules/transaction/metadata.rs(1 hunks)crates/amaru-ledger/src/rules/transaction/mod.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- 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
🔇 Additional comments (9)
crates/amaru-ledger/src/rules/transaction/mod.rs (1)
1-3: Nicely structured modular approach to transaction validation!Good job breaking down the transaction rules into logical modules. This modular approach will make maintenance and testing a piece of cake down the road.
crates/amaru-kernel/src/lib.rs (3)
32-32: Good addition of the Bytes type to public exportsSensibly exposed for the transaction validation work. Keep the exports tidy!
39-42: Well-organized type exports for transaction validationNice job adding these necessary Conway types to the public interface. Particularly
TransactionBodyandPseudoTransactionOutputwhich are essential for the new validation logic.
46-46: Good addition of OriginalHashThis will be useful for transaction hash tracking in the validation process.
crates/amaru-ledger/src/rules/transaction/metadata.rs (1)
48-49: Clarify the TODO comment about validateMetadataThe comment mentions that some validation logic is implemented during deserialization instead of here. It would be helpful to add a bit more detail about what specific validation is missing and perhaps create a tracking issue to ensure it doesn't get forgotten.
Could you clarify the specific validation that's pending implementation during deserialization? Is there a tracking issue for this TODO?
crates/amaru-ledger/src/rules.rs (4)
2-2: Good modular organization with the new transaction moduleNice addition of the transaction module - keeping things well-organized!
41-45: Well-structured transaction rule violation enumGood job creating a dedicated enum for transaction validation errors. This clear categorization will make error handling and reporting much more straightforward.
86-127: Well-integrated transaction validation in the block validation processExcellent work incorporating transaction validation into the block validation process. The code retrieves all necessary components (transaction bodies, witness sets, auxiliary data), performs validation on each transaction, and properly maps errors to the block validation context.
134-137:❓ Verification inconclusive
Plans for unused parameters in validate_transaction
I notice there are several parameters with leading underscores indicating they're currently unused. Do you have plans to utilize these in future implementations? Including a TODO comment might be helpful if there are specific enhancements planned.
🏁 Script executed:
#!/bin/bash # Check for any existing TODOs related to these parameters rg "validate_transaction.*witness_set|is_valid" -A 5 -B 5Length of output: 2290
Clarify Future Plans for Unused Parameters in validate_transaction
I’ve noticed that the parameters
_witness_setand_is_validin thevalidate_transactionfunction (incrates/amaru-ledger/src/rules.rsaround lines 134–137) are currently unused. If these parameters are earmarked for future enhancements, could you add a TODO comment to indicate their intended use? Otherwise, if they're no longer required, it might be best to consider removing them to keep the code tidy.Cheers, and looking forward to your thoughts on this!
|
|
||
| for (i, (transaction, raw_transaction)) in | ||
| (0u32..).zip(transactions.iter().zip(raw_transactions)) | ||
| { |
There was a problem hiding this comment.
First zip might be replaced with transactions.iter().enumerate ?
This PR introduces some initial transaction validations for the block validation
Summary by CodeRabbit
New Features
Chores