-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: sealing: Avoid nil dereference in debug log #9822
Conversation
0c57075
to
a62aa29
Compare
a62aa29
to
4854bef
Compare
I'm trying to manually apply this patch on our production setups as we speak |
Tested on prod machines & fixes issue when applied to 1.18.0 |
thanks @RobQuistNL ! Magik has done. the same and we will try to get this reviewed and do a patch asap! |
if onChainInfo == nil { | ||
return 0, big.Zero(), xerrors.Errorf("sector info for sector %d not found", sn) | ||
} |
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.
Instead of fixing this here, I think I would rather fix GetSector()
in chain/actors/builtin/miner/vX.go
(these are generated files). I think we should return an error from GetSector()
if we fail to find it, instead of the nil, nil
that we're currently returning. It's not idiomatic and is what caused this issue in the first place.
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.
This is documented behaviour, we want some way there to indicate “sector not found” that works over jsonrpc - Im pręty sure other parts of the codebase depend on this behaviour
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.
Did Aayush get the typed json-rpc errors PR done? There are other parts of our code that aren't checking for nil, nil
, such as warmpup.go
that we should also fix if we're going to keep the behaviour the same. We should also throw comments in a couple more places just to make sure as well.
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.
This looks good to me (i would've preferred to only include the panic fix for the patch release, but I think this is correct).
Related Issues
Fixes #9769
Proposed Changes
Don't panic, also improve assigner debug logs
Additional Info
Before the Shark upgrade
checkDealAssignable
would only ever returnfalse
if the sector was an upgrade sector.When that happened we'd try to get sector expiration for the
CC update sector %d cannot fit deal...
debug log, which would panic when we try that on non-upgrade sectors - which can now happen due to the new TermMax handling logic incheckDealAssignable
when the sector has some deals assigned, and we have an incoming deal which can't fit into the sector.Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>
fix: mempool: Introduce a cache for valid signatures
PR type
: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea
, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps