test(drive-abci): guard chain lock, masternode list replay and version table invariants

packages/rs-drive-abci/src/execution/engine/consensus_params_update/mod.rs

@@ -600,6 +600,7 @@ mod tests {
 mod evidence_policy_tests {
     use super::*;
     use crate::platform_types::epoch_info::v0::EpochInfoV0;
+    use dpp::version::PLATFORM_VERSIONS;
     use prost::Message;
 
     fn epoch_info(is_epoch_change: bool, current_epoch_index: u16) -> EpochInfo {
@@ -680,6 +681,37 @@ mod evidence_policy_tests {
         }
     }
 
+    /// Tenderdash refuses consensus parameters that break the evidence rules of its
+    /// `ValidateConsensusParams` (`types/params.go`): ages in blocks and in time above zero and
+    /// a byte budget that is not negative. A refused update fails the activation block on every
+    /// node, and again after each restart, so the chain halts. The age in time must also fit the
+    /// Go `time.Duration` Tenderdash decodes it into.
+    #[test]
+    fn should_have_an_evidence_policy_tenderdash_accepts_in_every_platform_version() {
+        assert!(
+            PLATFORM_VERSIONS
+                .iter()
+                .any(|platform_version| platform_version.consensus.evidence.is_some()),
+            "some protocol version sets an evidence policy"
+        );
+        for platform_version in PLATFORM_VERSIONS {
+            let Some(evidence) = platform_version.consensus.evidence else {
+                continue;
+            };
+            let case = format!(
+                "protocol version {}: {evidence:?}",
+                platform_version.protocol_version
+            );
+            assert!(evidence.max_age_num_blocks > 0, "{case}");
+            assert!(evidence.max_age_duration_seconds > 0, "{case}");
+            assert!(
+                evidence.max_age_duration_seconds <= i64::MAX / 1_000_000_000,
+                "{case}: the age in time overflows a Go time.Duration"
+            );
+            assert!(evidence.max_bytes >= 0, "{case}");
+        }
+    }
+
     #[test]
     fn should_not_repeat_evidence_update_after_activation() {
         let latest = PlatformVersion::latest();

packages/rs-drive-abci/src/execution/engine/run_block_proposal/v0/mod.rs

@@ -172,7 +172,7 @@ where
         if let Some(core_chain_lock_update) = core_chain_lock_update.as_ref() {
             if !known_from_us {
                 let verification_result = self.verify_chain_lock(
-                    block_state_info.round, // the round is to allow us to bypass local verification in case of chain stall
+                    block_state_info.round, // only logged: no generation verifies differently in a later round
                     &block_platform_state,
                     core_chain_lock_update,
                     true, // if it's not known from us, then we should try submitting it

packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_voter_identity/mod.rs

@@ -23,8 +23,9 @@ where
     /// Updates the voter identity in the masternode.
     ///
     /// This function updates the voter identity in the masternode. The old identity key is
-    /// disabled (which might make the identity unusable) and a new identity is added with
-    /// the new key, which is a non-unique key.
+    /// disabled (which might make the identity unusable), and either a new identity is added
+    /// with the new key, which is a non-unique key, or, when the voter identity of the new
+    /// voting address already exists, a key is re-enabled.
     ///
     /// When the masternode returns to a voting address it used before, version 1 (from
     /// protocol version 15) re-enables the key of that address's voter identity. Version 0

packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_voter_identity/v1/mod.rs

@@ -27,7 +27,8 @@ where
 {
     /// When a voter identity is updated the following events need to happen
     /// The old identity key is disabled (which might make the identity unusable)
-    /// A new identity is added with the new key, this new key is a non unique key.
+    /// Either a new identity is added with the new key, a non unique key, or the key of the
+    /// existing voter identity of the new voting address is re-enabled.
     ///
     /// Unlike version 0, when the voter identity of the new voting address already exists
     /// (the masternode returns to an address it used before), its keys are re-enabled on that
@@ -92,7 +93,7 @@ where
             keys_ids: old_voter_identity_key_ids,
         }));
 
-        // Part 2 : Create or Update Voting identity based on new key
+        // Create the voter identity of the new voting address, or re-enable its key
         let new_voter_identity = Self::create_voter_identity(
             &(pro_tx_hash.to_byte_array()),
             &new_voting_address,
@@ -112,22 +113,21 @@ where
             platform_version,
         )?;
 
-        // two possibilities
         if !new_voter_identity_key_ids.is_empty() {
-            // first is that the new voter key already existed
-            // if it is disabled re-enable it
+            // The masternode used the new voting address before, so its voter identity exists.
+            // Its key is re-enabled whether or not it is disabled.
 
             if new_voter_identity_key_ids.len() > 1 {
                 return Err(Error::Execution(ExecutionError::DriveIncoherence(
-                    "more than one masternode voter identity for an address and pro_tx_hash pair",
+                    "more than one key on the masternode voter identity for an address and pro_tx_hash pair",
                 )));
             }
 
             tracing::trace!(
                 identity_id = ?new_voter_identity.id(),
                 keys_ids = ?new_voter_identity_key_ids,
                 method = "update_voter_identity_v1",
-                "re-enable voter identity keys if they already exists and disabled"
+                "re-enable the keys of the existing voter identity"
             );
 
             drive_operations.push(IdentityOperation(ReEnableIdentityKeys {
@@ -141,7 +141,6 @@ where
                 "create a new voter identity"
             );
 
-            // other is that the
             drive_operations.push(IdentityOperation(AddNewIdentity {
                 identity: new_voter_identity,
                 is_masternode_identity: true,

packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/mod.rs

@@ -94,7 +94,7 @@ mod test {
     use crate::test::helpers::setup::{TempPlatform, TestPlatformBuilder};
     use dpp::bls::SecretKey;
     use dpp::core_types::validator_set::v0::ValidatorSetV0;
-    use dpp::dashcore::hashes::Hash;
+    use dpp::dashcore::hashes::{sha256, Hash};
     use dpp::dashcore::{Network, ProTxHash, PubkeyHash, QuorumHash, Txid};
     use dpp::dashcore_rpc::json::{
         DMNState, DMNStateDiff, MasternodeListDiff, MasternodeListItem, MasternodeType,
@@ -234,6 +234,163 @@ mod test {
             .expect("expected to commit");
     }
 
+    /// What `should_store_the_mainnet_masternode_lists_as_shipped_protocol_versions_do` expects:
+    /// the app hash each protocol version it covers leaves, then the stored masternode list they
+    /// share, as [`mainnet_masternode_list_replay`] describes it.
+    const MAINNET_MASTERNODE_LIST_SNAPSHOT_PATH: &str =
+        "tests/supporting_files/mainnet_protx_list_diffs/stored_by_protocol_versions_13_and_14.txt";
+
+    /// Replays the mainnet full list at init chain and the next diff in the first block through
+    /// the dispatcher at `platform_version`, and returns what the platform then holds: the app
+    /// hash, which covers the masternode identities the updates wrote, and the stored full list,
+    /// one line per masternode with the start of its ProTx hash as Core prints it and the start of
+    /// the SHA-256 of its stored entry. Asserts that the evonode list holds exactly the evonodes
+    /// of the full list, as stored there.
+    fn mainnet_masternode_list_replay(platform_version: &PlatformVersion) -> ([u8; 32], String) {
+        let mut platform = TestPlatformBuilder::new()
+            .with_initial_protocol_version(platform_version.protocol_version)
+            .build_with_mock_rpc()
+            .set_genesis_state();
+        let (init_core_height, next_core_height) = (2128896, 2129440);
+        let mut evonodes = BTreeSet::new();
+        let responses = [
+            mainnet_list_diff("1-2128896.json", false, &mut evonodes),
+            mainnet_list_diff("2128896-2129440.json", false, &mut evonodes),
+        ];
+        platform
+            .core_rpc
+            .expect_get_protx_diff_with_masternodes()
+            .returning(move |_base_block, block| {
+                Ok(responses[usize::from(block != init_core_height)].clone())
+            });
+
+        let transaction = platform.drive.grove.start_transaction();
+        let mut init_chain_state = platform.state.load().as_ref().clone();
+        platform
+            .update_masternode_list(
+                None,
+                &mut init_chain_state,
+                init_core_height,
+                true,
+                &BlockInfo {
+                    height: 1,
+                    core_height: init_core_height,
+                    time_ms: 1,
+                    ..Default::default()
+                },
+                &transaction,
+                platform_version,
+            )
+            .expect("expected to process the full masternode list");
+        let mut block_state = init_chain_state.clone();
+        platform
+            .update_masternode_list(
+                Some(&init_chain_state),
+                &mut block_state,
+                next_core_height,
+                false,
+                &BlockInfo {
+                    height: 2,
+                    core_height: next_core_height,
+                    time_ms: 2,
+                    ..Default::default()
+                },
+                &transaction,
+                platform_version,
+            )
+            .expect("expected to process the masternode list diff");
+        platform
+            .drive
+            .grove
+            .commit_transaction(transaction)
+            .unwrap()
+            .expect("expected to commit");
+
+        let full_list = block_state.full_masternode_list();
+        assert!(
+            block_state.hpmn_masternode_list().iter().eq(full_list
+                .iter()
+                .filter(|(_, masternode)| masternode.node_type == MasternodeType::Evo)),
+            "protocol version {}: the evonode list holds the evonodes of the full list",
+            platform_version.protocol_version
+        );
+
+        let app_hash = platform
+            .drive
+            .grove
+            .root_hash(None, &platform_version.drive.grove_version)
+            .unwrap()
+            .expect("expected the app hash");
+        let stored_list = full_list
+            .iter()
+            .map(|(pro_tx_hash, masternode)| {
+                let entry = serialize_masternode_entry(masternode, platform_version)
+                    .expect("expected to serialize the masternode entry");
+                let entry_hash = sha256::Hash::hash(&entry).to_byte_array();
+                format!(
+                    "{} {}\n",
+                    &pro_tx_hash.to_string()[..16],
+                    hex::encode(&entry_hash[..8])
+                )
+            })
+            .collect();
+        (app_hash, stored_list)
+    }
+
+    /// Every node replays shipped protocol versions with the binary it runs, so what they store
+    /// from Core's diffs must not change with the binary. Core's diffs reach Platform through the
+    /// Core RPC client's decoding and diff application, which a rust-dashcore update can change
+    /// without any Platform file changing. One such update made the client keep revived heights
+    /// and decode operator payouts in diffs, which would have changed 227 entries of these lists
+    /// had `update_state_masternode_list` v0 and v1 not undone it. Replaying the recorded mainnet
+    /// diffs at protocol versions 13 and 14 must give the masternode list, which both store
+    /// alike, and the app hashes the Platform 5.0.3 release gives.
+    ///
+    /// Regenerate the snapshot only for a deliberate change to what those versions store:
+    /// `UPDATE_MAINNET_MASTERNODE_LIST_SNAPSHOT=1 cargo test -p drive-abci --lib
+    /// should_store_the_mainnet_masternode_lists_as_shipped_protocol_versions_do`
+    #[test]
+    fn should_store_the_mainnet_masternode_lists_as_shipped_protocol_versions_do() {
+        let [(app_hash_13, stored_list_13), (app_hash_14, stored_list_14)] =
+            [13, 14].map(|protocol_version| {
+                mainnet_masternode_list_replay(
+                    PlatformVersion::get(protocol_version).expect("a shipped protocol version"),
+                )
+            });
+        assert!(
+            stored_list_13 == stored_list_14,
+            "protocol versions 13 and 14 store the same masternode list"
+        );
+        let snapshot = format!(
+            "app hash at protocol version 13 {}\napp hash at protocol version 14 {}\n{stored_list_14}",
+            hex::encode(app_hash_13),
+            hex::encode(app_hash_14)
+        );
+        let path =
+            PathBuf::from(env!("CARGO_MANIFEST_DIR")).join(MAINNET_MASTERNODE_LIST_SNAPSHOT_PATH);
+
+        if env::var_os("UPDATE_MAINNET_MASTERNODE_LIST_SNAPSHOT").is_some() {
+            std::fs::write(&path, &snapshot).expect("expected to write the snapshot");
+            return;
+        }
+
+        let committed = std::fs::read_to_string(&path).expect("expected to read the snapshot");
+        let differing: Vec<(&str, &str)> = committed
+            .lines()
+            .zip(snapshot.lines())
+            .filter(|(committed, replayed)| committed != replayed)
+            .take(10)
+            .collect();
+        assert!(
+            committed == snapshot,
+            "replaying the mainnet diffs no longer stores what {MAINNET_MASTERNODE_LIST_SNAPSHOT_PATH} \
+             holds ({} lines committed, {} replayed); first differing lines, committed then \
+             replayed: {differing:?}",
+            committed.lines().count(),
+            snapshot.lines().count()
+        );
+    }
+
     /// A `protx listdiff` response from the mainnet fixtures. With
     /// `core_23_addresses`, every masternode and diff also lists the
     /// `addresses` Core 23 prints beside the legacy fields. `evonodes`

packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock/mod.rs

@@ -16,9 +16,14 @@ impl<C> Platform<C>
 where
     C: CoreRPCLike,
 {
-    /// Verify the chain lock
-    /// If submit is true, we try to submit the chain lock if it is considered valid or we can not check to see if it is
-    /// valid on platform.
+    /// Verify the chain lock against the chain lock quorums Platform holds, and leave it to
+    /// `verify_chain_lock_through_core` when Platform cannot tell on its own.
+    ///
+    /// With `make_sure_core_is_synced`, a chain lock found valid locally is submitted to Core
+    /// to find out whether Core is synced to it. A chain lock left to Core is submitted by
+    /// `verify_chain_lock_through_core` 0 without a signature check of its own; from
+    /// `verify_chain_lock_through_core` 1 (protocol version 15) it is submitted only after
+    /// Core's `verifychainlock` accepts it.
     pub fn verify_chain_lock(
         &self,
         round: u32,

packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/mod.rs

@@ -33,11 +33,11 @@ where
     ///   Platform holds exactly the quorums active at the signing height; version 0 also returns
     ///   it when Platform might not hold the quorum the chain lock was signed with
     /// * `Ok(None)` - Platform cannot tell on its own, so Core has to decide. Nothing about the
-    ///   signature is known yet: `verify_chain_lock_through_core` has to check it
+    ///   signature is known yet: `verify_chain_lock_through_core` has to check it. This is also
+    ///   the answer when Platform holds no quorum to verify against
     /// * `Err(Error::BLSError)` - The signature could not be deserialized
-    /// * `Err(Error::Execution)` - An unknown method version, no quorums to verify against, or
-    ///   from version 1 a platform version without DKG mining schedules or with a schedule whose
-    ///   interval is 0
+    /// * `Err(Error::Execution)` - An unknown method version, or from version 1 a platform
+    ///   version without DKG mining schedules or with a schedule whose interval is 0
     pub fn verify_chain_lock_locally(
         &self,
         round: u32,

packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v1/mod.rs

@@ -704,6 +704,66 @@ mod tests {
         }
     }
 
+    /// Platform versions that verify chain locks locally from version 1 on.
+    fn platform_versions_verifying_chain_locks_locally_from_version_1(
+    ) -> impl Iterator<Item = &'static PlatformVersion> {
+        let platform_versions = PLATFORM_VERSIONS.iter().filter(|platform_version| {
+            platform_version
+                .drive_abci
+                .methods
+                .core_chain_lock
+                .verify_chain_lock_locally
+                >= 1
+        });
+        assert!(
+            platform_versions.clone().next().is_some(),
+            "some protocol version verifies chain locks locally from version 1 on"
+        );
+        platform_versions
+    }
+
+    /// Version 1 fails every chain lock in a mining window without the DKG mining schedules of
+    /// its version table, and so every validator fails the block. A table that selects it must
+    /// carry them, not inherit `None` from an earlier table.
+    #[test]
+    fn should_have_dkg_mining_schedules_in_every_platform_version_verifying_chain_locks_locally_from_version_1(
+    ) {
+        for platform_version in platform_versions_verifying_chain_locks_locally_from_version_1() {
+            assert!(
+                platform_version
+                    .drive_abci
+                    .methods
+                    .core_chain_lock
+                    .dkg_mining_schedules
+                    .is_some(),
+                "protocol version {} has no DKG mining schedules",
+                platform_version.protocol_version
+            );
+        }
+    }
+
+    /// Version 1 leaves to Core chain locks version 0 rejected locally, so it is only safe when
+    /// Core checks their signature: from `verify_chain_lock_through_core` 1, `verifychainlock`
+    /// does before `submitchainlock`, which skips that check for a height Core already has a
+    /// chain lock at or above. With `verify_chain_lock_through_core` 0 a forged chain lock at
+    /// such a height would be accepted.
+    #[test]
+    fn should_check_chain_locks_left_to_core_with_verifychainlock_in_every_platform_version_verifying_locally_from_version_1(
+    ) {
+        for platform_version in platform_versions_verifying_chain_locks_locally_from_version_1() {
+            assert!(
+                platform_version
+                    .drive_abci
+                    .methods
+                    .core_chain_lock
+                    .verify_chain_lock_through_core
+                    >= 1,
+                "protocol version {} submits chain locks left to Core without verifychainlock",
+                platform_version.protocol_version
+            );
+        }
+    }
+
     #[test]
     fn should_find_first_possible_quorum_change_from_dash_core_dkg_schedule() {
         // Dash Core mines a commitment only at offsets mining_window_start..=mining_window_end