-
Notifications
You must be signed in to change notification settings - Fork 78
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
Swallow deal termination error in cron #1125
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1125 +/- ##
==========================================
+ Coverage 88.33% 88.42% +0.08%
==========================================
Files 103 103
Lines 20409 20417 +8
==========================================
+ Hits 18029 18054 +25
+ Misses 2380 2363 -17
|
d8db36a
to
440e13d
Compare
actors/miner/src/lib.rs
Outdated
))?; | ||
)); | ||
// Intentionally swallow this error to prevent frozen market cron corruption from also freezing this miner cron. | ||
// This is safe from a malicious caller perspective because this method's callstack should always originate with the trusted system caller. |
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.
Please update this comment; it's confusing, given the code below that denies the "should".
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.
How hard would it be to add a unit test for this? I think most of the work would be setting up the state of having a sector with deals, then
- Call terminate sectors from the owner, mock the
ON_MINER_SECTORS_TERMINATE_METHOD
as failing, assert the call fails. - Call
on_deferred_cron_event
/handle_proving_deadline
from the syst,m mock theON_MINER_SECTORS_TERMINATE_METHOD
as failing, assert the call succeeds?
I did the bad thing of merging without test because otherwise I don't think this will make it into code freeze tomorrow and introducing without test is less risky than not introducing at all IMO. Follow up issue #1194 |
Today if market cron freezes over in case of programmer error then any miner actors making use of cron to terminate sectors will also freeze. We should prefer to not do this to make frozen cron easier to recover and less disruptive. We don't gain much from this. Any recovery of frozen cron is going to need a migration and that migration should be able to filter out deals that have terminated since market cron froze. Frozen cron from programmer error isn't esoteric: this is exactly the cause of calibration reset last year. With FVM approaching and new classes of exploits becoming possible it makes sense to do this hardening.