Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MaxCommitBatch/ MaxPreCommitBatch is too high #10612

Closed
jennijuju opened this issue Apr 3, 2023 · 3 comments · Fixed by #10647
Closed

MaxCommitBatch/ MaxPreCommitBatch is too high #10612

jennijuju opened this issue Apr 3, 2023 · 3 comments · Fixed by #10647
Assignees
Labels
kind/bug Kind: Bug P1 P1: Must be resolved
Milestone

Comments

@jennijuju
Copy link
Member

A couple protocol changes have resulted in PCB and PCA gas usage increase:

That being said, if user creates messages with the previous MaxPreCommitBatch (256) and MaxCommitBatch (819) - the message likely will fail due to message execution failed: exit SysErrOutOfGas as the block limit has reached.

its similar with #8986 #8982, however, again with higher impact from fip0045

we should

  • perform gas benchmarking and adjust the max values (this is set in protocol, we likely should adjust accordingly there as its protocol-impossible to include this many sectors in one message under the current block limit)
  • can start with lower max fees in lotus for now
@jennijuju jennijuju added P1 P1: Must be resolved kind/bug Kind: Bug labels Apr 3, 2023
@arajasek
Copy link
Contributor

arajasek commented Apr 3, 2023

Yes, I think it's time we do this. In addition to "making more sense", I think it's a good idea to have fewer messages taking up (potentially) an entire gas block limit. I think as protocol designers we want such messages to be rare (though they'll always be possible with the FEVM).

@arajasek
Copy link
Contributor

arajasek commented Apr 3, 2023

Would suggest creating a separate actors issue for the first point.

@shrenujbansal
Copy link
Contributor

shrenujbansal commented Apr 5, 2023

Its difficult to reliably get a sensible upper bound for what each of these limits needs to be since gas fees for these messages depends not only on the messages themselves but on the actor state as well, leading to a high variance in the number of sectors that can be aggregated per message
This is the case, even if you split messages for verified and non-verified sectors

My proposal is to simulate PCA/PCB with a StateCall and if we see that the message requires more gas than the block limit, split the batch into 2 and try again continuously until we arrive at a batch size that works

For ProveCommitAggregate, this means calculating the aggregate proofs multiple times. Talking to @magik6k, this should be cheap enough to do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Kind: Bug P1 P1: Must be resolved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants