-
Notifications
You must be signed in to change notification settings - Fork 20.1k
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
eth, internal: Implement getModifiedAccountsBy(Hash|Number) using trie diffs #15512
Conversation
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.
Two questions/suggestions beside the review comments:
- If I understand correctly, the differential iterator can only really find differences between two blocks, but if I lets say send some tokens from account A to B and B sends it back to A, then the token contract itself will not be present in the dump due it it being "clean", even though it changed multiple times internally. Not sure how we can highlight this best in a method doc to ensure people are aware and use it correctly. Similarly pondering whether this could be used to abuse some service relying on it (though arguably that's a bad implementation of said service).
- This may be tied exactly to the above, but I tried an example diff on Rinkeby, and I may or may not be missing an account. Perhaps could you double check?
debug.getDirtyAccountsByNumber(1269158, 1269159)
["0xe096e9ad65e61a4c096c79bdbda450bf7ca755c4", "0x91ab965f15fd0945ecd57ba09e267e41b550fb7a", "0xfc18cbc391de84dbd87db83b20935d3e89f5dd91", "0x0cb510e2f16c36ce039ee3164330d5f00ecf9eac"]
This essentially "diffs" the changes in block https://rinkeby.etherscan.io/block/1269159. That particular block updates the miner account 0xfc18cbc391de84dbd87db83b20935d3e89f5dd91
and contains two txs:
0xfc18cbc391de84dbd87db83b20935d3e89f5dd91
->0x91ab965f15fd0945ecd57ba09e267e41b550fb7a
0x0cb510e2f16c36ce039ee3164330d5f00ecf9eac
->
0x95f35f87608c3a871838f11cbefa8bd76fdf5689
However account 0x95f35f87608c3a871838f11cbefa8bd76fdf5689
isn't reported by the diff. Was it really not modified? It would seems so, but could you perhaps double check because the tx was indeed successful and it did do some SSTOREs.
eth/api.go
Outdated
} | ||
|
||
func (api *PrivateDebugAPI) getDirtyAccounts(startBlock, endBlock *types.Block) ([]common.Address, error) { | ||
oldTrie, err := trie.NewSecure(startBlock.Root(), api.eth.chainDb, 0) |
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.
startBlock
and endBlock
might not exist here if the user specifies a number > head, or a non existent hash. That will cause startBlock.Root()
and endBlock.Root()
to panic.
eth/api.go
Outdated
@@ -636,3 +636,33 @@ func storageRangeAt(st state.Trie, start []byte, maxResult int) StorageRangeResu | |||
} | |||
return result | |||
} | |||
|
|||
func (api *PrivateDebugAPI) GetDirtyAccountsByNumber(startNum, endNum uint64) ([]common.Address, error) { |
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.
Please add a method doc.
eth/api.go
Outdated
return api.getDirtyAccounts(api.eth.blockchain.GetBlockByNumber(startNum), api.eth.blockchain.GetBlockByNumber(endNum)) | ||
} | ||
|
||
func (api *PrivateDebugAPI) GetDirtyAccountsByHash(startHash, endHash common.Hash) ([]common.Address, error) { |
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.
Please add a method doc.
eth/api.go
Outdated
api.eth.blockchain.GetBlockByHash(endHash)) | ||
} | ||
|
||
func (api *PrivateDebugAPI) getDirtyAccounts(startBlock, endBlock *types.Block) ([]common.Address, error) { |
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.
I'd suggest adding a check to ensure startBlock is before endBlock to not be surprising that the inverted parametrization also works.
Also perhaps it would be nice to check that the blocks are on the same chain, though that may be expensive for large chains, so I'm uncertain about that.
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.
Also perhaps it would be nice to check that the blocks are on the same chain, though that may be expensive for large chains, so I'm uncertain about that.
Actually, now that you mention it, it can be another interesting usecase for this; investigating if there double-spends or generally diffs between non-canon blocks and canon-blocks. If the functionality is kept as is.
eth/api.go
Outdated
iter := trie.NewIterator(diff) | ||
var dirty []common.Address | ||
for iter.Next() { | ||
dirty = append(dirty, common.BytesToAddress(newTrie.GetKey(iter.Key))) |
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.
Perhaps the GetKey
part could use some checks if the preimage of something is not available?
Similarly I'd suggest to allow these methods to have a single parameter form too (i.e. make the second parameter optional), in which case internally it would diff the specified block with the one before it. It might be a nice short-hand usecase. |
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.
I'd suggest maybe naming the methods not getDirty..
, but instead getChanged..
or getModified...
.
That makes it more intuitive that if something changes, then changes back, it would not be included. And also, dirty
has connotations about caching, which this isn't about. And other connotations.
PTAL |
eth/api.go
Outdated
// GetModifiedAccountsByHash returns all accounts that have changed between the | ||
// two blocks specified. A change is defined as a difference in nonce, balance, | ||
// code hash, or storage hash. | ||
func (api *PrivateDebugAPI) GetModifiedAccountsByHash(startHash, endHash common.Hash) ([]common.Address, error) { |
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.
Why no single hash version for this?
eth/api.go
Outdated
// two blocks specified. A change is defined as a difference in nonce, balance, | ||
// code hash, or storage hash. | ||
// With one parameter, returns the list of accounts modified in the specified block. | ||
func (api *PrivateDebugAPI) GetModifiedAccountsByNumber(startNum, endNum *uint64) ([]common.Address, error) { |
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.
IMHO keep startNum as uint64
instead of pointers, otherwise not specifying any params will crash hard.
eth/api.go
Outdated
// With one argument, compare x-1 with x | ||
if endNum == nil { | ||
return api.getModifiedAccounts( | ||
api.eth.blockchain.GetBlockByNumber(*startNum - 1), |
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.
If startnum is the genesis, this will get funky :) Perhaps a special handling is needed for that?
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.
> debug.getModifiedAccountsByNumber(0)
Error: Both start block and end block are required
at web3.js:3143:20
at web3.js:6347:15
eth/api.go
Outdated
func (api *PrivateDebugAPI) getModifiedAccounts(startBlock, endBlock *types.Block) ([]common.Address, error) { | ||
if startBlock == nil || endBlock == nil { | ||
return nil, fmt.Errorf("Both start block and end block are required") | ||
} |
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.
You could probably make the two invocations above simpler if you allowed endBlock to be nil
, and if so, the make
endBlock = startBlock
startBlock = startBlock.Parent
Overall I still think it looks good, but some minor beauty-spots:
Correct ^
Don't know what's wrong here:
Ah, I see, I assumed there'd be a single-value-version of |
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.
LGTM
No description provided.