Implement EIP 1234 #1303
Implement EIP 1234 #1303
Conversation
cburgdorf
force-pushed
the
christoph/feat/1234
branch
9 times, most recently
from
ee980b5
to
6a9723e
Compare
cburgdorf
force-pushed
the
christoph/feat/1234
branch
from
6a9723e
to
6d83dc5
Compare
You are right. Should be fixed now. I'll make an inline comment to get feedback on an alternative implementation idea. |
| @@ -80,9 +92,3 @@ def validate_receipt(cls, receipt: Receipt) -> None: | |||
| @staticmethod | |||
| def get_block_reward(): | |||
| return EIP649_BLOCK_REWARD | |||
|
|
|||
| @staticmethod | |||
| def get_uncle_reward(block_number, uncle): | |||
There was a problem hiding this comment.
Alternatively, we could make this a @classmethod and then access the block reward on cls.block_reward which would allow us drop any uncle reward refinement in ConstantinopleVM. Thoughts?
There was a problem hiding this comment.
Conceptually I'm fine with that but I think it goes against the direction we're trying to go with composition > inheritance. I like the mechanism you've got in place now with the curried re-usable utilities so if you're happy with that I say keep it (and maybe backport it to the ByzantiumVM)
There was a problem hiding this comment.
Yup, I like it as-is, too. I kind of like the idea of pushing the get_uncle_reward(...) definition as far back as possible, so that even the import carries semantic meaning, like:
from eth.vm.forks.frontier import get_uncle_rewardWould mean that the uncle mechanism has stayed the same since frontier, with only the block reward changing in: get_uncle_reward = staticmethod(get_uncle_reward(EIP649_BLOCK_REWARD)).
(I don't remember when/if the uncle mechanism changed, but the general concept stands.)
There was a problem hiding this comment.
Yup, I like it as-is, too. I kind of like the idea of pushing the get_uncle_reward(...) definition as far back as possible
It was last changed in byzantium so I think it is already pushed as far back as possible.
There was a problem hiding this comment.
Going to request two test cases, ideally using something like the eth.tools.fixtures.builder if possible.
- Simple empty block mining which validates the miner is paid the correct amount
- Empty block with uncle which validates the miner and uncle miner are paid correctly.
|
@pipermerriam I was leaving out tests because I thought we'll have plenty of shared |
I figured as much. If it proves too big an issue to write them I could be convinced to leave them out but I think it's a good practice for us to start. I know of a few things that I fully expected would be covered by |
Totally agree to that. We can leave this PR open a bit longer if we aren't in a rush. I first want to finish up #1309 if possible. |
|
I added one test that involves regular blocks, a fork, included uncle and then ensures the balances of miner 1 and miner 2 match what we expect. I think that should cut it, no? Ps.: These chain builder tools are heaven! ☁️ ✨ |
There was a problem hiding this comment.
I think that should cut it, no?
Yeah, given that we can expect some more tests to come in after we update the fixtures. If we ignore those tests, then some conspicuously absent tests are:
- calculating uncle/nephew reward when the uncle is on a different VM (like: uncle in frontier at 0, nephew in constantinople at 1)
- uncles that are 2 -> 8 layers back
- validating that uncles > 8 back are invalid
(I'm saying the last two without having reviewed the uncles section of the YP recently, so probably have some detail wrong)
|
Interesting test cases! I'll create a good first starter issue for someone to grab to add expand the reward tests to cover these cases. |
What was wrong?
We need to implement EIP1234
How was it fixed?
Refactored Byzantium implementation to be more reusable for Constantinople. Left a comment inline for an alternative implementation idea.
Cute Animal Picture