Merge pull request #6284 from obycode/feat/signature-management

Do not count accept and reject from same signer

Author: obycode

stacks-signer/CHANGELOG.md

@@ -13,6 +13,10 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
 - Introduced `capitulate_miner_view_timeout_secs`: the duration (in seconds) for the signer to wait between updating the local state machine viewpoint and capitulating to other signers' miner views.
 - Added codepath to enable signers to evaluate block proposals and miner activity against global signer state for improved consistency and correctness. Currently feature gated behind the `SUPPORTED_SIGNER_PROTOCOL_VERSION`
 
+### Changed
+
+- Do not count both a block acceptance and a block rejection for the same signer/block. Also ignore repeated responses (mainly for logging purposes).
+
 ## [3.1.0.0.13.0]
 
 ### Changed

stacks-signer/src/signerdb.rs

@@ -40,7 +40,7 @@ use stacks_common::types::chainstate::ConsensusHash;
 use stacks_common::util::get_epoch_time_secs;
 use stacks_common::util::hash::Sha512Trunc256Sum;
 use stacks_common::util::secp256k1::MessageSignature;
-use stacks_common::{debug, define_u8_enum, error};
+use stacks_common::{debug, define_u8_enum, error, warn};
 
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
 /// A vote across the signer set for a block
@@ -649,6 +649,42 @@ static ADD_SIGNER_STATE_MACHINE_UPDATES_SIGNER_ADDR_REWARD_CYCLE_INDEX: &str = r
 CREATE INDEX idx_signer_addr_reward_cycle ON signer_state_machine_updates(reward_cycle, signer_addr, received_time DESC);
 "#;
 
+static DROP_BLOCK_SIGNATURES_TABLE: &str = r#"
+DROP TABLE IF EXISTS block_signatures;
+"#;
+
+static CREATE_BLOCK_SIGNATURES_TABLE_V16: &str = r#"
+CREATE TABLE IF NOT EXISTS block_signatures (
+    -- The block sighash commits to all of the stacks and burnchain state as of its parent,
+    -- as well as the tenure itself so there's no need to include the reward cycle.  Just
+    -- the sighash is sufficient to uniquely identify the block across all burnchain, PoX,
+    -- and stacks forks.
+    signer_signature_hash TEXT NOT NULL,
+    -- the signer address that signed the block
+    signer_addr TEXT NOT NULL,
+    -- signature itself
+    signature TEXT NOT NULL,
+    PRIMARY KEY (signer_signature_hash, signer_addr)
+) STRICT;"#;
+
+static DROP_BLOCK_REJECTION_SIGNER_ADDRS: &str = r#"
+DROP TABLE IF EXISTS block_rejection_signer_addrs;
+"#;
+
+static CREATE_BLOCK_REJECTION_SIGNER_ADDRS_V16: &str = r#"
+CREATE TABLE IF NOT EXISTS block_rejection_signer_addrs (
+    -- The block sighash commits to all of the stacks and burnchain state as of its parent,
+    -- as well as the tenure itself so there's no need to include the reward cycle.  Just
+    -- the sighash is sufficient to uniquely identify the block across all burnchain, PoX,
+    -- and stacks forks.
+    signer_signature_hash TEXT NOT NULL,
+    -- the signer address that rejected the block
+    signer_addr TEXT NOT NULL,
+    -- the reject reason code
+    reject_code INTEGER NOT NULL,
+    PRIMARY KEY (signer_signature_hash, signer_addr)
+) STRICT;"#;
+
 static SCHEMA_1: &[&str] = &[
     DROP_SCHEMA_0,
     CREATE_DB_CONFIG,
@@ -756,6 +792,10 @@ static SCHEMA_16: &[&str] = &[
     ADD_SIGNER_STATE_MACHINE_UPDATES_BURN_BLOCK_CONSENSUS_HASH_INDEX,
     ADD_SIGNER_STATE_MACHINE_UPDATES_RECEIVED_TIME_INDEX,
     ADD_SIGNER_STATE_MACHINE_UPDATES_SIGNER_ADDR_REWARD_CYCLE_INDEX,
+    DROP_BLOCK_SIGNATURES_TABLE,
+    CREATE_BLOCK_SIGNATURES_TABLE_V16,
+    DROP_BLOCK_REJECTION_SIGNER_ADDRS,
+    CREATE_BLOCK_REJECTION_SIGNER_ADDRS_V16,
     "INSERT INTO db_config (version) VALUES (16);",
 ];
 
@@ -1287,20 +1327,38 @@ impl SignerDb {
     pub fn add_block_signature(
         &self,
         block_sighash: &Sha512Trunc256Sum,
+        signer_addr: &StacksAddress,
         signature: &MessageSignature,
-    ) -> Result<(), DBError> {
-        let qry = "INSERT OR REPLACE INTO block_signatures (signer_signature_hash, signature) VALUES (?1, ?2);";
+    ) -> Result<bool, DBError> {
+        // Remove any block rejection entry for this signer and block hash
+        let del_qry = "DELETE FROM block_rejection_signer_addrs WHERE signer_signature_hash = ?1 AND signer_addr = ?2";
+        let del_args = params![block_sighash, signer_addr.to_string()];
+        self.db.execute(del_qry, del_args)?;
+
+        // Insert the block signature
+        let qry = "INSERT OR IGNORE INTO block_signatures (signer_signature_hash, signer_addr, signature) VALUES (?1, ?2, ?3);";
         let args = params![
             block_sighash,
+            signer_addr.to_string(),
             serde_json::to_string(signature).map_err(DBError::SerializationError)?
         ];
-
-        debug!("Inserting block signature.";
-            "signer_signature_hash" => %block_sighash,
-            "signature" => %signature);
-
-        self.db.execute(qry, args)?;
-        Ok(())
+        let rows_added = self.db.execute(qry, args)?;
+
+        let is_new_signature = rows_added > 0;
+        if is_new_signature {
+            debug!("Added block signature.";
+                "signer_signature_hash" => %block_sighash,
+                "signer_address" => %signer_addr,
+                "signature" => %signature
+            );
+        } else {
+            debug!("Duplicate block signature.";
+                "signer_signature_hash" => %block_sighash,
+                "signer_address" => %signer_addr,
+                "signature" => %signature
+            );
+        }
+        Ok(is_new_signature)
     }
 
     /// Get all signatures for a block
@@ -1323,22 +1381,63 @@ impl SignerDb {
         block_sighash: &Sha512Trunc256Sum,
         addr: &StacksAddress,
         reject_reason: &RejectReason,
-    ) -> Result<(), DBError> {
-        let qry = "INSERT OR REPLACE INTO block_rejection_signer_addrs (signer_signature_hash, signer_addr, reject_code) VALUES (?1, ?2, ?3);";
-        let args = params![
-            block_sighash,
-            addr.to_string(),
-            RejectReasonPrefix::from(reject_reason) as i64
-        ];
-
-        debug!("Inserting block rejection.";
-            "signer_signature_hash" => %block_sighash,
-            "signer_address" => %addr,
-            "reject_reason" => %reject_reason
-        );
+    ) -> Result<bool, DBError> {
+        // If this signer/block already has a signature, do not allow a rejection
+        let sig_qry = "SELECT EXISTS(SELECT 1 FROM block_signatures WHERE signer_signature_hash = ?1 AND signer_addr = ?2)";
+        let sig_args = params![block_sighash, addr.to_string()];
+        let exists = self.db.query_row(sig_qry, sig_args, |row| row.get(0))?;
+        if exists {
+            warn!("Cannot add block rejection because a signature already exists.";
+                "signer_signature_hash" => %block_sighash,
+                "signer_address" => %addr,
+                "reject_reason" => %reject_reason
+            );
+            return Ok(false);
+        }
 
-        self.db.execute(qry, args)?;
-        Ok(())
+        // Check if a row exists for this sighash/signer combo
+        let qry = "SELECT reject_code FROM block_rejection_signer_addrs WHERE signer_signature_hash = ?1 AND signer_addr = ?2 LIMIT 1";
+        let args = params![block_sighash, addr.to_string()];
+        let existing_code: Option<i64> =
+            self.db.query_row(qry, args, |row| row.get(0)).optional()?;
+
+        let reject_code = RejectReasonPrefix::from(reject_reason) as i64;
+
+        match existing_code {
+            Some(code) if code == reject_code => {
+                // Row exists with same reject_reason, do nothing
+                debug!("Duplicate block rejection.";
+                    "signer_signature_hash" => %block_sighash,
+                    "signer_address" => %addr,
+                    "reject_reason" => %reject_reason
+                );
+                Ok(false)
+            }
+            Some(_) => {
+                // Row exists but with different reject_reason, update it
+                let update_qry = "UPDATE block_rejection_signer_addrs SET reject_code = ?1 WHERE signer_signature_hash = ?2 AND signer_addr = ?3";
+                let update_args = params![reject_code, block_sighash, addr.to_string()];
+                self.db.execute(update_qry, update_args)?;
+                debug!("Updated block rejection reason.";
+                    "signer_signature_hash" => %block_sighash,
+                    "signer_address" => %addr,
+                    "reject_reason" => %reject_reason
+                );
+                Ok(true)
+            }
+            None => {
+                // Row does not exist, insert it
+                let insert_qry = "INSERT INTO block_rejection_signer_addrs (signer_signature_hash, signer_addr, reject_code) VALUES (?1, ?2, ?3)";
+                let insert_args = params![block_sighash, addr.to_string(), reject_code];
+                self.db.execute(insert_qry, insert_args)?;
+                debug!("Inserted block rejection.";
+                    "signer_signature_hash" => %block_sighash,
+                    "signer_address" => %addr,
+                    "reject_reason" => %reject_reason
+                );
+                Ok(true)
+            }
+        }
     }
 
     /// Get all signer addresses that rejected the block (and their reject codes)
@@ -2090,19 +2189,175 @@ pub mod tests {
         let db = SignerDb::new(db_path).expect("Failed to create signer db");
 
         let block_id = Sha512Trunc256Sum::from_data("foo".as_bytes());
+        let address1 = StacksAddress::burn_address(false);
+        let address2 = StacksAddress::burn_address(true);
         let sig1 = MessageSignature([0x11; 65]);
         let sig2 = MessageSignature([0x22; 65]);
 
         assert_eq!(db.get_block_signatures(&block_id).unwrap(), vec![]);
 
-        db.add_block_signature(&block_id, &sig1).unwrap();
+        db.add_block_signature(&block_id, &address1, &sig1).unwrap();
         assert_eq!(db.get_block_signatures(&block_id).unwrap(), vec![sig1]);
 
-        db.add_block_signature(&block_id, &sig2).unwrap();
+        db.add_block_signature(&block_id, &address2, &sig2).unwrap();
         assert_eq!(
             db.get_block_signatures(&block_id).unwrap(),
-            vec![sig1, sig2]
+            vec![sig2, sig1]
+        );
+    }
+
+    #[test]
+    fn duplicate_block_signatures() {
+        let db_path = tmp_db_path();
+        let db = SignerDb::new(db_path).expect("Failed to create signer db");
+
+        let block_id = Sha512Trunc256Sum::from_data("foo".as_bytes());
+        let address = StacksAddress::burn_address(false);
+        let sig1 = MessageSignature([0x11; 65]);
+        let sig2 = MessageSignature([0x22; 65]);
+
+        assert_eq!(db.get_block_signatures(&block_id).unwrap(), vec![]);
+
+        assert!(db.add_block_signature(&block_id, &address, &sig1).unwrap());
+        assert_eq!(db.get_block_signatures(&block_id).unwrap(), vec![sig1]);
+
+        assert!(!db.add_block_signature(&block_id, &address, &sig2).unwrap());
+        assert_eq!(db.get_block_signatures(&block_id).unwrap(), vec![sig1]);
+    }
+
+    #[test]
+    fn add_and_get_block_rejections() {
+        let db_path = tmp_db_path();
+        let db = SignerDb::new(db_path).expect("Failed to create signer db");
+
+        let block_id = Sha512Trunc256Sum::from_data("foo".as_bytes());
+        let address1 = StacksAddress::burn_address(false);
+        let address2 = StacksAddress::burn_address(true);
+
+        assert_eq!(
+            db.get_block_rejection_signer_addrs(&block_id).unwrap(),
+            vec![]
         );
+
+        assert!(db
+            .add_block_rejection_signer_addr(
+                &block_id,
+                &address1,
+                &RejectReason::DuplicateBlockFound,
+            )
+            .unwrap());
+        assert_eq!(
+            db.get_block_rejection_signer_addrs(&block_id).unwrap(),
+            vec![(address1, RejectReasonPrefix::DuplicateBlockFound)]
+        );
+
+        assert!(db
+            .add_block_rejection_signer_addr(
+                &block_id,
+                &address2,
+                &RejectReason::InvalidParentBlock
+            )
+            .unwrap());
+        assert_eq!(
+            db.get_block_rejection_signer_addrs(&block_id).unwrap(),
+            vec![
+                (address2, RejectReasonPrefix::InvalidParentBlock),
+                (address1, RejectReasonPrefix::DuplicateBlockFound),
+            ]
+        );
+    }
+
+    #[test]
+    fn duplicate_block_rejections() {
+        let db_path = tmp_db_path();
+        let db = SignerDb::new(db_path).expect("Failed to create signer db");
+
+        let block_id = Sha512Trunc256Sum::from_data("foo".as_bytes());
+        let address = StacksAddress::burn_address(false);
+
+        assert_eq!(
+            db.get_block_rejection_signer_addrs(&block_id).unwrap(),
+            vec![]
+        );
+
+        assert!(db
+            .add_block_rejection_signer_addr(&block_id, &address, &RejectReason::InvalidParentBlock)
+            .unwrap());
+        assert_eq!(
+            db.get_block_rejection_signer_addrs(&block_id).unwrap(),
+            vec![(address, RejectReasonPrefix::InvalidParentBlock)]
+        );
+
+        assert!(db
+            .add_block_rejection_signer_addr(&block_id, &address, &RejectReason::InvalidMiner)
+            .unwrap());
+        assert_eq!(
+            db.get_block_rejection_signer_addrs(&block_id).unwrap(),
+            vec![(address, RejectReasonPrefix::InvalidMiner)]
+        );
+
+        assert!(!db
+            .add_block_rejection_signer_addr(&block_id, &address, &RejectReason::InvalidMiner)
+            .unwrap());
+        assert_eq!(
+            db.get_block_rejection_signer_addrs(&block_id).unwrap(),
+            vec![(address, RejectReasonPrefix::InvalidMiner)]
+        );
+    }
+
+    #[test]
+    fn reject_then_accept() {
+        let db_path = tmp_db_path();
+        let db = SignerDb::new(db_path).expect("Failed to create signer db");
+
+        let block_id = Sha512Trunc256Sum::from_data("foo".as_bytes());
+        let address = StacksAddress::burn_address(false);
+        let sig1 = MessageSignature([0x11; 65]);
+
+        assert_eq!(db.get_block_signatures(&block_id).unwrap(), vec![]);
+
+        assert!(db
+            .add_block_rejection_signer_addr(&block_id, &address, &RejectReason::InvalidParentBlock)
+            .unwrap());
+        assert_eq!(
+            db.get_block_rejection_signer_addrs(&block_id).unwrap(),
+            vec![(address, RejectReasonPrefix::InvalidParentBlock)]
+        );
+
+        assert!(db.add_block_signature(&block_id, &address, &sig1).unwrap());
+        assert_eq!(db.get_block_signatures(&block_id).unwrap(), vec![sig1]);
+        assert!(db
+            .get_block_rejection_signer_addrs(&block_id)
+            .unwrap()
+            .is_empty());
+    }
+
+    #[test]
+    fn accept_then_reject() {
+        let db_path = tmp_db_path();
+        let db = SignerDb::new(db_path).expect("Failed to create signer db");
+
+        let block_id = Sha512Trunc256Sum::from_data("foo".as_bytes());
+        let address = StacksAddress::burn_address(false);
+        let sig1 = MessageSignature([0x11; 65]);
+
+        assert_eq!(db.get_block_signatures(&block_id).unwrap(), vec![]);
+
+        assert!(db.add_block_signature(&block_id, &address, &sig1).unwrap());
+        assert_eq!(db.get_block_signatures(&block_id).unwrap(), vec![sig1]);
+        assert!(db
+            .get_block_rejection_signer_addrs(&block_id)
+            .unwrap()
+            .is_empty());
+
+        assert!(!db
+            .add_block_rejection_signer_addr(&block_id, &address, &RejectReason::InvalidParentBlock)
+            .unwrap());
+        assert_eq!(db.get_block_signatures(&block_id).unwrap(), vec![sig1]);
+        assert!(db
+            .get_block_rejection_signer_addrs(&block_id)
+            .unwrap()
+            .is_empty());
     }
 
     #[test]