-
Notifications
You must be signed in to change notification settings - Fork 88
fix: send tx_inputs, use existing asset witness #1490
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ use miden_node_proto::domain::mempool::MempoolEvent; | |
| use miden_node_proto::generated::{self as proto}; | ||
| use miden_node_utils::FlattenResult; | ||
| use miden_protocol::block::BlockNumber; | ||
| use miden_protocol::transaction::ProvenTransaction; | ||
| use miden_protocol::transaction::{ProvenTransaction, TransactionInputs}; | ||
| use miden_tx::utils::Serializable; | ||
| use tokio_stream::StreamExt; | ||
| use tonic::Status; | ||
|
|
@@ -45,10 +45,11 @@ impl BlockProducerClient { | |
| pub async fn submit_proven_transaction( | ||
| &self, | ||
| proven_tx: ProvenTransaction, | ||
| tx_inputs: &TransactionInputs, | ||
| ) -> Result<(), Status> { | ||
| let request = proto::transaction::ProvenTransaction { | ||
| transaction: proven_tx.to_bytes(), | ||
| transaction_inputs: None, | ||
| transaction_inputs: Some(tx_inputs.to_bytes()), | ||
| }; | ||
|
Comment on lines
45
to
53
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is needed: block producer ignores this field. Here is how it is supposed to work:
|
||
|
|
||
| self.client.clone().submit_proven_transaction(request).await?; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,20 +74,24 @@ impl DataStore for TransactionInputsDataStore { | |
| }); | ||
| } | ||
|
|
||
| Result::<Vec<_>, _>::from_iter(vault_keys.into_iter().map(|vault_key| { | ||
| match self.tx_inputs.account().vault().open(vault_key) { | ||
| Ok(vault_proof) => { | ||
| AssetWitness::new(vault_proof.into()).map_err(|err| DataStoreError::Other { | ||
| error_msg: "failed to open vault asset tree".into(), | ||
| source: Some(err.into()), | ||
| let stored_witnesses = self.tx_inputs.asset_witnesses(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this will work. As discussed in another comment, this will only cover the pre-fetched witnesses. As @igamigo mentioned, we should be able to get rid of this field entirely but the change would need to happen in For broader context, this should be properly addressed with #1493.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll close this PR then, and open another one only with the changes for the network monitor
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I opend #1501 with the standing changes
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be clear, I believe this does fix the issue (and I think @SantiagoPittella did some tests to confirm this). This covers the pre-fetched witnesses only, but AFAICT (might be missing something so take it with a grain of salt), this is enough because on re-execution that's the only time this will be called, as all other witnesses get loaded from advice data directly and do not go through the But going to open an issue for potentially removing the field in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The pre-fetched asset witnesses will cover witness of asset of all input notes. So, for transactions that only consume notes, this will be fine. But I don't think these witness will cover assets for output note. These are generated during transaction execution and so, are not added to this filed. |
||
|
|
||
| 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()), | ||
| }), | ||
| } | ||
| })) | ||
| }) | ||
|
Comment on lines
+77
to
+93
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK this change is fine because cc @PhilippGackstatter is there a reason we keep prefetched asset witnesses separate? If not, could they be removed
If not, I think the behavior of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original motivation is described here: 0xMiden/miden-base#2113 (comment) Now that we have
I understand the cause for confusion. The behavior, that some data is added to advice inputs after execution, seems like a property of the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. iiuc then the change above relies on the fact that the provided If so, then this is definitely confusing and will lead to issues since this is passed in by users.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 My assumptions (some of which might be incorrect) are:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we merge this PR?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these changes are still valid, so I’m okay with merging this. Though I still have the concerns I mentioned above. |
||
| .collect() | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
Why do we need
Box::pin()here? And same question for line 250 above.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.
It was related to the changes that I did on
Context::execute_transaction, but since we are rolling back that, the box pin aren't needed anymore.