core/txpool/legacypool: use uint256.Int instead of big.Int #28606
Conversation
weiihann
requested review from
karalabe,
holiman and
rjl493456442
as code owners
|
Just realized this has been done in this PR: #28598 Closing this :) |
|
#28598 doesn't implement these changes (it just touches the legacypool because it changes the statedb API to not use |
weiihann
force-pushed
the
txpool-uint256
branch
from
60e9d06
to
b796c60
Compare
weiihann
force-pushed
the
txpool-uint256
branch
from
b796c60
to
00e93da
Compare
fjl
changed the title
fjl
changed the title
| @@ -198,7 +199,7 @@ func validatePoolInternals(pool *LegacyPool) error { | |||
| if nonce := pool.pendingNonces.get(addr); nonce != last+1 { | |||
| return fmt.Errorf("pending nonce mismatch: have %v, want %v", nonce, last+1) | |||
| } | |||
| if txs.totalcost.Cmp(common.Big0) < 0 { | |||
| if txs.totalcost.Cmp(common.Uint0) < 0 { | |||
There was a problem hiding this comment.
uint256.Int has IsZero, iirc
There was a problem hiding this comment.
oh btw uint256 cannot be negative, so this whole clause needs to be replace dwith something else. Probably overflow-check?
There was a problem hiding this comment.
sorry for the late reply, I've added the new changes to check for overflow
weiihann
force-pushed
the
txpool-uint256
branch
from
00e93da
to
3e1015f
Compare
| @@ -198,8 +200,8 @@ func validatePoolInternals(pool *LegacyPool) error { | |||
| if nonce := pool.pendingNonces.get(addr); nonce != last+1 { | |||
| return fmt.Errorf("pending nonce mismatch: have %v, want %v", nonce, last+1) | |||
| } | |||
| if txs.totalcost.Cmp(common.Big0) < 0 { | |||
| return fmt.Errorf("totalcost went negative: %v", txs.totalcost) | |||
| if txs.totalcost.Cmp(max) > 0 { | |||
There was a problem hiding this comment.
This does not make sense. A uint256.Int is limited between 0 and max (ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff). It is inherently not possible for it to go above max.
Making a change like this, where we swap out a type is not just a matter of making it compile and done. You need to actually spend some time considering the consequences of the changes. Some checks may no longer be required, and in some places maybe we need to add new checks.
There was a problem hiding this comment.
My bad, I should've been more careful. In my opinion, we can remove this check since it's not possible to check for an overflow here.
core/txpool/legacypool/list.go
Outdated
| @@ -456,7 +458,7 @@ func (l *list) LastElement() *types.Transaction { | |||
| // total cost of all transactions. | |||
| func (l *list) subTotalCost(txs []*types.Transaction) { | |||
| for _, tx := range txs { | |||
| l.totalcost.Sub(l.totalcost, tx.Cost()) | |||
| l.totalcost.Sub(l.totalcost, uint256.MustFromBig(tx.Cost())) | |||
There was a problem hiding this comment.
This call to Sub can now overflow, which it could not previously. This can be handled in a couple of ways:
- Ignore it (what this PR does)
- Set to zero on overflow, instead of near infinite
panicon overflow
Not sure what approach is the best, but it's definitely a point to consider
There was a problem hiding this comment.
I think going with panic on overflow is the best approach here. Considering the current uses of subTotalCost, it subtracts the total cost of previously added txs, so it should not be possible to underflow.
weiihann
force-pushed
the
txpool-uint256
branch
3 times, most recently
from
ba22f0a
to
943a771
Compare
weiihann
force-pushed
the
txpool-uint256
branch
from
943a771
to
af03289
Compare
core/txpool/legacypool/list.go
Outdated
| // Otherwise overwrite the old transaction with the current one | ||
| l.txs.Put(tx) | ||
| if cost := tx.Cost(); l.costcap.Cmp(cost) < 0 { | ||
| if cost := uint256.MustFromBig(tx.Cost()); l.costcap.Cmp(cost) < 0 { |
There was a problem hiding this comment.
The Add function is a public function on the list. It should not panic, which it does if you use MustFromBig like this. I'm adding a testcase and a fix
holiman
force-pushed
the
txpool-uint256
branch
from
a01d18e
to
0099b05
Compare
holiman
force-pushed
the
txpool-uint256
branch
from
eae806f
to
a5962f1
Compare
…28606) This change makes the legacy transaction pool use of `uint256.Int` instead of `big.Int`. The changes are made primarily only on the internal functions of legacypool. --------- Co-authored-by: Martin Holst Swende <martin@swende.se>
Related issue: #28577
This PR changes the use of
big.Inttouint256.Intin the legacypool package. The changes are made primarily only on the internal functions of legacypool, without touching other dependencies and related interfaces.