-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix gas balance validation issue when balance < gas cost < SGT balance #14
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,6 @@ import ( | |
"github.com/ethereum/go-ethereum/crypto/kzg4844" | ||
"github.com/ethereum/go-ethereum/log" | ||
"github.com/ethereum/go-ethereum/params" | ||
"github.com/holiman/uint256" | ||
) | ||
|
||
// L1 Info Gas Overhead is the amount of gas the the L1 info deposit consumes. | ||
|
@@ -253,39 +252,33 @@ func ValidateTransactionWithState(tx *types.Transaction, signer types.Signer, op | |
} | ||
} | ||
// Ensure the transactor has enough funds to cover the transaction costs | ||
var ( | ||
balance = opts.State.GetBalance(from).ToBig() | ||
cost = tx.Cost() | ||
) | ||
|
||
sgtBalance := new(big.Int) | ||
if opts.Chainconfig != nil && opts.Chainconfig.IsOptimism() && opts.Chainconfig.Optimism.UseSoulGasToken { | ||
sgtBalanceSlot := core.TargetSGTBalanceSlot(from) | ||
sgtBalanceValue := opts.State.GetState(types.SoulGasTokenAddr, sgtBalanceSlot) | ||
sgtBalance = new(uint256.Int).SetBytes(sgtBalanceValue[:]).ToBig() | ||
balance, err := core.GetEffectiveGasBalance(opts.State, opts.Chainconfig, from, tx.Value()) | ||
if err != nil { | ||
return fmt.Errorf("%w: balance %v, tx value %v", err, balance, tx.Value()) | ||
} | ||
cost := tx.GasCost() | ||
|
||
if opts.L1CostFn != nil { | ||
if l1Cost := opts.L1CostFn(tx.RollupCostData()); l1Cost != nil { // add rollup cost | ||
cost = cost.Add(cost, l1Cost) | ||
} | ||
} | ||
if balance.Cmp(cost) < 0 && sgtBalance.Cmp(cost) < 0 { | ||
return fmt.Errorf("%w: balance %v, sgt balance:%v, tx cost %v, overshot %v", core.ErrInsufficientFunds, balance, sgtBalance, cost, new(big.Int).Sub(cost, balance)) | ||
if balance.Cmp(cost) < 0 { | ||
return fmt.Errorf("%w: balance %v, tx cost %v, overshot %v", core.ErrInsufficientFunds, balance, cost, new(big.Int).Sub(cost, balance)) | ||
} | ||
// Ensure the transactor has enough funds to cover for replacements or nonce | ||
// expansions without overdrafts | ||
spent := opts.ExistingExpenditure(from) | ||
if prev := opts.ExistingCost(from, tx.Nonce()); prev != nil { | ||
bump := new(big.Int).Sub(cost, prev) | ||
need := new(big.Int).Add(spent, bump) | ||
if balance.Cmp(need) < 0 && sgtBalance.Cmp(need) < 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like we're only handing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. opts.ExistingExpediture() is the function to sum up all |
||
return fmt.Errorf("%w: balance %v, sgt balance:%v, queued cost %v, tx bumped %v, overshot %v", core.ErrInsufficientFunds, balance, sgtBalance, spent, bump, new(big.Int).Sub(need, balance)) | ||
if balance.Cmp(need) < 0 { | ||
return fmt.Errorf("%w: balance %v, queued cost %v, tx bumped %v, overshot %v", core.ErrInsufficientFunds, balance, spent, bump, new(big.Int).Sub(need, balance)) | ||
} | ||
} else { | ||
need := new(big.Int).Add(spent, cost) | ||
if balance.Cmp(need) < 0 && sgtBalance.Cmp(need) < 0 { | ||
return fmt.Errorf("%w: balance %v, sgt balance:%v, queued cost %v, tx cost %v, overshot %v", core.ErrInsufficientFunds, balance, sgtBalance, spent, cost, new(big.Int).Sub(need, balance)) | ||
if balance.Cmp(need) < 0 { | ||
return fmt.Errorf("%w: balance %v, queued cost %v, tx cost %v, overshot %v", core.ErrInsufficientFunds, balance, spent, cost, new(big.Int).Sub(need, balance)) | ||
} | ||
// Transaction takes a new nonce value out of the pool. Ensure it doesn't | ||
// overflow the number of permitted transactions from a single account | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -382,6 +382,15 @@ func (tx *Transaction) Cost() *big.Int { | |
return total | ||
} | ||
|
||
// Cost returns (gas * gasPrice) + (blobGas * blobGasPrice). | ||
func (tx *Transaction) GasCost() *big.Int { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is more neat: |
||
total := new(big.Int).Mul(tx.GasPrice(), new(big.Int).SetUint64(tx.Gas())) | ||
if tx.Type() == BlobTxType { | ||
total.Add(total, new(big.Int).Mul(tx.BlobGasFeeCap(), new(big.Int).SetUint64(tx.BlobGas()))) | ||
} | ||
return total | ||
} | ||
|
||
// RollupCostData caches the information needed to efficiently compute the data availability fee | ||
func (tx *Transaction) RollupCostData() RollupCostData { | ||
if tx.Type() == DepositTxType { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason that we want to relax the txpool filtering condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of
tx.value == acc.balance
, andtx.value > sgtBalance
, passing eitheracc.balance
oracc.sgtBalance
asbalance
to filter the tx because of the conditiontx.Cost() <= balance
. By relaxingbalance = acc.balance + acc.sgtBalance
, the tx will not be filtered out.Note that the filter here can be relaxed: it is not guaranteed that all txs of the same account can be executed successfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the worst case for this relax, will it lead to DOS ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original filter is already relaxed, but definitively we can do more analysis here (or put TODO here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally if the relax happens(when gas limit < actual gas cost), the balance will become zero quickly, so there's no much room for DOS. But now the evil user may deliberately construct transactions with value that can slip into the tx pool and potentially cause DOS.