From 04452a3e14c010d557900de4da3c2a1b40074145 Mon Sep 17 00:00:00 2001 From: Herr Seppia Date: Mon, 16 Dec 2024 10:33:53 +0100 Subject: [PATCH 1/6] consensus: use prev_state_root for verification --- consensus/src/commons.rs | 6 ++++++ consensus/src/operations.rs | 2 ++ consensus/src/proposal/block_generator.rs | 1 + consensus/src/validation/step.rs | 15 +++++++++++++-- 4 files changed, 22 insertions(+), 2 deletions(-) diff --git a/consensus/src/commons.rs b/consensus/src/commons.rs index 674f66eeb..5dc108b81 100644 --- a/consensus/src/commons.rs +++ b/consensus/src/commons.rs @@ -33,6 +33,7 @@ pub struct RoundUpdate { seed: Seed, hash: [u8; 32], + state_root: [u8; 32], att: Attestation, att_voters: Vec, timestamp: u64, @@ -59,6 +60,7 @@ impl RoundUpdate { timestamp: tip_header.timestamp, base_timeouts, att_voters, + state_root: tip_header.state_hash, } } @@ -81,6 +83,10 @@ impl RoundUpdate { pub fn att_voters(&self) -> &Vec { &self.att_voters } + + pub fn state_root(&self) -> [u8; 32] { + self.state_root + } } #[async_trait::async_trait] diff --git a/consensus/src/operations.rs b/consensus/src/operations.rs index 4b776aec9..99246a082 100644 --- a/consensus/src/operations.rs +++ b/consensus/src/operations.rs @@ -26,6 +26,7 @@ pub struct CallParams { pub to_slash: Vec, pub voters_pubkey: Vec, pub max_txs_bytes: usize, + pub prev_state_root: StateRoot, } #[derive(Default)] @@ -77,6 +78,7 @@ pub trait Operations: Send + Sync { async fn verify_state_transition( &self, + prev_commit: StateRoot, blk: &Block, voters: &[Voter], ) -> Result; diff --git a/consensus/src/proposal/block_generator.rs b/consensus/src/proposal/block_generator.rs index 5ee57eddc..fe56b2103 100644 --- a/consensus/src/proposal/block_generator.rs +++ b/consensus/src/proposal/block_generator.rs @@ -132,6 +132,7 @@ impl Generator { to_slash, voters_pubkey: voters.to_owned(), max_txs_bytes, + prev_state_root: ru.state_root(), }; let result = diff --git a/consensus/src/validation/step.rs b/consensus/src/validation/step.rs index 01dc02418..ae3b38882 100644 --- a/consensus/src/validation/step.rs +++ b/consensus/src/validation/step.rs @@ -103,7 +103,14 @@ impl ValidationStep { error!(event = "invalid faults", ?err); Vote::Invalid(header.hash) } else { - match Self::call_vst(candidate, &voters, &executor).await { + match Self::call_vst( + ru.state_root(), + candidate, + &voters, + &executor, + ) + .await + { Ok(_) => Vote::Valid(header.hash), Err(err) => { error!(event = "failed_vst_call", ?err); @@ -154,11 +161,15 @@ impl ValidationStep { } async fn call_vst( + prev_commit: [u8; 32], candidate: &Block, voters: &[Voter], executor: &Arc, ) -> anyhow::Result<()> { - match executor.verify_state_transition(candidate, voters).await { + match executor + .verify_state_transition(prev_commit, candidate, voters) + .await + { Ok(output) => { // Ensure the `event_bloom` and `state_root` returned // from the VST call are the From 0420ec56af9eddbf174014175857569a9a2f7e5e Mon Sep 17 00:00:00 2001 From: Herr Seppia Date: Mon, 16 Dec 2024 10:34:30 +0100 Subject: [PATCH 2/6] node: use prev_state_root for verification --- node/src/chain/acceptor.rs | 10 +++++++--- node/src/chain/consensus.rs | 3 ++- node/src/vm.rs | 2 ++ 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/node/src/chain/acceptor.rs b/node/src/chain/acceptor.rs index 152dd5315..79b6fd684 100644 --- a/node/src/chain/acceptor.rs +++ b/node/src/chain/acceptor.rs @@ -628,6 +628,7 @@ impl Acceptor { let mut task = self.task.write().await; let mut tip = self.tip.write().await; + let prev_header = tip.inner().header().clone(); let mut provisioners_list = self.provisioners_list.write().await; let block_time = blk.header().timestamp - tip.inner().header().timestamp; @@ -636,7 +637,7 @@ impl Acceptor { // Verify Block Header let (pni, prev_block_voters, tip_block_voters) = verify_block_header( self.db.clone(), - &tip.inner().header().clone(), + &prev_header, &provisioners_list, blk.header(), ) @@ -658,8 +659,11 @@ impl Acceptor { let vm = self.vm.write().await; let (stakes, finality) = self.db.read().await.update(|db| { - let (txs, verification_output, stake_events) = - vm.accept(blk, &prev_block_voters[..])?; + let (txs, verification_output, stake_events) = vm.accept( + prev_header.state_hash, + blk, + &prev_block_voters[..], + )?; for spent_tx in txs.iter() { events.push(TransactionEvent::Executed(spent_tx).into()); } diff --git a/node/src/chain/consensus.rs b/node/src/chain/consensus.rs index 9a3d2a518..13727e393 100644 --- a/node/src/chain/consensus.rs +++ b/node/src/chain/consensus.rs @@ -315,6 +315,7 @@ impl Operations for Executor { async fn verify_state_transition( &self, + prev_root: [u8; 32], blk: &Block, voters: &[Voter], ) -> Result { @@ -322,7 +323,7 @@ impl Operations for Executor { let vm = self.vm.read().await; - vm.verify_state_transition(blk, voters) + vm.verify_state_transition(prev_root, blk, voters) .map_err(OperationError::InvalidVST) } diff --git a/node/src/vm.rs b/node/src/vm.rs index 00b68df8f..f43899a82 100644 --- a/node/src/vm.rs +++ b/node/src/vm.rs @@ -29,12 +29,14 @@ pub trait VMExecution: Send + Sync + 'static { fn verify_state_transition( &self, + prev_root: [u8; 32], blk: &Block, voters: &[Voter], ) -> anyhow::Result; fn accept( &self, + prev_root: [u8; 32], blk: &Block, voters: &[Voter], ) -> anyhow::Result<( From a448ee6ac144b53df4120b501a5146f4deaa5bfc Mon Sep 17 00:00:00 2001 From: Herr Seppia Date: Mon, 16 Dec 2024 10:34:56 +0100 Subject: [PATCH 3/6] rusk: use prev_state_root for verification --- rusk/benches/block_ingestion.rs | 2 ++ rusk/src/lib/node/rusk.rs | 17 +++++++++++------ rusk/src/lib/node/vm.rs | 4 ++++ rusk/tests/common/state.rs | 16 ++++++++++++---- 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/rusk/benches/block_ingestion.rs b/rusk/benches/block_ingestion.rs index 715ee0cd3..0a4415db5 100644 --- a/rusk/benches/block_ingestion.rs +++ b/rusk/benches/block_ingestion.rs @@ -118,6 +118,7 @@ fn bench_accept( let generator = PublicKey::new(*DUSK_CONSENSUS_KEY).into_inner(); let txs = Arc::new(txs); + let prev_root = rusk.state_root(); for n_txs in N_TXS { let rusk = rusk.clone(); @@ -131,6 +132,7 @@ fn bench_accept( let txs = txs[..*n_txs].to_vec(); rusk.accept_transactions( + prev_root, BLOCK_HEIGHT, BLOCK_GAS_LIMIT, BLOCK_HASH, diff --git a/rusk/src/lib/node/rusk.rs b/rusk/src/lib/node/rusk.rs index 0b8da4b1f..bc0b72fcd 100644 --- a/rusk/src/lib/node/rusk.rs +++ b/rusk/src/lib/node/rusk.rs @@ -121,10 +121,12 @@ impl Rusk { let block_gas_limit = self.block_gas_limit; let generator = params.generator_pubkey.inner(); let to_slash = params.to_slash.clone(); + let prev_state_root = params.prev_state_root; let voters = ¶ms.voters_pubkey[..]; - let mut session = self.new_block_session(block_height, None)?; + let mut session = + self.new_block_session(block_height, prev_state_root)?; let mut block_gas_left = block_gas_limit; @@ -179,7 +181,8 @@ impl Rusk { // transaction, since it is technically valid. if gas_spent > block_gas_left { info!("Skipping {tx_id_hex} due gas_spent {gas_spent} greater than left: {block_gas_left}"); - session = self.new_block_session(block_height, None)?; + session = self + .new_block_session(block_height, prev_state_root)?; for spent_tx in &spent_txs { // We know these transactions were correctly @@ -259,6 +262,7 @@ impl Rusk { #[allow(clippy::too_many_arguments)] pub fn verify_transactions( &self, + prev_commit: [u8; 32], block_height: u64, block_hash: Hash, block_gas_limit: u64, @@ -267,7 +271,7 @@ impl Rusk { slashing: Vec, voters: &[Voter], ) -> Result<(Vec, VerificationOutput)> { - let session = self.new_block_session(block_height, None)?; + let session = self.new_block_session(block_height, prev_commit)?; accept( session, @@ -292,6 +296,7 @@ impl Rusk { #[allow(clippy::too_many_arguments)] pub fn accept_transactions( &self, + prev_commit: [u8; 32], block_height: u64, block_gas_limit: u64, block_hash: Hash, @@ -305,7 +310,7 @@ impl Rusk { VerificationOutput, Vec, )> { - let session = self.new_block_session(block_height, None)?; + let session = self.new_block_session(block_height, prev_commit)?; let (spent_txs, verification_output, session, events) = accept( session, @@ -472,9 +477,9 @@ impl Rusk { pub(crate) fn new_block_session( &self, block_height: u64, - commit: Option<[u8; 32]>, + commit: [u8; 32], ) -> Result { - let mut session = self._session(block_height, commit)?; + let mut session = self._session(block_height, Some(commit))?; let _: CallReceipt<()> = session .call(STAKE_CONTRACT, "before_state_transition", &(), u64::MAX) .expect("before_state_transition to success"); diff --git a/rusk/src/lib/node/vm.rs b/rusk/src/lib/node/vm.rs index fdcff50b5..87da36518 100644 --- a/rusk/src/lib/node/vm.rs +++ b/rusk/src/lib/node/vm.rs @@ -45,6 +45,7 @@ impl VMExecution for Rusk { fn verify_state_transition( &self, + prev_commit: [u8; 32], blk: &Block, voters: &[Voter], ) -> anyhow::Result { @@ -57,6 +58,7 @@ impl VMExecution for Rusk { let (_, verification_output) = self .verify_transactions( + prev_commit, blk.header().height, blk.header().hash, blk.header().gas_limit, @@ -72,6 +74,7 @@ impl VMExecution for Rusk { fn accept( &self, + prev_root: [u8; 32], blk: &Block, voters: &[Voter], ) -> anyhow::Result<( @@ -88,6 +91,7 @@ impl VMExecution for Rusk { let (txs, verification_output, stake_events) = self .accept_transactions( + prev_root, blk.header().height, blk.header().gas_limit, blk.header().hash, diff --git a/rusk/tests/common/state.rs b/rusk/tests/common/state.rs index 79e7f964e..5a533d134 100644 --- a/rusk/tests/common/state.rs +++ b/rusk/tests/common/state.rs @@ -103,6 +103,7 @@ pub fn generator_procedure( missed_generators: Vec, expected: Option, ) -> anyhow::Result> { + let prev_root = rusk.state_root(); let expected = expected.unwrap_or(ExecuteResult { executed: txs.len(), discarded: 0, @@ -146,6 +147,7 @@ pub fn generator_procedure( to_slash, voters_pubkey: voters.clone(), max_txs_bytes: usize::MAX, + prev_state_root: prev_root, }; let (transfer_txs, discarded, execute_output) = @@ -176,10 +178,12 @@ pub fn generator_procedure( ) .expect("valid block"); - let verify_output = rusk.verify_state_transition(&block, &voters)?; + let verify_output = + rusk.verify_state_transition(prev_root, &block, &voters)?; info!("verify_state_transition new verification: {verify_output}",); - let (accept_txs, accept_output, _) = rusk.accept(&block, &voters)?; + let (accept_txs, accept_output, _) = + rusk.accept(prev_root, &block, &voters)?; assert_eq!(accept_txs.len(), expected.executed, "all txs accepted"); @@ -210,6 +214,7 @@ pub fn generator_procedure2( expected: Option, generator: Option, ) -> anyhow::Result<(Vec, [u8; 32])> { + let prev_root = rusk.state_root(); let expected = expected.unwrap_or(ExecuteResult { executed: txs.len(), discarded: 0, @@ -254,6 +259,7 @@ pub fn generator_procedure2( to_slash, voters_pubkey: voters.clone(), max_txs_bytes: usize::MAX, + prev_state_root: prev_root, }; let (transfer_txs, discarded, execute_output) = @@ -284,10 +290,12 @@ pub fn generator_procedure2( ) .expect("valid block"); - let verify_output = rusk.verify_state_transition(&block, &voters)?; + let verify_output = + rusk.verify_state_transition(prev_root, &block, &voters)?; info!("verify_state_transition new verification: {verify_output}",); - let (accept_txs, accept_output, _) = rusk.accept(&block, &voters)?; + let (accept_txs, accept_output, _) = + rusk.accept(prev_root, &block, &voters)?; assert_eq!(accept_txs.len(), expected.executed, "all txs accepted"); From baa30219da4c1edf460d23a6dc03477b01e95deb Mon Sep 17 00:00:00 2001 From: Herr Seppia Date: Wed, 18 Dec 2024 16:09:28 +0100 Subject: [PATCH 4/6] consensus: don't vote Invalid if tip change due to race conditions --- consensus/src/errors.rs | 32 +++++++++++++++++- consensus/src/operations.rs | 2 +- consensus/src/validation/step.rs | 56 +++++++++++++++----------------- 3 files changed, 58 insertions(+), 32 deletions(-) diff --git a/consensus/src/errors.rs b/consensus/src/errors.rs index 4e60ce4ec..0e9881cb6 100644 --- a/consensus/src/errors.rs +++ b/consensus/src/errors.rs @@ -70,7 +70,7 @@ impl From for ConsensusError { #[derive(Debug, Error)] pub enum OperationError { #[error("failed to call VST {0}")] - InvalidVST(anyhow::Error), + InvalidVST(VstError), #[error("failed to call EST {0}")] InvalidEST(anyhow::Error), #[error("failed to verify header {0}")] @@ -116,6 +116,36 @@ pub enum HeaderError { Storage(&'static str, anyhow::Error), } +#[derive(Debug, Error)] +pub enum VstError { + #[error( + "mismatch, event_bloom: {}, candidate_event_bloom: {}", + hex::encode(.0.as_ref()), + hex::encode(.1.as_ref()) + )] + MismatchEventBloom(Box<[u8; 256]>, Box<[u8; 256]>), + #[error( + "mismatch, state_hash: {}, candidate_state_hash: {}", + hex::encode(.0), + hex::encode(.1) + )] + MismatchStateHash([u8; 32], [u8; 32]), + #[error("Chain tip different from the expected one")] + TipChanged, + #[error("Invalid slash from block: {0}")] + InvalidSlash(io::Error), + #[error("Invalid generator: {0:?}")] + InvalidGenerator(dusk_bytes::Error), + #[error("Generic error in vst: {0}")] + Generic(String), +} + +impl VstError { + pub fn must_vote(&self) -> bool { + !matches!(self, Self::TipChanged) + } +} + impl HeaderError { pub fn must_vote(&self) -> bool { match self { diff --git a/consensus/src/operations.rs b/consensus/src/operations.rs index 99246a082..d02cedf9a 100644 --- a/consensus/src/operations.rs +++ b/consensus/src/operations.rs @@ -81,7 +81,7 @@ pub trait Operations: Send + Sync { prev_commit: StateRoot, blk: &Block, voters: &[Voter], - ) -> Result; + ) -> Result; async fn execute_state_transition( &self, diff --git a/consensus/src/validation/step.rs b/consensus/src/validation/step.rs index ae3b38882..b9088705b 100644 --- a/consensus/src/validation/step.rs +++ b/consensus/src/validation/step.rs @@ -6,7 +6,6 @@ use std::sync::Arc; -use anyhow::anyhow; use node_data::bls::PublicKeyBytes; use node_data::ledger::{to_str, Block}; use node_data::message::payload::{Validation, Vote}; @@ -19,6 +18,7 @@ use tracing::{debug, error, info, Instrument}; use crate::commons::{Database, RoundUpdate}; use crate::config::is_emergency_iter; +use crate::errors::VstError; use crate::execution_ctx::ExecutionCtx; use crate::msg_handler::StepOutcome; use crate::operations::{Operations, Voter}; @@ -113,7 +113,11 @@ impl ValidationStep { { Ok(_) => Vote::Valid(header.hash), Err(err) => { - error!(event = "failed_vst_call", ?err); + let voting = err.must_vote(); + error!(event = "invalid_vst", ?err, voting); + if !voting { + return; + } Vote::Invalid(header.hash) } } @@ -165,36 +169,28 @@ impl ValidationStep { candidate: &Block, voters: &[Voter], executor: &Arc, - ) -> anyhow::Result<()> { - match executor + ) -> Result<(), VstError> { + let output = executor .verify_state_transition(prev_commit, candidate, voters) - .await - { - Ok(output) => { - // Ensure the `event_bloom` and `state_root` returned - // from the VST call are the - // ones we expect to have with the - // current candidate block. - if output.event_bloom != candidate.header().event_bloom { - return Err(anyhow!( - "mismatch, event_bloom: {}, candidate_event_bloom: {}", - hex::encode(output.event_bloom), - hex::encode(candidate.header().event_bloom) - )); - } + .await?; - if output.state_root != candidate.header().state_hash { - return Err(anyhow!( - "mismatch, state_hash: {}, candidate_state_hash: {}", - hex::encode(output.state_root), - hex::encode(candidate.header().state_hash) - )); - } - } - Err(err) => { - return Err(anyhow!("vm_err: {:?}", err)); - } - }; + // Ensure the `event_bloom` and `state_root` returned + // from the VST call are the + // ones we expect to have with the + // current candidate block. + if output.event_bloom != candidate.header().event_bloom { + return Err(VstError::MismatchEventBloom( + Box::new(output.event_bloom), + Box::new(candidate.header().event_bloom), + )); + } + + if output.state_root != candidate.header().state_hash { + return Err(VstError::MismatchStateHash( + output.state_root, + candidate.header().state_hash, + )); + } Ok(()) } From 4dad39daaf3499ed8455b4cc3a828ab3488dc532 Mon Sep 17 00:00:00 2001 From: Herr Seppia Date: Wed, 18 Dec 2024 16:10:19 +0100 Subject: [PATCH 5/6] node: remove anyhow from VST --- node/src/chain/consensus.rs | 7 ++++--- node/src/vm.rs | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/node/src/chain/consensus.rs b/node/src/chain/consensus.rs index 13727e393..e4e6eee57 100644 --- a/node/src/chain/consensus.rs +++ b/node/src/chain/consensus.rs @@ -10,7 +10,9 @@ use std::time::Duration; use async_trait::async_trait; use dusk_consensus::commons::{RoundUpdate, TimeoutSet}; use dusk_consensus::consensus::Consensus; -use dusk_consensus::errors::{ConsensusError, HeaderError, OperationError}; +use dusk_consensus::errors::{ + ConsensusError, HeaderError, OperationError, VstError, +}; use dusk_consensus::operations::{ CallParams, Operations, Output, VerificationOutput, Voter, }; @@ -318,13 +320,12 @@ impl Operations for Executor { prev_root: [u8; 32], blk: &Block, voters: &[Voter], - ) -> Result { + ) -> Result { info!("verifying state"); let vm = self.vm.read().await; vm.verify_state_transition(prev_root, blk, voters) - .map_err(OperationError::InvalidVST) } async fn execute_state_transition( diff --git a/node/src/vm.rs b/node/src/vm.rs index f43899a82..c999335d1 100644 --- a/node/src/vm.rs +++ b/node/src/vm.rs @@ -4,6 +4,7 @@ // // Copyright (c) DUSK NETWORK. All rights reserved. +use dusk_consensus::errors::VstError; use dusk_consensus::operations::{CallParams, VerificationOutput, Voter}; use dusk_consensus::user::provisioners::Provisioners; use dusk_consensus::user::stake::Stake; @@ -32,7 +33,7 @@ pub trait VMExecution: Send + Sync + 'static { prev_root: [u8; 32], blk: &Block, voters: &[Voter], - ) -> anyhow::Result; + ) -> Result; fn accept( &self, From 169ae695d9496e9ca9bde88687db05662fb23f65 Mon Sep 17 00:00:00 2001 From: Herr Seppia Date: Wed, 18 Dec 2024 16:11:25 +0100 Subject: [PATCH 6/6] rusk: change session opening to verify TIP instead of open old commits --- rusk/src/lib/error.rs | 5 +++++ rusk/src/lib/node/rusk.rs | 5 ++++- rusk/src/lib/node/vm.rs | 16 ++++++++++++---- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/rusk/src/lib/error.rs b/rusk/src/lib/error.rs index 39ea449b8..fdacc6625 100644 --- a/rusk/src/lib/error.rs +++ b/rusk/src/lib/error.rs @@ -58,6 +58,8 @@ pub enum Error { InvalidCreditsCount(u64, usize), /// Memo too large MemoTooLarge(usize), + /// Chain tip different from the expected one + TipChanged, } impl std::error::Error for Error {} @@ -182,6 +184,9 @@ impl fmt::Display for Error { Error::MemoTooLarge(size) => { write!(f, "The memo size {size} is too large") } + Error::TipChanged => { + write!(f, "Chain tip different from the expected one") + } } } } diff --git a/rusk/src/lib/node/rusk.rs b/rusk/src/lib/node/rusk.rs index bc0b72fcd..878c739dc 100644 --- a/rusk/src/lib/node/rusk.rs +++ b/rusk/src/lib/node/rusk.rs @@ -479,7 +479,10 @@ impl Rusk { block_height: u64, commit: [u8; 32], ) -> Result { - let mut session = self._session(block_height, Some(commit))?; + let mut session = self._session(block_height, None)?; + if session.root() != commit { + return Err(Error::TipChanged); + } let _: CallReceipt<()> = session .call(STAKE_CONTRACT, "before_state_transition", &(), u64::MAX) .expect("before_state_transition to success"); diff --git a/rusk/src/lib/node/vm.rs b/rusk/src/lib/node/vm.rs index 87da36518..0f60abb2d 100644 --- a/rusk/src/lib/node/vm.rs +++ b/rusk/src/lib/node/vm.rs @@ -6,6 +6,7 @@ mod query; +use dusk_consensus::errors::VstError; use node_data::events::contract::ContractEvent; use tracing::info; @@ -48,13 +49,14 @@ impl VMExecution for Rusk { prev_commit: [u8; 32], blk: &Block, voters: &[Voter], - ) -> anyhow::Result { + ) -> Result { info!("Received verify_state_transition request"); let generator = blk.header().generator_bls_pubkey; let generator = BlsPublicKey::from_slice(&generator.0) - .map_err(|e| anyhow::anyhow!("Error in from_slice {e:?}"))?; + .map_err(VstError::InvalidGenerator)?; - let slashing = Slash::from_block(blk)?; + let slashing = + Slash::from_block(blk).map_err(VstError::InvalidSlash)?; let (_, verification_output) = self .verify_transactions( @@ -67,7 +69,13 @@ impl VMExecution for Rusk { slashing, voters, ) - .map_err(|inner| anyhow::anyhow!("Cannot verify txs: {inner}!!"))?; + .map_err(|inner| { + if let crate::Error::TipChanged = inner { + VstError::TipChanged + } else { + VstError::Generic(format!("Cannot verify txs: {inner}!!")) + } + })?; Ok(verification_output) }