fix gas balance validation issue when balance < gas cost < SGT balance #14
Conversation
|
Add more test cases to cover the tx.value > 0 issue. The txpool filtering condition (for the same account) is relaxed a bit but it should still work. Please double check. |
| @@ -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.
I think this is more neat: return new(big.Int).Sub(tx.Cost(), tx.Value()), what do you think ?
| @@ -1723,7 +1725,9 @@ func (pool *LegacyPool) demoteUnexecutables() { | |||
| pool.all.Remove(hash) | |||
| log.Trace("Removed old pending transaction", "hash", hash) | |||
| } | |||
| balance := core.GetEffectiveGasBalance(pool.currentState, pool.chainconfig, addr) | |||
| balance, sgtBalance := core.GetGasBalances(pool.currentState, pool.chainconfig, addr) | |||
There was a problem hiding this comment.
What is the reason that we want to relax the txpool filtering condition?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What's the worst case for this relax, will it lead to DOS ?
There was a problem hiding this comment.
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.
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.
| } | ||
| // 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.
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.
There was a problem hiding this comment.
opts.ExistingExpediture() is the function to sum up all tx.Cost(). We should probably separate the cost for tx.Value() and tx.GasCost().
|
File issue #15 to track the validation issue of multiple txs of the same account. We can close this PR so that it is not a blocking issue. |
Description
The fix aims to address the validation issue during the tx submission when balance < gas cost < SGT balance (see #11), including
Tests
Run a devnet, and be able to