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 <lukeparker5132@gmail.com>

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)?;
+      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;
 
-      // verify that evidence messages are signed correctly
-      if !first.verify_signature(&schema) {
-        Err(TransactionError::InvalidSignature)?
-      }
-      let first = first.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)?;
+          }
 
-      if let Some(second) = second {
-        if !second.verify_signature(&schema) {
-          Err(TransactionError::InvalidSignature)?
+          // Distinct messages within the same step
+          if !((first.round == second.round) && (first.data.step() == second.data.step())) {
+            Err(TransactionError::InvalidContent)?;
+          }
         }
-        let second = second.msg;
+        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;
 
-        // 2 types of evidence here
-        // 1- multiple distinct messages for the same block + round + step
-        // 2- precommitted to multiple blocks
+          if (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()) {
-          return Ok(());
-        }
-
-        // 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)?
         }
+        Evidence::InvalidPrecommit(msg) => {
+          let msg = decode_and_verify_signed_message::<N>(msg, &schema)?.msg;
 
-        // No fault identified
-        Err(TransactionError::InvalidContent)?
-      }
-
-      // 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()))
 }

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

@@ -10,7 +10,7 @@ use scale::Encode;
 use tendermint::{
   time::CanonicalInstant,
   round::RoundData,
-  Data, SignedMessageFor, commit_msg,
+  Data, commit_msg, Evidence,
   ext::{RoundNumber, Commit, Signer as SignerTrait},
 };
 
@@ -51,7 +51,7 @@ async fn invalid_valid_round() {
     async move {
       let data = Data::Proposal(valid_round, TendermintBlock(vec![]));
       let signed = signed_from_data::<N>(signer.clone().into(), signer_id, 0, 0, data).await;
-      (signed.clone(), TendermintTx::SlashEvidence((signed, None::<SignedMessageFor<N>>).encode()))
+      (signed.clone(), TendermintTx::SlashEvidence(Evidence::InvalidValidRound(signed.encode())))
     }
   };
 
@@ -69,7 +69,7 @@ async fn invalid_valid_round() {
   let mut random_sig = [0u8; 64];
   OsRng.fill_bytes(&mut random_sig);
   signed.sig = random_sig;
-  let tx = TendermintTx::SlashEvidence((signed.clone(), None::<SignedMessageFor<N>>).encode());
+  let tx = TendermintTx::SlashEvidence(Evidence::InvalidValidRound(signed.encode()));
 
   // should fail
   assert!(verify_tendermint_tx::<N>(&tx, validators, commit).is_err());
@@ -89,7 +89,7 @@ async fn invalid_precommit_signature() {
       let signed =
         signed_from_data::<N>(signer.clone().into(), signer_id, 1, 0, Data::Precommit(precommit))
           .await;
-      (signed.clone(), TendermintTx::SlashEvidence((signed, None::<SignedMessageFor<N>>).encode()))
+      (signed.clone(), TendermintTx::SlashEvidence(Evidence::InvalidPrecommit(signed.encode())))
     }
   };
 
@@ -119,7 +119,7 @@ async fn invalid_precommit_signature() {
     let mut random_sig = [0u8; 64];
     OsRng.fill_bytes(&mut random_sig);
     signed.sig = random_sig;
-    let tx = TendermintTx::SlashEvidence((signed.clone(), None::<SignedMessageFor<N>>).encode());
+    let tx = TendermintTx::SlashEvidence(Evidence::InvalidPrecommit(signed.encode()));
     assert!(verify_tendermint_tx::<N>(&tx, validators, commit).is_err());
   }
 }
@@ -134,19 +134,45 @@ async fn evidence_with_prevote() {
   let prevote = |block_id| {
     let signer = signer.clone();
     async move {
-      TendermintTx::SlashEvidence(
-        (
-          signed_from_data::<N>(signer.clone().into(), signer_id, 0, 0, Data::Prevote(block_id))
-            .await,
-          None::<SignedMessageFor<N>>,
-        )
+      // it should fail for all reasons.
+      let mut txs = vec![];
+      txs.push(TendermintTx::SlashEvidence(Evidence::InvalidPrecommit(
+        signed_from_data::<N>(signer.clone().into(), signer_id, 0, 0, Data::Prevote(block_id))
+          .await
           .encode(),
-      )
+      )));
+      txs.push(TendermintTx::SlashEvidence(Evidence::InvalidValidRound(
+        signed_from_data::<N>(signer.clone().into(), signer_id, 0, 0, Data::Prevote(block_id))
+          .await
+          .encode(),
+      )));
+      // Since these require a second message, provide this one again
+      // ConflictingMessages can be fired for actually conflicting Prevotes however
+      txs.push(TendermintTx::SlashEvidence(Evidence::ConflictingMessages(
+        signed_from_data::<N>(signer.clone().into(), signer_id, 0, 0, Data::Prevote(block_id))
+          .await
+          .encode(),
+        signed_from_data::<N>(signer.clone().into(), signer_id, 0, 0, Data::Prevote(block_id))
+          .await
+          .encode(),
+      )));
+      txs.push(TendermintTx::SlashEvidence(Evidence::ConflictingPrecommit(
+        signed_from_data::<N>(signer.clone().into(), signer_id, 0, 0, Data::Prevote(block_id))
+          .await
+          .encode(),
+        signed_from_data::<N>(signer.clone().into(), signer_id, 0, 0, Data::Prevote(block_id))
+          .await
+          .encode(),
+      )));
+      txs
     }
   };
 
-  // No prevote message should be valid as slash evidence at this time
-  for prevote in [prevote(None).await, prevote(Some([0x22u8; 32])).await] {
+  // No prevote message alone should be valid as slash evidence at this time
+  for prevote in prevote(None).await {
+    assert!(verify_tendermint_tx::<N>(&prevote, validators.clone(), commit).is_err());
+  }
+  for prevote in prevote(Some([0x22u8; 32])).await {
     assert!(verify_tendermint_tx::<N>(&prevote, validators.clone(), commit).is_err());
   }
 }
@@ -169,23 +195,35 @@ async fn conflicting_msgs_evidence_tx() {
   {
     // non-conflicting data should fail
     let signed_1 = signed_for_b_r(0, 0, Data::Proposal(None, TendermintBlock(vec![0x11]))).await;
-    let tx = TendermintTx::SlashEvidence((&signed_1, Some(&signed_1)).encode());
+    let tx = TendermintTx::SlashEvidence(Evidence::ConflictingMessages(
+      signed_1.encode(),
+      signed_1.encode(),
+    ));
     assert!(verify_tendermint_tx::<N>(&tx, validators.clone(), commit).is_err());
 
     // conflicting data should pass
     let signed_2 = signed_for_b_r(0, 0, Data::Proposal(None, TendermintBlock(vec![0x22]))).await;
-    let tx = TendermintTx::SlashEvidence((&signed_1, Some(signed_2)).encode());
+    let tx = TendermintTx::SlashEvidence(Evidence::ConflictingMessages(
+      signed_1.encode(),
+      signed_2.encode(),
+    ));
     verify_tendermint_tx::<N>(&tx, validators.clone(), commit).unwrap();
 
     // Except if it has a distinct round number, as we don't check cross-round conflicts
     // (except for Precommit)
     let signed_2 = signed_for_b_r(0, 1, Data::Proposal(None, TendermintBlock(vec![0x22]))).await;
-    let tx = TendermintTx::SlashEvidence((&signed_1, Some(signed_2)).encode());
+    let tx = TendermintTx::SlashEvidence(Evidence::ConflictingMessages(
+      signed_1.encode(),
+      signed_2.encode(),
+    ));
     verify_tendermint_tx::<N>(&tx, validators.clone(), commit).unwrap_err();
 
     // Proposals for different block numbers should also fail as evidence
     let signed_2 = signed_for_b_r(1, 0, Data::Proposal(None, TendermintBlock(vec![0x22]))).await;
-    let tx = TendermintTx::SlashEvidence((&signed_1, Some(signed_2)).encode());
+    let tx = TendermintTx::SlashEvidence(Evidence::ConflictingMessages(
+      signed_1.encode(),
+      signed_2.encode(),
+    ));
     verify_tendermint_tx::<N>(&tx, validators.clone(), commit).unwrap_err();
   }
 
@@ -193,23 +231,35 @@ async fn conflicting_msgs_evidence_tx() {
   {
     // non-conflicting data should fail
     let signed_1 = signed_for_b_r(0, 0, Data::Prevote(Some([0x11; 32]))).await;
-    let tx = TendermintTx::SlashEvidence((&signed_1, Some(&signed_1)).encode());
+    let tx = TendermintTx::SlashEvidence(Evidence::ConflictingMessages(
+      signed_1.encode(),
+      signed_1.encode(),
+    ));
     assert!(verify_tendermint_tx::<N>(&tx, validators.clone(), commit).is_err());
 
     // conflicting data should pass
     let signed_2 = signed_for_b_r(0, 0, Data::Prevote(Some([0x22; 32]))).await;
-    let tx = TendermintTx::SlashEvidence((&signed_1, Some(signed_2)).encode());
+    let tx = TendermintTx::SlashEvidence(Evidence::ConflictingMessages(
+      signed_1.encode(),
+      signed_2.encode(),
+    ));
     verify_tendermint_tx::<N>(&tx, validators.clone(), commit).unwrap();
 
     // Except if it has a distinct round number, as we don't check cross-round conflicts
     // (except for Precommit)
     let signed_2 = signed_for_b_r(0, 1, Data::Prevote(Some([0x22; 32]))).await;
-    let tx = TendermintTx::SlashEvidence((&signed_1, Some(signed_2)).encode());
+    let tx = TendermintTx::SlashEvidence(Evidence::ConflictingMessages(
+      signed_1.encode(),
+      signed_2.encode(),
+    ));
     verify_tendermint_tx::<N>(&tx, validators.clone(), commit).unwrap_err();
 
     // Proposals for different block numbers should also fail as evidence
     let signed_2 = signed_for_b_r(1, 0, Data::Prevote(Some([0x22; 32]))).await;
-    let tx = TendermintTx::SlashEvidence((&signed_1, Some(signed_2)).encode());
+    let tx = TendermintTx::SlashEvidence(Evidence::ConflictingMessages(
+      signed_1.encode(),
+      signed_2.encode(),
+    ));
     verify_tendermint_tx::<N>(&tx, validators.clone(), commit).unwrap_err();
   }
 
@@ -218,17 +268,26 @@ async fn conflicting_msgs_evidence_tx() {
     let sig = signer.sign(&[]).await; // the inner signature doesn't matter
 
     let signed_1 = signed_for_b_r(0, 0, Data::Precommit(Some(([0x11; 32], sig)))).await;
-    let tx = TendermintTx::SlashEvidence((&signed_1, Some(&signed_1)).encode());
+    let tx = TendermintTx::SlashEvidence(Evidence::ConflictingPrecommit(
+      signed_1.encode(),
+      signed_1.encode(),
+    ));
     assert!(verify_tendermint_tx::<N>(&tx, validators.clone(), commit).is_err());
 
     // For precommit, the round number is ignored
     let signed_2 = signed_for_b_r(0, 1, Data::Precommit(Some(([0x22; 32], sig)))).await;
-    let tx = TendermintTx::SlashEvidence((&signed_1, Some(signed_2)).encode());
+    let tx = TendermintTx::SlashEvidence(Evidence::ConflictingPrecommit(
+      signed_1.encode(),
+      signed_2.encode(),
+    ));
     verify_tendermint_tx::<N>(&tx, validators.clone(), commit).unwrap();
 
     // Yet the block number isn't
     let signed_2 = signed_for_b_r(1, 0, Data::Precommit(Some(([0x22; 32], sig)))).await;
-    let tx = TendermintTx::SlashEvidence((&signed_1, Some(signed_2)).encode());
+    let tx = TendermintTx::SlashEvidence(Evidence::ConflictingPrecommit(
+      signed_1.encode(),
+      signed_2.encode(),
+    ));
     assert!(verify_tendermint_tx::<N>(&tx, validators.clone(), commit).is_err());
   }
 
@@ -248,7 +307,10 @@ async fn conflicting_msgs_evidence_tx() {
     )
     .await;
 
-    let tx = TendermintTx::SlashEvidence((signed_1, Some(signed_2)).encode());
+    let tx = TendermintTx::SlashEvidence(Evidence::ConflictingMessages(
+      signed_1.encode(),
+      signed_2.encode(),
+    ));
 
     // update schema so that we don't fail due to invalid signature
     let signer_pub =
@@ -265,7 +327,10 @@ async fn conflicting_msgs_evidence_tx() {
   {
     let signed_1 = signed_for_b_r(0, 0, Data::Proposal(None, TendermintBlock(vec![]))).await;
     let signed_2 = signed_for_b_r(0, 0, Data::Prevote(None)).await;
-    let tx = TendermintTx::SlashEvidence((signed_1, Some(signed_2)).encode());
+    let tx = TendermintTx::SlashEvidence(Evidence::ConflictingMessages(
+      signed_1.encode(),
+      signed_2.encode(),
+    ));
     assert!(verify_tendermint_tx::<N>(&tx, validators.clone(), commit).is_err());
   }
 }

coordinator/tributary/tendermint/src/ext.rs

@@ -282,7 +282,7 @@ pub trait Network: Sized + Send + Sync {
   /// Trigger a slash for the validator in question who was definitively malicious.
   ///
   /// The exact process of triggering a slash is undefined and left to the network as a whole.
-  async fn slash(&mut self, validator: Self::ValidatorId, slash_event: SlashEvent<Self>);
+  async fn slash(&mut self, validator: Self::ValidatorId, slash_event: SlashEvent);
 
   /// Validate a block.
   async fn validate(&mut self, block: &Self::Block) -> Result<(), BlockError>;

coordinator/tributary/tendermint/src/lib.rs

@@ -104,7 +104,7 @@ impl<V: ValidatorId, B: Block, S: Signature> SignedMessage<V, B, S> {
 
 #[derive(Clone, PartialEq, Eq, Debug)]
 enum TendermintError<N: Network> {
-  Malicious(N::ValidatorId, Option<SignedMessageFor<N>>),
+  Malicious(N::ValidatorId, Option<Evidence>),
   Temporal,
   AlreadyHandled,
 }
@@ -131,13 +131,18 @@ pub enum SlashReason {
   InvalidMessage,
 }
 
-// TODO: Move WithEvidence to a proper Evidence enum, denoting the explicit reason its faulty
-// This greatly simplifies the checking process and prevents new-reasons added here not being
-// handled elsewhere
+#[derive(Clone, PartialEq, Eq, Debug, Encode, Decode)]
+pub enum Evidence {
+  ConflictingMessages(Vec<u8>, Vec<u8>),
+  ConflictingPrecommit(Vec<u8>, Vec<u8>),
+  InvalidPrecommit(Vec<u8>),
+  InvalidValidRound(Vec<u8>),
+}
+
 #[derive(Clone, PartialEq, Eq, Debug)]
-pub enum SlashEvent<N: Network> {
+pub enum SlashEvent {
   Id(SlashReason, u64, u32),
-  WithEvidence(SignedMessageFor<N>, Option<SignedMessageFor<N>>),
+  WithEvidence(Evidence),
 }
 
 /// A machine executing the Tendermint protocol.
@@ -266,7 +271,7 @@ impl<N: Network + 'static> TendermintMachine<N> {
     self.reset(round, proposal).await;
   }
 
-  async fn slash(&mut self, validator: N::ValidatorId, slash_event: SlashEvent<N>) {
+  async fn slash(&mut self, validator: N::ValidatorId, slash_event: SlashEvent) {
     // TODO: If the new slash event has evidence, emit to prevent a low-importance slash from
     // cancelling emission of high-importance slashes
     if !self.block.slashes.contains(&validator) {
@@ -500,12 +505,16 @@ impl<N: Network + 'static> TendermintMachine<N> {
             log::trace!("added block {} (produced by machine)", hex::encode(id.as_ref()));
             self.reset(msg.round, proposal).await;
           }
-          Err(TendermintError::Malicious(sender, evidence_msg)) => {
+          Err(TendermintError::Malicious(sender, evidence)) => {
             let current_msg = SignedMessage { msg: msg.clone(), sig: sig.clone() };
 
-            let slash = if let Some(old_msg) = evidence_msg {
+            let slash = if let Some(ev) = evidence {
               // if the malicious message contains a block, only vote to slash
               // TODO: Should this decision be made at a higher level?
+              // A higher-level system may be able to verify if the contained block is fatally
+              // invalid
+              // A higher-level system may accept the bandwidth size of this, even if the issue is
+              // just the valid round field
               if let Data::Proposal(_, _) = &current_msg.msg.data {
                 SlashEvent::Id(
                   SlashReason::InvalidBlock,
@@ -513,11 +522,8 @@ impl<N: Network + 'static> TendermintMachine<N> {
                   self.block.round().number.0,
                 )
               } else {
-                // if old msg and new msg is not the same, use both as evidence.
-                SlashEvent::WithEvidence(
-                  old_msg.clone(),
-                  if old_msg != current_msg { Some(current_msg.clone()) } else { None },
-                )
+                // slash with evidence otherwise
+                SlashEvent::WithEvidence(ev)
               }
             } else {
               // we don't have evidence. Slash with vote.
@@ -563,7 +569,10 @@ impl<N: Network + 'static> TendermintMachine<N> {
         if !self.validators.verify(msg.sender, &commit_msg(end_time.canonical(), id.as_ref()), sig)
         {
           log::warn!(target: "tendermint", "Validator produced an invalid commit signature");
-          Err(TendermintError::Malicious(msg.sender, Some(signed.clone())))?;
+          Err(TendermintError::Malicious(
+            msg.sender,
+            Some(Evidence::InvalidPrecommit(signed.encode())),
+          ))?;
         }
         return Ok(true);
       }
@@ -676,7 +685,12 @@ impl<N: Network + 'static> TendermintMachine<N> {
                 .unwrap();
 
               // Slash the validator for publishing an invalid commit signature
-              self.slash(*validator, SlashEvent::WithEvidence(msg, None)).await;
+              self
+                .slash(
+                  *validator,
+                  SlashEvent::WithEvidence(Evidence::InvalidPrecommit(msg.encode())),
+                )
+                .await;
             }
           }
         }
@@ -760,7 +774,10 @@ impl<N: Network + 'static> TendermintMachine<N> {
         // Malformed message
         if vr.0 >= self.block.round().number.0 {
           log::warn!(target: "tendermint", "Validator claimed a round from the future was valid");
-          Err(TendermintError::Malicious(msg.sender, Some(signed.clone())))?;
+          Err(TendermintError::Malicious(
+            msg.sender,
+            Some(Evidence::InvalidValidRound(signed.encode())),
+          ))?;
         }
 
         if self.block.log.has_consensus(*vr, Data::Prevote(Some(block.id()))) {

coordinator/tributary/tendermint/src/message_log.rs

@@ -1,8 +1,9 @@
 use std::{sync::Arc, collections::HashMap};
 
 use log::debug;
+use parity_scale_codec::Encode;
 
-use crate::{ext::*, RoundNumber, Step, Data, DataFor, TendermintError, SignedMessageFor};
+use crate::{ext::*, RoundNumber, Step, Data, DataFor, TendermintError, SignedMessageFor, Evidence};
 
 type RoundLog<N> = HashMap<<N as Network>::ValidatorId, HashMap<Step, SignedMessageFor<N>>>;
 pub(crate) struct MessageLog<N: Network> {
@@ -33,7 +34,10 @@ impl<N: Network> MessageLog<N> {
           target: "tendermint",
           "Validator sent multiple messages for the same block + round + step"
         );
-        Err(TendermintError::Malicious(msg.sender, Some(existing.clone())))?;
+        Err(TendermintError::Malicious(
+          msg.sender,
+          Some(Evidence::ConflictingMessages(existing.encode(), signed.encode())),
+        ))?;
       }
       return Ok(false);
     }
@@ -44,7 +48,10 @@ impl<N: Network> MessageLog<N> {
         if let Data::Precommit(Some((prev_hash, _))) = prev.msg.data {
           if hash != prev_hash {
             debug!(target: "tendermint", "Validator precommitted to multiple blocks");
-            Err(TendermintError::Malicious(msg.sender, Some(prev.clone())))?;
+            Err(TendermintError::Malicious(
+              msg.sender,
+              Some(Evidence::ConflictingPrecommit(prev.encode(), signed.encode())),
+            ))?;
           }
         } else {
           panic!("message in precommitted wasn't Precommit");

coordinator/tributary/tendermint/tests/ext.rs

@@ -137,7 +137,7 @@ impl Network for TestNetwork {
     }
   }
 
-  async fn slash(&mut self, _: TestValidatorId, _: SlashEvent<Self>) {
+  async fn slash(&mut self, _: TestValidatorId, _: SlashEvent) {
     dbg!("Slash");
     todo!()
   }