fix: read a coinbase payload of a newer version

dash-spv/src/sml_engine.rs

@@ -25,8 +25,8 @@ use dashcore::sml::masternode_list::MasternodeList;
 use dashcore::sml::quorum_entry::qualified_quorum_entry::QualifiedQuorumEntry;
 use dashcore::sml::quorum_entry::qualified_quorum_entry::VerifyingChainLockSignaturesType;
 use dashcore::sml::quorum_validation_error::QuorumValidationError;
+use dashcore::transaction::special_transaction::coinbase::CoinbasePayload;
 use dashcore::transaction::special_transaction::quorum_commitment::QuorumEntry;
-use dashcore::transaction::special_transaction::TransactionPayload;
 use dashcore::{BlockHash, QuorumHash};
 use dashcore::{Network, Transaction};
 use tokio::sync::RwLock;
@@ -173,16 +173,14 @@ fn build_cycle_quorum_map(
 
 /// The ChainLock `coinbase_tx`, the coinbase of the block at `height`, carries
 /// for member selection, as Dash Core's `GetNonNullCoinbaseChainlock` reads it
-/// (`src/evo/cbtx.cpp:200-211`).
+/// (`src/evo/cbtx.cpp:200-211`). A payload of a version newer than this
+/// library decodes is read as far as it knows the layout, see
+/// [`CoinbasePayload::from_coinbase_tx`].
 fn valid_coinbase_chain_lock(
     coinbase_tx: &Transaction,
     height: CoreBlockHeight,
 ) -> Option<BLSSignature> {
-    let Some(TransactionPayload::CoinbasePayloadType(payload)) =
-        &coinbase_tx.special_transaction_payload
-    else {
-        return None;
-    };
+    let payload = CoinbasePayload::from_coinbase_tx(coinbase_tx)?;
     let signature = payload.chain_lock_candidate(height)?;
     signature.as_scheme(BlsScheme::Modern).is_valid().then_some(signature)
 }
@@ -1483,7 +1481,7 @@ pub(crate) mod test_support {
 #[cfg(test)]
 mod tests {
     use super::test_support::{fixture_heights, quorum_entry, TestEngine};
-    use super::{build_cycle_quorum_map, qr_info_diffs};
+    use super::{build_cycle_quorum_map, qr_info_diffs, valid_coinbase_chain_lock};
     use super::{FeedError, MasternodeListEngine, QRInfoFeedResult, WORK_DIFF_DEPTH};
     use crate::error::{StorageError, StorageResult};
     use crate::storage::{BlockHeaderStorage, BlockHeaderTip};
@@ -3306,6 +3304,25 @@ mod tests {
         );
     }
 
+    /// A coinbase payload of a version this library does not decode still
+    /// carries the ChainLock member selection reads: Dash Core only appends
+    /// fields in a new version, so it holds the version 3 fields where they
+    /// were.
+    #[test]
+    fn a_newer_coinbase_payload_carries_its_chain_lock() {
+        const HEIGHT: CoreBlockHeight = 2_227_096;
+        let diff: MnListDiff = deserialize(include_bytes!(
+            "../../dash/tests/data/test_DML_diffs/mn_list_diff_0_2227096.bin"
+        ))
+        .expect("diff");
+        let chain_lock = valid_coinbase_chain_lock(&diff.coinbase_tx, HEIGHT);
+        assert!(chain_lock.is_some(), "the capture's coinbase carries a ChainLock");
+
+        let newer = diff.coinbase_tx.with_newer_coinbase_payload();
+
+        assert_eq!(valid_coinbase_chain_lock(&newer, HEIGHT), chain_lock);
+    }
+
     /// On an engine that checks coinbases, a restarted engine records the
     /// newest list's coinbase ChainLock from the diff from that block to
     /// itself, and records nothing from one whose coinbase proof or roots fail.

dash/src/blockdata/transaction/special_transaction/coinbase.rs

@@ -20,13 +20,19 @@
 
 use hashes::Hash;
 
+use super::{TransactionPayload, TransactionType};
+use crate::Transaction;
 use crate::bls_sig_utils::BLSSignature;
 use crate::consensus::encode::{compact_size_len, read_compact_size, write_compact_size};
 use crate::consensus::{Decodable, Encodable, encode};
 use crate::hash_types::{MerkleRootAssetUnlocks, MerkleRootMasternodeList, MerkleRootQuorums};
 use crate::io;
 use crate::io::{Error, ErrorKind};
 
+/// Newest payload version whose layout this crate decodes: the last version
+/// `CoinbasePayload::consensus_decode` reads fields for.
+const NEWEST_DECODED_VERSION: u16 = 4;
+
 /// A Coinbase payload. This is contained as the payload of a coinbase special transaction.
 /// The Coinbase payload is described in DIP4.
 ///
@@ -204,6 +210,32 @@ impl CoinbasePayload {
         }
     }
 
+    /// The payload of `coinbase`, read as far as this crate knows its layout.
+    ///
+    /// The transaction decoder keeps a payload raw when bytes follow the fields this crate
+    /// knows, as they do in a newer version that appends fields. Dash Core only appends fields in
+    /// a new version, so a raw payload of a version newer than this crate decodes is read as the
+    /// newest version it decodes, leaving the newer fields out. Should a later version change
+    /// the meaning of a field this crate knows, this has to change with it. The result keeps the
+    /// payload's `version`, so it does not encode to the payload's bytes.
+    ///
+    /// `None` for a transaction without a coinbase payload, a raw payload of a version this
+    /// crate decodes, which Dash Core never produces, or one too short for the fields this crate
+    /// knows.
+    pub fn from_coinbase_tx(coinbase: &Transaction) -> Option<CoinbasePayload> {
+        match &coinbase.special_transaction_payload {
+            Some(TransactionPayload::CoinbasePayloadType(payload)) => Some(payload.clone()),
+            Some(TransactionPayload::RawPayloadType(tx_type, bytes))
+                if *tx_type == TransactionType::Coinbase.to_u16() =>
+            {
+                CoinbasePayload::consensus_decode(&mut &bytes[..])
+                    .ok()
+                    .filter(|payload| payload.version > NEWEST_DECODED_VERSION)
+            }
+            _ => None,
+        }
+    }
+
     /// The ChainLock signature this coinbase carries for member selection at
     /// `block_height`, as Dash Core's `GetNonNullCoinbaseChainlock` reads it
     /// (`src/evo/cbtx.cpp:200-211`) short of `IsValid()`, which the caller
@@ -383,9 +415,11 @@ impl Decodable for CoinbasePayload {
 mod tests {
     use hashes::Hash;
 
+    use crate::Transaction;
     use crate::bls_sig_utils::BLSSignature;
     use crate::consensus::{Decodable, Encodable};
     use crate::hash_types::{MerkleRootAssetUnlocks, MerkleRootMasternodeList, MerkleRootQuorums};
+    use crate::transaction::special_transaction::TransactionPayload;
     use crate::transaction::special_transaction::coinbase::CoinbasePayload;
     use test_case::test_case;
 
@@ -680,4 +714,69 @@ mod tests {
         json["version"] = 3.into();
         assert!(serde_json::from_value::<CoinbasePayload>(json).is_err());
     }
+
+    /// A raw coinbase payload of a version newer than this crate decodes is
+    /// read as far as it knows the layout: the fields up to version 4, under
+    /// the payload's own version. A raw payload of a version it decodes, or
+    /// one cut short anywhere inside those fields, gives nothing.
+    #[test]
+    fn from_coinbase_tx_reads_the_known_fields_of_a_newer_payload() {
+        let typed = mainnet_v20_payload();
+        let coinbase = Transaction {
+            version: 3,
+            lock_time: 0,
+            input: vec![],
+            output: vec![],
+            special_transaction_payload: Some(TransactionPayload::CoinbasePayloadType(
+                typed.clone(),
+            )),
+        };
+        assert_eq!(CoinbasePayload::from_coinbase_tx(&coinbase), Some(typed.clone()));
+        let newer = coinbase.with_newer_coinbase_payload();
+        let Some(TransactionPayload::RawPayloadType(tx_type, bytes)) =
+            &newer.special_transaction_payload
+        else {
+            panic!("a raw payload");
+        };
+        let raw = |bytes: Vec<u8>| Transaction {
+            special_transaction_payload: Some(TransactionPayload::RawPayloadType(*tx_type, bytes)),
+            ..newer.clone()
+        };
+        let with_version = |version: u16| {
+            let mut bytes = bytes.clone();
+            bytes[..2].copy_from_slice(&version.to_le_bytes());
+            raw(bytes)
+        };
+
+        let known_fields = CoinbasePayload {
+            version: 4,
+            merkle_root_asset_unlocks: Some(MerkleRootAssetUnlocks::all_zeros()),
+            ..typed
+        };
+        for version in [5, u16::MAX] {
+            assert_eq!(
+                CoinbasePayload::from_coinbase_tx(&with_version(version)),
+                Some(CoinbasePayload {
+                    version,
+                    ..known_fields.clone()
+                }),
+                "version {version}"
+            );
+        }
+        for version in 0..=4 {
+            assert_eq!(
+                CoinbasePayload::from_coinbase_tx(&with_version(version)),
+                None,
+                "version {version}"
+            );
+        }
+        for len in 0..known_fields.size() {
+            assert_eq!(
+                CoinbasePayload::from_coinbase_tx(&raw(bytes[..len].to_vec())),
+                None,
+                "cut to {len} bytes"
+            );
+        }
+        assert_eq!(CoinbasePayload::from_coinbase_tx(&Transaction::dummy_empty()), None);
+    }
 }

dash/src/sml/masternode_list/merkle_roots.rs

@@ -4,7 +4,7 @@ use crate::Transaction;
 use crate::hash_types::{MerkleRootMasternodeList, MerkleRootQuorums};
 use crate::sml::error::SmlError;
 use crate::sml::masternode_list::MasternodeList;
-use crate::transaction::special_transaction::TransactionPayload;
+use crate::transaction::special_transaction::coinbase::CoinbasePayload;
 
 /// Computes the Merkle root from a list of hashes.
 ///
@@ -46,14 +46,14 @@ pub fn merkle_root_from_hashes(hashes: Vec<sha256d::Hash>) -> Option<sha256d::Ha
 
 impl MasternodeList {
     /// The quorum root is in the coinbase payload from its version 2. An empty
-    /// list hashes to the all-zero root, as in Dash Core.
+    /// list hashes to the all-zero root, as in Dash Core. A payload of a
+    /// version newer than this crate decodes is read as far as it knows the
+    /// layout, see [`CoinbasePayload::from_coinbase_tx`].
     pub fn verify_coinbase_roots(
         &self,
         coinbase_transaction: &Transaction,
     ) -> Result<(), SmlError> {
-        let Some(TransactionPayload::CoinbasePayloadType(coinbase_payload)) =
-            &coinbase_transaction.special_transaction_payload
-        else {
+        let Some(coinbase_payload) = CoinbasePayload::from_coinbase_tx(coinbase_transaction) else {
             return Err(SmlError::CoinbaseCommitmentMismatch(self.block_hash));
         };
 
@@ -159,14 +159,15 @@ mod tests {
     use hashes::Hash;
     use test_case::test_case;
 
-    use crate::consensus::deserialize;
+    use crate::consensus::{deserialize, serialize};
     use crate::hash_types::{MerkleRootMasternodeList, MerkleRootQuorums};
     use crate::network::message_sml::MnListDiff;
     use crate::sml::error::SmlError;
     use crate::sml::masternode_list::MasternodeList;
+    use crate::test_utils::UNASSIGNED_SPECIAL_TX_TYPE;
     use crate::transaction::special_transaction::TransactionPayload;
     use crate::transaction::special_transaction::coinbase::CoinbasePayload;
-    use crate::{BlockHash, Network};
+    use crate::{BlockHash, Network, Transaction};
 
     #[test_case(|_| {}, None; "matching")]
     #[test_case(
@@ -191,10 +192,27 @@ mod tests {
         tamper: fn(&mut CoinbasePayload),
         error: Option<fn(BlockHash) -> SmlError>,
     ) {
-        let diff: MnListDiff = deserialize(include_bytes!(
+        let (list, mut coinbase) = fixture_list(|_| {});
+        let Some(TransactionPayload::CoinbasePayloadType(payload)) =
+            &mut coinbase.special_transaction_payload
+        else {
+            panic!("coinbase payload");
+        };
+        tamper(payload);
+
+        let result = list.verify_coinbase_roots(&coinbase);
+
+        assert_eq!(result, error.map_or(Ok(()), |error| Err(error(list.block_hash))));
+    }
+
+    /// The list of the 2227096 capture, built after `edit`, and the capture's
+    /// coinbase committing to its masternodes.
+    fn fixture_list(edit: fn(&mut MnListDiff)) -> (MasternodeList, Transaction) {
+        let mut diff: MnListDiff = deserialize(include_bytes!(
             "../../../tests/data/test_DML_diffs/mn_list_diff_0_2227096.bin"
         ))
         .expect("expected to deserialize");
+        edit(&mut diff);
         let mut coinbase = diff.coinbase_tx.clone();
         let list = MasternodeList::from_diff(diff, 2_227_096, Network::Mainnet).expect("builds");
         let Some(TransactionPayload::CoinbasePayloadType(payload)) =
@@ -204,10 +222,74 @@ mod tests {
         };
         // The capture's masternodes do not hash to its coinbase's root, its quorums do.
         payload.merkle_root_masternode_list = list.masternode_merkle_root.expect("masternodes");
-        tamper(payload);
+        (list, coinbase)
+    }
+
+    /// `coinbase` with its payload written as a version this crate does not
+    /// decode, its type and bytes edited, and decoded back from the wire.
+    fn newer_coinbase(
+        coinbase: &Transaction,
+        edit: impl FnOnce(&mut u16, &mut Vec<u8>),
+    ) -> Transaction {
+        let mut newer = coinbase.with_newer_coinbase_payload();
+        let Some(TransactionPayload::RawPayloadType(tx_type, bytes)) =
+            &mut newer.special_transaction_payload
+        else {
+            panic!("a raw payload");
+        };
+        edit(tx_type, bytes);
+        let decoded: Transaction = deserialize(&serialize(&newer)).expect("decodes");
+        assert_eq!(decoded, newer, "the decoder keeps the payload raw");
+        decoded
+    }
+
+    /// A coinbase payload of a version this crate does not decode stays raw.
+    /// Dash Core only appends fields in a new version, so the roots are read
+    /// from the fields this crate knows, at bytes 6..38 and 38..70, and still
+    /// have to match. A raw payload of a version this crate decodes, which
+    /// Dash Core never produces, or of another transaction type commits to
+    /// nothing.
+    #[test_case(|_, _| {}, true; "of a newer version")]
+    #[test_case(|_, bytes| bytes[6] ^= 1, false; "of a newer version off the masternode list root")]
+    #[test_case(|_, bytes| bytes[38] ^= 1, false; "of a newer version off the quorum list root")]
+    #[test_case(
+        |_, bytes| bytes[..2].copy_from_slice(&4u16.to_le_bytes()),
+        false;
+        "of a known version with bytes left over"
+    )]
+    #[test_case(
+        |tx_type, _| *tx_type = UNASSIGNED_SPECIAL_TX_TYPE,
+        false;
+        "of another transaction type"
+    )]
+    fn verify_coinbase_roots_of_a_raw_payload(edit: fn(&mut u16, &mut Vec<u8>), matches: bool) {
+        let (list, coinbase) = fixture_list(|_| {});
+        let coinbase = newer_coinbase(&coinbase, edit);
 
         let result = list.verify_coinbase_roots(&coinbase);
 
-        assert_eq!(result, error.map_or(Ok(()), |error| Err(error(list.block_hash))));
+        let expected = if matches {
+            Ok(())
+        } else {
+            Err(SmlError::CoinbaseCommitmentMismatch(list.block_hash))
+        };
+        assert_eq!(result, expected);
+    }
+
+    /// A newer coinbase payload cut short before its quorum list root commits
+    /// to nothing, even for a list without quorums, whose root is all zeros.
+    #[test]
+    fn a_newer_coinbase_payload_cut_short_commits_to_nothing() {
+        let (list, coinbase) = fixture_list(|diff| {
+            diff.new_quorums.clear();
+            diff.quorums_chainlock_signatures.clear();
+        });
+        assert!(list.quorums.is_empty());
+        let coinbase = newer_coinbase(&coinbase, |_, bytes| bytes.truncate(38));
+        assert_eq!(CoinbasePayload::from_coinbase_tx(&coinbase), None);
+
+        let result = list.verify_coinbase_roots(&coinbase);
+
+        assert_eq!(result, Err(SmlError::CoinbaseCommitmentMismatch(list.block_hash)));
     }
 }

dash/src/test_utils/transaction.rs

@@ -1,5 +1,11 @@
 use std::ops::Range;
 
+use hashes::Hash;
+
+use crate::consensus::serialize;
+use crate::hash_types::MerkleRootAssetUnlocks;
+use crate::transaction::special_transaction::coinbase::CoinbasePayload;
+use crate::transaction::special_transaction::{TransactionPayload, TransactionType};
 use crate::{Address, OutPoint, ScriptBuf, Transaction, TxIn, TxOut, Txid, Witness};
 
 /// A special transaction type no Dash Core version assigns
@@ -75,4 +81,31 @@ impl Transaction {
             special_transaction_payload: None,
         }
     }
+
+    /// This coinbase with its version 3 payload written as a version this crate does not decode,
+    /// the way a later Dash Core release that appends fields would write it: the version 4
+    /// fields under version 5, then 8 bytes of a field of its own. The transaction decoder keeps
+    /// such a payload raw.
+    pub fn with_newer_coinbase_payload(&self) -> Transaction {
+        let Some(TransactionPayload::CoinbasePayloadType(payload)) =
+            &self.special_transaction_payload
+        else {
+            panic!("a coinbase payload");
+        };
+        assert_eq!(payload.version, 3, "a version 3 coinbase payload");
+        let mut bytes = serialize(&CoinbasePayload {
+            version: 4,
+            merkle_root_asset_unlocks: Some(MerkleRootAssetUnlocks::all_zeros()),
+            ..payload.clone()
+        });
+        bytes[..2].copy_from_slice(&5u16.to_le_bytes());
+        bytes.extend([0xA5; 8]);
+        Transaction {
+            special_transaction_payload: Some(TransactionPayload::RawPayloadType(
+                TransactionType::Coinbase.to_u16(),
+                bytes,
+            )),
+            ..self.clone()
+        }
+    }
 }