Phase 2 Validation #553
Phase 2 Validation #553
Conversation
WalkthroughThis PR adds a thread-safe ArenaPool for Plutus evaluation, splits transaction validation into phase-one/phase-two with new error types, refactors ScriptContext/TxInfo/Redeemers and versioned ToPlutusData, threads ArenaPool through ledger/block validation and tests, updates workspace deps, and augments CI with a RISC‑V toolchain setup. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Ledger
participant BlockValidator
participant PhaseOne as Phase One
participant PhaseTwo as Phase Two
participant ArenaPool
participant UPLC as uplc-turbo
Client->>Ledger: roll_forward(point, raw_block, arena_pool)
Ledger->>BlockValidator: validate(arena_pool, network, ...)
BlockValidator->>PhaseOne: phase_one::execute(context, network, ...)
alt phase_one fails
PhaseOne-->>BlockValidator: PhaseOneError
BlockValidator-->>Ledger: Invalid (Transaction)
else phase_one ok
PhaseOne-->>BlockValidator: Ok(consumed_inputs)
BlockValidator->>PhaseTwo: phase_two::execute(context, arena_pool, ...)
loop per Plutus script
PhaseTwo->>ArenaPool: acquire()
ArenaPool-->>PhaseTwo: PooledArena
PhaseTwo->>UPLC: evaluate(program, args, cost_model)
alt evaluation fails & is_valid
UPLC-->>PhaseTwo: UplcMachineError
else evaluation ok or not required
UPLC-->>PhaseTwo: Success
end
Note right of PhaseTwo: PooledArena drops (arena reset) when scope ends
end
alt all scripts OK or failures allowed
PhaseTwo-->>BlockValidator: Ok(())
BlockValidator-->>Ledger: Valid
else any script failure invalidates
PhaseTwo-->>BlockValidator: PhaseTwoError
BlockValidator-->>Ledger: Invalid (Transaction)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
| &mut self, | ||
| point: &Point, | ||
| raw_block: &RawBlock, | ||
| arena_pool: &ArenaPool, |
There was a problem hiding this comment.
I am not a huge fan of the drilling of the arena_pool through so many methods/functions. I just couldn't quite find the right place to have the arena_pool live. It is not state, it isn't quite the validation context either.
Would love some thoughts about the best place this arena pool should live.
yHSJ
force-pushed
the
josh/phase-2-validation
branch
3 times, most recently
from
cf5cc33 to
ca8042e
Compare
yHSJ
force-pushed
the
josh/phase-2-validation
branch
from
bf400fb to
016afc9
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: 6
🧹 Nitpick comments (14)
.github/workflows/continuous-integration.yml (1)
61-66: Consider pinning the toolchain release or adding fallback/stability measures.The riscv32-glibc-llvm tarball is downloaded from a GitHub release with a hardcoded date tag. If that release is ever removed or the URL structure changes, the entire riscv32 build job fails silently mid-setup. You know how it is—external dependencies can be right dodgy sometimes, mate.
Consider one of these approaches:
Option 1: Mirror the tarball to a more stable storage (e.g., your own artifact repository or a pinned release in your repo) to reduce dependency on external GitHub release availability.
Option 2: Add integrity verification with checksums:
setup: | rustup component add rust-src --toolchain nightly curl -L -o riscv32-glibc-llvm.tar.xz \ "https://github.com/riscv-collab/riscv-gnu-toolchain/releases/download/2025.11.04/riscv32-glibc-ubuntu-24.04-llvm.tar.xz" + echo "expected_checksum riscv32-glibc-llvm.tar.xz" | sha256sum -c - tar -xvf riscv32-glibc-llvm.tar.xz export CC_riscv32gc_unknown_linux_gnu="$(pwd)/riscv/bin/riscv32-unknown-linux-gnu-clang"Option 3: Add validation that the expected compiler path exists after extraction:
tar -xvf riscv32-glibc-llvm.tar.xz + if [[ ! -f "$(pwd)/riscv/bin/riscv32-unknown-linux-gnu-clang" ]]; then + echo "Error: riscv32 compiler not found at expected path after extraction" + exit 1 + fi export CC_riscv32gc_unknown_linux_gnu="$(pwd)/riscv/bin/riscv32-unknown-linux-gnu-clang"crates/amaru-ledger/src/rules/transaction/phase_one/scripts.rs (1)
111-112: Righto, that's a fair dinkum placeholder mate!The TODO here makes sense given you're splitting validation into phases – scripts get their moment in the spotlight during phase two. Good on ya for leaving breadcrumbs.
Since this is marking future work, do you want me to help open an issue to track the script evaluation implementation? Happy to draft one up for ya!
crates/amaru/src/bin/amaru/cmd/mod.rs (1)
38-40: Arena defaults look sensible, mate!Ten arenas at a meg each is a fair starting point for script evaluation. Conservative enough to not blow up memory, but should handle most workloads.
Down the track, you might wanna make these configurable via CLI args (like
--max-arenasand--arena-size) for folks who need to tune performance. But that's a nice-to-have for later – no need to gold-plate it now.crates/amaru/src/stages/mod.rs (1)
113-114: Default values are consistent with CLI defaults.The hardcoded defaults
(10, 1_024_000)matchDEFAULT_MAX_ARENASandDEFAULT_ARENA_SIZEfrom the CLI module. Consider extracting these to shared constants to avoid drift.You could define these constants in the
stagesmodule and reference them from bothConfig::default()and the CLI defaults to ensure they stay in sync. Not critical, but would be a nice DRY improvement, like using the same save file across multiple playthroughs!crates/amaru-kernel/src/lib.rs (2)
817-851: Comprehensive HasOwnership implementation for Certificate.This exhaustively covers all 19 Certificate variants - impressive attention to detail! A few observations:
- The
PoolRegistrationandPoolRetirementcases correctly useStakeCredential::AddrKeyhashsince pool operators are identified by key hashes.- Using
.clone()onStakeCredentialis cheap (it's just an enum wrapping aHash<28>).- The verbose field matching in
PoolRegistration(lines 824-833) could use..to ignore unused fields, but it's also self-documenting as-is.If you fancy a slightly more concise version for
PoolRegistration:- Certificate::PoolRegistration { - operator: id, - vrf_keyhash: _vrf_keyhash, - pledge: _pledge, - cost: _cost, - margin: _margin, - reward_account: _reward_account, - pool_owners: _pool_owners, - relays: _relays, - pool_metadata: _pool_metadata, - } => Some(StakeCredential::AddrKeyhash(*id)), + Certificate::PoolRegistration { operator: id, .. } => Some(StakeCredential::AddrKeyhash(*id)),
1071-1084: Ordering implementation enables deterministic redeemer processing.The
Ordimplementation compares by tag index first, then by redeemer index - this matches the canonical ordering expected by the Cardano ledger for script execution.The match on Lines 1073-1075 could be slightly simplified, but it's perfectly readable as-is.
A wee bit more idiomatic Rust if you're feeling adventurous:
impl Ord for RedeemerAdapter { fn cmp(&self, other: &Self) -> Ordering { - match self.0.tag.as_index().cmp(&other.0.tag.as_index()) { - by_tag @ Ordering::Less | by_tag @ Ordering::Greater => by_tag, - Ordering::Equal => self.index.cmp(&other.index), - } + self.0.tag.as_index().cmp(&other.0.tag.as_index()) + .then_with(|| self.index.cmp(&other.index)) } }This uses
then_withwhich is the idiomatic way to chain comparisons in Rust - like chaining combos in a fighting game!crates/amaru-kernel/src/arena_pool.rs (3)
35-41: Two separate Mutexes – like dual-wielding, powerful but tricky.Having
arenasandtotal_createdas separate Mutexes works because you've maintained consistent lock ordering (arenas → total_created). However, this does add cognitive overhead – like trying to manage two skill trees simultaneously.A simpler approach could be to include
total_created: usizedirectly inside thearenasMutex guard (e.g.,Mutex<(VecDeque<Bump>, usize)>). This would eliminate the second lock and simplify reasoning about the state. But the current design is correct, so this is just food for thought.
64-95: FIXME comments noted – poison handling is worth revisiting.The
unwrap_or_else(|p| p.into_inner())pattern at lines 68, 79, and 94 essentially ignores mutex poisoning. For an arena pool, this is probably acceptable since a panic in one thread shouldn't invalidate the arena state itself – it's just memory allocation. But documenting this design decision would be ace, mate.If you do want stricter handling later, consider:
- Logging when a poisoned mutex is recovered
- Propagating the error for critical paths
43-59: Constructor looks good – consider validating inputs.The constructor is straightforward. One small suggestion: you might want to validate that
max_size > 0to prevent a configuration that would causeacquire()to block forever (since no arenas could ever be created). A debug assertion or explicit check would prevent "softlock" scenarios.pub fn new(max_size: usize, initial_capacity: usize) -> Self { + debug_assert!(max_size > 0, "ArenaPool max_size must be greater than 0"); Self { inner: Arc::new(Inner {crates/amaru-plutus/src/script_context/v3.rs (1)
522-534: CostModels TODO noted – validation against Haskell pending.Fair cop on the TODO comment here. The implementation looks reasonable – encoding the three optional cost model arrays (v1, v2, v3) in a constructor. But as you've noted, this needs validation against the canonical Haskell implementation before going to prod. Like testing your speedrun strats on practice mode first!
Consider creating a tracking issue for this validation work so it doesn't get lost in the sauce.
Would you like me to open an issue to track the CostModels validation against Haskell?
crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs (1)
155-155: The&(*network).into()pattern is safe, but you're adding an unnecessary outer reference.The
From<NetworkName>impl returns&EraHistorydirectly (pointing to static constants likeMAINNET_ERA_HISTORY), so the outer&is redundant—it's like putting extra toppings on a pizza that's already perfectly cooked, mate. You can simplify to(*network).into()on both lines 155 and 180. No data loss, no platform corruption gremlins hiding in the code; NetworkName maps one-to-one to its era history like a speedrun has only one frame-perfect route.crates/amaru-ledger/src/rules/block.rs (1)
253-273: Revisit the cloning strategy, especially for the witness set.Line 261 clones the transaction, which seems necessary given that
phase_one::executetakesKeepRawby value. But line 262 does&witness_set.clone(), which creates a temporary clone just to borrow it immediately. That's like ordering a flat white to-go and then drinking it in the café—it works, but it's not exactly efficient.Check if you can avoid the clone on line 262 by passing
witness_setdirectly, or if the clone is truly needed, store it in a variable first.Consider this refactor if the clone is necessary:
- let consumed_inputs = match transaction::phase_one::execute( + let cloned_witness_set = witness_set.clone(); + let consumed_inputs = match transaction::phase_one::execute( context, network, protocol_params, era_history, governance_activity, pointer, !failed_transactions.has(i), transaction.clone(), - &witness_set.clone(), + &cloned_witness_set, auxiliary_data, ) {crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (1)
140-143: Consider replacingunreachable!()with a proper error.Line 142: You're using
unreachable!()for Native scripts with redeemers, but if this invariant is ever violated (say, due to a bug elsewhere), you'll get a panic instead of a graceful error. It's like assuming your coffee machine will never break—until it does, and you're left stranded without your morning brew.Consider returning a proper error here instead.
Apply this diff:
- Script::Native(_) => { - unreachable!("cannot have a redeemer point to a native_script") - } + Script::Native(_) => { + return Err(PhaseTwoError::ScriptContextStateError( + PlutusDataError::InvalidScriptType + )); + }Note: You may need to add
InvalidScriptTypevariant toPlutusDataErroror choose an appropriate existing error.crates/amaru-plutus/src/script_context/mod.rs (1)
801-816: Same unreachable!() concern as in phase_two, mate.Line 813: You're using
unreachable!()for Native scripts again. While the comment says "a redeemer should never point to a native_script", defensive programming suggests returning an error would be safer. It's the same issue as inphase_two/mod.rsline 142—let's keep the error handling consistent and robust.Consider returning a
Result<Vec<u8>, Error>and handling Native scripts with a proper error variant.
📜 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 (25)
.github/workflows/continuous-integration.yml(1 hunks)Cargo.toml(2 hunks)crates/amaru-kernel/Cargo.toml(1 hunks)crates/amaru-kernel/src/arena_pool.rs(1 hunks)crates/amaru-kernel/src/lib.rs(4 hunks)crates/amaru-ledger/Cargo.toml(1 hunks)crates/amaru-ledger/src/block_validator.rs(4 hunks)crates/amaru-ledger/src/rules.rs(4 hunks)crates/amaru-ledger/src/rules/block.rs(5 hunks)crates/amaru-ledger/src/rules/transaction/mod.rs(1 hunks)crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs(7 hunks)crates/amaru-ledger/src/rules/transaction/phase_one/outputs.rs(1 hunks)crates/amaru-ledger/src/rules/transaction/phase_one/scripts.rs(1 hunks)crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs(1 hunks)crates/amaru-ledger/src/state.rs(3 hunks)crates/amaru-ledger/tests/evaluate_ledger_states.rs(3 hunks)crates/amaru-plutus/src/script_context/mod.rs(20 hunks)crates/amaru-plutus/src/script_context/v1.rs(8 hunks)crates/amaru-plutus/src/script_context/v2.rs(3 hunks)crates/amaru-plutus/src/script_context/v3.rs(7 hunks)crates/amaru-plutus/src/to_plutus_data.rs(2 hunks)crates/amaru/src/bin/amaru/cmd/mod.rs(1 hunks)crates/amaru/src/bin/amaru/cmd/run.rs(2 hunks)crates/amaru/src/stages/mod.rs(6 hunks)examples/shared/src/lib.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (17)
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 387
File: crates/amaru-stores/src/lib.rs:40-40
Timestamp: 2025-08-20T13:02:25.763Z
Learning: In the amaru-stores crate, amaru_slot_arithmetic types like Epoch and EraHistory are used throughout the main crate code in modules like in_memory/mod.rs, rocksdb/consensus.rs, and rocksdb/ledger/columns/, not just in tests. This means amaru-slot-arithmetic should be a regular dependency, not a dev-dependency.
Applied to files:
crates/amaru-kernel/Cargo.tomlcrates/amaru-ledger/src/rules.rsCargo.tomlcrates/amaru-kernel/src/lib.rscrates/amaru-ledger/Cargo.tomlcrates/amaru-ledger/src/state.rscrates/amaru/src/stages/mod.rscrates/amaru-ledger/tests/evaluate_ledger_states.rscrates/amaru-plutus/src/script_context/mod.rscrates/amaru-plutus/src/script_context/v3.rs
📚 Learning: 2025-08-08T14:46:53.013Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/pool_params.rs:107-116
Timestamp: 2025-08-08T14:46:53.013Z
Learning: In crates/amaru-kernel/src/pool_params.rs, when serializing Relay::SingleHostAddr IPv6 to text, the project intentionally reverses each 4-byte chunk before constructing std::net::Ipv6Addr. This matches cardano-ledger’s IPv6 representation (four little-endian Word32 chunks). Do not “simplify” by passing the raw 16 bytes directly to Ipv6Addr::from; that would break ledger compatibility.
Applied to files:
crates/amaru-ledger/src/rules.rscrates/amaru-plutus/src/to_plutus_data.rscrates/amaru-ledger/src/state.rscrates/amaru-ledger/tests/evaluate_ledger_states.rs
📚 Learning: 2025-05-12T14:21:27.470Z
Learnt from: stevana
Repo: pragma-org/amaru PR: 210
File: simulation/amaru-sim/src/simulator/simulate.rs:264-277
Timestamp: 2025-05-12T14:21:27.470Z
Learning: The team plans to replace the out-of-process test in `simulation/amaru-sim/src/simulator/simulate.rs` with an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (`../../target/debug/echo`) and making tests more reliable.
Applied to files:
crates/amaru-ledger/src/rules.rscrates/amaru-ledger/tests/evaluate_ledger_states.rs
📚 Learning: 2025-08-23T15:44:44.318Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 392
File: crates/ouroboros-traits/src/is_header/fake.rs:52-88
Timestamp: 2025-08-23T15:44:44.318Z
Learning: In the Amaru project, FakeHeader in crates/ouroboros-traits/src/is_header/fake.rs is specifically designed for testing purposes, and panic behavior (like copy_from_slice() panicking on malformed hex) is the preferred approach rather than graceful error handling, as it helps identify test data issues quickly during development.
Applied to files:
crates/amaru-ledger/src/rules.rs
📚 Learning: 2025-06-14T16:38:35.449Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 263
File: crates/amaru-consensus/src/consensus/store.rs:220-223
Timestamp: 2025-06-14T16:38:35.449Z
Learning: In `NetworkName::Preprod.into()` when converting to `&EraHistory`, the From implementation returns a static reference to a constant value, not a temporary. This makes it safe to return directly from functions expecting `&EraHistory` without storing it in a struct field.
Applied to files:
crates/amaru-ledger/src/rules.rs
📚 Learning: 2025-06-14T16:31:53.134Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 263
File: simulation/amaru-sim/src/simulator/simulate.rs:298-300
Timestamp: 2025-06-14T16:31:53.134Z
Learning: StageRef in the pure-stage crate supports serde serialization and deserialization (derives serde::Serialize and serde::Deserialize), enabling it to be used in structs that also derive these traits for TraceBuffer and replay functionality.
Applied to files:
Cargo.toml
📚 Learning: 2025-04-04T16:49:53.462Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 0
File: :0-0
Timestamp: 2025-04-04T16:49:53.462Z
Learning: The ledger validation architecture abstracts state management from validations using traits like UtxoStateManagement, PoolsStateManagement, AccountsStateManagement, etc. to keep validation rules lean and independent of storage concerns.
Applied to files:
crates/amaru-ledger/src/block_validator.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In crates/amaru-consensus, flood prevention for duplicate invalid headers should be implemented in the validate_header stage or database layer, not in the store_header stage, since store_header runs before validation and cannot determine header validity.
Applied to files:
crates/amaru-ledger/src/block_validator.rs
📚 Learning: 2025-04-20T17:57:23.233Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:272-285
Timestamp: 2025-04-20T17:57:23.233Z
Learning: In test infrastructure code, rkuhn prefers explicit panics (using .unwrap() or similar) over returning Result types, as test failures should be immediate and obvious.
Applied to files:
crates/amaru-kernel/src/lib.rscrates/amaru-ledger/tests/evaluate_ledger_states.rs
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, ValidateHeader::handle_roll_forward returns a Result<PullEvent, ConsensusError>, not ValidateHeaderEvent as might be expected from the older code structure.
Applied to files:
crates/amaru-ledger/src/state.rs
📚 Learning: 2025-02-03T11:15:22.640Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 75
File: crates/amaru/src/consensus/mod.rs:164-165
Timestamp: 2025-02-03T11:15:22.640Z
Learning: In the Amaru project, chain selection operations (roll_forward and rollback) should use separate result types to leverage the type system for preventing impossible states, rather than using runtime checks or panics.
Applied to files:
crates/amaru-ledger/src/state.rs
📚 Learning: 2025-09-29T16:44:14.807Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/protocol.rs:94-106
Timestamp: 2025-09-29T16:44:14.807Z
Learning: In the amaru-network crate protocol.rs file, the correct Cardano mini-protocol ID assignments are: PROTO_N2N_KEEP_ALIVE = 8 and PROTO_N2N_PEER_SHARE = 10, as verified against the network specification by the maintainer.
Applied to files:
crates/amaru-ledger/src/state.rscrates/amaru-plutus/src/script_context/v1.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-09-29T20:08:29.906Z
Learnt from: yHSJ
Repo: pragma-org/amaru PR: 453
File: crates/amaru-metrics/src/ledger.rs:60-104
Timestamp: 2025-09-29T20:08:29.906Z
Learning: In amaru-ledger/src/state.rs, the txs_processed field represents the number of transactions in the current block (block.transaction_bodies.len() as u64), not a cumulative total. Therefore, using a Counter with .add() in the metrics is correct for accumulating per-block transaction counts.
Applied to files:
crates/amaru-ledger/src/rules/block.rs
📚 Learning: 2025-08-18T08:10:32.640Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 374
File: crates/amaru-stores/src/in_memory/mod.rs:427-433
Timestamp: 2025-08-18T08:10:32.640Z
Learning: The MemoryStore in crates/amaru-stores/src/in_memory/mod.rs is planned for a major revamp, so unimplemented methods like set_proposals_roots and set_constitution are intentionally left as placeholders until the revamp is complete.
Applied to files:
crates/amaru-plutus/src/script_context/v1.rscrates/amaru-plutus/src/script_context/v3.rs
📚 Learning: 2025-01-31T09:19:55.491Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 64
File: crates/amaru/src/ledger/state/transaction.rs:62-68
Timestamp: 2025-01-31T09:19:55.491Z
Learning: In the Cardano ledger, withdrawal uniqueness is guaranteed by the on-wire format's serialization, which considers duplicate withdrawals as invalid. This guarantee is reflected in Rust's type system, making additional runtime uniqueness checks unnecessary even when using vectors internally, as the data originates from minted blocks.
Applied to files:
crates/amaru-plutus/src/script_context/v1.rs
📚 Learning: 2025-05-21T18:58:48.631Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 228
File: crates/amaru-stores/src/rocksdb/consensus.rs:89-128
Timestamp: 2025-05-21T18:58:48.631Z
Learning: The InMemConsensusStore implementation in crates/amaru-stores/src/rocksdb/consensus.rs will be fleshed out incrementally on a by-need basis, driven by test requirements rather than implementing all functionality upfront.
Applied to files:
crates/amaru/src/stages/mod.rs
📚 Learning: 2025-08-18T08:10:35.849Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 374
File: crates/amaru-stores/src/in_memory/mod.rs:431-433
Timestamp: 2025-08-18T08:10:35.849Z
Learning: The MemoryStore in crates/amaru-stores/src/in_memory/mod.rs is planned for a major revamp soon, so unimplemented methods like set_constitution, set_proposals_roots are intentionally left aside until the revamp is complete.
Applied to files:
crates/amaru-plutus/src/script_context/v3.rs
🧬 Code graph analysis (8)
crates/amaru-ledger/src/rules.rs (2)
crates/amaru-ledger/src/state.rs (4)
network(222-224)protocol_parameters(230-232)new(144-167)new(1060-1070)crates/amaru-kernel/src/protocol_parameters.rs (1)
new(479-492)
crates/amaru-plutus/src/to_plutus_data.rs (1)
crates/amaru-plutus/src/script_context/v3.rs (16)
to_plutus_data(39-48)to_plutus_data(52-61)to_plutus_data(65-87)to_plutus_data(91-100)to_plutus_data(104-113)to_plutus_data(117-119)to_plutus_data(123-125)to_plutus_data(129-141)to_plutus_data(144-186)to_plutus_data(190-197)to_plutus_data(203-276)to_plutus_data(280-296)to_plutus_data(301-310)to_plutus_data(324-363)to_plutus_data(367-369)to_plutus_data(373-375)
crates/amaru-kernel/src/arena_pool.rs (1)
crates/amaru-ledger/src/block_validator.rs (1)
new(40-53)
crates/amaru-ledger/src/block_validator.rs (1)
crates/amaru-kernel/src/arena_pool.rs (1)
new(49-59)
crates/amaru-plutus/src/script_context/v2.rs (1)
crates/amaru-plutus/src/script_context/v1.rs (13)
to_plutus_data(31-33)to_plutus_data(42-44)to_plutus_data(48-72)to_plutus_data(87-105)to_plutus_data(116-129)to_plutus_data(137-142)to_plutus_data(146-148)to_plutus_data(155-157)to_plutus_data(190-224)to_plutus_data(229-241)to_plutus_data(252-263)to_plutus_data(267-269)to_plutus_data(273-275)
crates/amaru-ledger/src/rules/block.rs (2)
crates/amaru-kernel/src/network.rs (1)
fmt(458-465)crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs (1)
execute(89-230)
crates/amaru-plutus/src/script_context/v1.rs (1)
crates/amaru-kernel/src/lib.rs (8)
script_hash(864-864)script_hash(868-875)script_hash(879-881)script_hash(885-887)script_hash(895-897)stake_credential(762-762)stake_credential(766-775)normalize_redeemers(1054-1069)
crates/amaru-ledger/tests/evaluate_ledger_states.rs (1)
crates/amaru-ledger/src/state.rs (1)
network(222-224)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build x86_64/windows
- GitHub Check: Build wasm32
- GitHub Check: Build x86_64/linux
- GitHub Check: Build aarch64/macos
- GitHub Check: Benches
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (51)
crates/amaru-ledger/tests/evaluate_ledger_states.rs (2)
19-19: Network type migration looks spot on!Switching from
NetworktoNetworkNamekeeps things cleaner and more explicit. Nice one!
215-229: Phase one execution wired up properly.The switch to
phase_one::executewithNetworkName::Preprodand the updated expectation fromOk(())toOk(_)tells me you're now returning the consumed inputs. That's the right move for a multi-phase validation setup – phase one does its bit and hands over the baton.crates/amaru-ledger/src/rules/transaction/phase_one/outputs.rs (1)
140-143: Import cleanup looks good!Just shuffling the deck chairs to make the module structure cleaner. No dramas here.
crates/amaru-ledger/src/rules.rs (4)
139-142: Imports updated for ArenaPool and NetworkName.Bringing in the cavalry for arena-based allocation. Looking good!
175-175: Static arena pool for tests – nice and tidy!Using a
LazyLockto initialize the arena pool once for all tests is the right call. Saves you from creating/destroying pools in every test case. The 10 arenas at 1MB each matches your CLI defaults too – consistency across the board!
187-188: Block validation wired up with ArenaPool.Passing the arena pool through to validation is exactly what you need for phase two script execution. And
NetworkName::Preprodkeeps things consistent with the other test updates.
220-221: Arena pool threaded through header size test.Same deal here – you're properly passing the arena pool through even in the negative test case. Consistency is key, and you've nailed it!
crates/amaru-ledger/src/rules/transaction/mod.rs (1)
15-16: Phase module structure established – beauty!Clean and simple module file exposing the two validation phases. This sets up the public API exactly as you'd want it for a phased validation approach. Like splitting a boss fight into two stages – you gotta beat phase one before you can tackle phase two!
crates/amaru-kernel/Cargo.toml (1)
46-46: Bumpalo's getting proper use in the kernel crate. Found it imported inarena_pool.rs, where it's doing exactly what you'd hope—powering theBumpallocator for the ArenaPool. Dependency is legit, no dead weight here, mate.crates/amaru-ledger/Cargo.toml (2)
39-39: Dependency is legit – amaru-plutus is actively used in the crate.Found it imported in
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs:23. Not just cruft; this one's got a proper job to do.
30-30: No worries, mate—uplc-turbo is legit being used.Found it pulling weight in
phase_two/mod.rswhere it's handling the PlutusVersion enum for V2 and V3 support. It's not just sitting on the bench—it's in the game doing the heavy lifting for Plutus script execution. All good here.examples/shared/src/lib.rs (2)
46-47: LGTM! Arena pool wired up nicely for the example.The hardcoded values
(10, 1_024_000)match the defaults used in the main CLI (DEFAULT_MAX_ARENASandDEFAULT_ARENA_SIZE). For example code, this is totally grand - keeps it self-contained without needing to import constants.
110-122: Block validation call updated correctly.The
arena_poolreference is passed beforenetwork, matching the updatedvalidate_blocksignature. Like a well-choreographed dance scene in La La Land - everything's in sync!crates/amaru-ledger/src/block_validator.rs (3)
36-36: Public field exposure is intentional here.The
arena_poolfield beingpubmakes sense given thatstateis also public. SinceArenaPoolusesArcinternally (as seen in the kernel implementation), it's cheap to clone and thread-safe. No worries here - it's like having a shared Netflix account that everyone can use without stepping on each other's toes!
40-53: Constructor properly threads the arena_pool through.The
arena_poolis accepted and stored correctly. The initialization order makes sense - store first, then arena_pool, then other config. Clean as a whistle!
74-81: Roll forward correctly passes arena_pool reference.Passing
&self.arena_pooltoroll_forwardis the right call since the pool is shared and internally synchronized viaArc<Mutex<...>>. This avoids unnecessary cloning while maintaining thread safety.crates/amaru/src/bin/amaru/cmd/run.rs (2)
93-104: CLI argument structure looks solid.The new
max_arenasargument follows the established pattern nicely - long flag, env var, default value. Documentation clearly explains the memory/performance trade-off. Good stuff!
171-172: Config fields properly wired from CLI args.Both
max_arenasandarena_sizeare correctly passed through to theConfigstruct. The plumbing is complete!crates/amaru-plutus/src/script_context/v2.rs (2)
85-90: Withdrawals serialization now ensures deterministic ordering.The change to materialize the inner map via
iter().collect::<BTreeMap<_, _>>()before delegating ensures consistent ordering regardless of the underlyingKeyValuePairsvariant. This is important for Plutus data serialization where determinism matters - like speedrunning, every byte needs to be reproducible!
109-126: StakeAddress PlutusData implementations follow established v1 pattern.The implementation correctly mirrors the v1 version (from
v1.rsLines 136-147) but usesconstr_v2!macros. The documentation aboutPointervsCredentialvariants is helpful context. Clean and consistent!crates/amaru/src/stages/mod.rs (3)
175-182: Arena pool creation and propagation looks correct.The
ArenaPoolis created once at bootstrap time using the configured values, then passed down tomake_ledger. This ensures a single pool instance is shared across the validation pipeline - efficient resource management!Regarding the drilling concern from the previous review: the current approach is pragmatic. The arena pool isn't quite "state" (it's a resource pool) and isn't validation context either. Storing it in
BlockValidatorseems like a reasonable home since that's where validation happens. It's like keeping your game controller plugged into the console - not part of the game state, but needed for gameplay!
97-98: Config struct extended with arena configuration.New fields integrate cleanly with the existing config pattern.
338-371: Both ledger paths correctly receive the arena_pool.The
make_ledgerfunction properly threadsarena_poolthrough toBlockValidator::newfor bothInMemandRocksDbstore types. No path is left without arena access.crates/amaru-ledger/src/state.rs (2)
601-606: Public API extended with arena_pool parameter.The
roll_forwardmethod now acceptsarena_pool: &ArenaPoolas a parameter. This is a public API change that callers need to update - but all call sites in this PR have been updated accordingly.Passing by reference is the right choice since
ArenaPoolusesArcinternally and we don't need ownership transfer.
617-625: Validation call correctly forwards arena_pool.The
arena_poolis passed first torules::validate_block, followed byself.network(). The ordering matches the updated function signature. Like passing the baton in a relay race - smooth handoff!crates/amaru-kernel/src/lib.rs (2)
179-180: Arena pool module cleanly integrated into kernel exports.Follows the established pattern of
mod+pub use *. External crates can now accessArenaPoolthrough the kernel prelude. Nice and tidy!
970-994: RedeemerAdapter provides clean conversion interface.The newtype wrapper around
RedeemerwithDerefandFromimplementations allows seamless conversion from both owned and borrowed redeemers. This is handy for phase 2 validation where redeemers need to be collected and ordered.crates/amaru-plutus/src/to_plutus_data.rs (1)
159-174: Acknowledge the workaround – fair dinkum, mate!G'day! This match-on-V approach is like using a cheat code in a game – it works, but it's not exactly Dark Souls elegant, ya know? The good news is the
IsKnownPlutusVersiontrait bound at line 118 guarantees we'll never hit thatunreachable!()in practice, so it's safe as houses.The comment's honest self-assessment is appreciated – like Geralt admitting he's not great at dancing. When you circle back to clean this up, consider whether a helper trait with associated types could let you dispatch without the explicit match. But for now, this does the job.
crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs (3)
59-86: Nice rename toPhaseOneError– consistency is key, like matching your gear set!Solid rename from
InvalidTransactiontoPhaseOneError. This aligns beautifully with thePhaseTwoErrorin the phase_two module, giving the error taxonomy a cohesive vibe – like when all your party members finally have matching legendary gear.
100-103: Return type change improves function purity – top shelf!Returning
Vec<TransactionInput>instead of mutating context internally is a cracker of a refactor. It's like going from "the princess is in another castle" side effects to clean functional style where the caller controls when inputs get consumed. Makes testing easier and the data flow more explicit.
220-229: Clean return of consumed inputs – good stuff!This change from mutating context to returning the
consumed_inputsvector is like choosing the non-lethal playthrough – less side effects, more control for the caller. The logic determining which inputs to consume (regular vs collateral based onis_valid) remains correct.crates/amaru-kernel/src/arena_pool.rs (1)
171-183: Solid RAII pattern – ManuallyDrop usage is correct!The Drop implementation is clean as a whistle. The unsafe
ManuallyDrop::takeat line 174 is used correctly – only called once during drop, exactly as the pattern intends. Thearena.reset()ensures memory is reclaimed before returning to pool. Nice work implementing the RAII guard pattern properly – like a well-designed inventory system that auto-organizes.crates/amaru-plutus/src/script_context/v3.rs (2)
640-653: New StakeAddress implementations – looking good!The ToPlutusData<3> implementations for both
amaru_kernel::StakeAddressand the wrapperStakeAddressare clean. Matching onStakePayload::StakevsStakePayload::Scriptand encoding to appropriate constructors (0 for keyhash, 1 for script) aligns with the Plutus data model. Top shelf work, mate!
672-676: Good call disabling the v9 test – pragmatic decision.The commented-out v9 test at line 675 with the explanatory comment is honest about the scope. Since you're explicitly disregarding protocol version 9's quirky behavior, documenting why the test is disabled prevents future confusion. It's like leaving a note explaining why you skipped that one optional boss – not worth the hassle for the loot.
crates/amaru-plutus/src/script_context/v1.rs (4)
132-149: StakeAddress handling with Pointer clarification – nice documentation!The comments at lines 133-136 explaining why the Pointer variant can be hardcoded to
Constr(0, [cred])is really helpful context. It's like those loading screen tips that actually teach you something useful about the game mechanics. The ledger serialization constraints guarantee we'll never see a Pointer-based StakeAddress here, so this is safe.
160-225: Certificate refactor usingDerefandPallasCertificate– cleaner matching!Using
self.deref()to get the underlyingPallasCertificateat line 191 makes the match statement more explicit about what's being matched against. The extended documentation (lines 167-189) explaining the translation rules and constraints is chef's kiss – documentation that actually documents!The wildcard catch-all at line 219 for unsupported certificate types with a descriptive error is the right call. Better to fail explicitly than silently misbehave.
266-270: Withdrawals serialization simplified – less allocations, more better!The change from building an intermediate vector to
self.0.iter().collect::<Vec<_>>()is a small but clean improvement. It's like optimizing your inventory management – same result, less faffing about.
331-335: Test updates for PROTOCOL_VERSION_10 – consistency across versions!The test harness updates mirror the v3 changes, keeping the test infrastructure consistent across Plutus versions. Using
PROTOCOL_VERSION_10and the updatedScriptContext::newsignature ensures the tests reflect the actual API. Good synchronization!crates/amaru-ledger/src/rules/block.rs (3)
275-291: LGTM on the phase-two integration!Nice work wiring up the arena pool and phase-two validation. The error handling flows cleanly through
TransactionInvalid, and you're passing references to phase_two, which is bang-on for performance.
293-295: Spot-on with the input consumption timing!Consuming the inputs only after both phases succeed is the right move. It's like making sure both your coffee and your smashed avo are ready before you start breakfast—no point committing to one if the other's gonna fail.
216-219: No action needed here, mate. I had the same suspicion you did, but after checking the logic, bothis_validand!failed_transactions.has(i)are computing the exact same thing fromblock.invalid_transactions— they're just two sides of the same coin. It's like watching The Matrix and The Matrix Reloaded; they look different on the surface, but they're telling the same story. Both phase_one and phase_two are getting consistent information about whether each transaction is valid. The precomputation is clean, and there's no Lost in Translation moment to worry about here.Likely an incorrect or invalid review comment.
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (4)
39-57: Solid error handling setup!The
PhaseTwoErrorenum covers all the bases—missing inputs, translation failures, script issues, and machine errors. It's like having a Swiss Army knife for error scenarios. Well done!
82-84: LGTM on the early return!If there's no redeemer, there's nothing to validate—fair call to short-circuit here. It's like showing up to a party and realizing it's cancelled; might as well head home early.
235-242: The validity state logic looks bang-on!This handles the case where
is_valid=false(collateral path) correctly: scripts should fail, and if they don't, you flag it asValidityStateError. It's like expecting a movie to be bad and being disappointed when it's actually good—wait, that's backwards. Anyway, the logic checks out!
187-190: Theexpecthere is a ticking time bomb, mate.Lines 187-189: You're using
expectwith a clippy allow, which means if the PlutusData CBOR decoding ever fails, this will panic and take down the whole validation. That's a bit like playing Russian roulette with your ledger state!The TODO comment mentions this should be fixed. Consider prioritizing this, as it's a potential correctness/reliability issue.
Do you want me to help generate a proper error handling path here, or open an issue to track this technical debt?
⛔ Skipped due to learnings
Learnt from: rkuhn Repo: pragma-org/amaru PR: 149 File: crates/amaru/src/stages/consensus/chain_forward.rs:73-75 Timestamp: 2025-04-20T18:02:25.073Z Learning: In the current development stage, rkuhn prefers using explicit panics (via `.expect()` or `.unwrap()`) for fatal errors in the application code that would tear down the node, rather than propagating errors with `Result`. The intention is to eventually transition to proper error handling with `Result` as the codebase matures.Learnt from: KtorZ Repo: pragma-org/amaru PR: 370 File: crates/amaru-kernel/src/borrowed_datum.rs:32-39 Timestamp: 2025-08-08T14:39:50.527Z Learning: In the amaru project, when converting BorrowedDatumOption::Data to an owned DatumOption in crates/amaru-kernel/src/borrowed_datum.rs, the call `.unwrap()` refers to pallas’s KeepRaw::unwrap, which is infallible (always returns the inner value) and is not a panic risk. Future reviews should not flag this unwrap as dangerous.Learnt from: abailly Repo: pragma-org/amaru PR: 435 File: crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs:113-116 Timestamp: 2025-09-06T09:16:25.025Z Learning: In cardano-node serialized ledger state CBOR encoding, indefinite-length structures may be terminated with 0xFF "break" markers. The current code in convert_ledger_state.rs unconditionally strips the last byte (bytes[p..bytes.len() - 1]), which could corrupt data if the trailing byte is not actually a CBOR break marker.Learnt from: jeluard Repo: pragma-org/amaru PR: 69 File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117 Timestamp: 2025-01-21T15:32:17.911Z Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.Learnt from: KtorZ Repo: pragma-org/amaru PR: 370 File: crates/minicbor-extra/src/lib.rs:50-55 Timestamp: 2025-08-08T14:34:06.105Z Learning: Project uses Rust 1.88 stable; using Result::is_err_and is acceptable and should not be flagged as unstable. In particular, crates/minicbor-extra/src/lib.rs relies on is_err_and when checking end_of_input; future reviews should not suggest replacing it for stability reasons.Learnt from: KtorZ Repo: pragma-org/amaru PR: 370 File: crates/amaru-kernel/src/transaction_pointer.rs:36-44 Timestamp: 2025-08-08T14:35:35.562Z Learning: In the amaru project, when decoding CBOR arrays, prefer using minicbor_extra::heterogenous_array with the expected length to validate definite-length arrays and correctly handle indefinite-length arrays. Example: crates/amaru-kernel/src/transaction_pointer.rs Decode should use heterogenous_array(d, 2, …) instead of ignoring the length from d.array().Learnt from: rkuhn Repo: pragma-org/amaru PR: 149 File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:272-285 Timestamp: 2025-04-20T17:57:23.233Z Learning: In test infrastructure code, rkuhn prefers explicit panics (using .unwrap() or similar) over returning Result types, as test failures should be immediate and obvious.crates/amaru-plutus/src/script_context/mod.rs (6)
124-126: LGTM on the budget accessor!Clean and straightforward method for accessing the execution units budget. No dramas here!
81-92: The datum resolution logic looks solid!You're correctly handling all three datum cases (None, Hash, Inline) for Spend redeemers. The logic flows like a good espresso—smooth and well-structured.
217-372: The TxInfo construction is complex but well-structured!You've got a lot going on here—collecting scripts from witness sets, building the script table, and wiring it all together. The
#[allow(clippy::too_many_arguments)]is justified given the complexity of transaction construction. The logic looks sound, and the script table integration is clean.
375-383: LGTM on the script context generation!Nice and clean method for producing all the script contexts needed for validation. The filter_map approach elegantly handles cases where ScriptContext::new returns None.
840-849: LGTM on the HasScriptHash implementation!Clean and straightforward delegation to
compute_hashfor each script variant. No worries here!
423-526: The ScriptPurpose::builder is complex but well-implemented!You've got a lot of arms in this match statement, handling all the different redeemer tags (Spend, Mint, Reward, Cert, Vote, Propose). The logic for each case looks correct—you're looking up the appropriate script, inserting it into the script_table, and building the right ScriptPurpose. It's like a choose-your-own-adventure book, except all paths lead to properly validated scripts.
The
#[allow(clippy::too_many_arguments)]is justified here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/amaru-ledger/src/rules/block.rs (2)
216-221: Good call on leaving the TODO, but let's make sure this gets tracked.The logic here assumes
Nonemeans "no invalid transactions" which feels right, but as the TODO notes, we need to confirm against the Haskell reference implementation. It's a bit like trusting a wiki walkthrough without checking the actual game mechanics yourself – usually fine, but occasionally you get burned!Worth opening an issue to track this verification so it doesn't slip through the cracks.
Would you like me to open an issue to track verifying this
invalid_transactionshandling against the Haskell ledger spec?
263-264: Quick check on those clones.The
transaction.clone()makes sense since you needtransactionagain for phase two. Butwitness_set.clone()might be avoidable if phase one can take a reference instead? Worth a quick peek at whetherphase_one::executeactually needs ownership.Not a blocker – just a "nice to have" for performance if cloning is expensive here.
#!/bin/bash # Check the phase_one execute signature for witness_set parameter type rg -n "pub fn execute" crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs -A 15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/amaru-ledger/src/rules/block.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-01-21T15:32:17.911Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 69
File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117
Timestamp: 2025-01-21T15:32:17.911Z
Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.
Applied to files:
crates/amaru-ledger/src/rules/block.rs
📚 Learning: 2025-09-29T20:08:29.906Z
Learnt from: yHSJ
Repo: pragma-org/amaru PR: 453
File: crates/amaru-metrics/src/ledger.rs:60-104
Timestamp: 2025-09-29T20:08:29.906Z
Learning: In amaru-ledger/src/state.rs, the txs_processed field represents the number of transactions in the current block (block.transaction_bodies.len() as u64), not a cumulative total. Therefore, using a Counter with .add() in the metrics is correct for accumulating per-block transaction counts.
Applied to files:
crates/amaru-ledger/src/rules/block.rs
🧬 Code graph analysis (1)
crates/amaru-ledger/src/rules/block.rs (4)
crates/amaru-kernel/src/network.rs (1)
fmt(458-465)crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (1)
execute(68-243)crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs (1)
execute(89-230)crates/amaru-ledger/src/rules/transaction/phase_one/metadata.rs (1)
execute(33-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build aarch64/macos
- GitHub Check: Build riscv32
- GitHub Check: Build x86_64/linux
- GitHub Check: Build x86_64/windows
- GitHub Check: Analyze (rust)
- GitHub Check: Test coverage
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Benches
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
crates/amaru-ledger/src/rules/block.rs (4)
39-45: Nice work on the error abstraction, mate!The
TransactionInvalidenum is a solid pattern here – usingthiserrorwith#[from]for automatic conversion from bothPhaseOneErrorandPhaseTwoErrorkeeps the error handling clean and composable. Like a well-designed skill tree in an RPG, each error type slots in nicely.
60-64: All good here!The
violation: TransactionInvalidfield ties in perfectly with the new error abstraction. TheDisplayimpl downstream handles it gracefully too.
174-181: Function signature update looks grand!The addition of
arena_pooland switch toNetworkNamealigns well with the phase two validation requirements. Like upgrading your gear before a boss fight – you've got everything you need now.
294-298: The consume-after-validation pattern is spot on!Consuming inputs only after both phases pass is the right call – like saving your game after beating a tough section, not before. Solid approach.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/amaru-plutus/src/script_context/mod.rs (1)
56-62: The publicredeemerfield is still exposed with a TODO, mate.This was flagged in a previous review — the TODO says it shouldn't be exposed, but it's still
pub. Like leaving the cheat codes enabled after playtesting.Consider using a
#[cfg(test)]accessor method instead to keep the field private in production builds.🔎 Proposed fix
pub struct ScriptContext<'a> { tx_info: &'a TxInfo<'a>, - // TODO: this should not be exposed, this is just for testing purposes atm - pub redeemer: &'a Redeemer, + redeemer: &'a Redeemer, datum: Option<&'a PlutusData>, script_purpose: &'a ScriptPurpose<'a>, } +#[cfg(test)] +impl<'a> ScriptContext<'a> { + pub fn redeemer(&self) -> &'a Redeemer { + self.redeemer + } +}
🧹 Nitpick comments (2)
crates/amaru-kernel/src/ordered_redeemer.rs (1)
18-30: Ord implementation is correct and well-documented!The comparison logic (tag first, then index) matches what the doc comment claims about Haskell compatibility. Like syncing save files across platforms — gotta keep the order consistent.
One small suggestion: the match on lines 25-28 could be simplified using
then()orthen_with(), matching the pattern used elsewhere in the codebase (seeProposalIdAdapter::cmpin lib.rs lines 335-341).🔎 Optional simplification
impl Ord for OrderedRedeemer<'_> { fn cmp(&self, other: &Self) -> Ordering { - match self.tag.as_index().cmp(&other.tag.as_index()) { - by_tag @ Ordering::Less | by_tag @ Ordering::Greater => by_tag, - Ordering::Equal => self.index.cmp(&other.index), - } + self.tag + .as_index() + .cmp(&other.tag.as_index()) + .then_with(|| self.index.cmp(&other.index)) } }crates/amaru-plutus/src/script_context/mod.rs (1)
238-246: Consider using field init shorthand on line 241.Since the variable and field have the same name, you can use the shorthand syntax. A tiny nit, but keeps things DRY like a proper Aussie summer.
🔎 Shorthand fix
set.iter() .map(|certificate| Certificate { - protocol_version: protocol_version, + protocol_version, certificate, }) .collect()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Cargo.toml(2 hunks)crates/amaru-kernel/src/lib.rs(3 hunks)crates/amaru-kernel/src/ordered_redeemer.rs(1 hunks)crates/amaru-ledger/Cargo.toml(1 hunks)crates/amaru-ledger/src/block_validator.rs(3 hunks)crates/amaru-plutus/src/script_context/mod.rs(17 hunks)crates/amaru-plutus/src/script_context/v1.rs(9 hunks)crates/amaru-plutus/src/script_context/v2.rs(2 hunks)crates/amaru-plutus/src/script_context/v3.rs(8 hunks)crates/amaru-plutus/src/to_plutus_data.rs(2 hunks)crates/amaru/src/bin/amaru/cmd/run.rs(2 hunks)crates/amaru/src/stages/mod.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/amaru-ledger/src/block_validator.rs
- crates/amaru-plutus/src/to_plutus_data.rs
- crates/amaru/src/bin/amaru/cmd/run.rs
- crates/amaru/src/stages/mod.rs
- Cargo.toml
- crates/amaru-plutus/src/script_context/v3.rs
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 387
File: crates/amaru-stores/src/lib.rs:40-40
Timestamp: 2025-08-20T13:02:25.763Z
Learning: In the amaru-stores crate, amaru_slot_arithmetic types like Epoch and EraHistory are used throughout the main crate code in modules like in_memory/mod.rs, rocksdb/consensus.rs, and rocksdb/ledger/columns/, not just in tests. This means amaru-slot-arithmetic should be a regular dependency, not a dev-dependency.
Applied to files:
crates/amaru-ledger/Cargo.tomlcrates/amaru-kernel/src/lib.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-04-20T17:57:23.233Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:272-285
Timestamp: 2025-04-20T17:57:23.233Z
Learning: In test infrastructure code, rkuhn prefers explicit panics (using .unwrap() or similar) over returning Result types, as test failures should be immediate and obvious.
Applied to files:
crates/amaru-kernel/src/lib.rs
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru-kernel/src/lib.rscrates/amaru-kernel/src/ordered_redeemer.rscrates/amaru-plutus/src/script_context/v1.rscrates/amaru-plutus/src/script_context/v2.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-08-08T14:46:53.013Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/pool_params.rs:107-116
Timestamp: 2025-08-08T14:46:53.013Z
Learning: In crates/amaru-kernel/src/pool_params.rs, when serializing Relay::SingleHostAddr IPv6 to text, the project intentionally reverses each 4-byte chunk before constructing std::net::Ipv6Addr. This matches cardano-ledger’s IPv6 representation (four little-endian Word32 chunks). Do not “simplify” by passing the raw 16 bytes directly to Ipv6Addr::from; that would break ledger compatibility.
Applied to files:
crates/amaru-plutus/src/script_context/v1.rs
📚 Learning: 2025-01-31T09:19:55.491Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 64
File: crates/amaru/src/ledger/state/transaction.rs:62-68
Timestamp: 2025-01-31T09:19:55.491Z
Learning: In the Cardano ledger, withdrawal uniqueness is guaranteed by the on-wire format's serialization, which considers duplicate withdrawals as invalid. This guarantee is reflected in Rust's type system, making additional runtime uniqueness checks unnecessary even when using vectors internally, as the data originates from minted blocks.
Applied to files:
crates/amaru-plutus/src/script_context/v1.rs
📚 Learning: 2025-09-29T16:44:14.807Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/protocol.rs:94-106
Timestamp: 2025-09-29T16:44:14.807Z
Learning: In the amaru-network crate protocol.rs file, the correct Cardano mini-protocol ID assignments are: PROTO_N2N_KEEP_ALIVE = 8 and PROTO_N2N_PEER_SHARE = 10, as verified against the network specification by the maintainer.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
🧬 Code graph analysis (2)
crates/amaru-kernel/src/ordered_redeemer.rs (1)
crates/amaru-kernel/src/lib.rs (5)
cmp(336-341)partial_cmp(330-332)deref(278-280)deref(295-297)deref(353-355)
crates/amaru-plutus/src/script_context/mod.rs (3)
crates/amaru-kernel/src/lib.rs (13)
from(284-286)from(307-309)from(345-347)from(513-518)from(522-529)from(581-589)datum(691-691)datum(695-706)redeemers(980-980)redeemers(998-1017)deref(278-280)deref(295-297)deref(353-355)crates/amaru-kernel/src/ordered_redeemer.rs (4)
from(47-49)from(53-55)from(59-61)deref(41-43)crates/amaru-plutus/src/script_context/v3.rs (18)
redeemers(681-704)withdrawals(331-346)to_plutus_data(37-46)to_plutus_data(50-59)to_plutus_data(63-85)to_plutus_data(89-98)to_plutus_data(102-111)to_plutus_data(115-117)to_plutus_data(121-123)to_plutus_data(127-139)to_plutus_data(142-184)to_plutus_data(188-195)to_plutus_data(201-274)to_plutus_data(278-294)to_plutus_data(299-308)to_plutus_data(322-361)to_plutus_data(365-367)to_plutus_data(371-373)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (15)
crates/amaru-ledger/Cargo.toml (1)
37-38: These deps are firing on all cylinders, mate.Brilliant – both
amaru-plutusanduplc-turboare getting their moment to shine in the phase two validation code. They're imported and actively doing the heavy lifting over incrates/amaru-ledger/src/rules/transaction/phase_two/mod.rs, handling the Plutus V2 and V3 script execution like seasoned stunt doubles on a blockbuster set. Workspace dependencies are wired up properly, and they're in the right place as regular deps – not sitting idle in dev-dependencies.Solid work on bringing these legends into the party. The Plutus V3 validation mission is good to go.
crates/amaru-kernel/src/lib.rs (3)
124-126: G'day, new module on the block looks ace!The
ordered_redeemermodule addition and re-export pattern matches the existing codebase style nicely. Clean and consistent, like a well-organised inventory in Stardew Valley.
182-184: Arena pool module addition looks bonzer!Following the same pattern as the other module exports. No dramas here.
820-842: Brilliant work on theHasOwnershipimplementation forCertificate, mate.You've nailed all 17 Conway-era certificate variants comprehensively. The logic is spot-on—stake-based certs return the credential, while pool ops are properly identified by their key hash since that's how the ledger actually tracks 'em. Smart move skipping the wildcard arm; the compiler's got your back if some fresh certificate type lands in the next hard fork. Ship it!
crates/amaru-plutus/src/script_context/v2.rs (2)
85-86: Nice refactor on the Withdrawals serialization, cobber.Collecting into a
BTreeMapbefore callingto_plutus_dataensures deterministic ordering. It's like sorting your Pokédex — consistency matters for reproducibility.
90-106: StakeAddress Plutus serialization looking proper!The double-constructor pattern (
Constr(0, [Constr(0/1, [hash])])) correctly handles the V1/V2 representation where stake credentials are wrapped in aPointer | Credentialenum. The doc comment explaining that Pointer is unreachable from ledger serialization is a nice touch — like leaving a note for future archaeologists.The delegation pattern on line 103-105 cleanly forwards to the kernel implementation, avoiding duplication.
crates/amaru-kernel/src/ordered_redeemer.rs (1)
38-62: Deref and From impls are tidy and ergonomic!The Cow-based design gives you flexibility to work with both borrowed and owned redeemers without unnecessary cloning. The three
Fromimpls cover all the common cases — it's like having the right adapter for every console generation.crates/amaru-plutus/src/script_context/v1.rs (3)
130-147: StakeAddress Plutus serialization for V1 mirrors V2 — consistency achieved!Same pattern as v2.rs with the double-constructor wrapping. The doc comment about Pointer being unreachable is duplicated here, which is fine since these are version-specific impls. Like having the same quest available in both New Game and New Game+.
158-223: Certificate handling with translation layer looks solid!The logic to translate
Reg/UnRegvariants to their legacyStakeRegistration/StakeDeregistrationforms for V1/V2 is spot-on. The doc comment on lines 165-168 clearly explains this translation, which is clutch for anyone maintaining this code later.Switching to
self.deref()and using thePallasCertificatetype alias is a nice touch — it makes the intent crystal clear, like a well-labelled map in an RPG.
311-335: Test updates look ripper!The changes to use
Redeemers::iter_from,PROTOCOL_VERSION_10, andredeemer.deref()align with the new API shapes introduced in this PR. The test still covers the core logic of verifying script context generation against expected fixtures.crates/amaru-plutus/src/script_context/mod.rs (5)
114-116: Handybudget()accessor for execution units!Clean and simple — provides read access to the redeemer's ex_units without exposing more than needed. Nice one!
364-372:to_script_contexts()provides a clean way to iterate script executions!This pairs each
ScriptContextwith its associatedScript, making phase-2 validation straightforward. Thefilter_maphandles the case whereScriptContext::newreturnsNonegracefully. Solid API design, like a well-balanced character build.
829-838:HasScriptHashimplementation forScriptis ace!Delegates to the appropriate
compute_hash()for each variant. This consolidates script hash computation in one place — DRY as a bone, mate.
970-984: Certificate wrapper struct with protocol versioning is clever!This allows version-aware serialization (for handling the Conway protocol 9 bug mentioned in the comment) while still providing transparent access via
Deref. It's like having a compatibility layer for old game saves.
1083-1136: Redeemers refactored to pair with ScriptPurpose — solid design!The new structure
BTreeMap<OrderedRedeemer<'a>, ScriptPurpose<'a>>makes the relationship between redeemers and their purposes explicit. TheToPlutusDataimpl correctly serialises this as a map of(purpose, redeemer)pairs.The
iter_frommethod handles both list and map variants ofPallasRedeemers— nice polymorphism, like a class that works with both controller and keyboard inputs.
yHSJ
force-pushed
the
josh/phase-2-validation
branch
from
caffed0 to
e97c084
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (1)
100-120: Typo still lurking: "resolved_reference_inptus" → "resolved_reference_inputs".Looks like this sneaky little typo escaped the last review pass, mate. Just a quick find-and-replace job.
🔎 Quick fix
- let mut resolved_reference_inptus = transaction_body + let mut resolved_reference_inputs = transaction_body .reference_inputs ... - resolved_inputs.append(&mut resolved_reference_inptus); + resolved_inputs.append(&mut resolved_reference_inputs);crates/amaru-plutus/src/script_context/mod.rs (3)
58-59: Redeemer field exposure—TODO says this shouldn't be public, but it is.The TODO comment on line 58 explicitly states "this should not be exposed, this is just for testing purposes atm," yet the field is marked
pub. This is like leaving the backdoor unlocked because you forgot your keys once—it works, but it's not a solid long-term strategy. Consider using a test-only accessor or a#[cfg(test)]method instead.Based on past review comments, this was already flagged. Has this been addressed in a later commit, or is it still pending?
🔎 Suggested fix to hide the field
pub struct ScriptContext<'a> { tx_info: &'a TxInfo<'a>, - // TODO: this should not be exposed, this is just for testing purposes atm - pub redeemer: &'a Redeemer, + redeemer: &'a Redeemer, datum: Option<&'a PlutusData>, script_purpose: &'a ScriptPurpose<'a>, } +#[cfg(test)] +impl<'a> ScriptContext<'a> { + pub fn redeemer(&self) -> &'a Redeemer { + self.redeemer + } +}
444-461: Key-based withdrawals should return None—this is a past review issue.G'day! The current code at lines 450-452 returns
Some(ScriptPurpose::Rewarding(StakeCredential::AddrKeyhash(*hash)))forStakePayload::Stake, which incorrectly treats key-based withdrawals as script-validated. Key-based credentials don't have scripts, so they can't have redeemers. Phase 1 checks whether the transaction is constructed correctly; returningSomehere creates a malformed script context.This was flagged in a past review. The fix is to return
NoneforStakePayload::Stake:🔎 Proposed fix to return None for key-based withdrawals
RedeemerTag::Reward => { withdrawals .0 .keys() .nth(index) .and_then(|stake| match stake.0.payload() { - StakePayload::Stake(hash) => Some(ScriptPurpose::Rewarding( - StakeCredential::AddrKeyhash(*hash), - )), + StakePayload::Stake(_) => None, // Key-based withdrawals don't have scripts StakePayload::Script(hash) => { let script = scripts.get(hash); script.map(|script| { script_table.insert(redeemer.clone(), script.clone()); ScriptPurpose::Rewarding(StakeCredential::ScriptHash(*hash)) }) } }) }Based on past review comments, this was already flagged as a critical issue. Please ensure this is addressed before merging.
790-805: Script::to_bytes() with unreachable for Native scripts—consider safer handling.The
unreachable!()on line 802 forScript::Nativeassumes that "a redeemer should never point to a native_script." While this is true for phase-2 validation (native scripts don't use redeemers), if this invariant is ever violated—due to a buggy caller, future changes, or malformed transactions—this will panic at runtime.This was flagged in a past review. Consider either:
- Returning a
Resultwith an explicit error forNativescripts, or- Annotating with
#[expect(clippy::unreachable)]and adding a comment documenting the invariant.🔎 Alternative: Return Result for Native scripts
impl Script<'_> { - pub fn to_bytes(&self) -> Result<Vec<u8>, minicbor::decode::Error> { + pub fn to_bytes(&self) -> Result<Vec<u8>, ScriptToBytesError> { fn decode_cbor_bytes(cbor: &[u8]) -> Result<Vec<u8>, minicbor::decode::Error> { minicbor::decode::Decoder::new(cbor) .bytes() .map(|b| b.to_vec()) } match self { - Script::PlutusV1(s) => decode_cbor_bytes(s.0.as_ref()), - Script::PlutusV2(s) => decode_cbor_bytes(s.0.as_ref()), - Script::PlutusV3(s) => decode_cbor_bytes(s.0.as_ref()), - Script::Native(_) => unreachable!("a redeemer should never point to a native_script"), + Script::PlutusV1(s) => decode_cbor_bytes(s.0.as_ref()).map_err(Into::into), + Script::PlutusV2(s) => decode_cbor_bytes(s.0.as_ref()).map_err(Into::into), + Script::PlutusV3(s) => decode_cbor_bytes(s.0.as_ref()).map_err(Into::into), + Script::Native(_) => Err(ScriptToBytesError::NativeScriptUnsupported), } } } +#[derive(Debug, Error)] +pub enum ScriptToBytesError { + #[error("Native scripts cannot be serialized to bytes for redeemer context")] + NativeScriptUnsupported, + #[error("CBOR decode error: {0}")] + CborDecode(#[from] minicbor::decode::Error), +}Alternatively, if you want to keep the panic:
Script::PlutusV3(s) => decode_cbor_bytes(s.0.as_ref()), - Script::Native(_) => unreachable!("a redeemer should never point to a native_script"), + #[expect(clippy::unreachable)] + Script::Native(_) => { + // Invariant: Native scripts are never referenced by redeemers in phase-2 validation. + // If this is hit, it indicates a programming error in the caller. + unreachable!("a redeemer should never point to a native_script") + }
🧹 Nitpick comments (4)
crates/amaru-ledger/src/rules/transaction/phase_one/scripts.rs (1)
111-112: G'day mate, this TODO might be a bit of a plot hole now!Since this PR's introducing phase_two to handle the actual script evaluation (via uplc_turbo and all that jazz), leaving this TODO here in phase_one could lead some poor dev on a bit of a wild goose chase. Like leaving a quest marker in the wrong zone, y'know?
Phase one's job is to validate scripts, datums, and redeemers are all accounted for – which it does brilliantly. The actual script execution's happening in phase_two now. Either clarify this comment to reference phase_two, or maybe just bin it altogether to avoid confusion.
🎬 Suggested options
Option 1: Remove the TODO (preferred if phase_two already handles this)
- // TODO: evaluate scripts - Ok(())Option 2: Clarify that evaluation happens in phase_two
- // TODO: evaluate scripts + // NOTE: Script evaluation happens in phase_two + Ok(())crates/amaru/src/stages/mod.rs (1)
119-120: Default arena count of 1 might limit parallelism, just a heads up.With
ledger_vm_alloc_arena_countdefaulting to 1, you're essentially serializing script evaluations that need an arena—like having only one controller for a co-op game. If parallel script execution is the goal (as the comment on line 97-98 suggests), operators might want to bump this up in production.Not a blocker by any means—conservative defaults are grand for safety. Just flagging it for documentation or future tuning considerations.
crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs (1)
152-180: The&(*network).into()pattern works, but reads a bit like a Kojima plot twist.This dereference-convert-reference dance does the job, but it's a mouthful to parse on first read. If
outputs::executecould accept&NetworkNamedirectly (or if there's aFrom<&NetworkName>impl for the target type), it might clean things up.Not a blocker at all—the code is correct and the conversion is clear once you grok it. Just mentioning it as a potential future polish.
crates/amaru-plutus/src/script_context/mod.rs (1)
207-360: TxInfo::new refactor with protocol_version and script table building—comprehensive but complex.Righto, this is a chunky refactor! The addition of
protocol_versionparameter, script collection from witness sets (lines 281-300), andscript_tableconstruction viaScriptPurpose::builder(lines 304-334) is well-structured. The logic correctly iterates through redeemers, builds purposes, and populates the script table. The error handling withTxInfoTranslationError::InvalidRedeemeris appropriate.However, line 207 has
#[allow(clippy::too_many_arguments)]—this function now takes 8 parameters. While the complexity might be justified, consider if some parameters could be grouped into a context struct for future maintainability.💡 Optional: Consider a context struct for TxInfo::new params
If the parameter list grows further, you might bundle some into a
TxInfoContextstruct to improve ergonomics:pub struct TxInfoContext<'a> { pub slot: &'a Slot, pub network: NetworkName, pub era_history: &'a EraHistory, pub protocol_version: ProtocolVersion, } impl<'a> TxInfo<'a> { pub fn new( tx: &'a MintedTransactionBody<'_>, witness_set: &'a MintedWitnessSet<'_>, tx_id: &Hash<32>, utxos: &'a Utxos, context: &TxInfoContext, ) -> Result<Self, TxInfoTranslationError> { // ... } }This would reduce the parameter count and make future additions easier.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.claude/settings.local.json(1 hunks).github/workflows/continuous-integration.yml(1 hunks)Cargo.toml(2 hunks)crates/amaru-kernel/Cargo.toml(1 hunks)crates/amaru-kernel/src/arena_pool.rs(1 hunks)crates/amaru-kernel/src/lib.rs(3 hunks)crates/amaru-kernel/src/ordered_redeemer.rs(1 hunks)crates/amaru-ledger/Cargo.toml(1 hunks)crates/amaru-ledger/src/block_validator.rs(3 hunks)crates/amaru-ledger/src/rules.rs(4 hunks)crates/amaru-ledger/src/rules/block.rs(5 hunks)crates/amaru-ledger/src/rules/transaction/mod.rs(1 hunks)crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs(7 hunks)crates/amaru-ledger/src/rules/transaction/phase_one/outputs.rs(1 hunks)crates/amaru-ledger/src/rules/transaction/phase_one/scripts.rs(1 hunks)crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs(1 hunks)crates/amaru-ledger/src/state.rs(3 hunks)crates/amaru-ledger/tests/evaluate_ledger_states.rs(3 hunks)crates/amaru-plutus/src/script_context/mod.rs(17 hunks)crates/amaru-plutus/src/script_context/v1.rs(9 hunks)crates/amaru-plutus/src/script_context/v2.rs(2 hunks)crates/amaru-plutus/src/script_context/v3.rs(8 hunks)crates/amaru-plutus/src/to_plutus_data.rs(2 hunks)crates/amaru/src/bin/amaru/cmd/run.rs(1 hunks)crates/amaru/src/bin/ledger/cmd/mod.rs(2 hunks)crates/amaru/src/stages/mod.rs(6 hunks)examples/shared/src/lib.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- Cargo.toml
- crates/amaru-ledger/Cargo.toml
- crates/amaru-kernel/Cargo.toml
- crates/amaru-ledger/src/rules.rs
- crates/amaru/src/bin/amaru/cmd/run.rs
- crates/amaru-ledger/src/rules/transaction/mod.rs
- examples/shared/src/lib.rs
- crates/amaru-kernel/src/lib.rs
- .github/workflows/continuous-integration.yml
- crates/amaru-ledger/tests/evaluate_ledger_states.rs
- crates/amaru-plutus/src/to_plutus_data.rs
- crates/amaru-ledger/src/rules/transaction/phase_one/outputs.rs
🧰 Additional context used
🧠 Learnings (18)
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 387
File: crates/amaru-stores/src/lib.rs:40-40
Timestamp: 2025-08-20T13:02:25.763Z
Learning: In the amaru-stores crate, amaru_slot_arithmetic types like Epoch and EraHistory are used throughout the main crate code in modules like in_memory/mod.rs, rocksdb/consensus.rs, and rocksdb/ledger/columns/, not just in tests. This means amaru-slot-arithmetic should be a regular dependency, not a dev-dependency.
Applied to files:
crates/amaru/src/bin/ledger/cmd/mod.rscrates/amaru-ledger/src/state.rscrates/amaru-ledger/src/block_validator.rscrates/amaru/src/stages/mod.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-05-21T18:58:48.631Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 228
File: crates/amaru-stores/src/rocksdb/consensus.rs:89-128
Timestamp: 2025-05-21T18:58:48.631Z
Learning: The InMemConsensusStore implementation in crates/amaru-stores/src/rocksdb/consensus.rs will be fleshed out incrementally on a by-need basis, driven by test requirements rather than implementing all functionality upfront.
Applied to files:
crates/amaru/src/bin/ledger/cmd/mod.rs
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru/src/bin/ledger/cmd/mod.rscrates/amaru-plutus/src/script_context/v3.rscrates/amaru-ledger/src/rules/transaction/phase_one/mod.rscrates/amaru-ledger/src/rules/block.rscrates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-kernel/src/arena_pool.rscrates/amaru-ledger/src/state.rscrates/amaru-ledger/src/rules/transaction/phase_one/scripts.rscrates/amaru-plutus/src/script_context/v2.rscrates/amaru-kernel/src/ordered_redeemer.rscrates/amaru-ledger/src/block_validator.rscrates/amaru/src/stages/mod.rscrates/amaru-plutus/src/script_context/v1.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-01-21T15:32:17.911Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 69
File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117
Timestamp: 2025-01-21T15:32:17.911Z
Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.
Applied to files:
crates/amaru-ledger/src/rules/block.rscrates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-02-03T11:15:22.640Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 75
File: crates/amaru/src/consensus/mod.rs:164-165
Timestamp: 2025-02-03T11:15:22.640Z
Learning: In the Amaru project, chain selection operations (roll_forward and rollback) should use separate result types to leverage the type system for preventing impossible states, rather than using runtime checks or panics.
Applied to files:
crates/amaru-ledger/src/rules/block.rscrates/amaru-ledger/src/state.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In crates/amaru-consensus, flood prevention for duplicate invalid headers should be implemented in the validate_header stage or database layer, not in the store_header stage, since store_header runs before validation and cannot determine header validity.
Applied to files:
crates/amaru-ledger/src/rules/block.rscrates/amaru-ledger/src/block_validator.rs
📚 Learning: 2025-09-29T20:08:29.906Z
Learnt from: yHSJ
Repo: pragma-org/amaru PR: 453
File: crates/amaru-metrics/src/ledger.rs:60-104
Timestamp: 2025-09-29T20:08:29.906Z
Learning: In amaru-ledger/src/state.rs, the txs_processed field represents the number of transactions in the current block (block.transaction_bodies.len() as u64), not a cumulative total. Therefore, using a Counter with .add() in the metrics is correct for accumulating per-block transaction counts.
Applied to files:
crates/amaru-ledger/src/rules/block.rs
📚 Learning: 2025-08-11T06:17:56.499Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 372
File: crates/amaru-consensus/src/consensus/headers_tree.rs:612-614
Timestamp: 2025-08-11T06:17:56.499Z
Learning: The `indextree::Arena<T>` struct in version 4.7.4 has a `count()` method that returns the total number of nodes in the arena, including removed nodes. This is a valid method to call on Arena instances in Rust code using the indextree crate.
Applied to files:
crates/amaru-kernel/src/arena_pool.rs
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, ValidateHeader::handle_roll_forward returns a Result<PullEvent, ConsensusError>, not ValidateHeaderEvent as might be expected from the older code structure.
Applied to files:
crates/amaru-ledger/src/state.rs
📚 Learning: 2025-08-08T14:46:53.013Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/pool_params.rs:107-116
Timestamp: 2025-08-08T14:46:53.013Z
Learning: In crates/amaru-kernel/src/pool_params.rs, when serializing Relay::SingleHostAddr IPv6 to text, the project intentionally reverses each 4-byte chunk before constructing std::net::Ipv6Addr. This matches cardano-ledger’s IPv6 representation (four little-endian Word32 chunks). Do not “simplify” by passing the raw 16 bytes directly to Ipv6Addr::from; that would break ledger compatibility.
Applied to files:
crates/amaru-ledger/src/state.rscrates/amaru/src/stages/mod.rscrates/amaru-plutus/src/script_context/v1.rs
📚 Learning: 2025-09-29T16:44:14.807Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/protocol.rs:94-106
Timestamp: 2025-09-29T16:44:14.807Z
Learning: In the amaru-network crate protocol.rs file, the correct Cardano mini-protocol ID assignments are: PROTO_N2N_KEEP_ALIVE = 8 and PROTO_N2N_PEER_SHARE = 10, as verified against the network specification by the maintainer.
Applied to files:
crates/amaru-ledger/src/state.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-04-04T16:49:53.462Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 0
File: :0-0
Timestamp: 2025-04-04T16:49:53.462Z
Learning: The ledger validation architecture abstracts state management from validations using traits like UtxoStateManagement, PoolsStateManagement, AccountsStateManagement, etc. to keep validation rules lean and independent of storage concerns.
Applied to files:
crates/amaru-ledger/src/block_validator.rs
📚 Learning: 2025-01-31T09:19:55.491Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 64
File: crates/amaru/src/ledger/state/transaction.rs:62-68
Timestamp: 2025-01-31T09:19:55.491Z
Learning: In the Cardano ledger, withdrawal uniqueness is guaranteed by the on-wire format's serialization, which considers duplicate withdrawals as invalid. This guarantee is reflected in Rust's type system, making additional runtime uniqueness checks unnecessary even when using vectors internally, as the data originates from minted blocks.
Applied to files:
crates/amaru-plutus/src/script_context/v1.rs
📚 Learning: 2025-08-08T14:34:06.105Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/minicbor-extra/src/lib.rs:50-55
Timestamp: 2025-08-08T14:34:06.105Z
Learning: Project uses Rust 1.88 stable; using Result::is_err_and is acceptable and should not be flagged as unstable. In particular, crates/minicbor-extra/src/lib.rs relies on is_err_and when checking end_of_input; future reviews should not suggest replacing it for stability reasons.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-08-08T14:39:50.527Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/borrowed_datum.rs:32-39
Timestamp: 2025-08-08T14:39:50.527Z
Learning: In the amaru project, when converting BorrowedDatumOption::Data to an owned DatumOption in crates/amaru-kernel/src/borrowed_datum.rs, the call `.unwrap()` refers to pallas’s KeepRaw::unwrap, which is infallible (always returns the inner value) and is not a panic risk. Future reviews should not flag this unwrap as dangerous.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-09-29T16:39:24.001Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/mux.rs:317-325
Timestamp: 2025-09-29T16:39:24.001Z
Learning: In crates/amaru-network/src/mux.rs, the outgoing() method intentionally uses unwrap() after get_mut(&proto_id) as a fail-fast mechanism. This panic is designed to catch programming errors where an actor tries to send on an unregistered protocol, and should not be changed to return a Result since it represents internal code bugs that should terminate the process, not external input that should be handled gracefully.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-09-06T09:16:25.025Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 435
File: crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs:113-116
Timestamp: 2025-09-06T09:16:25.025Z
Learning: In cardano-node serialized ledger state CBOR encoding, indefinite-length structures may be terminated with 0xFF "break" markers. The current code in convert_ledger_state.rs unconditionally strips the last byte (bytes[p..bytes.len() - 1]), which could corrupt data if the trailing byte is not actually a CBOR break marker.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-04-20T18:02:25.073Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward.rs:73-75
Timestamp: 2025-04-20T18:02:25.073Z
Learning: In the current development stage, rkuhn prefers using explicit panics (via `.expect()` or `.unwrap()`) for fatal errors in the application code that would tear down the node, rather than propagating errors with `Result`. The intention is to eventually transition to proper error handling with `Result` as the codebase matures.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
🧬 Code graph analysis (5)
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (2)
crates/amaru-plutus/src/script_context/mod.rs (21)
from(566-568)from(634-636)from(648-675)from(679-708)from(712-717)from(808-815)from(819-826)from(849-855)from(859-865)from(869-874)from(888-896)from(938-953)from(961-963)from(991-993)from(1072-1079)from(1143-1158)new(68-90)new(208-361)new(601-620)new(1086-1088)budget(114-116)crates/amaru-ledger/src/rules/transaction/phase_one/inputs.rs (1)
execute(46-150)
crates/amaru-kernel/src/arena_pool.rs (1)
crates/amaru-ledger/src/block_validator.rs (1)
new(41-54)
crates/amaru-plutus/src/script_context/v2.rs (2)
crates/amaru-plutus/src/script_context/v1.rs (13)
to_plutus_data(29-31)to_plutus_data(40-42)to_plutus_data(46-70)to_plutus_data(85-103)to_plutus_data(114-127)to_plutus_data(135-140)to_plutus_data(144-146)to_plutus_data(153-155)to_plutus_data(188-222)to_plutus_data(227-239)to_plutus_data(250-261)to_plutus_data(265-267)to_plutus_data(271-273)crates/amaru-plutus/src/script_context/v3.rs (3)
to_plutus_data(37-46)to_plutus_data(50-59)to_plutus_data(63-85)
crates/amaru-ledger/src/block_validator.rs (3)
crates/amaru-kernel/src/arena_pool.rs (1)
new(43-55)crates/amaru-ouroboros-traits/src/can_validate_blocks/mod.rs (2)
new(36-38)new(103-105)crates/amaru-ledger/src/rules/block.rs (1)
anyhow(120-125)
crates/amaru-plutus/src/script_context/mod.rs (2)
crates/amaru-kernel/src/lib.rs (16)
from(286-288)from(309-311)from(347-349)from(515-520)from(524-531)from(583-591)redeemers(982-982)redeemers(1000-1019)script_hash(857-857)script_hash(861-868)script_hash(872-874)script_hash(878-880)script_hash(888-890)deref(280-282)deref(297-299)deref(355-357)crates/amaru-kernel/src/ordered_redeemer.rs (4)
from(47-49)from(53-55)from(59-61)deref(41-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Build riscv32
- GitHub Check: clippy
- GitHub Check: Build aarch64/macos
- GitHub Check: Build x86_64/linux
- GitHub Check: Build x86_64/windows
- GitHub Check: Build aarch64/linux
- GitHub Check: Build wasm32
- GitHub Check: Test the CLI
- GitHub Check: Benches
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (54)
.claude/settings.local.json (1)
1-10: Ditch the.localfile from version control—Claude's tooling wants it local-only, mate.So here's the thing: when Claude Code creates a
.claude/settings.local.jsonfile, it automatically tosses it into.gitignoreby design. Committing this file breaks that protection and can cause headaches down the track—each dev's got their own machine-specific Claude setup, and pushing yours into the shared repo is like forcing your video game controller config onto your mates. Not the vibe.If you need team-wide Claude permissions (like allowing certain git commands), use
.claude/settings.jsoninstead—that's the shared config that's meant to be committed. The.localsuffix is basically Claude's way of saying "this stays on your machine only, yeah?"Two-minute fix: remove
.claude/settings.local.jsonfrom the commit and let it live in.gitignorewhere it belongs.crates/amaru-kernel/src/arena_pool.rs (5)
22-36: Nice one, mate! Clean pool design.The
Arc<Inner>wrapping withMutex+Condvarcombo is the classic thread-safe pool pattern — like picking up a well-known spell from your inventory instead of crafting something dodgy. Makes cloning cheap and sharing across threads straightforward.
43-55: Pre-allocation approach looks ripper!Creating all arenas upfront at startup is the way to go — fail fast if memory's tight rather than getting blindsided later during script evaluation. It's like loading your save before a boss fight instead of finding out mid-battle you're under-leveled.
60-79: Solid blocking acquire with proper spurious wakeup handling.The loop structure correctly handles spurious wakeups from the
Condvar— like double-checking your quest log after a cutscene to make sure the objective actually updated. The poison recovery viainto_inneris a pragmatic choice for keeping things moving.
84-91: Non-blocking variant is clean and to the point.Short, sweet, and does exactly what it says on the tin. No drama here.
108-118: RAII guard with proper cleanup — top shelf stuff.The
ManuallyDrop::takeis the right call here (pun intended), and the safety comment at line 110 makes the invariant crystal clear. Resetting the arena before returning it to the pool ensures memory gets recycled properly — no leaky abstractions sneaking through like a stealth section in a Hitman game.crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (4)
39-57: Error enum is comprehensive and well-structured.Like a proper skill tree covering all the failure modes — missing inputs, translation issues, deserialization problems, cost model gaps, and UPLC machine errors. The
thiserrorderive keeps the boilerplate down. Legend.
59-65: Pragmatic error struct — avoiding lifetime shenanigans.Using
Stringinstead of the originalMachineErrorto dodge lifetime complexity is a fair trade-off. You can always revisit this when you've got more bandwidth, but for now it gets the job done without turning your code into a Souls-like difficulty spike.
67-84: Function signature is hefty but understandable for validation context.Nine parameters is a lot, but validation functions are like RPG boss encounters — they need all their stats, equipment, and buffs loaded in. The early exit when there's no redeemer is a smart short-circuit. No point spinning up the whole Plutus machinery if there's nothing to evaluate.
235-243: Validity state inversion logic is spot-on.The
is_valid = falsecase flipping success/failure is exactly how Cardano handles "expected to fail" transactions. If the transaction claims it's invalid but scripts pass anyway, that's sus and rightly flagged asValidityStateError. Nice attention to protocol semantics.crates/amaru/src/bin/ledger/cmd/mod.rs (2)
32-41: Clean wiring of ArenaPool with sensible defaults.Renaming to
rocks_db_configavoids confusion now that there's aConfigfor the arena settings too. Using the default config for arena sizing keeps things consistent with other initialization paths. Like having your game settings sync across platforms — just works.
35-46: BlockValidator construction properly updated.All the pieces slot together like a well-designed crafting system. ArenaPool gets passed through, store and snapshots are in place. Good stuff.
crates/amaru-ledger/src/block_validator.rs (3)
36-38: Field naming gives good context.
vm_eval_pooltells you exactly what it's for — like a well-labeled inventory slot. Past feedback about naming has been addressed. Making it public gives flexibility for testing or external access if needed.
40-54: Constructor update is clean and follows the existing pattern.Nothing fancy, just threading the ArenaPool through. Works a treat.
74-82: ArenaPool correctly passed to state.roll_forward.The pool reference flows through the validation path as expected. Smooth handoff.
crates/amaru-ledger/src/state.rs (2)
35-42: Import updates look good.ArenaPool joins the party, ready for action. Import housekeeping is on point.
601-625: ArenaPool threading is pragmatic, even if a bit drill-heavy.I hear ya on the concern about drilling
arena_poolthrough multiple layers — it's like passing the same quest item through every NPC conversation. But honestly, for a resource pool that's specifically needed at the script evaluation layer, explicit passing keeps ownership clear and avoids hidden state.If it gets unwieldy later, you could consider bundling validation-time resources into a context struct, but for now this is perfectly serviceable.
crates/amaru-plutus/src/script_context/v1.rs (5)
15-26: Imports are clean and well-organized.New types for
StakeAddress,Withdrawals, and thePallasCertificatealias are all accounted for. UsingDereffor certificate matching is a nice touch.
130-147: StakeAddress serialization handles the V1/V2 quirks properly.The nested
Constr(0, [Constr(0/1, [hash])])pattern correctly represents the "Pointer vs Credential" enum that PlutusV1/V2 expects, even though Pointers can't actually be constructed. Good defensive documentation explaining the invariant. The wrapper delegation keeps things DRY.
158-223: Certificate serialization handles V1/V2 compatibility like a champ.Translating the newer
Reg/UnRegcertificate variants back to the oldStakeRegistration/StakeDeregistrationrepresentation is the right call for backwards compatibility — like having a remaster that still supports legacy save files.The doc comments clearly spell out which variants are supported and which will error. The
Constr(0, [cred])hardcoding for StakeCredential is well-justified by the serialization constraints.
264-268: Withdrawals serialization is streamlined.Simple and effective — collect the iterator into a vec and serialize. No fuss, no muss.
277-355: Tests updated to reflect API changes.The test harness now uses
PROTOCOL_VERSION_10, properRedeemers::iter_fromwith deref, and the updatedTxInfo::newsignature. Good hygiene keeping tests in sync with implementation changes — like updating your mod load order after a major patch.crates/amaru-kernel/src/ordered_redeemer.rs (1)
18-62: LGTM! Solid wrapper type for ordered redeemers, mate.This
OrderedRedeemerwrapper is clean as a whistle. The ordering semantics (tag index first, then redeemer index) properly match what Haskell expects—like getting the combo right in a fighting game. TheDerefandFromimpls make it ergonomic to use without any friction.Nice use of
Cowto support both borrowed and owned variants without duplicating code. This is giving me "Portal 2 companion cube" vibes—simple, elegant, and does exactly what it needs to do.crates/amaru/src/stages/mod.rs (2)
96-104: Nice documentation on the config fields, cobber!Love that you've documented what these arena settings actually do—like getting proper tooltips in a game menu. The comments about memory footprint and re-allocation trade-offs are helpful for operators tuning their nodes.
341-372: ArenaPool integration looks proper, mate.The
ArenaPoolis created once and threaded through to bothInMemandRocksDbbackedBlockValidatorinstances consistently. Usingconfig.networkdirectly instead of a separate parameter keeps things DRY—like Geralt's approach to combat, efficient and no wasted moves.crates/amaru-plutus/src/script_context/v2.rs (2)
83-88: Withdrawals serialization now materializes the map—looks intentional.Creating a
BTreeMapfrom the iterator before passing toToPlutusDataensures consistent ordering. Just like sorting your inventory in an RPG before saving, this guarantees deterministic serialization.If the inner type is already a
BTreeMap, this is a bit redundant (creating a map of references), but it's safe and correct. If it's something else (like aKeyValuePairs), this conversion ensures proper BTreeMap ordering for Plutus data encoding.
90-107: StakeAddress encoding matches the cross-version pattern—bonzer work!The PlutusV2 encoding here aligns nicely with both V1 and V3 implementations (as shown in the relevant snippets). The outer
constr_v2!(0, ...)represents the "Credential" variant, while the inner constructor distinguishes between key (0) and script (1) hashes—like choosing your character class in an RPG.The comment about the
Pointervariant being unconstructable by ledger serialization is a nice touch. It's the kind of tribal knowledge that prevents future head-scratching sessions.crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs (2)
58-86: Renaming toPhaseOneErroris a grand call.With the introduction of phase_one/phase_two validation split, having
PhaseOneErrorinstead of the genericInvalidTransactionmakes the error hierarchy much clearer—like getting proper subtitles in a foreign film instead of auto-generated ones.
219-229: Returning consumed inputs instead of consuming in-place—smart refactor.This change separates "what to consume" from "when to consume," giving the caller control over the timing. Like decoupling save points from checkpoints in a game—more flexibility for the orchestrator.
The logic is correct: valid transactions consume their inputs, invalid ones consume collateral (if any).
crates/amaru-ledger/src/rules/block.rs (4)
39-45:TransactionInvalidwrapper is a clean way to unify phase errors.Using
thiserrorwith#[from]makes the conversion automatic—like auto-equipping the best gear in an RPG. Both phase errors get wrapped with clear, distinct messages.
173-184: Execute signature updated to thread ArenaPool and NetworkName—looking proper.The new parameters flow naturally through to both validation phases. The
ArenaPoolenables phase_two script evaluation, andNetworkNamealigns with the phase_one API changes. Signature matches the downstream expectations nicely.
216-217: Single source of truth foris_valid—past feedback addressed!Computing
is_validonce and passing it to both phases keeps everything in sync. Like the Konami Code—you only need to enter it once. Good fix from the previous review.
250-292: Phase execution order is correct—validate then mutate.The orchestration here is sound: phase_one validates and returns consumed inputs, phase_two runs script evaluation, and inputs are only consumed after both phases complete. Like a two-stage gauntlet where you can't advance till both trials are passed.
One note on the
is_validflag: it's not something you can skip phase_two for. In Cardano's design,is_validis an assertion about what the scripts should do. Phase_two always runs and validates that assertion—ifis_valid=false, scripts must actually fail for phase_two to succeed. It's a clever inversion: the transaction author is saying "I expect my scripts to fail," and phase_two confirms they do. So the current flow, which always executes both phases in order, is exactly right.crates/amaru-plutus/src/script_context/v3.rs (7)
18-28: LGTM on the import updates, mate!The new imports for
Certificate,CostModels,StakeAddress, andStakePayloadare spot on and align perfectly with the protocol-version-aware certificate handling and expanded PlutusV3 data model. This is solid wiring for the phase 2 validation work.
202-275: Protocol-version-aware certificate serialization looks ace.Righto, the switch from direct
Certificateusage toself.certificate(aPallasCertificate) with conditional deposit emission based onself.protocol_version.0 > 9is exactly the ticket for handling the Conway protocol 9 quirk. The pattern-matching covers all the certificate variants, and the deposit logic is consistent acrossReg/UnRegvariants. This is a ripper implementation!
621-628: StakeAddress ToPlutusData impl for amaru_kernel::StakeAddress is clean.The pattern-matching on
StakePayload::StakeandStakePayload::Scriptwith constructor indices 0 and 1 respectively is straightforward and correct. This aligns with Plutus script context expectations for stake credentials.
630-634: StakeAddress wrapper's ToPlutusData delegation is spot on.Delegating to the underlying
amaru_kernel::StakeAddressimplementation keeps things DRY and consistent. No dramas here!
657-692: Test case for protocol version 10 certificates—looks solid.The test case is properly wired to use
PROTOCOL_VERSION_10and the newRedeemers::iter_fromAPI. TheScriptContext::newcall usesredeemer.deref()correctly, and the test validates the produced context against expected vectors. Brilliant work!
520-532: Cost models serialization format needs validation against the Haskell ledger implementation.The TODO comment flags that this implementation is a "best guess". Given how cost models are handled in the Haskell ledger—with version-specific parsing, lenient deserialization from
Map Word8 [Int64], and recent fixes for PlutusV3 parameter count—verifying this serialization against the actual implementation is critical before merging. Misaligned serialization here could subtly break script execution budget calculations or consensus.Before merging, confirm the format against the Cardano ledger Haskell source. If correct, remove the TODO; otherwise, adjust the implementation.
449-454: CostModels serialization at index 18 needs validation against Cardano ledger specs.The code serializes
cost_models_for_script_languagesat index 18 following the sequential pattern (ada_per_utxo_byte at 17 → cost_models at 18 → execution_costs at 19). However, the implementation itself contains TWO unresolved TODOs:
- Line 449: "this is from Aiken, need to implement this"
- Line 520-521: "this has to be validated against the Haskell logic, this is a 'best guess'"
The CostModels PlutusData encoding is explicitly flagged as a best guess pending validation. Cross-check the Cardano ledger's Haskell implementation and CDDL specifications to confirm both the index positioning and the CostModels structure (currently encoding as a constructor wrapping three optional fields for V1, V2, V3).
crates/amaru-plutus/src/script_context/mod.rs (14)
15-26: Import changes are spot on for the phase 2 wiring.The addition of
OrderedRedeemer,ProtocolVersion,PallasRedeemers,StakePayload, and other types properly supports the script table and redeemer wiring refactor. All the pieces are in place for protocol-version-aware script evaluation.
68-90: ScriptContext::new updated to use OrderedRedeemer—logic looks sound.The transition from a plain
Redeemerlookup toOrderedRedeemer::from(redeemer)correctly leverages the new ordered redeemer abstraction. The datum resolution forSpendredeemers is handled properly, checkingtx_info.data.0.get(hash)for hash datums and inline datums. Nice one!
114-117: Added budget() accessor for ExUnits—handy for resource budgeting.This public accessor exposes the redeemer's
ex_units, which is useful for downstream code that needs to check execution budgets. Clean and straightforward implementation.
158-169: TxInfo struct updated with Certificate<'a> and script_table—good architectural choice.The addition of
certificates: Vec<Certificate<'a>>andscript_table: BTreeMap<OrderedRedeemer<'a>, Script<'a>>enables protocol-version-aware certificate handling and efficient script lookups. This is solid groundwork for phase 2 validation.
363-372: to_script_contexts() method—clean iterator-based construction.The method efficiently constructs all
ScriptContextinstances by iterating overscript_tableand callingScriptContext::new(self, redeemer). Thefilter_mapcorrectly handles cases whereScriptContext::newreturnsNone(e.g., for malformed redeemers). This is a ripper implementation for phase 2 validation orchestration!
374-394: translate_inputs updated to populate scripts map—looks good.The addition of the
scriptsparameter and the logic to insert reference scripts into the map (lines 387-389) correctly wires up scripts from UTxO outputs. The error handling remains solid.
426-436: Spend redeemer handling—correctly filters for script-locked outputs.The logic at lines 427-435 correctly checks if the output is locked by a script (
StakeCredential::ScriptHash(hash)) and returnsNonefor key-locked outputs. This ensures only script-validated spends get redeemers. Solid work!
462-472: Certificate redeemer handling—correctly filters for script-based certificates.Similar to the Spend case, the logic correctly checks
certificate.credential()for aScriptHashand returnsNonefor key-based certificates. This is the right approach.
473-483: Vote redeemer handling—correctly filters for script-based voters.The pattern continues: checking
voter.credential()forScriptHashand returningNoneotherwise. Consistent and correct.
484-513: Propose redeemer handling—correctly filters for guardrail scripts.The logic extracts the guardrail script hash from
ParameterChangeandTreasuryWithdrawalsgovernance actions and correctly returnsNonefor other action types that don't have guardrail scripts. This is a bit of a labyrinth with all the match arms, but it's correct!
829-838: HasScriptHash implementation for Script—clean and correct.The delegation to the underlying
compute_hash()methods forNativeScriptandPlutusScriptvariants is straightforward and correct. No dramas here!
970-984: Certificate<'a> wrapper with protocol_version—solid design choice.The
Certificate<'a>struct wraps a&'a PallasCertificatealong with aprotocol_versionfield, enabling protocol-version-aware serialization (as seen in v3.rs). TheDerefimplementation provides transparent access to the underlying certificate. This is a ripper abstraction for handling the Conway protocol 9 bug!
1083-1119: Redeemers<'a> struct with new APIs—well-designed abstraction.The
Redeemers<'a>struct wraps aBTreeMap<OrderedRedeemer<'a>, ScriptPurpose<'a>>and provides:
- A
new()constructor- A
Derefimpl for transparent map access- A
ToPlutusDataimpl that serializes as a map keyed byScriptPurposeThis is a clean API design that encapsulates the redeemer-to-purpose mapping. The
ToPlutusDataimplementation correctly pairsScriptPurposekeys withRedeemervalues.
1120-1136: Redeemers::iter_from for PallasRedeemers—handles both List and Map variants correctly.The
iter_frommethod correctly handles bothPallasRedeemers::ListandPallasRedeemers::Mapvariants, converting them toOrderedRedeemerinstances. For theMapvariant, it reconstructsRedeemerinstances from the key-value pairs. The use ofBox<dyn Iterator>to abstract over the two iterator types is a good choice for ergonomics.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/amaru-plutus/src/to_plutus_data.rs (1)
159-162: G'day! This double conversion is doing a bit of unnecessary work, mate.Right, so you're calling
to_plutus_data()twice here—once on theStakeAddress(which gives you aResult<PlutusData, PlutusDataError>), then using?to unwrap it, then calling.to_plutus_data()again on thePlutusDataitself. That second call (per the impl on lines 413-420) just clones the data and wraps it back inOk. It's like taking the ring to Mordor and then asking Frodo to walk it back to the Shire and return it to Mordor again—works, but why?Based on your previous comment about struggling with the compiler, I reckon you might've been fighting the type checker and landed on this solution. The trick is: the first call already returns
Result<PlutusData, PlutusDataError>, which is exactly what this match arm needs to return!🔎 Streamlined version without the double conversion
Address::Stake(stake_address) => { - <amaru_kernel::StakeAddress as ToPlutusData<V>>::to_plutus_data(stake_address)? - .to_plutus_data() + <amaru_kernel::StakeAddress as ToPlutusData<V>>::to_plutus_data(stake_address) }No
?, no second call—just delegate directly since it already returns the right type.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/amaru-plutus/src/to_plutus_data.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-14T16:36:04.502Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 263
File: crates/pure-stage/src/simulation/state.rs:33-36
Timestamp: 2025-06-14T16:36:04.502Z
Learning: In simulation and replay systems that require cloneable and serializable states, error types must often be converted to String rather than stored as trait objects (like Box<dyn Error> or anyhow::Error) because trait objects cannot be cloned, which breaks the snapshotting and replay functionality needed for deterministic simulation.
Applied to files:
crates/amaru-plutus/src/to_plutus_data.rs
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru-plutus/src/to_plutus_data.rs
🧬 Code graph analysis (1)
crates/amaru-plutus/src/to_plutus_data.rs (2)
crates/amaru-plutus/src/script_context/mod.rs (1)
to_plutus_data(1105-1117)crates/amaru-plutus/src/script_context/v3.rs (15)
to_plutus_data(37-46)to_plutus_data(50-59)to_plutus_data(63-85)to_plutus_data(89-98)to_plutus_data(102-111)to_plutus_data(115-117)to_plutus_data(121-123)to_plutus_data(127-139)to_plutus_data(142-184)to_plutus_data(188-195)to_plutus_data(201-274)to_plutus_data(278-294)to_plutus_data(299-308)to_plutus_data(322-361)to_plutus_data(365-367)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Build wasm32
- GitHub Check: Build aarch64/macos
- GitHub Check: Build x86_64/linux
- GitHub Check: Build x86_64/windows
- GitHub Check: Build riscv32
- GitHub Check: Build aarch64/linux
- GitHub Check: Benches
- GitHub Check: Test the CLI
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
crates/amaru-plutus/src/to_plutus_data.rs (4)
25-25: Solid choice bringing in thiserror, mate!Clean error handling setup. This'll make the error types much more ergonomic.
27-44: Nice error design, legend!The
PlutusDataErrorenum covers the bases well—separating version-specific issues from general errors. The helper constructor on line 38 is a nice touch for keeping call sites clean.
46-66: Top-notch versioned trait design, cobber!Love the const generic approach for version selection—it's like having a type-safe choose-your-own-adventure for Plutus versions. The
IsKnownPlutusVersionconstraint is proper defensive coding, making invalid versions unrepresentable. Chef's kiss! 👨🍳
118-118: All good, legend – trait bound is fully wired for all versions.The implementations are solid across the board: V1 (line 130 in v1.rs), V2 (line 90 in v2.rs), and V3 (line 621 in v3.rs) all have
ToPlutusDataproperly implemented foramaru_kernel::StakeAddress. When the Address impl at line 118 calls it at line 160, it'll find exactly what it needs for whichever version V resolves to. No circular dependencies, no missing pieces – she's a beaut.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/amaru-plutus/src/script_context/mod.rs (1)
304-334: The redeemer error could use a bit more detail, cobber.Lines 312-323: When
ScriptPurpose::builderreturnsNone, you're throwingInvalidRedeemer(ix)whereixis the enumerate index. That's helpful, but it doesn't tell us which redeemer tag/index combo actually failed—just the position in the list/map iteration.Consider including
redeemer.tagandredeemer.indexin the error for easier debugging. Something like:.ok_or_else(|| TxInfoTranslationError::InvalidRedeemer { position: ix, tag: redeemer.tag, index: redeemer.index, })?You'd need to tweak the error enum to hold that extra context, but it'll save you (and future devs) a heap of time when a redeemer goes walkabout.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/amaru-plutus/src/script_context/mod.rs(17 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-08-08T14:34:06.105Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/minicbor-extra/src/lib.rs:50-55
Timestamp: 2025-08-08T14:34:06.105Z
Learning: Project uses Rust 1.88 stable; using Result::is_err_and is acceptable and should not be flagged as unstable. In particular, crates/minicbor-extra/src/lib.rs relies on is_err_and when checking end_of_input; future reviews should not suggest replacing it for stability reasons.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-08-08T14:39:50.527Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/borrowed_datum.rs:32-39
Timestamp: 2025-08-08T14:39:50.527Z
Learning: In the amaru project, when converting BorrowedDatumOption::Data to an owned DatumOption in crates/amaru-kernel/src/borrowed_datum.rs, the call `.unwrap()` refers to pallas’s KeepRaw::unwrap, which is infallible (always returns the inner value) and is not a panic risk. Future reviews should not flag this unwrap as dangerous.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-09-29T16:39:24.001Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/mux.rs:317-325
Timestamp: 2025-09-29T16:39:24.001Z
Learning: In crates/amaru-network/src/mux.rs, the outgoing() method intentionally uses unwrap() after get_mut(&proto_id) as a fail-fast mechanism. This panic is designed to catch programming errors where an actor tries to send on an unregistered protocol, and should not be changed to return a Result since it represents internal code bugs that should terminate the process, not external input that should be handled gracefully.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-09-06T09:16:25.025Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 435
File: crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs:113-116
Timestamp: 2025-09-06T09:16:25.025Z
Learning: In cardano-node serialized ledger state CBOR encoding, indefinite-length structures may be terminated with 0xFF "break" markers. The current code in convert_ledger_state.rs unconditionally strips the last byte (bytes[p..bytes.len() - 1]), which could corrupt data if the trailing byte is not actually a CBOR break marker.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-04-20T18:02:25.073Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward.rs:73-75
Timestamp: 2025-04-20T18:02:25.073Z
Learning: In the current development stage, rkuhn prefers using explicit panics (via `.expect()` or `.unwrap()`) for fatal errors in the application code that would tear down the node, rather than propagating errors with `Result`. The intention is to eventually transition to proper error handling with `Result` as the codebase matures.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-01-21T15:32:17.911Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 69
File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117
Timestamp: 2025-01-21T15:32:17.911Z
Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 387
File: crates/amaru-stores/src/lib.rs:40-40
Timestamp: 2025-08-20T13:02:25.763Z
Learning: In the amaru-stores crate, amaru_slot_arithmetic types like Epoch and EraHistory are used throughout the main crate code in modules like in_memory/mod.rs, rocksdb/consensus.rs, and rocksdb/ledger/columns/, not just in tests. This means amaru-slot-arithmetic should be a regular dependency, not a dev-dependency.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-09-29T16:44:14.807Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/protocol.rs:94-106
Timestamp: 2025-09-29T16:44:14.807Z
Learning: In the amaru-network crate protocol.rs file, the correct Cardano mini-protocol ID assignments are: PROTO_N2N_KEEP_ALIVE = 8 and PROTO_N2N_PEER_SHARE = 10, as verified against the network specification by the maintainer.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
🧬 Code graph analysis (1)
crates/amaru-plutus/src/script_context/mod.rs (4)
crates/amaru-kernel/src/lib.rs (8)
from(286-288)from(309-311)from(347-349)from(515-520)from(524-531)from(583-591)datum(693-693)datum(697-708)crates/amaru-kernel/src/ordered_redeemer.rs (3)
from(47-49)from(53-55)from(59-61)crates/amaru-plutus/src/script_context/v1.rs (1)
inputs(47-51)crates/amaru-plutus/src/script_context/v3.rs (16)
to_plutus_data(37-46)to_plutus_data(50-59)to_plutus_data(63-85)to_plutus_data(89-98)to_plutus_data(102-111)to_plutus_data(115-117)to_plutus_data(121-123)to_plutus_data(127-139)to_plutus_data(142-184)to_plutus_data(188-195)to_plutus_data(201-274)to_plutus_data(278-294)to_plutus_data(299-308)to_plutus_data(322-361)to_plutus_data(365-367)to_plutus_data(371-373)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Build riscv32
- GitHub Check: Test coverage
- GitHub Check: Build aarch64/linux
- GitHub Check: Build wasm32
- GitHub Check: Build aarch64/macos
- GitHub Check: Build x86_64/linux
- GitHub Check: Build x86_64/windows
- GitHub Check: Test the CLI
- GitHub Check: Benches
- GitHub Check: clippy
- GitHub Check: Analyze (rust)
🔇 Additional comments (10)
crates/amaru-plutus/src/script_context/mod.rs (10)
114-116: Nice little accessor for the execution budget, cobber!This
budget()method is a tidy way to grab theex_unitsfor phase-2 validation. Straightforward and does what it says on the tin.
158-158: Solid refactoring to wire up script evaluation, mate.The changes to
certificates(line 158),redeemers(line 162), and the newscript_table(line 169) are spot-on for phase-2 validation. You're keeping everything private and using well-typed wrappers (Certificate<'a>,Redeemers<'a>) to manage protocol versioning and script lookups. This is clean architecture—like building with LEGO instead of duct tape.Also applies to: 162-162, 169-169
207-360: TxInfo construction is a beast, but it's well-structured for phase-2, mate.This function's grown to 8 parameters and does a lot of heavy lifting (collecting scripts, building redeemers, populating
script_table), but it's all necessary for phase-2 validation. The#[allow(clippy::too_many_arguments)]is warranted here—splitting this further would likely make it harder to follow.The script collection (lines 281-300) and redeemer wiring (lines 304-334) are clear and methodical. The
script_tableside-effect in the builder is a bit subtle, but it's documented by the parameter name and works well for this use case.
363-372: Perfect entry point for phase-2 script evaluation, mate!This
to_script_contexts()method is exactly what you need to kick off script execution. It pairs eachScriptContextwith itsScript, and thefilter_mapgracefully handles any redeemers that don't have a valid context (though by this point they should all be valid ifTxInfo::newsucceeded). Clean API for the validator—like handing someone a loaded weapon instead of a box of parts.
374-394: Collecting script refs from UTxOs—nice one, cobber.Lines 387-389: You're grabbing script references from UTxOs and adding them to the
scriptsmap. This is spot-on for phase-2, since scripts can be referenced on-chain rather than included in the witness set. The&mutparameter makes the side effect explicit, so no surprises here.
412-508: Builder refactor is top-notch—like a factory line for script purposes, mate.This refactor systematically handles each redeemer tag by:
- Checking if the credential/type is script-based (filtering out key-based)
- Looking up the script in the
scriptsmap- Inserting it into
script_table- Returning the appropriate
ScriptPurposeorNoneA few observations that are all working as intended:
- Key-based credentials return
None: Lines 433-435 (Spend), 451-453 (Reward), 462-464 (Cert), 473-475 (Vote)—this is correct, as key-based actions don't have scripts to execute. The past review comment about reward withdrawals has been addressed properly.- Missing scripts return
None: If a script hash is referenced but not found in thescriptsmap (e.g., not in witness_set or UTxOs), you'll getNone, which triggersInvalidRedeemer. That's the right failure mode for a missing script.- Side-effect population: The
script_table.insert()calls are a bit subtle, but the parameter names make it clear this is intentional.This is solid work—like assembling a puzzle where all the pieces actually fit.
822-831: HasScriptHash impl is clean as a whistle, mate.This trait implementation just delegates to
compute_hash()for each script variant. Straightforward and does exactly what's needed for script lookups. No dramas here.
963-977: Certificate wrapper is a clever workaround for the Conway protocol bug, mate.Line 966: The comment mentions a bug in Conway protocol 9 that requires version-aware serialization. This
Certificate<'a>wrapper that holdsprotocol_versionis a clean solution—you can swap out serialization logic based on the version without polluting the call sites. TheDerefimplementation (lines 971-977) keeps it transparent to use.This is a textbook example of the Newtype pattern for adding behavior to an external type. Nice work!
1076-1129: Redeemers refactor is a ripper—proper type instead of a loose alias, mate!You've gone from a bare type alias to a proper wrapper (lines 1076-1090) with a
Dereffor transparent access. TheToPlutusDataimplementation (lines 1092-1111) systematically converts the map toPlutusData::Map, and theiter_fromhelper (lines 1113-1129) gracefully handles bothPallasRedeemers::Listand::Mapformats.This is a solid improvement in type safety and encapsulation—like upgrading from a cardboard box to a proper toolbox. The explicit trait calls at lines 1103-1104 make it clear which version of
ToPlutusDatayou're invoking, which is helpful for readers.
68-82: Index bounds validation is already handled upstream—this concern is resolved.The redeemer index validation happens during
TxInfo::new()construction (lines 312-323), whereScriptPurpose::builder()validates each redeemer's index against its corresponding collection (inputs, mint, certs, etc.). If the index is out of bounds,builder()returnsNone, which causesTxInfo::new()to fail withInvalidRedeemer(ix). By the timeScriptContext::new()is called at line 74, the redeemer has already survived that validation check, so the.get(redeemer.index as usize)call is safe—no further bounds checking needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/amaru-plutus/src/script_context/v3.rs (2)
297-309: Theunwrap()on line 304 is a bit spicy, but I see the play.The allow attribute acknowledges this, and the GovAction doc comment explains that invalid reward addresses would fail phase-one validation first. Still, if something slips through phase-one, this'll panic rather than returning an error.
Just flagging it since we're in Phase 2 validation territory—you might want a fallback error path here for extra defensive coding, but it's not a blocker if phase-one is rock solid.
🔎 Optional defensive refactor
impl ToPlutusData<3> for Proposal { fn to_plutus_data(&self) -> Result<PlutusData, PlutusDataError> { + let reward_address = Address::from_bytes(&self.reward_account) + .map_err(|_| PlutusDataError::Custom("invalid reward address in proposal".into()))?; constr_v3!( 0, [ self.deposit, - Address::from_bytes(&self.reward_account).unwrap(), + reward_address, self.gov_action ] ) } }
520-532: The TODO here is giving me "I have a bad feeling about this" vibes.This implementation is marked as a "best guess" needing validation against the Haskell logic. For Phase 2 validation with Plutus V3 scripts, an incorrect
CostModelsencoding could cause script context mismatches—leading to valid scripts failing or, worse, invalid ones passing.Would be ace to get this cross-referenced with the Haskell ledger spec or Aiken's implementation before this ships to mainnet. Want me to help track this down or open an issue to ensure it doesn't slip through the cracks?
Cardano CostModels PlutusData encoding specification
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/amaru-plutus/src/script_context/v3.rs(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-01-21T15:32:17.911Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 69
File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117
Timestamp: 2025-01-21T15:32:17.911Z
Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.
Applied to files:
crates/amaru-plutus/src/script_context/v3.rs
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru-plutus/src/script_context/v3.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unused-dependencies
- GitHub Check: Build x86_64/windows
- GitHub Check: clippy
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
crates/amaru-plutus/src/script_context/v3.rs (4)
198-275: Solid work on the protocol-version-aware certificate handling, mate!The branching logic for the version 9 deposit bug is spot-on—like a well-executed save in Dark Souls, you're preserving backwards compatibility while moving forward. All
PallasCertificatevariants are covered, constructor indices look correct, and the pattern matching is exhaustive. Good stuff!
621-634: Clean delegation pattern here, like a well-designed game menu system.The
StakeAddresswrapper properly delegates to the kernel type, and theStakePayloadmatching with constructor indices 0/1 for keyhash/script aligns with the Plutus spec. No drama here!
658-660: Noted the commented-out protocol version 9 test.Fair enough to skip it if you're disregarding v9 support—just like skipping an optional side quest when you're focused on the main story. The comment explains the rationale, which is grand. Just make sure this decision is documented somewhere more permanent (ADR or similar) if v9 compatibility is officially off the table.
672-696: Test setup looks proper with the new Redeemers API.The
Redeemers::iter_from()pattern and subsequentredeemer.deref()usage is consistent with the updated ScriptContext construction flow. UsingPROTOCOL_VERSION_10as the hardcoded version makes sense for the test fixtures. She'll be right!
|
@yHSJ rebasing should help having the e2e tests pass |
yHSJ
force-pushed
the
josh/phase-2-validation
branch
from
6cb2651 to
eba3d87
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
crates/amaru-plutus/src/script_context/mod.rs (2)
58-59: Thepub redeemerfield is still exposed despite the TODO.As flagged before, exposing this for testing purposes isn't ideal. Consider a
#[cfg(test)]accessor method instead:🔎 Suggested approach
pub struct ScriptContext<'a> { tx_info: &'a TxInfo<'a>, - // TODO: this should not be exposed, this is just for testing purposes atm - pub redeemer: &'a Redeemer, + redeemer: &'a Redeemer, datum: Option<&'a PlutusData>, script_purpose: &'a ScriptPurpose<'a>, } +#[cfg(test)] +impl<'a> ScriptContext<'a> { + pub fn redeemer(&self) -> &'a Redeemer { + self.redeemer + } +}
783-798: Theunreachable!()for Native scripts is still here.While the protocol guarantees redeemers can't point to native scripts, using
unreachable!()means a bug elsewhere could crash the node. Consider returning a proper error instead - it's like having a fallback save point in case something goes sideways.The function already returns
Result, so this would be a natural fit:🔎 Defensive approach
match self { Script::PlutusV1(s) => decode_cbor_bytes(s.0.as_ref()), Script::PlutusV2(s) => decode_cbor_bytes(s.0.as_ref()), Script::PlutusV3(s) => decode_cbor_bytes(s.0.as_ref()), - Script::Native(_) => unreachable!("a redeemer should never point to a native_script"), + Script::Native(_) => Err(minicbor::decode::Error::message( + "cannot decode native script to bytes for redeemer execution", + )), }crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (2)
100-120: Still got a typo here, friend! "resolved_reference_inptus" should be "resolved_reference_inputs".The variable name has a sneaky typo that's been called out before. Just a quick find-and-replace job:
🔎 Quick fix
- let mut resolved_reference_inptus = transaction_body + let mut resolved_reference_inputs = transaction_body .reference_inputs // ... snip ... .unwrap_or_default(); - resolved_inputs.append(&mut resolved_reference_inptus); + resolved_inputs.append(&mut resolved_reference_inputs);
183-192: Thatexpectis still a ticking time bomb, mate.As flagged before, this
.expect()could panic in production if the CBOR round-trip fails. Even if it "should never happen," it's like leaving your house unlocked because you live in a nice neighbourhood - works until it doesn't.🔎 Suggested fix - propagate the error
let constants = args .iter() - .map(|arg| { + .map(|arg| -> Result<Constant<'_>, PhaseTwoError> { let bytes = to_cbor(&arg); - #[allow(clippy::expect_used)] - let data = PlutusData::from_cbor(&arena, &bytes) - .expect("unable to decode PlutusData cbor"); - Constant::Data(data) + let data = PlutusData::from_cbor(&arena, &bytes).map_err(|_| { + PhaseTwoError::ScriptDeserializationError( + cbor::decode::Error::message("PlutusData round-trip failed"), + ) + })?; + Ok(Constant::Data(data)) }) - .collect::<Vec<_>>(); + .collect::<Result<Vec<_>, _>>()?;
🧹 Nitpick comments (9)
.claude/settings.local.json (1)
1-10: G'day mate! This config's a bit of a curveball for a Phase 2 validation PR, innit?Fair dinkum, the permissions setup here is solid as – you're going with an allowlist approach which is proper security hygiene, like keeping your inventory management tight in Resident Evil. But here's the rub: this Claude config doesn't really vibe with the PR's stated objectives around ArenaPool and Plutus V3 validation, yeah?
A couple of thoughts, no worries:
Scope creep alert: This feels like developer tooling that'd be better off in its own wee PR, separate from the blockchain validation logic. Keeps things tidy, like Marie Kondo's take on version control.
Those wildcards though: The
:*patterns are pretty broad. For a local settings file that's probably grand, but worth a quick sanity check thatgit rebase:*andgit checkout:*won't let Claude go rogue and rebase your main branch into oblivion. I've seen weirder things happen (looking at you, every FromSoftware game ever).Missing some mates: No
git commitorgit pushin the allow list? Might be intentional, but feels like you'd want those for a proper dev workflow.Not a blocker by any stretch, just feels like this might've hitched a ride on the wrong PR, like Frodo accidentally bringing the One Ring to a beach party.
.github/workflows/continuous-integration.yml (1)
75-76: Consider documenting why this specific version was chosen, cobber.You've got the version
2025.11.04hardcoded in there, which is grand for reproducibility (love a good pinned version, like choosing the same save slot in Dark Souls). But there's no indication of why this particular version or whether it should be kept in sync with something else.If this version becomes outdated or if there's a specific reason you need this exact release, it'd be helpful to document that. Even just a comment noting "minimum version required for X feature" or "tested up to this version" would make future updates easier.
crates/amaru-ledger/tests/evaluate_ledger_states.rs (1)
215-226: G'day! Just curious about the phase split testing coverage, mate.You've updated this test to specifically call
phase_one::execute, which is grouse. But I'm not seeing any tests forphase_twoanywhere in this file. Given that the PR introduces Phase 2 validations (Plutus V3 script execution), shouldn't we have some test coverage for that new phase as well? Like, even a smoke test to ensure the plumbing works?Also, quick sanity check: you've switched from
Network::TestnettoNetworkName::Preprod. Looking at the fixture paths, they do reference preprod data, so this seems intentional and correct. But worth confirming this wasn't an accidental change during the refactor.Want me to generate a shell script to verify if there are phase_two tests hiding in other files, or help sketch out what a basic phase_two test might look like?
crates/amaru/src/bin/ledger/cmd/mod.rs (1)
35-42: Crikey! Using default config for the ArenaPool, are we?You're initializing the ArenaPool with
Config::default()values here. That's fair dinkum for getting things off the ground, but these arena settings can be pretty crucial for performance, yeah? Like tuning a car engine – different workloads might need different configurations.Since this is in the ledger binary, reckon it'd be worth exposing these as CLI arguments or config file options down the track? Users running beefy validation workloads might want to crank up the arena count or size, while resource-constrained environments would want to dial it back.
Not saying it needs to happen in this PR – sometimes you gotta ship the MVP before adding all the bells and whistles – but it's worth keeping on the radar.
If you want, I can help sketch out what a CLI config structure for these params might look like. No worries if you want to punt it to a future PR though!
examples/shared/src/lib.rs (1)
46-47: Strewth, those magic numbers could use a bit of explanation, cobber!You've got
ArenaPool::new(10, 1_024_000)here – 10 arenas at roughly 1MB each. Now, I know this is example code and all, but these values look like they were pulled from a hat, yeah?Since examples often become the de facto documentation (like how everyone learned React from TodoMVC back in the day), it'd be ace to add a quick comment explaining why you chose these specific values. Something like:
// Create an arena pool with 10 arenas of 1MB each // This is sufficient for typical block validation workloads let arena_pool = ArenaPool::new(10, 1_024_000);That way, when someone inevitably copies this code (as we all do with examples), they'll have a better sense of whether these values suit their use case or need tweaking.
crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs (1)
152-175: The&(*network).into()conversion pattern is functional but a wee bit clunky.It works, but reading
&(*network).into()requires a double-take. Consider whether you could adjustoutputs::executeto accept&NetworkNamedirectly, or create a helper method. Not a blocker by any means — just a readability nit, like those subtitles that scroll too fast in a cutscene.crates/amaru-kernel/src/arena_pool.rs (1)
60-79: Theacquire()loop with Condvar is correct, but there's a subtle inefficiency.The pattern here works, but I notice:
- Lock acquired at line 62
- If empty, wait at lines 68-72 (which atomically releases and re-acquires)
- Loop continues, re-acquiring lock at line 62
When
wait()returns, you already hold the lock, but then you immediately drop the guard and re-lock at the top of the loop. You could restructure to avoid the extra lock/unlock cycle:🔎 Proposed refactor to avoid redundant locking
pub fn acquire(&self) -> PooledArena { - let arena = loop { - let mut guard = self.inner.arenas.lock().unwrap_or_else(|p| p.into_inner()); - - if let Some(arena) = guard.pop_front() { - break arena; - } - - guard = self - .inner - .condvar - .wait(guard) - .unwrap_or_else(|p| p.into_inner()); - }; + let mut guard = self.inner.arenas.lock().unwrap_or_else(|p| p.into_inner()); + let arena = loop { + if let Some(arena) = guard.pop_front() { + break arena; + } + guard = self + .inner + .condvar + .wait(guard) + .unwrap_or_else(|p| p.into_inner()); + }; PooledArena { arena: ManuallyDrop::new(arena), pool: self.inner.clone(), } }crates/amaru-plutus/src/to_plutus_data.rs (1)
159-162: The double.to_plutus_data()call is a wee bit confusing, mate.This pattern of calling
to_plutus_data()then calling it again on the result is working becausePlutusDataitself implementsToPlutusData(which just clones). It's like that scene in Inception where you go a dream level deeper but nothing really changes.The first call returns
Result<PlutusData, PlutusDataError>, then the?unwraps it, then you callto_plutus_data()on thePlutusDataitself - which is essentially a no-op clone wrapped inOk.Consider simplifying to just return the result directly:
🔎 Proposed simplification
Address::Stake(stake_address) => { - <amaru_kernel::StakeAddress as ToPlutusData<V>>::to_plutus_data(stake_address)? - .to_plutus_data() + <amaru_kernel::StakeAddress as ToPlutusData<V>>::to_plutus_data(stake_address) }crates/amaru-plutus/src/script_context/mod.rs (1)
411-508: ScriptPurpose::builder is comprehensive but getting chunky.This function handles six different redeemer tags, each with its own script resolution logic. It works correctly, but at ~100 lines it's pushing the complexity boundary. It's like those RPG skill trees that just keep growing!
A few observations:
- Each branch follows a similar pattern: get item by index → check if script-based → lookup script → insert into table → return purpose
- The
Proposebranch has the most complex script hash extraction logicConsider extracting the common pattern into a helper, but this is optional - the current code is readable enough:
🔎 Optional refactor idea
fn try_insert_script<'a, T>( redeemer: &OrderedRedeemer<'a>, script_hash: &Hash<28>, scripts: &BTreeMap<Hash<28>, Script<'a>>, script_table: &mut BTreeMap<OrderedRedeemer<'a>, Script<'a>>, make_purpose: impl FnOnce() -> T, ) -> Option<T> { scripts.get(script_hash).map(|script| { script_table.insert(redeemer.clone(), script.clone()); make_purpose() }) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.cargo/deny.toml.claude/settings.local.json.github/workflows/continuous-integration.ymlCargo.tomlcrates/amaru-kernel/Cargo.tomlcrates/amaru-kernel/src/arena_pool.rscrates/amaru-kernel/src/lib.rscrates/amaru-kernel/src/ordered_redeemer.rscrates/amaru-ledger/Cargo.tomlcrates/amaru-ledger/src/block_validator.rscrates/amaru-ledger/src/rules.rscrates/amaru-ledger/src/rules/block.rscrates/amaru-ledger/src/rules/transaction/mod.rscrates/amaru-ledger/src/rules/transaction/phase_one/certificates.rscrates/amaru-ledger/src/rules/transaction/phase_one/collateral.rscrates/amaru-ledger/src/rules/transaction/phase_one/fees.rscrates/amaru-ledger/src/rules/transaction/phase_one/inputs.rscrates/amaru-ledger/src/rules/transaction/phase_one/metadata.rscrates/amaru-ledger/src/rules/transaction/phase_one/mint.rscrates/amaru-ledger/src/rules/transaction/phase_one/mod.rscrates/amaru-ledger/src/rules/transaction/phase_one/outputs.rscrates/amaru-ledger/src/rules/transaction/phase_one/outputs/inherent_value.rscrates/amaru-ledger/src/rules/transaction/phase_one/proposals.rscrates/amaru-ledger/src/rules/transaction/phase_one/scripts.rscrates/amaru-ledger/src/rules/transaction/phase_one/vkey_witness.rscrates/amaru-ledger/src/rules/transaction/phase_one/voting_procedures.rscrates/amaru-ledger/src/rules/transaction/phase_one/withdrawals.rscrates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-ledger/src/state.rscrates/amaru-ledger/tests/evaluate_ledger_states.rscrates/amaru-plutus/src/script_context/mod.rscrates/amaru-plutus/src/script_context/v1.rscrates/amaru-plutus/src/script_context/v2.rscrates/amaru-plutus/src/script_context/v3.rscrates/amaru-plutus/src/to_plutus_data.rscrates/amaru/src/bin/amaru/cmd/run.rscrates/amaru/src/bin/ledger/cmd/mod.rscrates/amaru/src/stages/mod.rsexamples/shared/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/amaru-ledger/src/rules.rs
- crates/amaru-ledger/src/block_validator.rs
- Cargo.toml
- crates/amaru-ledger/src/state.rs
- crates/amaru-ledger/src/rules/transaction/mod.rs
- crates/amaru-ledger/Cargo.toml
- crates/amaru-ledger/src/rules/transaction/phase_one/scripts.rs
- .cargo/deny.toml
- crates/amaru/src/bin/amaru/cmd/run.rs
🧰 Additional context used
🧠 Learnings (22)
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 387
File: crates/amaru-stores/src/lib.rs:40-40
Timestamp: 2025-08-20T13:02:25.763Z
Learning: In the amaru-stores crate, amaru_slot_arithmetic types like Epoch and EraHistory are used throughout the main crate code in modules like in_memory/mod.rs, rocksdb/consensus.rs, and rocksdb/ledger/columns/, not just in tests. This means amaru-slot-arithmetic should be a regular dependency, not a dev-dependency.
Applied to files:
crates/amaru-kernel/Cargo.tomlcrates/amaru/src/bin/ledger/cmd/mod.rscrates/amaru-kernel/src/lib.rscrates/amaru-ledger/tests/evaluate_ledger_states.rscrates/amaru/src/stages/mod.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-08-08T14:46:53.013Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/pool_params.rs:107-116
Timestamp: 2025-08-08T14:46:53.013Z
Learning: In crates/amaru-kernel/src/pool_params.rs, when serializing Relay::SingleHostAddr IPv6 to text, the project intentionally reverses each 4-byte chunk before constructing std::net::Ipv6Addr. This matches cardano-ledger’s IPv6 representation (four little-endian Word32 chunks). Do not “simplify” by passing the raw 16 bytes directly to Ipv6Addr::from; that would break ledger compatibility.
Applied to files:
crates/amaru-plutus/src/script_context/v1.rsexamples/shared/src/lib.rscrates/amaru/src/bin/ledger/cmd/mod.rscrates/amaru/src/stages/mod.rs
📚 Learning: 2025-01-31T09:19:55.491Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 64
File: crates/amaru/src/ledger/state/transaction.rs:62-68
Timestamp: 2025-01-31T09:19:55.491Z
Learning: In the Cardano ledger, withdrawal uniqueness is guaranteed by the on-wire format's serialization, which considers duplicate withdrawals as invalid. This guarantee is reflected in Rust's type system, making additional runtime uniqueness checks unnecessary even when using vectors internally, as the data originates from minted blocks.
Applied to files:
crates/amaru-plutus/src/script_context/v1.rs
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru-plutus/src/script_context/v1.rscrates/amaru-ledger/src/rules/transaction/phase_one/outputs.rsexamples/shared/src/lib.rscrates/amaru-ledger/src/rules/transaction/phase_one/mod.rscrates/amaru-plutus/src/script_context/v2.rscrates/amaru/src/bin/ledger/cmd/mod.rscrates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-ledger/src/rules/block.rscrates/amaru-kernel/src/lib.rscrates/amaru-ledger/tests/evaluate_ledger_states.rscrates/amaru/src/stages/mod.rscrates/amaru-kernel/src/arena_pool.rscrates/amaru-plutus/src/to_plutus_data.rscrates/amaru-plutus/src/script_context/v3.rscrates/amaru-kernel/src/ordered_redeemer.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-05-21T18:58:48.631Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 228
File: crates/amaru-stores/src/rocksdb/consensus.rs:89-128
Timestamp: 2025-05-21T18:58:48.631Z
Learning: The InMemConsensusStore implementation in crates/amaru-stores/src/rocksdb/consensus.rs will be fleshed out incrementally on a by-need basis, driven by test requirements rather than implementing all functionality upfront.
Applied to files:
crates/amaru/src/bin/ledger/cmd/mod.rs
📚 Learning: 2025-01-21T15:32:17.911Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 69
File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117
Timestamp: 2025-01-21T15:32:17.911Z
Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-ledger/src/rules/block.rscrates/amaru-plutus/src/script_context/v3.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-08-08T14:39:50.527Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/borrowed_datum.rs:32-39
Timestamp: 2025-08-08T14:39:50.527Z
Learning: In the amaru project, when converting BorrowedDatumOption::Data to an owned DatumOption in crates/amaru-kernel/src/borrowed_datum.rs, the call `.unwrap()` refers to pallas’s KeepRaw::unwrap, which is infallible (always returns the inner value) and is not a panic risk. Future reviews should not flag this unwrap as dangerous.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-04-20T18:02:25.073Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward.rs:73-75
Timestamp: 2025-04-20T18:02:25.073Z
Learning: In the current development stage, rkuhn prefers using explicit panics (via `.expect()` or `.unwrap()`) for fatal errors in the application code that would tear down the node, rather than propagating errors with `Result`. The intention is to eventually transition to proper error handling with `Result` as the codebase matures.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-09-06T09:16:25.025Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 435
File: crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs:113-116
Timestamp: 2025-09-06T09:16:25.025Z
Learning: In cardano-node serialized ledger state CBOR encoding, indefinite-length structures may be terminated with 0xFF "break" markers. The current code in convert_ledger_state.rs unconditionally strips the last byte (bytes[p..bytes.len() - 1]), which could corrupt data if the trailing byte is not actually a CBOR break marker.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-12-28T19:26:35.354Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 612
File: crates/amaru-protocols/src/blockfetch/mod.rs:151-173
Timestamp: 2025-12-28T19:26:35.354Z
Learning: In crates/amaru-protocols/src/blockfetch/mod.rs, the blockfetch initiator uses .expect() when popping from the request queue on NoBlocks and Done results. These are intentional fail-fast assertions: the protocol state machine guarantees the queue is non-empty when these messages arrive, so an empty queue indicates a protocol violation. A misbehaving peer triggers an erroneous protocol transition that will close the connection (supervision to be implemented in a future PR). This follows the project's fail-fast philosophy for protocol invariants.
<!--
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-ledger/src/rules/block.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-08-12T12:28:24.027Z
Learnt from: etorreborre
Repo: pragma-org/amaru PR: 372
File: simulation/amaru-sim/src/simulator/mod.rs:410-412
Timestamp: 2025-08-12T12:28:24.027Z
Learning: In the Amaru project, panic statements are acceptable in simulation/test code (like amaru-sim crate) as they help identify configuration issues quickly during development, rather than needing proper error handling like production code.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-08-08T14:34:06.105Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/minicbor-extra/src/lib.rs:50-55
Timestamp: 2025-08-08T14:34:06.105Z
Learning: Project uses Rust 1.88 stable; using Result::is_err_and is acceptable and should not be flagged as unstable. In particular, crates/minicbor-extra/src/lib.rs relies on is_err_and when checking end_of_input; future reviews should not suggest replacing it for stability reasons.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-02-03T11:15:22.640Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 75
File: crates/amaru/src/consensus/mod.rs:164-165
Timestamp: 2025-02-03T11:15:22.640Z
Learning: In the Amaru project, chain selection operations (roll_forward and rollback) should use separate result types to leverage the type system for preventing impossible states, rather than using runtime checks or panics.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-ledger/src/rules/block.rs
📚 Learning: 2025-04-20T17:57:23.233Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:272-285
Timestamp: 2025-04-20T17:57:23.233Z
Learning: In test infrastructure code, rkuhn prefers explicit panics (using .unwrap() or similar) over returning Result types, as test failures should be immediate and obvious.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-kernel/src/lib.rscrates/amaru-ledger/tests/evaluate_ledger_states.rs
📚 Learning: 2025-08-08T14:35:35.562Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/transaction_pointer.rs:36-44
Timestamp: 2025-08-08T14:35:35.562Z
Learning: In the amaru project, when decoding CBOR arrays, prefer using minicbor_extra::heterogenous_array with the expected length to validate definite-length arrays and correctly handle indefinite-length arrays. Example: crates/amaru-kernel/src/transaction_pointer.rs Decode should use heterogenous_array(d, 2, …) instead of ignoring the length from d.array().
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-ledger/src/rules/block.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In crates/amaru-consensus, flood prevention for duplicate invalid headers should be implemented in the validate_header stage or database layer, not in the store_header stage, since store_header runs before validation and cannot determine header validity.
Applied to files:
crates/amaru-ledger/src/rules/block.rs
📚 Learning: 2025-09-29T20:08:29.906Z
Learnt from: yHSJ
Repo: pragma-org/amaru PR: 453
File: crates/amaru-metrics/src/ledger.rs:60-104
Timestamp: 2025-09-29T20:08:29.906Z
Learning: In amaru-ledger/src/state.rs, the txs_processed field represents the number of transactions in the current block (block.transaction_bodies.len() as u64), not a cumulative total. Therefore, using a Counter with .add() in the metrics is correct for accumulating per-block transaction counts.
Applied to files:
crates/amaru-ledger/src/rules/block.rs
📚 Learning: 2025-05-12T14:21:27.470Z
Learnt from: stevana
Repo: pragma-org/amaru PR: 210
File: simulation/amaru-sim/src/simulator/simulate.rs:264-277
Timestamp: 2025-05-12T14:21:27.470Z
Learning: The team plans to replace the out-of-process test in `simulation/amaru-sim/src/simulator/simulate.rs` with an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (`../../target/debug/echo`) and making tests more reliable.
Applied to files:
crates/amaru-ledger/tests/evaluate_ledger_states.rs
📚 Learning: 2025-08-11T06:17:56.499Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 372
File: crates/amaru-consensus/src/consensus/headers_tree.rs:612-614
Timestamp: 2025-08-11T06:17:56.499Z
Learning: The `indextree::Arena<T>` struct in version 4.7.4 has a `count()` method that returns the total number of nodes in the arena, including removed nodes. This is a valid method to call on Arena instances in Rust code using the indextree crate.
Applied to files:
crates/amaru-kernel/src/arena_pool.rs
📚 Learning: 2025-06-14T16:36:04.502Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 263
File: crates/pure-stage/src/simulation/state.rs:33-36
Timestamp: 2025-06-14T16:36:04.502Z
Learning: In simulation and replay systems that require cloneable and serializable states, error types must often be converted to String rather than stored as trait objects (like Box<dyn Error> or anyhow::Error) because trait objects cannot be cloned, which breaks the snapshotting and replay functionality needed for deterministic simulation.
Applied to files:
crates/amaru-plutus/src/to_plutus_data.rs
📚 Learning: 2025-09-29T16:39:24.001Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/mux.rs:317-325
Timestamp: 2025-09-29T16:39:24.001Z
Learning: In crates/amaru-network/src/mux.rs, the outgoing() method intentionally uses unwrap() after get_mut(&proto_id) as a fail-fast mechanism. This panic is designed to catch programming errors where an actor tries to send on an unregistered protocol, and should not be changed to return a Result since it represents internal code bugs that should terminate the process, not external input that should be handled gracefully.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-09-29T16:44:14.807Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/protocol.rs:94-106
Timestamp: 2025-09-29T16:44:14.807Z
Learning: In the amaru-network crate protocol.rs file, the correct Cardano mini-protocol ID assignments are: PROTO_N2N_KEEP_ALIVE = 8 and PROTO_N2N_PEER_SHARE = 10, as verified against the network specification by the maintainer.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
🧬 Code graph analysis (8)
crates/amaru-plutus/src/script_context/v1.rs (2)
crates/amaru-plutus/src/script_context/v3.rs (8)
to_plutus_data(37-46)to_plutus_data(50-59)to_plutus_data(63-85)to_plutus_data(89-98)to_plutus_data(102-111)to_plutus_data(115-117)to_plutus_data(121-123)redeemers(681-704)crates/amaru-kernel/src/lib.rs (5)
stake_credential(767-767)stake_credential(771-780)redeemers(982-982)redeemers(1000-1019)new(326-328)
crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs (3)
crates/amaru-ledger/src/state.rs (2)
network(224-226)protocol_parameters(232-234)crates/amaru-ledger/src/rules/transaction/phase_one/collateral.rs (1)
is_valid(94-96)crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (1)
transaction_body(86-98)
crates/amaru-plutus/src/script_context/v2.rs (2)
crates/amaru-plutus/src/script_context/v3.rs (3)
to_plutus_data(37-46)to_plutus_data(50-59)to_plutus_data(63-85)crates/amaru-plutus/src/script_context/v1.rs (13)
to_plutus_data(29-31)to_plutus_data(40-42)to_plutus_data(46-70)to_plutus_data(85-103)to_plutus_data(114-127)to_plutus_data(135-140)to_plutus_data(144-146)to_plutus_data(153-155)to_plutus_data(188-222)to_plutus_data(227-239)to_plutus_data(250-261)to_plutus_data(265-267)to_plutus_data(271-273)
crates/amaru-ledger/tests/evaluate_ledger_states.rs (1)
crates/amaru-ledger/src/state.rs (1)
network(224-226)
crates/amaru-kernel/src/arena_pool.rs (1)
crates/amaru-ledger/src/block_validator.rs (1)
new(41-54)
crates/amaru-plutus/src/to_plutus_data.rs (1)
crates/amaru-plutus/src/script_context/v3.rs (16)
to_plutus_data(37-46)to_plutus_data(50-59)to_plutus_data(63-85)to_plutus_data(89-98)to_plutus_data(102-111)to_plutus_data(115-117)to_plutus_data(121-123)to_plutus_data(127-139)to_plutus_data(142-184)to_plutus_data(188-195)to_plutus_data(201-274)to_plutus_data(278-294)to_plutus_data(299-308)to_plutus_data(322-361)to_plutus_data(365-367)to_plutus_data(371-373)
crates/amaru-plutus/src/script_context/v3.rs (4)
crates/amaru-plutus/src/script_context/mod.rs (8)
to_plutus_data(1098-1110)script_hash(823-830)Redeemers(309-330)iter_from(1114-1128)new(68-90)new(208-361)new(594-613)new(1079-1081)crates/amaru-plutus/src/script_context/v2.rs (2)
to_plutus_data(26-28)to_plutus_data(32-51)crates/amaru-kernel/src/lib.rs (10)
stake_credential(767-767)stake_credential(771-780)script_hash(857-857)script_hash(861-868)script_hash(872-874)script_hash(878-880)script_hash(888-890)redeemers(982-982)redeemers(1000-1019)new(326-328)crates/amaru-plutus/src/to_plutus_data.rs (1)
None(154-154)
crates/amaru-plutus/src/script_context/mod.rs (3)
crates/amaru-kernel/src/lib.rs (10)
new(326-328)from(286-288)from(309-311)from(347-349)from(515-520)from(524-531)from(583-591)deref(280-282)deref(297-299)deref(355-357)crates/amaru-kernel/src/ordered_redeemer.rs (4)
from(47-49)from(53-55)from(59-61)deref(41-43)crates/amaru-kernel/src/is_header/mod.rs (5)
from(185-190)hash(35-35)hash(137-139)hash(199-201)hash(221-223)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build x86_64/windows
- GitHub Check: Build x86_64/linux
- GitHub Check: clippy
- GitHub Check: Test coverage
- GitHub Check: Build riscv32
- GitHub Check: Test the CLI
- GitHub Check: Build aarch64/macos
- GitHub Check: Benches
- GitHub Check: Analyze (rust)
🔇 Additional comments (28)
crates/amaru-kernel/src/lib.rs (3)
124-126: Lookin' good, mate!The new
ordered_redeemermodule follows the existing pattern in the crate — module declaration followed by glob re-export. Clean as a whistle.
184-186: Nice one, the arena_pool module slots in perfectly.Follows the same convention as other modules in the crate. Good stuff!
822-844: Solid implementation ofHasOwnershipforCertificate.This covers all the certificate variants nicely. The mapping logic is sound:
- Stake-related certs return the stake credential directly
- Pool certs return the operator/pool id as an
AddrKeyhashOne thing I noticed — there's no fallback arm here, which means if new certificate variants are added to pallas in the future, this will break at compile time. That's actually a good thing in this case — "fail fast" like in Dark Souls, ya know? Forces the dev to handle new cases explicitly.
crates/amaru-kernel/src/ordered_redeemer.rs (2)
18-30: Crikey, this is a clean bit of work!The
OrderedRedeemerwrapper is well-designed — usingCowgives you the flexibility of both borrowed and owned data, like having your cake and eating it too. TheOrdimplementation that compares bytag.as_index()first then byindexmatches the Haskell implementation as noted in the doc comment. That's some proper cross-language compatibility right there, like making sure your save files work on both PS5 and PC!
38-44: Deref toRedeemeris the right call here.This lets callers access
tag,index,data, andex_unitsdirectly through the wrapper without any ceremony. Ergonomic as heck.crates/amaru/src/stages/mod.rs (3)
98-106: Fair dinkum config additions for the VM arena pool!The comments are helpful — they explain the trade-offs nicely. A couple of thoughts:
ledger_vm_alloc_arena_count: Default of 1 works for sequential validation, but if you're planning parallel script evaluation down the road (like running multiple quests simultaneously in an RPG), you'll want to bump this up.ledger_vm_alloc_arena_size: 1MB is a solid starting point. The note about memory leaking is spot on — bumpalo arenas intentionally don't deallocate, so this is "pay upfront" memory.
338-341: ArenaPool initialization looks proper.Creating the pool once here and passing it down to the
BlockValidatoris the right pattern — like crafting your gear before heading into the dungeon. Pre-allocating arenas upfront means no surprise allocation costs during block validation.
345-366: BlockValidator wiring updated correctly for both store types.Both
InMemandRocksDbbranches now receivevm_eval_poolandconfig.network. The symmetry here is nice — same interface regardless of backend. Good pattern, like having the same controls whether you're playing with keyboard or controller.crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs (2)
58-86: Renaming toPhaseOneErroris a solid move.With
phase_twonow in the mix, havingPhaseOneErrorandPhaseTwoErrormakes the codebase way more navigable — like having proper waypoints on your map instead of just "here be dragons."
219-229: Return value change is well thought out.Returning the
consumed_inputsinstead of relying on side effects is cleaner and makes the data flow explicit. Theis_validbranching logic correctly handles:
- Valid tx → consume regular inputs
- Invalid tx → consume collateral inputs
This sets up phase two nicely to know what was actually consumed. Good design, mate!
crates/amaru-kernel/src/arena_pool.rs (2)
38-55: Pre-allocation on construction is the way to go.Based on past review discussions, this approach of creating all arenas upfront is solid — fail early if memory can't be allocated, and no surprise latency spikes during runtime. Like loading all your assets during the loading screen instead of mid-gameplay stutters.
108-117: Drop implementation is sound.The SAFETY comment is appreciated. Using
ManuallyDrop::takeonce in the drop handler is correct — the arena is reset before being returned to the pool, ready for reuse. The Condvar notify ensures waiters get signaled. Clean as a freshly reset save file!crates/amaru-plutus/src/script_context/v1.rs (3)
130-147: StakeAddress serialization for PlutusV1 looks correct.The nesting is right —
Constr(0, [Constr(0/1, [hash])])where the outer 0 represents "Credential" (not Pointer) and the inner 0/1 represents key vs script hash. The comment about Pointers not being constructible via ledger serialization is a nice bit of context. Like an Easter egg that explains the lore!
264-268: Withdrawals serialization change to collect into Vec is fine.For v1, serializing withdrawals as a list (via
Vec) rather than a map is correct per the Plutus spec for that version.
188-222: Certificate handling in v1 is spot-on—like a perfect speedrun strat.The match arms handle all the Plutus v1-relevant variants (StakeRegistration, Reg, StakeDeregistration, UnReg, StakeDelegation, PoolRegistration, PoolRetirement) explicitly, translating older certificate types for backward compat. The wildcard catch-all properly rejects newer governance certificates (VoteDeleg, DRepCert variants, AuthCommitteeHot, etc.) added in the Conway era—they've got no business in a v1 context, so returning
unsupported_versionis the right call. No silent failures, no broken assumptions. Brilliant stuff.crates/amaru-plutus/src/script_context/v2.rs (2)
83-88: Withdrawals serialization now explicitly materializes the BTreeMap.Collecting to
BTreeMapbefore serialization ensures deterministic ordering. The intermediate allocation is minimal since you're just collecting references. Good defensive programming — like saving before a boss fight!
90-106: StakeAddress PlutusV2 serialization mirrors v1 correctly.The pattern is identical to v1 (and matches the v3 approach from the relevant snippets), just using
constr_v2!instead. The doc comment correctly notes that Pointer variants aren't constructible. Consistency across versions is on point — like a well-designed UI that works the same across all platforms.crates/amaru-ledger/src/rules/block.rs (2)
39-45: Nice one, cobber! Clean error abstraction here.The
TransactionInvalidenum wrapping both phase errors is a solid pattern. It's like choosing your character class in an RPG - you've got your distinct error types that can still be treated uniformly when needed. The#[from]derive makes conversions smooth as butter.
250-292: Solid execution flow - phase one, phase two, then consume. Like a proper boss fight sequence!The pattern here is spot-on:
- Phase one validates and returns the consumed inputs
- Phase two runs script evaluation
- Only if both succeed do you actually consume the inputs
This ensures you don't corrupt state if phase two fails. It's like saving your game before the final boss - if you die, you haven't lost your progress. Good defensive design, mate!
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (2)
235-242: The validity state logic is clean and correct.When
is_validis true, you expect scripts to succeed - if they fail, that's an error. Whenis_validis false (transaction already marked invalid), you expect scripts to fail - if they succeed, something's wrong.It's like that scene in The Matrix where you're testing if someone is The One - you expect a certain outcome based on context. Nice inversion logic here!
201-204: Safe to keep as-is; overflow is theoretically impossible with valid Cardano transactions.Cardano protocol limits guarantee
budget.mem≤ 62M andbudget.steps≤ 40B—both well underi64::MAX(~9.2 quintillion). While unchecked casts are technically risky, the overflow scenario is physically unreachable within legitimate transaction constraints. The codebase also uses similaru64 as i64conversions throughout (seecollateral.rs) without defensive checks, so adding try_from here would be inconsistent. If you're concerned about catching bugs elsewhere that might somehow inflate these values, error handling would be more appropriate than silent clamping toi64::MAX.crates/amaru-plutus/src/script_context/v3.rs (3)
198-212: Protocol version 9 bug handling - good historical compatibility!This is like when game devs have to maintain backwards compatibility with old save files. The comment explains the bug nicely, and the branching on
protocol_version.0 > 9is the right approach.For
StakeRegistrationandStakeDeregistration(the old-style certs), you always emitNonefor deposits. For the newReg/UnRegvariants, you emit the deposit only on protocol version 10+. Solid work, mate!
672-679: Test setup looks good - proper use of Redeemers.deref().The test code properly unwraps the redeemers and uses
deref()to access the underlying data. UsingPROTOCOL_VERSION_10explicitly makes the test deterministic. Fair dinkum testing setup!
520-532: Address the TODO on CostModels encoding before production deployment.This isn't just a nitpick — the CostModels PlutusData encoding genuinely needs validation against the Cardano ledger spec. Our web research confirmed that CostModels are critical for script evaluation, the ordering of parameters matters, and different Plutus versions have specific requirements. The current implementation uses
constr_v3!(0, [plutus_v1, plutus_v2, plutus_v3]), which is a reasonable guess, but it's unvalidated against the Haskell reference implementation.Without test vectors from real on-chain governance transactions that include CostModels updates, there's a risk of silent encoding mismatches. Add integration tests using actual Cardano ledger examples to verify the encoding matches expectations.
crates/amaru-plutus/src/script_context/mod.rs (4)
114-116: Nice addition - exposing the budget through ScriptContext.Clean accessor for the execution units. This'll be handy when you need to track or report on script costs. Short, sweet, and does exactly what it says on the tin!
363-372: Theto_script_contextsmethod is a clean abstraction.This is like a factory method in a game engine - takes the TxInfo and produces all the individual ScriptContexts you need to evaluate. The filter_map handles cases where ScriptContext construction fails gracefully. Noice!
963-977: Certificate wrapper nicely encapsulates the protocol version context.Having the
protocol_versiontravel with the certificate means serialization logic can make the right decisions without needing to pass version info separately. TheDerefimplementation keeps the ergonomics clean - you can still use certificate methods directly. It's like a character skin in a game - same functionality, different presentation depending on context!
1076-1129: Redeemers wrapper is well-designed with proper iteration support.The
iter_fromstatic method handles both list and map redeemer formats from Pallas, and theDerefimpl means you can use it like a regular BTreeMap. TheToPlutusDataimplementation correctly pairs each redeemer with its purpose for serialization.One small thought - the boxed iterator in
iter_fromis fine for this use case, but if performance ever becomes a concern, you might consider using an enum dispatch instead. For now though, this is totally sound!
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @scripts/cleanup-runner.sh:
- Around line 145-155: The loop over the to_remove array prints warnings for
non-existent elements but doesn't prevent them from being passed to sudo rm -rf;
update the cleanup logic in the for loop that iterates over "${to_remove[@]}"
(and/or the preparation of the to_remove array) so non-existent entries are
actually skipped: either add a continue inside the if [ ! -e "$element" ] block
to avoid adding that element to the final removal, or build a new filtered list
of existing paths and use that filtered array in the sudo rm -rf
"${to_remove[@]}" call; ensure the unique symbol to_remove and the for loop
handling are adjusted accordingly.
♻️ Duplicate comments (1)
.github/workflows/continuous-integration.yml (1)
77-78: Mate, the checksum verification is still MIA! 🔒This was flagged in the previous review but hasn't been addressed. You're downloading a compiler toolchain from the internet without verifying its integrity. If that GitHub release ever gets compromised or there's a man-in-the-middle scenario, you're building your entire project with a potentially dodgy toolchain—not ideal, yeah?
🔎 Add SHA256 verification
First, grab the SHA256 checksum for this specific release (you can get it by downloading the file once manually or checking if the release page provides checksums). Then:
setup: | rustup component add rust-src --toolchain nightly + EXPECTED_SHA256="<insert-actual-sha256-here>" curl -L -o riscv32-glibc-llvm.tar.xz \ "https://github.com/riscv-collab/riscv-gnu-toolchain/releases/download/2025.11.04/riscv32-glibc-ubuntu-24.04-llvm.tar.xz" + echo "$EXPECTED_SHA256 riscv32-glibc-llvm.tar.xz" | sha256sum -c - tar -xvf riscv32-glibc-llvm.tar.xz export CC_riscv32gc_unknown_linux_gnu="$(pwd)/riscv/bin/riscv32-unknown-linux-gnu-clang"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/continuous-integration.ymlscripts/cleanup-runner.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build aarch64/macos
- GitHub Check: Build riscv32
- GitHub Check: Build x86_64/windows
- GitHub Check: Build x86_64/linux
- GitHub Check: Benches
- GitHub Check: Test the CLI
- GitHub Check: Test coverage
- GitHub Check: clippy
- GitHub Check: Analyze (rust)
🔇 Additional comments (9)
.github/workflows/continuous-integration.yml (2)
67-68: Nice one! The explanation landed. 👌You've added the comment that KtorZ asked for in the previous review. It's clear why you need the custom RISC-V toolchain for the uplc-turbo crate compilation. Future you (or some other dev six months from now) will thank present you for this breadcrumb.
256-257: Clean refactor—centralising the cleanup logic. 🧹Replacing the ad-hoc deletion commands with a dedicated script is a solid move. Makes it way easier to maintain and tweak the cleanup logic without touching the workflow YAML. Plus, you're properly setting execute permissions first—no dramas there.
scripts/cleanup-runner.sh (7)
1-6: Solid foundation with the error handling flags. 👍Starting with
set -euo pipefailis proper defensive bash scripting—exits on errors, catches undefined variables, and fails on pipe errors. Plus, crediting the inspiration from the Rust project's cleanup script is good form.
7-25: Helper functions look sweet—simple and defensive. ✅The architecture detection and runner environment check are straightforward. The
${RUNNER_ENVIRONMENT:-}syntax is proper bash defensive coding, handling the case where the variable might not be set. No worries here.
35-46: Space helpers looking good. 📊The
getAvailableSpaceandformatByteCountfunctions do what they say on the tin. Usingdf -aincludes pseudo filesystems, but that's fine for this use case—you're just tracking changes. The KB-to-bytes conversion and numfmt formatting is spot on.
48-85: Output functions with nice defensive touches. 🎨The negative-space check in
printSavedSpaceis a smart safeguard, and using GitHub Actions'::warning::format means it'll show up properly in the CI logs. Both functions are just cosmetic helpers—they're doing their job well.
158-167: Clean wrapper function—does exactly what it needs to. ✅This function captures the space before running an operation, executes it, then reports how much space was saved. Simple, clear, and effective. No dramas here.
169-226: Cleanup operations are well-structured with proper error handling. 🧼The package cleanup with dpkg lock timeout handling is smart—avoids race conditions with other apt processes. The conditional package lists based on architecture and runner type make sense. Docker and swap cleanup operations are straightforward with appropriate
|| truefallbacks so they won't tank the whole script if something's not present. All looking solid.
228-246: Main execution flow is clean and well-orchestrated. 🎬The script captures the initial state, runs each cleanup step while measuring impact, then shows the final results. It's like a proper montage sequence—you know exactly what's happening at each stage. The before/after DF output is particularly helpful for debugging CI issues.
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: KtorZ <matthias.benkort@gmail.com>
Less confusing of a name with clearer intent (providing Ord instances). Using Cow also works well here to avoid unnecessary clones, since we already have lifetimes on TxInfo anyway. Also moved into its own module, to keep the ongoing pattern in the kernel for newly introduced types. Signed-off-by: KtorZ <matthias.benkort@gmail.com>
…FIXME The FIXME is in fact unecessary; we need not to use the exact same version as uplc-turbo; in fact, that's an issue for the dependency constraint solver to solve. We should even be able to use different versions so long as they are compatible. Signed-off-by: KtorZ <matthias.benkort@gmail.com>
- Clarify names outside of the ledger and add comments explaining what the config parameters stands for. - Reduce the default number of arenas to 1; we aren't parellalizing anything at the moment, so there's no point. - Move where we create the arena; it's not a concern of the upper layers (I would even be tempted to move it further down the line, inside the ledger state creation) - DO NOT EXPOSE arena settings to end-users. This is not a configuration we want to bother users with. This is something we have to fine-tune as implementors. Signed-off-by: KtorZ <matthias.benkort@gmail.com>
Signed-off-by: KtorZ <matthias.benkort@gmail.com>
- Avoid duplicating sorting logic between 'Redeemers' and
'OrderedRedeemer'. The Ord instance on the latter already carry the
sorting logic, so we can
- Store redeemers in an 'inverted' BTreeMap:
- This works because we know redeemer values are unique (different
tags and indexes; although I admittedly haven't controlled that we
have a test for it in the rules that would correctly rule out
duplications).
- Doing this, we reduce the number of traversals we have to do on
the redeemers, everything is much more streamlined.
- We also allow near-instant lookups of redeemers, instead of having
search them by tag & index in O(n).
- Factor out the common instance for ToPlutusData between V2 and V3;
generally speaking, we should avoid duplicating instances where
possible. We use a parameterized trait for that reason!
Signed-off-by: KtorZ <matthias.benkort@gmail.com>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
yHSJ
force-pushed
the
josh/phase-2-validation
branch
from
f965929 to
e839b85
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/amaru-ledger/tests/evaluate_ledger_states.rs (1)
34-35: Fix network mismatch: test vectors and validation must use the same network.Line 34 imports test vectors with
NetworkName::Testnet(1), but line 217 validates transactions withNetworkName::Preprod. Using different networks for vector import and transaction validation will cause parameter and rule mismatches—like trying to run PAL game code on an NTSC console, yeah? The test vectors are specifically constructed for one network's context, so validating against a different one defeats the purpose.Either use
NetworkName::Testnet(1)consistently throughout, or clarify why these should differ.
🤖 Fix all issues with AI Agents
In @crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs:
- Line 100: Rename the misspelled local variable resolved_reference_inptus to
resolved_reference_inputs and update every usage/occurrence (e.g., where it's
later referenced) to the corrected name so the code compiles and reads clearly;
search for all instances of resolved_reference_inptus in this module and replace
them with resolved_reference_inputs.
In @crates/amaru-plutus/src/to_plutus_data.rs:
- Around line 159-162: The Address::Stake arm is calling
<amaru_kernel::StakeAddress as ToPlutusData<V>>::to_plutus_data(stake_address)?
and then calling .to_plutus_data() again on the resulting PlutusData which
causes an unnecessary clone/allocation; remove the trailing .to_plutus_data()
and return the Result from <amaru_kernel::StakeAddress as
ToPlutusData<V>>::to_plutus_data(stake_address) directly (or add an explicit
type annotation on the match arm return if the compiler needs help) to avoid the
extra allocation.
🧹 Nitpick comments (5)
crates/amaru-kernel/src/arena_pool.rs (2)
60-79: Blocking acquire looks good, but consider poisoned mutex handling.The loop-and-wait pattern with
Condvaris spot-on for avoiding busy-wait CPU thrashing. However, theunwrap_or_else(|p| p.into_inner())on lines 62 and 72 will recover from poisoned mutexes by extracting the inner data.In a production node, a poisoned mutex typically indicates a panic in another thread while holding the lock—continuing with potentially corrupted state might not be the safest play. Consider whether panicking on poison is more appropriate for this critical path.
🔎 Alternative: panic on poisoned mutex
pub fn acquire(&self) -> PooledArena { let arena = loop { - let mut guard = self.inner.arenas.lock().unwrap_or_else(|p| p.into_inner()); + let mut guard = self.inner.arenas.lock() + .expect("arena pool mutex poisoned"); if let Some(arena) = guard.pop_front() { break arena; } guard = self .inner .condvar .wait(guard) - .unwrap_or_else(|p| p.into_inner()); + .expect("arena pool condvar wait failed on poisoned mutex"); }; PooledArena { arena: ManuallyDrop::new(arena), pool: self.inner.clone(), } }
43-43: Consider documentinginitial_capacityguidance.The
initial_capacityparameter (line 46) determines the pre-allocated size of each arena. It'd be helpful to document what a reasonable value looks like (e.g., "typical Plutus script evaluation uses ~1MB per arena" or similar), so users don't accidentally under-provision or waste memory.📝 Suggested doc enhancement
/// Create a new arena pool with a fixed number of pre-allocated arenas. /// /// All `size` arenas are created immediately with `initial_capacity` bytes each. + /// The `initial_capacity` should be large enough to handle typical workloads without + /// reallocation (e.g., 1MB for Plutus script evaluation). /// If all arenas are in use, `acquire()` will block. pub fn new(size: usize, initial_capacity: usize) -> Self {crates/amaru-kernel/src/ordered_redeemer.rs (1)
18-30: Nicely crafted ordering implementation.Crikey, this is some solid work! The ordering logic—tag first, then index—matches the Haskell ledger implementation, which is crucial for protocol compatibility. The use of
Cow<'a, Redeemer>is a smart move, like choosing the right tool for the job in your RPG loadout.One small style note: the match on
Ordering::Equalcould be slightly simplified:🔎 Optional simplification
impl Ord for OrderedRedeemer<'_> { fn cmp(&self, other: &Self) -> Ordering { - match self.tag.as_index().cmp(&other.tag.as_index()) { - by_tag @ Ordering::Less | by_tag @ Ordering::Greater => by_tag, - Ordering::Equal => self.index.cmp(&other.index), - } + self.tag + .as_index() + .cmp(&other.tag.as_index()) + .then_with(|| self.index.cmp(&other.index)) } }This mirrors the pattern used elsewhere in the codebase (e.g.,
ProposalIdAdapter::cmpincrates/amaru-kernel/src/lib.rslines 337-342).crates/amaru-plutus/src/script_context/v2.rs (1)
22-22: Unused import:Address.Mate, looks like you've imported
Addressfromamaru_kernelbut it's not actually used anywhere in this file. It's like buying a game on Steam during a sale and never installing it—we've all been there, but best to clean it up!🔎 Suggested fix
-use amaru_kernel::{Address, StakePayload}; +use amaru_kernel::StakePayload;crates/amaru-plutus/src/script_context/v3.rs (1)
520-532: TODO needs follow-up: CostModels encoding validation.G'day! The TODO comment here is a fair dinkum heads-up that this encoding is a "best guess" and needs validation against the Haskell implementation. Like trying to speedrun a game without a proper guide—might work, might not!
Would recommend creating a tracking issue to verify this against the Cardano Haskell ledger spec before shipping to production. The structure (constructor 0 with three optional cost model vectors) looks plausible, but CostModels encoding can be tricky.
Do you want me to open an issue to track the validation of this CostModels encoding against the Haskell implementation?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.cargo/deny.toml.github/workflows/continuous-integration.ymlCargo.tomlcrates/amaru-kernel/Cargo.tomlcrates/amaru-kernel/src/arena_pool.rscrates/amaru-kernel/src/lib.rscrates/amaru-kernel/src/ordered_redeemer.rscrates/amaru-ledger/Cargo.tomlcrates/amaru-ledger/src/block_validator.rscrates/amaru-ledger/src/rules.rscrates/amaru-ledger/src/rules/block.rscrates/amaru-ledger/src/rules/transaction/mod.rscrates/amaru-ledger/src/rules/transaction/phase_one/certificates.rscrates/amaru-ledger/src/rules/transaction/phase_one/collateral.rscrates/amaru-ledger/src/rules/transaction/phase_one/fees.rscrates/amaru-ledger/src/rules/transaction/phase_one/inputs.rscrates/amaru-ledger/src/rules/transaction/phase_one/metadata.rscrates/amaru-ledger/src/rules/transaction/phase_one/mint.rscrates/amaru-ledger/src/rules/transaction/phase_one/mod.rscrates/amaru-ledger/src/rules/transaction/phase_one/outputs.rscrates/amaru-ledger/src/rules/transaction/phase_one/outputs/inherent_value.rscrates/amaru-ledger/src/rules/transaction/phase_one/proposals.rscrates/amaru-ledger/src/rules/transaction/phase_one/scripts.rscrates/amaru-ledger/src/rules/transaction/phase_one/vkey_witness.rscrates/amaru-ledger/src/rules/transaction/phase_one/voting_procedures.rscrates/amaru-ledger/src/rules/transaction/phase_one/withdrawals.rscrates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-ledger/src/state.rscrates/amaru-ledger/tests/evaluate_ledger_states.rscrates/amaru-plutus/src/script_context/mod.rscrates/amaru-plutus/src/script_context/v1.rscrates/amaru-plutus/src/script_context/v2.rscrates/amaru-plutus/src/script_context/v3.rscrates/amaru-plutus/src/to_plutus_data.rscrates/amaru/src/bin/amaru/cmd/run.rscrates/amaru/src/bin/ledger/cmd/mod.rscrates/amaru/src/stages/mod.rsexamples/shared/src/lib.rs
✅ Files skipped from review due to trivial changes (2)
- crates/amaru-ledger/src/rules/transaction/phase_one/scripts.rs
- crates/amaru-ledger/src/rules/block.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- Cargo.toml
- crates/amaru-kernel/Cargo.toml
- crates/amaru-ledger/src/rules.rs
- crates/amaru-ledger/Cargo.toml
- .cargo/deny.toml
- crates/amaru/src/bin/amaru/cmd/run.rs
- crates/amaru-ledger/src/rules/transaction/mod.rs
- crates/amaru/src/bin/ledger/cmd/mod.rs
- crates/amaru-ledger/src/rules/transaction/phase_one/outputs.rs
🧰 Additional context used
🧠 Learnings (21)
📚 Learning: 2025-08-08T14:46:53.013Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/pool_params.rs:107-116
Timestamp: 2025-08-08T14:46:53.013Z
Learning: In crates/amaru-kernel/src/pool_params.rs, when serializing Relay::SingleHostAddr IPv6 to text, the project intentionally reverses each 4-byte chunk before constructing std::net::Ipv6Addr. This matches cardano-ledger’s IPv6 representation (four little-endian Word32 chunks). Do not “simplify” by passing the raw 16 bytes directly to Ipv6Addr::from; that would break ledger compatibility.
Applied to files:
crates/amaru-plutus/src/script_context/v1.rsexamples/shared/src/lib.rscrates/amaru/src/stages/mod.rscrates/amaru-ledger/src/state.rs
📚 Learning: 2025-01-31T09:19:55.491Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 64
File: crates/amaru/src/ledger/state/transaction.rs:62-68
Timestamp: 2025-01-31T09:19:55.491Z
Learning: In the Cardano ledger, withdrawal uniqueness is guaranteed by the on-wire format's serialization, which considers duplicate withdrawals as invalid. This guarantee is reflected in Rust's type system, making additional runtime uniqueness checks unnecessary even when using vectors internally, as the data originates from minted blocks.
Applied to files:
crates/amaru-plutus/src/script_context/v1.rs
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru-plutus/src/script_context/v1.rscrates/amaru-kernel/src/arena_pool.rsexamples/shared/src/lib.rscrates/amaru-plutus/src/script_context/v2.rscrates/amaru/src/stages/mod.rscrates/amaru-ledger/src/state.rscrates/amaru-ledger/src/rules/transaction/phase_one/mod.rscrates/amaru-kernel/src/ordered_redeemer.rscrates/amaru-plutus/src/script_context/v3.rscrates/amaru-ledger/tests/evaluate_ledger_states.rscrates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-ledger/src/block_validator.rscrates/amaru-plutus/src/to_plutus_data.rscrates/amaru-kernel/src/lib.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-08-11T06:17:56.499Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 372
File: crates/amaru-consensus/src/consensus/headers_tree.rs:612-614
Timestamp: 2025-08-11T06:17:56.499Z
Learning: The `indextree::Arena<T>` struct in version 4.7.4 has a `count()` method that returns the total number of nodes in the arena, including removed nodes. This is a valid method to call on Arena instances in Rust code using the indextree crate.
Applied to files:
crates/amaru-kernel/src/arena_pool.rs
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 387
File: crates/amaru-stores/src/lib.rs:40-40
Timestamp: 2025-08-20T13:02:25.763Z
Learning: In the amaru-stores crate, amaru_slot_arithmetic types like Epoch and EraHistory are used throughout the main crate code in modules like in_memory/mod.rs, rocksdb/consensus.rs, and rocksdb/ledger/columns/, not just in tests. This means amaru-slot-arithmetic should be a regular dependency, not a dev-dependency.
Applied to files:
crates/amaru/src/stages/mod.rscrates/amaru-ledger/src/state.rscrates/amaru-ledger/tests/evaluate_ledger_states.rscrates/amaru-ledger/src/block_validator.rscrates/amaru-kernel/src/lib.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-09-29T16:44:14.807Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/protocol.rs:94-106
Timestamp: 2025-09-29T16:44:14.807Z
Learning: In the amaru-network crate protocol.rs file, the correct Cardano mini-protocol ID assignments are: PROTO_N2N_KEEP_ALIVE = 8 and PROTO_N2N_PEER_SHARE = 10, as verified against the network specification by the maintainer.
Applied to files:
crates/amaru-ledger/src/state.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-01-21T15:32:17.911Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 69
File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117
Timestamp: 2025-01-21T15:32:17.911Z
Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.
Applied to files:
crates/amaru-plutus/src/script_context/v3.rscrates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-05-12T14:21:27.470Z
Learnt from: stevana
Repo: pragma-org/amaru PR: 210
File: simulation/amaru-sim/src/simulator/simulate.rs:264-277
Timestamp: 2025-05-12T14:21:27.470Z
Learning: The team plans to replace the out-of-process test in `simulation/amaru-sim/src/simulator/simulate.rs` with an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (`../../target/debug/echo`) and making tests more reliable.
Applied to files:
crates/amaru-ledger/tests/evaluate_ledger_states.rs
📚 Learning: 2025-04-20T17:57:23.233Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:272-285
Timestamp: 2025-04-20T17:57:23.233Z
Learning: In test infrastructure code, rkuhn prefers explicit panics (using .unwrap() or similar) over returning Result types, as test failures should be immediate and obvious.
Applied to files:
crates/amaru-ledger/tests/evaluate_ledger_states.rscrates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-kernel/src/lib.rs
📚 Learning: 2025-08-08T14:39:50.527Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/borrowed_datum.rs:32-39
Timestamp: 2025-08-08T14:39:50.527Z
Learning: In the amaru project, when converting BorrowedDatumOption::Data to an owned DatumOption in crates/amaru-kernel/src/borrowed_datum.rs, the call `.unwrap()` refers to pallas’s KeepRaw::unwrap, which is infallible (always returns the inner value) and is not a panic risk. Future reviews should not flag this unwrap as dangerous.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-04-20T18:02:25.073Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward.rs:73-75
Timestamp: 2025-04-20T18:02:25.073Z
Learning: In the current development stage, rkuhn prefers using explicit panics (via `.expect()` or `.unwrap()`) for fatal errors in the application code that would tear down the node, rather than propagating errors with `Result`. The intention is to eventually transition to proper error handling with `Result` as the codebase matures.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-12-28T19:26:35.354Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 612
File: crates/amaru-protocols/src/blockfetch/mod.rs:151-173
Timestamp: 2025-12-28T19:26:35.354Z
Learning: In crates/amaru-protocols/src/blockfetch/mod.rs, the blockfetch initiator uses .expect() when popping from the request queue on NoBlocks and Done results. These are intentional fail-fast assertions: the protocol state machine guarantees the queue is non-empty when these messages arrive, so an empty queue indicates a protocol violation. A misbehaving peer triggers an erroneous protocol transition that will close the connection (supervision to be implemented in a future PR). This follows the project's fail-fast philosophy for protocol invariants.
<!--
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-ledger/src/block_validator.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-09-06T09:16:25.025Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 435
File: crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs:113-116
Timestamp: 2025-09-06T09:16:25.025Z
Learning: In cardano-node serialized ledger state CBOR encoding, indefinite-length structures may be terminated with 0xFF "break" markers. The current code in convert_ledger_state.rs unconditionally strips the last byte (bytes[p..bytes.len() - 1]), which could corrupt data if the trailing byte is not actually a CBOR break marker.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-08-12T12:28:24.027Z
Learnt from: etorreborre
Repo: pragma-org/amaru PR: 372
File: simulation/amaru-sim/src/simulator/mod.rs:410-412
Timestamp: 2025-08-12T12:28:24.027Z
Learning: In the Amaru project, panic statements are acceptable in simulation/test code (like amaru-sim crate) as they help identify configuration issues quickly during development, rather than needing proper error handling like production code.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-08-08T14:34:06.105Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/minicbor-extra/src/lib.rs:50-55
Timestamp: 2025-08-08T14:34:06.105Z
Learning: Project uses Rust 1.88 stable; using Result::is_err_and is acceptable and should not be flagged as unstable. In particular, crates/minicbor-extra/src/lib.rs relies on is_err_and when checking end_of_input; future reviews should not suggest replacing it for stability reasons.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-plutus/src/script_context/mod.rs
📚 Learning: 2025-02-03T11:15:22.640Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 75
File: crates/amaru/src/consensus/mod.rs:164-165
Timestamp: 2025-02-03T11:15:22.640Z
Learning: In the Amaru project, chain selection operations (roll_forward and rollback) should use separate result types to leverage the type system for preventing impossible states, rather than using runtime checks or panics.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-08-08T14:35:35.562Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/transaction_pointer.rs:36-44
Timestamp: 2025-08-08T14:35:35.562Z
Learning: In the amaru project, when decoding CBOR arrays, prefer using minicbor_extra::heterogenous_array with the expected length to validate definite-length arrays and correctly handle indefinite-length arrays. Example: crates/amaru-kernel/src/transaction_pointer.rs Decode should use heterogenous_array(d, 2, …) instead of ignoring the length from d.array().
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-04-04T16:49:53.462Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 0
File: :0-0
Timestamp: 2025-04-04T16:49:53.462Z
Learning: The ledger validation architecture abstracts state management from validations using traits like UtxoStateManagement, PoolsStateManagement, AccountsStateManagement, etc. to keep validation rules lean and independent of storage concerns.
Applied to files:
crates/amaru-ledger/src/block_validator.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In crates/amaru-consensus, flood prevention for duplicate invalid headers should be implemented in the validate_header stage or database layer, not in the store_header stage, since store_header runs before validation and cannot determine header validity.
Applied to files:
crates/amaru-ledger/src/block_validator.rs
📚 Learning: 2025-06-14T16:36:04.502Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 263
File: crates/pure-stage/src/simulation/state.rs:33-36
Timestamp: 2025-06-14T16:36:04.502Z
Learning: In simulation and replay systems that require cloneable and serializable states, error types must often be converted to String rather than stored as trait objects (like Box<dyn Error> or anyhow::Error) because trait objects cannot be cloned, which breaks the snapshotting and replay functionality needed for deterministic simulation.
Applied to files:
crates/amaru-plutus/src/to_plutus_data.rs
📚 Learning: 2025-09-29T16:39:24.001Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/mux.rs:317-325
Timestamp: 2025-09-29T16:39:24.001Z
Learning: In crates/amaru-network/src/mux.rs, the outgoing() method intentionally uses unwrap() after get_mut(&proto_id) as a fail-fast mechanism. This panic is designed to catch programming errors where an actor tries to send on an unregistered protocol, and should not be changed to return a Result since it represents internal code bugs that should terminate the process, not external input that should be handled gracefully.
Applied to files:
crates/amaru-plutus/src/script_context/mod.rs
🧬 Code graph analysis (8)
crates/amaru-plutus/src/script_context/v1.rs (2)
crates/amaru-plutus/src/script_context/v3.rs (7)
to_plutus_data(37-46)to_plutus_data(50-59)to_plutus_data(63-85)to_plutus_data(89-98)to_plutus_data(102-111)to_plutus_data(115-117)to_plutus_data(121-123)crates/amaru-plutus/src/script_context/v2.rs (8)
to_plutus_data(26-28)to_plutus_data(32-51)to_plutus_data(59-68)to_plutus_data(72-74)to_plutus_data(78-80)to_plutus_data(84-87)to_plutus_data(95-100)to_plutus_data(104-106)
crates/amaru-kernel/src/arena_pool.rs (1)
crates/amaru-ledger/src/block_validator.rs (1)
new(41-54)
crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs (2)
crates/amaru-ledger/src/state.rs (2)
network(224-226)protocol_parameters(232-234)crates/amaru-ledger/src/rules/transaction/phase_one/collateral.rs (1)
is_valid(94-96)
crates/amaru-kernel/src/ordered_redeemer.rs (1)
crates/amaru-kernel/src/lib.rs (2)
cmp(338-343)partial_cmp(332-334)
crates/amaru-plutus/src/script_context/v3.rs (4)
crates/amaru-plutus/src/script_context/mod.rs (2)
to_plutus_data(1098-1110)script_hash(823-830)crates/amaru-plutus/src/script_context/v2.rs (2)
to_plutus_data(26-28)to_plutus_data(32-51)crates/amaru-kernel/src/lib.rs (7)
stake_credential(767-767)stake_credential(771-780)script_hash(857-857)script_hash(861-868)script_hash(872-874)script_hash(878-880)script_hash(888-890)crates/amaru-plutus/src/to_plutus_data.rs (1)
None(154-154)
crates/amaru-ledger/tests/evaluate_ledger_states.rs (2)
crates/amaru-ledger/src/state.rs (1)
network(224-226)crates/amaru-ledger/src/rules/block.rs (1)
execute(174-296)
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (3)
crates/amaru-plutus/src/script_context/v3.rs (2)
to_plutus_data(37-46)to_plutus_data(50-59)crates/amaru-plutus/src/script_context/mod.rs (1)
to_plutus_data(1098-1110)crates/amaru-plutus/src/script_context/v1.rs (13)
to_plutus_data(29-31)to_plutus_data(40-42)to_plutus_data(46-70)to_plutus_data(85-103)to_plutus_data(114-127)to_plutus_data(135-140)to_plutus_data(144-146)to_plutus_data(153-155)to_plutus_data(188-222)to_plutus_data(227-239)to_plutus_data(250-261)to_plutus_data(265-267)to_plutus_data(271-273)
crates/amaru-ledger/src/block_validator.rs (2)
crates/amaru-ledger/src/state.rs (5)
network(224-226)protocol_parameters(232-234)new(146-169)new(1062-1072)era_history(228-230)crates/amaru-kernel/src/arena_pool.rs (1)
new(43-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Examples (ledger-in-nodejs)
- GitHub Check: Build aarch64/linux
- GitHub Check: Build x86_64/linux
- GitHub Check: Build wasm32
- GitHub Check: Test coverage
- GitHub Check: clippy
- GitHub Check: Analyze (rust)
🔇 Additional comments (51)
crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs (4)
59-86: LGTM on the rename, mate!The
InvalidTransaction→PhaseOneErrorrename makes perfect sense now that you've got a proper two-phase validation split. Clear naming like this is the kind of thing that makes navigating a codebase feel less like a Dark Souls boss fight and more like a pleasant stroll through the Shire.
155-155: Network conversion pattern looks spot on.The
&(*network).into()conversion pattern aligns with the broader shift toNetworkNamein the public API. This is consistent with the usage shown in the relevant snippets fromcrates/amaru-ledger/src/rules/block.rs.Also applies to: 180-180
220-229: Nice work on the explicit input consumption!Computing and returning
consumed_inputsbased onis_validis much clearer than relying on side effects buried in the context. This makes the resource lifecycle transparent—like having a proper inventory system instead of just hoping your gear magically appears in your backpack.
88-100: All call sites are properly handling the new return type.Searched the codebase and found two call sites. In
crates/amaru-ledger/src/rules/block.rs, the code captures theVec<TransactionInput>in theconsumed_inputsvariable and uses it downstream—proper stuff. Over in the test fileevaluate_ledger_states.rs, the return value is explicitly ignored using the_pattern in the match arm, which is the right move for a test that only cares about success/failure. Like a good save in a speedrun, the return type change has been integrated cleanly across the board.crates/amaru-kernel/src/arena_pool.rs (3)
22-55: Solid pre-allocation strategy, champ!Creating all arenas upfront (lines 44-46) addresses the concerns from past reviews perfectly. This ensures:
- All memory is reserved at initialization (fail-fast if OOM)
- Deterministic runtime behavior (no surprise allocations mid-execution)
- Simpler logic (no dynamic arena creation)
The bounded pool with
Condvarfor blocking is exactly the right pattern here—like having a proper item queue instead of spinning in place spamming the interact button.
84-91: try_acquire implementation is clean.Non-blocking variant looks good. Same consideration about poisoned mutex handling applies here, but otherwise this is textbook stuff.
108-118: RAII drop implementation is correct.The
unsafe { ManuallyDrop::take(&mut self.arena) }is necessary and properly scoped (as discussed in past reviews). The reset-then-return-then-notify sequence ensures:
- Arena is cleared of allocations
- Arena goes back to the pool
- One waiting thread is woken up
This is the kind of careful resource management that makes Rust shine—like having a proper save system instead of just hoping you don't crash.
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (5)
82-84: Early exit for no-redeemer case is solid.Quick bail-out when there's no script work to do—nice and efficient. No point running the Phase Two gauntlet if there's no script execution needed.
139-139: Arena acquisition per-script is correct but worth noting.Each script evaluation acquires its own arena (line 139). This is the right call for isolation—if one script blows up the arena memory, it doesn't affect others. However, it does mean the pool size needs to accommodate concurrent script evaluations within a transaction.
Just making sure this is intentional and that the
ArenaPoolsize is configured with this per-script usage in mind (as seen in the example withArenaPool::new(10, 1_024_000)).Based on learnings, I should ask about the pool sizing strategy:
Do you have guidance on sizing the
ArenaPoolrelative to expected maximum scripts per transaction? With complex transactions potentially having multiple scripts, the pool needs enough capacity to avoid blocking during transaction validation.
235-242: Validity state logic handles invalid transactions correctly.The logic at lines 235-242 correctly inverts the result for invalid transactions:
- If
is_validis true: script_results must be Ok, else error- If
is_validis false: script failures are expected and return OkThis is the kind of dual-state handling that can be tricky, but you've nailed it. Like properly implementing a poison/buff system where the effect depends on the target's state.
183-192: Theexpecton PlutusData conversion could panic in production.Lines 188-189 use
.expect("unable to decode PlutusData cbor")which will panic if the round-trip conversion fails. Given the TODO comment acknowledges this is a "really bad conversion" between Pallas and uplc PlutusData types, there's a risk this could blow up unexpectedly.Consider propagating the error instead of panicking—even if it "should never happen," defensive coding here would turn a potential runtime crash into a graceful
PhaseTwoError.🔎 Suggested approach
Add a new variant to
PhaseTwoError:#[derive(Debug, Error)] pub enum PhaseTwoError { // ... existing variants ... + #[error("PlutusData round-trip conversion failed")] + PlutusDataConversionError, }Then replace the expect with proper error handling:
let constants = args .iter() .map(|arg| { let bytes = to_cbor(&arg); - #[allow(clippy::expect_used)] - let data = PlutusData::from_cbor(&arena, &bytes) - .expect("unable to decode PlutusData cbor"); - Constant::Data(data) + let data = PlutusData::from_cbor(&arena, &bytes) + .map_err(|_| PhaseTwoError::PlutusDataConversionError)?; + Ok(Constant::Data(data)) }) - .collect::<Vec<_>>(); + .collect::<Result<Vec<_>, PhaseTwoError>>()?;⛔ Skipped due to learnings
Learnt from: KtorZ Repo: pragma-org/amaru PR: 370 File: crates/amaru-kernel/src/borrowed_datum.rs:32-39 Timestamp: 2025-08-08T14:39:50.527Z Learning: In the amaru project, when converting BorrowedDatumOption::Data to an owned DatumOption in crates/amaru-kernel/src/borrowed_datum.rs, the call `.unwrap()` refers to pallas’s KeepRaw::unwrap, which is infallible (always returns the inner value) and is not a panic risk. Future reviews should not flag this unwrap as dangerous.Learnt from: rkuhn Repo: pragma-org/amaru PR: 149 File: crates/amaru/src/stages/consensus/chain_forward.rs:73-75 Timestamp: 2025-04-20T18:02:25.073Z Learning: In the current development stage, rkuhn prefers using explicit panics (via `.expect()` or `.unwrap()`) for fatal errors in the application code that would tear down the node, rather than propagating errors with `Result`. The intention is to eventually transition to proper error handling with `Result` as the codebase matures.Learnt from: jeluard Repo: pragma-org/amaru PR: 69 File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117 Timestamp: 2025-01-21T15:32:17.911Z Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.Learnt from: rkuhn Repo: pragma-org/amaru PR: 149 File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:272-285 Timestamp: 2025-04-20T17:57:23.233Z Learning: In test infrastructure code, rkuhn prefers explicit panics (using .unwrap() or similar) over returning Result types, as test failures should be immediate and obvious.Learnt from: abailly Repo: pragma-org/amaru PR: 435 File: crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs:113-116 Timestamp: 2025-09-06T09:16:25.025Z Learning: In cardano-node serialized ledger state CBOR encoding, indefinite-length structures may be terminated with 0xFF "break" markers. The current code in convert_ledger_state.rs unconditionally strips the last byte (bytes[p..bytes.len() - 1]), which could corrupt data if the trailing byte is not actually a CBOR break marker.Learnt from: KtorZ Repo: pragma-org/amaru PR: 370 File: crates/minicbor-extra/src/lib.rs:50-55 Timestamp: 2025-08-08T14:34:06.105Z Learning: Project uses Rust 1.88 stable; using Result::is_err_and is acceptable and should not be flagged as unstable. In particular, crates/minicbor-extra/src/lib.rs relies on is_err_and when checking end_of_input; future reviews should not suggest replacing it for stability reasons.Learnt from: KtorZ Repo: pragma-org/amaru PR: 370 File: crates/amaru-kernel/src/transaction_pointer.rs:36-44 Timestamp: 2025-08-08T14:35:35.562Z Learning: In the amaru project, when decoding CBOR arrays, prefer using minicbor_extra::heterogenous_array with the expected length to validate definite-length arrays and correctly handle indefinite-length arrays. Example: crates/amaru-kernel/src/transaction_pointer.rs Decode should use heterogenous_array(d, 2, …) instead of ignoring the length from d.array().Learnt from: etorreborre Repo: pragma-org/amaru PR: 372 File: simulation/amaru-sim/src/simulator/mod.rs:410-412 Timestamp: 2025-08-12T12:28:24.027Z Learning: In the Amaru project, panic statements are acceptable in simulation/test code (like amaru-sim crate) as they help identify configuration issues quickly during development, rather than needing proper error handling like production code.Learnt from: rkuhn Repo: pragma-org/amaru PR: 612 File: crates/amaru-protocols/src/blockfetch/mod.rs:151-173 Timestamp: 2025-12-28T19:26:35.354Z Learning: In crates/amaru-protocols/src/blockfetch/mod.rs, the blockfetch initiator uses .expect() when popping from the request queue on NoBlocks and Done results. These are intentional fail-fast assertions: the protocol state machine guarantees the queue is non-empty when these messages arrive, so an empty queue indicates a protocol violation. A misbehaving peer triggers an erroneous protocol transition that will close the connection (supervision to be implemented in a future PR). This follows the project's fail-fast philosophy for protocol invariants. <!--Learnt from: abailly Repo: pragma-org/amaru PR: 75 File: crates/amaru/src/consensus/mod.rs:164-165 Timestamp: 2025-02-03T11:15:22.640Z Learning: In the Amaru project, chain selection operations (roll_forward and rollback) should use separate result types to leverage the type system for preventing impossible states, rather than using runtime checks or panics.
141-143: Unreachable check is solid, mate. Phase One'spartition_scripts()function never flags Native scripts as requiring redeemers (line 161 in scripts.rs is a no-op), while Plutus variants do (lines 164-176). The subsequent redeemer validation ensures provided redeemers match required ones exactly, so a Native script with a redeemer would've been rejected as extraneous earlier. The invariant holds—this is like a well-written plot where loose ends are tied up by Act Two.crates/amaru-ledger/tests/evaluate_ledger_states.rs (1)
215-226: Phase One migration looks clean.The update from
transaction::executetotransaction::phase_one::executewithNetworkName::Preprodand the new result patternOk(_)all align with the API changes. Assuming the network type is correct (see previous comment), this is solid.examples/shared/src/lib.rs (2)
46-46: ArenaPool sizing looks reasonable for an example.Creating an
ArenaPoolwith 10 arenas of 1MB each is a solid starting point for demonstration purposes. That's enough to handle multiple concurrent script evaluations without blocking.For production, you'll probably want to tune this based on actual workload (number of scripts per block, concurrency, etc.)—but for an example, this is spot-on.
110-122: validate_block parameter order updated correctly.The
&arena_poolnow comes before&networkin the call tovalidate_block(lines 112-113), matching the updated signature. The arena is properly instantiated and passed through—like properly equipping your gear before heading into a dungeon.crates/amaru-ledger/src/block_validator.rs (2)
40-54: LGTM! Clean integration of the arena pool.Nice one! The constructor properly accepts the
vm_eval_pooland stores it, making it available for block validation. This is like setting up your inventory before heading into the dungeon—everything's in place for when you need it.
70-83: Solid wiring of the arena pool through to state validation.The pool reference is correctly passed down to
state.roll_forward, enabling arena-based script evaluation during block processing. The error handling remains unchanged and appropriate. Top shelf work, mate!crates/amaru-ledger/src/state.rs (2)
35-42: Imports look tidy.The
ArenaPoolimport is properly grouped with other amaru-kernel types. Clean as a whistle, mate!
603-627: Clean integration of ArenaPool into the validation path.Fair dinkum work here! The
arena_poolparameter is correctly threaded through tovalidate_block.Regarding your earlier comment about drilling the arena_pool through many methods—yeah, it's a bit like passing the One Ring through Middle-earth, but sometimes the straightforward path is the right one. The pool doesn't fit cleanly as "state" or "validation context" since it's more of a shared resource for VM execution. If this becomes unwieldy later, you could consider a dedicated "execution environment" struct that bundles arena pool with other VM-related resources.
crates/amaru-kernel/src/ordered_redeemer.rs (1)
38-62: LGTM on Deref and From impls.The
DereftoRedeemerallows transparent field access, and the trio ofFromimplementations cover all the common use cases—owned, borrowed, and Cow. Very ergonomic, like a well-designed game menu!crates/amaru-plutus/src/script_context/v1.rs (4)
130-147: Clean StakeAddress serialization for PlutusV1.The implementation correctly wraps the stake credential in the V1/V2
Constr(0, [cred])envelope as documented. The comment aboutPointerbeing impossible by ledger serialization logic is a helpful note for future maintainers. Good stuff, mate!
158-223: Thorough certificate handling with excellent docs.Ripper work on the documentation here! The translation of Conway-era
Reg/UnRegto the oldStakeRegistration/StakeDeregistrationrepresentation for V1/V2 compatibility is spot on. The explicit error handling for unsupported certificate types protects against silent failures.The
deref()approach at line 189 cleanly unwraps the certificate for pattern matching. Like reading the manual before playing a complex game—you've covered all the edge cases!
264-268: Simplified Withdrawals serialization.The move to a direct vector path for withdrawals is cleaner. Collect into a Vec, serialize—no intermediate mapping shenanigans needed.
298-355: Tests properly updated for protocol version awareness.The test now correctly passes
PROTOCOL_VERSION_10toTxInfo::newand usesredeemer.deref()for theScriptContextconstruction. These tests will catch regressions in the V1 script context encoding—like having a save point before a boss fight!crates/amaru/src/stages/mod.rs (3)
98-107: Good config tunability for arena pools.Nice addition of these config fields! The comments clearly explain what each parameter controls. Having these configurable rather than hardcoded is the right call—like having a settings menu in your game for different hardware profiles.
338-369: Clean wiring of ArenaPool through ledger creation.Beaut! The
vm_eval_poolis constructed once and passed toBlockValidator::newfor both InMem and RocksDB paths. This ensures consistent arena management regardless of storage backend.
121-123: Solid starting point for the arena defaults.The 1MB allocation aligns nicely with what the community reports for typical Plutus script evaluation—most scripts sit well under 1,000,000 memory units. A single arena is definitely the "safe mode" choice here, yeah; you're not gonna blow the memory budget on a typical tx, but you're also not gonna set the world on fire with parallel throughput either. It's like starting a new game on normal difficulty instead of hard—sensible move for Phase 2 shakedown. The arena's intentional memory leaking means you'll want to revisit these numbers once you've got real profiling data, but as a baseline? Spot on.
crates/amaru-kernel/src/lib.rs (3)
124-126: LGTM on ordered_redeemer module export.Clean module addition and re-export. The
pub use ordered_redeemer::*makes the type ergonomically accessible throughout the crate.
184-186: LGTM on arena_pool module export.Same pattern as above—clean addition. The ArenaPool type is now part of the public API, ready for use in ledger validation.
822-844: Comprehensive HasOwnership impl for Certificate.Solid implementation covering all certificate variants! The match arms correctly extract stake credentials from delegation-style certs and map pool IDs to
AddrKeyhashfor registration/retirement. This enables ownership-based lookups across the full certificate taxonomy.One thought: the
clone()on stake_credential (line 839) is necessary since we're returning an owned value, but it's worth noting this is a small allocation for each call.crates/amaru-plutus/src/to_plutus_data.rs (2)
27-44: Clean error type with thiserror.The
PlutusDataErrorenum is well-structured with theUnsupportedVersionvariant carrying both message and version number. The helper constructorunsupported_versionis a nice ergonomic touch. Proper error handling, mate!
115-118: Trait bound addition for StakeAddress.The added bound
amaru_kernel::StakeAddress: ToPlutusData<V>at line 118 ensures the version-specific implementation is available. This is necessary since different Plutus versions encode StakeAddress differently (V1/V2 wrap in an extraConstr(0, ...)layer).crates/amaru-plutus/src/script_context/v2.rs (2)
83-88: LGTM!The Withdrawals serialization now materializes the inner map via
.iter().collect::<BTreeMap<_, _>>()before delegating. This ensures a consistent reference-based map for the ToPlutusData trait. Solid work, like a well-crafted side quest!
90-107: LGTM!The
ToPlutusData<2>implementations forStakeAddressare spot on. The comment explaining the V1/V2 enum structure (withPointerbeing impossible to construct via ledger serialization) is a nice touch—like getting those helpful loading screen tips in a game! The nestedconstr_v2!calls correctly encode the Credential variant with the appropriate inner constructor for key vs. script hashes.crates/amaru-plutus/src/script_context/v3.rs (6)
15-30: LGTM!Imports are well-organized and consistent with the new
Certificate,CostModels, andStakeAddresshandling. Good on ya for keeping things tidy!
198-275: LGTM!The
Certificateserialization now correctly handles the protocol version 9 bug where deposit values were omitted. The conditional logic (protocol_version.0 > 9) is clean and matches the documented behavior. It's like patching a game-breaking bug while maintaining backward compatibility—nice work, mate!
449-454: LGTM!CostModels serialization at index 18 is now wired up properly. The TODO has been addressed—always satisfying to check off that quest log!
609-619: LGTM!The
DatumsandVotesserialization impls are clean delegations. Simple and effective—like a well-optimized game engine!
621-634: LGTM!The
ToPlutusData<3>forStakeAddressis nicely simplified compared to V1/V2—no nested Credential enum, just direct constructors for key (0) and script (1). Like upgrading from an old-gen console to the new one—cleaner and more streamlined!
636-716: LGTM!Tests are properly updated to use the new
Redeemers::iter_from()andPROTOCOL_VERSION_10wiring. The commented-out test for protocol version 9 with an explanation is a thoughtful touch—like leaving a note for future players about an optional boss fight they can skip!crates/amaru-plutus/src/script_context/mod.rs (10)
15-35: LGTM!Imports are comprehensive and well-organized. The aliasing of
Certificate as PallasCertificatehelps disambiguate from the wrapper type nicely—like giving NPCs distinct names so you don't get confused in dialogue!
64-116: LGTM!The
ScriptContext::newandbudget()implementations are solid. The datum lookup logic forSpendredeemers (checkingDatumOption::Hashagainsttx_info.data) is well-structured. Nice addition of thebudget()accessor—handy for script execution resource tracking!
152-170: LGTM!The
TxInfostructural changes are well-designed. Thescript_tableaddition for mapping redeemers to scripts is a key piece for Phase 2 validation—like having an inventory system that actually shows what each item does!
196-361: LGTM!The
TxInfo::newconstruction is comprehensive. The script collection from both UTxO reference scripts and witness sets ensures all scripts are available for lookup. Theprotocol_versionintegration forCertificatewrapping is clean.One note: the mutable
script_tablepassed toScriptPurpose::builderfollows a side-effect pattern for populating the map. It works, but could be considered for refactoring to a more functional approach in the future if clarity becomes a concern.
363-372: LGTM!The
to_script_contexts()method is a clean abstraction for producing all script contexts paired with their scripts. This is the key entry point for Phase 2 validation—like the "Start Mission" button that kicks off all the action!
374-394: LGTM!The
translate_inputsextension to populate the scripts map from reference scripts is well-implemented. Clean side-effect handling with the mutable reference—efficient and clear!
411-508: LGTM!The
ScriptPurpose::builderrefactor is comprehensive. Each redeemer tag correctly:
- Checks for script-based credentials (returning
Nonefor key-based)- Looks up the script from the scripts map
- Populates the
script_tablefor later evaluationThe
Proposebranch handling for guardrail scripts in governance actions is particularly nice—covering all the edge cases like a completionist going for 100%!
963-977: LGTM!The
Certificate<'a>wrapper cleanly encapsulates the protocol version context alongside the certificate reference. TheDerefimpl makes it transparent to use—ergonomic design like a well-crafted controller!
1076-1129: LGTM!The
Redeemers<'a>abstraction is solid:
- Clean wrapper around the
BTreeMapToPlutusDataimpl correctly maps to(ScriptPurpose, Redeemer)pairsiter_fromhandles bothListandMapredeemer formats from PallasThe
iter_fromhandling of both formats is particularly important for compatibility—like supporting both controller and keyboard inputs!
1373-1376: LGTM!Test imports updated for proptest usage. Clean and functional!
.github/workflows/continuous-integration.yml (1)
67-68: Nice one! That comment clears things up beautifully. 🎯You've nailed the explanation for why this RISC-V toolchain dance is necessary. Future devs (including yourself after a long weekend) will appreciate understanding why uplc-turbo needs this specific clang compiler to build. Solid work addressing KtorZ's feedback!
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs:
- Around line 208-230: The match over result.term needs PlutusVersion-specific
handling: when plutus_version == PlutusVersion::V3 only accept Term::Constant
wrapping BuiltinUnit as success and return an UplcMachineError otherwise; for
other versions keep the current acceptance of any non-Error Term variants.
Update the branch that handles Ok(term) (examining result.term) to first check
plutus_version and validate Term::Constant(BuiltinUnit) for V3, returning
PhaseTwoError::UplcMachineError (using UplcMachineError { plutus_version, info:
result.info, err: ... }) on failure, while preserving the existing Ok(()) path
for V1/V2.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-plutus/src/to_plutus_data.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/amaru-plutus/src/to_plutus_data.rs
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-01-21T15:32:17.911Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 69
File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117
Timestamp: 2025-01-21T15:32:17.911Z
Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-08-08T14:39:50.527Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/borrowed_datum.rs:32-39
Timestamp: 2025-08-08T14:39:50.527Z
Learning: In the amaru project, when converting BorrowedDatumOption::Data to an owned DatumOption in crates/amaru-kernel/src/borrowed_datum.rs, the call `.unwrap()` refers to pallas’s KeepRaw::unwrap, which is infallible (always returns the inner value) and is not a panic risk. Future reviews should not flag this unwrap as dangerous.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-04-20T18:02:25.073Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward.rs:73-75
Timestamp: 2025-04-20T18:02:25.073Z
Learning: In the current development stage, rkuhn prefers using explicit panics (via `.expect()` or `.unwrap()`) for fatal errors in the application code that would tear down the node, rather than propagating errors with `Result`. The intention is to eventually transition to proper error handling with `Result` as the codebase matures.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-12-28T19:26:35.354Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 612
File: crates/amaru-protocols/src/blockfetch/mod.rs:151-173
Timestamp: 2025-12-28T19:26:35.354Z
Learning: In crates/amaru-protocols/src/blockfetch/mod.rs, the blockfetch initiator uses .expect() when popping from the request queue on NoBlocks and Done results. These are intentional fail-fast assertions: the protocol state machine guarantees the queue is non-empty when these messages arrive, so an empty queue indicates a protocol violation. A misbehaving peer triggers an erroneous protocol transition that will close the connection (supervision to be implemented in a future PR). This follows the project's fail-fast philosophy for protocol invariants.
<!--
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-09-06T09:16:25.025Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 435
File: crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs:113-116
Timestamp: 2025-09-06T09:16:25.025Z
Learning: In cardano-node serialized ledger state CBOR encoding, indefinite-length structures may be terminated with 0xFF "break" markers. The current code in convert_ledger_state.rs unconditionally strips the last byte (bytes[p..bytes.len() - 1]), which could corrupt data if the trailing byte is not actually a CBOR break marker.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-08-12T12:28:24.027Z
Learnt from: etorreborre
Repo: pragma-org/amaru PR: 372
File: simulation/amaru-sim/src/simulator/mod.rs:410-412
Timestamp: 2025-08-12T12:28:24.027Z
Learning: In the Amaru project, panic statements are acceptable in simulation/test code (like amaru-sim crate) as they help identify configuration issues quickly during development, rather than needing proper error handling like production code.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-08-08T14:34:06.105Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/minicbor-extra/src/lib.rs:50-55
Timestamp: 2025-08-08T14:34:06.105Z
Learning: Project uses Rust 1.88 stable; using Result::is_err_and is acceptable and should not be flagged as unstable. In particular, crates/minicbor-extra/src/lib.rs relies on is_err_and when checking end_of_input; future reviews should not suggest replacing it for stability reasons.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-02-03T11:15:22.640Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 75
File: crates/amaru/src/consensus/mod.rs:164-165
Timestamp: 2025-02-03T11:15:22.640Z
Learning: In the Amaru project, chain selection operations (roll_forward and rollback) should use separate result types to leverage the type system for preventing impossible states, rather than using runtime checks or panics.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-04-20T17:57:23.233Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:272-285
Timestamp: 2025-04-20T17:57:23.233Z
Learning: In test infrastructure code, rkuhn prefers explicit panics (using .unwrap() or similar) over returning Result types, as test failures should be immediate and obvious.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-08-08T14:35:35.562Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/transaction_pointer.rs:36-44
Timestamp: 2025-08-08T14:35:35.562Z
Learning: In the amaru project, when decoding CBOR arrays, prefer using minicbor_extra::heterogenous_array with the expected length to validate definite-length arrays and correctly handle indefinite-length arrays. Example: crates/amaru-kernel/src/transaction_pointer.rs Decode should use heterogenous_array(d, 2, …) instead of ignoring the length from d.array().
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
🧬 Code graph analysis (1)
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (2)
crates/amaru-plutus/src/script_context/mod.rs (4)
new(68-90)new(208-361)new(594-613)new(1079-1081)crates/amaru-ledger/src/rules/transaction/phase_one/inputs.rs (1)
execute(46-150)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: End-to-end snapshot tests (preview, 2, 680, true, 15, 10.5.3)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 182, 15, 10.5.3)
- GitHub Check: Build riscv32
- GitHub Check: Build x86_64/linux
- GitHub Check: clippy
- GitHub Check: Analyze (rust)
- GitHub Check: Build x86_64/windows
- GitHub Check: Test coverage
- GitHub Check: Build wasm32
- GitHub Check: Build aarch64/linux
- GitHub Check: Test the CLI
- GitHub Check: Build aarch64/macos
- GitHub Check: Benches
🔇 Additional comments (4)
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (4)
39-65: Solid error type hierarchy, mate!The PhaseTwoError enum and UplcMachineError struct cover all the bases—from missing inputs to translation failures to evaluation errors. Using thiserror keeps the error messages clean and the pragmatic String for the error message in UplcMachineError (instead of wrestling with lifetimes) is a sensible trade-off.
82-84: Early exit optimization looks good.Skipping phase two entirely when there's no redeemer is a nice performance win—like finding a shortcut in a Souls game that lets you skip an entire boss fight.
235-242: Validity state logic is spot on!The final check at lines 235-242 is correct: if
is_validis true, script failures are propagated; if false, script failures are expected and we returnOk(()). This matches the Cardano ledger spec where transactions can be marked invalid but still processed. Nice work!
142-142: Theunreachable!()is actually sound—phase_one guarantees native scripts can't have redeemers.Phase one's
partition_scripts()never adds native scripts to therequired_redeemerslist (see phase_one/scripts.rs), then strictly validates that any provided redeemers match that list exactly. If a redeemer points to a native script, phase_one rejects it as extraneous before phase_two ever runs. Sincescripts_to_executeonly contains scripts that passed validation, hittingScript::Nativehere is truly impossible—not just optimistic. This follows the project's fail-fast philosophy: the protocol state machine guarantees it, so the panic is justified.
…us v3 Signed-off-by: yHSJ <josh@securitybot.info>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (2)
59-65: The String in UplcMachineError sidesteps lifetime complexity but loses type safety.Fair play on the honest comment, mate—avoiding "lifetime hell" is relatable. But storing
err: Stringinstead of the properMachineErrormeans you're giving up structured error information and making downstream error handling a bit of a guessing game with string parsing.If you ever circle back to tackle those lifetimes (perhaps with an arena or
'staticvariant), you'd get better error introspection and debuggability. For now, it's a reasonable punt.
252-259: Theis_validflag logic is subtle but correct for Cardano's two-phase validation.This bit's a head-scratcher at first glance—when
is_validisfalse, we flip the logic: scripts should fail, so success is an error (ValidityStateError), and failure isOk(()). That's Cardano's collateral and two-phase semantics in action: invalid transactions are expected to have failing scripts.The logic is sound, just a bit counterintuitive. Might be worth a comment here for future maintainers who haven't spent time in Cardano's validation trenches.
📝 Optional clarifying comment
if is_valid { script_results } else { + // For invalid transactions (is_valid=false), we expect scripts to fail. + // If scripts succeed, that's a validation error; if they fail as expected, return Ok. match script_results { Ok(_) => Err(PhaseTwoError::ValidityStateError), Err(_) => Ok(()), } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-01-21T15:32:17.911Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 69
File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117
Timestamp: 2025-01-21T15:32:17.911Z
Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-08-08T14:39:50.527Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/borrowed_datum.rs:32-39
Timestamp: 2025-08-08T14:39:50.527Z
Learning: In the amaru project, when converting BorrowedDatumOption::Data to an owned DatumOption in crates/amaru-kernel/src/borrowed_datum.rs, the call `.unwrap()` refers to pallas’s KeepRaw::unwrap, which is infallible (always returns the inner value) and is not a panic risk. Future reviews should not flag this unwrap as dangerous.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-04-20T18:02:25.073Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward.rs:73-75
Timestamp: 2025-04-20T18:02:25.073Z
Learning: In the current development stage, rkuhn prefers using explicit panics (via `.expect()` or `.unwrap()`) for fatal errors in the application code that would tear down the node, rather than propagating errors with `Result`. The intention is to eventually transition to proper error handling with `Result` as the codebase matures.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-12-28T19:26:35.354Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 612
File: crates/amaru-protocols/src/blockfetch/mod.rs:151-173
Timestamp: 2025-12-28T19:26:35.354Z
Learning: In crates/amaru-protocols/src/blockfetch/mod.rs, the blockfetch initiator uses .expect() when popping from the request queue on NoBlocks and Done results. These are intentional fail-fast assertions: the protocol state machine guarantees the queue is non-empty when these messages arrive, so an empty queue indicates a protocol violation. A misbehaving peer triggers an erroneous protocol transition that will close the connection (supervision to be implemented in a future PR). This follows the project's fail-fast philosophy for protocol invariants.
<!--
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-09-06T09:16:25.025Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 435
File: crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs:113-116
Timestamp: 2025-09-06T09:16:25.025Z
Learning: In cardano-node serialized ledger state CBOR encoding, indefinite-length structures may be terminated with 0xFF "break" markers. The current code in convert_ledger_state.rs unconditionally strips the last byte (bytes[p..bytes.len() - 1]), which could corrupt data if the trailing byte is not actually a CBOR break marker.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-08-12T12:28:24.027Z
Learnt from: etorreborre
Repo: pragma-org/amaru PR: 372
File: simulation/amaru-sim/src/simulator/mod.rs:410-412
Timestamp: 2025-08-12T12:28:24.027Z
Learning: In the Amaru project, panic statements are acceptable in simulation/test code (like amaru-sim crate) as they help identify configuration issues quickly during development, rather than needing proper error handling like production code.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-02-03T11:15:22.640Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 75
File: crates/amaru/src/consensus/mod.rs:164-165
Timestamp: 2025-02-03T11:15:22.640Z
Learning: In the Amaru project, chain selection operations (roll_forward and rollback) should use separate result types to leverage the type system for preventing impossible states, rather than using runtime checks or panics.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-08-08T14:34:06.105Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/minicbor-extra/src/lib.rs:50-55
Timestamp: 2025-08-08T14:34:06.105Z
Learning: Project uses Rust 1.88 stable; using Result::is_err_and is acceptable and should not be flagged as unstable. In particular, crates/minicbor-extra/src/lib.rs relies on is_err_and when checking end_of_input; future reviews should not suggest replacing it for stability reasons.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-04-20T17:57:23.233Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:272-285
Timestamp: 2025-04-20T17:57:23.233Z
Learning: In test infrastructure code, rkuhn prefers explicit panics (using .unwrap() or similar) over returning Result types, as test failures should be immediate and obvious.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-08-08T14:35:35.562Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/transaction_pointer.rs:36-44
Timestamp: 2025-08-08T14:35:35.562Z
Learning: In the amaru project, when decoding CBOR arrays, prefer using minicbor_extra::heterogenous_array with the expected length to validate definite-length arrays and correctly handle indefinite-length arrays. Example: crates/amaru-kernel/src/transaction_pointer.rs Decode should use heterogenous_array(d, 2, …) instead of ignoring the length from d.array().
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs
🧬 Code graph analysis (1)
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (3)
crates/amaru-plutus/src/script_context/v1.rs (13)
to_plutus_data(29-31)to_plutus_data(40-42)to_plutus_data(46-70)to_plutus_data(85-103)to_plutus_data(114-127)to_plutus_data(135-140)to_plutus_data(144-146)to_plutus_data(153-155)to_plutus_data(188-222)to_plutus_data(227-239)to_plutus_data(250-261)to_plutus_data(265-267)to_plutus_data(271-273)crates/amaru-plutus/src/script_context/mod.rs (17)
to_plutus_data(1098-1110)from(559-561)from(627-629)from(641-668)from(672-701)from(705-710)from(801-808)from(812-819)from(842-848)from(852-858)from(862-867)from(881-889)from(931-946)from(954-956)from(984-986)from(1065-1072)from(1136-1151)crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs (1)
execute(89-230)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 182, 15, 10.5.3)
- GitHub Check: End-to-end snapshot tests (preview, 2, 680, true, 15, 10.5.3)
- GitHub Check: Build aarch64/linux
- GitHub Check: Build x86_64/windows
- GitHub Check: Build riscv32
- GitHub Check: Build x86_64/linux
- GitHub Check: Build wasm32
- GitHub Check: Build aarch64/macos
- GitHub Check: Test the CLI
- GitHub Check: Test coverage
- GitHub Check: Benches
- GitHub Check: clippy
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (6)
67-84: Clean early exit and sensible function signature.The early bail-out when there's no redeemer (lines 82-84) is spot on—no point in firing up the Plutus engine if there's nothing to validate. Eight params might seem like a mouthful, but transaction validation needs all this context, and the generic
C: UtxoSlice + fmt::Debugkeeps it flexible.
86-121: Input resolution logic is solid, and the typo's been squashed.Nice work here—resolving both regular inputs and reference inputs, propagating
MissingInputerrors cleanly, and merging them into a unifiedUtxosfor the script context. The variable naming is now correct (shout-out to fixingresolved_reference_inputs), and the optional handling of reference inputs with.transpose()is elegant.
123-134: TxInfo construction and script context setup look good.Standard wiring here—building the
TxInfowith all the transaction metadata (body, witnesses, hash, utxos, slot, network, era history, protocol version) and then extracting the script contexts to execute. Clean and straightforward.
208-247: V3 validation now correctly enforces CIP-117's unit requirement—top marks!Brilliant work here. The version-specific handling is spot on:
- V1/V2 (lines 209-231): Accept any non-
Errorterm—standard Plutus semantics.- V3 (lines 234-246): Only accept
Term::Constant(Constant::Unit)per CIP-117—exactly as specified.This addresses the previous review's concern about V3 scripts falsely passing when returning non-unit terms (like an unsaturated script hitting
Term::Var). The explicit check for unit ensures V3 scripts play by the new rules.
180-192: The.expect()on PlutusData round-trip conversion remains—still a panic risk.This is the same concern from the last review, mate. Line 189's
expect("unable to decode PlutusData cbor")will panic if the Pallas → uplc PlutusData round-trip fails. The TODO at line 180 even acknowledges this is a "really bad conversion."Previous reviews suggested propagating a
PhaseTwoErrorinstead, but theexpect()is still here. If the round-trip fails (corrupt CBOR, version mismatch, whatever), you get a production panic instead of a graceful error.Consider the past suggestion: map the
.expect()to.map_err(|_| PhaseTwoError::ScriptDeserializationError(...))and change themap()to aResultchain so you can propagate failures.⛔ Skipped due to learnings
Learnt from: KtorZ Repo: pragma-org/amaru PR: 370 File: crates/amaru-kernel/src/borrowed_datum.rs:32-39 Timestamp: 2025-08-08T14:39:50.527Z Learning: In the amaru project, when converting BorrowedDatumOption::Data to an owned DatumOption in crates/amaru-kernel/src/borrowed_datum.rs, the call `.unwrap()` refers to pallas’s KeepRaw::unwrap, which is infallible (always returns the inner value) and is not a panic risk. Future reviews should not flag this unwrap as dangerous.Learnt from: KtorZ Repo: pragma-org/amaru PR: 370 File: crates/minicbor-extra/src/lib.rs:50-55 Timestamp: 2025-08-08T14:34:06.105Z Learning: Project uses Rust 1.88 stable; using Result::is_err_and is acceptable and should not be flagged as unstable. In particular, crates/minicbor-extra/src/lib.rs relies on is_err_and when checking end_of_input; future reviews should not suggest replacing it for stability reasons.Learnt from: rkuhn Repo: pragma-org/amaru PR: 149 File: crates/amaru/src/stages/consensus/chain_forward.rs:73-75 Timestamp: 2025-04-20T18:02:25.073Z Learning: In the current development stage, rkuhn prefers using explicit panics (via `.expect()` or `.unwrap()`) for fatal errors in the application code that would tear down the node, rather than propagating errors with `Result`. The intention is to eventually transition to proper error handling with `Result` as the codebase matures.Learnt from: rkuhn Repo: pragma-org/amaru PR: 612 File: crates/amaru-protocols/src/blockfetch/mod.rs:151-173 Timestamp: 2025-12-28T19:26:35.354Z Learning: In crates/amaru-protocols/src/blockfetch/mod.rs, the blockfetch initiator uses .expect() when popping from the request queue on NoBlocks and Done results. These are intentional fail-fast assertions: the protocol state machine guarantees the queue is non-empty when these messages arrive, so an empty queue indicates a protocol violation. A misbehaving peer triggers an erroneous protocol transition that will close the connection (supervision to be implemented in a future PR). This follows the project's fail-fast philosophy for protocol invariants. <!--Learnt from: jeluard Repo: pragma-org/amaru PR: 69 File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117 Timestamp: 2025-01-21T15:32:17.911Z Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.Learnt from: abailly Repo: pragma-org/amaru PR: 435 File: crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs:113-116 Timestamp: 2025-09-06T09:16:25.025Z Learning: In cardano-node serialized ledger state CBOR encoding, indefinite-length structures may be terminated with 0xFF "break" markers. The current code in convert_ledger_state.rs unconditionally strips the last byte (bytes[p..bytes.len() - 1]), which could corrupt data if the trailing byte is not actually a CBOR break marker.
136-171: Theunreachable!()for Native scripts is actually solid and confirmed bulletproof.The invariant holds because the
script_table(populated at line 302 ofamaru-plutus/src/script_context/mod.rs) only contains scripts that have redeemers attached. Since Cardano's ledger rules dictate that only Plutus scripts (V1, V2, V3) can have redeemers—Native scripts are validated separately in Phase One and never receive redeemers—thescript_tableis guaranteed to contain only Plutus scripts. Whento_script_contexts()iterates over this table, it can never yield a Native script.No action needed here; the code is correct as written.
This PR introduces a subset of Phase 2 validations, targeting Plutus V3 script execution.
In order to achieve that, we also need to introduce an
ArenaPool, which contains a pool of arenas (go figure), used for script evaluation.Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.