From 997dd611d5fa24530df4a0998f2c07b96574c26b Mon Sep 17 00:00:00 2001
From: Luke Parker <lukeparker5132@gmail.com>
Date: Wed, 12 Apr 2023 16:18:42 -0400
Subject: [PATCH] Don't add blocks which aren't valid

Previously, Tendermint needed to be live more than it needed to be correct.
Under the original intention for it, correctness would fail if any coin
desynced, which would cause the node to fail entirely. By accepting a
supermajority's view of state, despite its own, a single coin's failure would
only lead to inability to participate with that single coin.

Now that Tendermint is solely for Tributary, nodes should halt a coin-specific
chain if their view of the chain differs. They are unable to meaningless
participate regardless.

This also means a supermajority of validators can no longer fake messages from
other validators, allowing the Tributary chain to use uniform weights with much
less impact. There is still enough impact they can't be used (ability to cause
a fork), yet they should allow uniform block production (as that's solely a DoS
concern).

While we prior could've simply additionally checked signatures, add_block's
lack of a failure case would've meant it had to panic. This would've been a DoS
possible a minority-weight *which affected the entire coordinator* and
therefore *the entire validator for all coins*.
---
 coordinator/tributary/src/blockchain.rs       | 23 +++++++++++++------
 coordinator/tributary/src/tests/blockchain.rs | 10 ++++----
 coordinator/tributary/src/transaction.rs      |  2 --
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/coordinator/tributary/src/blockchain.rs b/coordinator/tributary/src/blockchain.rs
index 86b7f37b..f73c874b 100644
--- a/coordinator/tributary/src/blockchain.rs
+++ b/coordinator/tributary/src/blockchain.rs
@@ -54,16 +54,23 @@ impl<T: Transaction> Blockchain<T> {
     block.verify(self.genesis, self.tip, locally_provided, self.next_nonces.clone())
   }
 
-  /// Add a block, assuming it's valid.
-  ///
-  /// Do not call this without either verifying the block or having it confirmed under consensus.
-  /// Doing so will cause a panic or action an invalid transaction.
-  pub fn add_block(&mut self, block: &Block<T>) {
+  /// Add a block.
+  #[must_use]
+  pub fn add_block(&mut self, block: &Block<T>) -> bool {
+    // TODO: Handle desyncs re: provided transactions properly
+    if self.verify_block(block).is_err() {
+      return false;
+    }
+
+    // None of the following assertions should be reachable since we verified the block
     self.tip = block.hash();
     for tx in &block.transactions {
       match tx.kind() {
         TransactionKind::Provided => {
-          self.provided.withdraw(tx.hash());
+          assert!(
+            self.provided.withdraw(tx.hash()),
+            "verified block had a provided transaction we didn't have"
+          );
         }
         TransactionKind::Unsigned => {}
         TransactionKind::Signed(Signed { signer, nonce, .. }) => {
@@ -72,10 +79,12 @@ impl<T: Transaction> Blockchain<T> {
             .insert(*signer, nonce + 1)
             .expect("block had signed transaction from non-participant");
           if prev != *nonce {
-            panic!("block had an invalid nonce");
+            panic!("verified block had an invalid nonce");
           }
         }
       }
     }
+
+    true
   }
 }
diff --git a/coordinator/tributary/src/tests/blockchain.rs b/coordinator/tributary/src/tests/blockchain.rs
index 4ccd793a..c4c49f43 100644
--- a/coordinator/tributary/src/tests/blockchain.rs
+++ b/coordinator/tributary/src/tests/blockchain.rs
@@ -35,7 +35,7 @@ fn block_addition() {
   assert_eq!(block.header.parent, genesis);
   assert_eq!(block.header.transactions, [0; 32]);
   blockchain.verify_block(&block).unwrap();
-  blockchain.add_block(&block);
+  assert!(blockchain.add_block(&block));
   assert_eq!(blockchain.tip(), block.hash());
 }
 
@@ -155,7 +155,7 @@ fn signed_transaction() {
 
     // Verify and add the block
     blockchain.verify_block(&block).unwrap();
-    blockchain.add_block(&block);
+    assert!(blockchain.add_block(&block));
     assert_eq!(blockchain.tip(), block.hash());
   };
 
@@ -194,11 +194,11 @@ fn provided_transaction() {
   blockchain.verify_block(&block).unwrap();
 
   // add_block should work for verified blocks
-  blockchain.add_block(&block);
+  assert!(blockchain.add_block(&block));
 
   let block = Block::new(blockchain.tip(), &txs, HashMap::new());
   // The provided transaction should no longer considered provided, causing this error
   assert!(blockchain.verify_block(&block).is_err());
-  // add_block should also work for unverified provided transactions if told to add them
-  blockchain.add_block(&block);
+  // add_block should fail for unverified provided transactions if told to add them
+  assert!(!blockchain.add_block(&block));
 }
diff --git a/coordinator/tributary/src/transaction.rs b/coordinator/tributary/src/transaction.rs
index 290038d0..d4e9aa0e 100644
--- a/coordinator/tributary/src/transaction.rs
+++ b/coordinator/tributary/src/transaction.rs
@@ -58,8 +58,6 @@ impl ReadWrite for Signed {
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub enum TransactionKind<'a> {
   /// This tranaction should be provided by every validator, solely ordered by the block producer.
-  ///
-  /// This transaction is only valid if a supermajority of validators provided it.
   Provided,
 
   /// An unsigned transaction, only able to be included by the block producer.