fix: add withdrawals to redeemers pointers check #708
Conversation
WalkthroughAdds 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 rgb(230, 240, 255)
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 rgb(240, 255, 240)
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🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. 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
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