From 082fca55e61158c9b48e02dde647c44006130e58 Mon Sep 17 00:00:00 2001 From: Alex Coats Date: Tue, 12 Mar 2024 13:40:09 -0400 Subject: [PATCH 01/11] Order inputs on selection instead of sorting later --- .../block_builder/transaction_builder/mod.rs | 100 ++++++++---------- .../transaction_builder/requirement/mana.rs | 6 -- 2 files changed, 47 insertions(+), 59 deletions(-) diff --git a/sdk/src/client/api/block_builder/transaction_builder/mod.rs b/sdk/src/client/api/block_builder/transaction_builder/mod.rs index 0e951614e4..062eae796b 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/mod.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/mod.rs @@ -13,7 +13,6 @@ use alloc::collections::BTreeMap; use std::collections::{HashMap, HashSet}; use crypto::keys::bip44::Bip44; -use packable::PackableExt; pub use self::{burn::Burn, error::TransactionBuilderError, requirement::Requirement}; use crate::{ @@ -39,7 +38,7 @@ use crate::{ signed_transaction::{Transaction, TransactionCapabilities, TransactionCapabilityFlag}, TaggedDataPayload, }, - protocol::{CommittableAgeRange, ProtocolParameters}, + protocol::ProtocolParameters, slot::{SlotCommitmentId, SlotIndex}, }, }; @@ -313,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. @@ -419,12 +417,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 = Vec::new(); let mut context_inputs = self .bic_context_inputs @@ -433,7 +425,7 @@ impl TransactionBuilder { .chain(self.commitment_context_input.map(ContextInput::from)) .collect::>(); - 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()); @@ -464,7 +456,7 @@ impl TransactionBuilder { let data = PreparedTransactionData { transaction, - inputs_data, + inputs_data: self.selected_inputs, remainders: self.remainders.data, mana_rewards: self.mana_rewards.into_iter().collect(), }; @@ -477,7 +469,7 @@ impl TransactionBuilder { fn select_input(&mut self, input: InputSigningData) -> Result, TransactionBuilderError> { log::debug!("Selecting input {:?}", input.output_id()); - let mut added_output = None; + let mut added_output = false; if let Some(output) = self.transition_input(&input)? { // No need to check for `outputs_requirements` because // - the sender feature doesn't need to be verified as it has been removed @@ -485,7 +477,7 @@ impl TransactionBuilder { // - input doesn't need to be checked for as we just transitioned it // - foundry account requirement should have been met already by a prior `required_account_nft_addresses` self.added_outputs.push(output); - added_output = self.added_outputs.last(); + added_output = true; } if let Some(requirement) = self.required_account_nft_addresses(&input)? { @@ -493,14 +485,14 @@ impl TransactionBuilder { self.requirements.push(requirement); } - self.selected_inputs.push(input); + self.insert_input_sorted(input); // New inputs/outputs may need context inputs if !self.requirements.contains(&Requirement::ContextInputs) { self.requirements.push(Requirement::ContextInputs); } - Ok(added_output) + Ok(added_output.then(|| self.added_outputs.last().unwrap())) } /// Sets the required inputs of an [`TransactionBuilder`]. @@ -680,38 +672,38 @@ 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, - commitment_slot_index: SlotIndex, - committable_age_range: CommittableAgeRange, - ) -> Result, 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, Vec) = - inputs.into_iter().partition(|input_signing_data| { + fn insert_input_sorted(&mut self, input: InputSigningData) { + let input_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 outputs are already filtered out. + .unwrap() + .expect("expiration unlockable outputs already filtered out"); + if input_required_address.is_ed25519() { + if let Some(position) = self + .selected_inputs + .iter() + .position(|i| i.output_id() > input.output_id()) + { + self.selected_inputs.insert(position, input); + } else { + self.selected_inputs.push(input); + } + } else { + match self.selected_inputs.iter().position(|input_signing_data| { let required_address = input_signing_data .output - .required_address(commitment_slot_index, committable_age_range) + .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 outputs are already filtered out. .unwrap() .expect("expiration unlockable outputs already filtered out"); - - required_address.is_ed25519() - }); - - 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"); - - match sorted_inputs - .iter() - .position(|input_signing_data| match required_address { + match required_address { Address::Account(unlock_address) => { if let Output::Account(account_output) = &input_signing_data.output { *unlock_address.account_id() @@ -728,10 +720,11 @@ impl TransactionBuilder { } } _ => false, - }) { + } + }) { Some(position) => { // Insert after the output we need - sorted_inputs.insert(position + 1, input); + self.selected_inputs.insert(position + 1, input); } None => { // insert before address @@ -747,31 +740,32 @@ impl TransactionBuilder { 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| { + match self.selected_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 + .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 outputs are already filtered + // out. .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); + self.selected_inputs.insert(position, input); } // just push output - None => sorted_inputs.push(input), + None => self.selected_inputs.push(input), } } else { // just push basic or foundry output - sorted_inputs.push(input); + self.selected_inputs.push(input); } } } } - - Ok(sorted_inputs) } } diff --git a/sdk/src/client/api/block_builder/transaction_builder/requirement/mana.rs b/sdk/src/client/api/block_builder/transaction_builder/requirement/mana.rs index 58a0b8989f..7b157d67b0 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/requirement/mana.rs +++ b/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() From 62c44679b44969d5aff767a0ee3b4892c61c1130 Mon Sep 17 00:00:00 2001 From: Alex Coats Date: Tue, 12 Mar 2024 15:41:47 -0400 Subject: [PATCH 02/11] update comments --- .../client/api/block_builder/transaction_builder/mod.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/sdk/src/client/api/block_builder/transaction_builder/mod.rs b/sdk/src/client/api/block_builder/transaction_builder/mod.rs index 062eae796b..91b882e3e3 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/mod.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/mod.rs @@ -679,7 +679,7 @@ impl TransactionBuilder { self.latest_slot_commitment_id.slot_index(), self.protocol_parameters.committable_age_range(), ) - // PANIC: safe to unwrap as non basic/account/foundry/nft outputs are already filtered out. + // PANIC: safe to unwrap as non basic/account/foundry/nft/delegation outputs are already filtered out. .unwrap() .expect("expiration unlockable outputs already filtered out"); if input_required_address.is_ed25519() { @@ -700,7 +700,8 @@ impl TransactionBuilder { self.latest_slot_commitment_id.slot_index(), self.protocol_parameters.committable_age_range(), ) - // PANIC: safe to unwrap as non basic/account/foundry/nft outputs are already filtered out. + // PANIC: safe to unwrap as non basic/account/foundry/nft/delegation outputs are already filtered + // out. .unwrap() .expect("expiration unlockable outputs already filtered out"); match required_address { @@ -747,8 +748,8 @@ impl TransactionBuilder { self.latest_slot_commitment_id.slot_index(), self.protocol_parameters.committable_age_range(), ) - // PANIC: safe to unwrap as non basic/account/foundry/nft outputs are already filtered - // out. + // PANIC: safe to unwrap as non basic/account/foundry/nft/delegation outputs are already + // filtered out. .unwrap() .expect("expiration unlockable outputs already filtered out"); required_address == account_or_nft_address From 0472c8f668b37e6485b94b8a43ca601a8278c0a4 Mon Sep 17 00:00:00 2001 From: Alex Coats Date: Wed, 13 Mar 2024 12:28:42 -0400 Subject: [PATCH 03/11] Use a custom collection type for order --- .../block_builder/transaction_builder/mod.rs | 214 ++++++++++-------- .../transaction_builder/remainder.rs | 2 +- 2 files changed, 121 insertions(+), 95 deletions(-) diff --git a/sdk/src/client/api/block_builder/transaction_builder/mod.rs b/sdk/src/client/api/block_builder/transaction_builder/mod.rs index 91b882e3e3..5394681fd2 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/mod.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/mod.rs @@ -9,7 +9,7 @@ pub(crate) mod remainder; pub(crate) mod requirement; pub(crate) mod transition; -use alloc::collections::BTreeMap; +use alloc::collections::{BTreeMap, VecDeque}; use std::collections::{HashMap, HashSet}; use crypto::keys::bip44::Bip44; @@ -180,7 +180,7 @@ impl Client { pub struct TransactionBuilder { available_inputs: Vec, required_inputs: HashSet, - selected_inputs: Vec, + selected_inputs: OrderedInputs, bic_context_inputs: HashSet, commitment_context_input: Option, reward_context_inputs: HashSet, @@ -252,7 +252,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(), @@ -456,7 +456,7 @@ impl TransactionBuilder { let data = PreparedTransactionData { transaction, - inputs_data: self.selected_inputs, + inputs_data: self.selected_inputs.into_iter().collect(), remainders: self.remainders.data, mana_rewards: self.mana_rewards.into_iter().collect(), }; @@ -485,7 +485,16 @@ impl TransactionBuilder { self.requirements.push(requirement); } - self.insert_input_sorted(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(input, required_address); // New inputs/outputs may need context inputs if !self.requirements.contains(&Requirement::ContextInputs) { @@ -671,102 +680,119 @@ impl TransactionBuilder { } }) } +} - fn insert_input_sorted(&mut self, input: InputSigningData) { - let input_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"); - if input_required_address.is_ed25519() { - if let Some(position) = self - .selected_inputs - .iter() - .position(|i| i.output_id() > input.output_id()) - { - self.selected_inputs.insert(position, input); - } else { - self.selected_inputs.push(input); - } +#[derive(Clone, Debug, Default)] +pub(crate) struct OrderedInputs { + ed25519: VecDeque, + other: BTreeMap>, + len: usize, +} + +impl OrderedInputs { + pub(crate) fn iter(&self) -> <&Self as IntoIterator>::IntoIter { + self.into_iter() + } + + pub(crate) fn insert(&mut self, input: InputSigningData, required_address: Address) { + if required_address.is_ed25519() { + self.ed25519.push_back(input); } else { - match self.selected_inputs.iter().position(|input_signing_data| { - let required_address = input_signing_data - .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"); - 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 - } - } - _ => false, + self.other.entry(required_address).or_default().push(input); + } + self.len += 1; + } + + pub(crate) fn len(&self) -> usize { + self.len + } + + pub(crate) fn is_empty(&self) -> bool { + self.len() == 0 + } +} + +impl<'a> IntoIterator for &'a OrderedInputs { + type Item = &'a InputSigningData; + type IntoIter = alloc::vec::IntoIter; + + fn into_iter(self) -> Self::IntoIter { + let mut other = self + .other + .iter() + .map(|(k, v)| (k, v.iter().collect::>())) + .collect::>(); + let mut inputs = Vec::new(); + let mut queue = self.ed25519.iter().collect::>(); + while let Some(input) = queue.pop_front() { + // Add associated inputs to the front of the queue + match &input.output { + Output::Account(account_output) => { + queue = other + .remove(&Address::Account(AccountAddress::new( + account_output.account_id_non_null(input.output_id()), + ))) + .into_iter() + .flatten() + .chain(queue) + .collect() } - }) { - Some(position) => { - // Insert after the output we need - self.selected_inputs.insert(position + 1, input); + Output::Nft(nft_output) => { + queue = other + .remove(&Address::Nft(NftAddress::new( + nft_output.nft_id_non_null(input.output_id()), + ))) + .into_iter() + .flatten() + .chain(queue) + .collect() } - None => { - // insert before address - let account_or_nft_address = match &input.output { - Output::Account(account_output) => Some(Address::Account(AccountAddress::new( + _ => (), + }; + inputs.push(input); + } + inputs.extend(other.into_values().flatten()); + inputs.into_iter() + } +} + +impl IntoIterator for OrderedInputs { + type Item = InputSigningData; + type IntoIter = alloc::vec::IntoIter; + + fn into_iter(mut self) -> Self::IntoIter { + let mut inputs = Vec::new(); + let mut queue = self.ed25519; + while let Some(input) = queue.pop_front() { + // Add associated inputs to the front of the queue + match &input.output { + Output::Account(account_output) => { + queue = self + .other + .remove(&Address::Account(AccountAddress::new( account_output.account_id_non_null(input.output_id()), - ))), - Output::Nft(nft_output) => Some(Address::Nft(NftAddress::new( + ))) + .into_iter() + .flatten() + .chain(queue) + .collect() + } + Output::Nft(nft_output) => { + queue = self + .other + .remove(&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 self.selected_inputs.iter().position(|input_signing_data| { - let required_address = input_signing_data - .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"); - required_address == account_or_nft_address - }) { - Some(position) => { - // Insert before the output with this address required for unlocking - self.selected_inputs.insert(position, input); - } - // just push output - None => self.selected_inputs.push(input), - } - } else { - // just push basic or foundry output - self.selected_inputs.push(input); - } + ))) + .into_iter() + .flatten() + .chain(queue) + .collect() } - } + _ => (), + }; + inputs.push(input); } + inputs.extend(self.other.into_values().flatten()); + inputs.into_iter() } } diff --git a/sdk/src/client/api/block_builder/transaction_builder/remainder.rs b/sdk/src/client/api/block_builder/transaction_builder/remainder.rs index 7111bdfb7b..130ed3754a 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/remainder.rs +++ b/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)>, 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 From e7d0daca2f6b11fc95a1791c1e6d4dcd52f548a2 Mon Sep 17 00:00:00 2001 From: Alex Coats Date: Wed, 13 Mar 2024 12:45:43 -0400 Subject: [PATCH 04/11] optimize iters --- .../block_builder/transaction_builder/mod.rs | 109 ++++++++---------- 1 file changed, 50 insertions(+), 59 deletions(-) diff --git a/sdk/src/client/api/block_builder/transaction_builder/mod.rs b/sdk/src/client/api/block_builder/transaction_builder/mod.rs index 5394681fd2..23223bf166 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/mod.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/mod.rs @@ -10,6 +10,7 @@ pub(crate) mod requirement; pub(crate) mod transition; use alloc::collections::{BTreeMap, VecDeque}; +use core::borrow::Borrow; use std::collections::{HashMap, HashSet}; use crypto::keys::bip44::Bip44; @@ -690,7 +691,7 @@ pub(crate) struct OrderedInputs { } impl OrderedInputs { - pub(crate) fn iter(&self) -> <&Self as IntoIterator>::IntoIter { + pub(crate) fn iter(&self) -> OrderedInputsIter<&InputSigningData> { self.into_iter() } @@ -712,87 +713,77 @@ impl OrderedInputs { } } -impl<'a> IntoIterator for &'a OrderedInputs { - type Item = &'a InputSigningData; - type IntoIter = alloc::vec::IntoIter; +#[derive(Clone, Debug)] +pub(crate) struct OrderedInputsIter> { + queue: VecDeque, + other: BTreeMap>, +} - fn into_iter(self) -> Self::IntoIter { - let mut other = self - .other - .iter() - .map(|(k, v)| (k, v.iter().collect::>())) - .collect::>(); - let mut inputs = Vec::new(); - let mut queue = self.ed25519.iter().collect::>(); - while let Some(input) = queue.pop_front() { +impl> Iterator for OrderedInputsIter { + type Item = I; + + fn next(&mut self) -> Option { + if let Some(input) = self.queue.pop_front() { // Add associated inputs to the front of the queue - match &input.output { + match &input.borrow().output { Output::Account(account_output) => { - queue = other + for input in self + .other .remove(&Address::Account(AccountAddress::new( - account_output.account_id_non_null(input.output_id()), + account_output.account_id_non_null(input.borrow().output_id()), ))) .into_iter() .flatten() - .chain(queue) - .collect() + .rev() + { + self.queue.push_front(input); + } } Output::Nft(nft_output) => { - queue = other + for input in self + .other .remove(&Address::Nft(NftAddress::new( - nft_output.nft_id_non_null(input.output_id()), + nft_output.nft_id_non_null(input.borrow().output_id()), ))) .into_iter() .flatten() - .chain(queue) - .collect() + .rev() + { + self.queue.push_front(input); + } } _ => (), }; - inputs.push(input); + return Some(input); + } + None + } +} + +impl<'a> IntoIterator for &'a OrderedInputs { + type Item = &'a InputSigningData; + type IntoIter = OrderedInputsIter; + + fn into_iter(self) -> Self::IntoIter { + OrderedInputsIter { + queue: self.ed25519.iter().collect(), + other: self + .other + .iter() + .map(|(k, v)| (k.clone(), v.iter().collect())) + .collect(), } - inputs.extend(other.into_values().flatten()); - inputs.into_iter() } } impl IntoIterator for OrderedInputs { type Item = InputSigningData; - type IntoIter = alloc::vec::IntoIter; + type IntoIter = OrderedInputsIter; - fn into_iter(mut self) -> Self::IntoIter { - let mut inputs = Vec::new(); - let mut queue = self.ed25519; - while let Some(input) = queue.pop_front() { - // Add associated inputs to the front of the queue - match &input.output { - Output::Account(account_output) => { - queue = self - .other - .remove(&Address::Account(AccountAddress::new( - account_output.account_id_non_null(input.output_id()), - ))) - .into_iter() - .flatten() - .chain(queue) - .collect() - } - Output::Nft(nft_output) => { - queue = self - .other - .remove(&Address::Nft(NftAddress::new( - nft_output.nft_id_non_null(input.output_id()), - ))) - .into_iter() - .flatten() - .chain(queue) - .collect() - } - _ => (), - }; - inputs.push(input); + fn into_iter(self) -> Self::IntoIter { + OrderedInputsIter { + queue: self.ed25519, + other: self.other, } - inputs.extend(self.other.into_values().flatten()); - inputs.into_iter() } } From 3e96e37ac7b73b73ce24f3c78f5de9e72fa1b1c7 Mon Sep 17 00:00:00 2001 From: Alex Coats Date: Wed, 13 Mar 2024 13:10:31 -0400 Subject: [PATCH 05/11] use others in iter --- .../api/block_builder/transaction_builder/mod.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/sdk/src/client/api/block_builder/transaction_builder/mod.rs b/sdk/src/client/api/block_builder/transaction_builder/mod.rs index 23223bf166..0497b80cc6 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/mod.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/mod.rs @@ -686,7 +686,7 @@ impl TransactionBuilder { #[derive(Clone, Debug, Default)] pub(crate) struct OrderedInputs { ed25519: VecDeque, - other: BTreeMap>, + other: BTreeMap>, len: usize, } @@ -699,7 +699,7 @@ impl OrderedInputs { if required_address.is_ed25519() { self.ed25519.push_back(input); } else { - self.other.entry(required_address).or_default().push(input); + self.other.entry(required_address).or_default().push_back(input); } self.len += 1; } @@ -716,7 +716,7 @@ impl OrderedInputs { #[derive(Clone, Debug)] pub(crate) struct OrderedInputsIter> { queue: VecDeque, - other: BTreeMap>, + other: BTreeMap>, } impl> Iterator for OrderedInputsIter { @@ -756,6 +756,14 @@ impl> Iterator for OrderedInputsIter { }; 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 } } From 589b35e5c1d9ab653321b54936d180bb889d7f49 Mon Sep 17 00:00:00 2001 From: Alex Coats Date: Wed, 13 Mar 2024 13:29:31 -0400 Subject: [PATCH 06/11] reverse params --- sdk/src/client/api/block_builder/transaction_builder/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/src/client/api/block_builder/transaction_builder/mod.rs b/sdk/src/client/api/block_builder/transaction_builder/mod.rs index f4bc9b9a11..a1ba2781d9 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/mod.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/mod.rs @@ -509,7 +509,7 @@ impl TransactionBuilder { // 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(input, required_address); + self.selected_inputs.insert(required_address, input); Ok(added_output.then(|| self.added_outputs.last().unwrap())) } @@ -704,7 +704,7 @@ impl OrderedInputs { self.into_iter() } - pub(crate) fn insert(&mut self, input: InputSigningData, required_address: Address) { + pub(crate) fn insert(&mut self, required_address: Address, input: InputSigningData) { if required_address.is_ed25519() { self.ed25519.push_back(input); } else { From 5c8764945668d725156d8c9ee080e30a0f232e48 Mon Sep 17 00:00:00 2001 From: Alex Coats Date: Wed, 13 Mar 2024 13:31:43 -0400 Subject: [PATCH 07/11] remove address clones --- .../block_builder/transaction_builder/mod.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/sdk/src/client/api/block_builder/transaction_builder/mod.rs b/sdk/src/client/api/block_builder/transaction_builder/mod.rs index a1ba2781d9..59f2cc4b8b 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/mod.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/mod.rs @@ -700,7 +700,7 @@ pub(crate) struct OrderedInputs { } impl OrderedInputs { - pub(crate) fn iter(&self) -> OrderedInputsIter<&InputSigningData> { + pub(crate) fn iter(&self) -> OrderedInputsIter<&Address, &InputSigningData> { self.into_iter() } @@ -723,12 +723,12 @@ impl OrderedInputs { } #[derive(Clone, Debug)] -pub(crate) struct OrderedInputsIter> { +pub(crate) struct OrderedInputsIter + Ord + core::hash::Hash, I: Borrow> { queue: VecDeque, - other: BTreeMap>, + other: BTreeMap>, } -impl> Iterator for OrderedInputsIter { +impl + Ord + core::hash::Hash, I: Borrow> Iterator for OrderedInputsIter { type Item = I; fn next(&mut self) -> Option { @@ -779,23 +779,19 @@ impl> Iterator for OrderedInputsIter { impl<'a> IntoIterator for &'a OrderedInputs { type Item = &'a InputSigningData; - type IntoIter = OrderedInputsIter; + 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.clone(), v.iter().collect())) - .collect(), + other: self.other.iter().map(|(k, v)| (k, v.iter().collect())).collect(), } } } impl IntoIterator for OrderedInputs { type Item = InputSigningData; - type IntoIter = OrderedInputsIter; + type IntoIter = OrderedInputsIter; fn into_iter(self) -> Self::IntoIter { OrderedInputsIter { From e70e9d780d88f2664d84624987537c91d25866d3 Mon Sep 17 00:00:00 2001 From: Alex Coats Date: Wed, 13 Mar 2024 13:34:40 -0400 Subject: [PATCH 08/11] back it up --- sdk/src/client/api/block_builder/transaction_builder/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/src/client/api/block_builder/transaction_builder/mod.rs b/sdk/src/client/api/block_builder/transaction_builder/mod.rs index 59f2cc4b8b..745f0bd48b 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/mod.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/mod.rs @@ -705,7 +705,7 @@ impl OrderedInputs { } pub(crate) fn insert(&mut self, required_address: Address, input: InputSigningData) { - if required_address.is_ed25519() { + if required_address.is_ed25519_backed() { self.ed25519.push_back(input); } else { self.other.entry(required_address).or_default().push_back(input); From 4d6179d90d73f12928f304ba77fe0d558501d417 Mon Sep 17 00:00:00 2001 From: Alex Coats Date: Wed, 13 Mar 2024 16:06:12 -0400 Subject: [PATCH 09/11] cleanup and don't sort all the time --- sdk/examples/wallet/logger.rs | 9 +------- .../offline_signing/1_prepare_transaction.rs | 5 +--- sdk/examples/wallet/spammer.rs | 8 ++----- .../block_builder/transaction_builder/mod.rs | 23 ++++++++----------- .../transaction_builder/remainder.rs | 2 +- .../transaction_builder/requirement/amount.rs | 2 +- .../transaction_builder/requirement/mana.rs | 4 ++-- .../requirement/native_tokens.rs | 2 +- .../secret_manager/address_generation.rs | 13 ++++------- sdk/tests/client/secret_manager/mnemonic.rs | 1 - sdk/tests/client/secret_manager/stronghold.rs | 1 - 11 files changed, 22 insertions(+), 48 deletions(-) diff --git a/sdk/examples/wallet/logger.rs b/sdk/examples/wallet/logger.rs index 907a75120d..d058344abf 100644 --- a/sdk/examples/wallet/logger.rs +++ b/sdk/examples/wallet/logger.rs @@ -9,11 +9,7 @@ //! ``` use iota_sdk::{ - client::{ - api::GetAddressesOptions, - constants::SHIMMER_COIN_TYPE, - secret::{mnemonic::MnemonicSecretManager, SecretManager}, - }, + client::{constants::SHIMMER_COIN_TYPE, secret::mnemonic::MnemonicSecretManager}, crypto::keys::bip44::Bip44, wallet::{ClientOptions, Wallet}, }; @@ -39,9 +35,6 @@ async fn main() -> Result<(), Box> { // Restore a wallet let client_options = ClientOptions::new().with_node(&std::env::var("NODE_URL").unwrap())?; - let secret_manager = SecretManager::Mnemonic(MnemonicSecretManager::try_from_mnemonic( - std::env::var("MNEMONIC").unwrap(), - )?); let secret_manager = MnemonicSecretManager::try_from_mnemonic(std::env::var("MNEMONIC").unwrap())?; diff --git a/sdk/examples/wallet/offline_signing/1_prepare_transaction.rs b/sdk/examples/wallet/offline_signing/1_prepare_transaction.rs index 93c374fc96..fdae78ef91 100644 --- a/sdk/examples/wallet/offline_signing/1_prepare_transaction.rs +++ b/sdk/examples/wallet/offline_signing/1_prepare_transaction.rs @@ -9,10 +9,7 @@ //! ``` use iota_sdk::{ - client::{ - api::PreparedTransactionDataDto, constants::SHIMMER_COIN_TYPE, secret::SecretManager, - stronghold::StrongholdAdapter, - }, + client::{api::PreparedTransactionDataDto, constants::SHIMMER_COIN_TYPE, secret::SecretManager}, crypto::keys::bip44::Bip44, types::block::address::Bech32Address, wallet::{ClientOptions, SendParams, Wallet}, diff --git a/sdk/examples/wallet/spammer.rs b/sdk/examples/wallet/spammer.rs index 52786a6f73..70fc0aaf88 100644 --- a/sdk/examples/wallet/spammer.rs +++ b/sdk/examples/wallet/spammer.rs @@ -12,14 +12,10 @@ use iota_sdk::{ client::{ constants::SHIMMER_COIN_TYPE, request_funds_from_faucet, - secret::{mnemonic::MnemonicSecretManager, SecretManage, SecretManager}, + secret::{mnemonic::MnemonicSecretManager, SecretManager}, }, crypto::keys::bip44::Bip44, - types::block::{ - address::{Address, Bech32Address, Hrp}, - output::BasicOutput, - payload::signed_transaction::TransactionId, - }, + types::block::{address::Bech32Address, output::BasicOutput, payload::signed_transaction::TransactionId}, wallet::{ClientOptions, FilterOptions, SendParams, Wallet}, }; diff --git a/sdk/src/client/api/block_builder/transaction_builder/mod.rs b/sdk/src/client/api/block_builder/transaction_builder/mod.rs index 745f0bd48b..72c0408cdd 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/mod.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/mod.rs @@ -700,8 +700,15 @@ pub(crate) struct OrderedInputs { } impl OrderedInputs { - pub(crate) fn iter(&self) -> OrderedInputsIter<&Address, &InputSigningData> { - self.into_iter() + pub(crate) fn iter_sorted(&self) -> OrderedInputsIter<&Address, &InputSigningData> { + OrderedInputsIter { + queue: self.ed25519.iter().collect(), + other: self.other.iter().map(|(k, v)| (k, v.iter().collect())).collect(), + } + } + + pub(crate) fn iter(&self) -> impl Iterator + Clone { + self.ed25519.iter().chain(self.other.values().flatten()) } pub(crate) fn insert(&mut self, required_address: Address, input: InputSigningData) { @@ -777,18 +784,6 @@ impl + Ord + core::hash::Hash, I: Borrow> I } } -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(), - } - } -} - impl IntoIterator for OrderedInputs { type Item = InputSigningData; type IntoIter = OrderedInputsIter; diff --git a/sdk/src/client/api/block_builder/transaction_builder/remainder.rs b/sdk/src/client/api/block_builder/transaction_builder/remainder.rs index 130ed3754a..ecdd651693 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/remainder.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/remainder.rs @@ -50,7 +50,7 @@ impl TransactionBuilder { return Ok(Some((remainder_address.clone(), None))); } - for input in &self.selected_inputs { + for input in self.selected_inputs.iter() { let required_address = input .output .required_address( diff --git a/sdk/src/client/api/block_builder/transaction_builder/requirement/amount.rs b/sdk/src/client/api/block_builder/transaction_builder/requirement/amount.rs index ad2ae9f372..1abaf3ce74 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/requirement/amount.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/requirement/amount.rs @@ -91,7 +91,7 @@ impl TransactionBuilder { let mut inputs_sdr = HashMap::new(); let mut outputs_sdr = HashMap::new(); - for selected_input in &self.selected_inputs { + for selected_input in self.selected_inputs.iter() { inputs_sum += selected_input.output.amount(); if let Some(sdruc) = sdruc_not_expired(&selected_input.output, self.latest_slot_commitment_id.slot_index()) diff --git a/sdk/src/client/api/block_builder/transaction_builder/requirement/mana.rs b/sdk/src/client/api/block_builder/transaction_builder/requirement/mana.rs index 7b157d67b0..92e159a823 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/requirement/mana.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/requirement/mana.rs @@ -39,7 +39,7 @@ impl TransactionBuilder { if !self.selected_inputs.is_empty() && self.all_outputs().next().is_some() { let inputs = self .selected_inputs - .iter() + .iter_sorted() .map(|i| Input::Utxo(UtxoInput::from(*i.output_id()))); let outputs = self.all_outputs().cloned(); @@ -305,7 +305,7 @@ impl TransactionBuilder { .into() .unwrap_or_else(|| self.burn.as_ref().map_or(true, |b| !b.generated_mana())); - for input in &self.selected_inputs { + for input in self.selected_inputs.iter() { selected_mana += self.total_mana(input, include_generated)?; } diff --git a/sdk/src/client/api/block_builder/transaction_builder/requirement/native_tokens.rs b/sdk/src/client/api/block_builder/transaction_builder/requirement/native_tokens.rs index 5595928a10..b65efdd25c 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/requirement/native_tokens.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/requirement/native_tokens.rs @@ -139,7 +139,7 @@ impl TransactionBuilder { let TokenScheme::Simple(output_foundry_simple_ts) = output_foundry.token_scheme(); let mut initial_creation = true; - for input in &self.selected_inputs { + for input in self.selected_inputs.iter() { if let Output::Foundry(input_foundry) = &input.output { let token_id = output_foundry.token_id(); diff --git a/sdk/tests/client/secret_manager/address_generation.rs b/sdk/tests/client/secret_manager/address_generation.rs index 7ff9584cce..85ef64d7c6 100644 --- a/sdk/tests/client/secret_manager/address_generation.rs +++ b/sdk/tests/client/secret_manager/address_generation.rs @@ -3,26 +3,21 @@ #[cfg(feature = "stronghold")] use crypto::keys::bip39::Mnemonic; -use crypto::keys::bip44::Bip44; +#[cfg(feature = "ledger_nano")] +use iota_sdk::client::secret::ledger_nano::LedgerSecretManager; #[cfg(feature = "stronghold")] use iota_sdk::client::secret::stronghold::StrongholdSecretManager; -#[cfg(feature = "ledger_nano")] -use iota_sdk::client::secret::{ledger_nano::LedgerSecretManager, GenerateAddressOptions}; -#[cfg(feature = "events")] -use iota_sdk::wallet::events::{WalletEvent, WalletEventType}; use iota_sdk::{ client::{ - api::GetAddressesOptions, constants::{IOTA_COIN_TYPE, SHIMMER_COIN_TYPE}, secret::{mnemonic::MnemonicSecretManager, SecretManager}, ClientError, }, - types::block::address::{Hrp, ToBech32Ext}, - wallet::{ClientOptions, Wallet}, + types::block::address::ToBech32Ext, }; use pretty_assertions::assert_eq; -use crate::client::common::{setup, tear_down, DEFAULT_MNEMONIC, NODE_LOCAL}; +use crate::client::common::{setup, tear_down, DEFAULT_MNEMONIC}; #[tokio::test] async fn address_generation_mnemonic() -> Result<(), Box> { diff --git a/sdk/tests/client/secret_manager/mnemonic.rs b/sdk/tests/client/secret_manager/mnemonic.rs index 84cf4e6cbe..aad323aa43 100644 --- a/sdk/tests/client/secret_manager/mnemonic.rs +++ b/sdk/tests/client/secret_manager/mnemonic.rs @@ -2,7 +2,6 @@ // SPDX-License-Identifier: Apache-2.0 use iota_sdk::client::{ - api::GetAddressesOptions, constants::{SHIMMER_COIN_TYPE, SHIMMER_TESTNET_BECH32_HRP}, secret::SecretManager, ClientError, diff --git a/sdk/tests/client/secret_manager/stronghold.rs b/sdk/tests/client/secret_manager/stronghold.rs index bb087859ee..7f304d4b15 100644 --- a/sdk/tests/client/secret_manager/stronghold.rs +++ b/sdk/tests/client/secret_manager/stronghold.rs @@ -2,7 +2,6 @@ // SPDX-License-Identifier: Apache-2.0 use iota_sdk::client::{ - api::GetAddressesOptions, constants::{SHIMMER_COIN_TYPE, SHIMMER_TESTNET_BECH32_HRP}, secret::SecretManager, ClientError, From 2827ddb247f8eb7ff77793f0ab14af63cb6e5a26 Mon Sep 17 00:00:00 2001 From: Alex Coats Date: Thu, 14 Mar 2024 08:41:38 -0400 Subject: [PATCH 10/11] comments --- .../api/block_builder/transaction_builder/mod.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sdk/src/client/api/block_builder/transaction_builder/mod.rs b/sdk/src/client/api/block_builder/transaction_builder/mod.rs index 72c0408cdd..c272777e1e 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/mod.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/mod.rs @@ -739,6 +739,13 @@ impl + Ord + core::hash::Hash, I: Borrow> I type Item = I; fn next(&mut self) -> Option { + // Inputs that are unlocked by Ed25519 addresses go in the queue first + // because they do not need to reference other inputs for their unlocks. + // Each one may have additional dependents which are added to the front of + // the queue to be sorted immediately after the input they depend upon. + // Those can also have dependents which will go after them. + // This creates a tree structure with many to one relationship, which is + // flattened by this loop in insertion order. if let Some(input) = self.queue.pop_front() { // Add associated inputs to the front of the queue match &input.borrow().output { @@ -772,8 +779,11 @@ impl + Ord + core::hash::Hash, I: Borrow> I }; return Some(input); } + // When the queue is empty, just add anything that is left over to the end of the list. if let Some(mut entry) = self.other.first_entry() { if let Some(input) = entry.get_mut().pop_front() { + // Since the structure is a list-of-lists, we need to pop + // the inner list if it's empty. if entry.get().is_empty() { self.other.pop_first(); } From 08f5330891773aa1bbbd28dc09081e4e8c61f315 Mon Sep 17 00:00:00 2001 From: Alex Coats Date: Fri, 15 Mar 2024 09:11:40 -0400 Subject: [PATCH 11/11] remove IntoIter impl --- .../block_builder/transaction_builder/mod.rs | 23 ++++++++----------- .../transaction_builder/requirement/mana.rs | 2 +- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/sdk/src/client/api/block_builder/transaction_builder/mod.rs b/sdk/src/client/api/block_builder/transaction_builder/mod.rs index c56bc673dc..70b1e185d7 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/mod.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/mod.rs @@ -464,7 +464,7 @@ impl TransactionBuilder { let data = PreparedTransactionData { transaction, - inputs_data: self.selected_inputs.into_iter().collect(), + inputs_data: self.selected_inputs.into_sorted_iter().collect(), remainders: self.remainders.data, mana_rewards: self.mana_rewards.into_iter().collect(), }; @@ -696,13 +696,20 @@ pub(crate) struct OrderedInputs { } impl OrderedInputs { - pub(crate) fn iter_sorted(&self) -> OrderedInputsIter<&Address, &InputSigningData> { + pub(crate) fn sorted_iter(&self) -> OrderedInputsIter<&Address, &InputSigningData> { OrderedInputsIter { queue: self.ed25519.iter().collect(), other: self.other.iter().map(|(k, v)| (k, v.iter().collect())).collect(), } } + pub(crate) fn into_sorted_iter(self) -> OrderedInputsIter { + OrderedInputsIter { + queue: self.ed25519, + other: self.other, + } + } + pub(crate) fn iter(&self) -> impl Iterator + Clone { self.ed25519.iter().chain(self.other.values().flatten()) } @@ -789,15 +796,3 @@ impl + Ord + core::hash::Hash, I: Borrow> I None } } - -impl IntoIterator for OrderedInputs { - type Item = InputSigningData; - type IntoIter = OrderedInputsIter; - - fn into_iter(self) -> Self::IntoIter { - OrderedInputsIter { - queue: self.ed25519, - other: self.other, - } - } -} diff --git a/sdk/src/client/api/block_builder/transaction_builder/requirement/mana.rs b/sdk/src/client/api/block_builder/transaction_builder/requirement/mana.rs index 92e159a823..d98500d904 100644 --- a/sdk/src/client/api/block_builder/transaction_builder/requirement/mana.rs +++ b/sdk/src/client/api/block_builder/transaction_builder/requirement/mana.rs @@ -39,7 +39,7 @@ impl TransactionBuilder { if !self.selected_inputs.is_empty() && self.all_outputs().next().is_some() { let inputs = self .selected_inputs - .iter_sorted() + .sorted_iter() .map(|i| Input::Utxo(UtxoInput::from(*i.output_id()))); let outputs = self.all_outputs().cloned();