-
Notifications
You must be signed in to change notification settings - Fork 77
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
Conversation
9ad0bdb
to
8c59e66
Compare
@@ -71,6 +71,11 @@ impl TransactionMastStore { | |||
self.insert(note.note().script().mast().clone()); | |||
} | |||
|
|||
// load foreign account's MAST forests |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
crates/miden-objects/src/errors.rs
Outdated
@@ -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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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 |
#[test] | ||
fn test_fpi_anchoring_validations() { |
There was a problem hiding this comment.
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!
pub fn execute_code(&self, code: &str) -> Result<Process, ExecutionError> { | ||
pub fn execute_code( | ||
&self, | ||
code: &str, | ||
assembler: Assembler, | ||
) -> Result<Process, ExecutionError> { |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
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()); |
There was a problem hiding this comment.
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.
There was a problem hiding this 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>, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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
.
@bobbinth I think this can be reviewed and I can push the refactors I mentioned to a different branch. |
There was a problem hiding this 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.
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:
DataStore
to now implyMastForestStore
.DataStore
into the transaction host'sMastForestStore
.InputNotes
as opposed to a list of note IDs so that the executor can check against the executing account interface.get_transaction_inputs()
can no longer return a fullTransactionInputs
, since there is no note data to instantiate an object. For now, I've mad this return a tuple of structs.ForeignAccountInputs
struct, and a list ofForeignTransactionInputs
toTransactionArgs
. 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 separateAccountCode
commitment list to create the internalAccountProcedureIndexMap
.