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: handle burn chain flapping #4563

Merged
merged 5 commits into from Mar 21, 2024
Merged

fix: handle burn chain flapping #4563

merged 5 commits into from Mar 21, 2024

Conversation

obycode
Copy link
Contributor

@obycode obycode commented Mar 20, 2024

This test (from Jude) can reproduce the problematic behavior when the burnchain flaps between two branches.

  • We get blocks at height 211 - 213
  • Then we get a fork, with different blocks at 211-213, as well as 214 and 215.
  • We then flap back to the original fork, so it goes back to the common ancestor, 210, and tries to download 211-215
  • When it gets block 211, it is already in the database, so it fails, cancelling the download of the rest of the blocks, leaving 214 and 215 in this branch not stored
  • Then we try to store 216, but its parent 215 is not stored yet, so we cannot continue.

The fix for this is to ignore attempts to store duplicate blocks.

Description

Applicable issues

  • fixes #

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@obycode obycode requested a review from jcnelson March 20, 2024 02:33
@obycode
Copy link
Contributor Author

obycode commented Mar 20, 2024

@jcnelson I couldn't come up with a good reason why we couldn't just ignore the duplicate entry. Let me know if you see any potential problems from this.

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 6.33803% with 133 lines in your changes are missing coverage. Please review.

Project coverage is 82.31%. Comparing base (0ccffa8) to head (be97a0b).

Files Patch % Lines
testnet/stacks-node/src/tests/neon_integrations.rs 2.77% 105 Missing ⚠️
testnet/stacks-node/src/tests/bitcoin_regtest.rs 0.00% 24 Missing ⚠️
stackslib/src/burnchains/db.rs 71.42% 2 Missing ⚠️
stackslib/src/burnchains/bitcoin/indexer.rs 50.00% 1 Missing ⚠️
stackslib/src/chainstate/coordinator/mod.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4563      +/-   ##
===========================================
+ Coverage    82.20%   82.31%   +0.11%     
===========================================
  Files          404      404              
  Lines       293057   293191     +134     
===========================================
+ Hits        240909   241350     +441     
+ Misses       52148    51841     -307     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jcnelson
Copy link
Member

Can you actually base this on develop? We'd want to test this prior to shipping it in a release. We'll also want this merged into next ASAP because we'll definitely hit it in 2.5 testnet.

@obycode
Copy link
Contributor Author

obycode commented Mar 20, 2024

Heads up: I will reapply these changes on develop and force push in a few minutes.

This test (from Jude) can reproduce the problematic behavior when
the burnchain flaps between two branches.

- We get blocks at height 211 - 213
- Then we get a fork, with different blocks at 211-213, as well as 214
  and 215.
- We then flap back to the original fork, so it goes back to the common
  ancestor, 210, and tries to download 211-215
- When it gets block 211, it is already in the database, so it fails,
  cancelling the download of the rest of the blocks, leaving 214 and 215
  in this branch not stored
- Then we try to store 216, but its parent 215 is not stored yet, so we
  cannot continue.

The fix for this is to ignore attempts to store duplicate blocks.
@obycode obycode changed the base branch from master to develop March 20, 2024 14:18
@obycode obycode requested a review from kantai March 20, 2024 14:18
kantai
kantai previously approved these changes Mar 20, 2024
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

LGTM -- just a few comments

jcnelson
jcnelson previously approved these changes Mar 20, 2024
jcnelson
jcnelson previously approved these changes Mar 20, 2024
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.

LGTM. Just make sure CI passes 🙏

@obycode
Copy link
Contributor Author

obycode commented Mar 20, 2024

Hmm, the new tests failed with Failed starting bitcoind: (). Is there something else that needs to be added for the CI setup? It works locally.

@kantai
Copy link
Member

kantai commented Mar 20, 2024

Not that I know of, but we might get a better error from CI with this map_err removed (since you added Debug to the error anyways):

    btcd_controller
        .start_bitcoind()
        .map_err(|_e| ())

This test passes locally but fails in CI. We cannot let it hold back
this PR, so comment it out for now.
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.

Approving for now, but we need to get that CI test working

@obycode obycode merged commit eaa1b22 into develop Mar 21, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants