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

Improve GasEstimateGasLimit to avoid VM.Call failure: not enough gas #4080

Closed
jennijuju opened this issue Sep 29, 2020 · 10 comments
Closed

Improve GasEstimateGasLimit to avoid VM.Call failure: not enough gas #4080

jennijuju opened this issue Sep 29, 2020 · 10 comments

Comments

@jennijuju
Copy link
Member

jennijuju commented Sep 29, 2020

To fix #4054

We think #4054 is caused by the message required more gas than it is allowed(hits the gasLimit). Since most miners reported similar issue didn't customize the gasLimit themselves, the issue should be that GasEstimateGasLimit is not giving a good estimation or not enough extra room.

@jennijuju jennijuju added area/ux Area: UX area/chain/legacyvm Area: Chain/VM labels Sep 29, 2020
@jennijuju jennijuju added need/analysis Hint: Needs Analysis P2 P2: Should be resolved and removed area/ux Area: UX labels Oct 2, 2020
@Stebalien
Copy link
Member

One theory is that the cost of SubmitPoRepForBulkVerify varies over the lifetime of the block as its internal structure changes. The long term solution is to change the on-chain data structures to make the cost of the internal SubmitPoRepForBulkVerify consistent. My current proposal is to store PoReps in a linked list, especially given the fact that this is a temporary datastructure. However, this will need to wait for actors v3.

The short term solution will likely be to bump the gas estimation for this message by some fixed amount.

@Stebalien
Copy link
Member

Note: these are all unconfirmed guesses. The underlying issue may be something totally different.

@Stebalien
Copy link
Member

Ok, I've found a 40% overhead case where a single miner added over 8 sectors in a block. When that happens, the AMT storing the prove commits ends up opening a new layer. This leads to:

  • 1 new block with 8 proofs.
  • 1 new block with one proof.
  • A new root block for the AMT.

Usually, a callback will write ~1 small block with a single proof, then flush the HAMT. In this case, lotus correctly estimated how much the messages would cost, but it got lucky.

If the cost always increased linearly as more messages were added to a block, I'd expect lotus to reliably over estimate gas as the gas estimate assumes that all prior still-enqueued messages will be executed first.

Unfortunately, the cost does not increase linearly as the cost may go down when one leaf of the AMT is filled and a new leaf is opened (because the new leaf will be 8x smaller than the now full leaf). This means the cost will sawtooth.

This works out when all enqueued messages get executed in the same block, but when half get executed in one block and half in another, the gas estimation sawtooth can get offset from reality.

@Stebalien
Copy link
Member

curl https://ipfs.io/ipfs/Qmee2wGgFVHM7dsU5YJwS3xsu5FffRSNChTQy3cRMFiLWA/trace.json | jq '.Trace[] | select(.Msg.From == "f3uuvgrmepryqhcpmxtcucbjsbasvcl4k226lkiyl4v7ko75zqirrbsys7jt3cqxb5jgviubit5jcfkzqbsa6a") | select(.Msg.Method == 7) | .MsgRct.GasUsed'

Result:

41045327
43287811
45379811
47471811
49563811
51655811
53823053
55915053
58271193
43538123
45630123
47722123

@ipfslab
Copy link

ipfslab commented Dec 9, 2020

How to solve this problem? What should I change about config.toml?

@kaptinlin
Copy link

any update?

@wongwinghim
Copy link

My miner crash to the same issue. Is there any thing I can do to resolve it?

@jennijuju
Copy link
Member Author

@ZenGround0 I saw that the linked actor issue has a won't fix label = wondering if we should close this one and apply the same label too?

@Pegasus-starry
Copy link

@jennijuju I have met this issue more frequently these days? I want to know when to solve this problem?

@ZenGround0
Copy link
Contributor

@jennijuju I support tagging this as won't fix too.

@Pegasus-starry the way to avoid this problem altogether is to use ProveCommitAggregate which also has the benefit of costing less gas per prove commit.

@jennijuju jennijuju added status/won't fix and removed P2 P2: Should be resolved need/analysis Hint: Needs Analysis area/chain/legacyvm Area: Chain/VM labels Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants