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

More nil checking in InsertReceiptChain #2036

Merged
merged 1 commit into from
Mar 10, 2023
Merged

Conversation

karlb
Copy link
Contributor

@karlb karlb commented Mar 7, 2023

Fixes #1920

This just applies the change suggested in the issue. It seems reasonable and passed the tests, but I don't understand which situation would cause nil return values, yet.

@akeyless-target-app
Copy link

akeyless-target-app bot commented Mar 7, 2023

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 9546b30

coverage: 49.3% of statements across all listed packages
coverage:  63.0% of statements in consensus/istanbul
coverage:  40.1% of statements in consensus/istanbul/announce
coverage:  54.8% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  24.3% of statements in consensus/istanbul/backend/internal/replica
coverage:  63.3% of statements in consensus/istanbul/core
coverage:  45.0% of statements in consensus/istanbul/db
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  64.4% of statements in consensus/istanbul/uptime
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random
CommentID: 43f4952ca7

@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Patch coverage: 59.62% and project coverage change: -0.60 ⚠️

Comparison is base (e831c00) 54.82% compared to head (e343bf3) 54.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2036      +/-   ##
==========================================
- Coverage   54.82%   54.23%   -0.60%     
==========================================
  Files         688      692       +4     
  Lines       92542   115463   +22921     
==========================================
+ Hits        50737    62621   +11884     
- Misses      37993    49012   +11019     
- Partials     3812     3830      +18     
Impacted Files Coverage Δ
cmd/geth/main.go 20.47% <ø> (+0.47%) ⬆️
cmd/utils/flags.go 2.58% <0.00%> (-0.20%) ⬇️
consensus/istanbul/protocol.go 0.00% <ø> (ø)
contracts/gasprice_minimum/gasprice_minimum.go 47.50% <0.00%> (-12.97%) ⬇️
contracts/testutil/utils.go 25.00% <ø> (-5.00%) ⬇️
core/chain_makers.go 69.93% <0.00%> (+2.43%) ⬆️
core/sys_context.go 77.35% <ø> (-1.22%) ⬇️
core/vm/operations_acl.go 2.94% <ø> (-1.51%) ⬇️
eth/api_backend.go 0.00% <0.00%> (ø)
eth/downloader/queue.go 80.45% <ø> (-0.47%) ⬇️
... and 36 more

... and 610 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@karlb karlb marked this pull request as ready for review March 7, 2023 14:57
@palango palango requested a review from gastonponti March 7, 2023 15:22
@palango
Copy link
Contributor

palango commented Mar 7, 2023

@karlb The upstream geth issues doesn't have a clear solution, did you check what solved it for them?

@karlb
Copy link
Contributor Author

karlb commented Mar 7, 2023

@karlb The upstream geth issues doesn't have a clear solution, did you check what solved it for them?

I don't think they know what solved the issue, the issue is just closed as stale. That part of the code has been changed significantly, so there is no trivial way to cherry-pick a change in either direction.

@palango
Copy link
Contributor

palango commented Mar 7, 2023

I don't think they know what solved the issue, the issue is just closed as stale. That part of the code has been changed significantly, so there is no trivial way to cherry-pick a change in either direction.

So the idea is to add the check and see if it happens again? Maybe we should add some log message then...

@karlb
Copy link
Contributor Author

karlb commented Mar 7, 2023

So the idea is to add the check and see if it happens again?

Unless anyone has a good idea on how to reproduce or explain this, yes. Anything is better than a segfault. Well, anything but a corrupted state, but since we exit the function at a pretty sane point, I think we are safe here.

Maybe we should add some log message then...

Yes, good idea.

@karlb karlb force-pushed the fix-fast-forward-nil-error branch from 8fd35f9 to 9546b30 Compare March 10, 2023 10:28
@github-actions
Copy link

github-actions bot commented Mar 10, 2023

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 244cad0

coverage: 50.7% of statements across all listed packages
coverage:  63.4% of statements in consensus/istanbul
coverage:  40.1% of statements in consensus/istanbul/announce
coverage:  54.7% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  24.3% of statements in consensus/istanbul/backend/internal/replica
coverage:  68.5% of statements in consensus/istanbul/core
coverage:  45.0% of statements in consensus/istanbul/db
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  64.4% of statements in consensus/istanbul/uptime
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random

@github-actions
Copy link

github-actions bot commented Mar 10, 2023

5771 passed, 46 skipped

@karlb karlb force-pushed the fix-fast-forward-nil-error branch from 9546b30 to e343bf3 Compare March 10, 2023 10:45
@karlb karlb merged commit 244cad0 into master Mar 10, 2023
@karlb karlb deleted the fix-fast-forward-nil-error branch March 10, 2023 11:03
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.

Segfault when doing fast forward receipt download
2 participants