FPI: foreign account data loader

[Fumuran]

WiP

TODO:

• Update docs
• Update changelog
• Implement tests

[bobbinth]

I don't mind these being separate - but a coupe of comments:

• I would probably create two separate procedures - e.g., something like load_account_storage_data and load_account_procedure_data.
• This code is probably very similar to what we do in the prologue for the native account. Is there a way to use these procedures in both places?
• Why do we use exec.mem::pipe_words_to_memory rather than exec.mem::pipe_double_words_to_memory here? As far as I know, both storage slots and procedures are take up 2 words.

[Fumuran]

I think the best option will be to move the validate_account_procedures and validate_account_storage_slots procedures from the prologue to the memory and use them in both places: in prologue and in api. That will solve the highlighted problems.

[Fumuran]

For some reason api.rs file in the tx-prover keeps generating with strange formatting, on which rustfmt is complaining.

[PhilippGackstatter]

For some reason api.rs file in the tx-prover keeps generating with strange formatting, on which rustfmt is complaining.

prost-build does its own formatting with prettyplease by default and the format feature is enabled by default, too. So since that code is already formatted plus generated code tends not to be looked at very much it seems fine to just skip the formatting from rustfmt for the generated module. You could just add this in bin/tx-prover/src/server/mod.rs:

#[rustfmt::skip]
pub mod generated;

[Fumuran]

So since that code is already formatted plus generated code tends not to be looked at very much it seems fine to just skip the formatting from rustfmt for the generated module. You could just add this in bin/tx-prover/src/server/mod.rs:

I was thinking about it, It was just strange to me that current version of this module in next branch has a different formatting, which is accepted by the CI formatting checker. Somehow Santiago managed to make the proper formatting, so I should be able to do so too.

[bobbinth]

Thank you! Looks good! Not a full review, but I left some comments inline.

[Fumuran]

Well, it turned out that implementation of the foreign account verification took only a few more lines, so I included it in this PR.

[PhilippGackstatter]

I was thinking about it, It was just strange to me that current version of this module in next branch has a different formatting, which is accepted by the CI formatting checker. Somehow Santiago managed to make the proper formatting, so I should be able to do so too.

If you force a rebuild of that file on next it also does not conform to what rustfmt expects. So I think the proper formatting on next is just the result of make format. Actually, the tx_kernel_errors.rs has the same issue now and it keeps regenerating on next and shows up as a changed file in git status, which is a bit annoying. I think we should probably add a rustfmt::skip to both of these modules. Wdyt 🙂 ?

[bobbinth]

I think we should probably add a rustfmt::skip to both of these modules.

Yeah - for auto-generated files I think we should skip the formatting so that they don't change as rustfmt rules get updated.

[bobbinth]

Looks good! Thank you! I left some more comments inline - most of them are very minor.

[bobbinth]

After this PR is merged, let's make a small follow-up PR to make this consistent with the storage loader (e.g., not pass num_procs via the advice map).

[bobbinth]

Now that this PR is merged, @Fumuran, could you make a small PR to address the above comment?

Also, should we close the original FPI issue and create a new one which describes what changes need to be made after handling of dynamic procedure invocation is fixed in miden-vm?

[Fumuran]

Yep, sure.

Yes, I think it will be better. I'll create an issue for that.

[Fumuran]

Created the #922 issue.

[bobbinth]

Looks good! Thank you! I left one optional nit inline.