fix: advice inputs in TransactionInputs not being propagated

[PhilippGackstatter]

Fixes advice inputs in TransactionInputs not being propagated through to transaction execution.

When a transaction finishes execution, its final advice inputs, including generated signatures are set in the TransactionInputs::advice_inputs field:

https://github.com/0xMiden/miden-base/blob/d52c65e85949b0b85b38fef04b2e86ab5cbe281a/crates/miden-tx/src/executor/mod.rs#L386-L390

However, the transaction execution API is:

pub async fn execute_transaction(
        &self,
        account_id: AccountId,
        block_ref: BlockNumber,
        notes: InputNotes<InputNote>,
        tx_args: TransactionArgs,
    ) -> Result<ExecutedTransaction, TransactionExecutorError> {

and the advice inputs from the TransactionInputs are not passed to this function (because there is no obvious way to do so).

This PR removes the advice_inputs field in the TransactionInputs and instead uses the field in TransactionArgs, which can be provided to this API. For the same reason, this fails re-execution on the node, which doesn't use the actual advice inputs from TransactionInputs: https://github.com/0xMiden/miden-node/blob/9bac0c11c32788e6dc1fd8376c5f5f55d9b45927/crates/rpc/src/server/validator.rs#L35-L40

With this change, the usage in the node should automatically work, cc @Mirko-von-Leipzig.

An update to the TransactionContext was also required, to propagate the advice inputs from the transaction args through to the new transaction args we're building. I think this chain of inputs and args is pretty hard to understand and we clearly missed it before and imo would be worth taking another look at.

There is one test I had to ignore because it uses custom advice stack input in some way that now fails, and I don't know exactly why. I believe this is only related to tests, and is unlikely to affect anything in practice, so I think it should be ok to look at this in another PR.

closes #2092

[Mirko-von-Leipzig]

Suggested change

	// Start with empty advice inputs so the advice stack is correctly build.
	// Start with empty advice inputs so the advice stack is correctly built.

[igamigo]

Looks great!

I think this chain of inputs and args is pretty hard to understand and we clearly missed it before and imo would be worth taking another look at.

Agreed

[bobbinth]

I've spent some time thinking about this, and I wonder if a slightly different approach would work better here. I like the conceptual separation between TransactionInputs.tx_args and TransactionInputs.advice_inputs: the former is what is provided by the user, and the latter captures the state of the advice provider at the end of transaction execution.

There is some overlap between the two: TransactionInputs.tx_args.advice_inputs should be a strict subset of TransactionInputs.advice_inputs - but we have #1286 for this.

The main issue with the current approach is that TransactionInputs was designed for proving transactions rather than re-execution - so, there was no easy way to pass TransactionInputs.advice_inputs to the TransactionExecutor::execute_transaction() method. One way to get around this is implemented in this PR.

Another approach is to add something like TransactionInputs::to_args() method. This method would take the internal tx_args, and append TransactionInputs.advice_inputs into the arg's advice inputs. Then, in the node, we'd just call TransactionInputs::to_args() to get the args needed for transaction re-execution.

[Mirko-von-Leipzig]

I've spent some time thinking about this, and I wonder if a slightly different approach would work better here. I like the conceptual separation between TransactionInputs.tx_args and TransactionInputs.advice_inputs: the former is what is provided by the user, and the latter captures the state of the advice provider at the end of transaction execution.

There is some overlap between the two: TransactionInputs.tx_args.advice_inputs should be a strict subset of TransactionInputs.advice_inputs - but we have #1286 for this.

The main issue with the current approach is that TransactionInputs was designed for proving transactions rather than re-execution - so, there was no easy way to pass TransactionInputs.advice_inputs to the TransactionExecutor::execute_transaction() method. One way to get around this is implemented in this PR.

Another approach is to add something like TransactionInputs::to_args() method. This method would take the internal tx_args, and append TransactionInputs.advice_inputs into the arg's advice inputs. Then, in the node, we'd just call TransactionInputs::to_args() to get the args needed for transaction re-execution.

I think the main requirement would from our end would be that its difficult to "hold wrong" aka it should be obvious what is required via types/IDE.

[igamigo]

I'm not fully convinced it's much better than the current approach but it does sound more consistent with the conceptual separation between args and inputs, where the args are left untouched and inputs get modified throughout the execution. I took a look at the code to review a few questions I had and from the outside it may also still be a little confusing that the transaction "inputs" structure gets modified to hold important data (it also has a bunch of mutators which seem have some degree of overlap); aka it's not clear at first sight what is static and what gets mutated at what point necessarily.

I think the main requirement would from our end would be that its difficult to "hold wrong" aka it should be obvious what is required via types/IDE.

This could probably be solved fairly easily if we name the method something like TransactionInputs::to_reexecution_args()/TransactionInputs::to_args_for_reexecution() and/or add a newtype for this? My understanding is that this would not be used anywhere else for now so better to make it as explicit as possible.

[PhilippGackstatter]

I think the main requirement would from our end would be that its difficult to "hold wrong" aka it should be obvious what is required via types/IDE.

I agree with this. I think the main reason we didn't catch this is what @igamigo touched on, i.e. it's not clear what is being mutated vs left untouched. The TransactionInputs::to_args approach doesn't seem clearer.

The change in this PR was the quickest reasonable fix I could think of, so there may be a better overall structure for this. I also agree that the conceptual separation of the two advice inputs makes sense, but I'd probably try to work on this separately.

[PhilippGackstatter]

In the first iteration of the PR this overwrote the advice inputs. I now changed it to extend it, which makes the test_nested_fpi_cyclic_invocation test pass now (and I removed the ignore).

The test was failing during the LocalTransactionProver::prove call, because the custom advice stack inputs the test was setting on the advice inputs got overwritten after the tx execution by the call to with_advice_inputs in build_executed_transaction. So the custom advice stack inputs were no longer there at the time of proving. Thanks @Fumuran for the hint!

[bobbinth]

I opened an alternative approach in #2099 which is different from the alternative I proposed earlier, and conceptually is closer to this PR - though, it does modify fewer files.

[bobbinth]

Superseded by #2099.