Properly save justifications and GrandPa commits for later (#2400)

* Properly save justifications and GrandPa commits

* PR number

* Oops, forgot two todos

bin/full-node/src/run/consensus_service.rs

@@ -1089,9 +1089,9 @@ impl SyncBackground {
                     }
                 }
 
-                all::ProcessOne::VerifyJustification(verify) => {
+                all::ProcessOne::VerifyFinalityProof(verify) => {
                     let span = tracing::debug_span!(
-                        "justification-verification",
+                        "finality-proof-verification",
                         outcome = tracing::field::Empty,
                         error = tracing::field::Empty,
                     );
@@ -1100,7 +1100,7 @@ impl SyncBackground {
                     match verify.perform() {
                         (
                             sync_out,
-                            all::JustificationVerifyOutcome::NewFinalized {
+                            all::FinalityProofVerifyOutcome::NewFinalized {
                                 finalized_blocks,
                                 updates_best_block,
                             },
@@ -1158,7 +1158,23 @@ impl SyncBackground {
                             database_set_finalized(&self.database, new_finalized_hash).await;
                             continue;
                         }
-                        (sync_out, all::JustificationVerifyOutcome::Error(error)) => {
+                        (sync_out, all::FinalityProofVerifyOutcome::GrandpaCommitPending) => {
+                            span.record("outcome", &"pending");
+                            self.sync = sync_out;
+                            continue;
+                        }
+                        (sync_out, all::FinalityProofVerifyOutcome::AlreadyFinalized) => {
+                            span.record("outcome", &"already-finalized");
+                            self.sync = sync_out;
+                            continue;
+                        }
+                        (sync_out, all::FinalityProofVerifyOutcome::GrandpaCommitError(error)) => {
+                            span.record("outcome", &"failure");
+                            span.record("error", &tracing::field::display(error));
+                            self.sync = sync_out;
+                            continue;
+                        }
+                        (sync_out, all::FinalityProofVerifyOutcome::JustificationError(error)) => {
                             span.record("outcome", &"failure");
                             span.record("error", &tracing::field::display(error));
                             self.sync = sync_out;

bin/full-node/src/run/network_service.rs

@@ -916,10 +916,11 @@ async fn update_round(inner: &Arc<Inner>, event_senders: &mut [mpsc::Sender<Even
                 }
                 service::Event::GrandpaCommitMessage {
                     chain_index,
+                    peer_id,
                     message,
                 } => {
                     tracing::debug!(
-                        %chain_index,
+                        %chain_index, %peer_id,
                         target_hash = %HashDisplay(message.decode().message.target_hash),
                         "grandpa-commit-message"
                     );

bin/light-base/src/network_service.rs

@@ -829,6 +829,7 @@ pub enum Event {
     },
     /// Received a GrandPa commit message from the network.
     GrandpaCommitMessage {
+        peer_id: PeerId,
         chain_index: usize,
         message: service::EncodedGrandpaCommitMessage,
     },
@@ -1159,6 +1160,7 @@ async fn update_round<TPlat: Platform>(
                 }
                 service::Event::GrandpaCommitMessage {
                     chain_index,
+                    peer_id,
                     message,
                 } => {
                     log::debug!(
@@ -1169,6 +1171,7 @@ async fn update_round<TPlat: Platform>(
                     );
                     break Event::GrandpaCommitMessage {
                         chain_index,
+                        peer_id,
                         message,
                     };
                 }

bin/light-base/src/sync_service/standalone.rs

@@ -95,7 +95,7 @@ pub(super) async fn start_standalone_chain<TPlat: Platform>(
 
     // Main loop of the syncing logic.
     //
-    // This loop contains some CPU-heavy operations (e.g. verifying justifications and warp sync
+    // This loop contains some CPU-heavy operations (e.g. verifying finality proofs and warp sync
     // proofs) but also responding to messages sent by the foreground sync service. In order to
     // avoid long delays in responding to foreground messages, the CPU-heavy operations are split
     // into small chunks, and each iteration of the loop processes at most one of these chunks and
@@ -575,7 +575,7 @@ impl<TPlat: Platform> Task<TPlat> {
         true
     }
 
-    /// Verifies one block, or justification, or warp sync fragment, etc. that is queued for
+    /// Verifies one block, or finality proof, or warp sync fragment, etc. that is queued for
     /// verification.
     ///
     /// Returns `self` and a boolean indicating whether something has been processed.
@@ -748,12 +748,12 @@ impl<TPlat: Platform> Task<TPlat> {
                 }
             }
 
-            all::ProcessOne::VerifyJustification(verify) => {
-                // Justification to verify.
+            all::ProcessOne::VerifyFinalityProof(verify) => {
+                // Finality proof to verify.
                 match verify.perform() {
                     (
                         sync,
-                        all::JustificationVerifyOutcome::NewFinalized {
+                        all::FinalityProofVerifyOutcome::NewFinalized {
                             updates_best_block,
                             finalized_blocks,
                             ..
@@ -763,7 +763,7 @@ impl<TPlat: Platform> Task<TPlat> {
 
                         log::debug!(
                             target: &self.log_target,
-                            "Sync => JustificationVerified(finalized_blocks={})",
+                            "Sync => FinalityProofVerified(finalized_blocks={})",
                             finalized_blocks.len(),
                         );
 
@@ -778,10 +778,18 @@ impl<TPlat: Platform> Task<TPlat> {
                         });
                     }
 
-                    (sync, all::JustificationVerifyOutcome::Error(error)) => {
+                    (
+                        sync,
+                        all::FinalityProofVerifyOutcome::AlreadyFinalized
+                        | all::FinalityProofVerifyOutcome::GrandpaCommitPending,
+                    ) => {
+                        self.sync = sync;
+                    }
+
+                    (sync, all::FinalityProofVerifyOutcome::JustificationError(error)) => {
                         self.sync = sync;
 
-                        // TODO: print which peer sent the justification
+                        // TODO: print which peer sent the proof
                         log::debug!(
                             target: &self.log_target,
                             "Sync => JustificationVerificationError(error={:?})",
@@ -794,6 +802,23 @@ impl<TPlat: Platform> Task<TPlat> {
                             error
                         );
                     }
+
+                    (sync, all::FinalityProofVerifyOutcome::GrandpaCommitError(error)) => {
+                        self.sync = sync;
+
+                        // TODO: print which peer sent the proof
+                        log::debug!(
+                            target: &self.log_target,
+                            "Sync => GrandpaCommitVerificationError(error={:?})",
+                            error,
+                        );
+
+                        log::warn!(
+                            target: &self.log_target,
+                            "Error while verifying GrandPa commit: {}",
+                            error
+                        );
+                    }
                 }
             }
 
@@ -1032,9 +1057,14 @@ impl<TPlat: Platform> Task<TPlat> {
 
             network_service::Event::GrandpaCommitMessage {
                 chain_index,
+                peer_id,
                 message,
             } if chain_index == self.network_chain_index => {
-                match self.sync.grandpa_commit_message(&message.as_encoded()) {
+                let sync_source_id = *self.peers_source_id_map.get(&peer_id).unwrap();
+                match self
+                    .sync
+                    .grandpa_commit_message(sync_source_id, &message.as_encoded())
+                {
                     Ok(()) => {
                         // TODO: print more details
                         log::debug!(

bin/wasm-node/CHANGELOG.md

@@ -6,6 +6,11 @@
 
 - When a database and a chain specification checkpoint are both provided to `addChain`, the block in the database is used only if it has a higher block number than the block in the chain specification checkpoint. This makes it possible to bypass issues where smoldot is incapable of syncing over a certain block by updating the chain specification, without having to manually clear existing databases. ([#2401](https://github.com/paritytech/smoldot/pull/2401))
 
+### Fixed
+
+- Fix errors about verifying justifications. Justifications and Grandpa commits that can't be verified yet are now properly stored in memory in order to be verified later, instead of producing errors. ([#2400](https://github.com/paritytech/smoldot/pull/2400))
+- Fix issue where unverified justifications would overwrite one another, meaning that an invalid justification could potentially prevent a valid justification from being taken into account. ([#2400](https://github.com/paritytech/smoldot/pull/2400))
+
 ## 0.6.19 - 2022-06-14
 
 ### Fixed

src/chain/blocks_tree/finality.rs

@@ -115,12 +115,12 @@ impl<T> NonFinalizedTree<T> {
     /// be used to apply the finalization.
     pub fn verify_grandpa_commit_message(
         &mut self,
-        scale_encoded_message: &[u8],
+        scale_encoded_commit: &[u8],
     ) -> Result<FinalityApply<T>, CommitVerifyError> {
         self.inner
             .as_mut()
             .unwrap()
-            .verify_grandpa_commit_message(scale_encoded_message)
+            .verify_grandpa_commit_message(scale_encoded_commit)
     }
 
     /// Sets the latest known finalized block. Trying to verify a block that isn't a descendant of
@@ -337,15 +337,16 @@ impl<T> NonFinalizedTreeInner<T> {
     /// See [`NonFinalizedTree::verify_grandpa_commit_message`].
     fn verify_grandpa_commit_message(
         &mut self,
-        scale_encoded_message: &[u8],
+        verify_grandpa_commit_message: &[u8],
     ) -> Result<FinalityApply<T>, CommitVerifyError> {
         // The code below would panic if the chain doesn't use Grandpa.
         if !matches!(self.finality, Finality::Grandpa { .. }) {
             return Err(CommitVerifyError::NotGrandpa);
         }
 
-        let decoded_commit = grandpa::commit::decode::decode_grandpa_commit(scale_encoded_message)
-            .map_err(|_| CommitVerifyError::InvalidCommit)?;
+        let decoded_commit =
+            grandpa::commit::decode::decode_grandpa_commit(verify_grandpa_commit_message)
+                .map_err(|_| CommitVerifyError::InvalidCommit)?;
 
         // Delegate the first step to the other function.
         let (block_index, expected_authorities_set_id, authorities_list) = self
@@ -356,7 +357,7 @@ impl<T> NonFinalizedTreeInner<T> {
             .map_err(CommitVerifyError::FinalityVerify)?;
 
         let mut verification = grandpa::commit::verify::verify(grandpa::commit::verify::Config {
-            commit: scale_encoded_message,
+            commit: verify_grandpa_commit_message,
             expected_authorities_set_id,
             num_authorities: u32::try_from(authorities_list.clone().count()).unwrap(),
         });
@@ -371,7 +372,9 @@ impl<T> NonFinalizedTreeInner<T> {
                     });
                 }
                 grandpa::commit::verify::InProgress::FinishedUnknown => {
-                    return Err(CommitVerifyError::NotEnoughKnownBlocks)
+                    return Err(CommitVerifyError::NotEnoughKnownBlocks {
+                        target_block_number: u64::from(decoded_commit.message.target_number),
+                    })
                 }
                 grandpa::commit::verify::InProgress::Finished(Err(error)) => {
                     return Err(CommitVerifyError::VerificationFailed(error))
@@ -638,7 +641,11 @@ pub enum CommitVerifyError {
     ///
     /// This doesn't mean that the commit is bad, but that it can't be verified without adding
     /// more blocks to the tree.
-    NotEnoughKnownBlocks,
+    #[display(fmt = "Not enough blocks are known to verify this commit")]
+    NotEnoughKnownBlocks {
+        /// Block number that the commit targets.
+        target_block_number: u64,
+    },
     /// The commit verification has failed. The commit is invalid and should be thrown away.
     #[display(fmt = "{}", _0)]
     VerificationFailed(grandpa::commit::verify::Error),

src/network/service.rs

@@ -1595,6 +1595,7 @@ where
                     if let protocol::GrandpaNotificationRef::Commit(_) = decoded_notif {
                         break Some(Event::GrandpaCommitMessage {
                             chain_index,
+                            peer_id,
                             message: EncodedGrandpaCommitMessage(notification),
                         });
                     }
@@ -2237,6 +2238,8 @@ pub enum Event {
 
     /// Received a GrandPa commit message from the network.
     GrandpaCommitMessage {
+        /// Identity of the sender of the message.
+        peer_id: PeerId,
         /// Index of the chain the commit message relates to.
         chain_index: usize,
         message: EncodedGrandpaCommitMessage,