Issue refunds based on multi-dimensinal base fee #4082
Conversation
❌ 212 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
MishkaRogachev
force-pushed
the
issue-refunds-based-on-multi-dimensinal-base-fee
branch
from
d448d36 to
31f7320
Compare
MishkaRogachev
force-pushed
the
issue-refunds-based-on-multi-dimensinal-base-fee
branch
from
e5f5f4d to
8c1113f
Compare
MishkaRogachev
force-pushed
the
issue-refunds-based-on-multi-dimensinal-base-fee
branch
4 times, most recently
from
f672189 to
459947c
Compare
|
There is a flaky test |
arbos/l2pricing/model.go
Outdated
| if ps.ArbosVersion < ArbosMultiGasConstraintsVersion { | ||
| baseFeeWei, err := ps.BaseFeeWei() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return new(big.Int).Mul( | ||
| new(big.Int).SetUint64(gasUsed.SingleGas()), | ||
| baseFeeWei, | ||
| ), nil | ||
| } |
There was a problem hiding this comment.
It is risky to keep the old behaviour by returning baseFee*gasUsed. If there is a minor mismatch in the calculations that we are not seeing, there will be a divergence in the chain. Instead, the ArbOS check should be in the EndTxHook, and, if we are in an old ArbOS version, the code shouldn't do any calculations at all. I know that approach doesn't look as clean, but it guarantees we won't mess up.
arbos/l2pricing/model.go
Outdated
| return nil, err | ||
| } | ||
| // Override zero with minimum base fee, | ||
| // it may happen if there is no constraint configured for this resource kind |
There was a problem hiding this comment.
This could not happen unless it is a bug. Also, it is better to set the default as the single-dimensional/max baseFee instead of the minBaseFee, so we don't give free refunds in case there is a bug.
There was a problem hiding this comment.
Yes, the comment is misleading - I'll fix it and replace default with max fee of all constraints.
| baseFee, _ := ps.calcBaseFeeFromExponent(exp) | ||
|
|
||
| // #nosec G115 safe: kind < multigas.NumResourceKind | ||
| _ = ps.multiGasBaseFees.Set(multigas.ResourceKind(kind), baseFee) |
There was a problem hiding this comment.
ResourceKindL1Calldata is a special case that won't have L2 constraints. When computing the L1-calldata gas, the L1-pricing model uses the single-dimensional base fee, which is the max base fee. (Roughly, gasUsed[ResourceKindL1Calldata] = posterFee / baseFee). So, this dimension should be maxBaseFee as well, otherwise we will be refunding almost all L1-calldata gas.
There was a problem hiding this comment.
At present, GrowBacklog does not receive gas in this dimension, and there should be no constraints on L1-calldata (I would suggest prohibiting this in ArbOwner). So I would suggest to work-around it in MultiDimensionalPriceForRefund together with zero-fee check
There was a problem hiding this comment.
At present, GrowBacklog does not receive gas in this dimension, and there should be no constraints on L1-calldata
That is correct.
I would suggest prohibiting this in ArbOwner
I agree.
As a result, the base fee should always be minBaseFee and L1-calldata gas will never be refunded.
We can solve this problem in two ways.
- The first one that I proposed was setting the base fee for this dimension as the maxBaseFee. Then,
MultiDimensionalPriceForRefunddoesn't need to handle any special case and naturally won't give a refund to L1-calldata. - The second one is letting the base fee for this dimension be the min base fee since it won't have any constraint. Then, in
MultiDimensionalPriceForRefund, we should handle this dimension separately to ensure we won't give a refund.
I'm fine with either approach.
| } | ||
| } | ||
|
|
||
| func TestEndTxHookMultiGasRefundNormalTx(t *testing.T) { |
There was a problem hiding this comment.
I think this change also deserves system tests. (Both for the normal case and for retryables).
MishkaRogachev
force-pushed
the
issue-refunds-based-on-multi-dimensinal-base-fee
branch
from
459947c to
63e9b9a
Compare
MishkaRogachev
force-pushed
the
issue-refunds-based-on-multi-dimensinal-base-fee
branch
from
63e9b9a to
cf1867f
Compare
Close NIT-4109
Depends on OffchainLabs/go-ethereum#594
Changes:
MultiDimensionalPriceForRefundto calculate multi-dimensional price (with a test)