Skip to content

refactor: Change way of providing code and inputs to executor #1229

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.

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
merged 36 commits into from
Apr 22, 2025

Conversation

igamigo
Copy link
Collaborator

@igamigo igamigo commented Mar 19, 2025

Closes #938

Stale comments - `TransactionHost` now receives an `Arc` instead of `TransactionMastStore`. Here, I tried making `MastForestStore` a subtrait of `DataStore`. This approach did not work too well because we want `TransactionExecutor` to hold a `DataStore` trait object wrapped over an `Arc`, but `TransactionHost` to have a `MastForestStore`. However, trait upcasting coercion is unstable ([until 1.86 at least](https://github.com/rust-lang/rust/pull/134367)) so we cannot easily do that there. I thought this would be simple-ish to work around, but anything that did not involve unsafe code implied the trait becoming non object-safe. - We could make `TransactionHost` hold a `DataStore` trait object instead (that could be shared with the executor), but it doesn't really make sense in the context of proving transactions. - Another consequence of this change is that the caller is now fully responsible for loading account and transaction code. This removes the ambiguity discussed in this issue: https://github.com//issues/938, but may be far from what @bobbinth had in mind. As long as we want to abstract over a `MastForestStore`, loading the code within `execute_transaction()` is not really possible.

A bunch of test code was changed but the main changes in this PR involve:

  • Changing DataStore to now imply MastForestStore.
    • As part of this, I upgraded Rust to 1.86. This is needed for trait upcasting the executor's DataStore into the transaction host's MastForestStore.
  • I've changed the executor API to take InputNotes as opposed to a list of note IDs so that the executor can check against the executing account interface.
    • Due to this, get_transaction_inputs() can no longer return a full TransactionInputs, since there is no note data to instantiate an object. For now, I've mad this return a tuple of structs.
  • I've added a ForeignAccountInputs struct, and a list of ForeignTransactionInputs to TransactionArgs. This way, advice inputs related to foreign accounts are created internally and do not need to be added externally. With this, there is also no need to pass a separate AccountCode commitment list to create the internal AccountProcedureIndexMap.

@igamigo igamigo force-pushed the igamigo-refactor-executor branch from 9ad0bdb to 8c59e66 Compare March 19, 2025 04:10
@igamigo igamigo marked this pull request as ready for review April 7, 2025 21:17
@igamigo igamigo added the no docs This PR does not require an update to documentation files label Apr 7, 2025
@igamigo igamigo requested review from PhilippGackstatter, Fumuran and bobbinth and removed request for PhilippGackstatter and Fumuran April 7, 2025 21:17
@@ -71,6 +71,11 @@ impl TransactionMastStore {
self.insert(note.note().script().mast().clone());
}

// load foreign account's MAST forests
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should update the function's documentation with this new behaviour.

@igamigo igamigo linked an issue Apr 8, 2025 that may be closed by this pull request
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for the updates.

@@ -450,6 +450,8 @@ pub enum TransactionInputError {
InputNoteNotInBlock(NoteId, BlockNumber),
#[error("account ID computed from seed is invalid")]
InvalidAccountIdSeed(#[source] AccountIdError),
#[error("merkle path for {0} is invalid")]
InvalidMerklePath(String, #[source] MerkleError),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
InvalidMerklePath(String, #[source] MerkleError),
InvalidMerklePath(Box<str>, #[source] MerkleError),

Nit: We could box the string to save 8 bytes. This is the largest variant in the enum now and dictates its stack size. Just requires adding an .into() after the format! where it is used.

@@ -23,8 +23,8 @@ pub trait DataStore: MastForestStore {
/// specified ID and consuming input notes created in blocks in the input `ref_blocks` set.
///
/// The highest block number in `ref_blocks` will be the transaction reference block. In
/// general, it is recommended that bock_ref corresponds to the latest block available in
/// the data store.
/// general, it is recommended that the refernece corresponds to the latest block available
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// general, it is recommended that the refernece corresponds to the latest block available
/// general, it is recommended that the reference corresponds to the latest block available

Comment on lines +42 to +43
#[test]
fn test_fpi_anchoring_validations() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding these tests!

Comment on lines 59 to 62
pub fn execute_code(&self, code: &str) -> Result<Process, ExecutionError> {
pub fn execute_code(
&self,
code: &str,
assembler: Assembler,
) -> Result<Process, ExecutionError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you already refactored all usages now, but I think I would prefer this:

// Assembles with TransactionKernel::testing_assembler
pub fn execute_code(&self, code: &str) -> ...
pub fn execute_code_with_assembler(&self, code: &str, assembler: Assembler) -> ...

Mostly because convenience is nice when writing tests. If it's not too much work to refactor, I'd prefer this. If it is, we could just rename the current method to execute_code_with_assembler and add execute_code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, true. I added the function and changed the calls.

Comment on lines -627 to +636
let last_block = self.blocks.last().expect("one block should always exist");
let epoch_block_number = self
.blocks
.last()
.expect("one block should always exist")
.header()
.epoch_block_num();
let account_id_anchor =
self.blocks.get(epoch_block_number.as_usize()).unwrap().header();
account_builder =
account_builder.anchor(AccountIdAnchor::try_from(last_block.header()).unwrap());
account_builder.anchor(AccountIdAnchor::try_from(account_id_anchor).unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this. I guess we never created new accounts before where the latest block was not also an epoch block.

Copy link
Contributor

@Fumuran Fumuran left a comment

Choose a reason for hiding this comment

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

Looks great! There's not much I can add, just a one small nit.

@@ -72,7 +74,8 @@ pub struct TransactionContextBuilder {
advice_inputs: AdviceInputs,
authenticator: Option<MockAuthenticator>,
expected_output_notes: Vec<Note>,
foreign_account_codes: Vec<AccountCode>,
foreign_accounts: Vec<ForeignAccountInputs>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Not a strong preference, but I would use foreign_account_inputs here, so as not to confuse it with actual foreign accounts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, also changed the other similar occurrences

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a few comments inline. I think the main one is that we need to rebase/update this PR from the latest next.

@igamigo
Copy link
Collaborator Author

igamigo commented Apr 21, 2025

@bobbinth I think this can be reviewed and I can push the refactors I mentioned to a different branch.
As a refresher, the idea was to make ForeignAccountInputs into a struct with two different components, one specifically for account-related data which has all the headers and any storage map proof (essentially PartialAccount as suggested here) and an AccountWitness component. Or maybe it can be tackled as part of the #1296 refactor as well.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left two small comments inline.

I think this can be reviewed and I can push the refactors I mentioned to a different branch.

Yes, let's do that in a different PR.

@bobbinth bobbinth merged commit 2b1677b into next Apr 22, 2025
16 checks passed
@bobbinth bobbinth deleted the igamigo-refactor-executor branch April 22, 2025 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no docs This PR does not require an update to documentation files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revisit how transaction inputs are provided to the executor
5 participants