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

handle the case when epoch data is nil #2590

Merged
merged 14 commits into from
Jun 20, 2022

Conversation

kishansagathiya
Copy link
Contributor

Changes

Tests

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

Issues

Fixes #2584

Primary Reviewer

@timwu20

@kishansagathiya kishansagathiya changed the title handle the case when epoch data handle the case when epoch data is nil Jun 8, 2022
@timwu20
Copy link
Contributor

timwu20 commented Jun 8, 2022

Looks like you merged my branch in? I'll merge my branch in to make the diff smaller.

@jimjbrettj
Copy link
Contributor

Does sync unit test fail locally for you? I have it failing remote on my branch as well but it seems to be fine local

@qdm12
Copy link
Contributor

qdm12 commented Jun 16, 2022

@jimjbrettj I think it's because of my bad test sometimes deadlocking, it's fixed in #2605

lib/grandpa/grandpa.go Outdated Show resolved Hide resolved
dot/state/epoch.go Outdated Show resolved Hide resolved
if header != nil && errors.Is(err, chaindb.ErrKeyNotFound) {
epochData, err = s.getEpochDataFromMemory(epoch, header)
if err != nil {
return nil, fmt.Errorf("failed to get epoch data from memory: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit/FYI I realised recently wrapping errors with cannot ... or failed ... add quite a bit of repetition when the call stack is deep, so I would suggest keeping it shorter like

Suggested change
return nil, fmt.Errorf("failed to get epoch data from memory: %w", err)
return nil, fmt.Errorf("get epoch data from memory: %w", err)

Co-authored-by: Quentin McGaw <[email protected]>
@codecov
Copy link

codecov bot commented Jun 16, 2022

Codecov Report

Merging #2590 (9aeebbb) into development (7f55bec) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

@@               Coverage Diff               @@
##           development    #2590      +/-   ##
===============================================
- Coverage        61.95%   61.89%   -0.07%     
===============================================
  Files              215      215              
  Lines            28407    28420      +13     
===============================================
- Hits             17600    17591       -9     
- Misses            9060     9075      +15     
- Partials          1747     1754       +7     
Flag Coverage Δ
unit-tests 61.89% <0.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
dot/state/epoch.go 48.63% <0.00%> (-0.91%) ⬇️
lib/babe/verify.go 91.47% <ø> (ø)
pkg/scale/varying_data_type.go 91.48% <ø> (ø)
dot/network/inbound.go 92.98% <0.00%> (-7.02%) ⬇️
dot/network/block_announce.go 58.40% <0.00%> (-4.81%) ⬇️
lib/grandpa/message.go 61.15% <0.00%> (-2.16%) ⬇️
lib/grandpa/vote_message.go 80.24% <0.00%> (-1.24%) ⬇️
dot/network/notifications.go 65.17% <0.00%> (-1.04%) ⬇️
dot/sync/chain_sync.go 76.75% <0.00%> (+0.62%) ⬆️
dot/network/light.go 86.05% <0.00%> (+0.79%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f55bec...9aeebbb. Read the comment docs.

@kishansagathiya kishansagathiya merged commit 1fa1c65 into development Jun 20, 2022
@kishansagathiya kishansagathiya deleted the kishan/fix/build-blocks-cc-0-9-20 branch June 20, 2022 09:41
@github-actions
Copy link

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

runtime: unknown pc when running a cross client devnet.
5 participants