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

Fix: Incorporate unlocks in mempool admitter, #3623 #3624

Merged
merged 3 commits into from
Mar 20, 2023

Conversation

kantai
Copy link
Member

@kantai kantai commented Mar 20, 2023

Description

This fixes the mempool admission behavior in #3623 by supplying a pointer to the current burn state (sortition db) to the mempool admission logic.

Will update this PR with regression testing before merging.

Applicable issues

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated

@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Merging #3624 (aa5f167) into master (d69012e) will increase coverage by 29.57%.
The diff coverage is 8.26%.

@@             Coverage Diff             @@
##           master    #3624       +/-   ##
===========================================
+ Coverage    0.18%   29.75%   +29.57%     
===========================================
  Files         298      298               
  Lines      275068   275543      +475     
===========================================
+ Hits          512    81999    +81487     
+ Misses     274556   193544    -81012     
Impacted Files Coverage Δ
src/chainstate/stacks/tests/block_construction.rs 0.00% <0.00%> (ø)
src/main.rs 0.00% <0.00%> (ø)
src/net/relay.rs 23.55% <0.00%> (+23.55%) ⬆️
testnet/stacks-node/src/tests/bitcoin_regtest.rs 15.84% <0.00%> (+15.84%) ⬆️
testnet/stacks-node/src/tests/mempool.rs 0.00% <0.00%> (ø)
testnet/stacks-node/src/tests/integrations.rs 40.03% <21.87%> (+40.03%) ⬆️
testnet/stacks-node/src/tests/mod.rs 46.44% <36.36%> (+46.44%) ⬆️
src/chainstate/stacks/db/blocks.rs 34.21% <66.66%> (+34.21%) ⬆️
src/core/mempool.rs 73.17% <100.00%> (+73.17%) ⬆️
src/net/p2p.rs 52.45% <100.00%> (+52.45%) ⬆️
... and 3 more

... and 203 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kantai
Copy link
Member Author

kantai commented Mar 20, 2023

New regression test added:

chainstate::stacks::tests::block_construction::mempool_incorporate_pox_unlocks

You can test the regression pretty simply by applying this diff to this PR and then running that test:

diff --git a/src/core/mempool.rs b/src/core/mempool.rs
index 7f3c195d2..5d5d121dc 100644
--- a/src/core/mempool.rs
+++ b/src/core/mempool.rs
@@ -180,7 +180,7 @@ impl MemPoolAdmitter {
         tx_size: u64,
     ) -> Result<(), MemPoolRejection> {
         chainstate.will_admit_mempool_tx(
-            &sortdb.index_conn(),
+            &clarity::vm::database::NULL_BURN_STATE_DB,
             &self.cur_consensus_hash,
             &self.cur_block,
             tx,

And then testing:

$ cargo test block_construction::mempool_incorporate
...
INFO [1679332313.674012] [src/chainstate/burn/db/sortdb.rs:4694] [chainstate::stacks::tests::block_construction::mempool_incorporate_pox_unlocks] ACCEPTED(38) leader key register 9158ed4afbd48e7c855e025384ddd1c181a8e26ff5b5083e0067f21ca0bc3b06 at 38,1
INFO [1679332313.720628] [src/chainstate/stacks/tests/block_construction.rs:4373] [chainstate::stacks::tests::block_construction::mempool_incorporate_pox_unlocks] Checking balance, v1_unlock_height: 37, burn_block_height: 37
thread 'chainstate::stacks::tests::block_construction::mempool_incorporate_pox_unlocks' panicked at 'called `Result::unwrap()` on an `Err` value: NotEnoughFunds(9999990000, 0)', src/chainstate/stacks/tests/block_construction.rs:4440:27
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    chainstate::stacks::tests::block_construction::mempool_incorporate_pox_unlocks

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1176 filtered out; finished in 2.31s

error: test failed, to rerun pass `--lib`

@kantai kantai requested a review from jcnelson March 20, 2023 17:15
@kantai kantai requested a review from igorsyl March 20, 2023 17:19
@kantai kantai self-assigned this Mar 20, 2023
@kantai kantai added the bug Unwanted or unintended property causing functional harm label Mar 20, 2023
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

Thanks for jumping on this @kantai!

Copy link
Contributor

@pavitthrap pavitthrap left a comment

Choose a reason for hiding this comment

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

verified regression - thanks for the fix!

@kantai kantai merged commit a3feafd into master Mar 20, 2023
@igorsyl
Copy link
Contributor

igorsyl commented Mar 20, 2023

@kantai sorry I missed this review. I'm way overloaded. Thanks for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unwanted or unintended property causing functional harm
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Mempool admission doesn't handle unlocks correctly
4 participants