Flexible roots

[aakilfernandes]

This makes two changes

1. _findPath takes a root as second argument. This avoids the race condition where multiple gets/sets are taking place with changing roots

https://github.com/ethereumjs/merkle-patricia-tree/compare/getWithRoot?expand=1#diff-66120f21e6456d8c708592f111155feeR277

2. Creates a public getWithRoot function that allows a custom root. This is necessary to fix a race condition in testRPC where the root is being manipulated

trufflesuite/ganache-cli-archive#234

[coveralls]

Coverage increased (+0.2%) to 87.46% when pulling 7dd851c on getWithRoot into 828febf on master.

[coveralls]

Coverage increased (+0.2%) to 87.46% when pulling 7dd851c on getWithRoot into 828febf on master.

[coveralls]

Coverage increased (+0.2%) to 87.46% when pulling c81da6e on getWithRoot into 828febf on master.

[benjamincburns]

@aakilfernandes - can you please update this to resolve the conflicts?

@jwasinger - is this repo under your purview? If so, I think this is a fairly important issue for the TestRPC, due to trufflesuite/ganache-cli-archive#234. It's especially bad because when this is happening we'll possibly return bad data rather than firing off an error.

[jwasinger]

@benjamincburns yeah let me take a look to make sure I understand the changes here. An easy fix for this issue is to make a copy of the trie before doing operations on it.

There's overhead incurred but it should mitigate the issue.

[jwasinger]

e.g.

let tmptrie = state.trie.copy()
tmptrie.root = someroot

// do stuff

[benjamincburns]

Thanks for the quick response! And yeah, that's likely what we'll do if these changes pose problems.

[benjamincburns]

@jwasinger it seems that .copy() doesn't do much.

merkle-patricia-tree/baseTrie.js

Lines 662 to 664 in dc43642

	Trie.prototype.copy = function () {
	return new Trie(this.db, this.root)
	}

[jwasinger]

Okay that's even better. There is no actual data copying involved because the db backend is reused. I'd say based on that, we can close this issue and recommend that users utilize the code I posted above to enable concurrent access to the trie.

[benjamincburns]

@jwasinger is it better? Are there any operations performed by the VM involving the trie which aren't idempotent? For example, some operation which fails if some data already exists in the trie?

[ryanio]

Okay that's even better. There is no actual data copying involved because the db backend is reused. I'd say based on that, we can close this issue and recommend that users utilize the code I posted above to enable concurrent access to the trie.

great, thanks :) i will close this PR then.

trie op race conditions should also be totally resolved with moving to sync with #113.

[benjamincburns]

@ryanio since #113 has been closed, should this be reopened?

[ryanio]

@benjamincburns ah sorry about that. Would you not agree with this reply from @jwasinger:

I'd say based on that, we can close this issue and recommend that users utilize the code I posted above to enable concurrent access to the trie.

[benjamincburns]

I defer to @davidmurdoch, as he manages Ganache these days. I just was going through my (long list of rather old) notifications and noticed that this was closed in favor of another issue that had also been closed, and figured I should flag it.