feat: remove asset_witnesses field from TransactionInputs by Farukest · Pull Request #2274 · 0xMiden/protocol

Farukest · 2026-01-14T12:26:23Z

Summary

Removes the asset_witnesses field from TransactionInputs struct.

Changes

Remove asset_witnesses field from TransactionInputs - witnesses are now added directly to advice_inputs via with_asset_witnesses()
Skip fetching witnesses that already exist in advice inputs (per @PhilippGackstatter's suggestion)
Update prepare_tx_inputs to return (TransactionInputs, u64) tuple
Update notes_checker for new signature

Closes #2261

PhilippGackstatter

Looks good to me!

I left some small comments about the structure, but directionally this looks great.

crates/miden-protocol/src/transaction/inputs/mod.rs

crates/miden-tx/src/executor/mod.rs

Address review feedback: - Keep asset_witnesses field in TransactionInputs (revert removal) - Refactor with_asset_witnesses() to use TransactionAdviceInputs::add_asset_witness() - Restore data extractor methods (read_storage_map_witness, etc.) - Add explicit error handling for non-fungible fee assets - Make account_id_map_key() public in TransactionAdviceInputs Closes 0xMiden#2261

PhilippGackstatter

Thanks for the updates! I left one comment that may be difficult to address, but I hope it's clear enough. Let me know if you need help.

crates/miden-protocol/src/transaction/kernel/advice_inputs.rs

crates/miden-tx/src/executor/mod.rs

Farukest · 2026-01-16T11:46:52Z

Looks good to me!

I left some small comments about the structure, but directionally this looks great.

Thank you for detailed guide. I handled all @PhilippGackstatter . needs review. let me know if anything I missed :)

PhilippGackstatter

Thank you! I think we're pretty close now 😃

crates/miden-protocol/src/transaction/inputs/mod.rs

crates/miden-tx/src/executor/notes_checker.rs

Farukest · 2026-01-16T14:44:42Z

I think we do need the correct fee balance here. I think we just don't have a test that uses the checker with non-zero fees, but if we did, I expect we would run into TransactionExecutorError::InsufficientFee.

So, I think we should pass in the initial_fee_balance. We'd have to pass it around quite a bit here so maybe it make sense to create a small internal wrapper in crates/miden-tx/src/executor/notes_checker.rs that wraps TransactionInputs and initial_fee_balance: u64, wdyt?

@PhilippGackstatter now all must be okay. still I'm ready to change if you need anything else 😅

…ce extraction

PhilippGackstatter

Looks good to me! Thanks for your work, and apologies for the back and forth. There's one issue that's causing the test failures - left a comment.

PhilippGackstatter · 2026-01-16T15:27:55Z

crates/miden-protocol/src/transaction/inputs/mod.rs

+        // Copy the tx_args advice inputs to self.advice_inputs first, so that any data
+        // from a previous execution (e.g., during re-execution) is preserved.
+        self.advice_inputs.extend(tx_args.advice_inputs().clone());


I'm not exactly sure why, but this line cause the test failures, e.g. in test_nested_fpi_native_account_invocation. Let's revert this.

Looks good to me! Thanks for your work, and apologies for the back and forth. There's one issue that's causing the test failures - left a comment.

No problem at all. I like work for Miden fun and educational :)
will handle now

bobbinth · 2026-01-16T21:07:06Z

crates/miden-protocol/src/transaction/inputs/mod.rs

    /// Pre-fetched asset witnesses for note assets and the fee asset.
    asset_witnesses: Vec<AssetWitness>,


Wasn't one of the objects to remove the asset_witnesses fields from TransactionInputs?

I created #2298 as an alternative.

what should I do now @bobbinth .. wait ? :)

Let's get some feedback from @PhilippGackstatter. Depending on that, we can decide which approach to take.

I think these PRs are mostly doing the same now, but #2298 handles the initial fee balance in a less intrusive way and avoids the need for PreparedTransactionInputs, so I think that's the better way to go.
Thank you for providing a basis to work from @Farukest!

I think these PRs are mostly doing the same now, but #2298 handles the initial fee balance in a less intrusive way and avoids the need for PreparedTransactionInputs, so I think that's the better way to go. Thank you for providing a basis to work from @Farukest!

Hi @PhilippGackstatter, @bobbinth should I close #2274 since this one is moving forward ? Also, would appreciate a co-author tag if possible. Thanks in advance :)

Yes, I'll close this now. Thank you for your help on this! I added the co-author tag - hopefully, it worked as intended.

bobbinth · 2026-01-20T00:31:00Z

Superseded by #2298.

Farukest force-pushed the feat/remove-asset-witnesses-field branch 2 times, most recently from 950514f to 74af7e5 Compare January 14, 2026 13:18

PhilippGackstatter reviewed Jan 15, 2026

View reviewed changes

crates/miden-protocol/src/transaction/inputs/mod.rs Outdated Show resolved Hide resolved

crates/miden-protocol/src/transaction/inputs/mod.rs Show resolved Hide resolved

crates/miden-tx/src/executor/mod.rs Outdated Show resolved Hide resolved

Farukest force-pushed the feat/remove-asset-witnesses-field branch from 74af7e5 to 187297f Compare January 15, 2026 13:38

Farukest force-pushed the feat/remove-asset-witnesses-field branch from 187297f to 238c044 Compare January 15, 2026 13:51

PhilippGackstatter mentioned this pull request Jan 15, 2026

feat(validator): FPI support in data store 0xMiden/node#1493

Merged

Merge branch 'next' into feat/remove-asset-witnesses-field

af9635c

PhilippGackstatter reviewed Jan 16, 2026

View reviewed changes

crates/miden-protocol/src/transaction/kernel/advice_inputs.rs Outdated Show resolved Hide resolved

crates/miden-tx/src/executor/mod.rs Outdated Show resolved Hide resolved

Farukest force-pushed the feat/remove-asset-witnesses-field branch from 476bd07 to 4c4e5e5 Compare January 16, 2026 11:44

PhilippGackstatter reviewed Jan 16, 2026

View reviewed changes

PhilippGackstatter mentioned this pull request Jan 16, 2026

Add API for transaction re-execution #2293

Open

Farukest force-pushed the feat/remove-asset-witnesses-field branch from 4c4e5e5 to fd1087a Compare January 16, 2026 14:42

address review comments: refactor with_asset_witnesses, fix fee balan…

8a79662

…ce extraction

Farukest force-pushed the feat/remove-asset-witnesses-field branch from fd1087a to 8a79662 Compare January 16, 2026 14:47

PhilippGackstatter approved these changes Jan 16, 2026

View reviewed changes

fix: revert advice_inputs.extend in set_tx_args_inner

3c67348

bobbinth reviewed Jan 16, 2026

View reviewed changes

bobbinth mentioned this pull request Jan 16, 2026

Remove asset_witnesses field from TransactionInputs #2298

Merged

Farukest force-pushed the feat/remove-asset-witnesses-field branch from 2b424e5 to 3c67348 Compare January 18, 2026 10:46

bobbinth closed this Jan 20, 2026

		/// Pre-fetched asset witnesses for note assets and the fee asset.
		asset_witnesses: Vec<AssetWitness>,

Conversation

Farukest commented Jan 14, 2026

Summary

Changes

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Farukest commented Jan 16, 2026

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Farukest commented Jan 16, 2026

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbinth commented Jan 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants