add reasons to slash evidence (#414)

* add reasons to slash evidence

* fix CI failing

* Remove unnecessary clones

.encode() takes &self

* InvalidVr to InvalidValidRound

* Unrelated to this PR: Clarify reasoning/potentials behind dropping evidence

* Clarify prevotes in SlashEvidence test

* Replace use of read_to_end

* Restore decode_signed_message

---------

Co-authored-by: Luke Parker <[email protected]>

coordinator/src/tributary/scanner.rs

@@ -13,7 +13,7 @@ use serai_client::{validator_sets::primitives::ValidatorSet, subxt::utils::Encod
 use tributary::{
   TransactionKind, Transaction as TributaryTransaction, Block, TributaryReader,
   tendermint::{
-    tx::{TendermintTx, decode_evidence},
+    tx::{TendermintTx, Evidence, decode_signed_message},
     TendermintNetwork,
   },
 };
@@ -80,7 +80,23 @@ async fn handle_block<
       TributaryTransaction::Tendermint(TendermintTx::SlashEvidence(ev)) => {
         // Since the evidence is on the chain, it should already have been validated
         // We can just punish the signer
-        let msgs = decode_evidence::<TendermintNetwork<D, Transaction, P>>(&ev).unwrap();
+        let data = match ev {
+          Evidence::ConflictingMessages(first, second) => (first, Some(second)),
+          Evidence::ConflictingPrecommit(first, second) => (first, Some(second)),
+          Evidence::InvalidPrecommit(first) => (first, None),
+          Evidence::InvalidValidRound(first) => (first, None),
+        };
+        let msgs = (
+          decode_signed_message::<TendermintNetwork<D, Transaction, P>>(&data.0).unwrap(),
+          if data.1.is_some() {
+            Some(
+              decode_signed_message::<TendermintNetwork<D, Transaction, P>>(&data.1.unwrap())
+                .unwrap(),
+            )
+          } else {
+            None
+          },
+        );
 
         // Since anything with evidence is fundamentally faulty behavior, not just temporal errors,
         // mark the node as fatally slashed

coordinator/tributary/src/lib.rs

@@ -15,6 +15,8 @@ use ::tendermint::{
   TendermintMachine, TendermintHandle,
 };
 
+pub use ::tendermint::Evidence;
+
 use serai_db::Db;
 
 use tokio::sync::RwLock;

coordinator/tributary/src/tendermint/mod.rs

@@ -319,19 +319,19 @@ impl<D: Db, T: TransactionTrait, P: P2p> Network for TendermintNetwork<D, T, P>
     self.p2p.broadcast(self.genesis, to_broadcast).await
   }
 
-  async fn slash(&mut self, validator: Self::ValidatorId, slash_event: SlashEvent<Self>) {
+  async fn slash(&mut self, validator: Self::ValidatorId, slash_event: SlashEvent) {
     log::error!(
       "validator {} triggered a slash event on tributary {} (with evidence: {})",
       hex::encode(validator),
       hex::encode(self.genesis),
-      matches!(slash_event, SlashEvent::WithEvidence(_, _)),
+      matches!(slash_event, SlashEvent::WithEvidence(_)),
     );
 
     let signer = self.signer();
     let Some(tx) = (match slash_event {
-      SlashEvent::WithEvidence(m1, m2) => {
+      SlashEvent::WithEvidence(evidence) => {
         // create an unsigned evidence tx
-        Some(TendermintTx::SlashEvidence((m1, m2).encode()))
+        Some(TendermintTx::SlashEvidence(evidence))
       }
       SlashEvent::Id(_reason, _block, _round) => {
         // TODO: Increase locally observed slash points

coordinator/tributary/src/tendermint/tx.rs

@@ -1,6 +1,6 @@
 use std::io;
 
-use scale::Decode;
+use scale::{Encode, Decode, IoReader};
 
 use blake2::{Digest, Blake2s256};
 
@@ -19,53 +19,24 @@ use tendermint::{
   ext::{Network, Commit, RoundNumber, SignatureScheme},
 };
 
+pub use tendermint::Evidence;
+
 #[allow(clippy::large_enum_variant)]
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub enum TendermintTx {
-  SlashEvidence(Vec<u8>),
+  SlashEvidence(Evidence),
 }
 
 impl ReadWrite for TendermintTx {
   fn read<R: io::Read>(reader: &mut R) -> io::Result<Self> {
-    let mut kind = [0];
-    reader.read_exact(&mut kind)?;
-    match kind[0] {
-      0 => {
-        let mut len = [0; 4];
-        reader.read_exact(&mut len)?;
-        let mut len =
-          usize::try_from(u32::from_le_bytes(len)).expect("running on a 16-bit system?");
-
-        let mut data = vec![];
-
-        // Read chunk-by-chunk so a claimed 4 GB length doesn't cause a 4 GB allocation
-        // While we could check the length is sane, that'd require we know what a sane length is
-        // We'd also have to maintain that length's sanity even as other parts of the codebase,
-        // and even entire crates, change
-        // This is fine as it'll eventually hit the P2P message size limit, yet doesn't require
-        // knowing it nor does it make any assumptions
-        const CHUNK_LEN: usize = 1024;
-        let mut chunk = [0; CHUNK_LEN];
-        while len > 0 {
-          let to_read = len.min(CHUNK_LEN);
-          data.reserve(to_read);
-          reader.read_exact(&mut chunk[.. to_read])?;
-          data.extend(&chunk[.. to_read]);
-          len -= to_read;
-        }
-        Ok(TendermintTx::SlashEvidence(data))
-      }
-      _ => Err(io::Error::new(io::ErrorKind::Other, "invalid transaction type")),
-    }
+    Evidence::decode(&mut IoReader(reader))
+      .map(TendermintTx::SlashEvidence)
+      .map_err(|_| io::Error::new(io::ErrorKind::InvalidData, "invalid evidence format"))
   }
 
   fn write<W: io::Write>(&self, writer: &mut W) -> io::Result<()> {
     match self {
-      TendermintTx::SlashEvidence(ev) => {
-        writer.write_all(&[0])?;
-        writer.write_all(&u32::try_from(ev.len()).unwrap().to_le_bytes())?;
-        writer.write_all(ev)
-      }
+      TendermintTx::SlashEvidence(ev) => writer.write_all(&ev.encode()),
     }
   }
 }
@@ -92,13 +63,23 @@ impl Transaction for TendermintTx {
   }
 }
 
-pub fn decode_evidence<N: Network>(
-  mut ev: &[u8],
-) -> Result<(SignedMessageFor<N>, Option<SignedMessageFor<N>>), TransactionError> {
-  <(SignedMessageFor<N>, Option<SignedMessageFor<N>>)>::decode(&mut ev).map_err(|_| {
-    dbg!("failed to decode");
-    TransactionError::InvalidContent
-  })
+pub fn decode_signed_message<N: Network>(
+  mut data: &[u8],
+) -> Result<SignedMessageFor<N>, TransactionError> {
+  SignedMessageFor::<N>::decode(&mut data).map_err(|_| TransactionError::InvalidContent)
+}
+
+fn decode_and_verify_signed_message<N: Network>(
+  data: &[u8],
+  schema: &N::SignatureScheme,
+) -> Result<SignedMessageFor<N>, TransactionError> {
+  let msg = decode_signed_message::<N>(data)?;
+
+  // verify that evidence messages are signed correctly
+  if !msg.verify_signature(schema) {
+    Err(TransactionError::InvalidSignature)?
+  }
+  Ok(msg)
 }
 
 // TODO: Move this into tendermint-machine
@@ -115,70 +96,59 @@ pub(crate) fn verify_tendermint_tx<N: Network>(
   match tx {
     // TODO: Only allow one evidence per validator, since evidence is fatal
     TendermintTx::SlashEvidence(ev) => {
-      let (first, second) = decode_evidence::<N>(ev)?;
-
-      // verify that evidence messages are signed correctly
-      if !first.verify_signature(&schema) {
-        Err(TransactionError::InvalidSignature)?
-      }
-      let first = first.msg;
-
-      if let Some(second) = second {
-        if !second.verify_signature(&schema) {
-          Err(TransactionError::InvalidSignature)?
-        }
-        let second = second.msg;
-
-        // 2 types of evidence here
-        // 1- multiple distinct messages for the same block + round + step
-        // 2- precommitted to multiple blocks
+      match ev {
+        Evidence::ConflictingMessages(first, second) => {
+          let first = decode_and_verify_signed_message::<N>(first, &schema)?.msg;
+          let second = decode_and_verify_signed_message::<N>(second, &schema)?.msg;
+
+          // Make sure they're distinct messages, from the same sender, within the same block
+          if (first == second) || (first.sender != second.sender) || (first.block != second.block) {
+            Err(TransactionError::InvalidContent)?;
+          }
 
-        // Make sure they're distinct messages, from the same sender, within the same block
-        if (first == second) || (first.sender != second.sender) || (first.block != second.block) {
-          Err(TransactionError::InvalidContent)?;
+          // Distinct messages within the same step
+          if !((first.round == second.round) && (first.data.step() == second.data.step())) {
+            Err(TransactionError::InvalidContent)?;
+          }
         }
+        Evidence::ConflictingPrecommit(first, second) => {
+          let first = decode_and_verify_signed_message::<N>(first, &schema)?.msg;
+          let second = decode_and_verify_signed_message::<N>(second, &schema)?.msg;
 
-        // Distinct messages within the same step
-        if (first.round == second.round) && (first.data.step() == second.data.step()) {
-          return Ok(());
-        }
+          if (first.sender != second.sender) || (first.block != second.block) {
+            Err(TransactionError::InvalidContent)?;
+          }
 
-        // check whether messages are precommits to different blocks
-        // The inner signatures don't need to be verified since the outer signatures were
-        // While the inner signatures may be invalid, that would've yielded a invalid precommit
-        // signature slash instead of distinct precommit slash
-        if let Data::Precommit(Some((h1, _))) = first.data {
-          if let Data::Precommit(Some((h2, _))) = second.data {
-            if h1 == h2 {
-              Err(TransactionError::InvalidContent)?;
+          // check whether messages are precommits to different blocks
+          // The inner signatures don't need to be verified since the outer signatures were
+          // While the inner signatures may be invalid, that would've yielded a invalid precommit
+          // signature slash instead of distinct precommit slash
+          if let Data::Precommit(Some((h1, _))) = first.data {
+            if let Data::Precommit(Some((h2, _))) = second.data {
+              if h1 == h2 {
+                Err(TransactionError::InvalidContent)?;
+              }
+              return Ok(());
             }
-            return Ok(());
           }
-        }
 
-        // No fault identified
-        Err(TransactionError::InvalidContent)?
-      }
+          // No fault identified
+          Err(TransactionError::InvalidContent)?
+        }
+        Evidence::InvalidPrecommit(msg) => {
+          let msg = decode_and_verify_signed_message::<N>(msg, &schema)?.msg;
 
-      // 2 types of evidence can be here
-      // 1- invalid commit signature
-      // 2- vr number that was greater than or equal to the current round
-      match &first.data {
-        Data::Proposal(vr, _) => {
-          // check the vr
-          if vr.is_none() || vr.unwrap().0 < first.round.0 {
+          let Data::Precommit(Some((id, sig))) = &msg.data else {
             Err(TransactionError::InvalidContent)?
-          }
-        }
-        Data::Precommit(Some((id, sig))) => {
+          };
           // TODO: We need to be passed in the genesis time to handle this edge case
-          if first.block.0 == 0 {
+          if msg.block.0 == 0 {
             todo!("invalid precommit signature on first block")
           }
 
           // get the last commit
           // TODO: Why do we use u32 when Tendermint uses u64?
-          let prior_commit = match u32::try_from(first.block.0 - 1) {
+          let prior_commit = match u32::try_from(msg.block.0 - 1) {
             Ok(n) => match commit(n) {
               Some(c) => c,
               // If we have yet to sync the block in question, we will return InvalidContent based
@@ -193,16 +163,25 @@ pub(crate) fn verify_tendermint_tx<N: Network>(
 
           // calculate the end time till the msg round
           let mut last_end_time = CanonicalInstant::new(prior_commit.end_time);
-          for r in 0 ..= first.round.0 {
+          for r in 0 ..= msg.round.0 {
             last_end_time = RoundData::<N>::new(RoundNumber(r), last_end_time).end_time();
           }
 
           // verify that the commit was actually invalid
-          if schema.verify(first.sender, &commit_msg(last_end_time.canonical(), id.as_ref()), sig) {
+          if schema.verify(msg.sender, &commit_msg(last_end_time.canonical(), id.as_ref()), sig) {
+            Err(TransactionError::InvalidContent)?
+          }
+        }
+        Evidence::InvalidValidRound(msg) => {
+          let msg = decode_and_verify_signed_message::<N>(msg, &schema)?.msg;
+
+          let Data::Proposal(Some(vr), _) = &msg.data else {
+            Err(TransactionError::InvalidContent)?
+          };
+          if vr.0 < msg.round.0 {
             Err(TransactionError::InvalidContent)?
           }
         }
-        _ => Err(TransactionError::InvalidContent)?,
       }
     }
   }

coordinator/tributary/src/tests/transaction/mod.rs

@@ -16,7 +16,7 @@ use scale::Encode;
 
 use ::tendermint::{
   ext::{Network, Signer as SignerTrait, SignatureScheme, BlockNumber, RoundNumber},
-  SignedMessageFor, DataFor, Message, SignedMessage, Data,
+  SignedMessageFor, DataFor, Message, SignedMessage, Data, Evidence,
 };
 
 use crate::{
@@ -212,5 +212,5 @@ pub async fn random_evidence_tx<N: Network>(
   let data = Data::Proposal(Some(RoundNumber(0)), b);
   let signer_id = signer.validator_id().await.unwrap();
   let signed = signed_from_data::<N>(signer, signer_id, 0, 0, data).await;
-  TendermintTx::SlashEvidence((signed, None::<SignedMessageFor<N>>).encode())
+  TendermintTx::SlashEvidence(Evidence::InvalidValidRound(signed.encode()))
 }