feat: Pre-authenticated transaction submission#2262
Conversation
kkovaacs
left a comment
kkovaacs
left a comment
There was a problem hiding this comment.
Looks good to me, thanks!
Mirko-von-Leipzig
left a comment
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
I'm not sure we should be emphasizing trust everywhere. This is just another internal service. We could envisage extending this with mempool stats or introspection etc in the future so I would not hyperfixate on submission/trust.
Its already an internal proto service. The docs should be mainly in the CLI/markdown files.
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
sergerad
changed the title
| /// forwards the authenticated result to the sequencer's pre-authenticated submission API, rather | ||
| /// than forwarding the raw transaction upstream. Both must be provided together. | ||
| #[derive(clap::Args, Clone, Debug)] | ||
| pub struct PreAuthenticatedNodeOptions { |
There was a problem hiding this comment.
@Mirko-von-Leipzig I renamed trusted->pre-authenticated. You might still prefer to call this SequencerOptions etc instead?
There was a problem hiding this comment.
I think so, especially if we add other options down the line.
|
|
||
| // The chain height at which authentication took place. | ||
| fixed32 authentication_height = 4; | ||
| } |
There was a problem hiding this comment.
@Mirko-von-Leipzig we could just do single field of bytes (AuthenticatedTransaction::to_bytes) instead of these fields.
There was a problem hiding this comment.
At the moment it doesn't much matter; but it depends where we land with #1882.
I would keep this as is.
|
|
||
| // The chain height at which authentication took place. | ||
| fixed32 authentication_height = 4; | ||
| } |
There was a problem hiding this comment.
At the moment it doesn't much matter; but it depends where we land with #1882.
I would keep this as is.
| // sequencer trusts that the caller has already performed basic checks, proof verification, | ||
| // validator re-execution and authentication. It must therefore never be exposed publicly. | ||
| syntax = "proto3"; | ||
| package pre_authenticated; |
There was a problem hiding this comment.
I would still prefer we call this sequencer. This contains the sequencer's internal gRPC services.
If anything, you could change service Api to service Preauthenticated, but I also wouldn't do that.
There are other future methods we may want or need to add here, so forcing this name just because it happens to be the current two methods isn't a good idea imo. We already have the method names to indicate this.
| /// Submits each transaction in the batch to the validator for re-execution. | ||
| /// | ||
| /// The caller must ensure `transaction_inputs` matches the batch's transactions in length and | ||
| /// order. | ||
| async fn submit_batch_to_validator( |
There was a problem hiding this comment.
I feel like this could be a method on ValidatorClient instead of free floating.
| /// forwards the authenticated result to the sequencer's pre-authenticated submission API, rather | ||
| /// than forwarding the raw transaction upstream. Both must be provided together. | ||
| #[derive(clap::Args, Clone, Debug)] | ||
| pub struct PreAuthenticatedNodeOptions { |
There was a problem hiding this comment.
I think so, especially if we add other options down the line.
| if batch.transactions().len() != inputs.len() { | ||
| return Err(Status::invalid_argument(format!( | ||
| "Number of inputs {} does not match number of transactions {} in batch", | ||
| inputs.len(), | ||
| batch.transactions().len() | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Nit: this is a cheap check and should go above the auth_inputs decoding.
| debug_assert_eq!( | ||
| batch.transactions().len(), | ||
| inputs.len(), | ||
| "transaction inputs must match the batch's transactions" | ||
| ); |
There was a problem hiding this comment.
This should probably be a hard assert?
| /// When unset the pre-authenticated submission service is not exposed. This interface accepts | ||
| /// already-authenticated transactions from full nodes *without* re-verification. | ||
| #[arg( | ||
| long = "pre-authenticated.listen", |
There was a problem hiding this comment.
I would either call this sequencer.listen or if we want to emphasize that this is an internal api sequencer.internal.listen or internal.listen.
3 participants
Summary
Closes #2242.
Changelog