Skip to content

Commit

Permalink
A few small mempool fixes (kaspanet#212)
Browse files Browse the repository at this point in the history
* use `request.allow_orphan` when passing the rpc tx to the mempool

* minor

* include the original accepted transaction as well

* for now keep the original timestamp deviation tolerance
  • Loading branch information
michaelsutton authored Jun 25, 2023
1 parent 8e56e3e commit 770e489
Show file tree
Hide file tree
Showing 11 changed files with 41 additions and 28 deletions.
3 changes: 2 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 1 addition & 7 deletions consensus/core/src/config/bps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,7 @@ impl<const BPS: u64> Bps<BPS> {
}

pub const fn pruning_proof_m() -> u64 {
// Since the important levels remain logarithmically long, it seems that this
// constant does not need to scale with BPS.
// TODO: finalize this
// No need to scale this constant with BPS since the important block levels (higher) remain logarithmically long
PRUNING_PROOF_M
}

Expand Down Expand Up @@ -120,10 +118,6 @@ impl<const BPS: u64> Bps<BPS> {
pub const fn pre_deflationary_phase_base_subsidy() -> u64 {
50000000000 / BPS
}

// TODO: we might need to increase max_block_level (at least for mainnet) as a function of BPS
// since higher BPS means easier difficulty puzzles -> less zeros in pow hash
// pub const fn max_block_level() -> u64 { }
}

#[cfg(test)]
Expand Down
4 changes: 2 additions & 2 deletions consensus/core/src/config/constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ pub mod consensus {
pub const LEGACY_TIMESTAMP_DEVIATION_TOLERANCE: u64 = 132;

/// **New** timestamp deviation tolerance (seconds).
/// KIP-0004: 605 (~10 minutes)
pub const NEW_TIMESTAMP_DEVIATION_TOLERANCE: u64 = 605;
/// TODO: KIP-0004: 605 (~10 minutes)
pub const NEW_TIMESTAMP_DEVIATION_TOLERANCE: u64 = 132;

/// The desired interval between samples of the median time window (seconds).
/// KIP-0004: 10 seconds
Expand Down
1 change: 1 addition & 0 deletions mining/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ kaspa-txscript.workspace = true
kaspa-core.workspace = true
kaspa-mining-errors.workspace = true
kaspa-consensusmanager.workspace = true
kaspa-utils.workspace = true
thiserror.workspace = true
serde.workspace = true
log.workspace = true
Expand Down
10 changes: 5 additions & 5 deletions mining/src/manager_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -476,9 +476,9 @@ mod tests {
let unorphaned_txs = result.unwrap();
let (populated_txs, orphans) = mining_manager.get_all_transactions(true, true);
assert_eq!(
unorphaned_txs.len(), SKIPPED_TXS,
unorphaned_txs.len(), SKIPPED_TXS + 1,
"the mempool is expected to have unorphaned the remaining child transaction after the matching parent transaction was inserted into the mempool: expected: {}, got: {}",
unorphaned_txs.len(), SKIPPED_TXS
SKIPPED_TXS + 1, unorphaned_txs.len()
);
assert_eq!(
SKIPPED_TXS + SKIPPED_TXS,
Expand Down Expand Up @@ -632,13 +632,13 @@ mod tests {
let unorphaned_txs = result.as_ref().unwrap();
assert_eq!(
test.should_unorphan,
!unorphaned_txs.is_empty(),
unorphaned_txs.len() > 1,
"{}: child transaction should have been {} the orphan pool",
test.name,
test.parent_insert_result()
);
if !unorphaned_txs.is_empty() {
assert_eq!(unorphaned_txs[0].id(), child_txs[i].id(), "the unorphaned transaction should match the inserted parent");
if unorphaned_txs.len() > 1 {
assert_eq!(unorphaned_txs[1].id(), child_txs[i].id(), "the unorphaned transaction should match the inserted parent");
}
}
}
Expand Down
17 changes: 8 additions & 9 deletions mining/src/mempool/validate_and_insert_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use kaspa_consensus_core::{
tx::{MutableTransaction, Transaction, TransactionId, TransactionOutpoint, UtxoEntry},
};
use kaspa_core::info;
use kaspa_utils::vec::VecExtensions;

use super::tx::{Orphan, Priority};

Expand Down Expand Up @@ -60,8 +61,10 @@ impl Mempool {
// transaction reference and mutably for the call to process_orphans_after_accepted_transaction
let accepted_transaction =
self.transaction_pool.add_transaction(transaction, consensus.get_virtual_daa_score(), priority)?.mtx.tx.clone();
let accepted_orphans = self.process_orphans_after_accepted_transaction(consensus, &accepted_transaction)?;
Ok(accepted_orphans)
let mut accepted_transactions = self.process_orphans_after_accepted_transaction(consensus, &accepted_transaction)?;
// We include the original accepted transaction as well
accepted_transactions.swap_insert(0, accepted_transaction);
Ok(accepted_transactions)
}

fn validate_transaction_pre_utxo_entry(&self, transaction: &MutableTransaction) -> RuleResult<()> {
Expand Down Expand Up @@ -98,16 +101,12 @@ impl Mempool {
) -> RuleResult<Vec<Arc<Transaction>>> {
// Rust rewrite:
// - The function is relocated from OrphanPool into Mempool
let mut added_transactions = Vec::new();
let mut unorphaned_transactions =
self.get_unorphaned_transactions_after_accepted_transaction(consensus, accepted_transaction)?;
while !unorphaned_transactions.is_empty() {
let transaction = unorphaned_transactions.pop().unwrap();

let unorphaned_transactions = self.get_unorphaned_transactions_after_accepted_transaction(consensus, accepted_transaction)?;
let mut added_transactions = Vec::with_capacity(unorphaned_transactions.len() + 1); // +1 since some callers add the accepted tx itself
for transaction in unorphaned_transactions {
// The returned transactions are leaving the mempool but must also be added to
// the transaction pool so we clone.
added_transactions.push(transaction.mtx.tx.clone());

self.transaction_pool.add_mempool_transaction(transaction)?;
}
Ok(added_transactions)
Expand Down
7 changes: 6 additions & 1 deletion protocol/flows/src/flow_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,12 @@ impl FlowContext {
// TODO: call a handler function or a predefined registered service
}

pub async fn add_transaction(
/// Adds the rpc-submitted transaction to the mempool and propagates it to peers.
///
/// Transactions submitted through rpc are considered high priority. This definition does not affect the tx selection algorithm
/// but only changes how we manage the lifetime of the tx. A high-priority tx does not expire and is repeatedly rebroadcasted to
/// peers
pub async fn submit_rpc_transaction(
&self,
consensus: &ConsensusProxy,
transaction: Transaction,
Expand Down
2 changes: 1 addition & 1 deletion protocol/p2p/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ path = "./src/bin/server.rs"
[dependencies]
kaspa-core.workspace = true
kaspa-consensus-core.workspace = true
kaspa-mining.workspace = true
kaspa-mining-errors.workspace = true
kaspa-hashes.workspace = true
kaspa-math.workspace = true
kaspa-utils.workspace = true
Expand Down
2 changes: 1 addition & 1 deletion protocol/p2p/src/common.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{convert::error::ConversionError, core::peer::PeerKey, KaspadMessagePayloadType};
use kaspa_consensus_core::errors::{block::RuleError, consensus::ConsensusError, pruning::PruningImportError};
use kaspa_mining::errors::MiningManagerError;
use kaspa_mining_errors::manager::MiningManagerError;
use std::time::Duration;
use thiserror::Error;

Expand Down
6 changes: 5 additions & 1 deletion rpc/service/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,11 @@ impl RpcApi for RpcCoreService {
let transaction: Transaction = (&request.transaction).try_into()?;
let transaction_id = transaction.id();
let session = self.consensus_manager.consensus().session().await;
self.flow_context.add_transaction(&session, transaction, Orphan::Allowed).await.map_err(|err| {
let orphan = match request.allow_orphan {
true => Orphan::Allowed,
false => Orphan::Forbidden,
};
self.flow_context.submit_rpc_transaction(&session, transaction, orphan).await.map_err(|err| {
let err = RpcError::RejectedTransaction(transaction_id, err.to_string());
debug!("{err}");
err
Expand Down
9 changes: 9 additions & 0 deletions utils/src/vec.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
pub trait VecExtensions<T> {
/// Pushes the provided value to the container if the container is empty
fn push_if_empty(self, value: T) -> Self;

/// Inserts the provided `value` at `index` while swapping the item at index to the end of the container
fn swap_insert(&mut self, index: usize, value: T);
}

impl<T> VecExtensions<T> for Vec<T> {
Expand All @@ -10,4 +13,10 @@ impl<T> VecExtensions<T> for Vec<T> {
}
self
}

fn swap_insert(&mut self, index: usize, value: T) {
self.push(value);
let loc = self.len() - 1;
self.swap(index, loc);
}
}

0 comments on commit 770e489

Please sign in to comment.