[consensus] Return consensus position when calling submit_transaction #22157
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
arun-koshy
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| #[error( | ||
| "Failure serializing consensus position in the requested format: {:?}", | ||
| error | ||
| )] | ||
| ConsensusPositionSerializationError { error: String }, | ||
| #[error( | ||
| "Failure deserializing consensus position from the provided format: {:?}", | ||
| error | ||
| )] | ||
| ConsensusPositionDeserializationError { error: String }, |
There was a problem hiding this comment.
I think you can now use the unified grpc serde errors
| }) | ||
| } | ||
|
|
||
| async fn handle_get_effects( |
There was a problem hiding this comment.
We probably do want to use wait_for_effects since it handles the wait within the validator, instead of client keeping polling and wait.
On a side note the function name handle_vote_transaction is a bit strange in AuthorityState. I seems to be literally getting executed effects.
There was a problem hiding this comment.
@lxfind are you proposing on not introducing the handle_get_effects?
I thought that handle_vote_transaction is also acquiring locks if tx hasn't been executed , no? Which also made me curious if that would be ok to do when calling the get_effects
| @@ -548,7 +572,7 @@ mod tests { | |||
| // Now iterate over all the waiters. Everyone should have been acknowledged. | |||
| let mut block_status_waiters = Vec::new(); | |||
| while let Some(result) = included_in_block_waiters.next().await { | |||
| let (block_ref, block_status_waiter) = | |||
| let (block_ref, _tx_indices, block_status_waiter) = | |||
There was a problem hiding this comment.
Could you please extend the test to ensure that the correct indexes are getting calculated? (batched & single txes)
| // TODO(fastpath): Skip checking for execution effects | ||
| // Effects are useless in mfp if they cannot be mapped to a consensus | ||
| // position |
There was a problem hiding this comment.
So the effects will always be checked against a tx of a specific consensus position right?
| }) | ||
| } | ||
|
|
||
| async fn handle_get_effects( |
There was a problem hiding this comment.
@lxfind are you proposing on not introducing the handle_get_effects?
I thought that handle_vote_transaction is also acquiring locks if tx hasn't been executed , no? Which also made me curious if that would be ok to do when calling the get_effects
| }) | ||
| } | ||
|
|
||
| async fn handle_get_effects( |
There was a problem hiding this comment.
@arun-koshy could you please add a couple of lines of what someone should expect from the handle_get_effects request?
| } | ||
|
|
||
| #[derive(Clone, prost::Message)] | ||
| pub struct RawGetEffectsRequest { |
There was a problem hiding this comment.
Out of curiosity, why some responses are returned as "raw" on the API (where we explicitly serialise the fields) and others not? (ex the HandleCertificateResponseV2). Does it have to do with the ability of the server to serialise the properties it self or for reasons of determinism?
| // to the submitting client. They can handle retries as needed | ||
| // if the consensus position does not return the desired results | ||
| // (e.g. not sequenced due to garbage collection). |
There was a problem hiding this comment.
How is the client going to find out the new position?
Description
Moved logic from old
submit_transactionpath toget_effects. Newsubmit_transactionpath will only submit transaction to consensus and get back the consensus position.get_effectswill not submit to consensus and will just check if effects are available to be returned. This path may be deprecated if we don't have any use for it oncewait_for_effectsis in.ConsensusTxPositionis based on the tx indices calculated while transactions are being pulled when the block is being generated intry_new_block. There is no guarantee that a returnedConsensusTxPositionwill be sequenced and it is up to the client to retry and get a new consensus position if needed.Test plan
Still working on adding tests
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.