Skip to content
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

Add trait to encode objects for the advice map #677

Open
hackaugusto opened this issue May 8, 2024 · 2 comments
Open

Add trait to encode objects for the advice map #677

hackaugusto opened this issue May 8, 2024 · 2 comments

Comments

@hackaugusto
Copy link
Contributor

What should be done?

Currently there is a single trait to convert the different types of transactions to advice inputs:

/// Defines how inputs required to execute a transaction kernel can be extracted from self.
pub trait ToTransactionKernelInputs {
/// Returns stack and advice inputs required to execute the transaction kernel.
fn get_kernel_inputs(&self) -> (StackInputs, AdviceInputs);
}

However, there is not trait to convert individual structs into advice map data, so the whole map is populated via standalone functions:

fn extend_advice_inputs(
tx_inputs: &TransactionInputs,
tx_args: &TransactionArgs,
advice_inputs: &mut AdviceInputs,
) {
// build the advice stack
build_advice_stack(tx_inputs, tx_args.tx_script(), advice_inputs);
// build the advice map and Merkle store for relevant components
add_chain_mmr_to_advice_inputs(tx_inputs.block_chain(), advice_inputs);
add_account_to_advice_inputs(tx_inputs.account(), tx_inputs.account_seed(), advice_inputs);
add_input_notes_to_advice_inputs(tx_inputs.input_notes(), tx_args, advice_inputs);
advice_inputs.extend_map(tx_args.advice_map().clone());
}

This creates a problem while testing, because to populate the advice for testing, a transaction is needed, to create a transaction, one needs a the block header and account, and so on. This results in a lot of setup code to write tests and hides the important details.

To facilitate testing, it would be good to have a trait similar to the following (just a sketch, needs further work)

trait ToAdviceMap<T> {
  fn to_advice_map(&self) -> impl Iterator<Item=(Word, T)>;
}

It should make it easier to write tests without requiring creating the whole mock state.

How should it be done?

When is this task done?

After adding a strategy to simplify tests, perhaps with the trait above, and there is a test example to demonstrate the new approach.

Additional context

The motivation for this is testing of the new account verification code, which currently runs the complete prologue because there is no easy way to provide just the account seed:

#[cfg_attr(not(feature = "testing"), ignore)]
#[test]
pub fn test_prologue_create_account() {
let (_acct_id, account_seed) =
generate_account_seed(AccountSeedType::RegularAccountUpdatableCodeOnChain);
let (tx_inputs, tx_args) = mock_inputs_with_account_seed(
MockAccountType::StandardNew,
AssetPreservationStatus::Preserved,
Some(account_seed),
);
let code = "
use.miden::kernels::tx::prologue
begin
exec.prologue::prepare_transaction
end
";
let transaction = prepare_transaction(tx_inputs, tx_args, code, None);
let _process = run_tx(&transaction).unwrap();
}

@hackaugusto
Copy link
Contributor Author

hackaugusto commented May 8, 2024

Well, I wrote the above thinking that populating the advice map would be sufficient because I was thinking mostly about validate_seed:

#! Stack: []
#! Output: []
export.validate_seed

But there are other procedures which would also be great to test in isolation, that require advice stack data too, like process_acct_data:

#! Stack: []
#! Advice stack: [acct_id, 0, 0, nonce, ACCOUNT_VAULT_ROOT, ACCOUNT_STORAGE_ROOT, ACCOUNT_CODE_ROOT]
#! Output: []
#!
proc.process_acct_data

maybe another trait for stack inputs would also be useful. I also proposed more general traits in the crypto crate 0xPolygonMiden/crypto#312 which could be useful.

@bobbinth
Copy link
Contributor

To facilitate testing, it would be good to have a trait similar to the following (just a sketch, needs further work)

trait ToAdviceMap<T> {
  fn to_advice_map(&self) -> impl Iterator<Item=(Word, T)>;
}

It should make it easier to write tests without requiring creating the whole mock state.

One potential issue here is that the way an object is to be added to the advice provider is not an intrinsic property of an object but rather a property of the program which will use this object. So, for example, the transaction kernel may expect a block header to be inserted into the advice map in one way, while block header may expect it to be inserted in another way. Not to say that this will always happen (and maybe in case of the block header it will be the same in both cases), but it is definitely possible. This was the primary reason for introducing ToTransactionKernelInputs trait (and in the future we could have ToBlockKernelInputs trait as well).

Would it not be possible to implement ToTransactionKernelInputs trait on a mock struct to simplify testing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants