-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[Fastpath] Support transactions certified through consensus #19601
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
9933980
to
7382a74
Compare
7382a74
to
6e08480
Compare
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.
Changes look good to me. The debug data look fine at least for now 👍
pub type TransactionIndex = u16; | ||
|
||
/// Consensus timestamp in ms. | ||
pub type ConsensusTimestampMs = u64; |
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.
can we be consistent about whether or not we are prefixing consensus-related type aliases with Consensus
?
that is to say, should it be removed here or added on the types 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.
I was trying to avoid confusion with CheckpointTimestamp
or other timestamp types. But looking at how the Narwhal imported TimestampMs
type is used without issue, I will leave this as TimestampMs
for now.
10434de
to
b321dd1
Compare
Description
The fields in the
CertificateProof::Consensus
variant is chosen to enhance debuggability (including the transaction's position in the DAG: round, authority index, transaction index), without unnecessary data (consensus block digest is excluded).Test plan
CI
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.