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.
Export additional built-in miner actor methods for invocation by user actors, so to enable more decentralized DeFi protocol for Storage Provider (SP) services.