Skip to content

Commit

Permalink
Merge pull request #5209 from stacks-network/fix/2.5.0.0.7-rbf-logic
Browse files Browse the repository at this point in the history 
Fix/2.5.0.0.7 rbf logic
  • Loading branch information
@wileyj
wileyj committed Sep 19, 2024
2 parents 07dd941 + 2f979c9 commit 96efb61
Show file tree
Hide file tree
Showing 4 changed files with 217 additions and 65 deletions.
8 changes: 7 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
- `get-tenure-info?` added
- `get-block-info?` removed

## [2.5.0.0.7]
## [2.5.0.0.8]

### Added

Expand All @@ -33,6 +33,12 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
- Multi miner fixes jude (#5040)
- Remove spurious deadlock condition whenever the sortition DB is opened

## [2.5.0.0.7]

### Fixed

- Fix the RBF logic in the mempool admission which did not previously handle sponsor and origin nonce interactions correctly.

## [2.5.0.0.6]

### Changed
Expand Down
155 changes: 97 additions & 58 deletions stackslib/src/core/mempool.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -916,7 +916,7 @@ impl<'a> MemPoolTx<'a> {
&mut self,
coinbase_height: u64,
txid: &Txid,
prior_txid: Option<Txid>,
prior_txids: &[Txid],
) -> Result<Option<Txid>, MemPoolRejection> {
// is this the first-ever txid at this coinbase height?
let sql = "SELECT 1 FROM mempool WHERE height = ?1";
Expand All @@ -931,7 +931,7 @@ impl<'a> MemPoolTx<'a> {

MemPoolTx::with_bloom_state(self, |ref mut dbtx, ref mut bloom_counter| {
// remove replaced transaction
if let Some(prior_txid) = prior_txid {
for prior_txid in prior_txids.iter() {
bloom_counter.remove_raw(dbtx, &prior_txid.0)?;
}

Expand Down Expand Up @@ -2075,6 +2075,17 @@ impl MemPoolDB {
) -> Result<(), MemPoolRejection> {
let length = tx_bytes.len() as u64;

// don't allow a self sponsor with different nonces
if origin_address == sponsor_address && origin_nonce != sponsor_nonce {
info!("TX conflicts with own sponsor/origin nonces";
"new_txid" => %txid,
"origin_addr" => %origin_address,
"origin_nonce" => origin_nonce,
"sponsor_addr" => %sponsor_address,
"sponsor_nonce" => sponsor_nonce);
return Err(MemPoolRejection::ConflictingNonceInMempool);
}

// this transaction is said to arrive during this _tenure_, not during this _block_.
// In epoch 2.x, these are the same as `tip_consensus_hash` and `tip_block_header_hash`.
// In Nakamoto, they may be different.
Expand Down Expand Up @@ -2102,71 +2113,97 @@ impl MemPoolDB {
};

// do we already have txs with either the same origin nonce or sponsor nonce ?
let prior_tx = {
match MemPoolDB::get_tx_metadata_by_address(tx, true, origin_address, origin_nonce)? {
Some(prior_tx) => Some(prior_tx),
None => MemPoolDB::get_tx_metadata_by_address(
tx,
false,
sponsor_address,
sponsor_nonce,
)?,
let mut prior_txs: Vec<MemPoolTxMetadata> = vec![];
let prior_txs_to_check = [
MemPoolDB::get_tx_metadata_by_address(tx, true, origin_address, origin_nonce)?,
MemPoolDB::get_tx_metadata_by_address(tx, false, origin_address, origin_nonce)?,
MemPoolDB::get_tx_metadata_by_address(tx, true, sponsor_address, sponsor_nonce)?,
MemPoolDB::get_tx_metadata_by_address(tx, false, sponsor_address, sponsor_nonce)?,
];
for prior_tx_opt in prior_txs_to_check {
let Some(prior_tx) = prior_tx_opt else {
continue;
};
// only add if not already found before
let found_before = prior_txs
.iter()
.find(|already_in_list| already_in_list.txid == prior_tx.txid)
.is_some();
if !found_before {
prior_txs.push(prior_tx);
}
};
}

let mut replace_reason = MemPoolDropReason::REPLACE_BY_FEE;

// if so, is this a replace-by-fee? or a replace-in-chain-tip?
let add_tx = if let Some(ref prior_tx) = prior_tx {
if tx_fee > prior_tx.tx_fee {
// is this a replace-by-fee ?
debug!(
"Can replace {} with {} for {},{} by fee ({} < {})",
&prior_tx.txid, &txid, origin_address, origin_nonce, &prior_tx.tx_fee, &tx_fee
);
replace_reason = MemPoolDropReason::REPLACE_BY_FEE;
true
} else if !MemPoolDB::are_blocks_in_same_fork(
chainstate,
&prior_tx.tenure_consensus_hash,
&prior_tx.tenure_block_header_hash,
&consensus_hash,
&block_header_hash,
)? {
// is this a replace-across-fork ?
debug!(
"Can replace {} with {} for {},{} across fork",
&prior_tx.txid, &txid, origin_address, origin_nonce
);
replace_reason = MemPoolDropReason::REPLACE_ACROSS_FORK;
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" => %txid,
"old_txid" => %prior_tx.txid,
"origin_addr" => %origin_address,
"origin_nonce" => origin_nonce,
"sponsor_addr" => %sponsor_address,
"sponsor_nonce" => sponsor_nonce,
"new_fee" => tx_fee,
"old_fee" => prior_tx.tx_fee);
false
}
} else {
// no conflicting TX with this origin/sponsor, go ahead and add
let add_tx = if prior_txs.len() == 0 {
true
} else {
let mut all_passed = true;
for prior_tx in prior_txs.iter() {
if tx_fee > prior_tx.tx_fee {
// is this a replace-by-fee ?
debug!("Can replace TX by 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" => tx_fee,
"old_fee" => prior_tx.tx_fee);
replace_reason = MemPoolDropReason::REPLACE_BY_FEE;
} else if !MemPoolDB::are_blocks_in_same_fork(
chainstate,
&prior_tx.tenure_consensus_hash,
&prior_tx.tenure_block_header_hash,
&consensus_hash,
&block_header_hash,
)? {
// is this a replace-across-fork ?
debug!("Can replace TX across fork";
"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" => tx_fee,
"old_fee" => prior_tx.tx_fee);
replace_reason = MemPoolDropReason::REPLACE_ACROSS_FORK;
} 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" => %txid,
"old_txid" => %prior_tx.txid,
"origin_addr" => %origin_address,
"origin_nonce" => origin_nonce,
"sponsor_addr" => %sponsor_address,
"sponsor_nonce" => sponsor_nonce,
"new_fee" => tx_fee,
"old_fee" => prior_tx.tx_fee);
all_passed = false;
}
}
all_passed
};

if !add_tx {
return Err(MemPoolRejection::ConflictingNonceInMempool);
}

tx.update_bloom_counter(
coinbase_height,
&txid,
prior_tx.as_ref().map(|tx| tx.txid.clone()),
)?;
let prior_txids: Vec<_> = prior_txs.iter().map(|tx| tx.txid.clone()).collect();
tx.update_bloom_counter(coinbase_height, &txid, &prior_txids)?;

debug!("Inserting TX into mempool";
"txid" => %txid,
"replaced_txids" => ?prior_txids);

let sql = "DELETE FROM mempool WHERE txid = ?";
for txid in prior_txids.iter() {
tx.execute(sql, &[txid])
.map_err(|e| MemPoolRejection::DBError(db_error::SqliteError(e)))?;
}

let sql = "INSERT OR REPLACE INTO mempool (
txid,
Expand Down Expand Up @@ -2204,8 +2241,10 @@ impl MemPoolDB {
tx.update_mempool_pager(&txid)?;

// broadcast drop event if a tx is being replaced
if let (Some(prior_tx), Some(event_observer)) = (prior_tx, event_observer) {
event_observer.mempool_txs_dropped(vec![prior_tx.txid], replace_reason);
if let Some(event_observer) = event_observer {
if !prior_txids.is_empty() {
event_observer.mempool_txs_dropped(prior_txids, replace_reason);
}
};

Ok(())
Expand Down
111 changes: 108 additions & 3 deletions stackslib/src/core/tests/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1408,15 +1408,16 @@ fn mempool_db_load_store_replace_tx(#[case] behavior: MempoolCollectionBehavior)
let mut mempool_tx = mempool.tx_begin().unwrap();

eprintln!("add all txs");
for (i, mut tx) in txs.drain(..).enumerate() {
let txs_len = txs.len();
for (ix, mut tx) in txs.drain(..).enumerate() {
// make sure each address is unique per tx (not the case in codec_all_transactions)
let origin_address = StacksAddress {
version: 22,
bytes: Hash160::from_data(&i.to_be_bytes()),
bytes: Hash160::from_data(&ix.to_be_bytes()),
};
let sponsor_address = StacksAddress {
version: 22,
bytes: Hash160::from_data(&(i + 1).to_be_bytes()),
bytes: Hash160::from_data(&(ix + txs_len).to_be_bytes()),
};

tx.set_tx_fee(123);
Expand Down Expand Up @@ -2838,3 +2839,107 @@ fn test_filter_txs_by_type() {
},
);
}

#[test]
/// Test the handling of Origin/Sponsor RBF interactions
/// when origin and sponsor interact across two txs.
fn mempool_db_rbf_origin_sponsor() {
let mut chainstate = instantiate_chainstate(false, 0x80000000, function_name!());
let chainstate_path = chainstate_path(function_name!());
let mut mempool = MemPoolDB::open_test(false, 0x80000000, &chainstate_path).unwrap();

// create initial transaction
let mut mempool_tx = mempool.tx_begin().unwrap();
let height = 100;

let make_tx = |o_h160: &Hash160, o_nonce, s_h160: &Hash160, s_nonce, fee| {
let origin_condition =
TransactionSpendingCondition::Singlesig(SinglesigSpendingCondition {
signer: o_h160.clone(),
hash_mode: SinglesigHashMode::P2PKH,
key_encoding: TransactionPublicKeyEncoding::Compressed,
nonce: o_nonce,
tx_fee: 0,
signature: MessageSignature::from_raw(&vec![0xff; 65]),
});
let sponsor_condition =
TransactionSpendingCondition::Singlesig(SinglesigSpendingCondition {
signer: s_h160.clone(),
hash_mode: SinglesigHashMode::P2PKH,
key_encoding: TransactionPublicKeyEncoding::Compressed,
nonce: s_nonce,
tx_fee: fee,
signature: MessageSignature::from_raw(&vec![0xff; 65]),
});
let stx_address = StacksAddress {
version: 1,
bytes: Hash160([0xff; 20]),
};
let payload = TransactionPayload::TokenTransfer(
PrincipalData::from(QualifiedContractIdentifier {
issuer: stx_address.into(),
name: "hello-contract-name".into(),
}),
123,
TokenTransferMemo([0u8; 34]),
);
StacksTransaction {
version: TransactionVersion::Testnet,
chain_id: 0x80000000,
auth: TransactionAuth::Sponsored(origin_condition, sponsor_condition),
anchor_mode: TransactionAnchorMode::Any,
post_condition_mode: TransactionPostConditionMode::Allow,
post_conditions: Vec::new(),
payload,
}
};

let p_0 = Hash160::from_data(&[0; 16]);
let p_1 = Hash160::from_data(&[1; 16]);

let tx_1 = make_tx(&p_0, 1, &p_1, 2, 100);
let tx_2 = make_tx(&p_1, 2, &p_0, 1, 110);

assert!(!MemPoolDB::db_has_tx(&mempool_tx, &tx_1.txid()).unwrap());
assert!(!MemPoolDB::db_has_tx(&mempool_tx, &tx_2.txid()).unwrap());
MemPoolDB::try_add_tx(
&mut mempool_tx,
&mut chainstate,
&ConsensusHash([0x1; 20]),
&BlockHeaderHash([0x2; 32]),
false,
tx_1.txid(),
tx_1.serialize_to_vec(),
tx_1.get_tx_fee(),
height,
&tx_1.origin_address(),
tx_1.get_origin_nonce(),
tx_1.sponsor_address().as_ref().unwrap(),
tx_1.get_sponsor_nonce().unwrap(),
None,
)
.unwrap();

assert!(MemPoolDB::db_has_tx(&mempool_tx, &tx_1.txid()).unwrap());
assert!(!MemPoolDB::db_has_tx(&mempool_tx, &tx_2.txid()).unwrap());

MemPoolDB::try_add_tx(
&mut mempool_tx,
&mut chainstate,
&ConsensusHash([0x1; 20]),
&BlockHeaderHash([0x2; 32]),
false,
tx_2.txid(),
tx_2.serialize_to_vec(),
tx_2.get_tx_fee(),
height,
&tx_2.origin_address(),
tx_2.get_origin_nonce(),
tx_2.sponsor_address().as_ref().unwrap(),
tx_2.get_sponsor_nonce().unwrap(),
None,
)
.unwrap();
assert!(MemPoolDB::db_has_tx(&mempool_tx, &tx_2.txid()).unwrap());
assert!(!MemPoolDB::db_has_tx(&mempool_tx, &tx_1.txid()).unwrap());
}
8 changes: 5 additions & 3 deletions stackslib/src/net/prune.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,9 +199,11 @@ impl PeerNetwork {
match org_neighbors.get_mut(&org) {
None => {}
Some(ref mut neighbor_infos) => {
neighbor_infos.sort_by(|&(ref _nk1, ref stats1), &(ref _nk2, ref stats2)| {
PeerNetwork::compare_neighbor_uptime_health(stats1, stats2)
});
neighbor_infos.sort_unstable_by(
|&(ref _nk1, ref stats1), &(ref _nk2, ref stats2)| {
PeerNetwork::compare_neighbor_uptime_health(stats1, stats2)
},
);
}
}
}
Expand Down

0 comments on commit 96efb61

Please sign in to comment.