[PAN-2954] Support for NoReward and NoProof seal engines #1813
Conversation
Retesteth requires support for mining without block rewards and without PoW. No PoW simply requires exposing the nonce generator an dusing a special EthHashSolver. No Reward in the tests is interpreted to mean Zero Reward, which presents problems pre EIP158 with regard to zero balance accounts as the tests point the coinbase to a non-existent account. We need to add a sentinel NO_REWARD reward value (in addition to existing support for a zero value which is a short circuit) that will proceed with the block reward logic at zero value. UInt256.MAX_VALUE was chosen as that sentinel as it is a value that would never make sense otherwise, causing instant integer overflow in any real usecase. Retesteth will use Wei.ZERO or Wei.NO_REWARD based on the configured fork when mining in no-reward mode.
| @@ -158,6 +158,8 @@ private boolean rewardCoinbase( | |||
| if (blockReward.isZero()) { | |||
| return true; | |||
| } | |||
| // This reference equality check is deliberate. | |||
| final Wei blockReward = this.blockReward.equals(Wei.NO_REWARD) ? Wei.ZERO : this.blockReward; | |||
There was a problem hiding this comment.
You commented that this should be a reference equality check - did you mean to check that this.blockReward == Wei.NO_REWARD??
There was a problem hiding this comment.
Yes, stray comment. Will Fix.
| @@ -23,6 +23,7 @@ | |||
| public final class Wei extends BaseUInt256Value<Wei> { | |||
|
|
|||
| public static final Wei ZERO = of(0); | |||
| public static final Wei NO_REWARD = of(UInt256.MAX_VALUE); | |||
There was a problem hiding this comment.
This feels like the wrong place for this constant - maybe BlockProcessor instead?
There was a problem hiding this comment.
I can move it there. It also needs to be visible to AbstractBlockCreator and MainnetBlockProcessor, but it looks like the needed dependencies are in place.
| return true; | ||
| } | ||
| final Wei blockReward = blockRewardSpec.equals(Wei.NO_REWARD) ? Wei.ZERO : blockRewardSpec; |
There was a problem hiding this comment.
Instead of having this special Wei.NO_REWARD value - what about just adding a flag like forceUpdateBeneficiaryAccount(boolean shouldForceUpdate) or something like that? I think that would make the intentions behind this logic clearer. Then you can return early on line 263 iff blockRewards.isZero() && !forceUpdate.
There was a problem hiding this comment.
We would need to add this flag to both the block creation and the block validation. That would involve passing a flag into the ProtocolScheduleBuilder, and the question would be "is EIP 158 active?"
There was a problem hiding this comment.
I just pused an alternate approach where we expose EIP158 status in the ProtocolSpec, seems to get to both locations with a minimum of grief.
| @@ -158,6 +158,8 @@ private boolean rewardCoinbase( | |||
| if (blockReward.isZero()) { | |||
There was a problem hiding this comment.
would be nice if we could dedupe this method :D, but probably out of scope for this change
There was a problem hiding this comment.
yes, out of scope I'de say. Possibly could become static methods.
| * | ||
| * @return if EIP158 is in effect for this spec. | ||
| */ | ||
| public boolean isEIP158() { |
There was a problem hiding this comment.
Naming quibbles - maybe this could be isEIP158Enabled. Not sure if it would be a good idea name it something more "meaningful" like prohibitEmptyAccounts?
There was a problem hiding this comment.
I'll go with IsEIP158Enabled when I fix the acceptance tests. My long term vision is that we have an isEIPEnabled(int eip) option and we track all the EIPs. That way we can speculatively include EIPs in the code base and use the EIP registry as feature flags. But since we care about only one at this point I'll put the number in the name,
There was a problem hiding this comment.
Turns out it's a clique protocol dependency, so labeling it as EIP158 treatment is inaccurate, so I went with skipZeroBlockRewards.
|
I ran the reference tests but not the acceptance tests... fix is underway. |
…So the flag now goes to skipZeroBlockRewards and gets set on all the protocol schedule overrides.
| return true; | ||
| } | ||
| // This reference equality check is deliberate. |
| @@ -136,4 +143,62 @@ public void createMainnetBlock1_fixedDifficulty1() throws IOException { | |||
| // If we weren't setting difficulty to 2^256-1 a difficulty of 1 would have caused a | |||
| // IllegalArgumentException at the previous line, as 2^256 is 33 bytes. | |||
| } | |||
|
|
|||
| @Test | |||
| public void createMainnetBlock1_noReward() { | |||
There was a problem hiding this comment.
Suggest a name that's more specific to what you're testing:
| public void createMainnetBlock1_noReward() { | |
| public void rewardBeneficiary_zeroReward_skipZeroRewardsFalse() { |
| .buildProcessableBlockHeader(); | ||
|
|
||
| blockCreator.rewardBeneficiary( | ||
| mutableWorldState, header, Collections.emptyList(), Wei.ZERO, false); |
There was a problem hiding this comment.
Probably worth creating another version of this test where skipZeroBlockRewards is true
PR description
Retesteth requires support for mining without block rewards and without PoW.
No PoW simply requires exposing the nonce generator and using a special
EthHashSolver.
No Reward in the tests is interpreted to mean Zero Reward, which presents
problems pre EIP158 with regard to zero balance accounts as the tests point the
coinbase to a non-existent account. We need to add a sentinel NO_REWARD reward
value (in addition to existing support for a zero value which is a short
circuit) that will proceed with the block reward logic at zero value.
UInt256.MAX_VALUE was chosen as that sentinel as it is a value that would never
make sense otherwise, causing instant integer overflow in any real use case.
Retesteth will use Wei.ZERO or Wei.NO_REWARD based on the configured fork when
mining in no-reward mode.