Skip to content
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: add debug_accountRange #17438

Merged
merged 27 commits into from
Jul 13, 2019
Merged

eth: add debug_accountRange #17438

merged 27 commits into from
Jul 13, 2019

Conversation

jwasinger
Copy link
Contributor

@jwasinger jwasinger commented Aug 18, 2018

WIP Adds a debug API method AccountRangeAt which returns all accounts in the state for a given block and transaction index.

eth/api.go Outdated
}

//block hash or number, tx index, start address hash, max results
func (api *PrivateDebugAPI) AccountRangeAt(ctx context.Context, blockNr uint64, txIndex int, startAddr *common.Hash, maxResults int) (AccountRangeAtResult, error) {
Copy link
Contributor Author

@jwasinger jwasinger Aug 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to overload this so that I can have the rpc method accept a string for block number (e.g. "latest")?

@fjl
Copy link
Contributor

fjl commented Aug 18, 2018

I have not looked at the code, but we should think about the naming of the method. Existing *At methods access properties of an account at a specific address. Maybe it should just be debug_accountRange.

@jwasinger
Copy link
Contributor Author

jwasinger commented Aug 18, 2018

CC @cdetrio @gballet @winsvega

@axic
Copy link
Member

axic commented Aug 18, 2018

Existing *At methods access properties of an account at a specific address.

Hm, since it has a starting address, would something like accountRangeBetween or accountRangeFrom be better?

@jwasinger
Copy link
Contributor Author

jwasinger commented Aug 18, 2018

For cpp-eth the method is called debug_accountRangeAt. I think we can look at changing the naming there in order to get this method included in Geth. We are using this method for ewasm testnet integration with https://github.com/gobitfly/etherchain-light .

@@ -131,6 +131,10 @@ func (self *StateDB) Reset(root common.Hash) error {
return nil
}

func (self *StateDB) GetTrie() Trie {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this because I couldn't see any existing way to get access to the trie from the statedb

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do statedb.Database().OpenTrie()

@jwasinger
Copy link
Contributor Author

This also needs unit tests.

eth/api.go Outdated
}

//block hash or number, tx index, start address hash, max results
func (api *PrivateDebugAPI) AccountRangeAt(ctx context.Context, blockNr uint64, txIndex int, startAddr *common.Hash, maxResults int) (AccountRangeAtResult, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure startAddr should not be of common.Address type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right.

@winsvega
Copy link
Contributor

If to remove At.
Then what about accountStorageAt?

@jwasinger
Copy link
Contributor Author

Existing *At methods access properties of an account at a specific address.

Looks like it wouldn't have be renamed.

@jwasinger
Copy link
Contributor Author

Okay, have made suggested changes from @axic and @fjl . Another thing that @cdetrio pointed out was that I needed to be looking up the account key in preimage database to get the account address instead of keccak256(account address).

eth/api.go Outdated
}

blockHash := api.eth.blockchain.GetBlockByNumber(blockNr).Hash()
_, _, statedb, err := api.computeTxEnv(blockHash, txIndex, 0)
Copy link
Contributor Author

@jwasinger jwasinger Aug 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting an error from this line:

> curl -X POST -H 'Content-Type: application/json' --data '{"jsonrpc":"2.0","method":"debug_accountRange","params":[0, 0, "0x0000000000000000000000000000000000000000", 20],"id":1}' localhost:8545

{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"parent 0000000000000000000000000000000000000000000000000000000000000000 not found"}}

Not sure what I'm doing wrong here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a very quick look, and it seems that the message comes from here:

        block := api.eth.blockchain.GetBlockByHash(blockHash)
        if block == nil {
                return nil, vm.Context{}, nil, fmt.Errorf("block %x not found", blockHash)
        }
        parent := api.eth.blockchain.GetBlock(block.ParentHash(), block.NumberU64()-1)
        if parent == nil {
                return nil, vm.Context{}, nil, fmt.Errorf("parent %x not found", block.ParentHash())
        }

it seems normal that block 0's parent will not be found. The GetBlock function uses both the hash and the block number to find the parent, and the block number will be invalid.

@holiman
Copy link
Contributor

holiman commented Aug 19, 2018

On mobile here, so apologies for brevity... We usually use a blocknumOrHash type for specifying blocks, which also accept e.g the string 'latest'. Also, any use of this on mainchain will go OOM pretty quickly, unless a subscription is used instead of a full json dump. Perhaps a default limit should be used; so one doesn't accidentally OOM a node just by checking out "oh I wonder how this method works..."

@axic
Copy link
Member

axic commented Aug 19, 2018

Perhaps a default limit should be used; so one doesn't accidentally OOM a node just by checking out "oh I wonder how this method works..."

Sounds good. Also the debug namespace must be turned on explicitly, correct? So this will not pose a main risk to a regular client.

@cdetrio
Copy link
Member

cdetrio commented Aug 19, 2018

I have not looked at the code, but we should think about the naming of the method. Existing *At methods access properties of an account at a specific address. Maybe it should just be debug_accountRange

I thought the At in debug_storageRangeAt referred to the starting key in the range. But I'm fine with just debug_accountRange.

@jwasinger
Copy link
Contributor Author

blocknumOrHash

@holiman I looked through the codebase but couldn't find this type. Tried variations of capitalization as well.

@holiman
Copy link
Contributor

holiman commented Aug 20, 2018

See

func (bn *BlockNumber) UnmarshalJSON(data []byte) error {
Usage:
func (b *EthAPIBackend) HeaderByNumber(ctx context.Context, blockNr rpc.BlockNumber) (*types.Header, error) {
blockNr rpc.BlockNumber

eth/api.go Outdated Show resolved Hide resolved
eth/api.go Outdated Show resolved Hide resolved
@GitCop
Copy link

GitCop commented Aug 20, 2018

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 4d090fc

  • Commits must be prefixed with the package(s) they modify

  • Commit: a6ca2cb

  • Commits must be prefixed with the package(s) they modify

  • Commit: 6bfaed3

  • Commits must be prefixed with the package(s) they modify

  • Commit: 46cf98d

  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

1 similar comment
@GitCop
Copy link

GitCop commented Aug 20, 2018

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 4d090fc

  • Commits must be prefixed with the package(s) they modify

  • Commit: a6ca2cb

  • Commits must be prefixed with the package(s) they modify

  • Commit: 6bfaed3

  • Commits must be prefixed with the package(s) they modify

  • Commit: 46cf98d

  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

@axic
Copy link
Member

axic commented Aug 21, 2018

Updated this to be compatible with aleth/testeth.

Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made the requested changes, added some unit tests and made sure that the call could be invoked with a nil pointer made sure that it worked with empty results.

@gballet gballet changed the title eth: Add AccountRangeAt to debug API rpc: Add AccountRange to debug API Jul 10, 2019
@fjl fjl changed the title rpc: Add AccountRange to debug API et Jul 13, 2019
@fjl fjl changed the title et eth: add debug_accountRange Jul 13, 2019
@fjl
Copy link
Contributor

fjl commented Jul 13, 2019

@gballet not sure why you renamed this to "rpc: ...". The main change is in package "eth", the commit message prefix "eth" is appropriate.

@fjl fjl merged commit 6bd896a into ethereum:master Jul 13, 2019
@gballet
Copy link
Member

gballet commented Jul 14, 2019

the feature was an RPC call, seems logical enough to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants