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: check auth changes before processing block #4336

Merged
merged 8 commits into from
Dec 13, 2024

Conversation

dimartiro
Copy link
Contributor

@dimartiro dimartiro commented Nov 21, 2024

Changes

  • Process block headers and data before importing the block
  • Implement vdt for runtime.DigestItem

This changes will fix 2 issues:

  • When we have to apply forced changes for the current block right before processing it.
  • Decoding header digests in grandpa justifications.

Tests

make test

Issues

closes #4281

@dimartiro dimartiro changed the title Fix: check auth changes before processing block @dimartiro Fix: check auth changes before processing block Nov 21, 2024
@dimartiro dimartiro self-assigned this Nov 21, 2024
@dimartiro dimartiro added the P-high this should be addressed ASAP. label Nov 21, 2024
@dimartiro dimartiro force-pushed the dimartiro/grandpa/fix-justification branch from e9a96e7 to 1e52fad Compare November 21, 2024 14:16
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

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

Project coverage is 53.14%. Comparing base (d1ca7aa) to head (18c2431).
Report is 204 commits behind head on development.

Additional details and impacted files
@@               Coverage Diff               @@
##           development    #4336      +/-   ##
===============================================
+ Coverage        50.51%   53.14%   +2.62%     
===============================================
  Files              230      299      +69     
  Lines            29006    38244    +9238     
===============================================
+ Hits             14653    20323    +5670     
- Misses           12856    15988    +3132     
- Partials          1497     1933     +436     
---- 🚨 Try these New Features:

@dimartiro dimartiro force-pushed the dimartiro/grandpa/fix-justification branch from 18c2431 to 720d272 Compare November 25, 2024 10:33
@dimartiro dimartiro force-pushed the dimartiro/grandpa/fix-justification branch from c0750dd to 0167519 Compare November 29, 2024 14:52
@dimartiro dimartiro marked this pull request as ready for review November 29, 2024 15:10
@dimartiro dimartiro added S-sync-westend related to particular network syncing. T-bug this issue covers unexpected and/or wrong behaviour. S-grandpa issues related to block finality. labels Nov 29, 2024
haikoschol
haikoschol previously approved these changes Dec 2, 2024
Copy link
Contributor

@haikoschol haikoschol left a comment

Choose a reason for hiding this comment

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

@dimartiro I was too late to check on Grafana how far the node has synced and my local node hasn't gotten to the problematic block yet. But based on your screenshot on the issue and the code changes, I believe that the issue is fixed.

@dimartiro dimartiro force-pushed the dimartiro/grandpa/fix-justification branch 3 times, most recently from f841e2c to 2d8968e Compare December 5, 2024 15:11
@dimartiro dimartiro force-pushed the dimartiro/grandpa/fix-justification branch from 2c861aa to 248bfab Compare December 9, 2024 13:20
@haikoschol
Copy link
Contributor

The latest deployment stalled at block #1491033. Apart from lots of networking errors, I found this in the logs:

2024-12-10T04:52:45Z CRITICAL current sync strategy failed with: while handling ready block: applying forced changes: cannot find applicable forced change: failed while applying condition: cannot check ancestry: getting header: pebble: not found   pkg=sync

@haikoschol haikoschol dismissed their stale review December 10, 2024 13:33

Lots of changes since my approval. Will do another review once the current issue has been addressed.

@dimartiro
Copy link
Contributor Author

The latest deployment stalled at block #1491033. Apart from lots of networking errors, I found this in the logs:

2024-12-10T04:52:45Z CRITICAL current sync strategy failed with: while handling ready block: applying forced changes: cannot find applicable forced change: failed while applying condition: cannot check ancestry: getting header: pebble: not found   pkg=sync

Yeah, I'm changing the approach and trying with the latest comment from Eclesio

@dimartiro dimartiro force-pushed the dimartiro/grandpa/fix-justification branch from 248bfab to 507d3a9 Compare December 12, 2024 12:58
@dimartiro dimartiro force-pushed the dimartiro/grandpa/fix-justification branch from 507d3a9 to 0d9111a Compare December 13, 2024 12:46
@P1sar P1sar merged commit 34e6d9e into development Dec 13, 2024
24 checks passed
@P1sar P1sar deleted the dimartiro/grandpa/fix-justification branch December 13, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high this should be addressed ASAP. S-grandpa issues related to block finality. S-sync-westend related to particular network syncing. T-bug this issue covers unexpected and/or wrong behaviour.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Westend sync stalls at block #2318338
5 participants