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: skip trying to apply blocks from DA with mismatched heights. #594

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

omritoptix
Copy link
Contributor

@omritoptix omritoptix commented Feb 29, 2024

PR Standards

Opening a pull request should be able to meet the following requirements


For Author:

  • Targeted PR against correct branch
  • included the correct type prefix in the PR title
  • Linked to Github issue with discussion and accepted design
  • Targets only one github issue
  • Wrote unit and integration tests
  • All CI checks have passed
  • Added relevant godoc comments

For Reviewer:

  • confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • confirmed all author checklist items have been addressed

After reviewer approval:

  • In case targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@omritoptix omritoptix requested a review from a team as a code owner February 29, 2024 17:52
@omritoptix omritoptix linked an issue Feb 29, 2024 that may be closed by this pull request
@omritoptix omritoptix requested review from srene and mtsitrin February 29, 2024 17:52
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

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

Project coverage is 46.15%. Comparing base (1c5b853) to head (01e010a).

Files Patch % Lines
block/retriever.go 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #594      +/-   ##
==========================================
- Coverage   46.16%   46.15%   -0.01%     
==========================================
  Files         101      101              
  Lines       15560    15562       +2     
==========================================
  Hits         7183     7183              
- Misses       7368     7369       +1     
- Partials     1009     1010       +1     

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

@@ -87,6 +87,9 @@ func (m *Manager) processNextDABatch(ctx context.Context, daMetaData *da.DASubmi

for _, batch := range batchResp.Batches {
for i, block := range batch.Blocks {
if block.Header.Height != m.store.Height()+1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

it's already handled in the applyBlock

if block.Header.Height != m.store.Height()+1 {
		// We crashed after the commit and before updating the store height.
		m.logger.Error("Block not applied. Wrong height", "block height", block.Header.Height, "store height", m.store.Height())
		return nil
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know but it's bad handling because it logs an error as the current assumption is that applyBlock expects to get the correct height - as the comment states this error should be logged upon crash recovery.

This was confusing when trying to debug full node not syncing.

if we get this error logged for every block on the DA which is not in the wrong height it's bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok now I understand the pr
BTW, why would a wrong block be committed to the DA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its not a wrong block committed but if for example there is a batch for 100 blocks and you synced until block 20 and failed, next time you'll run your full node, you'll still get the same 100 blocks batch (as you need 20-100) but you'll start applying from block 1 again instead from block 20.

@omritoptix omritoptix merged commit d2a0a97 into main Mar 4, 2024
3 of 5 checks passed
@omritoptix omritoptix deleted the omritoptix/593-apply-only-relevant-blocks-from-da branch March 4, 2024 14:28
artemijspavlovs added a commit that referenced this pull request Mar 4, 2024
chore: add blocksize metrics

fix: emit block metrics upon creation

chore: typo fixes (#583)

feat: Add data availability validation for Celestia DA #422 (#575)

Co-authored-by: Omri <[email protected]>

chore: Change default block time and batch submit time  (#573)

fix: wrong submitted dapath to dimension hub. client info missing #586 (#587)

chore: add error log on submit batch error (#588)

refactor: retrying failed  availability checks after blob submission before trying to submit again  (#591)

chore: use prometheus metrics provider from options

chore: update go.sum

feat!: Update to dymension v3 (#595)

fix: skip trying to apply blocks from DA with mismatched heights. (#594)

fix: changing dymint rpc to enable queries for unhealthy nodes (#592)

chore: update go.sum

chore: add metrics as a parameter to upon node creation

feat!: Update to dymension v3 (#595)

fix: skip trying to apply blocks from DA with mismatched heights. (#594)

fix: changing dymint rpc to enable queries for unhealthy nodes (#592)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only attempt apply blocks from DA which match height
3 participants