Skip to content

Commit

Permalink
chore: improve error message
Browse files Browse the repository at this point in the history
  • Loading branch information
zhangsoledad committed Mar 23, 2023
1 parent 268c375 commit 7c4b96c
Show file tree
Hide file tree
Showing 9 changed files with 28 additions and 17 deletions.
2 changes: 1 addition & 1 deletion resource/ckb.toml
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ enable_deprecated_rpc = false # {{

[tx_pool]
max_tx_pool_size = 180_000_000 # 180mb
min_fee_rate = 1_000 # shannons/KB
min_fee_rate = 1_000 # Here fee_rate are calculated directly using size in units of shannons/KB
max_tx_verify_cycles = 70_000_000
max_ancestors_count = 25

Expand Down
9 changes: 6 additions & 3 deletions rpc/src/tests/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ fn test_submit_transaction_error() {
let min_fee_rate = FeeRate::from_u64(500);
let reject = Reject::LowFeeRate(min_fee_rate, 100, 50);
assert_eq!(
"PoolRejectedTransactionByMinFeeRate: The min fee rate is 500 shannons/KB, so the transaction fee should be 100 shannons at least, but only got 50",
"PoolRejectedTransactionByMinFeeRate: The min fee rate is 500 shannons/KW, so the transaction fee should be 100 shannons at least, but only got 50",
RPCError::from_submit_transaction_reject(&reject).message
);

Expand All @@ -32,9 +32,12 @@ fn test_submit_transaction_error() {
RPCError::from_submit_transaction_reject(&reject).message
);

let reject = Reject::Full("size".to_owned());
let reject = Reject::Full(format!(
"the fee_rate for this transaction is: {}",
FeeRate::from_u64(500)
));
assert_eq!(
"PoolIsFull: Transaction are replaced because the pool is full size",
"PoolIsFull: Transaction are replaced because the pool is full, the fee_rate for this transaction is: 500 shannons/KW",
RPCError::from_submit_transaction_reject(&reject).message
);

Expand Down
2 changes: 1 addition & 1 deletion test/src/specs/relay/transaction_relay_low_fee_rate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ impl Spec for TransactionRelayLowFeeRate {
node1,
0,
10,
"reject tx The min fee rate is 1000 shannons/KB, so the transaction fee should be 242 shannons at least, but only got 0"
"reject tx The min fee rate is 1000 shannons/KW, so the transaction fee should be 242 shannons at least, but only got 0"
)
.is_some());
}
Expand Down
3 changes: 1 addition & 2 deletions test/template/ckb.toml
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,7 @@ enable_deprecated_rpc = true

[tx_pool]
max_tx_pool_size = 180_000_000 # 180mb
max_cycles = 200_000_000_000
min_fee_rate = 0 # shannons/KB
min_fee_rate = 0 # Here fee_rate are calculated directly using size in units of shannons/KB
max_tx_verify_cycles = 70_000_000
max_ancestors_count = 25

Expand Down
16 changes: 11 additions & 5 deletions tx-pool/src/component/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,12 @@ impl TxEntry {
EvictKey::from(self)
}

/// Returns fee rate
pub fn fee_rate(&self) -> FeeRate {
let weight = get_transaction_weight(self.size, self.cycles);
FeeRate::calculate(self.fee, weight)
}

/// Update ancestor state for add an entry
pub fn add_entry_weight(&mut self, entry: &TxEntry) {
self.ancestors_count = self.ancestors_count.saturating_add(1);
Expand Down Expand Up @@ -183,9 +189,10 @@ impl Ord for TxEntry {
}

/// Currently we do not have trace descendants,
/// so we take the simplest strategy to evict the transactions by first comparing the fee_rate,
/// and then taking the one with the largest timestamp,
/// which also means that the fewer descendants may exist.
/// so first take the simplest strategy,
/// first compare fee_rate, select the smallest fee_rate,
/// and then select the latest timestamp, for eviction,
/// the latest timestamp which also means that the fewer descendants may exist.
#[derive(Eq, PartialEq, Clone, Debug)]
pub struct EvictKey {
fee_rate: FeeRate,
Expand All @@ -194,9 +201,8 @@ pub struct EvictKey {

impl From<&TxEntry> for EvictKey {
fn from(entry: &TxEntry) -> Self {
let weight = get_transaction_weight(entry.size, entry.cycles);
EvictKey {
fee_rate: FeeRate::calculate(entry.fee, weight),
fee_rate: entry.fee_rate(),
timestamp: entry.timestamp,
}
}
Expand Down
5 changes: 4 additions & 1 deletion tx-pool/src/pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,10 @@ macro_rules! evict_for_trim_size {
"removed by size limit {} timestamp({})",
tx_hash, entry.timestamp
);
let reject = Reject::Full(format!("fee: {}", entry.fee));
let reject = Reject::Full(format!(
"the fee_rate for this transaction is: {}",
entry.fee_rate()
));
$callbacks.call_reject($self, &entry, reject);
}
}
Expand Down
2 changes: 1 addition & 1 deletion tx-pool/src/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -941,7 +941,7 @@ fn _update_tx_pool_for_reorg(
// Remove expired transaction from pending
tx_pool.remove_expired(callbacks);

// Remove transactions from the pool until its size < size_limit.
// Remove transactions from the pool until its size <= size_limit.
tx_pool.limit_size(callbacks);
}

Expand Down
2 changes: 1 addition & 1 deletion util/types/src/core/fee_rate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,6 @@ impl FeeRate {

impl ::std::fmt::Display for FeeRate {
fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
write!(f, "{}", self.0)
write!(f, "{} shannons/KW", self.0)
}
}
4 changes: 2 additions & 2 deletions util/types/src/core/tx_pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use std::collections::HashMap;
#[derive(Error, Debug, Clone)]
pub enum Reject {
/// Transaction fee lower than config
#[error("The min fee rate is {0} shannons/KB, so the transaction fee should be {1} shannons at least, but only got {2}")]
#[error("The min fee rate is {0}, so the transaction fee should be {1} shannons at least, but only got {2}")]
LowFeeRate(FeeRate, u64, u64),

/// Transaction exceeded maximum ancestors count limit
Expand All @@ -26,7 +26,7 @@ pub enum Reject {
ExceededTransactionSizeLimit(u64, u64),

/// Transaction are replaced because the pool is full
#[error("Transaction are replaced because the pool is full {0}")]
#[error("Transaction are replaced because the pool is full, {0}")]
Full(String),

/// Transaction already exist in transaction_pool
Expand Down

0 comments on commit 7c4b96c

Please sign in to comment.