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

Crash the ledger if committing a block fails #2657

Merged
merged 5 commits into from
Feb 22, 2024
Merged

Conversation

sug0
Copy link
Collaborator

@sug0 sug0 commented Feb 19, 2024

Describe your changes

Rather than allowing CometBFT to keep processing blocks after a storage write has failed in Namada, we crash the ledger to avoid any potential corruption of state.

This PR is eligible for non-breaking draft.

Indicate on which release or other PRs this topic is based on

v0.31.4

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@sug0 sug0 added bug Something isn't working ledger labels Feb 19, 2024
@sug0 sug0 marked this pull request as ready for review February 19, 2024 13:36
@sug0 sug0 requested review from tzemanovic and yito88 February 19, 2024 13:37
@sug0
Copy link
Collaborator Author

sug0 commented Feb 19, 2024

Abides by #759

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (9181fbe) 53.38% compared to head (459eb72) 53.39%.
Report is 8 commits behind head on main.

Files Patch % Lines
crates/apps/src/lib/node/ledger/shell/mod.rs 93.33% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2657   +/-   ##
=======================================
  Coverage   53.38%   53.39%           
=======================================
  Files         302      302           
  Lines      103398   103394    -4     
=======================================
+ Hits        55203    55211    +8     
+ Misses      48195    48183   -12     

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

@Fraccaman
Copy link
Member

@sug0 I think this is still a breaking changes, no? We could have some nodes crashing with this PR and some other node not crashing on the same block

@sug0
Copy link
Collaborator Author

sug0 commented Feb 19, 2024

in theory yes, but I think no one has ever run into a failing commit_block yet. hm but we can move it to the breaking draft instead

Fraccaman pushed a commit that referenced this pull request Feb 20, 2024
* origin/tiago/commit-failure:
  Changelog for #2657
  Document fields in `commit` response
  Bump last processed Eth block before calling `commit_block`
  Remove mut response from `commit`
  Crash if committing a block fails
@tzemanovic tzemanovic merged commit 96fcafc into main Feb 22, 2024
17 checks passed
@tzemanovic tzemanovic deleted the tiago/commit-failure branch February 22, 2024 14:40
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.

4 participants