Skip to content

Commit

Permalink
[types] remove useless AccountSequenceInfo
Browse files Browse the repository at this point in the history 
This was probably back when we were exploring different types of
sequence numbers. If we ever get to this again, it is years away. The
effort to roll out a new sequence number is blocked by:
* Porting accounts into objects / resource groups
* Producing new mempool
* New transaction types
* Motivation to do so
  • Loading branch information
@davidiw
davidiw committed Feb 28, 2023
1 parent 66a74b1 commit d3ae89c
Show file tree
Hide file tree
Showing 12 changed files with 76 additions and 158 deletions.
6 changes: 2 additions & 4 deletions mempool/src/core_mempool/mempool.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ use aptos_crypto::HashValue;
use aptos_logger::prelude::*;
use aptos_types::{
account_address::AccountAddress,
account_config::AccountSequenceInfo,
mempool_status::{MempoolStatus, MempoolStatusCode},
transaction::SignedTransaction,
};
Expand Down Expand Up @@ -116,10 +115,9 @@ impl Mempool {
&mut self,
txn: SignedTransaction,
ranking_score: u64,
sequence_info: AccountSequenceInfo,
db_sequence_number: u64,
timeline_state: TimelineState,
) -> MempoolStatus {
let db_sequence_number = sequence_info.min_seq();
trace!(
LogSchema::new(LogEntry::AddTxn)
.txns(TxnsLog::new_txn(txn.sender(), txn.sequence_number())),
Expand All @@ -144,7 +142,7 @@ impl Mempool {
expiration_time,
ranking_score,
timeline_state,
AccountSequenceInfo::Sequential(db_sequence_number),
db_sequence_number,
now,
);

Expand Down
14 changes: 5 additions & 9 deletions mempool/src/core_mempool/transaction.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@

use crate::core_mempool::TXN_INDEX_ESTIMATED_BYTES;
use aptos_crypto::HashValue;
use aptos_types::{
account_address::AccountAddress, account_config::AccountSequenceInfo,
transaction::SignedTransaction,
};
use aptos_types::{account_address::AccountAddress, transaction::SignedTransaction};
use serde::{Deserialize, Serialize};
use std::{
mem::size_of,
Expand All @@ -34,13 +31,13 @@ impl MempoolTransaction {
expiration_time: Duration,
ranking_score: u64,
timeline_state: TimelineState,
seqno_type: AccountSequenceInfo,
seqno: u64,
insertion_time: SystemTime,
) -> Self {
Self {
sequence_info: SequenceInfo {
transaction_sequence_number: txn.sequence_number(),
account_sequence_number_type: seqno_type,
account_sequence_number: seqno,
},
txn,
expiration_time,
Expand Down Expand Up @@ -82,7 +79,7 @@ pub enum TimelineState {
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
pub struct SequenceInfo {
pub transaction_sequence_number: u64,
pub account_sequence_number_type: AccountSequenceInfo,
pub account_sequence_number: u64,
}

#[cfg(test)]
Expand All @@ -91,7 +88,6 @@ mod test {
use aptos_crypto::{ed25519::Ed25519PrivateKey, PrivateKey, SigningKey, Uniform};
use aptos_types::{
account_address::AccountAddress,
account_config::AccountSequenceInfo,
chain_id::ChainId,
transaction::{RawTransaction, Script, SignedTransaction, TransactionPayload},
};
Expand All @@ -113,7 +109,7 @@ mod test {
Duration::from_secs(1),
1,
TimelineState::NotReady,
AccountSequenceInfo::Sequential(0),
0,
SystemTime::now(),
)
}
Expand Down
102 changes: 39 additions & 63 deletions mempool/src/core_mempool/transaction_store.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ use aptos_crypto::HashValue;
use aptos_logger::{prelude::*, Level};
use aptos_types::{
account_address::AccountAddress,
account_config::AccountSequenceInfo,
mempool_status::{MempoolStatus, MempoolStatusCode},
transaction::SignedTransaction,
};
Expand Down Expand Up @@ -188,17 +187,16 @@ impl TransactionStore {
/// Insert transaction into TransactionStore. Performs validation checks and updates indexes.
pub(crate) fn insert(&mut self, txn: MempoolTransaction) -> MempoolStatus {
let address = txn.get_sender();
let sequence_number = txn.sequence_info;
let txn_seq_num = txn.sequence_info.transaction_sequence_number;
let acc_seq_num = txn.sequence_info.account_sequence_number;

// If the transaction is already in Mempool, we only allow the user to
// increase the gas unit price to speed up a transaction, but not the max gas.
//
// Transactions with all the same inputs (but possibly signed differently) are idempotent
// since the raw transaction is the same
if let Some(txns) = self.transactions.get_mut(&address) {
if let Some(current_version) =
txns.get_mut(&sequence_number.transaction_sequence_number)
{
if let Some(current_version) = txns.get_mut(&txn_seq_num) {
if current_version.txn.payload() != txn.txn.payload() {
return MempoolStatus::new(MempoolStatusCode::InvalidUpdate).with_message(
"Transaction already in mempool with a different payload".to_string(),
Expand All @@ -217,7 +215,7 @@ impl TransactionStore {
);
} else if current_version.txn.gas_unit_price() < txn.get_gas_price() {
// Update txn if gas unit price is a larger value than before
if let Some(txn) = txns.remove(&sequence_number.transaction_sequence_number) {
if let Some(txn) = txns.remove(&txn_seq_num) {
self.index_remove(&txn);
};
} else if current_version.get_gas_price() > txn.get_gas_price() {
Expand All @@ -233,21 +231,15 @@ impl TransactionStore {
}
}

if self.check_is_full_after_eviction(
&txn,
sequence_number.account_sequence_number_type.min_seq(),
) {
if self.check_is_full_after_eviction(&txn, acc_seq_num) {
return MempoolStatus::new(MempoolStatusCode::MempoolIsFull).with_message(format!(
"Mempool is full. Mempool size: {}, Capacity: {}",
self.system_ttl_index.size(),
self.capacity,
));
}

self.clean_committed_transactions(
&address,
sequence_number.account_sequence_number_type.min_seq(),
);
self.clean_committed_transactions(&address, acc_seq_num);

self.transactions
.entry(address)
Expand All @@ -269,20 +261,15 @@ impl TransactionStore {
let sender = txn.get_sender();
self.system_ttl_index.insert(&txn);
self.expiration_time_index.insert(&txn);
self.hash_index.insert(
txn.get_committed_hash(),
(sender, sequence_number.transaction_sequence_number),
);
self.hash_index
.insert(txn.get_committed_hash(), (sender, txn_seq_num));
let txn_size_bytes = txn.get_estimated_bytes();
txns.insert(sequence_number.transaction_sequence_number, txn);
self.sequence_numbers.insert(
sender,
sequence_number.account_sequence_number_type.min_seq(),
);
txns.insert(txn_seq_num, txn);
self.sequence_numbers.insert(sender, acc_seq_num);
self.size_bytes += txn_size_bytes;
self.track_indices();
}
self.process_ready_transactions(&address, sequence_number.account_sequence_number_type);
self.process_ready_transactions(&address, acc_seq_num);
MempoolStatus::new(MempoolStatusCode::Accepted)
}

Expand Down Expand Up @@ -420,44 +407,34 @@ impl TransactionStore {
/// should be included in both the PriorityIndex (ordering for Consensus) and
/// TimelineIndex (txns for SharedMempool).
/// - Other txns are considered to be "non-ready" and should be added to ParkingLotIndex.
fn process_ready_transactions(
&mut self,
address: &AccountAddress,
sequence_info: AccountSequenceInfo,
) {
fn process_ready_transactions(&mut self, address: &AccountAddress, sequence_num: u64) {
if let Some(txns) = self.transactions.get_mut(address) {
let mut min_seq = sequence_info.min_seq();

match sequence_info {
AccountSequenceInfo::Sequential(_) => {
while let Some(txn) = txns.get_mut(&min_seq) {
let process_ready = !self.priority_index.contains(txn);
self.priority_index.insert(txn);
let mut min_seq = sequence_num;

let process_broadcast_ready = txn.timeline_state == TimelineState::NotReady;
if process_broadcast_ready {
self.timeline_index.insert(txn);
}
while let Some(txn) = txns.get_mut(&min_seq) {
let process_ready = !self.priority_index.contains(txn);
self.priority_index.insert(txn);

if process_ready {
if let Ok(time_delta) =
SystemTime::now().duration_since(txn.insertion_time)
{
Self::log_ready_transaction(
txn.ranking_score,
self.timeline_index.get_bucket(txn.ranking_score),
time_delta,
process_broadcast_ready,
);
}
}
let process_broadcast_ready = txn.timeline_state == TimelineState::NotReady;
if process_broadcast_ready {
self.timeline_index.insert(txn);
}

// Remove txn from parking lot after it has been promoted to
// priority_index / timeline_index, i.e., txn status is ready.
self.parking_lot_index.remove(txn);
min_seq += 1;
if process_ready {
if let Ok(time_delta) = SystemTime::now().duration_since(txn.insertion_time) {
Self::log_ready_transaction(
txn.ranking_score,
self.timeline_index.get_bucket(txn.ranking_score),
time_delta,
process_broadcast_ready,
);
}
},
}

// Remove txn from parking lot after it has been promoted to
// priority_index / timeline_index, i.e., txn status is ready.
self.parking_lot_index.remove(txn);
min_seq += 1;
}

let mut parking_lot_txns = 0;
Expand All @@ -470,9 +447,10 @@ impl TransactionStore {
},
}
}

trace!(
LogSchema::new(LogEntry::ProcessReadyTxns).account(*address),
first_ready_seq_num = sequence_info.min_seq(),
first_ready_seq_num = sequence_num,
last_ready_seq_num = min_seq,
num_parked_txns = parking_lot_txns,
);
Expand Down Expand Up @@ -516,11 +494,9 @@ impl TransactionStore {
/// and potential promotion of sequential txns to PriorityIndex/TimelineIndex.
pub fn commit_transaction(&mut self, account: &AccountAddress, sequence_number: u64) {
let current_seq_number = self.get_sequence_number(account).map_or(0, |v| *v);
let new_seq_number =
AccountSequenceInfo::Sequential(max(current_seq_number, sequence_number + 1));
self.sequence_numbers
.insert(*account, new_seq_number.min_seq());
self.clean_committed_transactions(account, new_seq_number.min_seq());
let new_seq_number = max(current_seq_number, sequence_number + 1);
self.sequence_numbers.insert(*account, new_seq_number);
self.clean_committed_transactions(account, new_seq_number);
self.process_ready_transactions(account, new_seq_number);
}

Expand Down
11 changes: 5 additions & 6 deletions mempool/src/shared_mempool/tasks.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ use aptos_metrics_core::HistogramTimer;
use aptos_network::application::interface::NetworkClientInterface;
use aptos_storage_interface::state_view::LatestDbStateCheckpointView;
use aptos_types::{
account_config::AccountSequenceInfo,
mempool_status::{MempoolStatus, MempoolStatusCode},
on_chain_config::{OnChainConfigPayload, OnChainConsensusConfig},
transaction::SignedTransaction,
Expand Down Expand Up @@ -284,9 +283,9 @@ where
.into_iter()
.enumerate()
.filter_map(|(idx, t)| {
if let Ok(sequence_info) = seq_numbers[idx] {
if t.sequence_number() >= sequence_info.min_seq() {
return Some((t, sequence_info));
if let Ok(sequence_num) = seq_numbers[idx] {
if t.sequence_number() >= sequence_num {
return Some((t, sequence_num));
} else {
statuses.push((
t,
Expand Down Expand Up @@ -319,7 +318,7 @@ where
/// validation into the mempool.
#[cfg(not(feature = "consensus-only-perf-test"))]
fn validate_and_add_transactions<NetworkClient, TransactionValidator>(
transactions: Vec<(SignedTransaction, AccountSequenceInfo)>,
transactions: Vec<(SignedTransaction, u64)>,
smp: &SharedMempool<NetworkClient, TransactionValidator>,
timeline_state: TimelineState,
statuses: &mut Vec<(SignedTransaction, (MempoolStatus, Option<StatusCode>))>,
Expand Down Expand Up @@ -383,7 +382,7 @@ fn validate_and_add_transactions<NetworkClient, TransactionValidator>(
/// outstanding sequence numbers.
#[cfg(feature = "consensus-only-perf-test")]
fn validate_and_add_transactions<NetworkClient, TransactionValidator>(
transactions: Vec<(SignedTransaction, AccountSequenceInfo)>,
transactions: Vec<(SignedTransaction, u64)>,
smp: &SharedMempool<NetworkClient, TransactionValidator>,
timeline_state: TimelineState,
statuses: &mut Vec<(SignedTransaction, (MempoolStatus, Option<StatusCode>))>,
Expand Down
9 changes: 4 additions & 5 deletions mempool/src/tests/common.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ use aptos_config::config::{NodeConfig, MAX_APPLICATION_MESSAGE_SIZE};
use aptos_crypto::{ed25519::Ed25519PrivateKey, PrivateKey, Uniform};
use aptos_types::{
account_address::AccountAddress,
account_config::AccountSequenceInfo,
chain_id::ChainId,
mempool_status::MempoolStatusCode,
transaction::{RawTransaction, Script, SignedTransaction},
Expand Down Expand Up @@ -50,7 +49,7 @@ pub struct TestTransaction {
pub(crate) address: usize,
pub(crate) sequence_number: u64,
pub(crate) gas_price: u64,
pub(crate) account_seqno_type: AccountSequenceInfo,
pub(crate) account_seqno: u64,
}

impl TestTransaction {
Expand All @@ -59,7 +58,7 @@ impl TestTransaction {
address,
sequence_number,
gas_price,
account_seqno_type: AccountSequenceInfo::Sequential(0),
account_seqno: 0,
}
}

Expand Down Expand Up @@ -120,7 +119,7 @@ pub(crate) fn add_txns_to_mempool(
pool.add_txn(
txn.clone(),
txn.gas_unit_price(),
transaction.account_seqno_type,
transaction.account_seqno,
TimelineState::NotReady,
);
transactions.push(txn);
Expand All @@ -137,7 +136,7 @@ pub(crate) fn add_signed_txn(pool: &mut CoreMempool, transaction: SignedTransact
.add_txn(
transaction.clone(),
transaction.gas_unit_price(),
AccountSequenceInfo::Sequential(0),
0,
TimelineState::NotReady,
)
.code
Expand Down
Loading

0 comments on commit d3ae89c

Please sign in to comment.