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(dot/epoch): Resume node with correct previous next epoch data and config data #4105

Open
wants to merge 17 commits into
base: development
Choose a base branch
from

Conversation

ramiroJCB
Copy link
Contributor

@ramiroJCB ramiroJCB commented Jul 27, 2024

Changes

Explained better here , we are storing nextEpochData and nextConfigData in disk, so when cases that the information is not in memory gossamer does not enter in a error loop trying to retrieve them.

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

Copy link

codecov bot commented Jul 27, 2024

Codecov Report

Attention: Patch coverage is 56.73077% with 45 lines in your changes missing coverage. Please review.

Project coverage is 55.38%. Comparing base (d1ca7aa) to head (e3f085f).
Report is 159 commits behind head on development.

Additional details and impacted files
@@               Coverage Diff               @@
##           development    #4105      +/-   ##
===============================================
+ Coverage        50.51%   55.38%   +4.86%     
===============================================
  Files              230      269      +39     
  Lines            29006    28496     -510     
===============================================
+ Hits             14653    15782    +1129     
+ Misses           12856    10899    -1957     
- Partials          1497     1815     +318     

@kishansagathiya
Copy link
Contributor

@ramiroJCB super excited about you contributing to gossamer.
Keeping in my that this is still a draft PR, let us know if you are stuck somewhere while working on this. Also, make sure to add which issue you are trying to fix and also some description. That would help us review this better.

@ramiroJCB
Copy link
Contributor Author

@ramiroJCB super excited about you contributing to gossamer. Keeping in my that this is still a draft PR, let us know if you are stuck somewhere while working on this. Also, make sure to add which issue you are trying to fix and also some description. That would help us review this better.

Thank you @kishansagathiya !
I will let you guys know, I think I just need to test this fix feature and it will be done. Probably some minor code changes and it should be finish to make this a full fledge PR.

Copy link
Member

@EclesioMeloJunior EclesioMeloJunior left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution, I've suggested some changes

dot/state/epoch.go Outdated Show resolved Hide resolved
dot/state/epoch.go Outdated Show resolved Hide resolved
dot/state/epoch.go Outdated Show resolved Hide resolved
dot/state/epoch.go Outdated Show resolved Hide resolved
dot/state/epoch.go Outdated Show resolved Hide resolved
dot/state/epoch.go Outdated Show resolved Hide resolved
dot/state/epoch.go Outdated Show resolved Hide resolved
dot/state/epoch.go Outdated Show resolved Hide resolved
dot/state/epoch.go Outdated Show resolved Hide resolved
Copy link
Member

@EclesioMeloJunior EclesioMeloJunior left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the prev points, just a small thing and then I believe it is good to go!

dot/state/epoch.go Outdated Show resolved Hide resolved
@ramiroJCB ramiroJCB force-pushed the ramiroJCB/fix/epochDataNotInDisk branch from ab635e0 to c29d0ba Compare September 4, 2024 14:30
Copy link
Member

@EclesioMeloJunior EclesioMeloJunior left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

dot/state/epoch.go Outdated Show resolved Hide resolved
dot/state/epoch.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jimjbrettj jimjbrettj left a comment

Choose a reason for hiding this comment

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

A few minor comments, but overall looks great!

Love seeing contributions from outside contributed :) We would love to see continued contributions to Gossamer and if we can support you in any way please let us know. Thanks for the great work!

dot/state/epoch.go Outdated Show resolved Hide resolved
dot/state/epoch.go Outdated Show resolved Hide resolved
@EclesioMeloJunior EclesioMeloJunior changed the title fix(epoch):Save Next Epoch data and config and disk fix(dot/epoch): Resume node with previous next epoch data and config data Sep 18, 2024
@EclesioMeloJunior EclesioMeloJunior changed the title fix(dot/epoch): Resume node with previous next epoch data and config data fix(dot/epoch): Resume node with correct previous next epoch data and config data Sep 18, 2024
@ramiroJCB
Copy link
Contributor Author

A few minor comments, but overall looks great!

Love seeing contributions from outside contributed :) We would love to see continued contributions to Gossamer and if we can support you in any way please let us know. Thanks for the great work!

A few minor comments, but overall looks great!

Love seeing contributions from outside contributed :) We would love to see continued contributions to Gossamer and if we can support you in any way please let us know. Thanks for the great work!

I love that you guys accept external contributions :) that way I can learn.
The way I think you guys can support me is helping me find issues that you guys think I could tackle that can teach me about gossamer internals

Copy link
Contributor

@dimartiro dimartiro left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @ramiroJCB !!

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.

Cannot initialize BABE when epoch data is not stored in database
6 participants