search

ethereumjs / ethereumjs-block

Project is in active development and has been moved to the EthereumJS VM monorepo.
https://github.com/ethereumjs/ethereumjs-vm/tree/master/packages/block
Mozilla Public License 2.0
42 stars 49 forks source link

chore: remove homestead + update eth dependencies #69

Closed micahriggan closed 5 years ago

micahriggan commented 5 years ago

closes #68 closes #67

coveralls commented 5 years ago

Coverage Status

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

s1na commented 5 years ago

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

micahriggan commented 5 years ago

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

holgerd77 commented 5 years ago

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

alcuadrado commented 5 years ago

@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 commented 5 years ago

@alcuadrado updated to 2.1.0

holgerd77 commented 5 years ago

@alcuadrado Can you go on with a review here?

micahriggan commented 5 years ago

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 commented 5 years ago

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.

alcuadrado commented 5 years ago

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 commented 5 years ago

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

holgerd77 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

This change was incorporated in #72

Thanks for this PR @micahriggan !