Consensus Data Quarantining #20982

mystenmark · 2025-01-25T01:30:48Z

Description

All consensus output is held in memory until all checkpoints built from that output have been committed. This ensures that the epoch db is not corrupted in the event of a fork.
Consensus therefor becomes the primary replay log for all crash recovery.
Additionally this means there are several tables that we no longer need to write to, and their contents can live in memory.

Test plan

Extensive testing with both simtest and antithesis. In particular the upgrade flow (from non-DQ to DQ build) has been tested extensively with antithesis.

vercel · 2025-01-25T01:30:54Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 4, 2025 5:39am

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview		Feb 4, 2025 5:39am
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview		Feb 4, 2025 5:39am

aschran

awesome! mostly nits

aschran · 2025-01-31T17:44:49Z

crates/sui-core/src/authority.rs

@@ -1557,6 +1558,10 @@ impl AuthorityState {
                .force_reload_system_packages(&BuiltInFramework::all_package_ids());
        }

+        if matches!(tx_key, TransactionKey::RandomnessRound(_, _)) {


I understand this, but maybe a short comment explaining why this has to be handled specially would be helpful for posterity

Actually it turns out this is not necessary! I switched to driving the deletions of shared versions using pending checkpoint roots, which have TxKeys instead of digests.

aschran · 2025-01-31T17:47:05Z

crates/sui-core/src/authority.rs

+            .map(|c| *c.sequence_number())
+            .unwrap_or(0);
+
+        assert!(epoch_last_checkpoint >= highest_locally_built_checkpoint_seq);


in the spirit of us wishing for more useful panic messages in the mainnet outage, maybe add a message for what the values here are in case this assert fires?

aschran · 2025-01-31T17:49:43Z

crates/sui-core/src/authority/authority_per_epoch_store.rs

@@ -312,6 +314,9 @@ pub struct AuthorityPerEpochStore {
    /// and it needs to be cleared at the end of the epoch.
    tables: ArcSwapOption<AuthorityEpochTables>,

+    consensus_quarantine: RwLock<ConsensusOutputQuarantine>,


doc comments plz

aschran · 2025-01-31T17:59:35Z

crates/sui-core/src/authority/authority_per_epoch_store.rs

@@ -1940,19 +1833,24 @@ impl AuthorityPerEpochStore {
            }
        }

+        for key in &keys {
+            deferred_transactions.remove(key);


how much do I hate that BTreeMap has no range_delete :(

aschran · 2025-01-31T18:04:55Z

crates/sui-core/src/authority/authority_per_epoch_store.rs

@@ -2272,10 +2201,15 @@ impl AuthorityPerEpochStore {
        digests: &[TransactionDigest],


maybe add something like take_ to this function name so it's clear that the new version removes the values?

potentially same above with deferred tx (take_ instead of load_?)

did you mean for this comment to be somewhere else?

I meant for it to be one line up, I guess - but it is about this function

aschran · 2025-01-31T18:53:46Z

crates/sui-core/src/authority/authority_per_epoch_store.rs

@@ -312,6 +314,9 @@ pub struct AuthorityPerEpochStore {
    /// and it needs to be cleared at the end of the epoch.
    tables: ArcSwapOption<AuthorityEpochTables>,

+    consensus_quarantine: RwLock<ConsensusOutputQuarantine>,
+    consensus_output_cache: ConsensusEphemeralOutput,


Looks like you changed the name of this but didn't update it everywhere - would it be clearer for variable name to match the type?

consensus_output_cache

and then fix the comment above the struct definition which still has the old name as well?

Done, good point!

aschran · 2025-01-31T18:59:03Z

crates/sui-core/src/authority/consensus_quarantine.rs

+}
+
+// Read methods - all methods in this block return data from the quarantine which would otherwise
+// by found in the database.


aschran · 2025-01-31T19:00:31Z

crates/sui-core/src/authority/consensus_quarantine.rs

+    }
+}
+
+// Read methods - all methods in this block return data from the quarantine which would otherwise


I see some mutations in this block, not just reads?

aschran · 2025-01-31T19:03:44Z

crates/sui-core/src/authority/consensus_quarantine.rs

+
+    pub fn insert(&mut self, key: K, value: V) {
+        let entry = self.map.entry(key);
+        match entry {


would the more idiomatic pattern here be to use and_modify followed by or_insert?

maybe but then you move value twice because the compiler can't see that only one of and_modify and or_insert will be called

aschran · 2025-01-31T19:07:54Z

crates/sui-core/src/authority/consensus_quarantine.rs

+    // Returns true if the key was present, false otherwise.
+    // Note that the key may not be removed if present, as it may have a refcount > 1.
+    pub fn remove(&mut self, key: &K) -> bool {
+        let entry = self.map.entry(key.clone());


could this be

map.get_mut(key).map(...modify...).is_some()?

you mean to avoid a clone of the key?

but then i have to do multiple lookups, and the keys are almost always just arrays, so cloning should not be expensive

andll

Some small nits but overall looks good. I did not look to much into randomness and congestion control parts, since I am less familiar with those

andll · 2025-01-31T19:59:21Z

crates/sui-core/src/authority/authority_per_epoch_store.rs

+            .read()
+            .get_pending_checkpoints(last);
+
+        // retain only the checkpoints with heights greater than the highest height in the db


Would it be the same effect if we pass something like db_highest_height.or(last) into Quarantine:: get_pending_checkpoints?

You would need to take the lowest height in quarantine and use that as the upper bound for the db, i think? I'm not sure its worth it though, this code is temporary and only needed for upgrades. Will delete.

andll · 2025-01-31T20:02:38Z

crates/sui-core/src/authority/authority_per_epoch_store.rs

-        index: &CheckpointHeight,
-    ) -> SuiResult<Option<PendingCheckpointV2>> {
-        Ok(self.tables()?.pending_checkpoints_v2.get(index)?)
+    pub fn pending_checkpoint_exists(&self, index: &CheckpointHeight) -> SuiResult<bool> {


Looks like this is used for correctness check, should it check both DB and memory then at least for now?

i think since all writes go through the quarantine this is guaranteed to catch any duplicates?

andll · 2025-01-31T20:05:20Z

crates/sui-core/src/authority/consensus_quarantine.rs

+/// for the commit have been certified.
+pub(crate) struct ConsensusOutputQuarantine {
+    // Output from consensus handler
+    output_queue: VecDeque<ConsensusCommitOutput>,


Since this seemingly can grow unbounded, should we have a metric to track how many items are there?
(And similarly perhaps for shared_object_next_versions?)

good idea - it should be bounded due to consensus back pressure, although its somewhat indirect, so i agree monitoring this is a good idea

andll · 2025-01-31T20:07:47Z

crates/sui-core/src/authority/authority_per_epoch_store.rs

+        Ok(self
+            .consensus_quarantine
+            .read()
+            .pending_checkpoint_exists(index))
    }

    pub fn process_pending_checkpoint(


I know this is not relevant to this PR, but I wonder why we call this function process_pending_checkpoint, while in fact it process constructed checkpoint.

good point, will rename

andll · 2025-01-31T20:10:16Z

crates/sui-core/src/authority/authority_per_epoch_store.rs

-            .multi_contains_keys(digests)?)
+            .multi_contains_keys(fallback_keys)?;
+
+        assert_eq!(fallback_results.len(), fallback_indices.len());


nit: this assert is not really needed since .zip will panic anyway if iterators have different length

zip does not panic! https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c92b3b8d1997c05702446e71766ae8be

Right, I wonder if it has changed or why I thought zip will panic. Anyway, apparently there is also zip_eq method if you want to save one line of code :)

andll · 2025-01-31T20:20:39Z

crates/sui-core/src/authority/consensus_quarantine.rs

+        RefCountedHashMap<ObjectID, CongestionPerObjectDebt>,
+    congestion_control_object_debts: RefCountedHashMap<ObjectID, CongestionPerObjectDebt>,
+
+    processed_consensus_messages: RefCountedHashMap<SequencedConsensusTransactionKey, ()>,


I think we talked in the meeting too, but it seems that processed_consensus_messages does not need to be RefCountedHashMap? When first commit encountered message is written to disk, the message can be removed from memory, since it is on the disk anyway. Moreover, I think only one commit can ever put SequencedConsensusTransactionKey in this map?

I tried to fix this, but it turns out that for deferred transactions, we insert them twice. Once when they arrive for the first time, and once again when we load them from the deferral queue. It would be nice to fix this, but its quite a chore to do.

aschran · 2025-02-04T02:49:36Z

crates/sui-core/src/authority/authority_per_epoch_store.rs

-            for (i, key) in keys.enumerate() {
-                if consensus_quarantine.is_consensus_message_processed(&key) {
-                    results.push(true);
+        Ok(do_fallback_lookup(


just curious if do_fallback_lookup can be made to take an impl Iterator for the first arg instead of a slice, so that you can avoid the extra collect? seems like it should work; maybe it doesn't matter in practice, though.

i tried doing this and quickly fell down a rustc rabbit hole - in the interests of time I think i'm going to leave this as is

aschran · 2025-02-04T02:51:23Z

crates/sui-core/src/authority/consensus_quarantine.rs

+    }
+
+    pub fn insert_shared_object_assignments(&self, versions: &AssignedTxAndVersions) {
+        debug!("set_assigned_shared_object_versions: {:?}", versions);


was this meant to be left in? if so it doesn't seem to match the fn name?

updated the name and changed to trace

- All consensus output is held in memory until all checkpoints built from that output have been committed. This ensures that the epoch db is not corrupted in the event of a fork. - Consensus therefor becomes the primary replay log for all crash recovery. - Additionally this means there are several tables that we no longer need to write to, and their contents can live in memory.

mystenmark requested a review from a team as a code owner January 25, 2025 01:30

mystenmark requested review from aschran and andll January 25, 2025 01:30

mystenmark temporarily deployed to sui-typescript-aws-kms-test-env January 25, 2025 01:30 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 25, 2025 01:31 View deployment

mystenmark changed the title ~~Data Quarantining:~~ Consensus Data Quarantining Jan 25, 2025

Base automatically changed from mlogan-consensus-quarantine-mod to main January 27, 2025 22:07

mystenmark force-pushed the mlogan-data-quarantine branch from bd63227 to 2abeb16 Compare January 27, 2025 22:08

mystenmark temporarily deployed to sui-typescript-aws-kms-test-env January 27, 2025 22:08 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 27, 2025 22:10 View deployment

aschran reviewed Jan 31, 2025

View reviewed changes

andll reviewed Jan 31, 2025

View reviewed changes

mystenmark force-pushed the mlogan-data-quarantine branch from 2abeb16 to 0f0293e Compare February 3, 2025 23:03

vercel bot deployed to Preview – sui-docs February 3, 2025 23:05 View deployment

mystenmark requested a review from aschran February 4, 2025 01:43

aschran approved these changes Feb 4, 2025

View reviewed changes

mystenmark added 4 commits February 3, 2025 21:31

WIP this might break things?

cdf9a1b

PR comments

e75837e

PR comments

0f25665

mystenmark force-pushed the mlogan-data-quarantine branch from 0f0293e to 0f25665 Compare February 4, 2025 05:36

mystenmark deployed to sui-typescript-aws-kms-test-env February 4, 2025 05:36 — with GitHub Actions Active

vercel bot deployed to Preview – sui-docs February 4, 2025 05:39 View deployment

		@@ -2272,10 +2201,15 @@ impl AuthorityPerEpochStore {
		digests: &[TransactionDigest],

Consensus Data Quarantining #20982

Are you sure you want to change the base?

Consensus Data Quarantining #20982

Conversation

mystenmark commented Jan 25, 2025 • edited Loading

Description

Test plan

vercel bot commented Jan 25, 2025 • edited Loading

aschran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mystenmark commented Jan 25, 2025 •

edited

Loading

vercel bot commented Jan 25, 2025 •

edited

Loading