internal/ethapi: recover GetDiffAccounts and GetDiffAccountsWithScope#3516
internal/ethapi: recover GetDiffAccounts and GetDiffAccountsWithScope#3516buddh0 merged 4 commits intobnb-chain:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request recovers two previously removed API functions: GetDiffAccounts and GetDiffAccountsWithScope. These functions provide account difference information for specific blocks by replaying transactions and tracking state changes. The PR also adds a new GetDirtyAccounts method to the StateDB to support retrieving modified accounts.
Changes:
- Recovers
GetDiffAccountsandGetDiffAccountsWithScopeAPI methods for tracking account state changes - Adds helper functions
needToReplayandreplayfor transaction replay logic - Introduces new struct types
DiffAccountsInTxandDiffAccountsInBlockfor result formatting - Adds
GetDirtyAccountsmethod to StateDB to expose mutated accounts
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| internal/ethapi/api.go | Adds GetDiffAccounts and GetDiffAccountsWithScope API methods along with supporting replay and needToReplay helper functions |
| core/state/statedb.go | Adds GetDirtyAccounts method to expose accounts with pending mutations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| BlockHash common.Hash | ||
| Transactions []DiffAccountsInTx | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing documentation for the replay helper function. This is a complex function that performs block replay and computes account diffs. It should have documentation explaining its purpose, parameters, and return values.
| // replay re-executes the given block on top of its parent state and computes | |
| // per-transaction balance diffs for the specified accounts. | |
| // | |
| // The ctx parameter controls cancellation of the replay operation. The block | |
| // parameter is the block to be replayed, and accounts is the list of account | |
| // addresses for which balance changes should be tracked. | |
| // | |
| // It returns: | |
| // - *DiffAccountsInBlock containing the per-transaction balance deltas for | |
| // the requested accounts within the given block; | |
| // - *state.StateDB representing the post-state after successfully replaying | |
| // the block; | |
| // - error if the parent block or its state cannot be retrieved, or if the | |
| // replay itself fails. |
|
|
||
| return &blockAccessList | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing documentation for the GetDirtyAccounts function. Public methods should have documentation explaining their purpose and return values.
| // GetDirtyAccounts returns the set of accounts that have pending mutations | |
| // in the current state, as a slice of their addresses. |
internal/ethapi/api.go
Outdated
| spendValueMap[from] += receipt.GasUsed * tx.GasPrice().Uint64() | ||
| if receipt.Status == types.ReceiptStatusSuccessful { | ||
| spendValueMap[from] += tx.Value().Uint64() | ||
| } | ||
| } | ||
|
|
||
| if tx.To() == nil { | ||
| continue | ||
| } | ||
|
|
||
| if _, exists := accountSet[*tx.To()]; exists && receipt.Status == types.ReceiptStatusSuccessful { | ||
| receiveValueMap[*tx.To()] += tx.Value().Uint64() | ||
| } |
There was a problem hiding this comment.
Potential uint64 overflow when converting big.Int values to uint64. The GasPrice and Value can exceed uint64 limits, and multiplying GasUsed by GasPrice().Uint64() can also overflow. This could lead to incorrect calculation of spendValueMap and receiveValueMap, causing the needToReplay function to return incorrect results.
internal/ethapi/api.go
Outdated
| parentBalance := parentState.GetBalance(account).Uint64() | ||
| currentBalance := currentState.GetBalance(account).Uint64() | ||
| if receiveValueMap[account]-spendValueMap[account] != currentBalance-parentBalance { |
There was a problem hiding this comment.
Potential uint64 overflow when converting account balances. The GetBalance() returns a uint256 value which can exceed uint64 limits. This could lead to incorrect overflow/underflow in the comparison on line 1271, causing needToReplay to return incorrect results when dealing with accounts that have large balances.
| // GetDiffAccountsWithScope returns detailed changes of some interested accounts in a specific block number. | ||
| func (api *BlockChainAPI) GetDiffAccountsWithScope(ctx context.Context, blockNr rpc.BlockNumber, accounts []common.Address) (*DiffAccountsInBlock, error) { | ||
| if api.b.Chain() == nil { | ||
| return nil, errors.New("blockchain not support get diff accounts") |
There was a problem hiding this comment.
Inconsistent error message format. This error uses errors.New() while the similar error on line 1204 uses fmt.Errorf(). For consistency, both should use the same approach, preferably fmt.Errorf() to match the codebase pattern.
| return nil, errors.New("blockchain not support get diff accounts") | |
| return nil, fmt.Errorf("blockchain not support get diff accounts") |
| TxHash common.Hash | ||
| Accounts map[common.Address]*big.Int | ||
| } | ||
|
|
||
| type DiffAccountsInBlock struct { | ||
| Number uint64 | ||
| BlockHash common.Hash |
There was a problem hiding this comment.
Missing documentation for the DiffAccountsInTx struct fields. Public structs should have documentation for each exported field explaining their purpose and format.
| TxHash common.Hash | |
| Accounts map[common.Address]*big.Int | |
| } | |
| type DiffAccountsInBlock struct { | |
| Number uint64 | |
| BlockHash common.Hash | |
| // TxHash is the hash of the transaction whose account balance differences are recorded. | |
| TxHash common.Hash | |
| // Accounts maps account addresses to their net balance change (in wei) caused by the transaction. | |
| // A positive value indicates a gain in balance; a negative value indicates a loss. | |
| Accounts map[common.Address]*big.Int | |
| } | |
| type DiffAccountsInBlock struct { | |
| // Number is the number of the block for which account differences are reported. | |
| Number uint64 | |
| // BlockHash is the hash of the block corresponding to this diff information. | |
| BlockHash common.Hash | |
| // Transactions contains per-transaction account balance differences within the block. |
| TxHash common.Hash | ||
| Accounts map[common.Address]*big.Int | ||
| } | ||
|
|
||
| type DiffAccountsInBlock struct { | ||
| Number uint64 | ||
| BlockHash common.Hash |
There was a problem hiding this comment.
Missing documentation for the DiffAccountsInBlock struct fields. Public structs should have documentation for each exported field explaining their purpose and format.
| TxHash common.Hash | |
| Accounts map[common.Address]*big.Int | |
| } | |
| type DiffAccountsInBlock struct { | |
| Number uint64 | |
| BlockHash common.Hash | |
| // TxHash is the hash of the transaction whose account balance diffs are reported. | |
| TxHash common.Hash | |
| // Accounts maps each tracked account address to its balance difference caused by the transaction. | |
| Accounts map[common.Address]*big.Int | |
| } | |
| type DiffAccountsInBlock struct { | |
| // Number is the height of the block that was replayed. | |
| Number uint64 | |
| // BlockHash is the hash of the block whose transactions were replayed. | |
| BlockHash common.Hash | |
| // Transactions contains the per-transaction account balance diffs within the block. |
| } | ||
| return statedb.GetDirtyAccounts(), nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing documentation for the needToReplay helper function. This is a complex helper function with non-trivial logic that should have documentation explaining its purpose, parameters, and return values.
| // needToReplay determines whether a full state replay is required for the given block | |
| // in order to accurately compute the balance changes of the specified accounts. | |
| // | |
| // It inspects the block's transactions and their receipts, aggregating the gas costs and | |
| // transferred values for any account in the provided list, both as senders and recipients. | |
| // If receipts are missing or inconsistent with the block's transactions, or if a sender | |
| // cannot be derived, an error is returned and the helper signals that replay is not | |
| // possible based solely on receipts. | |
| // | |
| // Parameters: | |
| // - ctx: context used when querying receipts or other chain data, allowing cancellation. | |
| // - block: the block whose transactions and receipts are inspected. | |
| // - accounts: the set of accounts for which potential balance changes are evaluated. | |
| // | |
| // Returns: | |
| // - bool: true if a state replay is required to obtain accurate account changes, false if | |
| // replay is not needed or cannot be determined from receipts alone. | |
| // - error: non-nil if receipts are inconsistent or if any unexpected failure occurs while | |
| // processing the block. |
internal/ethapi/api.go
Outdated
| // GetDiffAccounts returns changed accounts in a specific block number. | ||
| func (api *BlockChainAPI) GetDiffAccounts(ctx context.Context, blockNr rpc.BlockNumber) ([]common.Address, error) { | ||
| if api.b.Chain() == nil { | ||
| return nil, fmt.Errorf("blockchain not support get diff accounts") |
There was a problem hiding this comment.
The error message is grammatically incorrect. It should read "blockchain does not support get diff accounts" instead of "blockchain not support get diff accounts".
| return nil, fmt.Errorf("blockchain not support get diff accounts") | |
| return nil, fmt.Errorf("blockchain does not support get diff accounts") |
| } | ||
|
|
||
| // Apply transaction | ||
| msg, _ := core.TransactionToMessage(tx, signer, parent.Header().BaseFee) |
There was a problem hiding this comment.
The error from TransactionToMessage is silently ignored. While other similar code in the codebase also ignores this error, in the context of a replay function that's computing account differences, a failure to convert a transaction could lead to incorrect results. Consider handling this error explicitly.
| msg, _ := core.TransactionToMessage(tx, signer, parent.Header().BaseFee) | |
| msg, err := core.TransactionToMessage(tx, signer, parent.Header().BaseFee) | |
| if err != nil { | |
| return nil, nil, fmt.Errorf("failed to convert transaction %#x to message: %v", tx.Hash(), err) | |
| } |
Description
internal/ethapi: recover GetDiffAccounts and GetDiffAccountsWithScope
Rationale
GetDiffAccounts was removed during v1.2.15...v1.3.0
GetDiffAccountsWithScope was removed in PR #3199
It's useful for some users, so I put them back but not based on the diff sync feature which is also removed.
Example
Changes
Notable changes: