Rework procedure authentication

[Fumuran]

Packages versions

miden-base: 0.5.0
branch: next

Bug description

The problem appeared during the testing of Foreign Procedure Invocation.

Context:

The native account have some "foreign" procedures, which could be used to get some information from the foreign account. Foreign account, on the other hand, potentially could have no procedures at all.

Edit: my understanding of how FPI works was not correct. The foreign procedure is a part of the foreign account, so during the invocation of execute_foreign_procedure we pass there the hash of the procedure in the foreign account, not native.

To make a successful call to the foreign account it should be provided to the merkle store (and then later will be loaded to the memory).

The bug occurs on the stage of the procedure authentication (here).

The authenticate_procedure procedure works like this

• By the provided procedure root get the index of the account procedure (we have a account procedure map in the Host which stores a procedure index and a corresponding procedure hash).
• Using the obtained index load from the memory the procedure hash and other procedure data.
• Compare the provided proc root and a proc root obtained from the memory to make sure that we load the data of the correct procedure.

What goes wrong

• We get the index of the provided procedure in native account.
• Using the proc index from native account we try to load the procedure data from memory, but in fact we load the procedure data with this index from the current (foreign) account, which could be completely different (unlike in our tests where we were using the same account code for both native and foreign accounts).
• At the stage of verification we get the failed assertion, because we are trying to compare two essentially different procedures.

How can this be reproduced?

Call the foreign procedure which is provided in the native account against the foreign account where this procedure is not provided.

Relevant log output

called Result::unwrap() on an Err value: TransactionExecutorError(ExecuteTransactionProgramFailed(FailedAssertion { clk: RowIndex(3215), err_code: 131083, err_msg: Some("Account procedure is not part of the account code") }))

[bobbinth]

If I understood correctly, we have some incorrect behavior when native and foreign accounts have different interfaces (i.e., expose a different set of procedures).

I think the issue may be related to how we build ProcedureIndexMap in the transaction host. Specifically, right now we build it only for the native account (see here). So, when a foreign account requests an index for a procedure that it has, unless the native account has exactly the same set of procedures, the return info would be wrong and the foreign procedure will fail to authenticate.

Assuming this is the issue, the fix should be to make the ProcedureIndexMap more general and able to handle procedures for multiple accounts. The trickiest thing would be figuring out how to let transaction host know about the foreign accounts (right now this info lives only in the advice provider). One way is to modify TransactionArgs to include AccountHeaders (or maybe even just code roots) for all foreign accounts. But I'm not sure this is the best way yet.

cc @igamigo as this will also be relevant to how the client provides this info during transaction execution.

[igamigo]

I think I'm a bit confused. Originally when testing on the client as I hit this issue the problem was as follows:

• Native account (basic wallet) tries to call foreign account's procedure that native account did not have (this version of the test - don't mind the commented MASM). The error there would look something like:

called `Result::unwrap()` on an `Err` value: TransactionExecutorError(ExecuteTransactionProgramFailed(EventError("Account procedure with root 0x1ef1a9cc88bb5808d9eef908cc79c29ff84c065be7d037d5adf34ead18691810 is not in the advice provider")))

• Andrey asked me to test a scenario where the foreign account did not have the procedure that we were trying to call, but the native account did (essentially this version of the test). This yields the error that is on the issue description:

called `Result::unwrap()` on an `Err` value: TransactionExecutorError(ExecuteTransactionProgramFailed(FailedAssertion { clk: RowIndex(3215), err_code: 131083, err_msg: Some("Account procedure is not part of the account code") }))

• When testing a scenario where both accounts have the same procedures, the call succeeds. I think this is why the tests in miden-base are succeeding right now.

EDIT: I force pushed to my branch and erased commit history. Hopefully the scenarios are understood without looking at the code but otherwise feel free to let me know and we can repro it.

I think this might have the same root cause (not familiar with ProcedureIndexMap), but since the errors are a bit different I thought I'd clarify what I was hitting just in case.

[bobbinth]

My hunch is that this is all due to the issue I described above - just different manifestations of it, and the errors seems to be consistent with it. The question though is how to fix it. One option (which I mentioned above), is to modify TransactionArgs to include headers of foreign accounts. It could look something like this:

pub struct TransactionArgs {
    tx_script: Option<TransactionScript>,
    foreign_accounts: Vec<AccountHeader>,
    note_args: BTreeMap<NoteId, Word>,
    advice_inputs: AdviceInputs,
}

Or if we wanted to keep extra data to the minimum, we could just track foreign account code commitments - but I'm not sure it is worth it.

Then, in the transaction host we'd build the correct map for all accounts and things should work (hopefully).

On the client side, we'd just need to add headers (which I think we already have anyway) to the args and things should work there too.

[bobbinth]

One other option is to introduce a ForeignAccountHeader struct. This could look something like this:

pub struct ForeignAccountHeader {
    account: AccountHeader,
    storage: AccountStorageHeader,
    code: AccountCodeHeader,
}

Where AccountCodeHeader would be a new struct that could look like so:

pub struct AccountCodeHeader {
    commitment: Digest,
    procedures: Vec<AccountProcedureInfo>,
}

Then, TransactionArgs could be:

pub struct TransactionArgs {
    tx_script: Option<TransactionScript>,
    foreign_accounts: Vec<ForeignAccountHeader>,
    note_args: BTreeMap<NoteId, Word>,
    advice_inputs: AdviceInputs,
}

This would mean that the client would just need to populate foreign_accounts with the data it gets from the node.

I think one remaining question is how we actually get account code loaded into the MastForestStore of the transaction executor. Maybe we need to add a separate method on the executor to load code - but I haven't thought through this yet.

[bobbinth]

Closed by #940.