Incorrect behavior of fakeBlockChain.prototype.getBlock

[alcuadrado]

Hi,

I'm pretty sure there's a bug here: https://github.com/ethereumjs/ethereumjs-vm/blob/master/lib/fakeBlockChain.js#L10

In my experience, using a VM instance without skipping validation will call this function with block hashes, so it's hashing the hash. hash should be blockTag.

Of course, the validation process fails anyway, but that incorrect implementation of getBlock is confusing, especially when debugging and seeing strange block hashes.

[alcuadrado]

Also, this is really weird: https://github.com/ethereumjs/ethereumjs-vm/blob/master/lib/fakeBlockChain.js#L12

[s1na]

Haven't looked deeply, but generally fakeBlockchain should be avoided apart from unit testing or something. Would you maybe like to submit a PR if you think there's a bug?

[holgerd77]

@s1na Hmm, fakeBlockchain is currently used as the default blockchain if no blockchain is passed, so I think it's problematic if we advice to avoid since this is not really transparent from the outside.

I am actually not really aware why fakeBlockchain was introduced in the first place and what is the main purpose? Was this rather functionality or performance-guided? Couldn't this behavior taken from the blockchain class directly? This would probably also avoid compatibility problems if blockchain and fakeBlockchain behavior diverge.

[alcuadrado]

@s1na Hmm, fakeBlockchain is currently used as the default blockchain if no blockchain is passed, so I think it's problematic if we advice to avoid since this is not really transparent from the outside.

That's how I found this issue. I wasn't passing any blockchain to the VM, and running a block without skipping validations. That resulted in a crash, and when trying to debug it, the block hashes that the VM saw were completely different from the ones of the blocks I was creating.

[s1na]

@holgerd77 What do you think about removing it? or moving it to the tests/ dir, where it can be used as a mock object?

[holgerd77]

What would you do on VM instantiation? Create a normal blockchain object as default?

[holgerd77]

Again: are you aware why this was introduced compared to using blockchain?

[s1na]

No, I'm also not sure why it was introduced. Git history shows this commit to have added fakeBlockchain as a default.

Yeah, if no blockchain was provided, create a new normal instance. If however blockchain has to be provided by the user, we could alternatively throw an error if none was provided (but what you suggested is probably the better approach).

[holgerd77]

Ok, then let's remove but also let's be a bit cautious here.

Circling @vpulim and @axic, your guys comments would be helpful for some confirmation.

[holgerd77]

Fixed by #466, will close.