fix: add withdrawals to redeemers pointers check#708
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds deterministic handling of stake withdrawals when building Plutus redeemer pointers: withdrawals are sorted by network and stake payload, parsed as Changes
Sequence Diagram(s)sequenceDiagram
participant Tx as Transaction
participant Func as mk_plutus_script_redeemer_pointers
participant Inputs as InputsAnalyzer
participant Mint as MintAnalyzer
participant Withdrawals as WithdrawalProcessor
rect rgba(230,240,255,0.5)
Tx->>Func: request redeemer pointers
end
Note over Func,Inputs: existing: analyze inputs
Func->>Inputs: inspect inputs
Inputs-->>Func: input redeemer pointers
Note over Func,Mint: existing: analyze mint entries
Func->>Mint: inspect mint entries
Mint-->>Func: mint redeemer pointers
rect rgba(240,255,240,0.5)
Note over Func,Withdrawals: new: deterministic withdrawal processing
Func->>Withdrawals: collect withdrawals
Withdrawals->>Withdrawals: sort_reward_accounts (network, payload, hash)
Withdrawals->>Withdrawals: parse stake address -> Address / StakePayload
alt script-backed & matches phase‑2
Withdrawals-->>Func: emit Reward redeemer pointer (RedeemersKey::Reward, index)
else non-script or non-matching
Withdrawals-->>Func: no pointer
else invalid address
Withdrawals-->>Func: unreachable (invalid reward addr)
end
end
Func-->>Tx: return complete redeemer pointer set
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pallas-validate/src/phase1/conway.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pallas-validate/src/phase1/conway.rs (3)
pallas-traverse/src/tx.rs (1)
withdrawals(476-492)pallas-validate/src/phase1/babbage.rs (1)
is_phase_2_script(989-1004)pallas-traverse/src/witnesses.rs (3)
plutus_v1_scripts(82-104)plutus_v2_scripts(196-213)plutus_v3_scripts(215-227)
🔇 Additional comments (1)
pallas-validate/src/phase1/conway.rs (1)
15-15: LGTM!The
Addressimport is necessary for parsing stake addresses in the withdrawal handling logic and is consistent with the existing import patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pallas-validate/src/phase1/conway.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pallas-validate/src/phase1/conway.rs (4)
pallas-traverse/src/tx.rs (1)
withdrawals(476-492)pallas-validate/src/phase2/script_context.rs (1)
withdrawals(413-419)pallas-validate/src/phase1/babbage.rs (1)
is_phase_2_script(989-1004)pallas-traverse/src/witnesses.rs (3)
plutus_v1_scripts(82-104)plutus_v2_scripts(196-213)plutus_v3_scripts(215-227)
🔇 Additional comments (1)
pallas-validate/src/phase1/conway.rs (1)
15-15: LGTM!The
Addressimport is correctly added to support parsing stake address bytes in the withdrawals check.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pallas-validate/src/phase1/conway.rs (1)
1153-1180: Refactor to eliminate code duplication.This function is duplicated from
pallas-validate/src/phase2/script_context.rs(lines 1081-1107). Consider extracting it to a shared module to maintain a single source of truth and avoid maintenance overhead.For example, you could move this function to a common utilities module and import it in both places:
// In a shared utilities module pub fn sort_reward_accounts(a: &Bytes, b: &Bytes) -> Ordering { // ... implementation }Then import in both phase1/conway.rs and phase2/script_context.rs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pallas-validate/src/phase1/conway.rs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pallas-validate/src/phase1/conway.rs (4)
pallas-validate/src/phase2/script_context.rs (3)
sort_reward_accounts(1082-1108)network_tag(1086-1092)withdrawals(413-419)pallas-traverse/src/tx.rs (1)
withdrawals(476-492)pallas-validate/src/phase1/babbage.rs (1)
is_phase_2_script(989-1004)pallas-traverse/src/witnesses.rs (3)
plutus_v1_scripts(82-104)plutus_v2_scripts(196-213)plutus_v3_scripts(215-227)
🔇 Additional comments (2)
pallas-validate/src/phase1/conway.rs (2)
15-16: LGTM!The new imports are necessary for withdrawal handling and deterministic sorting. All imports are used appropriately in the implementation.
Also applies to: 27-27
1225-1249: LGTM!The withdrawals handling is correctly implemented:
- Withdrawals are deterministically sorted before processing, ensuring stable redeemer pointer generation.
- Invalid addresses are gracefully skipped using the
if let Ok(...)pattern.- Script-backed stake addresses are correctly identified using
is_script().- Phase-2 script verification ensures only Plutus scripts generate Reward redeemer pointers.
This properly addresses the "withdraw 0 trick" validation issue mentioned in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pallas-validate/src/phase1/conway.rs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pallas-validate/src/phase1/conway.rs (4)
pallas-validate/src/phase2/script_context.rs (3)
sort_reward_accounts(1082-1108)network_tag(1086-1092)withdrawals(413-419)pallas-traverse/src/tx.rs (1)
withdrawals(476-492)pallas-validate/src/phase1/babbage.rs (1)
is_phase_2_script(989-1004)pallas-traverse/src/witnesses.rs (3)
plutus_v1_scripts(82-104)plutus_v2_scripts(196-213)plutus_v3_scripts(215-227)
🔇 Additional comments (2)
pallas-validate/src/phase1/conway.rs (2)
15-18: LGTM!The new imports support the withdrawals handling functionality. All are necessary for parsing stake addresses, sorting withdrawals, and creating reward redeemer pointers.
Also applies to: 29-29
1227-1251: Logic looks correct, but depends on fixing the sort function.The withdrawals handling logic correctly:
- Sorts withdrawals deterministically per Cardano ledger specifications
- Parses stake addresses with appropriate error handling
- Filters for script-backed stake withdrawals
- Creates
Rewardredeemer pointers only for phase-2 Plutus scriptsThis properly addresses the "withdraw 0 trick" mentioned in the PR objectives.
However, this code depends on
sort_reward_accountsnot panicking on invalid addresses. Please ensure the code duplication and error handling issues insort_reward_accounts(lines 1155-1182) are addressed first, as the current.expect()calls will cause panics before this graceful error handling can execute.
| // Sorting function for reward accounts (withdrawals). | ||
| fn sort_reward_accounts(a: &Bytes, b: &Bytes) -> Ordering { | ||
| let addr_a = Address::from_bytes(a).expect("invalid reward address in withdrawals."); | ||
| let addr_b = Address::from_bytes(b).expect("invalid reward address in withdrawals."); | ||
|
|
||
| fn network_tag(network: Network) -> u8 { | ||
| match network { | ||
| Network::Testnet => 0, | ||
| Network::Mainnet => 1, | ||
| Network::Other(tag) => tag, | ||
| } | ||
| } | ||
|
|
||
| if let (Address::Stake(accnt_a), Address::Stake(accnt_b)) = (addr_a, addr_b) { | ||
| if accnt_a.network() != accnt_b.network() { | ||
| return network_tag(accnt_a.network()).cmp(&network_tag(accnt_b.network())); | ||
| } | ||
|
|
||
| match (accnt_a.payload(), accnt_b.payload()) { | ||
| (StakePayload::Script(..), StakePayload::Stake(..)) => Ordering::Less, | ||
| (StakePayload::Stake(..), StakePayload::Script(..)) => Ordering::Greater, | ||
| (StakePayload::Script(hash_a), StakePayload::Script(hash_b)) => hash_a.cmp(hash_b), | ||
| (StakePayload::Stake(hash_a), StakePayload::Stake(hash_b)) => hash_a.cmp(hash_b), | ||
| } | ||
| } else { | ||
| unreachable!("invalid reward address in withdrawals."); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate code duplication and fix inconsistent error handling.
This function duplicates code from pallas-validate/src/phase2/script_context.rs (lines 1081-1107). Additionally, the use of .expect() creates inconsistent error handling: if withdrawals contain invalid addresses, the sort will panic before the graceful error handling in the loop at line 1233 can process them.
Consider these solutions:
Solution 1 (preferred): Import from phase2 module
If the phase2 module is accessible from phase1, import the function:
+use crate::phase2::script_context::sort_reward_accounts;
+
-// Sorting function for reward accounts (withdrawals).
-fn sort_reward_accounts(a: &Bytes, b: &Bytes) -> Ordering {
- let addr_a = Address::from_bytes(a).expect("invalid reward address in withdrawals.");
- let addr_b = Address::from_bytes(b).expect("invalid reward address in withdrawals.");
-
- fn network_tag(network: Network) -> u8 {
- match network {
- Network::Testnet => 0,
- Network::Mainnet => 1,
- Network::Other(tag) => tag,
- }
- }
-
- if let (Address::Stake(accnt_a), Address::Stake(accnt_b)) = (addr_a, addr_b) {
- if accnt_a.network() != accnt_b.network() {
- return network_tag(accnt_a.network()).cmp(&network_tag(accnt_b.network()));
- }
-
- match (accnt_a.payload(), accnt_b.payload()) {
- (StakePayload::Script(..), StakePayload::Stake(..)) => Ordering::Less,
- (StakePayload::Stake(..), StakePayload::Script(..)) => Ordering::Greater,
- (StakePayload::Script(hash_a), StakePayload::Script(hash_b)) => hash_a.cmp(hash_b),
- (StakePayload::Stake(hash_a), StakePayload::Stake(hash_b)) => hash_a.cmp(hash_b),
- }
- } else {
- unreachable!("invalid reward address in withdrawals.");
- }
-}
-Solution 2: Move to shared utilities module
If phase1 cannot access phase2, move this function to a shared location (e.g., crate::utils) and import it from both modules.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pallas-validate/src/phase1/conway.rs around lines 1155-1182, this sorting
function duplicates code from phase2 and uses .expect() which can panic; replace
the local function with a call to the canonical implementation from
pallas-validate/src/phase2/script_context.rs (preferred) or move the shared
sorting logic into a common crate::utils module and import it from both phase1
and phase2; ensure the shared function does not call .expect() but instead
returns a Result (or otherwise propagates/handles invalid-address errors) so
invalid withdrawal addresses are handled consistently by the existing error path
rather than panicking.
Using the patched version of pallas until next releases of both pallas and dolos. See: txpipe/pallas#708
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pallas-validate/src/phase1/conway.rs (1)
1155-1182:⚠️ Potential issue | 🔴 CriticalDon't let malformed withdrawals panic phase-1 validation.
sort_reward_accountsstill usesexpectandunreachable!on untrusted reward-account bytes. A malformed or non-stake withdrawal will abort validation during sorting beforecheck_redeemerscan return aValidationError; please parse/validate once and propagatePostAlonzo(InputDecoding)instead of panicking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallas-validate/src/phase1/conway.rs` around lines 1155 - 1182, sort_reward_accounts currently calls Address::from_bytes(...).expect(...) and uses unreachable!() which will panic on malformed or non-stake withdrawal bytes; change it to return a Result<Ordering, ValidationError> (or propagate an existing error type) and parse Address::from_bytes without expect, mapping any decode error to PostAlonzo(InputDecoding), and similarly handle the non-stake branch by returning the same ValidationError instead of unreachable!; update all call sites of sort_reward_accounts to propagate or handle the Result and ensure sorting returns a ValidationError when decoding fails rather than panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pallas-validate/src/phase1/conway.rs`:
- Around line 1234-1242: The code currently passes reference_scripts (which may
include native-script hashes) into is_phase_2_script causing native reference
scripts to be treated like Plutus and emit a Reward pointer; update the call
site around stake_addr.is_script() so you pass only Plutus reference hashes
(e.g., filter reference_scripts to include only hashes present in
plutus_v1_scripts || plutus_v2_scripts || plutus_v3_scripts, or derive a
pluts_only_reference_hashes collection from get_reference_script_hashes) before
calling is_phase_2_script(stake_hash, plutus_v1_scripts, plutus_v2_scripts,
plutus_v3_scripts, pluts_only_reference_hashes), or alternatively modify
is_phase_2_script to distinguish native vs Plutus refs internally.
---
Duplicate comments:
In `@pallas-validate/src/phase1/conway.rs`:
- Around line 1155-1182: sort_reward_accounts currently calls
Address::from_bytes(...).expect(...) and uses unreachable!() which will panic on
malformed or non-stake withdrawal bytes; change it to return a Result<Ordering,
ValidationError> (or propagate an existing error type) and parse
Address::from_bytes without expect, mapping any decode error to
PostAlonzo(InputDecoding), and similarly handle the non-stake branch by
returning the same ValidationError instead of unreachable!; update all call
sites of sort_reward_accounts to propagate or handle the Result and ensure
sorting returns a ValidationError when decoding fails rather than panicking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd8a7ad7-8245-461b-9453-35b056cf68ee
📒 Files selected for processing (1)
pallas-validate/src/phase1/conway.rs
| if stake_addr.is_script() { | ||
| let script_hash = stake_addr.payload().as_hash(); | ||
| if is_phase_2_script( | ||
| &script_hash, | ||
| plutus_v1_scripts, | ||
| plutus_v2_scripts, | ||
| plutus_v3_scripts, | ||
| reference_scripts, | ||
| ) { |
There was a problem hiding this comment.
Filter reference scripts to Plutus before adding Reward pointers.
is_phase_2_script treats any hash present in reference_scripts as redeemer-requiring, but get_reference_script_hashes also collects native reference scripts. For a withdrawal authorized by a native script supplied through a reference input, this branch will emit a Reward pointer and phase-1 will incorrectly fail with RedeemerMissing. Pass only Plutus reference hashes here, or make the helper distinguish native vs Plutus refs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pallas-validate/src/phase1/conway.rs` around lines 1234 - 1242, The code
currently passes reference_scripts (which may include native-script hashes) into
is_phase_2_script causing native reference scripts to be treated like Plutus and
emit a Reward pointer; update the call site around stake_addr.is_script() so you
pass only Plutus reference hashes (e.g., filter reference_scripts to include
only hashes present in plutus_v1_scripts || plutus_v2_scripts ||
plutus_v3_scripts, or derive a pluts_only_reference_hashes collection from
get_reference_script_hashes) before calling is_phase_2_script(stake_hash,
plutus_v1_scripts, plutus_v2_scripts, plutus_v3_scripts,
pluts_only_reference_hashes), or alternatively modify is_phase_2_script to
distinguish native vs Plutus refs internally.
|
Superseded by #761, which keeps the withdrawal redeemer-pointer logic and sorting but replaces the |
2 participants
mk_plutus_script_redeemer_pointers didn't include a check for withdrawals, which was causing phase 1 validation to fail for transactions that include plutus scripts in the withdrawals. This is relevant for the "withdraw 0 trick".
Summary by CodeRabbit
New Features
Improvements