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/2970 #3016

Merged
merged 7 commits into from
Jan 31, 2022
Merged

Fix/2970 #3016

merged 7 commits into from
Jan 31, 2022

Conversation

jcnelson
Copy link
Member

This fixes #2970 by ensuring that the unconfirmed state MARF trie is always committed when processing unconfirmed microblocks, even if the microblocks are invalid.

Thanks to @wileyj, I was able to identify the root cause of #2970: the node was processing an invalid microblock and then leaving the MARF open, which would then cause the node to crash when it attempted to mine a microblock (via InProgressError). The solution is to unconditionally close the unconfirmed state MARF trie, even if the unconfirmed microblock stream is bad. I have added a unit test that fails in develop but succeeds in this PR to demonstrate this.

Almost as concerning is how the invalid microblock came to be. The microblock in question was invalid because it contained a transaction with a bad nonce. Moreover, this node mined it -- it produced a microblock that was valid at the time it was created, but later became invalid through some unknown means. I need to figure out why this is, but for the time being, this PR addresses the crash.

…unconfirmed state, and in so doing, *never* leave the MARF trie open (#2970).  Confirm this with a unit test.
@jcnelson jcnelson changed the base branch from develop to feat/faster-bootup January 26, 2022 16:26
@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #3016 (3b2f218) into develop (a289504) will increase coverage by 54.40%.
The diff coverage is 99.62%.

Impacted file tree graph

@@             Coverage Diff              @@
##           develop    #3016       +/-   ##
============================================
+ Coverage    28.15%   82.56%   +54.40%     
============================================
  Files          242      242               
  Lines       195403   195667      +264     
============================================
+ Hits         55025   161556   +106531     
+ Misses      140378    34111   -106267     
Impacted Files Coverage Δ
src/chainstate/stacks/db/unconfirmed.rs 93.84% <99.62%> (+59.85%) ⬆️
src/chainstate/stacks/miner.rs 93.89% <100.00%> (+83.74%) ⬆️
src/main.rs 0.11% <0.00%> (+0.11%) ⬆️
testnet/stacks-node/src/run_loop/neon.rs 81.21% <0.00%> (+0.56%) ⬆️
src/cost_estimates/fee_medians.rs 93.27% <0.00%> (+0.89%) ⬆️
src/types/chainstate.rs 90.14% <0.00%> (+1.40%) ⬆️
src/cost_estimates/fee_scalar.rs 90.78% <0.00%> (+1.41%) ⬆️
src/burnchains/bitcoin/mod.rs 40.57% <0.00%> (+1.44%) ⬆️
src/vm/database/key_value_wrapper.rs 96.48% <0.00%> (+1.59%) ⬆️
src/chainstate/coordinator/comm.rs 85.98% <0.00%> (+2.80%) ⬆️
... and 200 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a289504...3b2f218. Read the comment docs.

@diwakergupta diwakergupta linked an issue Jan 26, 2022 that may be closed by this pull request
@jcnelson jcnelson changed the base branch from feat/faster-bootup to develop January 27, 2022 00:57
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.

lgtm

@CLAassistant
Copy link

CLAassistant commented Jan 31, 2022

CLA assistant check
All committers have signed the CLA.

@jcnelson
Copy link
Member Author

@pavitthrap can you sign the CLA? Thanks!

@pavitthrap
Copy link
Contributor

@jcnelson could you add a note about this in CHANGELOG.md?

@jcnelson jcnelson merged commit ca36b2e into develop Jan 31, 2022
@jcnelson jcnelson mentioned this pull request Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stacks-node crashed
4 participants