From eedc5604b304d1ba3c4b21e3397919e63d341cc2 Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Tue, 26 May 2020 21:30:25 -0400 Subject: [PATCH 1/6] for better testing/ordering, select and process transactions in order by nonce for now --- src/core/mempool.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/core/mempool.rs b/src/core/mempool.rs index 879b62075..a74312e7f 100644 --- a/src/core/mempool.rs +++ b/src/core/mempool.rs @@ -355,8 +355,7 @@ impl MemPoolDB { /// each ancestor chain tip from the given one, starting with the /// most recent chain tip and working backwards until there are /// no more transactions to consider. Each batch of transactions - /// passed to todo will be sorted in fee order, starting with the - /// highest. + /// passed to todo will be sorted in nonce order. pub fn iterate_candidates(&self, tip_burn_header_hash: &BurnchainHeaderHash, tip_block_hash: &BlockHeaderHash, @@ -389,6 +388,9 @@ impl MemPoolDB { loop { let available_txs = MemPoolDB::get_txs_at(&self.db, &tip_burn_header_hash, &tip_block_hash, next_timestamp)?; + + debug!("Have {} transactions at {}/{} height={} at or after {}", available_txs.len(), &tip_burn_header_hash, &tip_block_hash, tip_height, next_timestamp); + todo(available_txs)?; next_timestamp = match MemPoolDB::get_next_timestamp(&self.db, &tip_burn_header_hash, &tip_block_hash, next_timestamp)? { Some(ts) => ts, @@ -444,14 +446,15 @@ impl MemPoolDB { /// Get the next timestamp after this one that occurs in this chain tip. pub fn get_next_timestamp(conn: &DBConn, burnchain_header_hash: &BurnchainHeaderHash, block_header_hash: &BlockHeaderHash, timestamp: u64) -> Result, db_error> { - let sql = "SELECT accept_time FROM mempool WHERE accept_time > ?1 AND burn_header_hash = ?2 AND block_header_hash = ?3 ORDER BY accept_time LIMIT 1"; + let sql = "SELECT accept_time FROM mempool WHERE accept_time > ?1 AND burn_header_hash = ?2 AND block_header_hash = ?3 ORDER BY accept_time ASC LIMIT 1"; let args : &[&dyn ToSql] = &[&u64_to_sql(timestamp)?, burnchain_header_hash, block_header_hash]; query_row(conn, sql, args) } - /// Get all transactions at a particular timestamp and chain tip + /// Get all transactions at a particular timestamp on a given chain tip. + /// Order them by origin nonce. pub fn get_txs_at(conn: &DBConn, burn_header_hash: &BurnchainHeaderHash, block_header_hash: &BlockHeaderHash, timestamp: u64) -> Result, db_error> { - let sql = "SELECT * FROM mempool WHERE accept_time = ?1 AND burn_header_hash = ?2 AND block_header_hash = ?3 ORDER BY estimated_fee DESC"; + let sql = "SELECT * FROM mempool WHERE accept_time = ?1 AND burn_header_hash = ?2 AND block_header_hash = ?3 ORDER BY origin_nonce ASC"; let args : &[&dyn ToSql] = &[&u64_to_sql(timestamp)?, burn_header_hash, block_header_hash]; let rows = query_rows::(conn, &sql, args)?; Ok(rows) From ad47dfb57e9641b836e06156ea476698a0d5342c Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Tue, 26 May 2020 21:31:02 -0400 Subject: [PATCH 2/6] add test method to instantiate chainstate with initial balances --- src/chainstate/stacks/db/mod.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/chainstate/stacks/db/mod.rs b/src/chainstate/stacks/db/mod.rs index 708281ef7..8fdf0b994 100644 --- a/src/chainstate/stacks/db/mod.rs +++ b/src/chainstate/stacks/db/mod.rs @@ -1056,6 +1056,19 @@ pub mod test { StacksChainState::open(mainnet, chain_id, &path).unwrap() } + pub fn instantiate_chainstate_with_balances(mainnet: bool, chain_id: u32, test_name: &str, balances: Vec<(StacksAddress, u64)>) -> StacksChainState { + let path = chainstate_path(test_name); + match fs::metadata(&path) { + Ok(_) => { + fs::remove_dir_all(&path).unwrap(); + }, + Err(_) => {} + }; + + let initial_balances = Some(balances.into_iter().map(|(addr, balance)| (PrincipalData::from(addr), balance)).collect()); + StacksChainState::open_and_exec(mainnet, chain_id, &path, initial_balances, |_| {}, ExecutionCost::max_value()).unwrap() + } + pub fn open_chainstate(mainnet: bool, chain_id: u32, test_name: &str) -> StacksChainState { let path = chainstate_path(test_name); StacksChainState::open(mainnet, chain_id, &path).unwrap() From 1a322b3e9138bbbca66bcb27cafe8be00439856b Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Tue, 26 May 2020 21:31:22 -0400 Subject: [PATCH 3/6] when building an anchored block, allow using multiple transactions from the same origin as long as their nonces increase monotonically --- src/chainstate/stacks/miner.rs | 113 +++++++++++++++++++++++++++++++-- 1 file changed, 106 insertions(+), 7 deletions(-) diff --git a/src/chainstate/stacks/miner.rs b/src/chainstate/stacks/miner.rs index f87df99bb..16aff0da3 100644 --- a/src/chainstate/stacks/miner.rs +++ b/src/chainstate/stacks/miner.rs @@ -606,7 +606,7 @@ impl StacksBlockBuilder { debug!("Build anchored block off of {}/{} height {}", &tip_burn_header_hash, &tip_block_hash, tip_height); - let mut header_reader_chainstate = chainstate_handle.reopen()?; // uesd for reading block headers during an epoch + let mut header_reader_chainstate = chainstate_handle.reopen()?; // used for reading block headers during an epoch let mut chainstate = chainstate_handle.reopen_limited(execution_budget)?; // used for processing a block up to the given limit let mut builder = StacksBlockBuilder::make_block_builder(parent_stacks_header, proof, total_burn, pubkey_hash)?; @@ -615,8 +615,8 @@ impl StacksBlockBuilder { builder.try_mine_tx(&mut epoch_tx, coinbase_tx)?; let mut considered = HashSet::new(); // txids of all transactions we looked at - let mut mined_origin_nonces = HashMap::new(); // map addrs of mined transaction origins to the nonces we used - let mut mined_sponsor_nonces = HashMap::new(); // map addrs of mined transaction sponsors to the nonces we used + let mut mined_origin_nonces : HashMap = HashMap::new(); // map addrs of mined transaction origins to the nonces we used + let mut mined_sponsor_nonces : HashMap = HashMap::new(); // map addrs of mined transaction sponsors to the nonces we used let result = mempool.iterate_candidates(&tip_burn_header_hash, &tip_block_hash, tip_height, &mut header_reader_chainstate, |available_txs| { for txinfo in available_txs.into_iter() { @@ -624,12 +624,18 @@ impl StacksBlockBuilder { if considered.contains(&txinfo.tx.txid()) { continue; } - if mined_origin_nonces.get(&txinfo.tx.origin_address()).is_some() { - continue; + if let Some(nonce) = mined_origin_nonces.get(&txinfo.tx.origin_address()) { + if *nonce >= txinfo.tx.get_origin_nonce() { + continue; + } } if let Some(sponsor_addr) = txinfo.tx.sponsor_address() { - if mined_sponsor_nonces.get(&sponsor_addr).is_some() { - continue; + if let Some(nonce) = mined_sponsor_nonces.get(&sponsor_addr) { + if let Some(sponsor_nonce) = txinfo.tx.get_sponsor_nonce() { + if *nonce >= sponsor_nonce { + continue; + } + } } } @@ -3894,6 +3900,99 @@ pub mod test { } } } + + #[test] + fn test_build_anchored_blocks_too_expensive_transactions() { + let mut privks = vec![]; + let mut balances = vec![]; + let num_blocks = 3; + + for _ in 0..num_blocks { + let privk = StacksPrivateKey::new(); + let addr = StacksAddress::from_public_keys(C32_ADDRESS_VERSION_TESTNET_SINGLESIG, &AddressHashMode::SerializeP2PKH, 1, &vec![StacksPublicKey::from_private(&privk)]).unwrap(); + + privks.push(privk); + balances.push((addr.to_account_principal(), 100000000)); + } + + let mut peer_config = TestPeerConfig::new("test_build_anchored_blocks_too_expensive_transactions", 2012, 2013); + peer_config.initial_balances = balances; + + let mut peer = TestPeer::new(peer_config); + + let chainstate_path = peer.chainstate_path.clone(); + + let first_stacks_block_height = { + let sn = BurnDB::get_canonical_burn_chain_tip(&peer.burndb.as_ref().unwrap().conn()).unwrap(); + sn.block_height + }; + + let mut last_block = None; + for tenure_id in 0..num_blocks { + // send transactions to the mempool + let tip = BurnDB::get_canonical_burn_chain_tip(&peer.burndb.as_ref().unwrap().conn()).unwrap(); + + let (burn_ops, stacks_block, microblocks) = peer.make_tenure(|ref mut miner, ref mut burndb, ref mut chainstate, vrf_proof, ref parent_opt, ref parent_microblock_header_opt| { + let parent_tip = match parent_opt { + None => { + StacksChainState::get_genesis_header_info(&chainstate.headers_db).unwrap() + } + Some(block) => { + let ic = burndb.index_conn(); + let snapshot = BurnDB::get_block_snapshot_for_winning_stacks_block(&ic, &tip.burn_header_hash, &block.block_hash()).unwrap().unwrap(); // succeeds because we don't fork + StacksChainState::get_anchored_block_header_info(&chainstate.headers_db, &snapshot.burn_header_hash, &snapshot.winning_stacks_block_hash).unwrap().unwrap() + } + }; + + let parent_header_hash = parent_tip.anchored_header.block_hash(); + let parent_tip_bhh = parent_tip.burn_header_hash.clone(); + let coinbase_tx = make_coinbase(miner, tenure_id); + + let mut mempool = MemPoolDB::open(false, 0x80000000, &chainstate_path).unwrap(); + + if tenure_id == 2 { + let contract = " + (define-data-var bar int 0) + (define-public (get-bar) (ok (var-get bar))) + (define-public (set-bar (x int) (y int)) + (begin (var-set bar (/ x y)) (ok (var-get bar))))"; + + // should be mined once + let contract_tx = make_user_contract_publish(&privks[tenure_id], 0, 100000000 / 2 + 1, &format!("hello-world-{}", tenure_id), &contract); + let mut contract_tx_bytes = vec![]; + contract_tx.consensus_serialize(&mut contract_tx_bytes).unwrap(); + mempool.submit_raw(&parent_tip_bhh, &parent_header_hash, contract_tx_bytes).unwrap(); + + eprintln!("\n\ntransaction:\n{:#?}\n\n", &contract_tx); + + sleep_ms(2000); + + // should never be mined + let contract_tx = make_user_contract_publish(&privks[tenure_id], 1, 100000000 / 2, &format!("hello-world-{}-2", tenure_id), &contract); + let mut contract_tx_bytes = vec![]; + contract_tx.consensus_serialize(&mut contract_tx_bytes).unwrap(); + mempool.submit_raw(&parent_tip_bhh, &parent_header_hash, contract_tx_bytes).unwrap(); + + eprintln!("\n\ntransaction:\n{:#?}\n\n", &contract_tx); + + sleep_ms(2000); + } + + let anchored_block = StacksBlockBuilder::build_anchored_block(chainstate, &mempool, &parent_tip, tip.total_burn, vrf_proof, Hash160([tenure_id as u8; 20]), &coinbase_tx, ExecutionCost::max_value()).unwrap(); + + (anchored_block.0, vec![]) + }); + + last_block = Some(stacks_block.clone()); + + peer.next_burnchain_block(burn_ops.clone()); + peer.process_stacks_epoch_at_tip(&stacks_block, µblocks); + + test_debug!("\n\ncheck tenure {}: {} transactions\n", tenure_id, stacks_block.txs.len()); + + // assert_eq!(stacks_block.txs.len(), 1); + } + } // TODO: invalid block with duplicate microblock public key hash (okay between forks, but not // within the same fork) From 237febcc4bab981f4969dac92e52b983eb52a1a4 Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Tue, 26 May 2020 21:32:13 -0400 Subject: [PATCH 4/6] fix and add a test for #1623. The bug was manifested due to the fact that we need to bill the paying account for the transaction fee _after_ the transaction runs (eventually, so we'll only be billing accounts for computation used). However, a contract-call can empty an account when it runs, leaving it unable to pay the transaction fee. This could cause a runtime panic. This patch fixes it by re-loading the payer account after the payload is processed, and using that account to check to see if there are enough STX to pay the fee (and aborting if not) --- src/chainstate/stacks/db/transactions.rs | 76 +++++++++++++++++++++++- 1 file changed, 74 insertions(+), 2 deletions(-) diff --git a/src/chainstate/stacks/db/transactions.rs b/src/chainstate/stacks/db/transactions.rs index 92e045f62..6e8582b98 100644 --- a/src/chainstate/stacks/db/transactions.rs +++ b/src/chainstate/stacks/db/transactions.rs @@ -181,6 +181,21 @@ impl From for MemPoolRejection { } impl StacksChainState { + /// Get the payer account + fn get_payer_account(clarity_tx: &mut T, tx: &StacksTransaction) -> StacksAccount { + // who's paying the fee? + let payer_account = + if let Some(sponsor_address) = tx.sponsor_address() { + let payer_account = StacksChainState::get_account(clarity_tx, &sponsor_address.into()); + payer_account + } else { + let origin_account = StacksChainState::get_account(clarity_tx, &tx.origin_address().into()); + origin_account + }; + + payer_account + } + /// Check the account nonces for the supplied stacks transaction, /// returning the origin and payer accounts if valid. pub fn check_transaction_nonces(clarity_tx: &mut T, tx: &StacksTransaction) -> Result<(StacksAccount, StacksAccount), TransactionNonceMismatch> { @@ -669,10 +684,15 @@ impl StacksChainState { let tx_receipt = StacksChainState::process_transaction_payload(&mut transaction, tx, &origin_account)?; // pay fee borne by runtime costs. - // TODO: this is the fee *rate*, not the absolute fee. This code is broken until we have + // NOTE: the fee must be paid _after_ we run the payload, because we will (eventually) be + // debiting the account a fee equal to its transaction's runtime cost (which can only be + // determined by running the code). Hence, we need to refresh the payer account after the + // transaction body runs. + // TODO: this field is the fee *rate*, not the absolute fee. This code is broken until we have // the true block reward system built. + let new_payer_account = StacksChainState::get_payer_account(&mut transaction, tx); let fee = tx.get_fee_rate(); - StacksChainState::pay_transaction_fee(&mut transaction, fee, &payer_account)?; + StacksChainState::pay_transaction_fee(&mut transaction, fee, &new_payer_account)?; // update the account nonces StacksChainState::update_account_nonce(&mut transaction, &origin_account); @@ -3171,5 +3191,57 @@ pub mod test { } } + #[test] + fn process_smart_contract_fee_check() { + let contract = r#" + (define-public (send-stx (amount uint) (recipient principal)) + (stx-transfer? amount tx-sender recipient)) + "#; + + let privk = StacksPrivateKey::from_hex("6d430bb91222408e7706c9001cfaeb91b08c2be6d5ac95779ab52c6b431950e001").unwrap(); + let auth = TransactionAuth::from_p2pkh(&privk).unwrap(); + let addr = auth.origin().address_testnet(); + + let balances = vec![(addr.clone(), 1000000000)]; + + let mut chainstate = instantiate_chainstate_with_balances(false, 0x80000000, "process-smart-contract-fee_check", balances); + + let mut tx_contract_create = StacksTransaction::new(TransactionVersion::Testnet, + auth.clone(), + TransactionPayload::new_smart_contract(&"hello-world".to_string(), &contract.to_string()).unwrap()); + + tx_contract_create.chain_id = 0x80000000; + tx_contract_create.set_fee_rate(0); + + let mut signer = StacksTransactionSigner::new(&tx_contract_create); + signer.sign_origin(&privk).unwrap(); + + let signed_contract_tx = signer.get_tx().unwrap(); + + let mut tx_contract_call = StacksTransaction::new(TransactionVersion::Testnet, + auth.clone(), + TransactionPayload::new_contract_call( + addr.clone(), + "hello-world", + "send-stx", vec![Value::UInt(1000000000), Value::Principal(PrincipalData::from(StacksAddress::from_string("ST1H1B54MY50RMBRRKS7GV2ZWG79RZ1RQ1ETW4E01").unwrap()))]).unwrap()); + + tx_contract_call.chain_id = 0x80000000; + tx_contract_call.set_fee_rate(1); + tx_contract_call.set_origin_nonce(1); + tx_contract_call.post_condition_mode = TransactionPostConditionMode::Allow; + + let mut signer = StacksTransactionSigner::new(&tx_contract_call); + signer.sign_origin(&privk).unwrap(); + + let signed_contract_call_tx = signer.get_tx().unwrap(); + + let mut conn = chainstate.block_begin(&FIRST_BURNCHAIN_BLOCK_HASH, &FIRST_STACKS_BLOCK_HASH, &BurnchainHeaderHash([1u8; 32]), &BlockHeaderHash([1u8; 32])); + let (fee, _) = StacksChainState::process_transaction(&mut conn, &signed_contract_tx).unwrap(); + let err = StacksChainState::process_transaction(&mut conn, &signed_contract_call_tx).unwrap_err(); + conn.commit_block(); + + eprintln!("{:?}", &err); + assert_eq!(fee, 0); + } // TODO: test poison microblock } From 02126880894d2f1ed3327e0c27d6c24c4e537681 Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Tue, 26 May 2020 21:41:22 -0400 Subject: [PATCH 5/6] whoops, also check that the error we get from mining the transaction that liquidates its own balance is an InvalidFee error --- src/chainstate/stacks/db/transactions.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/chainstate/stacks/db/transactions.rs b/src/chainstate/stacks/db/transactions.rs index 6e8582b98..269c14b79 100644 --- a/src/chainstate/stacks/db/transactions.rs +++ b/src/chainstate/stacks/db/transactions.rs @@ -3242,6 +3242,7 @@ pub mod test { eprintln!("{:?}", &err); assert_eq!(fee, 0); + if let Error::InvalidFee = err {} else { assert!(false) }; } // TODO: test poison microblock } From f1f0043e82595df7c542cd15df0323433c435a3e Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Thu, 28 May 2020 00:12:46 -0400 Subject: [PATCH 6/6] fix failing test now that we prioritize transactions by nonce --- testnet/stacks-node/src/tests/integrations.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/testnet/stacks-node/src/tests/integrations.rs b/testnet/stacks-node/src/tests/integrations.rs index db51b1578..5ed8406f1 100644 --- a/testnet/stacks-node/src/tests/integrations.rs +++ b/testnet/stacks-node/src/tests/integrations.rs @@ -843,13 +843,13 @@ fn bad_contract_tx_rollback() { }, 2 => { assert_eq!(chain_tip.metadata.block_height, 3); - // Block #2 should have 4 txs -- coinbase + 1 transfer + 1 publish - assert_eq!(chain_tip.block.txs.len(), 3); + // Block #2 should have 4 txs -- coinbase + 2 transfer + 1 publish + assert_eq!(chain_tip.block.txs.len(), 4); }, 3 => { assert_eq!(chain_tip.metadata.block_height, 4); - // Block #2 should have 4 txs -- coinbase + 1 transfer - assert_eq!(chain_tip.block.txs.len(), 2); + // Block #2 should have 1 txs -- coinbase + assert_eq!(chain_tip.block.txs.len(), 1); }, _ => {}, }