Wired Mining into CliquePantheonController #22
Conversation
|
|
||
| @Override | ||
| public void disable() { | ||
| LOGGER.trace("Clique Mining cannot be disabled."); |
There was a problem hiding this comment.
Should this throw UnsupportedOperationException so the caller knows it's done nothing? Seems like it should at least be a WARN, although I'm not quite sure where this is called from.
| @@ -78,7 +78,7 @@ | |||
| private static final String MANDATORY_MODE_FORMAT_HELP = "<MODE>"; | |||
|
|
|||
| private static final Wei DEFAULT_MIN_TRANSACTION_GAS_PRICE = Wei.of(1000); | |||
| private static final BytesValue DEFAULT_EXTRA_DATA = BytesValue.EMPTY; | |||
| private static final BytesValue DEFAULT_EXTRA_DATA = BytesValue.wrap(new byte[32]); | |||
There was a problem hiding this comment.
I believe extra data for main net is variable length so defaulting to EMPTY is definitely better there. Actually, it probably should default to the pantheon version identifier string based on https://github.com/ethereum/wiki/wiki/Default-Extra-Data-Standard but that's out of scope here.
Might be better to leave this default to EMPTY but automatically leftPad any extra data we get to make it 32 bytes?
| new SystemClock(), | ||
| protocolContext.getConsensusState().getVoteTallyCache(), | ||
| Util.publicKeyToAddress(nodeKeys.getPublicKey()), | ||
| 15L), // TODO (tmm): This needs to come from |
There was a problem hiding this comment.
Probably need to at least complete this comment but better would be to make it actually come from config. :)
| return new CliquePantheonController( | ||
| genesisConfig, | ||
| protocolContext, | ||
| ethProtocolManager, | ||
| synchronizer, | ||
| nodeKeys, | ||
| transactionPool, | ||
| kv::close); | ||
| () -> { | ||
| miningCoordinator.disable(); |
There was a problem hiding this comment.
CliqueMiningCoordinator.disable() does nothing which suggests this shouldn't be called, but we definitely need to get mining to stop at shutdown time though. So maybe disable needs to actually stop mining?
There was a problem hiding this comment.
Done - we now allow mining to be started and stopped.
No description provided.