Address changes to FIP 98 while in last call

[Schwartz10]

Updates to FIP 98 while in last call:

• Added 3 special cases to consider when computing termination fees: when termination fee < fault fee, when a sector's age is < 140 days, and when a sector's termination fee is less than some minimum % of its initial pledge
• Added more test cases to address the special cases, as well as a test case for computing aggregate fault fees across multiple sectors
• Define new built-in actor methods to help callers (including FEVM callers by exporting built-in actor methods) to compute termination fees more easily

[LesnyRumcajs]

I believe the test cases section should be expanded as well, especially given 2 new special cases.

[LesnyRumcajs]

Current changes (without exports and more detailed test cases) based on this proposal: https://github.com/filecoin-project/builtin-actors/compare/lesnyrumcajs/fip-0098v2?expand=1

[jsoares]

Probably best for Rod's comments to be addressed before further reviews, but a more general ask: when making changes during last call, it would be advisable to use the PR description to explain what changes are proposed and why.

[Schwartz10]

@rvagg all outstanding comments have been addressed! @jsoares i've updated the description to highlight the changes that were made in last call

[rvagg]

@LesnyRumcajs this probably means this value should be in the actors policy framework rather than just a plain constant.

[LesnyRumcajs]

Is this going to be variable across different networks? I'm not opposed to this, but I'd like to understand what makes some constants get promoted to the actors policy from plain constants.

[Schwartz10]

I'm not the best person to answer where these variables should live, but I don't think it's necessary to have per network values for these variables. Actually seems more useful to always keep them in sync.

[LesnyRumcajs]

Is this correct? The currently proposed bounds in this PR suggest it's the other way around; if the termination fee is larger than the fault_fee * TERM_FEE_MAX_FAULT_FEE_MULTIPLE, it's bound to to that max. If the termination fee is smaller, it gets limited only by the TERM_FEE_MIN_PLEDGE_MULTIPLE

fault_fee = pledge_penalty_for_continued_fault(sector_power)
maximum_fee = fault_fee * TERM_FEE_MAX_FAULT_FEE_MULTIPLE  // (1.05 * fault_fee)
termination_fee = max(minimum_fee, min(age_adjusted_fee, maximum_fee))

[Schwartz10]

@LesnyRumcajs really good catch here - my tired eyes failed me last night. I fixed in a31032e

Noting that the fault_fee and min_termination_fee are not actually discounted by sector day age. Should we be applying the young-sector-age discount to fault fees? I don't think so, but @irenegia might be a good person to weigh in on this

[LesnyRumcajs]

Let's wait for @irenegia, then.

[LesnyRumcajs]

Is there any existing example of such an approach? Is this required?

[Schwartz10]

Are you asking about the max_termination_fee method? I'm not sure I understand what you're asking, but there wouldn't be an example as this FIP is now creating a new way to compute that

From my perspective, this is the single most important part of the fip - it allows FEVM actors to get a collateral value for a miner cc @anorth for visibility here

[LesnyRumcajs]

I mean an example of FEVM native oracle contract.

[Schwartz10]

Oooh, not off the top of my head, but i think the name is fancier than the concept - just a singleton FEVM actor that:

1. Exposes methods like max_termination_fee
2. When called, makes the associated calls to exported built-in actor methods to gather the building blocks to compute the answer
3. Computes the answer and returns it to the caller

In my book, this is "less good" than just including these methods in built-in actors because it will be harder / messier to maintain. Also potentially harder to upgrade in the future, but it is possible

[Stebalien]

Thoughts below but my main question is: how are you planning on using these methods? On-chain? Off-chain? Per-sector? For batches of sectors? That'll impact exactly what the final design should look like.

I'd much prefer to just have a built-in method so we can change it on network upgrades. I'd prefer not to expose any network_projection or termination_fee_percentage methods, instead exposing just a max_termination_fee method (and maybe a single_fault_fee method). I don't want to expose too many of the internals of how these fees are calculated in case they change in the future.

I'd prefer to expose these methods in terms of "given a set of sectors X/Y/Z, what is the max termination fee?", but I think it's probably also safe to do this in terms of power/pledge (we just have to guarantee that we'll never take any other per-sector parameters into account, but I think that's safe).

In terms of accuracy, the max_termination_fee isn't going to be 100% accurate if called for a batch of sectors (sum of power, sum of pledge). It'll still be correct when called sector-by-sector.

[Schwartz10]

@Stebalien thanks for chiming in here - this will be used on-chain to get the collateral value of a miner. It would look something like this in pseudo code:

var minerAddress = 0x0000000000....xxx
var minerID = xxx

var minerBalance = address(minerAddress).balance
var minerIP = precompiles.miner_initial_pledge(minerID)
var minerPower = precompiles.miner_power(minerID)
var minerMaxTermFee = precompiles.max_termination_fee(minerIP, minerPower)

var minerCollateralValue = minerBalance - minerMaxTermFee

var minerDebt = fetchFromContractToGetDebt(minerID)

var isUnderCollateralized = minerDebt > minerCollateralValue

if isUnderCollateralized { ... do something ... }
else { ... do something ...}

Something like that.

It is quite useful to be able to get power and initial pledge for other reasons, but ultimately the most important thing to come from this fip is a supported method to get the max termination fee that is supported by the core devs, upgraded as it needs to, and reports reliable enough info to rely on. It would be easier UX if the method took the minerID instead of IP and power, but IP and power are more composable primitives, per @anorth point in a previous message

[anorth]

I concur with @Stebalien I'd rather not expose the primitive methods, just the useful higher level ones (apologies if I suggested otherwise elsewhere, @Schwartz10). I do recall that I suggested the max_termination_fee method take pledge and power as parameters though; I think that makes it more useful since it can be used for one or more sectors, as well an entire miner, so long as the caller knows or can get the associated power and pledge.

But i'm not wedded to that – happy to let @Stebalien make a final decision.

[anorth]

Unasked question about what actor to put these two final methods on. It'll need to call to both power and reward actor. The power actor would be a reasonable singleton place IMO, but could be convinced otherwise.

[Stebalien]

I agree. So:

1. Add a method to the power actor to get a miner's QA power. We currently only export a method to get the miner's raw byte power (i.e., the amount of space they're proving).
2. Add a method to the miner actor to get the initial pledge.
3. Add a MaxTerminationFee method to the power actor.

[LesnyRumcajs]

It might make sense to add another column - which actor this method belongs to. This would also affect signatures. As I see it:

• power: miner_power,
• miner: miner_initial_pledge,
• ?: termination_fee_percentage, it's just a constant defined in the miner, so I guess there?
• ?: network_projection, could you please point me to a more specific implementation of this?

[Schwartz10]

cc @anorth for help / assistance on this

[anorth]

The network projection couldn't easily be a single call, as different actors have the state for each part:

• Reward actor has the reward estimator
• Power actor has the power estimator

AFAIK the only possible projection_period_start is the current epoch, so that cannot be a parameter to either.

But I think these are probably too low level. The discussion below suggests a maxTerminationPenalty method which sounds better. Whatever actor it's placed on, it will have to call to the reward and power actors to get the estimators.

We don't have prior art for where to put a constant, since actors all just have them compiled in.
Perhaps put the termination fee % on the power actor, as a well-defined singleton. But if @Stebalien or @ZenGround0 suggest something else, go with that. Or just don't expose this constant, abstract over it with a method for computing termination fees.

[anorth]

The network projection couldn't easily be a single call, as different actors have the state for each part:

• Reward actor has the reward estimator
• Power actor has the power estimator

AFAIK the only possible projection_period_start is the current epoch, so that cannot be a parameter to either.

But I think these are probably too low level. The discussion below suggests a maxTerminationPenalty method which sounds better. Whatever actor it's placed on, it will have to call to the reward and power actors to get the estimators.

We don't have prior art for where to put a constant, since actors all just have them compiled in.
Perhaps put the termination fee % on the power actor, as a well-defined singleton. But if @Stebalien or @ZenGround0 suggest something else, go with that. Or just don't expose this constant, abstract over it with a method for computing termination fees.

[anorth]

I concur with @Stebalien I'd rather not expose the primitive methods, just the useful higher level ones (apologies if I suggested otherwise elsewhere, @Schwartz10). I do recall that I suggested the max_termination_fee method take pledge and power as parameters though; I think that makes it more useful since it can be used for one or more sectors, as well an entire miner, so long as the caller knows or can get the associated power and pledge.

But i'm not wedded to that – happy to let @Stebalien make a final decision.

[Schwartz10]

@anorth @Stebalien @LesnyRumcajs I removed some methods that were "low level" in favor of the necessary ones from an FEVM perspective. You could also get rid of termination_fee_percentage here but i think this will become useful to invalidate assumptions if things change. I.e. - if a lending app checks this percentage every so often and it comes back different, we know something in our assumptions has changed, and we might issue an update on our end for handling logic differently. Less important than the others, but seems easy enough to export? Whatever y'all think

[LesnyRumcajs]

If we include this method, should the return type be f64? I'm not sure how big of a deal this is, but it's a floating point type, so it might not return exactly 0.085. I'm not sure we have a better way of handling those anywhere. Perhaps it should return the numerator and denominator (how it is handled in the code).

[LesnyRumcajs]

@Schwartz10 I went ahead with my suggestion in filecoin-project/builtin-actors#1639. Does this work for you? Is everyone in agreement that this method should be there at all?

[Schwartz10]

Good point - i dont mind returning two values for num and denom, but whatever those values return will likely end up getting converted into uint256 types in solidity, so ideally each returned number is convertible into uint256

[LesnyRumcajs]

They are u32 for now, so it should be fine.

[Schwartz10]

Updated 074cc7d

[LesnyRumcajs]

Given miner_initial_pledge lives under miner, it probably makes sense to rename it to initial_pledge and omit the parameters (the parameter is implicitly this/self). But maybe it's an implementation detail and doesn't matter in the context of this FIP.

[Schwartz10]

from the context of the fevm smart contracts we definitely want to be able to pass a parameter

[LesnyRumcajs]

Could you please elaborate? Why should it have a miner_id parameter if it's a method of a particular miner? To my understanding, it's the same as, e.g., get_peer_id, which doesn't take any parameters. Or should it live as a method of another actor?

[Schwartz10]

Actually, i think you are right - i think you would make a solidity wrapper method that passes the miner ID like we do here. Fixed in f5bd8e9