StateManager - _getStorageTrie: Fix Input Checking Logic

[Dappsters]

This PR fixes input checking logic for the StateManager._getStorageTrie function, removes repeated code, and adds documentation.

Changes

• Before, instanceof checks against the Address class sometimes returned false when the package was loaded
  as a sub-dependency due to differing references for the class definition. This caused an error to be thrown (see example below).
• Now, the check uses native Uint8Array for type checking, which is consistent across use cases.
• Before, if the input was not a Uint8Array, the hashing function was called twice because the intermediate value was discarded.
• Now, if the input is not a Uint8Array, the intermediate value is reused and the hashing function is only called once.

Resolved Issues

Fixes an error condition when the package is loaded as a sub-dependency causing inconsistent instanceof Address evaluations that resulted in the incorrect value being passed to the bytesToUnprefixedHex function which then throws an error.

'Bad' project dependencies structure:

- Main Project
  - Sub Package
    - EthereumJS Packages

Thrown error example for reference:

/service/node_modules/@noble/hashes/utils.js:41
        throw new Error('Uint8Array expected');
               ^

Error: Uint8Array expected
     at bytesToHex (/service/node_modules/@noble/hashes/utils.js:41:15)
     at DefaultStateManager._getStorageTrie (/service/node_modules/@ethereumjs/statemanager/dist/cjs/stateManager.js:235:60)
     at /service/node_modules/@ethereumjs/statemanager/dist/cjs/stateManager.js:312:38
     at new Promise (<anonymous>)
     at DefaultStateManager._modifyContractStorage (/service/node_modules/@ethereumjs/statemanager/dist/cjs/stateManager.js:311:16)
     at DefaultStateManager.clearContractStorage (/service/node_modules/@ethereumjs/statemanager/dist/cjs/stateManager.js:383:20)
     at async EVM._executeCreate (/service/node_modules/@ethereumjs/evm/dist/cjs/evm.js:358:9)
     at async EVM.runCall (/service/node_modules/@ethereumjs/evm/dist/cjs/evm.js:710:22)
     at async VM._runTx (/service/node_modules/@ethereumjs/vm/dist/cjs/runTx.js:359:22)
     at async VM.runTx (/service/node_modules/@ethereumjs/vm/dist/cjs/runTx.js:114:24)

Notes

The pre-existing comment // TODO PR: have a better interface for hashed address pull? references discussion from #3031, which states that the function may be removed as part of an ongoing cleanup effort in #3117. Because #3117 appears to be relatively inactive, this PR is still needed for the immediate future. The comment was left as-is.

[acolytec3]

This looks fine to me. @holgerd77 @jochem-brouwer any opinions here before merging?

[acolytec3]

I take that back, since builds are failing.

[acolytec3]

Why add this { bytes: Uint8Array } to the type union since it breaks the build on CI. Does this add some benefit in the specific usecase your PR addresses related to @ethereumjs/statemanager as a sub-dependency?

[Dappsters]

The added type simply documents that the function doesn't rely on any of the properties or methods of Address other than the bytes property. The resulting union is no more restrictive than the original, and allows usage without instantiating a complete Address object (e.g. for unit testing).

The build failure was due to the compiler not identifying the boolean inputIsHash flag as type narrowing via the instanceof call. Removing the flag and making that operation inline fixed the build failure.

[acolytec3]

Thanks, will take another look!

[codecov]

Codecov Report

Attention: Patch coverage is 95.83333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 83.13%. Comparing base (d24ca11) to head (d4cedad).
Report is 20 commits behind head on master.

Additional details and impacted files

Flag	Coverage Δ
block	80.78% <ø> (?)
blockchain	90.97% <ø> (ø)
common	94.29% <ø> (+<0.01%)	⬆️
devp2p	81.85% <ø> (?)
evm	73.75% <ø> (?)
genesis	99.98% <ø> (?)
statemanager	74.90% <95.83%> (+0.59%)	⬆️
trie	59.44% <ø> (?)
util	81.46% <ø> (+0.02%)	⬆️

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

[holgerd77]

LGTM

[holgerd77]

Hi @Dappsters, just to let you know: we have a very very late release round this time - since we had (close to) no release pressure - so that issues like this only get in now (or: in 1-2 weeks on release). 😬

Apologies!