Better input ordering

sdk/src/client/api/block_builder/transaction_builder/mod.rs

@@ -10,11 +10,11 @@ pub(crate) mod remainder;
 pub(crate) mod requirement;
 pub(crate) mod transition;
 
-use alloc::collections::BTreeMap;
+use alloc::collections::{BTreeMap, VecDeque};
+use core::borrow::Borrow;
 use std::collections::{HashMap, HashSet};
 
 use crypto::keys::bip44::Bip44;
-use packable::PackableExt;
 
 pub use self::{burn::Burn, error::TransactionBuilderError, requirement::Requirement};
 use crate::{
@@ -40,7 +40,7 @@ use crate::{
             signed_transaction::{Transaction, TransactionCapabilities, TransactionCapabilityFlag},
             TaggedDataPayload,
         },
-        protocol::{CommittableAgeRange, ProtocolParameters},
+        protocol::ProtocolParameters,
         slot::{SlotCommitmentId, SlotIndex},
     },
 };
@@ -182,7 +182,7 @@ impl Client {
 pub struct TransactionBuilder {
     available_inputs: Vec<InputSigningData>,
     required_inputs: HashSet<OutputId>,
-    selected_inputs: Vec<InputSigningData>,
+    selected_inputs: OrderedInputs,
     bic_context_inputs: HashSet<BlockIssuanceCreditContextInput>,
     commitment_context_input: Option<CommitmentContextInput>,
     reward_context_inputs: HashSet<OutputId>,
@@ -254,7 +254,7 @@ impl TransactionBuilder {
         Self {
             available_inputs,
             required_inputs: HashSet::new(),
-            selected_inputs: Vec::new(),
+            selected_inputs: Default::default(),
             bic_context_inputs: HashSet::new(),
             commitment_context_input: None,
             reward_context_inputs: HashSet::new(),
@@ -312,7 +312,6 @@ impl TransactionBuilder {
         }
 
         // Gets requirements from outputs.
-        // TODO this may re-evaluate outputs added by inputs
         self.outputs_requirements();
 
         // Gets requirements from burn.
@@ -430,12 +429,6 @@ impl TransactionBuilder {
             }
         }
 
-        let inputs_data = Self::sort_input_signing_data(
-            self.selected_inputs,
-            self.latest_slot_commitment_id.slot_index(),
-            self.protocol_parameters.committable_age_range(),
-        )?;
-
         let mut inputs: Vec<Input> = Vec::new();
         let mut context_inputs = self
             .bic_context_inputs
@@ -444,7 +437,7 @@ impl TransactionBuilder {
             .chain(self.commitment_context_input.map(ContextInput::from))
             .collect::<Vec<_>>();
 
-        for (idx, input) in inputs_data.iter().enumerate() {
+        for (idx, input) in self.selected_inputs.iter().enumerate() {
             inputs.push(Input::Utxo(UtxoInput::from(*input.output_id())));
             if self.reward_context_inputs.contains(input.output_id()) {
                 context_inputs.push(RewardContextInput::new(idx as u16).unwrap().into());
@@ -475,7 +468,7 @@ impl TransactionBuilder {
 
         let data = PreparedTransactionData {
             transaction,
-            inputs_data,
+            inputs_data: self.selected_inputs.into_iter().collect(),
             remainders: self.remainders.data,
             mana_rewards: self.mana_rewards.into_iter().collect(),
         };
@@ -507,9 +500,18 @@ impl TransactionBuilder {
         // New input may need context inputs
         self.fulfill_context_inputs_requirements(&input);
 
-        self.selected_inputs.push(input);
+        let required_address = input
+            .output
+            .required_address(
+                self.latest_slot_commitment_id.slot_index(),
+                self.protocol_parameters.committable_age_range(),
+            )
+            // PANIC: safe to unwrap as non basic/account/foundry/nft/delegation outputs are already filtered out.
+            .unwrap()
+            .expect("expiration unlockable outputs already filtered out");
+        self.selected_inputs.insert(required_address, input);
 
-        Ok(if added_output { self.added_outputs.last() } else { None })
+        Ok(added_output.then(|| self.added_outputs.last().unwrap()))
     }
 
     /// Sets the required inputs of an [`TransactionBuilder`].
@@ -688,99 +690,113 @@ impl TransactionBuilder {
             }
         })
     }
+}
 
-    // Inputs need to be sorted before signing, because the reference unlock conditions can only reference a lower index
-    pub(crate) fn sort_input_signing_data(
-        mut inputs: Vec<InputSigningData>,
-        commitment_slot_index: SlotIndex,
-        committable_age_range: CommittableAgeRange,
-    ) -> Result<Vec<InputSigningData>, TransactionBuilderError> {
-        // initially sort by output to make it deterministic
-        // TODO: rethink this, we only need it deterministic for tests, for the protocol it doesn't matter, also there
-        // might be a more efficient way to do this
-        inputs.sort_by_key(|i| i.output.pack_to_vec());
-        // filter for ed25519 address first
-        let (mut sorted_inputs, account_nft_address_inputs): (Vec<InputSigningData>, Vec<InputSigningData>) =
-            inputs.into_iter().partition(|input_signing_data| {
-                let required_address = input_signing_data
-                    .output
-                    .required_address(commitment_slot_index, committable_age_range)
-                    // PANIC: safe to unwrap as non basic/account/foundry/nft outputs are already filtered out.
-                    .unwrap()
-                    .expect("expiration unlockable outputs already filtered out");
+#[derive(Clone, Debug, Default)]
+pub(crate) struct OrderedInputs {
+    ed25519: VecDeque<InputSigningData>,
+    other: BTreeMap<Address, VecDeque<InputSigningData>>,
+    len: usize,
+}
 
-                required_address.is_ed25519()
-            });
+impl OrderedInputs {
+    pub(crate) fn iter(&self) -> OrderedInputsIter<&Address, &InputSigningData> {
+        self.into_iter()
+    }
 
-        for input in account_nft_address_inputs {
-            let required_address = input
-                .output
-                .required_address(commitment_slot_index, committable_age_range)?
-                .expect("expiration unlockable outputs already filtered out");
+    pub(crate) fn insert(&mut self, required_address: Address, input: InputSigningData) {
+        if required_address.is_ed25519_backed() {
+            self.ed25519.push_back(input);
+        } else {
+            self.other.entry(required_address).or_default().push_back(input);
+        }
+        self.len += 1;
+    }
 
-            match sorted_inputs
-                .iter()
-                .position(|input_signing_data| match required_address {
-                    Address::Account(unlock_address) => {
-                        if let Output::Account(account_output) = &input_signing_data.output {
-                            *unlock_address.account_id()
-                                == account_output.account_id_non_null(input_signing_data.output_id())
-                        } else {
-                            false
-                        }
-                    }
-                    Address::Nft(unlock_address) => {
-                        if let Output::Nft(nft_output) = &input_signing_data.output {
-                            *unlock_address.nft_id() == nft_output.nft_id_non_null(input_signing_data.output_id())
-                        } else {
-                            false
-                        }
+    pub(crate) fn len(&self) -> usize {
+        self.len
+    }
+
+    pub(crate) fn is_empty(&self) -> bool {
+        self.len() == 0
+    }
+}
+
+#[derive(Clone, Debug)]
+pub(crate) struct OrderedInputsIter<A: Borrow<Address> + Ord + core::hash::Hash, I: Borrow<InputSigningData>> {
+    queue: VecDeque<I>,
+    other: BTreeMap<A, VecDeque<I>>,
+}
+
+impl<A: Borrow<Address> + Ord + core::hash::Hash, I: Borrow<InputSigningData>> Iterator for OrderedInputsIter<A, I> {
+    type Item = I;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        if let Some(input) = self.queue.pop_front() {
+            // Add associated inputs to the front of the queue
+            match &input.borrow().output {
+                Output::Account(account_output) => {
+                    for input in self
+                        .other
+                        .remove(&Address::Account(AccountAddress::new(
+                            account_output.account_id_non_null(input.borrow().output_id()),
+                        )))
+                        .into_iter()
+                        .flatten()
+                        .rev()
+                    {
+                        self.queue.push_front(input);
                     }
-                    _ => false,
-                }) {
-                Some(position) => {
-                    // Insert after the output we need
-                    sorted_inputs.insert(position + 1, input);
                 }
-                None => {
-                    // insert before address
-                    let account_or_nft_address = match &input.output {
-                        Output::Account(account_output) => Some(Address::Account(AccountAddress::new(
-                            account_output.account_id_non_null(input.output_id()),
-                        ))),
-                        Output::Nft(nft_output) => Some(Address::Nft(NftAddress::new(
-                            nft_output.nft_id_non_null(input.output_id()),
-                        ))),
-                        _ => None,
-                    };
-
-                    if let Some(account_or_nft_address) = account_or_nft_address {
-                        // Check for existing outputs for this address, and insert before
-                        match sorted_inputs.iter().position(|input_signing_data| {
-                            let required_address = input_signing_data
-                                .output
-                                .required_address(commitment_slot_index, committable_age_range)
-                                // PANIC: safe to unwrap as non basic/alias/foundry/nft outputs are already filtered
-                                .unwrap()
-                                .expect("expiration unlockable outputs already filtered out");
-
-                            required_address == account_or_nft_address
-                        }) {
-                            Some(position) => {
-                                // Insert before the output with this address required for unlocking
-                                sorted_inputs.insert(position, input);
-                            }
-                            // just push output
-                            None => sorted_inputs.push(input),
-                        }
-                    } else {
-                        // just push basic or foundry output
-                        sorted_inputs.push(input);
+                Output::Nft(nft_output) => {
+                    for input in self
+                        .other
+                        .remove(&Address::Nft(NftAddress::new(
+                            nft_output.nft_id_non_null(input.borrow().output_id()),
+                        )))
+                        .into_iter()
+                        .flatten()
+                        .rev()
+                    {
+                        self.queue.push_front(input);
                     }
                 }
+                _ => (),
+            };
+            return Some(input);
+        }
+        if let Some(mut entry) = self.other.first_entry() {
+            if let Some(input) = entry.get_mut().pop_front() {
+                if entry.get().is_empty() {
+                    self.other.pop_first();
+                }
+                return Some(input);
             }
         }
+        None
+    }
+}
+
+impl<'a> IntoIterator for &'a OrderedInputs {
+    type Item = &'a InputSigningData;
+    type IntoIter = OrderedInputsIter<&'a Address, Self::Item>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        OrderedInputsIter {
+            queue: self.ed25519.iter().collect(),
+            other: self.other.iter().map(|(k, v)| (k, v.iter().collect())).collect(),
+        }
+    }
+}
 
-        Ok(sorted_inputs)
+impl IntoIterator for OrderedInputs {
+    type Item = InputSigningData;
+    type IntoIter = OrderedInputsIter<Address, Self::Item>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        OrderedInputsIter {
+            queue: self.ed25519,
+            other: self.other,
+        }
     }
 }

sdk/src/client/api/block_builder/transaction_builder/remainder.rs

@@ -30,7 +30,7 @@ impl TransactionBuilder {
         Ok(())
     }
 
-    /// Gets the remainder address from configuration of finds one from the inputs.
+    /// Gets the remainder address from configuration or finds one from the inputs.
     pub(crate) fn get_remainder_address(&self) -> Result<Option<(Address, Option<Bip44>)>, TransactionBuilderError> {
         if let Some(remainder_address) = &self.remainders.address {
             // Search in inputs for the Bip44 chain for the remainder address, so the ledger can regenerate it

sdk/src/client/api/block_builder/transaction_builder/requirement/mana.rs

@@ -37,12 +37,6 @@ impl TransactionBuilder {
         let mut should_recalculate = false;
 
         if !self.selected_inputs.is_empty() && self.all_outputs().next().is_some() {
-            self.selected_inputs = Self::sort_input_signing_data(
-                std::mem::take(&mut self.selected_inputs),
-                self.latest_slot_commitment_id.slot_index(),
-                self.protocol_parameters.committable_age_range(),
-            )?;
-
             let inputs = self
                 .selected_inputs
                 .iter()