Skip to content

eth, internal/web3ext: implement storageRangeAt#3407

Closed
obscuren wants to merge 2 commits into
ethereum:masterfrom
obscuren:storage-at
Closed

eth, internal/web3ext: implement storageRangeAt#3407
obscuren wants to merge 2 commits into
ethereum:masterfrom
obscuren:storage-at

Conversation

@obscuren
Copy link
Copy Markdown
Contributor

@obscuren obscuren commented Dec 5, 2016

StorageAt is currently limited and requires needless iterations due to
the limitation of the trie iterator. At present we can't start
iterating from any given key, thus we need to loop over any key
that's not inclusive in the range of start and end.

@obscuren obscuren added this to the 1.5.5 milestone Dec 5, 2016
@mention-bot
Copy link
Copy Markdown

@obscuren, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Arachnid, @tgerring and @Gustav-Simonsson to be potential reviewers.

@obscuren obscuren force-pushed the storage-at branch 2 times, most recently from 95f570e to 9c7a7c1 Compare December 5, 2016 23:12
@obscuren
Copy link
Copy Markdown
Contributor Author

obscuren commented Dec 5, 2016

var h = eth.getBlock(eth.blockNumber-2).hash;
var a = "0xFA7B9770Ca4cb04296Cac84F37736d4041251CDF";
var s = "0x0000000000000000000000000000000000000000000000000000000000000001";
var e = "0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff";
debug.storageRangeAt(h, 0, a, s, e, 100)

returns:

{
    complete: true,
    storage: {
      0x0000000000000000000000000000000000000000000000000000000000000001: "0x0000000000000000000000000000000000000000000000000000000000000000",
      0x0000000000000000000000000000000000000000000000000000000000000008: "0x0000000000000000000000000000000000000000000000000000000000000000",
      0x1632fdd11d2f42e69bbb9114d9cdb236c6e3dde438ce903a816e54a5c534f730: "0x0000000000000000000000000000000000000000000000000000000000000000",
      ...
      0xf3f7a9fe364faab93b216da50a3214154f22a0a2b415b23a84c8169e8b636f00: "0x0000000000000000000000000000000000000000000000000000000000000000",
      0xf3f7a9fe364faab93b216da50a3214154f22a0a2b415b23a84c8169e8b636f01: "0x0000000000000000000000000000000000000000000000000000000000000000"
    }
}

@obscuren obscuren added the review label Dec 5, 2016
@fjl
Copy link
Copy Markdown
Contributor

fjl commented Dec 6, 2016

Please rename to storageRangeAt. You also need to rebase because the environment PR is merged.

@obscuren obscuren force-pushed the storage-at branch 3 times, most recently from bf4b982 to 9373fb8 Compare December 6, 2016 21:45
StorageAt is currently limited and requires needless iterations due to
the limitation of the trie iterator. At present we can't start
iterating from any given key, thus we need to loop over any key
that's not inclusive in the range of start and end.
@zelig
Copy link
Copy Markdown
Contributor

zelig commented Dec 6, 2016

@obscuren this should work in the light client odr as well right?

fjl
fjl previously requested changes Dec 7, 2016
Comment thread eth/api.go
}
stateDb.DeleteSuicides()
continue
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please extract this code into its own function. This basically recomputes the state up to a certain transaction
and is duplicated in the tracing code just above.

Comment thread eth/api.go
trie := stateObject.GetTrie(api.eth.ChainDb())
it := trie.Iterator()

for it.Next() && len(result.Storage) < maxResult {
Copy link
Copy Markdown
Contributor

@fjl fjl Dec 7, 2016

Choose a reason for hiding this comment

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

would be better to use ForEachStorage here instead of exposing the trie

@fjl fjl changed the title eth, internal/web3ext: implement storageAt eth, internal/web3ext: implement storageRangeAt Dec 8, 2016
@fjl
Copy link
Copy Markdown
Contributor

fjl commented Dec 11, 2016

I've done the updates myself. The current version is pretty useless because it keeps the first n keys hit by the random iteration order. It'd be better to keep a sorted buffer so the function always returns the n lowest keys instead. I'll add that next.

@fjl fjl dismissed their stale review December 11, 2016 01:37

I'm taking over the PR because @obscuren is sick.

@karalabe
Copy link
Copy Markdown
Member

Postponed to 1.5.6

@karalabe karalabe modified the milestones: 1.5.6, 1.5.5 Dec 14, 2016
@fjl fjl force-pushed the storage-at branch 2 times, most recently from 7c101dc to cff315d Compare December 21, 2016 22:52
@fjl
Copy link
Copy Markdown
Contributor

fjl commented Dec 21, 2016

@obscuren Please review this.

The API has changed and is now correct in cases where pre-images of storage keys are missing. In your initial implementation, such keys would override the entry for 0x0000..00. I have discussed this change with @chriseth and @yann300.

The other bug that I fixed is the one mentioned above: your implementation kept the first N matching keys found during random iteration. This caused random gaps in the result, making the API pretty useless for actually syncing storage to the client. The new implementation returns the first N keys from the starting key out of all keys in storage. Clients can use this to download full storage incrementally by requesting from 0x00 with limit 100, then using the last key + 1 as the start key for the next call.

Example (on ropsten):

POST http://127.0.0.1:8545/

{
  "jsonrpc": "2.0",
  "id": 1,
  "method": "debug_storageRangeAt",
  "params": [
    "0xced82545131cdb6253f183a62dc3eaf81259be5cc744767798bcfb95664dc32c",
    4,
    "0xB4A5BB81C6962Fa87Af6886ABE59b3cc56ec44b8",
    "0x",
    "0x2b",
    8
  ]
}
{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "storage": [
      {
        "key": "0x0000000000000000000000000000000000000000000000000000000000000000",
        "hashedKey": "0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563",
        "value": "0x0000000000000000000000945e237bf1540eea146f5e830593d1d54c279616a9"
      },
      {
        "key": "0x0000000000000000000000000000000000000000000000000000000000000002",
        "hashedKey": "0x405787fa12a823e0f2b7631cc41b3ba8828b3321ca811111fa75cd3aa3bb5ace",
        "value": "0x0000000000000000000000000000000000000000000000000000000000000008"
      },
      {
        "key": "0x0000000000000000000000000000000000000000000000000000000000000003",
        "hashedKey": "0xc2575a0e9e593c00f959f8c92f12db2869c3395a3b0502d05e2516446f71f85b",
        "value": "0x0000000000000000000000000000000000000000000000000000000000000008"
      },
      {
        "key": "0x0eaf5590d73ba7d29089223577cbbfcf0e9f3b40702705a67cfc2acdeab681b9",
        "hashedKey": "0x69a39e3a308067b56e0f21a33483b53eab40c7ea8fa8901cf9637115581b85f0",
        "value": "0x0000000000000000000000000000000000000000000000000000000000000002"
      },
      {
        "key": "0x28b1076c273e1493e8c4dd35b96250b60e5baa02d8c3dc7a1b4f42ddcc803c6d",
        "hashedKey": "0x9c982a433270bb03dbacf4a166ace8c9355e99a1abba9946408031846845060b",
        "value": "0x0000000000000000000000000000000000000000000000000000000000000003"
      },
      {
        "key": "0x2a359015afbfbe3b36db751c88f03aed63209927e8cf9ca7f0b69158e29e28f5",
        "hashedKey": "0x1d3954ea05fac9c4f44f290d32ed11be13922e2c8162b3594bb3bb0405546e8e",
        "value": "0x0000000000000000000000000000000000000000000000000000000000000004"
      }
    ],
    "complete": false
  }
}

@fjl
Copy link
Copy Markdown
Contributor

fjl commented Dec 21, 2016

Now that I think about it, the API still isn't right. It works reasonably well when key pre-images are available. When they're not, all keys will be 0x0000..00 (but hashedKey will be different). Since the key range parameters operate on key instead of hashedKey, the client can't reliably skip over keys it has already downloaded. IMHO it would be better to change the API so the key range applies to hashedKey. We should probably also make the end parameter exclusive instead of inclusive.

@chriseth @yann300 please comment. This API is for remix so the result should make sense for whatever remix is doing.

@chriseth
Copy link
Copy Markdown

chriseth commented Jan 2, 2017

Concerning debug_preimage: Perhaps we could rephrase it as "... can be used to retrieve the preimage of a given SHA3 image, if available. Conforming implementations SHOULD at least provide preimages of any trie node and any SHA3 operation computed by the EVM, unless some syncing method was used that does not provide that information'.

@chriseth
Copy link
Copy Markdown

chriseth commented Jan 4, 2017

@gumb0 could you please adjust the implementation in cpp-ethereum to the new specification?

@karalabe
Copy link
Copy Markdown
Member

karalabe commented Jan 6, 2017

@fjl Is still still slated for 1.5.6 or do we push it out to 1.5.7? I'd like to finish the release merges today.

@fjl fjl modified the milestones: 1.5.7, 1.5.6 Jan 8, 2017
@karalabe karalabe modified the milestones: 1.5.8, 1.5.7 Jan 16, 2017
@fjl fjl modified the milestones: 1.5.8, 1.5.9 Feb 1, 2017
@yann300
Copy link
Copy Markdown

yann300 commented Mar 13, 2017

@fjl instead of returning an array for the storage,
is that possible to return a map of hashed key?

"storage": {
 "0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563":
      {
        "key": "0x0000000000000000000000000000000000000000000000000000000000000000",
        "hashedKey": "0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563",
        "value": "0x0000000000000000000000945e237bf1540eea146f5e830593d1d54c279616a9"
      },
...
}

@fjl
Copy link
Copy Markdown
Contributor

fjl commented Mar 13, 2017

Yes

@yann300
Copy link
Copy Markdown

yann300 commented Apr 13, 2017

@fjl is the return value is sorted by hashed keys?

@fjl
Copy link
Copy Markdown
Contributor

fjl commented Apr 19, 2017

This is now #14350

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants