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

fix gas balance validation issue when balance < gas cost < SGT balance #14

Merged
merged 3 commits into from
Oct 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions core/state_transition.go
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,41 @@ func (st *StateTransition) GetSoulBalance(account common.Address) *uint256.Int {
return balance
}

// Get the effective balance to pay gas
func GetEffectiveGasBalance(state vm.StateDB, chainconfig *params.ChainConfig, account common.Address, value *big.Int) (*big.Int, error) {
bal, sgtBal := GetGasBalancesInBig(state, chainconfig, account)
if value == nil {
value = big.NewInt(0)
}
if bal.Cmp(value) < 0 {
return nil, ErrInsufficientFundsForTransfer
}
bal.Sub(bal, value)
if bal.Cmp(sgtBal) < 0 {
return sgtBal, nil
}

return bal, nil
}

func GetGasBalances(state vm.StateDB, chainconfig *params.ChainConfig, account common.Address) (*uint256.Int, *uint256.Int) {
balance := state.GetBalance(account)
if chainconfig != nil && chainconfig.IsOptimism() && chainconfig.Optimism.UseSoulGasToken {
sgtBalanceSlot := TargetSGTBalanceSlot(account)
sgtBalanceValue := state.GetState(types.SoulGasTokenAddr, sgtBalanceSlot)
sgtBalance := new(uint256.Int).SetBytes(sgtBalanceValue[:])

return balance, sgtBalance
}

return balance, uint256.NewInt(0)
}

func GetGasBalancesInBig(state vm.StateDB, chainconfig *params.ChainConfig, account common.Address) (*big.Int, *big.Int) {
bal, sgtBal := GetGasBalances(state, chainconfig, account)
return bal.ToBig(), sgtBal.ToBig()
}

func (st *StateTransition) SubSoulBalance(account common.Address, amount *big.Int, reason tracing.BalanceChangeReason) (err error) {
current := st.GetSoulBalance(account).ToBig()
if current.Cmp(amount) < 0 {
Expand Down
8 changes: 4 additions & 4 deletions core/txpool/blobpool/blobpool.go
Original file line number Diff line number Diff line change
Expand Up @@ -667,10 +667,10 @@ func (p *BlobPool) recheck(addr common.Address, inclusions map[common.Hash]uint6
}
// Ensure that there's no over-draft, this is expected to happen when some
// transactions get included without publishing on the network
var (
balance = p.state.GetBalance(addr)
spent = p.spent[addr]
)
balance, sgtBalance := core.GetGasBalances(p.state, p.chain.Config(), addr)
// TODO: we may need a better filter such as tx.value < acc.balance
balance = balance.Add(balance, sgtBalance)
spent := p.spent[addr]
if spent.Cmp(balance) > 0 {
// Evict the highest nonce transactions until the pending set falls under
// the account's available balance
Expand Down
8 changes: 6 additions & 2 deletions core/txpool/legacypool/legacypool.go
Original file line number Diff line number Diff line change
Expand Up @@ -1520,7 +1520,9 @@ func (pool *LegacyPool) promoteExecutables(accounts []common.Address) []*types.T
pool.all.Remove(hash)
}
log.Trace("Removed old queued transactions", "count", len(forwards))
balance := pool.currentState.GetBalance(addr)
balance, sgtBalance := core.GetGasBalances(pool.currentState, pool.chainconfig, addr)
// TODO: we may need a better filter such as tx.value < acc.balance
balance = balance.Add(balance, sgtBalance)
balance = pool.reduceBalanceByL1Cost(list, balance)
// Drop all transactions that are too costly (low balance or out of gas)
drops, _ := list.Filter(balance, gasLimit)
Expand Down Expand Up @@ -1723,7 +1725,9 @@ func (pool *LegacyPool) demoteUnexecutables() {
pool.all.Remove(hash)
log.Trace("Removed old pending transaction", "hash", hash)
}
balance := pool.currentState.GetBalance(addr)
balance, sgtBalance := core.GetGasBalances(pool.currentState, pool.chainconfig, addr)
Copy link

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?

Copy link
Author

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, and tx.value > sgtBalance, passing either acc.balance or acc.sgtBalance as balance to filter the tx because of the condition tx.Cost() <= balance. By relaxing balance = 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it

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 ?

Copy link
Author

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)

Copy link

@blockchaindevsh blockchaindevsh Oct 25, 2024

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.

// TODO: we may need a better filter such as tx.value < acc.balance
balance = balance.Add(balance, sgtBalance)
balance = pool.reduceBalanceByL1Cost(list, balance)
// Drop all transactions that are too costly (low balance or out of gas), and queue any invalids back for later
drops, invalids := list.Filter(balance, gasLimit)
Expand Down
27 changes: 10 additions & 17 deletions core/txpool/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're only handing tx.value for the latest tx, but not for those already queued up; We may also need to accumulate tx.value to handle it completely.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opts.ExistingExpediture() is the function to sum up all tx.Cost(). We should probably separate the cost for tx.Value() and tx.GasCost().

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
Expand Down
9 changes: 9 additions & 0 deletions core/types/transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,15 @@ func (tx *Transaction) Cost() *big.Int {
return total
}

// Cost returns (gas * gasPrice) + (blobGas * blobGasPrice).
func (tx *Transaction) GasCost() *big.Int {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is more neat: return new(big.Int).Sub(tx.Cost(), tx.Value()), what do you think ?

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 {
Expand Down
11 changes: 4 additions & 7 deletions eth/gasestimator/gasestimator.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,15 +71,12 @@ func Estimate(ctx context.Context, call *core.Message, opts *Options, gasCap uin
}
// Recap the highest gas limit with account's available balance.
if feeCap.BitLen() != 0 {
balance := opts.State.GetBalance(call.From).ToBig()
balance, err := core.GetEffectiveGasBalance(opts.State, opts.Config, call.From, call.Value)
if err != nil {
return 0, nil, err
}

available := balance
if call.Value != nil {
if call.Value.Cmp(available) >= 0 {
return 0, nil, core.ErrInsufficientFundsForTransfer
}
available.Sub(available, call.Value)
}
if opts.Config.IsCancun(opts.Header.Number, opts.Header.Time) && len(call.BlobHashes) > 0 {
blobGasPerBlob := new(big.Int).SetInt64(params.BlobTxBlobGasPerBlob)
blobBalanceUsage := new(big.Int).SetInt64(int64(len(call.BlobHashes)))
Expand Down
7 changes: 7 additions & 0 deletions params/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,13 @@ func (c *ChainConfig) Description() string {
if c.InteropTime != nil {
banner += fmt.Sprintf(" - Interop: @%-10v\n", *c.InteropTime)
}
if c.L2BlobTime != nil {
banner += fmt.Sprintf(" - L2BLob: @%-10v\n", *c.L2BlobTime)
}
banner += "\n"
if c.Optimism != nil {
banner += fmt.Sprintf("SGT: %t, Back by native %t", c.Optimism.UseSoulGasToken, c.Optimism.IsSoulBackedByNative)
}
return banner
}

Expand Down