Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce duplicated stake and unstake validation code #2588

Closed
wants to merge 5 commits into from
Closed

Reduce duplicated stake and unstake validation code #2588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d61cb0b
feat(node): reduce duplicated stake and unstake validation code by re…
drcpu-github Jan 30, 2025
fe9754f
feat(json_rpc): include stake and unstake transactions in GetMempool
drcpu-github Jan 30, 2025
ac23127
feat(node): periodically revalidate pending stake and unstake transac…
drcpu-github Jan 30, 2025
b0e5e3e
fix(jsonrpc): add stake weights and hashes to block starting at V1_8
drcpu-github Jan 31, 2025
dda3e36
fix(jsonrpc): use versioned hash to query block confirmation status
drcpu-github Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion data_structures/src/chain/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3190,7 +3190,7 @@ impl TransactionsPool {
}

/// Remove stake transactions that would result in overstaking on a validator
pub fn remove_overstake_transactions(&mut self, transactions: Vec<Hash>) {
pub fn remove_invalid_stake_transactions(&mut self, transactions: Vec<Hash>) {
for st_tx_hash in transactions.iter() {
if let Some(st_tx) = self
.st_transactions
Expand Down
90 changes: 86 additions & 4 deletions node/src/actors/chain_manager/handlers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,13 @@ use witnet_data_structures::{
},
transaction_factory::{self, NodeBalance},
types::LastBeacon,
utxo_pool::{get_utxo_info, UtxoInfo},
utxo_pool::{get_utxo_info, UtxoDiff, UtxoInfo},
};
use witnet_util::timestamp::get_timestamp;
use witnet_validations::validations::{block_reward, total_block_reward, validate_rad_request};
use witnet_validations::validations::{
block_reward, total_block_reward, validate_rad_request, validate_stake_transaction,
validate_unstake_transaction,
};

use crate::{
actors::{
Expand Down Expand Up @@ -232,10 +235,87 @@ impl Handler<EpochNotification<EveryEpochPayload>> for ChainManager {
self.candidates.clear();
self.seen_candidates.clear();

// Periodically revalidate pending stake transactions since they can become invalid
if get_protocol_version(Some(current_epoch)) >= ProtocolVersion::V1_8
&& current_epoch % 10 == 0
{
let utxo_diff = UtxoDiff::new(
&self.chain_state.unspent_outputs_pool,
self.chain_state.block_number(),
);
let min_stake = self
.consensus_constants_wit2
.get_validator_min_stake_nanowits(current_epoch);
let max_stake = self
.consensus_constants_wit2
.get_validator_max_stake_nanowits(current_epoch);

let mut invalid_stake_transactions = Vec::<Hash>::new();
for st_tx in self.transactions_pool.st_iter() {
if let Err(e) = validate_stake_transaction(
st_tx,
&utxo_diff,
current_epoch,
self.epoch_constants.unwrap(),
&mut vec![],
&self.chain_state.stakes,
min_stake,
max_stake,
) {
log::debug!(
"Removing stake transaction {} as it became invalid: {}",
st_tx.hash(),
e
);
invalid_stake_transactions.push(st_tx.hash());
continue;
}
}

self.transactions_pool
.remove_invalid_stake_transactions(invalid_stake_transactions);
}

// Periodically revalidate pending unstake transactions since they can become invalid
if get_protocol_version(Some(current_epoch)) >= ProtocolVersion::V2_0
&& current_epoch % 10 == 0
{
let min_stake = self
.consensus_constants_wit2
.get_validator_min_stake_nanowits(current_epoch);
let unstake_delay = self
.consensus_constants_wit2
.get_unstaking_delay_seconds(current_epoch);

let mut invalid_unstake_transactions = Vec::<Hash>::new();
for ut_tx in self.transactions_pool.ut_iter() {
if let Err(e) = validate_unstake_transaction(
ut_tx,
current_epoch,
&self.chain_state.stakes,
min_stake,
unstake_delay,
) {
log::debug!(
"Removing unstake transaction {} as it became invalid: {}",
ut_tx.hash(),
e
);
invalid_unstake_transactions.push(ut_tx.hash());
continue;
}
}

self.transactions_pool
.remove_invalid_unstake_transactions(invalid_unstake_transactions);
}

log::debug!(
"Transactions pool size: {} value transfer, {} data request",
"Transactions pool size: {} value transfer, {} data request, {} stake, {} unstake",
self.transactions_pool.vt_len(),
self.transactions_pool.dr_len()
self.transactions_pool.dr_len(),
self.transactions_pool.st_len(),
self.transactions_pool.ut_len()
);
}

Expand Down Expand Up @@ -1996,6 +2076,8 @@ impl Handler<GetMempool> for ChainManager {
let res = GetMempoolResult {
value_transfer: self.transactions_pool.vt_iter().map(|t| t.hash()).collect(),
data_request: self.transactions_pool.dr_iter().map(|t| t.hash()).collect(),
stake: self.transactions_pool.st_iter().map(|t| t.hash()).collect(),
unstake: self.transactions_pool.ut_iter().map(|t| t.hash()).collect(),
};

Ok(res)
Expand Down
137 changes: 39 additions & 98 deletions node/src/actors/chain_manager/mining.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ use witnet_data_structures::{
radon_error::RadonError,
radon_report::{RadonReport, ReportContext, TypeLike},
staking::{
helpers::StakeKey,
stake::totalize_stakes,
stakes::{QueryStakesKey, StakesTracker},
},
Expand Down Expand Up @@ -63,7 +62,8 @@ use witnet_validations::{
validations::{
block_reward, calculate_liars_and_errors_count_from_tally, dr_transaction_fee,
merkle_tree_root, run_tally, st_transaction_fee, tally_bytes_on_encode_error,
update_utxo_diff, vt_transaction_fee,
update_utxo_diff, validate_stake_transaction, validate_unstake_transaction,
vt_transaction_fee,
},
};

Expand Down Expand Up @@ -1180,7 +1180,7 @@ pub fn build_block(

if protocol_version > V1_7 {
let max_st_weight = consensus_constants_wit2.get_maximum_stake_block_weight(epoch);
let mut overstake_transactions = Vec::<Hash>::new();
let mut invalid_stake_transactions = Vec::<Hash>::new();
let mut included_validators = HashSet::<PublicKeyHash>::new();
for st_tx in transactions_pool.st_iter() {
let validator_pkh = st_tx.body.output.authorization.public_key.pkh();
Expand All @@ -1192,37 +1192,28 @@ pub fn build_block(
continue;
}

// If a set of staking transactions is sent simultaneously to the transactions pool using a staking amount smaller
// than 'max_stake' they can all be accepted since they do not introduce overstaking yet. However, accepting all of
// them in subsequent blocks could violate the 'max_stake' rule. Thus we still need to check that we do not include
// all these staking transactions in a block so we do not produce an invalid block.
let stakes_key = QueryStakesKey::Key(StakeKey {
validator: st_tx.body.output.key.validator,
withdrawer: st_tx.body.output.key.withdrawer,
});
// Revalidate stake transaction since concurrent stake transactions could have invalidated the transaction
// we want to include here which would result in producing an invalid block.
let min_stake = consensus_constants_wit2.get_validator_min_stake_nanowits(epoch);
let max_stake = consensus_constants_wit2.get_validator_max_stake_nanowits(epoch);
match stakes.query_stakes(stakes_key) {
Ok(stake_entry) => {
// TODO: modify this to enable delegated staking with multiple withdrawer addresses on a single validator
let staked_amount: u64 = stake_entry
.first()
.map(|stake| stake.value.coins)
.unwrap()
.into();
if st_tx.body.output.value + staked_amount > max_stake {
overstake_transactions.push(st_tx.hash());
continue;
}
}
Err(_) => {
// This should never happen since a staking transaction to a non-existing (validator, withdrawer) pair
// with a value higher than 'max_stake' should not have been accepted in the transactions pool.
if st_tx.body.output.value > max_stake {
overstake_transactions.push(st_tx.hash());
continue;
}
}
};
if let Err(e) = validate_stake_transaction(
st_tx,
&utxo_diff,
epoch,
epoch_constants,
&mut vec![],
stakes,
min_stake,
max_stake,
) {
log::warn!(
"Cannot build block with stake transaction {}: {}",
st_tx.hash(),
e
);
invalid_stake_transactions.push(st_tx.hash());
continue;
}

let transaction_weight = st_tx.weight();
let transaction_fee =
Expand Down Expand Up @@ -1262,7 +1253,7 @@ pub fn build_block(
included_validators.insert(validator_pkh);
}

transactions_pool.remove_overstake_transactions(overstake_transactions);
transactions_pool.remove_invalid_stake_transactions(invalid_stake_transactions);
} else {
transactions_pool.clear_stake_transactions();
}
Expand All @@ -1281,71 +1272,21 @@ pub fn build_block(
continue;
}

// If a set of unstaking transactions is sent simultaneously to the transactions pool using an amount which leaves
// more than 'min_stake' staked they can all be accepted since they do not introduce understaking yet. However,
// accepting all of them in subsequent blocks could violate the 'min_stake' rule. Thus we still need to check that
// we do not include all these unstaking transactions in a block so we do not produce an invalid block.
let stakes_key = QueryStakesKey::Key(StakeKey {
validator: ut_tx.body.operator,
withdrawer: ut_tx.body.withdrawal.pkh,
});
// Revalidate unstake transaction since concurrent unstake transactions could have invalidated the transaction
// we want to include here which would result in producing an invalid block.
let min_stake = consensus_constants_wit2.get_validator_min_stake_nanowits(epoch);
match stakes.query_stakes(stakes_key) {
Ok(stake_entry) => {
// TODO: modify this to enable delegated staking with multiple withdrawer addresses on a single validator
let staked_amount: u64 = stake_entry
.first()
.map(|stake| stake.value.coins)
.unwrap()
.into();
if staked_amount - ut_tx.body.withdrawal.value - ut_tx.body.fee < min_stake {
log::info!(
"Unstaking with {} would result in understaking",
ut_tx.hash()
);
invalid_unstake_transactions.push(ut_tx.hash());
continue;
}
}
Err(_) => {
// Not finding a stake entry is possible if there are two concurrent unstake transactions where at least
// one of them unstakes all balance before the second one is included in a block. In that case, remove
// the latter one from our transaction pool.
log::info!("Cannot process unstake transaction {} since the full balance was already unstaked",
ut_tx.hash(),
);
invalid_unstake_transactions.push(ut_tx.hash());
continue;
}
};

// Double check the nonce before building a block. A nonce that was valid when the transaction was received
// and inserted into the transaction pool once validated, may not be anymore when we build the actual block
// due to concurrent transactions.
let key = StakeKey {
validator: ut_tx.body.operator,
withdrawer: ut_tx.body.withdrawal.pkh,
};
match stakes.query_nonce(key) {
Ok(nonce) => {
if ut_tx.body.nonce != nonce {
log::info!("Cannot process unstake transaction {} since the nonce does not match: {} != {}",
ut_tx.hash(),
ut_tx.body.nonce,
nonce,
);
invalid_unstake_transactions.push(ut_tx.hash());
continue;
}
}
Err(_) => {
log::info!("Cannot process unstake transaction {} since the associated stake entry does not exist anymore",
ut_tx.hash()
);
invalid_unstake_transactions.push(ut_tx.hash());
continue;
}
};
let unstake_delay = consensus_constants_wit2.get_unstaking_delay_seconds(epoch);
if let Err(e) =
validate_unstake_transaction(ut_tx, epoch, stakes, min_stake, unstake_delay)
{
log::warn!(
"Cannot build block with unstake transaction {}: {}",
ut_tx.hash(),
e
);
invalid_unstake_transactions.push(ut_tx.hash());
continue;
}

let transaction_weight = ut_tx.weight();
let new_ut_weight = ut_weight.saturating_add(transaction_weight);
Expand Down
14 changes: 9 additions & 5 deletions node/src/actors/json_rpc/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ use witnet_data_structures::{
tapi::ActiveWips, Block, DataRequestOutput, Epoch, Hash, Hashable, KeyedSignature,
PublicKeyHash, RADType, StakeOutput, StateMachine, SyncStatus,
},
get_environment,
proto::versioning::ProtocolVersion,
get_environment, get_protocol_version,
proto::versioning::{ProtocolVersion, VersionedHashable},
staking::prelude::*,
transaction::Transaction,
vrf::VrfMessage,
Expand Down Expand Up @@ -742,7 +742,7 @@ pub async fn get_block(params: Params) -> Result<Value, Error> {
match res {
Ok(Ok(mut output)) => {
let block_epoch = output.block_header.beacon.checkpoint;
let block_hash = output.hash();
let block_hash = output.versioned_hash(get_protocol_version(Some(block_epoch)));

let dr_weight = match serde_json::to_value(output.dr_weight()) {
Ok(x) => x,
Expand Down Expand Up @@ -812,7 +812,7 @@ pub async fn get_block(params: Params) -> Result<Value, Error> {
"tally" : tt_hashes
}));

if ProtocolVersion::from_epoch(block_epoch) == ProtocolVersion::V2_0 {
if ProtocolVersion::from_epoch(block_epoch) >= ProtocolVersion::V1_8 {
let st_hashes: Vec<_> = output
.txns
.stake_txns
Expand All @@ -825,7 +825,9 @@ pub async fn get_block(params: Params) -> Result<Value, Error> {
.expect("The result of getBlock should be an object")
.insert("stake".to_string(), serde_json::json!(st_hashes));
}
}

if ProtocolVersion::from_epoch(block_epoch) == ProtocolVersion::V2_0 {
let ut_hashes: Vec<_> = output
.txns
.unstake_txns
Expand Down Expand Up @@ -865,7 +867,7 @@ pub async fn get_block(params: Params) -> Result<Value, Error> {
"data_request": drt_weights,
}));

if ProtocolVersion::from_epoch(block_epoch) == ProtocolVersion::V2_0 {
if ProtocolVersion::from_epoch(block_epoch) >= ProtocolVersion::V1_8 {
let st_weights: Vec<_> = output
.txns
.stake_txns
Expand All @@ -878,7 +880,9 @@ pub async fn get_block(params: Params) -> Result<Value, Error> {
.expect("The result of getBlock should be an object")
.insert("stake".to_string(), st_weights.into());
}
}

if ProtocolVersion::from_epoch(block_epoch) >= ProtocolVersion::V2_0 {
let ut_weights: Vec<_> = output
.txns
.unstake_txns
Expand Down
4 changes: 4 additions & 0 deletions node/src/actors/messages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,10 @@ pub struct GetMempoolResult {
pub value_transfer: Vec<Hash>,
/// Pending data request transactions
pub data_request: Vec<Hash>,
/// Pending stake transactions
pub stake: Vec<Hash>,
/// Pending unstake transactions
pub unstake: Vec<Hash>,
}

/// Try to mine a block: signal the ChainManager to check if it can produce a new block
Expand Down