Client/StateManager: storageRangeAt() RPC call / EVMStateManager Interface Extension by KoningR · Pull Request #2922 · ethereumjs/ethereumjs-monorepo

KoningR · 2023-07-28T20:04:42Z

This PR implements debug_storageRangeAt() as discussed in ethereum/go-ethereum#14350. An implementation is only provided for DefaultStateManager, not for EthersStateManager. Additionally, hashed key preimages are always null because there does not seem to be a good way to retrieve them currently.

codecov · 2023-07-29T09:20:28Z

Codecov Report

Merging #2922 (bc2bc69) into master (f704378) will decrease coverage by 2.90%.
The diff coverage is n/a.

Additional details and impacted files

Flag	Coverage Δ
block	`88.66% <ø> (ø)`
blockchain	`92.58% <ø> (ø)`
client	`?`
common	`?`
ethash	`?`
evm	`69.93% <ø> (ø)`
rlp	`∅ <ø> (∅)`
statemanager	`?`
trie	`89.98% <ø> (+0.03%)`	⬆️
tx	`95.96% <ø> (ø)`
util	`86.77% <ø> (ø)`
vm	`79.20% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

g11tech · 2023-07-29T17:38:59Z

      [validators.transaction()],
      [validators.blockOption],
    ])
+    this.storageRangeAt = middleware(this.storageRangeAt.bind(this), 5, [[validators.hex]])


the validators here will only validate the first arg, reading from comments need 5 validators

g11tech · 2023-07-29T17:45:43Z

+  /**
+   * Returns a limited set of storage keys belonging to an account.
+   * @param params An array of 5 parameters:
+   *    1. The hash of the block that contains the requested account storage.


Suggested change

* 1. The hash of the block that contains the requested account storage.

* 1. The hash of the block at which to get storage from the state.

just making it less ambiguous

g11tech · 2023-07-29T17:47:07Z

+   * Returns a limited set of storage keys belonging to an account.
+   * @param params An array of 5 parameters:
+   *    1. The hash of the block that contains the requested account storage.
+   *    2. The number index of the requested transaction within the block.


Suggested change

* 2. The number index of the requested transaction within the block.

* 2. The transaction index of the requested block post which to get the storage.

g11tech · 2023-07-29T17:48:18Z

+   *    1. The hash of the block that contains the requested account storage.
+   *    2. The number index of the requested transaction within the block.
+   *    3. The address of the account.
+   *    4. The smallest (hashed) storage key that will be returned. To include the entire range, pass '0x00'.


Suggested change

* 4. The smallest (hashed) storage key that will be returned. To include the entire range, pass '0x00'.

* 4. The starting (hashed) key from which storage will be returned. To include the entire range, pass '0x00'.

g11tech · 2023-07-29T17:48:46Z

+   *    2. The number index of the requested transaction within the block.
+   *    3. The address of the account.
+   *    4. The smallest (hashed) storage key that will be returned. To include the entire range, pass '0x00'.
+   *    5. The maximum number of storage values that will be returned.


Suggested change

* 5. The maximum number of storage values that will be returned.

* 5. The maximum number of storage values that could be returned.

g11tech · 2023-07-29T17:50:33Z

+
+    let block: Block
+    try {
+      block = await this.service.chain.getBlock(blockHashBytes)


Suggested change

block = await this.service.chain.getBlock(blockHashBytes)

block = await this.chain.getBlock(blockHashBytes)

this.chain is available

g11tech · 2023-07-29T17:51:06Z

+      }
+    }
+
+    if (txIndex + 1 > block.transactions.length) {


Suggested change

if (txIndex + 1 > block.transactions.length) {

if (txIndex >= block.transactions.length) {

more comprehensible about the check

g11tech · 2023-07-29T17:52:14Z

+    }
+
+    try {
+      const block = await this.service.chain.getBlock(hexToBytes(blockHash))


isn't the block already read above (in params check) also this.chain can be directly refereed

g11tech · 2023-07-29T17:55:49Z

+    try {
+      address = Address.fromString(account)
+      blockHashBytes = hexToBytes(blockHash)
+      startKeyBytes = hexToBytes(startKey)


startKey could be directly converted to bigint using a hexToBig... helper , and if limit is also coming as hex encoded it needs to be converted to number/bigint as well as per the requirement

There does not appear to exist a hexToBigInt() helper. I have however moved the function calls to the same line.

if the hex is '0x' prefixed, you can just use the BigInt constructor like: BigInt('0xabcde')

g11tech · 2023-07-29T17:57:36Z

+   * @returns A {@link StorageRange} object that will contain at most `limit` entries in its `storage` field.
+   * The object will also contain `nextKey`, the next (hashed) storage key after the range included in `storage`.
+   */
+  async storageRangeAt(params: [string, number, string, string, number]) {


Suggested change

async storageRangeAt(params: [string, number, string, string, number]) {

async storageRangeAt(params: [string, string, string, string, string]) {

most like it will come as a hex encoded number ... not sure about that but do check and update/confirm

The reference implementation does use int for both of these.

https://github.com/ethereum/go-ethereum/blob/4e9775668efc329cd4d12644f5310c1b5ac2ab41/eth/api_debug.go#L212

holgerd77 · 2023-07-31T10:57:19Z

Thanks a lot for this PR! 🙏🙂❤️

…orepo into debug-storagerangeat

KoningR · 2023-07-31T13:05:34Z

It appears that parameters txIndex and limit are indeed passed as number instead of as hex strings. Currently, I have omitted validation for these parameters because I could not find a suitable validator in validation.ts. Is it suggested that I parse the input myself or is there a proper validator for the number type?

holgerd77 · 2023-07-31T13:33:47Z

It appears that parameters txIndex and limit are indeed passed as number instead of as hex strings. Currently, I have omitted validation for these parameters because I could not find a suitable validator in validation.ts. Is it suggested that I parse the input myself or is there a proper validator for the number type?

If you are open to do maybe add a very simple get int() validator to validation.ts, as simple as the bool one:

/**
   * bool validator to check if type is boolean
   * @param params parameters of method
   * @param index index of parameter
   */
  get bool() {
    return (params: any[], index: number) => {
      if (typeof params[index] !== 'boolean') {
        return {
          code: INVALID_PARAMS,
          message: `invalid argument ${index}: argument is not boolean`,
        }
      }
    }
  }

ScottyPoi · 2023-07-31T20:18:44Z

      [validators.transaction()],
      [validators.blockOption],
    ])
+    // TODO: txIndex and limit are currently not input validated.


do we need new validator types for these?

ScottyPoi · 2023-07-31T20:23:09Z

+    context.createdAddressNoStorage = thirdResult.createdAddress!!
+  })
+
+  it<TestSetup>('Should return the correct (number of) key value pairs.', async ({


What does the <TestSetup> type parameter do? is it just a label, or does it change the it function?

It is a typesafe way of passing variables from beforeEach() to tests (see here). For legibility, I had already annotated the TestSetup interface.

ScottyPoi · 2023-07-31T20:40:21Z

+      throw new Error('Missing VM.')
+    }
+
+    if (txIndex < 0) {


if we don't already have a validator for index parameters, it could be worth adding one. especially if there are other methods are doing this themselves

ScottyPoi · 2023-07-31T20:45:25Z

+      return await vmCopy.stateManager.dumpStorageRange(
+        // Validator already verified that `account` and `startKey` are properly formatted.
+        Address.fromString(account),
+        bytesToBigInt(hexToBytes(startKey)),


Suggested change

bytesToBigInt(hexToBytes(startKey)),

BigInt(startKey),

ScottyPoi · 2023-07-31T20:59:20Z

It appears that parameters txIndex and limit are indeed passed as number instead of as hex strings. Currently, I have omitted validation for these parameters because I could not find a suitable validator in validation.ts. Is it suggested that I parse the input myself or is there a proper validator for the number type?

If you are open to do maybe add a very simple get int() validator to validation.ts, as simple as the bool one:
/**
   * bool validator to check if type is boolean
   * @param params parameters of method
   * @param index index of parameter
   */
  get bool() {
    return (params: any[], index: number) => {
      if (typeof params[index] !== 'boolean') {
        return {
          code: INVALID_PARAMS,
          message: `invalid argument ${index}: argument is not boolean`,
        }
      }
    }
  }

Are there methods that use signed integers (negative numbers)? "index" will always be unsigned, so it might be better to add getUint, which throws if the input is < 0

ScottyPoi

Looks great! Thanks for including all the tests, and for adding the validator! super helpful 👍

jochem-brouwer

LGTM

g11tech

lgtm

gitpoap-bot · 2023-08-09T10:30:12Z

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2023 EthereumJS Contributor:

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

holgerd77 · 2023-08-09T10:33:39Z

@KoningR Hi there, thanks again for this great work! 🤩

We'll directly that this into the upcoming releases (so: today)!

Initial version of storageRangeAt().

e3e6220

acolytec3 requested review from ScottyPoi and jochem-brouwer July 29, 2023 09:23

g11tech reviewed Jul 29, 2023

View reviewed changes

am1r021 added PR state: needs review package: client labels Jul 30, 2023

holgerd77 added PR state: WIP type: feature and removed PR state: needs review labels Jul 31, 2023

KoningR added 2 commits July 31, 2023 14:52

Merge branch 'master' of https://github.com/ethereumjs/ethereumjs-mon…

026ffe4

…orepo into debug-storagerangeat

Update input validation of storageRangeAt().

b8d5adc

ScottyPoi reviewed Jul 31, 2023

View reviewed changes

Add validator for unsigned integers.

9a0a72f

KoningR requested a review from ScottyPoi August 4, 2023 13:35

ScottyPoi added 2 commits August 8, 2023 23:10

Merge branch 'master' into debug-storagerangeat

1ecc198

remove return await

720e3c3

ScottyPoi approved these changes Aug 9, 2023

View reviewed changes

jochem-brouwer approved these changes Aug 9, 2023

View reviewed changes

Merge branch 'master' into debug-storagerangeat

bc2bc69

holgerd77 added PR state: needs review package: common package: statemanager and removed PR state: WIP labels Aug 9, 2023

holgerd77 changed the title ~~Initial version of storageRangeAt().~~ Client/StateManager: storageRangeAt() RPC call / EVMStateManager Interface Extension Aug 9, 2023

g11tech approved these changes Aug 9, 2023

View reviewed changes

holgerd77 merged commit fa8a518 into ethereumjs:master Aug 9, 2023

	* 1. The hash of the block that contains the requested account storage.
	* 1. The hash of the block at which to get storage from the state.

	* 2. The number index of the requested transaction within the block.
	* 2. The transaction index of the requested block post which to get the storage.

	* 4. The smallest (hashed) storage key that will be returned. To include the entire range, pass '0x00'.
	* 4. The starting (hashed) key from which storage will be returned. To include the entire range, pass '0x00'.

	* 5. The maximum number of storage values that will be returned.
	* 5. The maximum number of storage values that could be returned.

	block = await this.service.chain.getBlock(blockHashBytes)
	block = await this.chain.getBlock(blockHashBytes)

	if (txIndex + 1 > block.transactions.length) {
	if (txIndex >= block.transactions.length) {

	async storageRangeAt(params: [string, number, string, string, number]) {
	async storageRangeAt(params: [string, string, string, string, string]) {

Uh oh!

Conversation

KoningR commented Jul 28, 2023

Uh oh!

codecov Bot commented Jul 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

g11tech Jul 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScottyPoi Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

g11tech Jul 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holgerd77 commented Jul 31, 2023

Uh oh!

KoningR commented Jul 31, 2023

Uh oh!

holgerd77 commented Jul 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScottyPoi commented Jul 31, 2023

Uh oh!

ScottyPoi left a comment

Choose a reason for hiding this comment

Uh oh!

jochem-brouwer left a comment

Choose a reason for hiding this comment

Uh oh!

g11tech left a comment

Choose a reason for hiding this comment

Uh oh!

gitpoap-bot Bot commented Aug 9, 2023

Uh oh!

holgerd77 commented Aug 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov Bot commented Jul 29, 2023 •

edited

Loading

g11tech Jul 29, 2023 •

edited

Loading

ScottyPoi Jul 31, 2023 •

edited

Loading

g11tech Jul 29, 2023 •

edited

Loading