FIP-TBD: Export sector termination method from miner actor #1035
Conversation
jennijuju
requested review from
momack2,
arajasek,
kaitlin-beegle,
anorth,
raulk,
jsoares and
TippyFlitsUK
as code owners
FIPS/fip-tbd.md
Outdated
| - GetAvailableBalance | ||
| - GetLockedReward | ||
| - GetLockedInitialPledge | ||
| - GetExpectedRewards |
There was a problem hiding this comment.
This depends on
ThisEpochRewardfrom the reward actor.- Some method on the power actor to get the QA power for a miner.
There was a problem hiding this comment.
Likely a bunch of newbie comments. Don't feel like you need to explain things to me if they aren't broadly applicable. I can learn / ask offline..
Also, if it's of use, I'm happy to take a cohesive pass on some of the grammar. I started but realized that may only be worthwhile if this FIP looks to be on track to get accepted.
FIPS/fip-tbd.md
Outdated
|
|
||
| `Vec<TerminationDeclaration>` can be nullable. When it is null, flush/terminate the sectors that are queued up in the cron for termination. | ||
|
|
||
| To prevent the same sectors to be added to the termination queue in the cron and consume block space maliciously, calling this method will immediately terminate the sectors in early_terminations queue first, then execute termination of the sectors submitted in TerminationDeclaration. |
There was a problem hiding this comment.
early_terminations queue
Is there a resource about this we can link to? (I assume not given https://spec.filecoin.io/ being outdate? )
FIPS/fip-tbd.md
Outdated
| ### Operation | ||
|
|
||
| **TerminateSectors** In Miner Actor |
There was a problem hiding this comment.
| ### Operation | |
| **TerminateSectors** In Miner Actor | |
| ### **TerminateSectors** In Miner Actor |
Idea of just making this the heading for this section
|
|
||
| This FIP introduces new builtin actor methods, therefore needs a new actors version shipped in a network upgrade. No breaking changes to existing methods, however. | ||
|
|
||
| ## Test Cases |
There was a problem hiding this comment.
Should we have a TODO to enumerate this out more? I assume we ultimately want a lit of asserts to make in the test cases to make sure everyone is aligned.
|
|
||
| ## Security Considerations | ||
|
|
||
| No new method is being introduced that can be used to attack the network, and bugs in the implementation will only lead to incorrect behavior being observed by user actors. |
There was a problem hiding this comment.
Are we sure? Isn't having a smart contract being able to invoke terminatesector on an SP's behalf a big deal.
There was a problem hiding this comment.
added to the secuirty consideration
There was a problem hiding this comment.
This line doesn't seem right to me at all. The method could be used to attack the network if it has a problem, and bugs could lead to all kinds of unexpected behaviour. But nevertheless, that's not the kind of thing we've bothered writing in any FIP that introduces a new method in the past. This isn't any safer or more dangerous than other FIPs. I don't think this sentence belongs at all.
I would appreciate a grammar review when this fip is closer to “merge draft” stage! I will do a grammar pass myself as well, wanted to get a quick gut check on the ideas first! |
jennijuju
changed the title
|
Have a redesign on the spec - will reopen once updated! |
- address editors initial feedback - update the spec for the new terminating sectors method - remove other apis and will introduce in a separate fip
|
Updated the FIP with the peer feedback received and addressed some editor feedback! I believe this FIP is ready for another round of Peer and editor review now! cc @anorth @Stebalien @filecoin-project/fips-editors @BigLep |
|
|
||
| There are a few motivations to this change: | ||
|
|
||
| - Trustless DeFi - it’s important that users of a DeFi protocol can trust code and not humans to protect their assets. When humans are in the loop, there is necessarily a “judgment call zone”, which violates all aspects of DeFi, it also creates personal safety issues for individuals who are known to process liquidations with no plausible deniability. |
There was a problem hiding this comment.
I am not sure how this statement supports the changes proposed by this FIP. Is the correct interpretation that exporting these actors simply allows for more/better DeFi operations to be executed via code?
Co-authored-by: Kaitlin Beegle <[email protected]>
Co-authored-by: Kaitlin Beegle <[email protected]>
|
|
||
| There are a few motivations to this change: | ||
|
|
||
| - Trustless DeFi - it’s important that users of a DeFi protocol can trust code and not humans to protect their assets. When humans are in the loop, there is necessarily a “judgment call zone”, which violates all aspects of DeFi, it also creates personal safety issues for individuals who are known to process liquidations with no plausible deniability. |
There was a problem hiding this comment.
| - Trustless DeFi - it’s important that users of a DeFi protocol can trust code and not humans to protect their assets. When humans are in the loop, there is necessarily a “judgment call zone”, which violates all aspects of DeFi, it also creates personal safety issues for individuals who are known to process liquidations with no plausible deniability. | |
| - Trustless DeFi - it’s important that users of a DeFi protocol can trust code and not humans to protect their assets. When humans are in the loop, there is necessarily a “judgment call zone”, which violates all aspects of DeFi. It also creates personal safety issues for individuals who are known to process liquidations with no plausible deniability. |
| - Liability concern - from the DeFi protocol creator’s perspective, being an external third party that has to process liquidations off chain (rather than creating a keeper network of liquidators) introduces liability and regulatory concern to the protocol, making it harder to operate in certain jurisdictions | ||
| - Incentive design - by designing their own rulesets for liquidations, application developers can incentivize keepers to protect their protocol in a decentralized manner | ||
|
|
||
| This FIPs propose a builtin actors APIs that can trigger sector termination in smart contracts that staking protocols often need to build offchain logics/oracles for currently. |
There was a problem hiding this comment.
| This FIPs propose a builtin actors APIs that can trigger sector termination in smart contracts that staking protocols often need to build offchain logics/oracles for currently. | |
| This FIP proposes a builtin actors APIs that can trigger sector termination in smart contracts that staking protocols often need to build offchain logics/oracles for currently. |
|
|
||
| **TerminateSectors2** In Miner Actor | ||
|
|
||
| To allow caller (storage providers, stake pools and etc) to have more precise control over the amount of the sectors to be terminated based on their operational needs, this FIP introduces a new `TerminateSector2` method. This method follows most of the behaviour of the existing `TerminateSectors` method (method 9), notably: |
There was a problem hiding this comment.
| To allow caller (storage providers, stake pools and etc) to have more precise control over the amount of the sectors to be terminated based on their operational needs, this FIP introduces a new `TerminateSector2` method. This method follows most of the behaviour of the existing `TerminateSectors` method (method 9), notably: | |
| To allow callers (e.g., storage providers, stake pools) to have more precise control over the amount of the sectors to be terminated based on their operational needs, this FIP introduces a new `TerminateSector2` method. This method follows most of the behaviour of the existing `TerminateSectors` method (method 9), notably: |
|
|
||
| To allow caller (storage providers, stake pools and etc) to have more precise control over the amount of the sectors to be terminated based on their operational needs, this FIP introduces a new `TerminateSector2` method. This method follows most of the behaviour of the existing `TerminateSectors` method (method 9), notably: | ||
| - Only a control address of the miner actor can call this method. | ||
| - Attempt to terminate sectors that are in the current or the next proving deadline will fail with `USR_ILLEGAL_ARGUMENT`. |
There was a problem hiding this comment.
| - Attempt to terminate sectors that are in the current or the next proving deadline will fail with `USR_ILLEGAL_ARGUMENT`. | |
| - Attempts to terminate sectors that are in the current or the next proving deadline will fail with `USR_ILLEGAL_ARGUMENT`. |
| - The method will always process the sector(s) that are already in the `early_termination` queue before processing newly submitted sectors. | ||
|
|
||
| This FIP proposes the following behaviour changes: | ||
| - Currently, `TerminateSectors` allows callers to submit early termination for at most 3000 sectors in one message. `TerminateSectors2` will remove this hardcoded upper bound. Instead, `TerminateSectors2` takes a `max_termination` parameter for caller to specify the maximum number of sectors to be terminated. |
There was a problem hiding this comment.
| - Currently, `TerminateSectors` allows callers to submit early termination for at most 3000 sectors in one message. `TerminateSectors2` will remove this hardcoded upper bound. Instead, `TerminateSectors2` takes a `max_termination` parameter for caller to specify the maximum number of sectors to be terminated. | |
| - Currently, `TerminateSectors` allows callers to submit early termination for at most 3000 sectors in one message. `TerminateSectors2` will remove this hardcoded upper bound. Instead, `TerminateSectors2` takes a `max_termination` parameter for callers to specify the maximum number of sectors to be terminated. |
| - Currently, `TerminateSectors` allows callers to submit early termination for at most 3000 sectors in one message. `TerminateSectors2` will remove this hardcoded upper bound. Instead, `TerminateSectors2` takes a `max_termination` parameter for caller to specify the maximum number of sectors to be terminated. | ||
| - `TerminateSectors2` will always first terminate the sectors that are already in the `early_termination` queue (either added through cron or through `TerminateSectors(2)` messages), then process the new sectors submitted in the message. This prevents the same sectors to be added to the termination queue in the cron and consume block space maliciously. | ||
| - A newly submitted sector will be skipped if it was already in the `early_termination` queue and got executed first, so that the sector will not be terminated twice. | ||
| - `TerminateSectors2` will only terminate sectors up to the number caller specified in the `max_termination` parameter. If there are more sectors in `early_termination` queue and the new sectors submitted in the message than the `max_termination` amount, the message will fail with `USR_ILLEGAL_ARGUMENT`. |
There was a problem hiding this comment.
| - `TerminateSectors2` will only terminate sectors up to the number caller specified in the `max_termination` parameter. If there are more sectors in `early_termination` queue and the new sectors submitted in the message than the `max_termination` amount, the message will fail with `USR_ILLEGAL_ARGUMENT`. | |
| - `TerminateSectors2` will only terminate sectors up to the number specified in the `max_termination` parameter. If there are more sectors in `early_termination` queue and the new sectors submitted in the message than the `max_termination` amount, the message will fail with `USR_ILLEGAL_ARGUMENT`. |
| - `TerminateSectors2` will only terminate sectors up to the number caller specified in the `max_termination` parameter. If there are more sectors in `early_termination` queue and the new sectors submitted in the message than the `max_termination` amount, the message will fail with `USR_ILLEGAL_ARGUMENT`. | ||
| - Caller can query the `early_termination` queue size ahead of submitting the message to determine the maximum number of sectors and/or new sectors it can terminate in one message. | ||
| - That said, `TerminateSectors2` will not add sectors to the termination queue. | ||
| - `TerminateSectors2` will fail with `SYS_OUT_OF_GAS` and abort the all execution if the caller doesn't have enough gas to execute the termination and/or the message itself run out of gas. |
There was a problem hiding this comment.
| - `TerminateSectors2` will fail with `SYS_OUT_OF_GAS` and abort the all execution if the caller doesn't have enough gas to execute the termination and/or the message itself run out of gas. | |
| - `TerminateSectors2` will fail with `SYS_OUT_OF_GAS` and abort all execution if the caller doesn't have enough gas to execute the termination and/or the message itself runs out of gas. |
|
|
||
| ## Backwards Compatibility | ||
|
|
||
| This FIP introduces new builtin actor methods, therefore needs a new actors version shipped in a network upgrade. No breaking changes to existing methods, however, existing methods may be deprecated in future network upgrades. |
There was a problem hiding this comment.
| This FIP introduces new builtin actor methods, therefore needs a new actors version shipped in a network upgrade. No breaking changes to existing methods, however, existing methods may be deprecated in future network upgrades. | |
| This FIP introduces new builtin actor methods, therefore needs a new actors version shipped in a network upgrade. There are no breaking changes to existing methods, however, existing methods may be deprecated in future network upgrades. |
|
|
||
| To prevents the same sectors to be added to the termination queue in the cron and consume block space maliciously with programmatic smart contracts, `TerminateSectors2` will not add any sectors to the cron queue and it will always terminate the sectors that are already in the `early_termination` queue first, then process the new sectors submitted in the message and skip the sectors that are already in the `early_termination` queue. | ||
|
|
||
| While the newly introduced `TerminateSectors2` method allows a smart contract to terminate a sector on SPs behalf, it still requires the SP to add the smart contract as a control address first. When SPs signs and send the message to add smart contract as a control address, this should be considered as a high trust action, as the SP is authorizing the control right to the smart contract. |
There was a problem hiding this comment.
| While the newly introduced `TerminateSectors2` method allows a smart contract to terminate a sector on SPs behalf, it still requires the SP to add the smart contract as a control address first. When SPs signs and send the message to add smart contract as a control address, this should be considered as a high trust action, as the SP is authorizing the control right to the smart contract. | |
| While the newly introduced `TerminateSectors2` method allows a smart contract to terminate a sector on an SP's behalf, it still requires the SP to add the smart contract as a control address first. When a SP signs and sends the message to add smart contract as a control address, this should be considered as a high trust action, as the SP is authorizing the control right to the smart contract. |
| - Currently, `TerminateSectors` allows callers to submit early termination for at most 3000 sectors in one message. `TerminateSectors2` will remove this hardcoded upper bound. Instead, `TerminateSectors2` takes a `max_termination` parameter for caller to specify the maximum number of sectors to be terminated. | ||
| - `TerminateSectors2` will always first terminate the sectors that are already in the `early_termination` queue (either added through cron or through `TerminateSectors(2)` messages), then process the new sectors submitted in the message. This prevents the same sectors to be added to the termination queue in the cron and consume block space maliciously. | ||
| - A newly submitted sector will be skipped if it was already in the `early_termination` queue and got executed first, so that the sector will not be terminated twice. | ||
| - `TerminateSectors2` will only terminate sectors up to the number caller specified in the `max_termination` parameter. If there are more sectors in `early_termination` queue and the new sectors submitted in the message than the `max_termination` amount, the message will fail with `USR_ILLEGAL_ARGUMENT`. |
There was a problem hiding this comment.
I know we discussed this a lot but I have forgotten the various reasoning. Why does this fail if there were no new sectors submitted for termination but the queue is longer than max_terminations? I think we should allow manually terminating part of the queue even if it's too long to ever do in a single message.
Failing if it can't terminate the sectors that were submitted extra (because the queue is too long) is ok
There was a problem hiding this comment.
I think we should allow manually terminating part of the queue even if it's too long to ever do in a single message.
this makes sense.
|
|
||
| ```rust | ||
| pub struct TerminateSectors2Params { | ||
| pub terminations: Option<Vec<TerminationDeclaration>>, |
There was a problem hiding this comment.
Do we need both optional and empty vector? How does the behaviour of an option with Some([]) differ from submitting None? Would just a vector suffice?
| } | ||
|
|
||
| pub struct TerminateSectors2Return { | ||
| // Set to true if all early termination work and new sectors termination work have been completed. Set to false otherwise. |
There was a problem hiding this comment.
This needs more explanation of how this return value could be false and when the entire method call fails instead. Again, I know we discussed this a while ago, but I have forgotten the outcomes.
|
|
||
| ## Security Considerations | ||
|
|
||
| No new method is being introduced that can be used to attack the network, and bugs in the implementation will only lead to incorrect behavior being observed by user actors. |
There was a problem hiding this comment.
This line doesn't seem right to me at all. The method could be used to attack the network if it has a problem, and bugs could lead to all kinds of unexpected behaviour. But nevertheless, that's not the kind of thing we've bothered writing in any FIP that introduces a new method in the past. This isn't any safer or more dangerous than other FIPs. I don't think this sentence belongs at all.
|
Unfortunately, I won't be able to work on this FIP for another couple of weeks. If anyone from the community wants to add to it or take it over, or propose another FIP for this, feel free to! In the meantime, I will covert this PR to a draft and reopen it when i get to pick it up again. |
Export additional built-in miner actor methods for invocation by user actors, so to enable more decentralized DeFi protocol for Storage Provider (SP) services.