-
Notifications
You must be signed in to change notification settings - Fork 110
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
base: development
Are you sure you want to change the base?
fix(dot/epoch): Resume node with correct previous next epoch data
and config data
#4105
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #4105 +/- ##
===============================================
+ Coverage 50.51% 55.29% +4.77%
===============================================
Files 230 268 +38
Lines 29006 28403 -603
===============================================
+ Hits 14653 15706 +1053
+ Misses 12856 10888 -1968
- Partials 1497 1809 +312 |
@ramiroJCB super excited about you contributing to gossamer. |
Thank you @kishansagathiya ! |
There was a problem hiding this 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
355e9af
to
82730de
Compare
c3da46a
to
a6aa1bf
Compare
There was a problem hiding this 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!
ab635e0
to
c29d0ba
Compare
…ht when running gossamer
f28ece7
to
5f4ae76
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
There was a problem hiding this 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!
next epoch data
and config data
next epoch data
and config data
next epoch data
and config data
Co-authored-by: JimboJ <[email protected]>
Co-authored-by: JimboJ <[email protected]>
Co-authored-by: Haiko Schol <[email protected]>
Co-authored-by: Haiko Schol <[email protected]>
I love that you guys accept external contributions :) that way I can learn. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @ramiroJCB !!
func getNextEpochOrConfigData[T types.NextConfigDataV1 | types.NextEpochData](iter database.Iterator) ( | ||
*T, uint64, common.Hash, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supernit:
func getNextEpochOrConfigData[T types.NextConfigDataV1 | types.NextEpochData](iter database.Iterator) ( | |
*T, uint64, common.Hash, error) { | |
func getNextEpochOrConfigData[T types.NextConfigDataV1 | types.NextEpochData](iter database.Iterator) ( | |
*T, uint64, common.Hash, error, | |
) { |
} | ||
|
||
func nextConfigDataKey(epoch uint64, hash common.Hash) []byte { | ||
partialKey := fmt.Sprintf("-%d:%s", epoch, hash.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the hyphen in "-%d"
for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, nevermind I see that that the prefix is used in the returned bytes. I would consider not using a hyphen at all so you don't have to split on it in getNextEpochOrConfigData
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff @ramiroJCB!
} | ||
|
||
func nextConfigDataKey(epoch uint64, hash common.Hash) []byte { | ||
partialKey := fmt.Sprintf("-%d:%s", epoch, hash.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, nevermind I see that that the prefix is used in the returned bytes. I would consider not using a hyphen at all so you don't have to split on it in getNextEpochOrConfigData
.
can you fix the deepsource issues? https://app.deepsource.com/gh/ChainSafe/gossamer/run/56bbe9cc-56a8-4028-9b51-56859df56b6a/go/ |
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