chore: remove homestead + update eth dependencies

[micahriggan]

closes #68
closes #67

• Removed the logic setting the homestead flag
• Updated ethereumjs-tx and ethereumjs-util
• Pointed all SHA3 logic to point to utils.KECCAK256

[coveralls]

Coverage increased (+0.2%) to 74.007% when pulling e7aaf77 on micahriggan:chore/update-eth-dependencies into 2c3908e on ethereumjs:master.

[s1na]

I personally like this more than #68, thanks! I'd however wait until ethereumjs/ethereumjs-tx#130 makes it into the next release, and then merge here with the latest ethereumjs-tx.

[micahriggan]

Okay, let me know if there's anything else I can do :)

[holgerd77]

@micahriggan Sorry that this takes so long, we'll do a tx release soon.

[alcuadrado]

@micahriggan thanks for submitting this PR!

We already released a new ethereumjs-tx version, that changes how hardforks are managed. Can you update this PR to the latest version? If you can't, please let me know and I'll do it.

[micahriggan]

@alcuadrado updated to 2.1.0

[holgerd77]

@alcuadrado Can you go on with a review here?

[alcuadrado]

LGTM. Thanks @micahriggan!

[holgerd77]

I think this is too dangerous to just pass on the opts dictionary since this can have unwanted side effects, e.g. if an options key is named the same on the two libraries (along future changes e.g.). Can you directly pass the instantiated Common object, so { common: this._common}?

[alcuadrado]

Oops, I missed this. It's a very good point

[micahriggan]

0810af6

I did that initially in this commit, but ran into issues with the tests.

[holgerd77]

Ok, we have to figure this out.

[micahriggan]

If you know the solution to
Error: Method called with neither a hardfork set nor provided by param at Common._chooseHardfork (/Users/micah/dev/ethereumjs-block/node_modules/ethereumjs-common/dist/index.js:114:23)

Then I can switch to using that.

[alcuadrado]

I figured this out. The problem is that if no Common object is provided, a new one is created without a hardfork param, and this results in an error.

In order to fix this, the else branch of the if that sets this._common should be something like:

    let chain = opts.chain ? opts.chain : 'mainnet'

    const tmpCommon = new Common(chain)
    const blockNumber = 123 // TODO: How do we get the block number here?
    
    let hardfork = opts.hardfork ? opts.hardfork : tmpCommon.activeHardfork(blockNumber)
    this._common = new Common(chain, hardfork)

[micahriggan]

this reminds me of my initial fix, https://github.com/ethereumjs/ethereumjs-block/pull/68/files

Seems like we'll need the common hardfork to be set after this.header

[micahriggan]

So I tried with this using the last hardfork, and that doesn't fix the original issue, so it will need to be based off of the height.

Edit: Should be good now

[alcuadrado]

This should work :) Thanks a lot @micahriggan ! Can you re-review it @holgerd77 ?

Something I noticed is that in the current code the BlockHeader#_common could have a null hardfork, but BlockHeader's methods handle that case.

[holgerd77]

Hmm, this can't be the solution yet, now we have got inconsistent hardfork setting logic in block and header which can eventually end up in differing common objects.

Not sure, maybe we need to pass options to header explicitly and use the eventually altered _common object directly or something. Or a unified util function used both by block and header - eventually from a separate util.js file or something getting for the logic of creating or taking the common object, so that this is unified?

Just some ideas, but we have to make sure that on 100% both end up with the same Common state.

[micahriggan]

I see what you mean. I'll mess around with it and see if I can get them to be synchronized

[micahriggan]

I tried making the BlockHeader's common object set the fork, then making block just inherit the common object from the header, but 15k tests fail if you do that. I don't really know enough to understand why.

[alcuadrado]

I kept working on this as part of the TS migration, and I found many edge-cases mostly due to BlockHeader#number being mutable.

Things like, what happens if you initialize a block with transactions on petersburg, and then change the block number to one pre-eip155?

Also, can you really share the common between a block an its uncles? they may belong to a different HF.

I tried updating the Common object whenever header.number was changed, but lots of tests fail that wait.

TBH, I'm a little lost about what to do.

[alcuadrado]

I also tried overwriting the number descriptor to forbid modifications once a block header is constructed, but this breaks BlockHeader#setGenesisParams.

[holgerd77]

@alcuadrado seems like this intended update (of Common and Transaction libraries) is pulling to the surface more insufficiencies and inconsistencies of the block library than expected and will as it looks now a bit open up different work areas.

@micahriggan I am just seeing that the commit history of this work is getting too extensive/intransparent anyhow to be directly merged and would need squashing and reorganizing.

@alcuadrado @micahriggan Would it generally be a way that we extract the util changes from here - @micahriggan would you be willing to open a separate PR on that with a single commit? - and we then close this PR here and take on the TypeScript transition first - which gives @alcuadrado some more opportunity to dig into the library.

@alcuadrado while working on this you can eventually already open separate issues on refactoring ideas/necessities and we tackle these separately afterwards working towards a more consistent solution which doesn't feel "hacky"?

Does this make sense? I have the impression we are trying to solve too many things at once otherwise eventually.

[alcuadrado]

@alcuadrado seems like this intended update (of Common and Transaction libraries) is pulling to the surface more insufficiencies and inconsistencies of the block library than expected and will as it looks now a bit open up different work areas.

Yes, absolutely. Unfortunately, I believe that this is not only affecting this library, but it's a general issue shared by all the libraries. My understanding is that it comes from:

1. Different objects have complex relationships between them (i.e. the block number changes the hardfork, and the hardfork changes the tx validation rules), and being able to mutate them individually will always lead to edge-cases.

2. These libraries try to serve two very different clients: dapp and smart contract developers, and protocol-level developers (as in using them in the VM). I think that both can be served with an appropriate design, but we now have an inconsistent mixture of protocol-level/RLP internal representation and application-level logic.

We have been discussing this with @s1na for some time in other issues.

@alcuadrado @micahriggan Would it generally be a way that we extract the util changes from here - @micahriggan would you be willing to open a separate PR on that with a single commit?

I'm not convinced that this is worth it or even a positive thing. It will fix some edge-cases, but opening the door to/ignoring others. I think the current state of the proposal keeps the previous behavior, which in most cases work. I'm not 100% sure about this, as it's a pretty complex situation.

@alcuadrado while working on this you can eventually already open separate issues on refactoring ideas/necessities and we tackle these separately afterwards working towards a more consistent solution which doesn't feel "hacky"?

Does this make sense? I have the impression we are trying to solve too many things at once otherwise eventually.

I'll take some time to recollect what was previously discussed, and open an issue in the organization repo.

[alcuadrado]

This change was incorporated in #72

Thanks for this PR @micahriggan !