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.

[Stebalien]

Basically, it likely doesn't matter much either which way and we might as well keep this as simple as possible. If nobody objects, we might as well leave it on the miner actor.

[rvagg]

I'm not too fussed, my standards on abstraction purity vs cost is a bit lower than some, but actors currently does have fairly nice separation of concerns so we ought to be careful not to break the barriers down too much. I'd like to see what happens to the constants and other building-blocks when we move this around.

Is max_termination_fee just initial_pledge * TERM_FEE_MIN_PLEDGE_MULTIPLE? So TERM_FEE_MIN_PLEDGE_MULTIPLE would need to move to the top-level to be reusable I guess, probably argues for at least this one to go into the Policy but maybe we should put more (or all, including TERMINATION_LIFETIME_CAP).

[Stebalien]

It's a little more involved. This is the function we're talking about:

https://github.com/filecoin-project/builtin-actors/pull/1639/files#diff-fbcb2ec1a9d82b18f146c728cafd643df0e7ae47a04d84be7644913fe89236e5R2224-R2253

[rvagg]

Oh, yuk, so that pulls in all of the pledge_penalty_* cruft which would need to be moved up to somewhere shared .. or something. Yeah, I don't know then, because doing it on the miner actor is pretty weird since it doesn't involve the actor at all.

[Stebalien]

It doesn't involve the miner actor but... it's still "miner actor logic". It's effectively a static function.

Also, this does mean that we can potentially introduce miner-specific parts to the fee (e.g., if we need to charge different fees for different proof types, etc). I can't imagine that ever being an issue, but it's a reason this could make sense.

[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

[momack2]

@Schwartz10 @LesnyRumcajs - this has the requisite approvals now - is it ready to merge?

[LesnyRumcajs]

@Schwartz10 @LesnyRumcajs - this has the requisite approvals now - is it ready to merge?

I believe the termination_fee_percentage exported method should still be removed from the FIP per #1128 (comment) and the Slack thread.

[LesnyRumcajs]

I believe all the outstanding comments were addressed. Mergio?