fix: send tx_inputs, use existing asset witness #1490
Conversation
SantiagoPittella
force-pushed
the
santiagopittella-fix-validator-datastore
branch
from
50fe5e3 to
2e8cc19
Compare
SantiagoPittella
force-pushed
the
santiagopittella-fix-validator-datastore
branch
from
2e8cc19 to
e4d4989
Compare
| let stored_witnesses = self.tx_inputs.asset_witnesses(); | ||
|
|
||
| vault_keys | ||
| .into_iter() | ||
| .map(|vault_key| { | ||
| stored_witnesses | ||
| .iter() | ||
| .find(|w| w.authenticates_asset_vault_key(vault_key)) | ||
| .cloned() | ||
| .ok_or_else(|| DataStoreError::Other { | ||
| error_msg: format!( | ||
| "asset witness not found for vault key {vault_key}", | ||
| ) | ||
| .into(), | ||
| source: None, | ||
| }) | ||
| }, | ||
| Err(err) => Err(DataStoreError::Other { | ||
| error_msg: "failed to open vault".into(), | ||
| source: Some(err.into()), | ||
| }), | ||
| } | ||
| })) | ||
| }) |
There was a problem hiding this comment.
AFAIK this change is fine because tx_inputs.asset_witnesses() only contains prefetched assets. On re-execution, get_vault_asset_witnesses() would only run for prefetched assets anyway since TransactionExecutor prefetches regardless of context and anything “dynamic” would otherwise be loaded from advice data directly.
cc @PhilippGackstatter is there a reason we keep prefetched asset witnesses separate? If not, could they be removed TransactionInputs? Then either:
- the executor can check whether the prefetched assets are already present in the advice map (and avoid the data store call if so), or maybe
- we can add a specific executor entrypoint/flag that skips prefetching because the VM will load them from the advice map
If not, I think the behavior of get_vault_asset_witnesses() only being called for prefetched assets when re-executing might be worth documenting (unless it already is somewhere) because it can be misleading at first sight.
There was a problem hiding this comment.
The original motivation is described here: 0xMiden/miden-base#2113 (comment)
Now that we have TransactionAdviceInputs in miden-protocol, at first glance, it looks like we could revisit this and add asset witnesses directly to TransactionInputs::advice_inputs in TransactionInputs::with_asset_witnesses.
Is this what you mean? If yes, then I can open an issue for this in base.
I think the behavior of
get_vault_asset_witnesses()only being called for prefetched assets when re-executing might be worth documenting (unless it already is somewhere) because it can be misleading at first sight.
I understand the cause for confusion. The behavior, that some data is added to advice inputs after execution, seems like a property of the TransactionExecutor. So, would it be sufficient to add a doc section on re-execution on TransactionExecutor that describes what data fetched from DataStore is added to the advice inputs after execution? This would then serve as a reference for this kind of question and we could note that this implies certain methods may not be called during re-execution.
There was a problem hiding this comment.
iiuc then the change above relies on the fact that the provided TransactionInputs is the one used to actually execute the ProvenTransaction? And that execution adds additional data to "inputs", making it an execution output artifact?
If so, then this is definitely confusing and will lead to issues since this is passed in by users.
There was a problem hiding this comment.
Is this what you mean?
I think so, but not sure if that is sufficient. What I'm mainly trying to get at is that it seems that on the re-execution case, dynamic calls to DataStore::get_vault_asset_witnesses() are basically skipped because the Merkle paths are already in the advice data (because you are taking the inputs from an ExecutedTransaction). This does not seem to be the case for the prefetched assets call to the DataStore, as it's called every time even if the data is already in the advice inputs. Additionally, only the prefetched asset witnesses are part of TransactionInputs separately. This means the validator implementor needs to know this DataStore call will only be called necessarily once in the whole transaction for the prefetched assets (and now that I see the comment you linked it makes sense why this was the case), and this is what I'd document more.
Alternatively, it would be great if the asset witnesses prefetch is skipped on re-execution as well. Then if you get a call to DataStore::get_vault_asset_witnesses() during re-execution it's basically an error every time and there's no ambiguity; also the implementation itself becomes trivial and can maybe be provided from miden-base. But maybe I'm missing something else here.
My assumptions (some of which might be incorrect) are:
- On dynamic calls to the
DataStore(i.e., not the prefetch ofprepare_tx_inputs()), the host first checks that the Merkle path is not already on the advice data.- If it is, it's loaded from there and the call is not done
- When creating inputs from an
ExecutedTransactionfor the re-execution case, you take the latest state of the advice data (this includes added witnesses) - Prefetched asset witnesses are retrieved every time regardless of them being in the advice inputs, and later get loaded again into the Merkle store
There was a problem hiding this comment.
Should we merge this PR?
There was a problem hiding this comment.
I think these changes are still valid, so I’m okay with merging this. Though I still have the concerns I mentioned above.
After #1460 , to verify a block the transactions needs to be verified too. We achieve that by sending the
TransactionInputswhen submitting.Also, simplifies and fixes the validator's
DataStore::get_vault_asset_witnessesimplementation.