feature(consensus): Recovery #1043

AlejandroCabeza · 2025-02-11T15:10:50Z

1. What does this PR implement?

Rebuild Node state from a previously saved state:

Implement a StorageAdapter wrapper over the storage relay.
Save tip, and Block and LedgerState and NoteWitness in security position.
Use saved data (if there is) to initialise Cryptarchia and Leader, defaulting to genesis strategy if no data is available.
Cleanup for clarity: Renaming, reordering, logs, etc.

Test plans to be implemented: #1139

2. Does the code have enough context to be clearly understood?

Yes

3. Who are the specification authors and who is accountable for this PR?

@AlejandroCabeza

4. Is the specification accurate and complete?

N/A

5. Does the implementation introduce changes in the specification?

N/A

Checklist

Warning

Do not merge the PR if any of the following is missing:

1. Description added.
2. Context and links to Specification document(s) added.
3. Main contact(s) (developers and specification authors) added
4. Implementation and Specification are 100% in sync including changes. This is critical.
5. Link PR to a specific milestone.

danielSanchezQ

Pretty clean approach. Added some comments. Probably we want to add a basic test for each recovery strategy.

danielSanchezQ · 2025-02-27T13:50:37Z

nomos-services/cryptarchia-consensus/src/lib.rs

+        });
+
+        // To avoid confusion, the order is reversed so it fits the natural `from..to` order
+        blocks.collect::<Vec<_>>().await.into_iter().rev().collect()


I think it would be better to return the stream here instead of the collected vec. Then collect it externally when used. Is more generic case imo.

The reason for this is to force it to natural order, because afaik, you can't reverse an stream.
Due to unfold's nature, items are in to to from order, which could be confusing to callers.

There is an iter.rev() which might be what you are looking for? This works on iterators, not sure if streams are something else?

That's probably what I'm already using in the into_iter().rev() section. It makes sense to me that you can't chain a Stream with a rev without explicitly storing it first: You need to store the whole stream in memory (e.g.: vector) before reversing it, otherwise how do you know what stream item is the first one?

danielSanchezQ · 2025-02-27T13:51:25Z

nomos-services/cryptarchia-consensus/src/lib.rs

+        relays: &CryptarchiaConsensusRelays<
+            BlendAdapter,
+            BS,
+            ClPool,
+            ClPoolAdapter,
+            DaPool,
+            DaPoolAdapter,
+            NetAdapter,
+            SamplingBackend,
+            SamplingRng,
+            Storage,
+            TxS,
+        >,


Now that I see this. CryptarchiaConsensusRelays should be a type alias to hide generics except in a single point.

Is there a way to do it without an associated type?

danielSanchezQ · 2025-02-27T13:55:54Z

nomos-services/cryptarchia-consensus/src/lib.rs

+        match initial_state.recovery_strategy() {
+            CryptarchiaInitialisationStrategy::Genesis => {
+                info!("Building Cryptarchia from genesis...");
+                let leader = Leader::from_genesis(genesis_id, leader_config, ledger_config.clone());
+                let cryptarchia = Cryptarchia::new(genesis_id, genesis_state, ledger_config);
+
+                (cryptarchia, leader)
+            }
+            CryptarchiaInitialisationStrategy::RecoveryFromGenesis(GenesisRecoveryStrategy {
+                tip,
+            }) => {
+                info!("Recovering Cryptarchia with Genesis strategy.");
+                let mut leader =
+                    Leader::from_genesis(genesis_id, leader_config, ledger_config.clone());
+                let cryptarchia = Self::recover_cryptarchia(
+                    genesis_id,
+                    genesis_state,
+                    tip,
+                    &mut leader,
+                    ledger_config,
+                    relays,
+                    block_subscription_sender,
+                )
+                .await;
+
+                (cryptarchia, leader)
+            }
+            CryptarchiaInitialisationStrategy::RecoveryFromSecurity(strategy) => {
+                let SecurityRecoveryStrategy {
+                    tip,
+                    security_block_id,
+                    security_ledger_state,
+                    security_leader_notes,
+                } = *strategy;
+
+                info!("Recovering Cryptarchia with Security strategy.");
+                let mut leader = Leader::new(
+                    security_block_id,
+                    security_leader_notes,
+                    leader_config.nf_sk,
+                    ledger_config.clone(),
+                );
+
+                let cryptarchia = Self::recover_cryptarchia(
+                    security_block_id,
+                    security_ledger_state,
+                    tip,
+                    &mut leader,
+                    ledger_config,
+                    relays,
+                    block_subscription_sender,
+                )
+                .await;
+
+                (cryptarchia, leader)
+            }
+        }
+    }


Nitpick: This could be refactore as an implementation of CryptarchiaInitialisationStrategy (?), with independent methods for each case.

impl CryptarchiaInitializationStrategy { pub fn recover(&self, *args) -> (Cryptarchia, Leader) { match self { CryptarchiaInitialisationStrategy::Genesis => recover_from_genesis(self, *args), ... } } }

Or at least separated methods for each branch.

The thought did cross my mind but I kinda discarded because I assumed it'd get a bit messy with parameters and such. I'll give it a try. Thanks!

As good as it sounded moving that code to the strategy structs, it propagates generics to them as well which seemed a bit messy.

For now I ended up just separating them into methods for each branch, and they can be easily moved into strategy if we want.

Do you guys consider worth it worth? That is, moving them to the strategy section despite that.

ntn-x2 · 2025-02-27T14:09:37Z

consensus/cryptarchia-engine/src/lib.rs

@@ -143,7 +143,7 @@ impl<Id> Cryptarchia<Id>
 where
    Id: Eq + std::hash::Hash + Copy,
 {
-    pub fn from_genesis(id: Id, config: Config) -> Self {
+    pub fn new(id: Id, config: Config) -> Self {


Could there be a clearer name for this method?

I'd say it's quite accurate. It's really not doing anything fancy, nor anything attached to a special state (genesis), just getting an instance with a given id.

Naming is fine I think.

ntn-x2 · 2025-02-27T14:11:59Z

nomos-services/cryptarchia-consensus/src/leadership.rs

+        LeaderConfig { notes, nf_sk }: LeaderConfig,
+        config: Config,
+    ) -> Self {
+        Leader::new(genesis, notes, nf_sk, config)


My pedantic clippy suggests me this as a better alternative

Suggested change

Leader::new(genesis, notes, nf_sk, config)

Self::new(genesis, notes, nf_sk, config)

That slipped by, thanks.

ntn-x2 · 2025-02-27T14:12:40Z

nomos-services/cryptarchia-consensus/src/leadership.rs

@@ -126,6 +135,10 @@ impl Leader {

        None
    }
+
+    pub(crate) fn notes(&self, header_id: &HeaderId) -> Option<&Vec<NoteWitness>> {


Would it make sense to rater return an impl Iterator here? Then the consumer can collect it however they want to.

self.notes is already a Vec, do you think it's actually worth casting it to return it here?

Probaly Option<&[NoteWitness]> is fine. From that can be transformer into iter if needed, or be indexed (which probably is not needed but anyway...).

nomos-services/cryptarchia-consensus/src/states.rs

ntn-x2 · 2025-02-27T14:17:49Z

nomos-services/cryptarchia-consensus/src/states.rs

+    type Settings = CryptarchiaSettings<TxS, BxS, NetworkAdapterSettings, BlendAdapterSettings>;
+    type Error = Error;
+
+    fn from_settings(_settings: &Self::Settings) -> Result<Self, Self::Error> {


Nothing to do with this PR, but this is the bit I was talking about elsewhere. All states must do this, and it looks like every time the implementation will always be the same, which to me indicates it's not optimal design.

nomos-services/cryptarchia-consensus/src/lib.rs

generics' constraints.

…t level.

…ist in StorageAdapter.

… Improve docs.

…ead. Simplify parameters on restore_from_recovery.

…sed on recovery strategy, now it doesn't need to update from genesis to k.

danielSanchezQ · 2025-03-14T14:26:56Z

nomos-services/cryptarchia-consensus/src/relays.rs

    DaPool::BlockId: Debug,
    DaPool::Item: Debug + DeserializeOwned + Eq + Hash + Clone + Send + Sync + 'static,
    DaPool::Key: Debug + 'static,
+    //


Yup, from the rebase, still cleaning up stuff.

AlejandroCabeza added the consensus label Feb 11, 2025

AlejandroCabeza added this to the Iteration 10 milestone Feb 11, 2025

AlejandroCabeza self-assigned this Feb 11, 2025

Base automatically changed from chore/group-consensus-relays to master February 17, 2025 12:47

AlejandroCabeza force-pushed the feature/recovery/consensus branch from af9aa0b to 3be949e Compare February 19, 2025 21:33

AlejandroCabeza changed the title ~~[DRAFT] feature(consensus): Recovery~~ feature(consensus): Recovery Feb 20, 2025

AlejandroCabeza force-pushed the feature/recovery/consensus branch 2 times, most recently from 30cb871 to e879e44 Compare February 21, 2025 13:35

AlejandroCabeza marked this pull request as ready for review February 21, 2025 13:35

AlejandroCabeza requested review from danielSanchezQ, ntn-x2 and zeegomo February 21, 2025 13:36

danielSanchezQ requested changes Feb 27, 2025

View reviewed changes

ntn-x2 reviewed Feb 27, 2025

View reviewed changes

AlejandroCabeza force-pushed the feature/recovery/consensus branch from e879e44 to 462ab57 Compare February 28, 2025 21:45

AlejandroCabeza added 15 commits March 14, 2025 13:19

Group relays into CryptarchiaConsensusRelays. Remove unnecessary

aba7eba

generics' constraints.

Reorder CryptarchiaConsensus impl blocks generic constraints.

a48aa55

Save block at security_param position.

be677d5

Add optimize todo.

9020d39

Fix parameter types.

877a82a

Implement StorageAdapter trait and struct.

caeddde

Add save_security_block function to StorageAdapter. Move docs to trai…

411674c

…t level.

Use StorageAdapter instead of relay. Remove functions that already ex…

55b1d3d

…ist in StorageAdapter.

Refactor security param block fetch from StorageAdapter to Consensus.…

0c43875

… Improve docs.

Remove unneeded methods from StorageAdapter

536a528

Use state to store tip and security block header id.

04f96d6

Move constraints to a higher layer.

de08d7d

Recover from security block to tip.

e752167

Improve log messages.

7719604

Add security ledger state to State.

5823258

AlejandroCabeza added 20 commits March 14, 2025 14:55

Move state update behind a function.

b28962e

Add State initializer shortcut from cryptarchia.

6a8d35e

Remove old can_recover (not useful), use old should_recover body inst…

f249ac9

…ead. Simplify parameters on restore_from_recovery.

Add recovery logic to rebuild leader and cryptarchia.

4ec816d

Rename initializer methods

66cf2d7

Refactor recovery strategy.

26ae607

Refactor cryptarchia state into its own module.

90c7b0a

Improve function name and docs

09e25fe

Move cryptarchia building to separate function.

f155877

Rename function for clarity

bf026f0

Extract follow chain range into a function.

18b4fc7

Reorder parameters for clarity

e8fd5c2

Reorder parameters so they all follow the same natural order.

4187756

Store Leader security notes for recovery.

4c755c7

Save security leader notes for recovery. Create Leader dynamically ba…

fad6a49

…sed on recovery strategy, now it doesn't need to update from genesis to k.

Simplify Leader's news.

77c938e

Improve function docs, reorder imports.

81bcd24

Reorder imports.

cef9000

Abstract recovery to methods.

b6568e3

Add documentation and PR suggestions.

c151fbc

AlejandroCabeza force-pushed the feature/recovery/consensus branch from 462ab57 to c151fbc Compare March 14, 2025 14:23

danielSanchezQ reviewed Mar 14, 2025

View reviewed changes

AlejandroCabeza added 4 commits March 14, 2025 16:41

Return slice on Leader::notes. Add some clippy warnings' exceptions.

8433391

Fix comment.

e6cb63f

Remove leftover empty comments.

a7c72da

Add initialisation strategy tests.

d9a56f6

AlejandroCabeza requested a review from danielSanchezQ March 14, 2025 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(consensus): Recovery #1043

feature(consensus): Recovery #1043

AlejandroCabeza commented Feb 11, 2025 •

edited

Loading

danielSanchezQ left a comment

danielSanchezQ Feb 27, 2025

AlejandroCabeza Feb 28, 2025

ntn-x2 Mar 6, 2025

AlejandroCabeza Mar 14, 2025

danielSanchezQ Feb 27, 2025

AlejandroCabeza Feb 28, 2025

danielSanchezQ Feb 27, 2025

AlejandroCabeza Feb 28, 2025 •

edited

Loading

AlejandroCabeza Mar 14, 2025 •

edited

Loading

ntn-x2 Feb 27, 2025

AlejandroCabeza Feb 28, 2025 •

edited

Loading

danielSanchezQ Mar 13, 2025

ntn-x2 Feb 27, 2025

AlejandroCabeza Feb 28, 2025

ntn-x2 Feb 27, 2025

AlejandroCabeza Feb 28, 2025

danielSanchezQ Mar 13, 2025

ntn-x2 Feb 27, 2025

danielSanchezQ Mar 14, 2025

AlejandroCabeza Mar 14, 2025 •

edited

Loading

	Leader::new(genesis, notes, nf_sk, config)
	Self::new(genesis, notes, nf_sk, config)

feature(consensus): Recovery #1043

Are you sure you want to change the base?

feature(consensus): Recovery #1043

Conversation

AlejandroCabeza commented Feb 11, 2025 • edited Loading

1. What does this PR implement?

2. Does the code have enough context to be clearly understood?

3. Who are the specification authors and who is accountable for this PR?

4. Is the specification accurate and complete?

5. Does the implementation introduce changes in the specification?

Checklist

danielSanchezQ 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

AlejandroCabeza Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

AlejandroCabeza Mar 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlejandroCabeza Feb 28, 2025 • edited Loading

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

AlejandroCabeza Mar 14, 2025 • edited Loading

Choose a reason for hiding this comment

AlejandroCabeza commented Feb 11, 2025 •

edited

Loading

AlejandroCabeza Feb 28, 2025 •

edited

Loading

AlejandroCabeza Mar 14, 2025 •

edited

Loading

AlejandroCabeza Feb 28, 2025 •

edited

Loading

AlejandroCabeza Mar 14, 2025 •

edited

Loading