Skip to content

fix: add withdrawals to redeemers pointers check #708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rodrigomd94 wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: add withdrawals to redeemers pointers check #708

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
34cb253
fix: add withdrawals to redeemers pointers check
rodrigomd94 Oct 23, 2025
455fb0b
fix: remove panic-inducing .expect() call on address parsing
rodrigomd94 Oct 23, 2025
0324f8f
fix: fix type mismatch when handling address parsing error
rodrigomd94 Oct 23, 2025
fd4bbcf
fix: sort reward accounts before getting redeemer pointers
rodrigomd94 Oct 23, 2025
955073c
chore: fix lint format
rodrigomd94 Oct 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 58 additions & 1 deletion pallas-validate/src/phase1/conway.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ use crate::utils::{
ValidationError::{self, *},
ValidationResult,
};
use pallas_addresses::{ScriptHash, ShelleyAddress, ShelleyPaymentPart};
use itertools::Itertools;
use pallas_addresses::{Address, Network, ScriptHash, ShelleyAddress, ShelleyPaymentPart, StakePayload};
use pallas_codec::utils::{Bytes, KeepRaw};
use pallas_primitives::{
babbage,
Expand All @@ -23,6 +24,7 @@ use pallas_primitives::{
AddrKeyhash, Hash, PlutusData, PlutusScript, PolicyId, PositiveCoin, TransactionInput,
};
use pallas_traverse::{MultiEraInput, MultiEraOutput, OriginalHash};
use std::cmp::Ordering;
use std::ops::Deref;

pub fn validate_conway_tx(
Expand Down Expand Up @@ -1129,6 +1131,7 @@ fn check_redeemers(

None => Vec::new(),
};

let plutus_scripts: Vec<RedeemersKey> = mk_plutus_script_redeemer_pointers(
plutus_v1_scripts,
plutus_v2_scripts,
Expand All @@ -1147,6 +1150,35 @@ fn sort_inputs(unsorted_inputs: &[TransactionInput]) -> Vec<TransactionInput> {
res
}

// 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.");
}
}
Comment on lines +1155 to +1182
Copy link

@coderabbitai coderabbitai bot Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.


fn mk_plutus_script_redeemer_pointers(
plutus_v1_scripts: &[PolicyId],
plutus_v2_scripts: &[PolicyId],
Expand Down Expand Up @@ -1190,6 +1222,31 @@ fn mk_plutus_script_redeemer_pointers(
}
}
}
if let Some(withdrawals) = &tx_body.withdrawals {
for (index, (stake_key_hash_bytes, _amount)) in withdrawals
.iter()
.sorted_by(|(accnt_a, _), (accnt_b, _)| sort_reward_accounts(accnt_a, accnt_b))
.enumerate()
{
if let Ok(Address::Stake(stake_addr)) = Address::from_bytes(stake_key_hash_bytes) {
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,
) {
res.push(RedeemersKey {
tag: pallas_primitives::conway::RedeemerTag::Reward,
index: index as u32,
})
}
}
}
}
}
res
}

Expand Down
Loading