Skip to content
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

Jump to bottom

Better input ordering #2175

Merged
merged 15 commits into from
Mar 20, 2024
Merged

Better input ordering #2175

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
082fca5
Order inputs on selection instead of sorting later
Mar 12, 2024
62c4467
update comments
Mar 12, 2024
8f48a49
Merge branch '2.0' into feat/input-order
Mar 12, 2024
0472c8f
Use a custom collection type for order
Mar 13, 2024
e7d0dac
optimize iters
Mar 13, 2024
3e96e37
use others in iter
Mar 13, 2024
1feddf3
Merge branch '2.0' into feat/input-order
Mar 13, 2024
589b35e
reverse params
Mar 13, 2024
5c87649
remove address clones
Mar 13, 2024
e70e9d7
back it up
Mar 13, 2024
4d6179d
cleanup and don't sort all the time
Mar 13, 2024
2827ddb
comments
Mar 14, 2024
c835f1c
Merge branch '2.0' into feat/input-order
Mar 14, 2024
08f5330
remove IntoIter impl
Mar 15, 2024
968f72f
Merge branch '2.0' into feat/input-order
Mar 19, 2024
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
100 changes: 47 additions & 53 deletions sdk/src/client/api/block_builder/transaction_builder/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand All @@ -39,7 +38,7 @@ use crate::{
signed_transaction::{Transaction, TransactionCapabilities, TransactionCapabilityFlag},
TaggedDataPayload,
},
protocol::{CommittableAgeRange, ProtocolParameters},
protocol::ProtocolParameters,
slot::{SlotCommitmentId, SlotIndex},
},
};
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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<Input> = Vec::new();
let mut context_inputs = self
.bic_context_inputs
Expand All @@ -433,7 +425,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());
Expand Down Expand Up @@ -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(),
};
Expand All @@ -477,30 +469,30 @@ impl TransactionBuilder {
fn select_input(&mut self, input: InputSigningData) -> Result<Option<&Output>, 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
// - the issuer feature doesn't need to be verified as the chain is not new
// - 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)? {
log::debug!("Adding {requirement:?} from input {:?}", input.output_id());
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`].
Expand Down Expand Up @@ -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<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| {
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.
Thoralf-M marked this conversation as resolved.
Show resolved Hide resolved
.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())
Thoralf-M marked this conversation as resolved.
Show resolved Hide resolved
{
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()
Expand All @@ -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
Expand All @@ -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)
}
}
6 changes: 0 additions & 6 deletions sdk/src/client/api/block_builder/transaction_builder/requirement/mana.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Loading