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.

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

Whitelist for Transaction Signers #826

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.lock

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

13 changes: 5 additions & 8 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@ resolver = "2"
members = [
"protocol-units/bridge/config",
"protocol-units/bridge/setup",
"protocol-units/execution/dof",
"protocol-units/execution/opt-executor",
"protocol-units/execution/fin-view",
"protocol-units/execution/util",
"protocol-units/execution/maptos/*",
"protocol-units/da/m1/*",
"protocol-units/sequencing/memseq/*",
"protocol-units/mempool/*",
Expand Down Expand Up @@ -73,10 +70,10 @@ m1-da-light-node-util = { path = "protocol-units/da/m1/util" }
m1-da-light-node-setup = { path = "protocol-units/da/m1/setup" }
m1-da-light-node-verifier = { path = "protocol-units/da/m1/light-node-verifier" }
## execution
maptos-dof-execution = { path = "protocol-units/execution/dof" }
maptos-opt-executor = { path = "protocol-units/execution/opt-executor" }
maptos-fin-view = { path = "protocol-units/execution/fin-view" }
maptos-execution-util = { path = "protocol-units/execution/util" }
maptos-dof-execution = { path = "protocol-units/execution/maptos/dof" }
maptos-opt-executor = { path = "protocol-units/execution/maptos/opt-executor" }
maptos-fin-view = { path = "protocol-units/execution/maptos/fin-view" }
maptos-execution-util = { path = "protocol-units/execution/maptos/util" }
## infra
movement-rest = { path = "protocol-units/movement-rest" }
## mempool
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ impl BackgroundTask {
mempool_config: &MempoolConfig,
transactions_in_flight: Arc<AtomicU64>,
transactions_in_flight_limit: u64,
) -> Self {
Self {
) -> Result<Self, anyhow::Error> {
Ok(Self {
inner: BackgroundInner::Full(TransactionPipe::new(
mempool_client_receiver,
transaction_sender,
Expand All @@ -44,8 +44,8 @@ impl BackgroundTask {
mempool_config,
transactions_in_flight,
transactions_in_flight_limit,
)),
}
)?),
})
}

pub(crate) fn read_only(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ use aptos_mempool::core_mempool::CoreMempool;
use aptos_mempool::SubmissionStatus;
use aptos_mempool::{core_mempool::TimelineState, MempoolClientRequest};
use aptos_storage_interface::{state_view::LatestDbStateCheckpointView as _, DbReader};
use aptos_types::account_address::AccountAddress;
use aptos_types::mempool_status::{MempoolStatus, MempoolStatusCode};
use aptos_types::transaction::SignedTransaction;
use aptos_types::vm_status::DiscardedVMStatus;
use aptos_vm_validator::vm_validator::{self, TransactionValidation, VMValidator};
use std::collections::HashSet;

use crate::gc_account_sequence_number::UsedSequenceNumberPool;
use futures::channel::mpsc as futures_mpsc;
Expand Down Expand Up @@ -42,6 +44,8 @@ pub struct TransactionPipe {
last_gc: Instant,
// The pool of used sequence numbers
used_sequence_number_pool: UsedSequenceNumberPool,
/// The accounts whitelisted for ingress
whitelisted_accounts: Option<HashSet<AccountAddress>>,
}

enum SequenceNumberValidity {
Expand All @@ -58,8 +62,8 @@ impl TransactionPipe {
mempool_config: &MempoolConfig,
transactions_in_flight: Arc<AtomicU64>,
transactions_in_flight_limit: u64,
) -> Self {
TransactionPipe {
) -> Result<Self, anyhow::Error> {
Ok(TransactionPipe {
mempool_client_receiver,
transaction_sender,
db_reader,
Expand All @@ -71,6 +75,14 @@ impl TransactionPipe {
mempool_config.sequence_number_ttl_ms,
mempool_config.gc_slot_duration_ms,
),
whitelisted_accounts: mempool_config.whitelisted_accounts()?,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need to be fallible? Wouldn't it be better to have Config correct on construction and whitelisted_accounts a simple getter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your second line of review answers this.

})
}

pub fn is_whitelisted(&self, address: &AccountAddress) -> Result<bool, Error> {
match &self.whitelisted_accounts {
Some(whitelisted_accounts) => Ok(whitelisted_accounts.contains(address)),
None => Ok(true),
}
}

Expand Down Expand Up @@ -182,6 +194,11 @@ impl TransactionPipe {
&mut self,
transaction: SignedTransaction,
) -> Result<SubmissionStatus, Error> {
// Check whether the account is whitelisted
if !self.is_whitelisted(&transaction.sender())? {
return Ok((MempoolStatus::new(MempoolStatusCode::TooManyTransactions), None));
}

// For now, we are going to consider a transaction in flight until it exits the mempool and is sent to the DA as is indicated by WriteBatch.
let in_flight = self.transactions_in_flight.load(std::sync::atomic::Ordering::Relaxed);
info!(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ impl Executor {
&self.config.mempool,
Arc::clone(&self.transactions_in_flight),
maptos_config.load_shedding.max_transactions_in_flight,
)
)?
};

let cx = Context::new(self.db().clone(), mempool_client_sender, maptos_config, node_config);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,6 @@ serde_derive = { workspace = true }
toml = { workspace = true }
godfig = { workspace = true }
hex = { workspace = true }
tokio = { workspace = true }

aptos-sdk = { workspace = true }
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ use std::str::FromStr;

use aptos_crypto::{ed25519::Ed25519PrivateKey, Genesis, ValidCryptoMaterialStringExt};
use aptos_types::chain_id::ChainId;
use godfig::env_default;
use godfig::{env_default, env_or_none};
use std::collections::HashSet;

// The default Maptos API listen hostname
env_default!(
Expand Down Expand Up @@ -167,3 +168,5 @@ env_default!(default_max_transactions_in_flight, "MAPTOS_MAX_TRANSACTIONS_IN_FLI
env_default!(default_sequence_number_ttl_ms, "MAPTOS_SEQUENCE_NUMBER_TTL_MS", u64, 1000 * 60 * 3);

env_default!(default_gc_slot_duration_ms, "MAPTOS_GC_SLOT_DURATION_MS", u64, 1000 * 2);

env_default!(default_ingress_account_whitelist, "MAPTOS_INGRESS_ACCOUNT_WHITELIST", String);
53 changes: 53 additions & 0 deletions protocol-units/execution/maptos/util/src/config/mempool.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
use super::common::{
default_gc_slot_duration_ms, default_ingress_account_whitelist, default_sequence_number_ttl_ms,
};
use aptos_types::account_address::AccountAddress;
use serde::{Deserialize, Serialize};
use std::collections::HashSet;

#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct Config {
/// The number of milliseconds a sequence number is valid for.
#[serde(default = "default_sequence_number_ttl_ms")]
pub sequence_number_ttl_ms: u64,

/// The duration of a garbage collection slot in milliseconds.
#[serde(default = "default_gc_slot_duration_ms")]
pub gc_slot_duration_ms: u64,

/// The whitelist (path) for the mempool
#[serde(default = "default_ingress_account_whitelist")]
pub ingress_account_whitelist: Option<String>,
}

impl Default for Config {
fn default() -> Self {
Self {
sequence_number_ttl_ms: default_sequence_number_ttl_ms(),
gc_slot_duration_ms: default_gc_slot_duration_ms(),
ingress_account_whitelist: default_ingress_account_whitelist(),
}
}
}

impl Config {
pub fn whitelisted_accounts(&self) -> Result<Option<HashSet<AccountAddress>>, anyhow::Error> {
match &self.ingress_account_whitelist {
Some(whitelist_path) => {
let mut whitelisted = HashSet::new();

// read the file from memory
let file_string = String::from_utf8(std::fs::read(whitelist_path)?)?;
Comment on lines +39 to +40
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done eagerly when config is initialized, not lazily in the getter which may be called multiple times.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, yes, but that requires modifying much more throughout the codebase. The approach here is to assume that the config is only responsible for providing the whitelist file path.


// for each line
for line in file_string.lines() {
let account = AccountAddress::from_hex(line.trim())?;
whitelisted.insert(account);
}

Ok(Some(whitelisted))
}
None => Ok(None),
}
}
}
25 changes: 0 additions & 25 deletions protocol-units/execution/util/src/config/mempool.rs

This file was deleted.

Loading