fix: gas: estimate gas with a zero base-fee #8991

Stebalien · 2022-07-07T20:16:22Z

Related Issues

Proposed Changes

Estimate gas with a zero base-fee. Otherwise, an account will need funds to estimate the max possible gas a message could take (which is usually the block gas limit).

This does mean gas estimation no longer checks if the sending account has enough funds to cover the message cost, but MpoolPush will now do this. Furthermore, subsystems like the window post worker will carefully pick an account with funds to cover the message. Previously, the fact that gas estimation would fail up-front caused this code to be less useful.

Checklist

Before you mark the PR ready for review, please make sure that:

All commits have a clear commit message.
The PR title is in the form of of <PR type>: <area>: <change being made>
- example: fix: mempool: Introduce a cache for valid signatures
- PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
- area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
This PR has tests for new functionality or change in behaviour
If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
CI is green

Stebalien · 2022-07-07T22:04:06Z

itests/deals_retry_deal_no_funds_test.go

@@ -178,69 +186,3 @@ func TestDealsRetryLackOfFunds_blockInPublishDeal(t *testing.T) {
 	case <-time.After(time.Second * 15):
 	}
 }
-
-func TestDealsRetryLackOfFunds_belowLimit(t *testing.T) {


As far as I can tell, this was testing a bug? We should always allow retries, no matter how low our account balance is.

I don't think that's true -- I believe we explicitly don't auto-retry (can be manually triggered, or happens on restart, per discussion here).

Is this test failing as a result of your change? It may just need to have its funds lowered to, like, 1 attoFIL.

Before: The deal would fail if the account didn't have enough funds to cover the gas, but would wait for a retry call if it had enough funds to cover the message, but not enough funds to process the deal.
Now: The deal will behave the same way in both cases. If we don't have enough funds for any reason, we'll let the user retry.

But yes, we never auto-retry. But that wasn't what this was testing. This was testing whether or not the deal failed if we were below the limit.

Yeah, you seem to be right...

nonsense · 2022-07-08T11:02:55Z

chain/stmgr/call.go

@@ -156,6 +157,10 @@ func (sm *StateManager) CallWithGas(ctx context.Context, msg *types.Message, pri
 	ctx, span := trace.StartSpan(ctx, "statemanager.CallWithGas")
 	defer span.End()

+	// Copy the message as we'll be modifying the nonce.


I don't understand why we need this copy now, and not before?

We needed it before as well, technically. In practice, this doesn't matter because there's exactly one caller of this method (gasEstimateGasLimit) and that caller also copies the message.

Otherwise, an account will need funds to estimate the max possible gas a message could take (which is usually the block gas limit). This does mean gas estimation no longer checks if the sending account has enough funds to cover the message cost, but MpoolPush will now do this.

codecov · 2022-07-08T18:58:17Z

Codecov Report

Merging #8991 (9b75390) into master (a825682) will decrease coverage by 0.06%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master    #8991      +/-   ##
==========================================
- Coverage   40.65%   40.59%   -0.07%     
==========================================
  Files         707      707              
  Lines       78716    78725       +9     
==========================================
- Hits        32004    31960      -44     
- Misses      41242    41286      +44     
- Partials     5470     5479       +9

Impacted Files	Coverage Δ
chain/stmgr/call.go	`67.02% <66.66%> (-6.42%)`	⬇️
node/impl/full/gas.go	`68.96% <100.00%> (+1.72%)`	⬆️
node/impl/full/mpool.go	`47.41% <100.00%> (+0.45%)`	⬆️
storage/sealer/sched_resources.go	`76.19% <0.00%> (-7.62%)`	⬇️
storage/sealer/worker_tracked.go	`80.83% <0.00%> (-6.67%)`	⬇️
chain/events/observer.go	`73.79% <0.00%> (-6.21%)`	⬇️
chain/consensus/filcns/weight.go	`70.58% <0.00%> (-5.89%)`	⬇️
chain/exchange/peer_tracker.go	`66.66% <0.00%> (-4.31%)`	⬇️
chain/consensus/filcns/mine.go	`64.28% <0.00%> (-2.39%)`	⬇️
... and 26 more

arajasek

Thank you for the PR, looks pretty landable, but a couple things.

itests/gas_estimation_test.go

arajasek · 2022-07-10T14:44:10Z

itests/deals_retry_deal_no_funds_test.go

@@ -178,69 +186,3 @@ func TestDealsRetryLackOfFunds_blockInPublishDeal(t *testing.T) {
 	case <-time.After(time.Second * 15):
 	}
 }
-
-func TestDealsRetryLackOfFunds_belowLimit(t *testing.T) {


I don't think that's true -- I believe we explicitly don't auto-retry (can be manually triggered, or happens on restart, per discussion here).

Is this test failing as a result of your change? It may just need to have its funds lowered to, like, 1 attoFIL.

Stebalien · 2022-07-11T15:58:09Z

Addressed CR.

Stebalien force-pushed the fix/estimate-with-zero-base-fee branch 5 times, most recently from 492e3d9 to 8244a6f Compare July 7, 2022 22:02

Stebalien commented Jul 7, 2022

View reviewed changes

Stebalien marked this pull request as ready for review July 8, 2022 01:11

Stebalien requested a review from a team as a code owner July 8, 2022 01:11

Stebalien requested review from nonsense and arajasek July 8, 2022 01:12

nonsense reviewed Jul 8, 2022

View reviewed changes

Stebalien force-pushed the fix/estimate-with-zero-base-fee branch from 8244a6f to d192b82 Compare July 8, 2022 16:47

Stebalien requested a review from magik6k July 8, 2022 18:01

arajasek reviewed Jul 10, 2022

View reviewed changes

address cr

9b75390

arajasek approved these changes Jul 13, 2022

View reviewed changes

arajasek merged commit 57e5aa6 into master Jul 13, 2022

arajasek deleted the fix/estimate-with-zero-base-fee branch July 13, 2022 15:57

arajasek mentioned this pull request Jul 15, 2022

Invalid Insufficient funds err on GasEstimateMessageGas #9035

Closed

18 tasks

TippyFlitsUK mentioned this pull request Aug 11, 2022

1.17.1 Release Planning #9158

Closed

simlecode mentioned this pull request Sep 23, 2022

fix: gas: estimate gas with a zero base-fee filecoin-project/venus#5322

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: gas: estimate gas with a zero base-fee #8991

fix: gas: estimate gas with a zero base-fee #8991

Stebalien commented Jul 7, 2022 •

edited

Loading

Stebalien Jul 7, 2022

arajasek Jul 10, 2022

Stebalien Jul 11, 2022

Stebalien Jul 11, 2022

arajasek Jul 13, 2022

nonsense Jul 8, 2022

Stebalien Jul 8, 2022

codecov bot commented Jul 8, 2022 •

edited

Loading

arajasek left a comment

arajasek Jul 10, 2022

Stebalien commented Jul 11, 2022

fix: gas: estimate gas with a zero base-fee #8991

fix: gas: estimate gas with a zero base-fee #8991

Conversation

Stebalien commented Jul 7, 2022 • edited Loading

Related Issues

Proposed Changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 8, 2022 • edited Loading

Codecov Report

arajasek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Jul 11, 2022

Stebalien commented Jul 7, 2022 •

edited

Loading

codecov bot commented Jul 8, 2022 •

edited

Loading