-
Notifications
You must be signed in to change notification settings - Fork 275
fix: Implement correct balance updating logic #1745
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
base: master
Are you sure you want to change the base?
Changes from 11 commits
271f827
88fce35
074ccbb
4b32196
ce9c108
ecc5585
3062088
3f7b7a0
9efe53f
e355e36
997ff94
b40c6a5
e0814ac
b2e1226
dd66770
a75b023
624c067
0476b28
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 |
---|---|---|
|
@@ -4,14 +4,22 @@ | |
package stateupgrade | ||
|
||
import ( | ||
"errors" | ||
"fmt" | ||
"math/big" | ||
|
||
"github.com/ava-labs/libevm/accounts/abi" | ||
"github.com/ava-labs/libevm/common" | ||
"github.com/holiman/uint256" | ||
|
||
"github.com/ava-labs/subnet-evm/params/extras" | ||
) | ||
|
||
var ( | ||
errInsufficientBalanceForSubtraction = errors.New("insufficient balance for subtraction") | ||
errBalanceOverflow = errors.New("balance overflow") | ||
) | ||
|
||
// Configure applies the state upgrade to the state. | ||
func Configure(stateUpgrade *extras.StateUpgrade, chainConfig ChainContext, state StateDB, blockContext BlockContext) error { | ||
isEIP158 := chainConfig.IsEIP158(blockContext.Number()) | ||
|
@@ -30,9 +38,31 @@ func upgradeAccount(account common.Address, upgrade extras.StateUpgradeAccount, | |
state.CreateAccount(account) | ||
} | ||
|
||
// Balance change detected - update the balance of the account | ||
if upgrade.BalanceChange != nil { | ||
balanceChange, _ := uint256.FromBig((*big.Int)(upgrade.BalanceChange)) | ||
state.AddBalance(account, balanceChange) | ||
// BalanceChange is a HexOrDecimal256, which is just a typed big.Int | ||
bigChange := (*big.Int)(upgrade.BalanceChange) | ||
switch bigChange.Sign() { | ||
case 1: // Positive | ||
balanceChange, _ := uint256.FromBig(bigChange) | ||
JonathanOppenheimer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
currentBalance := state.GetBalance(account) | ||
if new(big.Int).Add(currentBalance.ToBig(), balanceChange.ToBig()).Cmp(abi.MaxUint256) > 0 { | ||
JonathanOppenheimer marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
return fmt.Errorf("%w: account %s current balance %s + change %s would exceed maximum uint256", | ||
errBalanceOverflow, account.Hex(), currentBalance.ToBig().String(), balanceChange.ToBig().String()) | ||
} | ||
state.AddBalance(account, balanceChange) | ||
JonathanOppenheimer marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
case -1: // Negative | ||
|
||
absChange := new(big.Int).Abs(bigChange) | ||
balanceChange, _ := uint256.FromBig(absChange) | ||
currentBalance := state.GetBalance(account) | ||
if currentBalance.Cmp(balanceChange) < 0 { | ||
return fmt.Errorf("%w: account %s has %s but trying to subtract %s", | ||
errInsufficientBalanceForSubtraction, | ||
account.Hex(), currentBalance.ToBig().String(), balanceChange.ToBig().String()) | ||
} | ||
state.SubBalance(account, balanceChange) | ||
} | ||
// If zero (Sign() == 0), do nothing | ||
} | ||
if len(upgrade.Code) != 0 { | ||
// if the nonce is 0, set the nonce to 1 as we would when deploying a contract at | ||
|
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. thanks for adding tests! 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. (we should probably add these tests to verification) 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. what do you mean by we should add these tests to verification? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,195 @@ | ||
// Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved. | ||
// See the file LICENSE for licensing terms. | ||
|
||
package stateupgrade | ||
|
||
import ( | ||
"math/big" | ||
"testing" | ||
|
||
"github.com/ava-labs/libevm/accounts/abi" | ||
"github.com/ava-labs/libevm/common" | ||
"github.com/ava-labs/libevm/common/math" | ||
"github.com/ava-labs/libevm/core/rawdb" | ||
"github.com/ava-labs/libevm/core/state" | ||
"github.com/ava-labs/libevm/core/types" | ||
"github.com/holiman/uint256" | ||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/ava-labs/subnet-evm/core/extstate" | ||
"github.com/ava-labs/subnet-evm/params/extras" | ||
) | ||
|
||
func TestUpgradeAccount_BalanceChanges(t *testing.T) { | ||
testAddr := common.HexToAddress("0x1234567890123456789012345678901234567890") | ||
|
||
tests := []struct { | ||
name string | ||
initialBalance *uint256.Int | ||
balanceChange *math.HexOrDecimal256 | ||
accountExists bool | ||
wantBalance *uint256.Int | ||
wantError error | ||
}{ | ||
{ | ||
name: "positive balance change on existing account", | ||
initialBalance: uint256.NewInt(100), | ||
balanceChange: hexOrDecimal256FromInt64(50), | ||
accountExists: true, | ||
wantBalance: uint256.NewInt(150), | ||
}, | ||
{ | ||
name: "negative balance change with sufficient funds", | ||
initialBalance: uint256.NewInt(100), | ||
balanceChange: hexOrDecimal256FromInt64(-30), | ||
accountExists: true, | ||
wantBalance: uint256.NewInt(70), | ||
}, | ||
{ | ||
name: "negative balance change with insufficient funds", | ||
initialBalance: uint256.NewInt(50), | ||
balanceChange: hexOrDecimal256FromInt64(-100), | ||
accountExists: true, | ||
wantBalance: uint256.NewInt(50), // unchanged | ||
wantError: errInsufficientBalanceForSubtraction, | ||
}, | ||
{ | ||
name: "zero balance change", | ||
initialBalance: uint256.NewInt(100), | ||
balanceChange: hexOrDecimal256FromInt64(0), | ||
accountExists: true, | ||
wantBalance: uint256.NewInt(100), | ||
}, | ||
{ | ||
name: "nil balance change", | ||
initialBalance: uint256.NewInt(100), | ||
accountExists: true, | ||
wantBalance: uint256.NewInt(100), | ||
}, | ||
{ | ||
name: "new account with positive balance", | ||
initialBalance: uint256.NewInt(0), | ||
balanceChange: hexOrDecimal256FromInt64(1000), | ||
wantBalance: uint256.NewInt(1000), | ||
}, | ||
{ | ||
name: "new account with negative balance", | ||
initialBalance: uint256.NewInt(0), | ||
balanceChange: hexOrDecimal256FromInt64(-100), | ||
wantBalance: uint256.NewInt(0), // unchanged | ||
wantError: errInsufficientBalanceForSubtraction, | ||
}, | ||
{ | ||
name: "exact balance subtraction", | ||
initialBalance: uint256.NewInt(100), | ||
balanceChange: hexOrDecimal256FromInt64(-100), | ||
accountExists: true, | ||
wantBalance: uint256.NewInt(0), | ||
}, | ||
{ | ||
name: "off-by-one underflow", | ||
initialBalance: uint256.NewInt(100), | ||
balanceChange: hexOrDecimal256FromInt64(-101), | ||
accountExists: true, | ||
wantBalance: uint256.NewInt(100), // unchanged | ||
wantError: errInsufficientBalanceForSubtraction, | ||
}, | ||
{ | ||
name: "large positive balance change", | ||
// Initial balance: 2^255 - 1000 (a very large number near half of max uint256) | ||
initialBalance: uint256.MustFromBig(new(big.Int).Sub(new(big.Int).Lsh(big.NewInt(1), 255), big.NewInt(1000))), | ||
JonathanOppenheimer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
balanceChange: hexOrDecimal256FromInt64(500), | ||
accountExists: true, | ||
// Expected balance: 2^255 - 500 (initial + 500) | ||
wantBalance: uint256.MustFromBig(new(big.Int).Sub(new(big.Int).Lsh(big.NewInt(1), 255), big.NewInt(500))), | ||
}, | ||
{ | ||
name: "balance overflow protection", | ||
initialBalance: uint256.MustFromBig(abi.MaxUint256), // Max uint256 | ||
balanceChange: hexOrDecimal256FromInt64(1), | ||
accountExists: true, | ||
wantBalance: uint256.MustFromBig(abi.MaxUint256), // unchanged | ||
wantError: errBalanceOverflow, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
// Create a fresh state database for each test | ||
statedb := createTestStateDB(t) | ||
|
||
// Set up initial account state | ||
if tt.accountExists { | ||
setAccountBalance(statedb, testAddr, tt.initialBalance) | ||
} | ||
|
||
upgrade := extras.StateUpgradeAccount{ | ||
BalanceChange: tt.balanceChange, | ||
} | ||
err := upgradeAccount(testAddr, upgrade, statedb, false) | ||
require.ErrorIs(t, err, tt.wantError) | ||
if err != nil { | ||
require.ErrorContains(t, err, testAddr.Hex()) | ||
JonathanOppenheimer marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
} | ||
|
||
// Check final balance | ||
actualBalance := statedb.GetBalance(testAddr) | ||
require.Equal(t, tt.wantBalance, actualBalance, "final balance mismatch") | ||
|
||
// Verify account was created if it didn't exist | ||
require.True(t, statedb.Exist(testAddr), "account should exist after upgrade") | ||
}) | ||
} | ||
} | ||
|
||
func TestUpgradeAccount_CompleteUpgrade(t *testing.T) { | ||
statedb := createTestStateDB(t) | ||
addr := common.HexToAddress("0x1234567890123456789012345678901234567890") | ||
|
||
// Create a complete state upgrade with balance change, code, and storage | ||
code := []byte{0xde, 0xad, 0xbe, 0xef} | ||
storageKey := common.HexToHash("0x1234") | ||
storageValue := common.HexToHash("0x5678") | ||
|
||
upgrade := extras.StateUpgradeAccount{ | ||
BalanceChange: hexOrDecimal256FromInt64(1000), | ||
Code: code, | ||
Storage: map[common.Hash]common.Hash{storageKey: storageValue}, | ||
} | ||
require.NoError(t, upgradeAccount(addr, upgrade, statedb, true)) // Test with EIP158 = true | ||
|
||
// Verify all changes were applied | ||
require.Equal(t, uint256.NewInt(1000), statedb.GetBalance(addr)) | ||
require.True(t, statedb.Exist(addr)) | ||
require.Equal(t, uint64(1), statedb.GetNonce(addr)) // Should be set to 1 due to EIP158 and code deployment | ||
|
||
// Cast to access code and storage (these aren't in our StateDB interface) | ||
extStateDB := statedb.(*extstate.StateDB) | ||
require.Equal(t, code, extStateDB.GetCode(addr)) | ||
require.Equal(t, storageValue, extStateDB.GetState(addr, storageKey)) | ||
} | ||
|
||
// createTestStateDB creates a real StateDB instance for testing | ||
func createTestStateDB(t *testing.T) StateDB { | ||
t.Helper() | ||
|
||
db := rawdb.NewMemoryDatabase() | ||
statedb, err := state.New(types.EmptyRootHash, state.NewDatabase(db), nil) | ||
require.NoError(t, err) | ||
|
||
// Wrap it with extstate for predicate support (matching the interface) | ||
return extstate.New(statedb) | ||
} | ||
|
||
// setAccountBalance is a helper to set an account's initial balance for testing | ||
func setAccountBalance(statedb StateDB, addr common.Address, balance *uint256.Int) { | ||
// Cast to extstate.StateDB to access the underlying state | ||
extStateDB := statedb.(*extstate.StateDB) | ||
extStateDB.CreateAccount(addr) | ||
extStateDB.SetBalance(addr, balance) | ||
} | ||
|
||
// Helper function to create HexOrDecimal256 from int64 | ||
func hexOrDecimal256FromInt64(i int64) *math.HexOrDecimal256 { | ||
return (*math.HexOrDecimal256)(big.NewInt(i)) | ||
} |
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.
It's ugly to have this in this package and then have the definition in
config/extras
package. I'm working on a refactor to move them to here https://github.com/ava-labs/subnet-evm/pull/1811/files (this is not a blocker for this one)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! If you'd like it to be a blocker, this PR can certainly wait until after that one (and perhaps that makes more sense).