Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

BlockchainDouble seems to be using the wrong Trie import #664

Closed
smartcontracts opened this issue Nov 20, 2020 · 7 comments
Closed

BlockchainDouble seems to be using the wrong Trie import #664

smartcontracts opened this issue Nov 20, 2020 · 7 comments
Labels
Milestone

Comments

@smartcontracts
Copy link

Noticed this while trying to implement eth_getProof on top of ganache. It seems like BlockchainDouble is importing Trie as:

https://github.com/trufflesuite/ganache-core/blob/1177fe8a2711066a1b082b15d22585158001e80d/lib/blockchain_double.js#L9

When I'd expect to see:

var Trie = require("merkle-patricia-tree/secure");

The base Trie class (instead of the "secure" Trie) spits out invalid proofs because the keys aren't hashed before insertion.

You can see this discrepancy within ethereumjs-vm, which indeed uses the "secure" version here. Although using the base class is technically fine, it does produce a non-standard (and incompatible) result for eth_getProof.

@davidmurdoch
Copy link
Member

davidmurdoch commented Nov 20, 2020

I think this is due to the fact that we pre-seed accounts with ether on start up, and Secure trie makes things incompatible. Though I'm not entirely positive, and if that is true, I wonder if there is a simple way around it.

Have you tried swapping the Trie out for the SecureTrie then running the ganache-core tests? I'm curious if the eth_getBalance-related tests in test/accounts.js break (eth_getBalance returns "0x0" when we expect a non-zero value).

@davidmurdoch davidmurdoch added this to the 3.0.0 milestone Nov 20, 2020
@davidmurdoch
Copy link
Member

Adding this to the 3.0 milestone release.

@davidmurdoch
Copy link
Member

@kfichter I haven't yet researched the getProof myself, if you'd like to try adding eth_getProof in the next branch (which is in TypeScript) that'd be awesome! There isn't really a contributors guide yet, so it might be challenging to navigate at the moment. Let me know if you'd like to tackle it!

@davidmurdoch
Copy link
Member

fixed in ganache@3.0.0-internal.0

@smartcontracts
Copy link
Author

@davidmurdoch I'd definitely be happy to tackle eth_getProof if it's still a need! We have a hacky implementation that appears to be working correctly.

@davidmurdoch
Copy link
Member

@kfichter I still haven't implemented it in the next branch yet, so please feel free. You'll want to put the rpc method somewhere around here: https://github.com/trufflesuite/ganache-core/blob/7ef14689fbdc8f4919a492fc7dceea3b78dcc84d/src/chains/ethereum/src/api.ts#L829

and tests would go here: https://github.com/trufflesuite/ganache-core/tree/next/src/chains/ethereum/tests/api/eth (you'd name the file getProof.test.ts).

To set up the repo, check out the next branch then read the CONTRIBUING.md to get started.

Let me know if you need any help!

@davidmurdoch
Copy link
Member

Related issue: #382

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

No branches or pull requests

2 participants