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

fix(trie): pass down hash function for hashing on trie copies #2068

Merged
merged 2 commits into from
Jul 23, 2022
Merged

fix(trie): pass down hash function for hashing on trie copies #2068

merged 2 commits into from
Jul 23, 2022

Conversation

faustbrian
Copy link
Contributor

@faustbrian faustbrian commented Jul 22, 2022

Resolves #2067 (comment) via #2067 (comment)
Needed for #2067 to write proper tests

Something I noticed here https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/trie/src/trie/secure.ts#L109 is that hash: this.hash seems to be missing. The Trie passes it down when it calls copy but CheckpointTrie and SecureTrie don't. Is this intentional or an oversight?

  copy(): Trie {
    return new Trie({
      db: this.db.copy(),
      root: this.root,
      deleteFromDB: this._deleteFromDB,
      hash: this._hash,
    })
  }

I assume this could result in corrupted data if you copy a CheckpointTrie or SecureTrie that for example uses sha256 but then it doesn't pass down the hash function and the copy works with keccak256 hashes because const _hash = opts?.hash ?? keccak256 will fall back to the keccak256 function?

  it.test('created copy uses the correct hash function', async function (t) {
    const trie = new SecureTrie({
      db: new LevelDB(),
      hash: (value) => createHash('sha256').update(value).digest(),
    })

    await trie.put(Buffer.from('key1'), Buffer.from('value1'))
    trie.checkpoint()
    await trie.put(Buffer.from('key2'), Buffer.from('value2'))
    const trieCopy = trie.copy()
    const value = await trieCopy.get(Buffer.from('key1'))
    t.equal(value!.toString(), 'value1')
    t.end()
  })
# created copy uses the correct hash function

not ok 183 TypeError: Cannot read properties of null (reading 'toString')
  ---
    operator: error
    stack: |-
      TypeError: Cannot read properties of null (reading 'toString')
          at Test.<anonymous> (/Users/brianfaust/Work/bearmint/ethereumjs-monorepo/packages/trie/test/trie/secure.spec.ts:167:20)

I think the above reproduces the issue. If I then change to the following copy function for SecureTrie it'll pass.

  copy(includeCheckpoints = true): SecureTrie {
    const secureTrie = new SecureTrie({
      db: this.dbStorage.copy(),
      root: this.root,
      deleteFromDB: (this as any)._deleteFromDB,
      hash: this._hash,
    })
    if (includeCheckpoints && this.isCheckpoint) {
      secureTrie.db.checkpoints = [...this.db.checkpoints]
    }
    return secureTrie
  }

The above test will fail because it will store key1 hashed via sha256 and key2 hashed via keccak256 but then the copy of the trie will use keccak256 again because this.hash wasn't passed down which results in it looking up key1 hashed via keccak256 instead of the sha256 it was stored as.

@codecov
Copy link

codecov bot commented Jul 22, 2022

Codecov Report

Merging #2068 (1648b41) into master (d276fcc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
block 83.75% <ø> (ø)
blockchain 80.10% <ø> (ø)
client 78.37% <ø> (ø)
common 95.00% <ø> (ø)
devp2p 82.73% <ø> (+0.06%) ⬆️
ethash 90.81% <ø> (ø)
evm 40.89% <ø> (ø)
statemanager 84.55% <ø> (ø)
trie 80.82% <100.00%> (-0.03%) ⬇️
tx 92.18% <ø> (ø)
util 87.22% <ø> (ø)
vm 78.51% <ø> (ø)

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

@faustbrian faustbrian marked this pull request as ready for review July 22, 2022 04:26
@acolytec3 acolytec3 requested a review from jochem-brouwer July 22, 2022 13:05
Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM

@acolytec3 acolytec3 merged commit 3ccd1b4 into ethereumjs:master Jul 23, 2022
@faustbrian faustbrian deleted the trie-hash-inconsistency branch July 23, 2022 02:22
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.

3 participants