This repository was archived by the owner on Dec 16, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 277
fix: Implement correct balance updating logic #1745
Closed
JonathanOppenheimer
wants to merge
19
commits into
master
from
JonathanOppenheimer/validate-balance-change
Closed
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
271f827
Implement correct balance updating logic
JonathanOppenheimer 88fce35
Trim non-useful comments
JonathanOppenheimer 074ccbb
lint
JonathanOppenheimer 4b32196
Update stateupgrade/state_upgrade_test.go
JonathanOppenheimer ce9c108
Austin suggestions
JonathanOppenheimer ecc5585
Merge branch 'master' into JonathanOppenheimer/validate-balance-change
JonathanOppenheimer 3062088
Austin suggestions
JonathanOppenheimer 3f7b7a0
Apply suggestions from code review
JonathanOppenheimer 9efe53f
nits
JonathanOppenheimer e355e36
lint
JonathanOppenheimer 997ff94
Merge branch 'master' into JonathanOppenheimer/validate-balance-change
JonathanOppenheimer b40c6a5
remove contains check
JonathanOppenheimer e0814ac
Merge branch 'master' into JonathanOppenheimer/validate-balance-change
JonathanOppenheimer b2e1226
Merge branch 'master' into JonathanOppenheimer/validate-balance-change
JonathanOppenheimer dd66770
Merge branch 'master' into JonathanOppenheimer/validate-balance-change
JonathanOppenheimer a75b023
Merge branch 'master' into JonathanOppenheimer/validate-balance-change
JonathanOppenheimer 624c067
Cey suggestions
JonathanOppenheimer 0476b28
Merge branch 'master' into JonathanOppenheimer/validate-balance-change
JonathanOppenheimer 2b94a50
Merge branch 'master' into JonathanOppenheimer/validate-balance-change
JonathanOppenheimer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Collaborator
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!
Collaborator
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)
Member
Author
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? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,173 @@ | ||
| // 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/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 | ||
| }{ | ||
| { | ||
| 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: "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: "exact balance subtraction", | ||
| initialBalance: uint256.NewInt(100), | ||
| balanceChange: hexOrDecimal256FromInt64(-100), | ||
| accountExists: true, | ||
| wantBalance: uint256.NewInt(0), | ||
| }, | ||
| { | ||
| 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))), | ||
| 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 clamped to max uint256", | ||
| // Set initial balance to max uint256 - 100 | ||
| initialBalance: new(uint256.Int).Sub(new(uint256.Int).SetAllOne(), uint256.NewInt(100)), | ||
| balanceChange: hexOrDecimal256FromInt64(200), // This would overflow | ||
| accountExists: true, | ||
| wantBalance: new(uint256.Int).SetAllOne(), // max uint256 | ||
| }, | ||
| { | ||
| name: "balance underflow clamped to zero", | ||
| initialBalance: uint256.NewInt(50), | ||
| balanceChange: hexOrDecimal256FromInt64(-100), // More than current balance | ||
| accountExists: true, | ||
| wantBalance: uint256.NewInt(0), | ||
| }, | ||
| } | ||
|
|
||
| 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, | ||
| } | ||
| upgradeAccount(testAddr, upgrade, statedb, false) | ||
|
|
||
| // 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}, | ||
| } | ||
| 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)) | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/extraspackage. 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).