Skip to content

Remove asset_witnesses field from TransactionInputs #2298

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

Merged
bobbinth merged 8 commits into from
Jan 20, 2026
+112 −53
Merged

Remove asset_witnesses field from TransactionInputs #2298

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cfea856
refactor: remove `asset_witnesses` field from `TransactionInputs`
bobbinth Jan 16, 2026
d75af0e
chore: update changelog
bobbinth Jan 16, 2026
41d6978
Merge branch 'next' into bobbin-asset-witness
bobbinth Jan 17, 2026
168f865
feat: add TransactionInputs::read_vault_asset()
bobbinth Jan 18, 2026
7139863
feat: skip fetching asset witnesses if they are in the tx inputs
bobbinth Jan 18, 2026
b6ee9de
chore: update changelog
bobbinth Jan 18, 2026
d26357d
Merge branch 'next' into bobbin-asset-witness
bobbinth Jan 19, 2026
2ef192c
Merge branch 'next' into bobbin-asset-witness
bobbinth Jan 20, 2026
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
26 changes: 14 additions & 12 deletions crates/miden-protocol/src/transaction/inputs/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,6 @@ pub struct TransactionInputs {
tx_args: TransactionArgs,
advice_inputs: AdviceInputs,
foreign_account_code: Vec<AccountCode>,
/// Pre-fetched asset witnesses for note assets and the fee asset.
asset_witnesses: Vec<AssetWitness>,
/// Storage slot names for foreign accounts.
foreign_account_slot_names: BTreeMap<StorageSlotId, StorageSlotName>,
}
Expand Down Expand Up @@ -110,14 +108,20 @@ impl TransactionInputs {
tx_args: TransactionArgs::default(),
advice_inputs: AdviceInputs::default(),
foreign_account_code: Vec::new(),
asset_witnesses: Vec::new(),
foreign_account_slot_names: BTreeMap::new(),
})
}

/// Replaces the transaction inputs and assigns the given asset witnesses.
pub fn with_asset_witnesses(mut self, witnesses: Vec<AssetWitness>) -> Self {
self.asset_witnesses = witnesses;
for witness in witnesses {
self.advice_inputs.store.extend(witness.authenticated_nodes());
let smt_proof = SmtProof::from(witness);
self.advice_inputs
.map
.extend([(smt_proof.leaf().hash(), smt_proof.leaf().to_elements())]);
}
Comment on lines 116 to +123
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter Jan 17, 2026

Choose a reason for hiding this comment

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

I think the approach from #2099 more or less strongly requires that we extend both self.advice_inputs and self.tx_args.advice_inputs. I'm actually not sure how necessary this is (I find this pretty hard to think about now), but it seems it would be more consistent. So I would add the function added in #2274 for this:

/// Extends the advice inputs with the provided inputs.
///
/// This extends both the internal advice inputs and the transaction arguments' advice inputs,
/// ensuring that `self.advice_inputs` is always a subset of `self.tx_args.advice_inputs()`.
(pub) fn extend_advice_inputs(&mut self, advice_inputs: AdviceInputs) {
    self.advice_inputs.extend(advice_inputs.clone());
    self.tx_args.extend_advice_inputs(advice_inputs);
}

And then here do:

let mut tx_adv = TransactionAdviceInputs::default();
witnesses.into_iter().for_each(|witness| tx_adv.add_asset_witness(witness));
self.extend_advice_inputs(tx_adv.into_advice_inputs());

This reuses the existing asset witness -> advice input conversion and we don't duplicate it.

Copy link
Contributor Author

@bobbinth bobbinth Jan 19, 2026

Choose a reason for hiding this comment

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

I think the approach from #2099 more or less strongly requires that we extend both self.advice_inputs and self.tx_args.advice_inputs.

I'm not sure this is needed because TransactionInputs::set_advice_inputs() should already take care of this. But I agree the whole structure is confusing. I hope that with the structure I described in #1286 (comment), we'd be able to clarify the structure somewhat.

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter Jan 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure this is needed because TransactionInputs::set_advice_inputs() should already take care of this.

Technically yes, but it's not obvious that set_advice_inputs is called later (only after tx execution) and so this leaves this "invariant" temporarily broken. Not sure if this is a real problem, and I think it's fine to address this in #1286 or #2293.

Nit: I would prefer using TransactionAdviceInputs here so we use the lower-level leaf -> advice inputs conversion implementation (TransactionAdviceInputs::add_asset_witness) rather than move it outside that type.

Copy link
Contributor Author

@bobbinth bobbinth Jan 20, 2026

Choose a reason for hiding this comment

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

Nit: I would prefer using TransactionAdviceInputs here so we use the lower-level leaf -> advice inputs conversion implementation (TransactionAdviceInputs::add_asset_witness) rather than move it outside that type.

I've actually removed TransactionAdviceInputs::add_asset_witness() in this PR since it was no longer needed. I think we can bring it back if needed depending on how we address #1286 and #2293.


self
}

Expand Down Expand Up @@ -210,11 +214,6 @@ impl TransactionInputs {
&self.foreign_account_code
}

/// Returns the pre-fetched witnesses for note and fee assets.
pub fn asset_witnesses(&self) -> &[AssetWitness] {
&self.asset_witnesses
}

/// Returns the foreign account storage slot names.
pub fn foreign_account_slot_names(&self) -> &BTreeMap<StorageSlotId, StorageSlotName> {
&self.foreign_account_slot_names
Expand Down Expand Up @@ -263,6 +262,12 @@ impl TransactionInputs {
}

/// Reads the vault asset witnesses for the given account and vault keys.
///
/// # Errors
/// Returns an error if:
/// - A Merkle tree with the specified root is not present in the advice data of these inputs.
/// - Witnesses for any of the requested assets are not in the specified Merkle tree.
/// - Construction of the Merkle path or the leaf node for the witness fails.
pub fn read_vault_asset_witnesses(
&self,
vault_root: Word,
Expand Down Expand Up @@ -432,7 +437,6 @@ impl Serializable for TransactionInputs {
self.tx_args.write_into(target);
self.advice_inputs.write_into(target);
self.foreign_account_code.write_into(target);
self.asset_witnesses.write_into(target);
self.foreign_account_slot_names.write_into(target);
}
}
Expand All @@ -448,7 +452,6 @@ impl Deserializable for TransactionInputs {
let tx_args = TransactionArgs::read_from(source)?;
let advice_inputs = AdviceInputs::read_from(source)?;
let foreign_account_code = Vec::<AccountCode>::read_from(source)?;
let asset_witnesses = Vec::<AssetWitness>::read_from(source)?;
let foreign_account_slot_names =
BTreeMap::<StorageSlotId, StorageSlotName>::read_from(source)?;

Expand All @@ -460,7 +463,6 @@ impl Deserializable for TransactionInputs {
tx_args,
advice_inputs,
foreign_account_code,
asset_witnesses,
foreign_account_slot_names,
})
}
Expand Down
4 changes: 0 additions & 4 deletions crates/miden-protocol/src/transaction/inputs/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ fn test_read_foreign_account_inputs_missing_data() {
tx_args: crate::transaction::TransactionArgs::default(),
advice_inputs: crate::vm::AdviceInputs::default(),
foreign_account_code: Vec::new(),
asset_witnesses: Vec::new(),
foreign_account_slot_names: BTreeMap::new(),
};

Expand Down Expand Up @@ -139,7 +138,6 @@ fn test_read_foreign_account_inputs_with_storage_data() {
tx_args: crate::transaction::TransactionArgs::default(),
advice_inputs,
foreign_account_code: vec![code],
asset_witnesses: Vec::new(),
foreign_account_slot_names,
};

Expand Down Expand Up @@ -261,7 +259,6 @@ fn test_read_foreign_account_inputs_with_proper_witness() {
tx_args: crate::transaction::TransactionArgs::default(),
advice_inputs,
foreign_account_code: vec![code],
asset_witnesses: Vec::new(),
foreign_account_slot_names: BTreeMap::new(),
};

Expand Down Expand Up @@ -348,7 +345,6 @@ fn test_transaction_inputs_serialization_with_foreign_slot_names() {
tx_args: crate::transaction::TransactionArgs::default(),
advice_inputs: crate::vm::AdviceInputs::default(),
foreign_account_code: Vec::new(),
asset_witnesses: Vec::new(),
foreign_account_slot_names,
};

Expand Down
14 changes: 0 additions & 14 deletions crates/miden-protocol/src/transaction/kernel/advice_inputs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,9 @@ use alloc::vec::Vec;
use miden_processor::AdviceMutation;

use crate::account::{AccountHeader, AccountId, PartialAccount};
use crate::asset::AssetWitness;
use crate::block::account_tree::AccountWitness;
use crate::crypto::SequentialCommit;
use crate::crypto::merkle::InnerNodeInfo;
use crate::crypto::merkle::smt::SmtProof;
use crate::note::NoteAttachmentContent;
use crate::transaction::{
AccountInputs,
Expand Down Expand Up @@ -75,10 +73,6 @@ impl TransactionAdviceInputs {
}
}

tx_inputs.asset_witnesses().iter().for_each(|asset_witness| {
inputs.add_asset_witness(asset_witness.clone());
});

// Extend with extra user-supplied advice.
inputs.extend(tx_inputs.tx_args().advice_inputs().clone());

Expand Down Expand Up @@ -309,14 +303,6 @@ impl TransactionAdviceInputs {
self.extend_merkle_store(witness.authenticated_nodes());
}

/// Adds an asset witness to the advice inputs.
fn add_asset_witness(&mut self, witness: AssetWitness) {
self.extend_merkle_store(witness.authenticated_nodes());

let smt_proof = SmtProof::from(witness);
self.extend_map([(smt_proof.leaf().hash(), smt_proof.leaf().to_elements())]);
}

// NOTE INJECTION
// --------------------------------------------------------------------------------------------

Expand Down
33 changes: 20 additions & 13 deletions crates/miden-tx/src/executor/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,25 +318,32 @@ where
AccountProcedureIndexMap::new([tx_inputs.account().code()]);

let initial_fee_asset_balance = {
let vault_root = tx_inputs.account().vault().root();
let native_asset_id = tx_inputs.block_header().fee_parameters().native_asset_id();
let fee_asset_vault_key = AssetVaultKey::from_account_id(native_asset_id)
.expect("fee asset should be a fungible asset");

let fee_asset_witness = tx_inputs
.asset_witnesses()
.iter()
.find_map(|witness| witness.find(fee_asset_vault_key));

match fee_asset_witness {
Some(Asset::Fungible(fee_asset)) => fee_asset.amount(),
Some(Asset::NonFungible(_)) => {
return Err(TransactionExecutorError::FeeAssetMustBeFungible);
},
// If the witness does not contain the asset, its balance is zero.
None => 0,
// extract the fee asset amount from the transaction inputs
let fee_asset_witnesses =
tx_inputs.read_vault_asset_witnesses(vault_root, [fee_asset_vault_key].into());
if let Ok(witnesses) = fee_asset_witnesses {
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter Jan 17, 2026

Choose a reason for hiding this comment

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

The fee asset should always be in the advice inputs if things are going correctly: Even if we add logic to skip the pre-fetching of asset witnesses if they are already present, prepare_tx_inputs should basically guarantee that by the end of it, the fee asset witness is in tx_inputs.advice_inputs. So, I think we should return the error if the fee asset witness cannot be found:

// extract the fee asset amount from the transaction inputs
let fee_asset_witnesses = tx_inputs
    .read_vault_asset_witnesses(vault_root, [fee_asset_vault_key].into())
    .map_err(|source| TransactionExecutorError::FailedToReadFeeAssetWitness {
        native_asset_id,
        source,
    })?;
// This could be made nicer with a better TransactionInputs API as you mentioned in another comment.
let fee_asset_witness = fee_asset_witnesses
    .into_iter()
    .next()
    .expect("read_vault_asset_witnesses should have returned an error if the asset was not found");
match fee_asset_witness.find(fee_asset_vault_key) {
    Some(Asset::Fungible(fee_asset)) => fee_asset.amount(),
    Some(Asset::NonFungible(_)) => {
        return Err(TransactionExecutorError::FeeAssetMustBeFungible);
    },
    // If the witness does not contain the asset, its balance is zero.
    None => 0,
}

with a new error variant:

#[error(
    "failed to read fee asset witness with account ID {native_asset_id} from transaction inputs"
)]
FailedToReadFeeAssetWitness {
    native_asset_id: AccountId,
    source: TransactionInputsExtractionError,
},

Copy link
Contributor Author

@bobbinth bobbinth Jan 19, 2026

Choose a reason for hiding this comment

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

I did something similar in 168f865 - but let me know if you'd prefer your approach.

// we requested one witness and if there was no error only one witness should have
// been returned
assert_eq!(witnesses.len(), 1, "expected exactly one witness");
match witnesses[0].find(fee_asset_vault_key) {
Some(Asset::Fungible(fee_asset)) => fee_asset.amount(),
Some(Asset::NonFungible(_)) => {
return Err(TransactionExecutorError::FeeAssetMustBeFungible);
},
// If the witness does not contain the asset, its balance is zero.
None => 0,
}
} else {
// if read_vault_asset_witnesses() returned an error, we assume that the witness
// was not found because because otherwise the read request was valid
0
}
};

let host = TransactionExecutorHost::new(
tx_inputs.account(),
input_notes.clone(),
Expand Down
Loading