From 48cc36a4dfe9f4d9234d219f34b846a833c7a7a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ad=C3=A1n=20SDPC?= Date: Mon, 18 Nov 2024 17:29:59 +0100 Subject: [PATCH] fix(node): fix DR eligibility for (allegedly) all protocol versions fix #2527 --- data_structures/src/data_request.rs | 60 ++++++++++--------- data_structures/src/error.rs | 2 +- data_structures/src/proto/versioning.rs | 12 +++- data_structures/src/vrf.rs | 28 ++++++--- node/src/actors/chain_manager/mining.rs | 4 +- node/src/actors/chain_manager/mod.rs | 36 ++++++----- node/src/actors/inventory_manager/handlers.rs | 2 +- validations/src/tests/mod.rs | 20 +++---- 8 files changed, 98 insertions(+), 66 deletions(-) diff --git a/data_structures/src/data_request.rs b/data_structures/src/data_request.rs index 0bedbe409..e3b8fe76a 100644 --- a/data_structures/src/data_request.rs +++ b/data_structures/src/data_request.rs @@ -4,21 +4,20 @@ use std::{ }; use itertools::izip; - use serde::{Deserialize, Serialize}; -use crate::proto::versioning::ProtocolVersion; +use witnet_crypto::hash::calculate_sha256; + use crate::{ chain::{ - tapi::ActiveWips, DataRequestInfo, DataRequestOutput, DataRequestStage, DataRequestState, - Epoch, Hash, Hashable, PublicKeyHash, ValueTransferOutput, + DataRequestInfo, DataRequestOutput, DataRequestStage, DataRequestState, Epoch, + Hash, Hashable, PublicKeyHash, tapi::ActiveWips, ValueTransferOutput, }, error::{DataRequestError, TransactionError}, - get_protocol_version, + proto::versioning::{ProtocolVersion, VersionedHashable}, radon_report::{RadonReport, Stage, TypeLike}, transaction::{CommitTransaction, DRTransaction, RevealTransaction, TallyTransaction}, }; -use witnet_crypto::hash::calculate_sha256; /// Pool of active data requests #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] @@ -137,7 +136,7 @@ impl DataRequestPool { data_request: DRTransaction, block_hash: &Hash, ) -> Result<(), failure::Error> { - let dr_hash = data_request.hash(); + let dr_hash = data_request.versioned_hash(ProtocolVersion::from_epoch(epoch)); if data_request.signatures.is_empty() { return Err(TransactionError::SignatureNotFound.into()); } @@ -157,11 +156,13 @@ impl DataRequestPool { /// Add a commit to the corresponding data request fn add_commit( &mut self, + epoch: Epoch, pkh: PublicKeyHash, commit: CommitTransaction, block_hash: &Hash, ) -> Result<(), failure::Error> { - let tx_hash = commit.hash(); + let tx_hash = commit.versioned_hash(ProtocolVersion::from_epoch(epoch)); + log::debug!("Adding commit tx {}: {:?}", tx_hash, commit); // For a commit output, we need to get the corresponding data request input let dr_pointer = commit.body.dr_pointer; @@ -182,11 +183,12 @@ impl DataRequestPool { /// Add a reveal transaction fn add_reveal( &mut self, + epoch: Epoch, pkh: PublicKeyHash, reveal: RevealTransaction, block_hash: &Hash, ) -> Result<(), failure::Error> { - let tx_hash = reveal.hash(); + let tx_hash = reveal.versioned_hash(ProtocolVersion::from_epoch(epoch)); // For a commit output, we need to get the corresponding data request input let dr_pointer = reveal.body.dr_pointer; // The data request must be from a previous block, and must not be timelocked. @@ -338,20 +340,22 @@ impl DataRequestPool { pub fn process_commit( &mut self, commit_transaction: &CommitTransaction, + epoch: Epoch, block_hash: &Hash, ) -> Result<(), failure::Error> { let pkh = PublicKeyHash::from_public_key(&commit_transaction.signatures[0].public_key); - self.add_commit(pkh, commit_transaction.clone(), block_hash) + self.add_commit(epoch, pkh, commit_transaction.clone(), block_hash) } /// New reveals are added to their respective data requests, updating the stage to tally pub fn process_reveal( &mut self, reveal_transaction: &RevealTransaction, + epoch: Epoch, block_hash: &Hash, ) -> Result<(), failure::Error> { let pkh = PublicKeyHash::from_public_key(&reveal_transaction.signatures[0].public_key); - self.add_reveal(pkh, reveal_transaction.clone(), block_hash) + self.add_reveal(epoch, pkh, reveal_transaction.clone(), block_hash) } /// New data requests are inserted and wait for commitments @@ -554,7 +558,7 @@ pub fn data_request_has_too_many_witnesses( validator_count: usize, epoch: Option, ) -> bool { - if get_protocol_version(epoch) < ProtocolVersion::V2_0 { + if ProtocolVersion::from_epoch_opt(epoch) < ProtocolVersion::V2_0 { false } else { usize::from(dr_output.witnesses) > validator_count / 4 @@ -917,7 +921,7 @@ mod tests { vec![KeyedSignature::default()], ); - p.process_commit(&commit_transaction, &fake_block_hash) + p.process_commit(&commit_transaction, epoch, &fake_block_hash) .unwrap(); // And we can also get all the commit pointers from the data request @@ -967,7 +971,7 @@ mod tests { vec![KeyedSignature::default()], ); - p.process_reveal(&reveal_transaction, &fake_block_hash) + p.process_reveal(&reveal_transaction, 0, &fake_block_hash) .unwrap(); assert_eq!( @@ -1048,7 +1052,7 @@ mod tests { #[test] fn test_from_reveal_to_tally_3_stages_uncompleted() { - let (_epoch, fake_block_hash, mut p, dr_pointer) = add_data_requests_with_3_reveal_stages(); + let (epoch, fake_block_hash, mut p, dr_pointer) = add_data_requests_with_3_reveal_stages(); let commit_transaction = CommitTransaction::new( CommitTransactionBody::without_collateral( @@ -1075,9 +1079,9 @@ mod tests { }], ); - p.process_commit(&commit_transaction, &fake_block_hash) + p.process_commit(&commit_transaction, epoch, &fake_block_hash) .unwrap(); - p.process_commit(&commit_transaction2, &fake_block_hash) + p.process_commit(&commit_transaction2, epoch, &fake_block_hash) .unwrap(); // Update stages @@ -1098,7 +1102,7 @@ mod tests { vec![KeyedSignature::default()], ); - p.process_reveal(&reveal_transaction, &fake_block_hash) + p.process_reveal(&reveal_transaction, 0, &fake_block_hash) .unwrap(); // Update stages @@ -1139,7 +1143,7 @@ mod tests { #[test] fn test_from_reveal_to_tally_3_stages_completed() { - let (_epoch, fake_block_hash, mut p, dr_pointer) = add_data_requests_with_3_reveal_stages(); + let (epoch, fake_block_hash, mut p, dr_pointer) = add_data_requests_with_3_reveal_stages(); let commit_transaction = CommitTransaction::new( CommitTransactionBody::without_collateral( @@ -1166,9 +1170,9 @@ mod tests { }], ); - p.process_commit(&commit_transaction, &fake_block_hash) + p.process_commit(&commit_transaction, epoch, &fake_block_hash) .unwrap(); - p.process_commit(&commit_transaction2, &fake_block_hash) + p.process_commit(&commit_transaction2, epoch, &fake_block_hash) .unwrap(); // Update stages @@ -1189,7 +1193,7 @@ mod tests { vec![KeyedSignature::default()], ); - p.process_reveal(&reveal_transaction, &fake_block_hash) + p.process_reveal(&reveal_transaction, epoch, &fake_block_hash) .unwrap(); // Update stages @@ -1211,7 +1215,7 @@ mod tests { public_key: pk2, }], ); - p.process_reveal(&reveal_transaction2, &fake_block_hash) + p.process_reveal(&reveal_transaction2, epoch, &fake_block_hash) .unwrap(); // Update stages @@ -1225,7 +1229,7 @@ mod tests { #[test] fn test_from_reveal_to_tally_3_stages_zero_reveals() { - let (_epoch, fake_block_hash, mut p, dr_pointer) = add_data_requests_with_3_reveal_stages(); + let (epoch, fake_block_hash, mut p, dr_pointer) = add_data_requests_with_3_reveal_stages(); let commit_transaction = CommitTransaction::new( CommitTransactionBody::without_collateral( @@ -1252,9 +1256,9 @@ mod tests { }], ); - p.process_commit(&commit_transaction, &fake_block_hash) + p.process_commit(&commit_transaction, epoch, &fake_block_hash) .unwrap(); - p.process_commit(&commit_transaction2, &fake_block_hash) + p.process_commit(&commit_transaction2, epoch, &fake_block_hash) .unwrap(); // Update stages @@ -1315,7 +1319,7 @@ mod tests { #[test] fn my_claims() { // Test the `add_own_reveal` function - let (_epoch, fake_block_hash, mut p, dr_pointer) = add_data_requests(); + let (epoch, fake_block_hash, mut p, dr_pointer) = add_data_requests(); let commit_transaction = CommitTransaction::new( CommitTransactionBody::without_collateral( @@ -1340,7 +1344,7 @@ mod tests { Some(&reveal_transaction) ); - p.process_commit(&commit_transaction, &fake_block_hash) + p.process_commit(&commit_transaction, epoch, &fake_block_hash) .unwrap(); // Still in commit stage until we update diff --git a/data_structures/src/error.rs b/data_structures/src/error.rs index d5e88ed17..11f277e13 100644 --- a/data_structures/src/error.rs +++ b/data_structures/src/error.rs @@ -47,7 +47,7 @@ pub enum TransactionError { OutputNotFound { output: OutputPointer }, #[fail(display = "Data Request not found: {}", hash)] DataRequestNotFound { hash: Hash }, - #[fail(display = "Commit transaction has a invalid Proof of Eligibility")] + #[fail(display = "Commit transaction has an invalid Proof of Eligibility")] InvalidDataRequestPoe, #[fail( display = "Validator {} is not eligible to commit to a data request", diff --git a/data_structures/src/proto/versioning.rs b/data_structures/src/proto/versioning.rs index 86b23ff50..ccec78a97 100644 --- a/data_structures/src/proto/versioning.rs +++ b/data_structures/src/proto/versioning.rs @@ -97,14 +97,22 @@ pub enum ProtocolVersion { } impl ProtocolVersion { + #[inline] pub fn guess() -> Self { - get_protocol_version(None) + Self::from_epoch_opt(None) } + #[inline] pub fn from_epoch(epoch: Epoch) -> Self { - get_protocol_version(Some(epoch)) + Self::from_epoch_opt(Some(epoch)) + } + + #[inline] + pub fn from_epoch_opt(epoch: Option) -> Self { + get_protocol_version(epoch) } + pub fn next(&self) -> Self { match self { ProtocolVersion::V1_7 => ProtocolVersion::V1_8, diff --git a/data_structures/src/vrf.rs b/data_structures/src/vrf.rs index be4ef2277..352f40c86 100644 --- a/data_structures/src/vrf.rs +++ b/data_structures/src/vrf.rs @@ -217,20 +217,21 @@ impl DataRequestEligibilityClaim { } /// Verify a data request eligibility claim for a given vrf message and data request hash + /// FIXME: this assumes protocol version 1_7 pub fn verify( &self, vrf: &mut VrfCtx, vrf_input: CheckpointVRF, dr_hash: Hash, ) -> Result { - self.proof - .verify(vrf, &VrfMessage::data_request_v1(vrf_input, dr_hash)) - .map(|v| { - let mut sha256 = [0; 32]; - sha256.copy_from_slice(&v); + let vrf_mesage = VrfMessage::data_request_v1(vrf_input, dr_hash); + log::debug!("Verifying DR eligibility claim for dr {}.\nProof: {}\nVrf input: {:?}\nVrf message: {}", dr_hash, hex::encode(&self.proof.proof), vrf_input, hex::encode(&vrf_mesage.0)); + self.proof.verify(vrf, &vrf_mesage).map(|v| { + let mut sha256 = [0; 32]; + sha256.copy_from_slice(&v); - Hash::SHA256(sha256) - }) + Hash::SHA256(sha256) + }) } } @@ -317,4 +318,17 @@ mod tests { .unwrap(); assert!(proof.verify(vrf, vrf_input, dr_pointer2).is_err()); } + + #[test] + pub fn vrf_message_versions() { + let vrf_input = CheckpointVRF::default(); + let dr_hash = Hash::default(); + let withdrawer = PublicKeyHash::default(); + + let VrfMessage(v1_bytes) = VrfMessage::data_request_v1(vrf_input, dr_hash); + let VrfMessage(v2_bytes) = + VrfMessage::data_request_v2(vrf_input, dr_hash, Some(withdrawer)); + + assert_eq!(v1_bytes, v2_bytes[..(v2_bytes.len() - 24)]); + } } diff --git a/node/src/actors/chain_manager/mining.rs b/node/src/actors/chain_manager/mining.rs index 4cf2f432f..944dec504 100644 --- a/node/src/actors/chain_manager/mining.rs +++ b/node/src/actors/chain_manager/mining.rs @@ -469,9 +469,9 @@ impl ChainManager { // distinguish eligibility for stake entries belonging to multiple withdrawers but // operated by a single validator acting as a delegatee. let vrf_message = if protocol_version >= V2_0 { - VrfMessage::data_request_v1(dr_vrf_input, dr_pointer) - } else { VrfMessage::data_request_v2(dr_vrf_input, dr_pointer, first_withdrawer_address) + } else { + VrfMessage::data_request_v1(dr_vrf_input, dr_pointer) }; signature_mngr::vrf_prove(vrf_message) diff --git a/node/src/actors/chain_manager/mod.rs b/node/src/actors/chain_manager/mod.rs index 8fd3ec769..bc36a37ba 100644 --- a/node/src/actors/chain_manager/mod.rs +++ b/node/src/actors/chain_manager/mod.rs @@ -3111,7 +3111,7 @@ pub fn process_validations( protocol_version, replication_factor, )?; - log::debug!("Verifying {} block signatures", signatures_to_verify.len()); + log::trace!("Verifying {} block signatures", signatures_to_verify.len()); verify_signatures(signatures_to_verify, vrf_ctx)?; } @@ -3134,8 +3134,8 @@ pub fn process_validations( )?; if !resynchronizing { - log::debug!( - "Verifying {} transactions signatures", + log::trace!( + "Verifying {} transaction signatures", signatures_to_verify.len() ); verify_signatures(signatures_to_verify, vrf_ctx)?; @@ -3217,6 +3217,9 @@ fn update_pools( state_machine: StateMachine, ) -> ReputationInfo { let mut rep_info = ReputationInfo::new(); + let epoch = block.block_header.beacon.checkpoint; + let protocol = ProtocolVersion::from_epoch(epoch); + let block_hash = block.versioned_hash(protocol); let mut data_requests_with_too_many_witnesses = HashSet::::new(); for ta_tx in &block.txns.tally_txns { @@ -3233,7 +3236,7 @@ fn update_pools( rep_info.update(ta_tx, data_request_pool, own_pkh, node_stats); // IMPORTANT: Update the data request pool after updating reputation info - if let Err(e) = data_request_pool.process_tally(ta_tx, &block.hash()) { + if let Err(e) = data_request_pool.process_tally(ta_tx, &block_hash) { log::error!("Error processing tally transaction:\n{}", e); } @@ -3245,15 +3248,16 @@ fn update_pools( } for dr_tx in &block.txns.data_request_txns { - if data_requests_with_too_many_witnesses.contains(&dr_tx.hash()) { - log::debug!("Skipping data request {} as it was already processed with a TooManyWitnesses error", dr_tx.hash()); + let dr_hash = dr_tx.versioned_hash(protocol); + if data_requests_with_too_many_witnesses.contains(&dr_hash) { + log::debug!("Skipping data request {} as it was already processed with a TooManyWitnesses error", dr_hash); transactions_pool.dr_remove(dr_tx); continue; } if let Err(e) = data_request_pool.process_data_request( dr_tx, block.block_header.beacon.checkpoint, - &block.hash(), + &block_hash, ) { log::error!("Error processing data request transaction:\n{}", e); } else { @@ -3262,7 +3266,7 @@ fn update_pools( } for co_tx in &block.txns.commit_txns { - if let Err(e) = data_request_pool.process_commit(co_tx, &block.hash()) { + if let Err(e) = data_request_pool.process_commit(co_tx, epoch, &block_hash) { log::error!("Error processing commit transaction:\n{}", e); } else { if co_tx.body.proof.proof.pkh() == own_pkh { @@ -3279,10 +3283,11 @@ fn update_pools( } for re_tx in &block.txns.reveal_txns { - if let Err(e) = data_request_pool.process_reveal(re_tx, &block.hash()) { + if let Err(e) = data_request_pool.process_reveal(re_tx, epoch, &block_hash) { log::error!("Error processing reveal transaction:\n{}", e); } - transactions_pool.remove_one_reveal(&re_tx.body.dr_pointer, &re_tx.body.pkh, &re_tx.hash()); + let re_hash = re_tx.versioned_hash(protocol); + transactions_pool.remove_one_reveal(&re_tx.body.dr_pointer, &re_tx.body.pkh, &re_hash); } for st_tx in &block.txns.stake_txns { @@ -3785,6 +3790,7 @@ mod tests { fn test_rep_info_update() { let mut rep_info = ReputationInfo::default(); let mut dr_pool = DataRequestPool::default(); + let epoch = 0; let pk1 = PublicKey { compressed: 0, @@ -3849,12 +3855,12 @@ mod tests { dr_pool .add_data_request(1, dr_tx, &Hash::default()) .unwrap(); - dr_pool.process_commit(&co_tx, &Hash::default()).unwrap(); - dr_pool.process_commit(&co_tx2, &Hash::default()).unwrap(); - dr_pool.process_commit(&co_tx3, &Hash::default()).unwrap(); + dr_pool.process_commit(&co_tx, epoch, &Hash::default()).unwrap(); + dr_pool.process_commit(&co_tx2, epoch, &Hash::default()).unwrap(); + dr_pool.process_commit(&co_tx3, epoch, &Hash::default()).unwrap(); dr_pool.update_data_request_stages(None, None); - dr_pool.process_reveal(&re_tx1, &Hash::default()).unwrap(); - dr_pool.process_reveal(&re_tx2, &Hash::default()).unwrap(); + dr_pool.process_reveal(&re_tx1, epoch, &Hash::default()).unwrap(); + dr_pool.process_reveal(&re_tx2, epoch, &Hash::default()).unwrap(); rep_info.update( &ta_tx, diff --git a/node/src/actors/inventory_manager/handlers.rs b/node/src/actors/inventory_manager/handlers.rs index 71a10b32c..5026d42ea 100644 --- a/node/src/actors/inventory_manager/handlers.rs +++ b/node/src/actors/inventory_manager/handlers.rs @@ -40,7 +40,7 @@ impl InventoryManager { let total = msg.items.len(); for (i, item) in msg.items.into_iter().enumerate() { - log::debug!("Adding item {} out of {}", i, total); + log::trace!("Adding item {} out of {}", i, total); match item { StoreInventoryItem::Block(block) => { diff --git a/validations/src/tests/mod.rs b/validations/src/tests/mod.rs index e08113296..e29c28cdb 100644 --- a/validations/src/tests/mod.rs +++ b/validations/src/tests/mod.rs @@ -3609,7 +3609,7 @@ fn commitment_dr_in_reveal_stage() { let cs = sign_tx(PRIV_KEY_1, &cb, None); let c_tx = CommitTransaction::new(cb, vec![cs]); - dr_pool.process_commit(&c_tx, &block_hash).unwrap(); + dr_pool.process_commit(&c_tx, dr_epoch, &block_hash).unwrap(); dr_pool.update_data_request_stages(None, Some(dr_epoch)); let mut signatures_to_verify = vec![]; @@ -4192,7 +4192,7 @@ fn dr_pool_with_dr_in_reveal_stage() -> (DataRequestPool, Hash) { .process_data_request(&dr_transaction, epoch, &Hash::default()) .unwrap(); dr_pool.update_data_request_stages(None, Some(epoch)); - dr_pool.process_commit(&c_tx, &block_hash).unwrap(); + dr_pool.process_commit(&c_tx, epoch, &block_hash).unwrap(); dr_pool.update_data_request_stages(None, Some(epoch)); (dr_pool, dr_pointer) @@ -4460,7 +4460,7 @@ fn reveal_valid_commitment() { // Include CommitTransaction in DataRequestPool dr_pool - .process_commit(&commit_transaction, &fake_block_hash) + .process_commit(&commit_transaction, epoch, &fake_block_hash) .unwrap(); dr_pool.update_data_request_stages(None, Some(epoch)); @@ -4486,7 +4486,7 @@ fn reveal_valid_commitment() { // Include RevealTransaction in DataRequestPool dr_pool - .process_reveal(&reveal_transaction, &fake_block_hash) + .process_reveal(&reveal_transaction, epoch, &fake_block_hash) .unwrap(); dr_pool.update_data_request_stages(None, Some(epoch)); @@ -4631,7 +4631,7 @@ fn include_commits( let fake_block_hash = Hash::SHA256([1; 32]); for commit in commits.iter().take(commits_count) { - dr_pool.process_commit(commit, &fake_block_hash).unwrap(); + dr_pool.process_commit(commit, 0, &fake_block_hash).unwrap(); } dr_pool.update_data_request_stages(None, None); } @@ -4645,7 +4645,7 @@ fn include_reveals( let fake_block_hash = Hash::SHA256([2; 32]); for reveal in reveals.iter().take(reveals_count) { - dr_pool.process_reveal(reveal, &fake_block_hash).unwrap(); + dr_pool.process_reveal(reveal, 0, &fake_block_hash).unwrap(); } dr_pool.update_data_request_stages(None, None); } @@ -5110,7 +5110,7 @@ fn tally_dr_not_tally_stage() { ); dr_pool - .process_commit(&commit_transaction, &fake_block_hash) + .process_commit(&commit_transaction, epoch, &fake_block_hash) .unwrap(); dr_pool.update_data_request_stages(None, Some(epoch)); let x = validate_tally_transaction( @@ -5127,7 +5127,7 @@ fn tally_dr_not_tally_stage() { ); dr_pool - .process_reveal(&reveal_transaction, &fake_block_hash) + .process_reveal(&reveal_transaction, epoch, &fake_block_hash) .unwrap(); dr_pool.update_data_request_stages(None, Some(epoch)); let x = validate_tally_transaction( @@ -10228,10 +10228,10 @@ fn block_duplicated_reveals() { // Include CommitTransaction in DataRequestPool dr_pool - .process_commit(&commit_transaction, &last_block_hash) + .process_commit(&commit_transaction, dr_epoch, &last_block_hash) .unwrap(); dr_pool - .process_commit(&commit_transaction2, &last_block_hash) + .process_commit(&commit_transaction2, dr_epoch, &last_block_hash) .unwrap(); dr_pool.update_data_request_stages(None, Some(dr_epoch));