Support for Ethereum Recovery Proposals #1004
Conversation
* A little cleanup
* Fix log statements
* Adding tests on StateChangeObject
* Adding 0x prefix to all hex data for clarity (it's actually optional)
| /** | ||
| * Created by Dan Phifer, 2018-02-01. | ||
| */ | ||
| public class ErpConfig extends FrontierConfig /* TODO: Is FrontierConfig correct? */ { |
There was a problem hiding this comment.
I guess ErpConfig should extend ByzantiumConfig. This assumption is based on further 6_000_000 block number used in the context. Check ByzantiumConfig implementation for details
| public class ErpConfig extends FrontierConfig /* TODO: Is FrontierConfig correct? */ { | ||
|
|
||
| private final long EXTRA_DATA_AFFECTS_BLOCKS_NUMBER = 10; | ||
| public static final Logger logger = LoggerFactory.getLogger("config"); |
There was a problem hiding this comment.
Use general logger instead of config
| try { | ||
| initErpConfig(); | ||
| } catch (IOException e) { | ||
| // TODO: not sure what to do here. |
There was a problem hiding this comment.
throwing RuntimeException should be enough here
| */ | ||
| @Override | ||
| public byte[] getExtraData(byte[] minerExtraData, long blockNumber) { | ||
| // TODO: is EXTRA_DATA_AFFECTS_BLOCKS_NUMBER needed? |
There was a problem hiding this comment.
You may use this code if blockNumber belongs to one of the first 10 blocks, otherwise call to parent.getExtraData. Did I get you?
There was a problem hiding this comment.
I think so. I saw in the DaoHFConfig that the extra data was applied not just at the target block, but for an a total of 10 blocks. I wasn't sure why that was done or if the same logic would be needed in this case.
There was a problem hiding this comment.
I suppose it was done to check if miners accept the fork. Adding data in ten blocks in a row is more confident than just one
| throw new RuntimeException("Failed to load state change object for " + erpMetadata.getId(), e); | ||
| } | ||
|
|
||
| // TODO: Is this the right way to apply changes in batch? |
There was a problem hiding this comment.
Yes, it's implemented in the right way
| throw e; | ||
| } | ||
| finally { | ||
| track.close(); |
There was a problem hiding this comment.
don't have to do this, just omit
There was a problem hiding this comment.
I was thinking it would actually be better to leave it. The startTracking method on the Repository interface returns another Repository; it makes no promise about the implementation. Whether the close method is a no-op or not is an implementation detail of the returned class, right?
|
|
||
|
|
||
| ////////////////////////////////////////////// | ||
| // TODO: These methods we included in DaoHFConfig, so I have included them here as well. |
There was a problem hiding this comment.
You don't have to implement them since they are implemented by parent class
| @@ -32,5 +32,6 @@ public MainNetConfig() { | |||
| add(2_463_000, new Eip150HFConfig(new DaoHFConfig())); | |||
| add(2_675_000, new Eip160HFConfig(new DaoHFConfig())); | |||
| add(4_370_000, new ByzantiumConfig(new DaoHFConfig())); | |||
| add(6_000_000, new ErpConfig()); | |||
There was a problem hiding this comment.
Take a look at JsonNetConfig you might want to handle ErpConfig there too
| public void applyStateChanges(StateChangeObject sco, Repository repo) throws ErpExecutionException { | ||
| try { | ||
| for (StateChangeAction action : sco.actions) { | ||
| applyStateChangeAction(action, repo); |
There was a problem hiding this comment.
Applying code could be a part of StateChangeAction. It would be more readable then
There was a problem hiding this comment.
Ok, I will change to a custom deserializer, which will remove the RawStateChangeObject and will allow me to have separate action classes for each action type. so the interface will be action.applyStateChange(repo)
| import java.util.LinkedList; | ||
| import java.util.List; | ||
|
|
||
| public class ErpLoader { |
There was a problem hiding this comment.
I guess it could be simplified somehow. For example, there is no need for extra-classes like RawStateChangeObject
|
Since this PR is for review purposes will close it for now |
This PR contains the proposed reference implementation for supporting Ethereum Recovery Proposals (ERPs). There are a number of places where I'm unclear about the right approach, so your guidance is appreciated.
Please see the associated EIP issue for details: ethereum/EIPs#866
TL;DR the proposed ERP process provides a standard mechanism to support irregular state changes in uncontentious recovery cases where ownership is clear and supported by strong evidence. It can support ethereum/EIPs#156, as well as other issues.