Remove Batch Fee For Precommit, Lower BatchBalancer and Remove Gas-limited Constraints #1094

irenegia · 2024-12-19T16:39:18Z

We are proposing a FIP to support more batching/aggregation as low hanging fruit to give some onboarding gas saving in the short term

cc @AxCortesCubero @zixuanzh

@AxCortesCubero

We are proposing a FIP to support more batching/aggregation as low hanging fruit to give some onboarding gas saving in the short term cc @AxCortesCubero @zixuanzh

lanzafame

Lacking some reasoning for the chosen numbers for the batchBalancer but otherwise seems good.

FIPS/fip-lowerbatchBal.md

lanzafame · 2024-12-19T23:14:37Z

FIPS/fip-lowerbatchBal.md

+At precommit there are two options: (1) "simple" batching as in precommit (ie, one message for ≥ 2 sectors) where there is a porep proof for each sector in the batch, and (2) batching with aggregation, where there is one unique aggregated porep proof.
+Batching for provecommit is already rational (cost-effective), as the batch fee applies only to aggregated proofs (opposed to precommit, where the batch balancer applies to batching). 
+
+On the other hand, aggregation is rational only when the base fee exceeds 0.065 nanoFIL. Lowering the value of `batchBalancer` from 5 to 2 nanoFIL will reduce the crossover point to 0.026 nanoFIL, making the aggregation rational across a broader range of base fee values.


Why 2 nanoFIL? And why 0.026 nanoFIL for the crossover? Any modelling to back these choices versus other values?

More info here https://docs.google.com/document/d/17FYnKIGf1uf1cCUwfKd30pQpTq8fAux5YghRIlfqrdE/edit?tab=t.0#bookmark=id.4gq3p5g7oyn8

could you @ZX @AxCortesCubero please add a short recap of that to the FIP text?

Yeah, I would like to see the reasoning for the value choice in the FIP so that anyone future person reading this FIP can understand the full context without needing to hunt down gdoc links.

FIPS/fip-lowerbatchBal.md

rvagg · 2024-12-20T02:08:51Z

@irenegia very tiny nit: it might be good to make the casing of "batchBalancer" more consistent across the doc. In the authoritative code (builtin-actors), it's BATCH_BALANCER, in go-state-types it's BatchBalancer. I don't think there's a good reason to camelCase it, perhaps "BatchBalancer" would be best. You do have aggregate_fee in there, which is from builtin-actors and matches.

FIPS/fip-lowerbatchBal.md

jennijuju · 2024-12-21T04:42:46Z

FIPS/fip-lowerbatchBal.md

+fip: "00XX"
+title: Remove Batch Fee For Precommit and Lower batchBalancer
+author: irene, ZX, AX
+discussions-to: https://github.com/filecoin-project/FIPs/discussions/1092


Could you please create a separate discussion dedicated to this proposal - so that it is easier to capture a discussion that is specific to what is proposed here?

FIPS/fip-lowerbatchBal.md

jennijuju · 2024-12-21T05:21:35Z

FIPS/fip-lowerbatchBal.md

+At precommit there are two options: (1) "simple" batching as in precommit (ie, one message for ≥ 2 sectors) where there is a porep proof for each sector in the batch, and (2) batching with aggregation, where there is one unique aggregated porep proof.
+Batching for provecommit is already rational (cost-effective), as the batch fee applies only to aggregated proofs (opposed to precommit, where the batch balancer applies to batching). 
+
+On the other hand, aggregation is rational only when the base fee exceeds 0.065 nanoFIL. Lowering the value of `batchBalancer` from 5 to 2 nanoFIL will reduce the crossover point to 0.026 nanoFIL, making the aggregation rational across a broader range of base fee values.


Suggested change

On the other hand, aggregation is rational only when the base fee exceeds 0.065 nanoFIL. Lowering the value of `batchBalancer` from 5 to 2 nanoFIL will reduce the crossover point to 0.026 nanoFIL, making the aggregation rational across a broader range of base fee values.

On the other hand, aggregation is rational only when the base fee exceeds 0.065 nanoFIL today. Lowering the value of `batchBalancer` from 5 to 2 nanoFIL will reduce the crossover point to 0.026 nanoFIL, making the aggregation rational across a broader range of base fee values.

Can you specify how you get the 0.065 nanoFIL number? If this is true - it sounds like SP client like lotus-miner should change their default immediately or miners should manually update their cocnfig so that aggregation is enabled whenever base fee is higher than 0.065 nanoFIL and start saving (and the network does have >0.065nanoFIL base fee sometimes atm).

The value 0.065 nanoFIL was computed by @zixuanzh using the new cost (gas units) of PCS3, more details can be found here

jennijuju · 2024-12-21T05:32:26Z

FIPS/fip-lowerbatchBal.md

+- [Msg 2](https://www.filutils.com/en/message/bafy2bzaceah7m6jzravjoswo2pljzit36euu3sgz5jzbnpkcfp23b76texiv6): ProveCommitSectors3 for 4 sectors, batched (no aggregation): 96.M/4 = 24M per sector 
+- [Msg 3](https://www.filutils.com/en/message/bafy2bzacedeh74ds4x4l5nlfahmlvwn4obfukhgqnf6rxlaargvsm56sljune): ProveCommitSectors3 for 4 sectors, aggregated[^*]: 217M/4 = 54M per sector 
+
+[^*]: In practice, aggregation is currently giving less gas saving respect to batching due to a bug causing single proofs to be charged an incorrect amount of gas units. So currently, batching is to be preferred with respect to aggregation. Once this code bug is fixed, batching will cost more gas units and aggregation would be the option with the largest gas unit saving per sector. 


Suggested change

[^*]: In practice, aggregation is currently giving less gas saving respect to batching due to a bug causing single proofs to be charged an incorrect amount of gas units. So currently, batching is to be preferred with respect to aggregation. Once this code bug is fixed, batching will cost more gas units and aggregation would be the option with the largest gas unit saving per sector.

[^*]: In practice, aggregation is currently giving less gas saving respect to batching due to a bug causing single proofs to be charged an incorrect amount of gas units. So currently, batching is to be preferred with respect to aggregation. Once this bug in the ref-fvm implementation is addressed, proof aggregation will be the most gas-efficient approach if one can onboard 4 or more sectors at the same time.

can onboard 4 or more sectors at the same time.

technical: @irenegia can you confirm whether this is still correct?

ack, working on that

jennijuju · 2024-12-21T05:41:25Z

On a high level, this FIP is technically sound. However, I think it needs to document how the authors are getting/choosing the numbers mentioned in this FIP better & some editorial fixes.

Co-authored-by: Adrian Lanzafame <[email protected]> Co-authored-by: Jiaying Wang <[email protected]>

Co-authored-by: Rod Vagg <[email protected]> Co-authored-by: Adrian Lanzafame <[email protected]>

Co-authored-by: Jiaying Wang <[email protected]> Co-authored-by: Adrian Lanzafame <[email protected]>

irenegia · 2024-12-23T14:07:36Z

@irenegia very tiny nit: it might be good to make the casing of "batchBalancer" more consistent across the doc. In the authoritative code (builtin-actors), it's BATCH_BALANCER, in go-state-types it's BatchBalancer. I don't think there's a good reason to camelCase it, perhaps "BatchBalancer" would be best. You do have aggregate_fee in there, which is from builtin-actors and matches.

Changed to BatchBalancer everywhere

editorial changes + added links to precommit and provecommit msgs with deals

Ref: #1094 Ref: filecoin-project/builtin-actors#1586 Ref: filecoin-project/builtin-actors#1268 Ref: filecoin-project/lotus#10612

rvagg · 2025-01-07T01:38:03Z

I'm proposing expanding scope of this PR slightly in this PR: #1098

anorth · 2025-01-13T21:42:56Z

This looks pretty good to me too after a final round of comment resolution. Please do open a separate discussion thread dedicated to this specific proposal.

Ref: filecoin-project/FIPs#1094

rvagg · 2025-01-14T06:14:29Z

FIPS/fip-lowerbatchBal.md

+
+
+## Implementation
+Specs-actors PR: TODO


Suggested change

Specs-actors PR: TODO

builtin-actors PR: https://github.com/filecoin-project/builtin-actors/pull/1595

rvagg · 2025-01-14T06:26:55Z

FIPS/fip-lowerbatchBal.md

+- Certain test cases (similar to as FIP24)  may need to be adjusted when changing the BatchBalancer value.
+
+


Suggested change

- Certain test cases (similar to as FIP24) may need to be adjusted when changing the BatchBalancer value.

- Remove test cases from [FIP-0024](./fip-0024.md) that related to testing PreCommit and the 25%/75% split in batch fee between PreCommit and ProveCommit.

- Remove expectations that rely on the `SinglePreCommitGasUsage = 16433324.1825` value originating from [FIP-0024](./fip-0024.md). The remaining gas charge for commits be `SingleProveCommitGasUsage = 49299.972.5475`, i.e. applied only on ProveCommit.

- Decrease test expectations involving the batch balancer in ProveCommit from a value of `5` to `2`.

* Add removal of gas-limited constraints to gas-savings proposal Ref: #1094 Ref: filecoin-project/builtin-actors#1586 Ref: filecoin-project/builtin-actors#1268 Ref: filecoin-project/lotus#10612 * fixup! Add removal of gas-limited constraints to gas-savings proposal

Create fip-lowerbatchBal.md

54574ff

We are proposing a FIP to support more batching/aggregation as low hanging fruit to give some onboarding gas saving in the short term cc @AxCortesCubero @zixuanzh

irenegia requested review from momack2, arajasek, jennijuju, kaitlin-beegle, anorth, raulk, jsoares, TippyFlitsUK and rvagg as code owners December 19, 2024 16:39

Update README.md

627aaa6

lanzafame reviewed Dec 19, 2024

View reviewed changes

rvagg reviewed Dec 20, 2024

View reviewed changes