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 nil handling #10515

Merged
merged 2 commits into from
May 28, 2024
Merged

Fix nil handling #10515

merged 2 commits into from
May 28, 2024

Conversation

wmitsuda
Copy link
Member

A regression was introduced as part of #10402 during files merge process where a nil pointer can crash erigon.

INFO[05-27|15:38:55.343] [snapshots] state merge done accounts(val:148-150, hist:148-150, idx:148-150), storage(val:148-150, hist:148-150, idx:148-150), code(val:148-150, hist:148-150, idx:148-150), commitment(val:148-150)
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x103324dfb]

goroutine 832220 [running]:
github.com/ledgerwatch/erigon-lib/state.(*Aggregator).integrateMergedDirtyFiles(_, {{{0xfc002088040, 0x2, 0x2}, {0xfc002088070, 0x2, 0x2}, {0xfc0020880a0, 0x2, 0x2}, ...}, ...}, ...)
        github.com/ledgerwatch/[email protected]/state/aggregator.go:1526 +0x23b
github.com/ledgerwatch/erigon-lib/state.(*Aggregator).mergeLoopStep(0xc001a12200, {0x10530cca8, 0xc001a0c910})
        github.com/ledgerwatch/[email protected]/state/aggregator.go:685 +0x76a
github.com/ledgerwatch/erigon-lib/state.(*Aggregator).MergeLoop(...)
        github.com/ledgerwatch/[email protected]/state/aggregator.go:697
github.com/ledgerwatch/erigon-lib/state.(*Aggregator).BuildFilesInBackground.func1.1()
        github.com/ledgerwatch/[email protected]/state/aggregator.go:1632 +0x11b
created by github.com/ledgerwatch/erigon-lib/state.(*Aggregator).BuildFilesInBackground.func1 in goroutine 748468
        github.com/ledgerwatch/[email protected]/state/aggregator.go:1625 +0x4d3

@wmitsuda wmitsuda enabled auto-merge (squash) May 28, 2024 00:56
@wmitsuda wmitsuda requested review from awskii and AskAlexSharov May 28, 2024 00:58
Copy link
Collaborator

@AskAlexSharov AskAlexSharov left a comment

Choose a reason for hiding this comment

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

seems we never use filesItemI.i field. let's remove filesItemI class and use *filesItem? (and we will need nil-check)

@wmitsuda
Copy link
Member Author

seems we never use filesItemI.i field. let's remove filesItemI class and use *filesItem? (and we will need nil-check)

hm, true, will remove it

@wmitsuda
Copy link
Member Author

actually, we can remove the extra struct, but need to leave it without the nil check as pre-refactoring, because integrateMergedDirtyFiles rely on being called even if []*filesItem is nil depending on what comes in the in param.

@wmitsuda
Copy link
Member Author

also, do you still need the startJ return value in staticFilesInRange? if not, it can be dropped as well, I can't figure out what that means, I guess it was some legacy debug return value

@wmitsuda
Copy link
Member Author

startJ seems to be propagated up to SelectedStaticFilesV3.dI, but seems unused there as well

@wmitsuda wmitsuda requested a review from AskAlexSharov May 28, 2024 07:03
@awskii
Copy link
Member

awskii commented May 28, 2024

Confirm that we having *I abandoned fields

@awskii awskii dismissed AskAlexSharov’s stale review May 28, 2024 12:46

not allowing to merge with another review

@wmitsuda wmitsuda merged commit df3ca98 into main May 28, 2024
10 checks passed
@wmitsuda wmitsuda deleted the wmitsuda/fix-nil-handling branch May 28, 2024 12:46
@wmitsuda
Copy link
Member Author

Confirm that we having *I abandoned fields

thanks, will do another PR to remove remaining *I fields and dropping startJ returns in order to make the current code clearer.

@AskAlexSharov
Copy link
Collaborator

startJ - I don't remember. if it's unused - then unused.

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.

3 participants