chore: check matching account delta commitments#1093
Conversation
SantiagoPittella
force-pushed
the
santiagopittella-account-delta-checks
branch
from
00bad99 to
b257cc5
Compare
SantiagoPittella
requested review from
Mirko-von-Leipzig,
bobbinth and
igamigo
and removed request for
Mirko-von-Leipzig
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a comment inline. We should probably merge this after 0xMiden/protocol#1603 is merged.
Also, it would be great to add a test to make sure this works (the code in this PR would actually reject all incoming transactions) - but I'm not sure how easy it is.
Mirko-von-Leipzig
left a comment
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
Looks good; modulo the change from Bobbin.
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
|
Added a test, it is quite a bit hacky but serves it purpose. Also left a an approve on 0xMiden/protocol#1603 but has some merge conflicts, I can address them if you want @bobbinth |
…to_commitment and AuthRpoFalcon512
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Thank you! Looks good! I think the code itself is there, but I wonder if we should improve the construction of a dummy transaction a bit. I left a comment about this inline.
| let tx = ProvenTransactionBuilder::new( | ||
| account_id, | ||
| [8; 32].try_into().unwrap(), | ||
| [3; 32].try_into().unwrap(), | ||
| [22; 32].try_into().unwrap(), // delta commitment | ||
| 0.into(), | ||
| Word::default(), | ||
| u32::MAX.into(), | ||
| ExecutionProof::new(Proof::new_dummy(), HashFunction::default()), | ||
| ) | ||
| .account_update_details(AccountUpdateDetails::Delta( | ||
| AccountDelta::new( | ||
| account_id, | ||
| AccountStorageDelta::new(), | ||
| AccountVaultDelta::default(), | ||
| Felt::default(), | ||
| ) | ||
| .unwrap(), | ||
| )) | ||
| .build() | ||
| .unwrap(); |
There was a problem hiding this comment.
I wonder if there is an easier way to create a dummy proven transaction. At the very least, would be great to have a simpler way to create dummy ExecutionProof. Maybe we should make a small PR in miden-vm that adds ExecutionProof:dummy()? This way, we could avoid adding miden-air and winterfell dependencies in this PR.
cc @PhilippGackstatter and @igamigo in case you have other suggestions.
There was a problem hiding this comment.
I have the changes, but I don't have permission to push in the vm repo. Will sync lambdaclass fork and send it from there.
There was a problem hiding this comment.
Is there a world where we make it easier to create real txs? I've been thinking about a db-tx like API for a while now, but I don't actually know enough :)
// Create an account with a `Counter` component (however that works).
// This could also come from a mock chain.
let account = AccountBuilder::default().with_component(Counter).build();
// Create an AccountTx which holds a ref to `Account` plus whatever stuff it
// wants to execute. I imagine this would be some sort of cmd/vm stack.
let mut tx = account.begin_tx();
// This exposes the counter state + API.
// SAFETY: account definitely has a counter component.
let mut counter = tx.get_component(Counter).unwrap();
counter.increment(); // Or whatever method it has.
counter.increment();
// Do other stuff. Grab other components.
// This is now an ExecutedTransaction using the original account state.
let executed_tx = tx.execute().unwrap();
// You can now apply or discard the changes of the tx to the account.
tx.commit().unwrap();
// tx.abort().unwrap();There was a problem hiding this comment.
It would be nice to have something simple to create realistic transactions. Though I guess proving is still an issue ito performance for tests?
There was a problem hiding this comment.
Just opened 0xMiden/miden-vm#2007
miden-vm v0.16.3 with these changes is now on crates.io.
It would be nice to have something simple to create realistic transactions. Though I guess proving is still an issue ito performance for tests?
Generating a real proof is expensive, most other things shouldn't be.
@PhilippGackstatter @igamigo - anything you can recommend on how to improve creation of a dummy transaction here?
If there isn't anything too simple to do, I'd probably use what we have here and create another issue for improving creation of dummy transactions in miden-base.
There was a problem hiding this comment.
For creating ExecutedTransaction it's easiest to use MockChain::build_tx_context. Once we've gotten rid of the pending note APIs in MockChain, there shouldn't really be a way to create transactions that couldn't exist in a real chain, so that would be optimal. This can already be done now just by not using those APIs and instead adding notes via the APIs on MockChainBuilder or creating them yourself.
Once you have an ExecutedTransaction, then you could technically use ProvenTransactionExt::from_executed_transaction_mocked to convert that transaction into a proven transaction with a dummy proof. But I also would like to get rid of it, because it's hacky and could easily break or do something that a real transaction could not do as the codebase evolves.
Ideally, all our prover types in miden-base would all have a prove and prove_dummy function that takes an ExecutedTransaction and turns it into a ProvenTransaction with the only difference being the use of a dummy proof on the latter. That would be much easier to maintain. The batch and block prover roughly have that functionality, but it could be exposed in a better way.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you!
I'm going to merge this PR - but should we create an issue to refactor the test once the changes mentioned by @PhilippGackstatter are implemented in miden-base (I'm guessing this will be in the next release).
4 participants
closes #306