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

Inconsistent mempool admission: #2050, #1719 #2153

Merged
merged 4 commits into from
Dec 11, 2020
Merged

Inconsistent mempool admission: #2050, #1719 #2153

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5e0bb80
fix: #2050 by way of implementing #1719
kantai Dec 10, 2020
2143ff7
fix test
kantai Dec 11, 2020
23660f3
fix mempool test
kantai Dec 11, 2020
2065972
Merge remote-tracking branch 'origin/next' into fix/nonce-response-2050
kantai Dec 11, 2020
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
97 changes: 47 additions & 50 deletions src/chainstate/stacks/db/blocks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,12 @@ pub enum MemPoolRejection {
NoCoinbaseViaMempool,
NoSuchChainTip(ConsensusHash, BlockHeaderHash),
ConflictingNonceInMempool,
TooMuchChaining,
TooMuchChaining {
max_nonce: u64,
actual_nonce: u64,
principal: PrincipalData,
is_origin: bool,
},
DBError(db_error),
Other(String),
}
Expand All @@ -155,9 +160,22 @@ impl MemPoolRejection {
DeserializationFailure(e) => {
("Deserialization", Some(json!({"message": e.to_string()})))
}
TooMuchChaining => (
TooMuchChaining {
max_nonce,
actual_nonce,
principal,
is_origin,
..
} => (
"TooMuchChaining",
Some(json!({"message": "Nonce would exceed chaining limit in mempool"})),
Some(
json!({"message": "Nonce would exceed chaining limit in mempool",
"expected": max_nonce,
"actual": actual_nonce,
"principal": principal.to_string(),
"is_origin": is_origin
}),
),
),
FailedToValidate(e) => (
"SignatureValidation",
Expand Down Expand Up @@ -4838,7 +4856,6 @@ impl StacksChainState {
/// unconfirmed microblock stream trailing off of it.
pub fn will_admit_mempool_tx(
&mut self,
mempool_conn: &DBConn,
current_consensus_hash: &ConsensusHash,
current_block: &BlockHeaderHash,
tx: &StacksTransaction,
Expand Down Expand Up @@ -4881,14 +4898,7 @@ impl StacksChainState {
let current_tip =
StacksChainState::get_parent_index_block(current_consensus_hash, current_block);
let res = match self.with_read_only_clarity_tx(&NULL_BURN_STATE_DB, &current_tip, |conn| {
StacksChainState::can_include_tx(
mempool_conn,
conn,
&conf,
has_microblock_pubk,
tx,
tx_size,
)
StacksChainState::can_include_tx(conn, &conf, has_microblock_pubk, tx, tx_size)
}) {
Some(r) => r,
None => Err(MemPoolRejection::NoSuchChainTip(
Expand All @@ -4909,7 +4919,6 @@ impl StacksChainState {
&tx.txid(), mismatch_error.expected, mismatch_error.actual);
self.with_read_only_unconfirmed_clarity_tx(&NULL_BURN_STATE_DB, |conn| {
StacksChainState::can_include_tx(
mempool_conn,
conn,
&conf,
has_microblock_pubk,
Expand All @@ -4928,8 +4937,7 @@ impl StacksChainState {

/// Given an outstanding clarity connection, can we append the tx to the chain state?
/// Used when mining transactions.
pub fn can_include_tx<T: ClarityConnection>(
mempool: &DBConn,
fn can_include_tx<T: ClarityConnection>(
clarity_connection: &mut T,
chainstate_config: &DBConfig,
has_microblock_pubkey: bool,
Expand All @@ -4956,46 +4964,35 @@ impl StacksChainState {
let (origin, payer) =
match StacksChainState::check_transaction_nonces(clarity_connection, &tx, true) {
Ok(x) => x,
Err((mut e, (origin, payer))) => {
// let's see if the tx has matching nonce in the mempool
// -> you can _only_ replace-by-fee or replace-across-fork
// for nonces that increment the current chainstate's nonce.
// if there are "chained" transactions in the mempool,
// this check won't admit a rbf/raf for those.
let origin_addr = tx.origin_address();
let origin_nonce = tx.get_origin().nonce();
let origin_next_nonce =
MemPoolDB::get_next_nonce_for_address(mempool, &origin_addr)?;
if origin_next_nonce < origin.nonce {
// if errored, check if MEMPOOL_TX_CHAINING would admit this TX
Err((e, (origin, payer))) => {
// if the nonce is less than expected, then TX_CHAINING would not allow in any case
if e.actual < e.expected {
return Err(e.into());
}
if origin_next_nonce - origin.nonce >= MAXIMUM_MEMPOOL_TX_CHAINING {
return Err(MemPoolRejection::TooMuchChaining);
}
if origin_nonce != origin_next_nonce {
e.is_origin = true;
e.principal = origin_addr.into();
e.expected = origin_next_nonce;
e.actual = origin_nonce;
return Err(e.into());

let tx_origin_nonce = tx.get_origin().nonce();

let origin_max_nonce = origin.nonce + 1 + MAXIMUM_MEMPOOL_TX_CHAINING;
if origin_max_nonce < tx_origin_nonce {
return Err(MemPoolRejection::TooMuchChaining {
max_nonce: origin_max_nonce,
actual_nonce: tx_origin_nonce,
principal: tx.origin_address().into(),
is_origin: true,
});
}

if let Some(sponsor_addr) = tx.sponsor_address() {
let sponsor_nonce = tx.get_payer().nonce();
let sponsor_next_nonce =
MemPoolDB::get_next_nonce_for_address(mempool, &sponsor_addr)?;
if sponsor_next_nonce < payer.nonce {
return Err(e.into());
}
if sponsor_next_nonce - payer.nonce >= MAXIMUM_MEMPOOL_TX_CHAINING {
return Err(MemPoolRejection::TooMuchChaining);
}
if sponsor_nonce != sponsor_next_nonce {
e.is_origin = false;
e.principal = sponsor_addr.into();
e.expected = sponsor_next_nonce;
e.actual = sponsor_nonce;
return Err(e.into());
let tx_sponsor_nonce = tx.get_payer().nonce();
let sponsor_max_nonce = payer.nonce + 1 + MAXIMUM_MEMPOOL_TX_CHAINING;
if sponsor_max_nonce < tx_sponsor_nonce {
return Err(MemPoolRejection::TooMuchChaining {
max_nonce: sponsor_max_nonce,
actual_nonce: tx_sponsor_nonce,
principal: sponsor_addr.into(),
is_origin: false,
});
}
}
(origin, payer)
Expand Down
24 changes: 11 additions & 13 deletions src/core/mempool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,18 +79,11 @@ impl MemPoolAdmitter {

pub fn will_admit_tx(
&mut self,
mempool_conn: &DBConn,
chainstate: &mut StacksChainState,
tx: &StacksTransaction,
tx_size: u64,
) -> Result<(), MemPoolRejection> {
chainstate.will_admit_mempool_tx(
mempool_conn,
&self.cur_consensus_hash,
&self.cur_block,
tx,
tx_size,
)
chainstate.will_admit_mempool_tx(&self.cur_consensus_hash, &self.cur_block, tx, tx_size)
}
}

Expand Down Expand Up @@ -804,8 +797,15 @@ impl MemPoolDB {
true
} else {
// there's a >= fee tx in this fork, cannot add
info!("TX conflicts with sponsor/origin nonce in same fork with >= fee: new_txid={}, old_txid={}, origin_addr={}, origin_nonce={}, sponsor_addr={}, sponsor_nonce={}, new_fee={}, old_fee={}",
txid, prior_tx.txid, origin_address, origin_nonce, sponsor_address, sponsor_nonce, estimated_fee, prior_tx.estimated_fee);
info!("TX conflicts with sponsor/origin nonce in same fork with >= fee";
"new_txid" => %txid,
"old_txid" => %prior_tx.txid,
"origin_addr" => %origin_address,
"origin_nonce" => origin_nonce,
"sponsor_addr" => %sponsor_address,
"sponsor_nonce" => sponsor_nonce,
"new_fee" => estimated_fee,
"old_fee" => prior_tx.estimated_fee);
false
}
} else {
Expand Down Expand Up @@ -960,9 +960,7 @@ impl MemPoolDB {
mempool_tx
.admitter
.set_block(&block_hash, (*consensus_hash).clone());
mempool_tx
.admitter
.will_admit_tx(&mempool_tx.tx, chainstate, &tx, len)?;
mempool_tx.admitter.will_admit_tx(chainstate, &tx, len)?;
}

MemPoolDB::try_add_tx(
Expand Down
17 changes: 10 additions & 7 deletions testnet/stacks-node/src/tests/integrations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,7 @@ fn integration_test_get_info() {
eprintln!("Test: POST {} (invalid)", path);

// tx_xfer_invalid is 180 bytes long
let tx_xfer_invalid = make_stacks_transfer(&spender_sk, (round + 10).into(), 200, // bad nonce
let tx_xfer_invalid = make_stacks_transfer(&spender_sk, (round + 30).into(), 200, // bad nonce
&StacksAddress::from_string(ADDR_4).unwrap().into(), 456);

let tx_xfer_invalid_tx = StacksTransaction::consensus_deserialize(&mut &tx_xfer_invalid[..]).unwrap();
Expand Down Expand Up @@ -772,7 +772,7 @@ fn contract_stx_transfer() {
)
.unwrap_err()
{
MemPoolRejection::TooMuchChaining => true,
MemPoolRejection::TooMuchChaining { .. } => true,
_ => false,
};
assert!(result);
Expand Down Expand Up @@ -1399,8 +1399,8 @@ fn mempool_errors() {
eprintln!("Test: POST {} (invalid)", path);
let tx_xfer_invalid = make_stacks_transfer(
&spender_sk,
3,
200, // bad nonce
30, // bad nonce -- too much chaining
200,
&send_to,
456,
);
Expand All @@ -1425,15 +1425,18 @@ fn mempool_errors() {
res.get("error").unwrap().as_str().unwrap(),
"transaction rejected"
);
assert_eq!(res.get("reason").unwrap().as_str().unwrap(), "BadNonce");
assert_eq!(
res.get("reason").unwrap().as_str().unwrap(),
"TooMuchChaining"
);
let data = res.get("reason_data").unwrap();
assert_eq!(data.get("is_origin").unwrap().as_bool().unwrap(), true);
assert_eq!(
data.get("principal").unwrap().as_str().unwrap(),
&spender_addr.to_string()
);
assert_eq!(data.get("expected").unwrap().as_i64().unwrap(), 0);
assert_eq!(data.get("actual").unwrap().as_i64().unwrap(), 3);
assert_eq!(data.get("expected").unwrap().as_i64().unwrap(), 26);
assert_eq!(data.get("actual").unwrap().as_i64().unwrap(), 30);

let tx_xfer_invalid = make_stacks_transfer(
&spender_sk,
Expand Down
Loading