fix(validations): fix signing and validation of VTTs

aesedepece · aesedepece · commit cdbd32e94bd8 · 2024-10-16T14:02:35.000+02:00
diff --git a/data_structures/src/lib.rs b/data_structures/src/lib.rs
@@ -200,8 +200,8 @@ mod tests {
         // If this default changes before the transition to V2 is complete, almost everything will
         // break because data structures change schema and, serialization changes and hash
         // derivation breaks too
-        let version = get_protocol_version(None);
-        assert_eq!(version, ProtocolVersion::V1_7);
+        let protocol_version = get_protocol_version(None);
+        assert_eq!(protocol_version, ProtocolVersion::V1_7);
 
         // Register the different protocol versions
         register_protocol_version(ProtocolVersion::V1_7, 100, 45);
diff --git a/data_structures/src/proto/versioning.rs b/data_structures/src/proto/versioning.rs
@@ -8,10 +8,8 @@ use protobuf::Message as _;
 use serde::{Deserialize, Serialize};
 use strum_macros::{Display, EnumString};
 
-use crate::chain::Epoch;
-use crate::proto::schema::witnet::SuperBlock;
 use crate::{
-    chain::Hash,
+    chain::{Epoch, Hash},
     get_protocol_version,
     proto::{
         schema::witnet::{
@@ -23,7 +21,6 @@ use crate::{
         },
         ProtobufConvert,
     },
-    transaction::MemoizedHashable,
     types::Message,
 };
 
@@ -230,14 +227,7 @@ impl Versioned for crate::chain::BlockHeader {
 }
 
 impl Versioned for crate::chain::SuperBlock {
-    type LegacyType = SuperBlock;
-
-    fn to_versioned_pb_bytes(&self, _version: ProtocolVersion) -> Result<Vec<u8>, Error>
-    where
-        <Self as ProtobufConvert>::ProtoStruct: protobuf::Message,
-    {
-        Ok(self.hashable_bytes())
-    }
+    type LegacyType = <Self as ProtobufConvert>::ProtoStruct;
 }
 
 impl Versioned for crate::chain::Block {
@@ -274,10 +264,38 @@ impl Versioned for Message {
     }
 }
 
+impl Versioned for crate::transaction::Transaction {
+    type LegacyType = <Self as ProtobufConvert>::ProtoStruct;
+}
+
+impl Versioned for crate::transaction::VTTransaction {
+    type LegacyType = <Self as ProtobufConvert>::ProtoStruct;
+}
+impl Versioned for crate::transaction::VTTransactionBody {
+    type LegacyType = <Self as ProtobufConvert>::ProtoStruct;
+}
+impl Versioned for crate::transaction::DRTransactionBody {
+    type LegacyType = <Self as ProtobufConvert>::ProtoStruct;
+}
+impl Versioned for crate::transaction::CommitTransactionBody {
+    type LegacyType = <Self as ProtobufConvert>::ProtoStruct;
+}
+impl Versioned for crate::transaction::RevealTransaction {
+    type LegacyType = <Self as ProtobufConvert>::ProtoStruct;
+}
+impl Versioned for crate::transaction::RevealTransactionBody {
+    type LegacyType = <Self as ProtobufConvert>::ProtoStruct;
+}
+
 pub trait AutoVersioned: ProtobufConvert {}
 
 impl AutoVersioned for crate::chain::BlockHeader {}
 impl AutoVersioned for crate::chain::SuperBlock {}
+impl AutoVersioned for crate::transaction::VTTransactionBody {}
+impl AutoVersioned for crate::transaction::DRTransactionBody {}
+impl AutoVersioned for crate::transaction::CommitTransactionBody {}
+impl AutoVersioned for crate::transaction::RevealTransaction {}
+impl AutoVersioned for crate::transaction::RevealTransactionBody {}
 
 pub trait VersionedHashable {
     fn versioned_hash(&self, version: ProtocolVersion) -> Hash;
@@ -288,14 +306,34 @@ where
     T: AutoVersioned + Versioned,
     <Self as ProtobufConvert>::ProtoStruct: protobuf::Message,
 {
+    #[inline]
     fn versioned_hash(&self, version: ProtocolVersion) -> Hash {
         // This unwrap is kept in here just because we want `VersionedHashable` to have the same interface as
         // `Hashable`.
         witnet_crypto::hash::calculate_sha256(&self.to_versioned_pb_bytes(version).unwrap()).into()
     }
 }
 
+impl VersionedHashable for crate::transaction::Transaction {
+    fn versioned_hash(&self, version: ProtocolVersion) -> Hash {
+        match self {
+            crate::transaction::Transaction::ValueTransfer(tx) => tx.versioned_hash(version),
+            _ => {
+                todo!();
+            }
+        }
+    }
+}
+
+impl VersionedHashable for crate::transaction::VTTransaction {
+    #[inline]
+    fn versioned_hash(&self, version: ProtocolVersion) -> Hash {
+        self.body.versioned_hash(version)
+    }
+}
+
 impl VersionedHashable for crate::chain::Block {
+    #[inline]
     fn versioned_hash(&self, version: ProtocolVersion) -> Hash {
         self.block_header.versioned_hash(version)
     }
diff --git a/data_structures/src/staking/stakes.rs b/data_structures/src/staking/stakes.rs
@@ -900,6 +900,7 @@ mod tests {
         );
         assert_eq!(stakes.query_power(bob, Capability::Mining, 101), Ok(1_620));
         assert_eq!(stakes.query_power(charlie, Capability::Mining, 101), Ok(0));
+        assert_eq!(stakes.query_power(charlie, Capability::Mining, 102), Ok(0));
         assert_eq!(
             stakes.query_power(alice, Capability::Witnessing, 101),
             Ok(1_010)
diff --git a/node/src/actors/chain_manager/mining.rs b/node/src/actors/chain_manager/mining.rs
@@ -114,12 +114,13 @@ impl ChainManager {
 
         let mut beacon = chain_info.highest_block_checkpoint;
         let mut vrf_input = chain_info.highest_vrf_output;
+        let protocol_version = get_protocol_version(self.current_epoch);
 
         // The highest checkpoint beacon should contain the current epoch
         beacon.checkpoint = current_epoch;
         vrf_input.checkpoint = current_epoch;
 
-        let target_hash = if get_protocol_version(self.current_epoch) == V2_0 {
+        let target_hash = if protocol_version == V2_0 {
             let eligibility = self
                 .chain_state
                 .stakes
@@ -235,8 +236,7 @@ impl ChainManager {
                 );
 
                 // Sign the block hash
-                let protocol = get_protocol_version(Some(block_header.beacon.checkpoint));
-                let block_header_data = block_header.versioned_hash(protocol).data();
+                let block_header_data = block_header.versioned_hash(protocol_version).data();
 
                 signature_mngr::sign_data(block_header_data)
                     .map(|res| {
@@ -253,11 +253,11 @@ impl ChainManager {
                     beacon,
                     epoch_constants,
                 )
-                .map_ok(|_diff, act, _ctx| {
+                .map_ok(move |_diff, act, _ctx| {
                     // Send AddCandidates message to self
                     // This will run all the validations again
 
-                    let block_hash = block.hash();
+                    let block_hash = block.versioned_hash(protocol_version);
                     // FIXME(#1773): Currently last_block_proposed is not used, but removing it is a breaking change
                     act.chain_state.node_stats.last_block_proposed = block_hash;
                     act.chain_state.node_stats.block_proposed_count += 1;
diff --git a/node/src/actors/chain_manager/mod.rs b/node/src/actors/chain_manager/mod.rs
@@ -82,7 +82,7 @@ use witnet_data_structures::{
     },
     utxo_pool::{Diff, OwnUnspentOutputsPool, UnspentOutputsPool, UtxoDiff, UtxoWriteBatch},
     vrf::VrfCtx,
-    wit::{Wit, WIT_DECIMAL_PLACES},
+    wit::Wit,
 };
 use witnet_rad::{error::RadError::TooManyWitnesses, types::RadonTypes};
 use witnet_util::timestamp::seconds_to_human_string;
@@ -717,8 +717,8 @@ impl ChainManager {
                 resynchronizing,
                 &active_wips,
                 Some(&mut transaction_visitor),
-                protocol_version,
                 &self.chain_state.stakes,
+                protocol_version,
             )?;
 
             // Extract the collected priorities from the internal state of the visitor
@@ -884,8 +884,8 @@ impl ChainManager {
                     false,
                     &active_wips,
                     Some(&mut transaction_visitor),
-                    protocol_version,
                     &self.chain_state.stakes,
+                    protocol_version,
                 ) {
                     Ok(utxo_diff) => {
                         let priorities = transaction_visitor.take_state();
@@ -1552,6 +1552,7 @@ impl ChainManager {
                 // than or equal to the current epoch
                 block_epoch: current_epoch,
             };
+            let protocol_version = get_protocol_version(Some(current_epoch));
             let collateral_age = if active_wips.wip0027() {
                 PSEUDO_CONSENSUS_CONSTANTS_WIP0027_COLLATERAL_AGE
             } else {
@@ -1580,6 +1581,7 @@ impl ChainManager {
                 &active_wips,
                 chain_info.consensus_constants.superblock_period,
                 &self.chain_state.stakes,
+                protocol_version,
             ))
             .into_actor(self)
             .and_then(|fee, act, _ctx| {
@@ -2105,8 +2107,8 @@ impl ChainManager {
             self.chain_state.reputation_engine.as_ref().unwrap(),
             &consensus_constants,
             &active_wips,
-            protocol_version,
             &self.chain_state.stakes,
+            protocol_version,
         );
 
         let fut = async {
@@ -2131,6 +2133,7 @@ impl ChainManager {
                 &active_wips,
                 None,
                 &act.chain_state.stakes,
+                protocol_version,
             );
             async {
                 // Short-circuit if validation failed
@@ -2906,8 +2909,8 @@ pub fn process_validations(
     resynchronizing: bool,
     active_wips: &ActiveWips,
     transaction_visitor: Option<&mut dyn Visitor<Visitable = (Transaction, u64, u32)>>,
-    protocol_version: ProtocolVersion,
     stakes: &StakesTracker,
+    protocol_version: ProtocolVersion,
 ) -> Result<Diff, failure::Error> {
     if !resynchronizing {
         let mut signatures_to_verify = vec![];
@@ -2920,8 +2923,8 @@ pub fn process_validations(
             rep_eng,
             consensus_constants,
             active_wips,
-            protocol_version,
             stakes,
+            protocol_version,
         )?;
         log::debug!("Verifying {} block signatures", signatures_to_verify.len());
         verify_signatures(signatures_to_verify, vrf_ctx)?;
@@ -2941,6 +2944,7 @@ pub fn process_validations(
         active_wips,
         transaction_visitor,
         stakes,
+        protocol_version,
     )?;
 
     if !resynchronizing {
@@ -3545,6 +3549,7 @@ mod tests {
             OutputPointer, PartialConsensusConstants, PublicKey, SecretKey, Signature,
             ValueTransferOutput,
         },
+        proto::versioning::VersionedHashable,
         transaction::{
             CommitTransaction, DRTransaction, MintTransaction, RevealTransaction, VTTransaction,
             VTTransactionBody,
@@ -3983,8 +3988,12 @@ mod tests {
     static PRIV_KEY_1: [u8; 32] = [0xcd; 32];
     static PRIV_KEY_2: [u8; 32] = [0x43; 32];
 
-    fn sign_tx<H: Hashable>(mk: [u8; 32], tx: &H) -> KeyedSignature {
-        let Hash::SHA256(data) = tx.hash();
+    fn sign_tx<H: VersionedHashable>(
+        mk: [u8; 32],
+        tx: &H,
+        protocol_version: ProtocolVersion,
+    ) -> KeyedSignature {
+        let Hash::SHA256(data) = tx.versioned_hash(protocol_version);
 
         let secret_key =
             Secp256k1_SecretKey::from_slice(&mk).expect("32 bytes, within curve order");
@@ -4011,6 +4020,7 @@ mod tests {
     fn create_valid_block(chain_manager: &mut ChainManager, priv_key: &[u8; 32]) -> Block {
         let vrf = &mut VrfCtx::secp256k1().unwrap();
         let current_epoch = chain_manager.current_epoch.unwrap();
+        let protocol_version = get_protocol_version(Some(current_epoch));
 
         let consensus_constants = chain_manager.consensus_constants();
         let secret_key = SecretKey {
@@ -4064,7 +4074,7 @@ mod tests {
             proof: BlockEligibilityClaim::create(vrf, &secret_key, vrf_input).unwrap(),
             ..Default::default()
         };
-        let block_sig = sign_tx(*priv_key, &block_header);
+        let block_sig = sign_tx(*priv_key, &block_header, protocol_version);
 
         Block::new(block_header, block_sig, txns)
     }
@@ -4178,6 +4188,7 @@ mod tests {
     fn create_valid_transaction(
         _chain_manager: &mut ChainManager,
         priv_key: &[u8; 32],
+        protocol_version: ProtocolVersion,
     ) -> Transaction {
         let my_pkh = pkh(priv_key);
 
@@ -4197,7 +4208,7 @@ mod tests {
         let outputs = vec![vto];
 
         let vt_body = VTTransactionBody::new(inputs, outputs);
-        let signatures = vec![sign_tx(*priv_key, &vt_body)];
+        let signatures = vec![sign_tx(*priv_key, &vt_body, protocol_version)];
         let vtt = VTTransaction::new(vt_body, signatures);
 
         Transaction::ValueTransfer(vtt)
@@ -4211,14 +4222,16 @@ mod tests {
             let mut config = Config::default();
             config.storage.backend = StorageBackend::HashMap;
             let config = Arc::new(config);
+            let epoch = Some(2000000);
+            let protocol_version = get_protocol_version(epoch);
             // Start relevant actors
             config_mngr::start(config);
             storage_mngr::start();
 
             let mut ctx = Context::new();
             let mut chain_manager = ChainManager::default();
 
-            chain_manager.current_epoch = Some(2000000);
+            chain_manager.current_epoch = epoch;
             // 1 epoch = 1000 seconds, for easy testing
             chain_manager.epoch_constants = Some(EpochConstants {
                 checkpoint_zero_timestamp: 0,
@@ -4259,7 +4272,7 @@ mod tests {
             chain_manager.vrf_ctx = Some(VrfCtx::secp256k1().unwrap());
             chain_manager.sm_state = StateMachine::Synced;
 
-            let t1 = create_valid_transaction(&mut chain_manager, &PRIV_KEY_1);
+            let t1 = create_valid_transaction(&mut chain_manager, &PRIV_KEY_1, protocol_version);
             let mut t1_mal = t1.clone();
             // Malleability!
             match &mut t1_mal {
@@ -4280,7 +4293,10 @@ mod tests {
             }
 
             // Changing signatures field does not change transaction hash
-            assert_eq!(t1.hash(), t1_mal.hash());
+            assert_eq!(
+                t1.versioned_hash(protocol_version),
+                t1_mal.versioned_hash(protocol_version)
+            );
             // But the transactions are different
             assert_ne!(t1, t1_mal);
 
diff --git a/validations/src/tests/mod.rs b/validations/src/tests/mod.rs
diff --git a/validations/src/validations.rs b/validations/src/validations.rs
