Prepose consensus commit prologue in checkpoint #18023
Conversation
|
@halfprice is attempting to deploy a commit to the Mysten Labs Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
halfprice
force-pushed
the
zhewu/prologue-first-in-checkpoint
branch
from
6054be3 to
00e7285
Compare
halfprice
force-pushed
the
zhewu/prologue-first-in-checkpoint
branch
from
248d5df to
f004f5e
Compare
halfprice
requested review from
lxfind,
aschran,
williampsmith,
mwtian and
mystenmark
|
|
||
| // No other dependencies of this consensus commit prologue that haven't been included | ||
| // in any previous checkpoint. | ||
| assert_eq!(effects_in_current_checkpoint.len(), 1); |
There was a problem hiding this comment.
I was debating whether these should be debug_assert or assert, and eventually decides to make it assert. This is because all this PR is built on the assumption that there is at most one consensus commit prologue in a pending checkpoint, and all its previous dependencies must have been included in the previous checkpoint. If this is true, something must be wrong and we probably don't want the node to move forward to produce more false state.
| let mut effects_in_current_checkpoint = BTreeSet::new(); | ||
| let sorted_tx_effects_included_in_checkpoint = self | ||
| .resolve_checkpoint_transactions(pending.roots, &mut effects_in_current_checkpoint) | ||
| .await?; | ||
| let new_checkpoint = self | ||
| .create_checkpoints(sorted_tx_effects_included_in_checkpoint, pending.details) | ||
| .await?; | ||
| self.write_checkpoints(height, new_checkpoint).await?; |
There was a problem hiding this comment.
The code is structure this way primarily due to easy integration with @aschran 's #17955 .
If this lends before that one, we just need to put
let sorted_tx_effects_included_in_checkpoint = self
.resolve_checkpoint_transactions(pending.roots, &mut effects_in_current_checkpoint)
.await?;
into a loop over all the pending checkpoints, and concatenate all the sorted_tx_effects_included_in_checkpoint
| if let Some(consensus_commit_prologue_root) = consensus_commit_prologue_root { | ||
| if self | ||
| .protocol_config() | ||
| .prepose_consensus_commit_prologue_in_checkpoints() | ||
| { | ||
| // Put consensus commit prologue root at the beginning of the checkpoint roots. | ||
| checkpoint_roots.push(consensus_commit_prologue_root); | ||
| } else { | ||
| roots.insert(consensus_commit_prologue_root); | ||
| } | ||
| } |
There was a problem hiding this comment.
In theory, this part does not necessarily need to be guarded by the feature flag, because the causal sort in checkpoint builder is supposed to be input order agnostic. However, I'm scared that if the order here somehow affects the causal sort order in checkpoint builder, it would cause fork when upgrading. So I still guarded preposing the consensus commit prologue here using the feature flag to be cautious.
halfprice
force-pushed
the
zhewu/prologue-first-in-checkpoint
branch
from
f004f5e to
1d8806d
Compare
halfprice
force-pushed
the
zhewu/prologue-first-in-checkpoint
branch
from
0edb79e to
5024049
Compare
| } | ||
| sorted.extend(CausalOrder::causal_sort(unsorted)); | ||
|
|
||
| fail_point_if!("assert-prologue-first-in-checkpoint", || { |
There was a problem hiding this comment.
maybe this could just be if cfg!(msim) instead of a failpoint
| Ok(self | ||
| .state | ||
| .get_transaction_cache_reader() | ||
| .get_transaction_block(&root_digests[0])? |
There was a problem hiding this comment.
can you expect here instead of using filter? if the transaction block is not found it is a serious bug
There was a problem hiding this comment.
You mean remove the "?" instead of using an expect, correct? If so, done.
I don't think we can remove the filter since it's possible that a consensus commit doesn't contain a prologue txn.
halfprice
force-pushed
the
zhewu/prologue-first-in-checkpoint
branch
from
4037f72 to
cfa939c
Compare
## Description This PR moves consensus commit prologue to the beginning of the checkpoint if it presents. This is to make sure that the consensus commit prologue is before all the cancelled transactions in the checkpoint, and hence when doing sequential replay, we know which transactions are cancelled. This is achieved by finding the consensus commit prologue in the pending checkpoint, and always places it the first before all other sorted transactions. It also builds on several assumptions: - There is at most one consensus commit prologue transactions in a pending checkpoint. - The consensus commit prologue shouldn't have any dependencies that aren't in any previous checkpoints. ## Test plan Added consensus commit prologue invariant check for all integration simtests. --- ## 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. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK:
| if let Some((ccp_digest, ccp_effects)) = consensus_commit_prologue { | ||
| #[cfg(debug_assertions)] | ||
| { | ||
| // When consensus_commit_prologue is extracted, it should not be included in the `unsorted`. | ||
| for tx in unsorted.iter() { | ||
| assert!(tx.transaction_digest() != &ccp_digest); | ||
| } | ||
| } | ||
| sorted.push(ccp_effects); | ||
| } | ||
| sorted.extend(CausalOrder::causal_sort(unsorted)); |
There was a problem hiding this comment.
@halfprice fyi this line threw a warning for me when i was compiling this morning:
| if let Some((ccp_digest, ccp_effects)) = consensus_commit_prologue { | |
| #[cfg(debug_assertions)] | |
| { | |
| // When consensus_commit_prologue is extracted, it should not be included in the `unsorted`. | |
| for tx in unsorted.iter() { | |
| assert!(tx.transaction_digest() != &ccp_digest); | |
| } | |
| } | |
| sorted.push(ccp_effects); | |
| } | |
| sorted.extend(CausalOrder::causal_sort(unsorted)); | |
| if let Some((_ccp_digest, ccp_effects)) = consensus_commit_prologue { | |
| #[cfg(debug_assertions)] | |
| { | |
| // When consensus_commit_prologue is extracted, it should not be included in the `unsorted`. | |
| for tx in unsorted.iter() { | |
| assert!(tx.transaction_digest() != &_ccp_digest); | |
| } | |
| } | |
| sorted.push(ccp_effects); | |
| } | |
| sorted.extend(CausalOrder::causal_sort(unsorted)); |
Not sure if this would fix the warning or if it even makes sense to fix?...
Description
This PR moves consensus commit prologue to the beginning of the checkpoint if it presents. This is to make sure
that the consensus commit prologue is before all the cancelled transactions in the checkpoint, and hence
when doing sequential replay, we know which transactions are cancelled.
This is achieved by finding the consensus commit prologue in the pending checkpoint, and always
places it the first before all other sorted transactions.
It also builds on several assumptions:
Test plan
Added consensus commit prologue invariant check for all integration simtests.
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.