Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

[PAN-2352] Make alloc optional and provide nicer error messages when genesis config is invalid #1029

Merged
merged 5 commits into from
Mar 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public GenesisConfigOptions getConfigOptions() {
}

public Stream<GenesisAllocation> getAllocations() {
final JsonObject allocations = configRoot.getJsonObject("alloc");
final JsonObject allocations = configRoot.getJsonObject("alloc", new JsonObject());
return allocations.fieldNames().stream()
.map(key -> new GenesisAllocation(key, allocations.getJsonObject(key)));
}
Expand All @@ -80,34 +80,47 @@ public String getExtraData() {
return configRoot.getString("extradata", "");
}

public Long getGasLimit() {
return Long.decode(getRequiredString("gaslimit"));
public long getGasLimit() {
return parseLong("gasLimit", getRequiredString("gaslimit"));
}

public String getMixHash() {
return configRoot.getString("mixhash", "");
}

public String getNonce() {
return configRoot.getString("nonce", "");
return configRoot.getString("nonce", "0x0");
}

public Optional<String> getCoinbase() {
return Optional.ofNullable(configRoot.getString("coinbase"));
}

public long getTimestamp() {
return Long.parseLong(configRoot.getString("timestamp", "0x0").substring(2), 16);
return parseLong("timestamp", configRoot.getString("timestamp", "0x0"));
}

private String getRequiredString(final String key) {
if (!configRoot.containsKey(key)) {
throw new IllegalArgumentException(
String.format("Invalid Genesis block configuration, missing value for '%s'", key));
String.format("Invalid genesis block configuration, missing value for '%s'", key));
}
return configRoot.getString(key);
}

private long parseLong(final String name, final String value) {
try {
return Long.decode(value);
} catch (final NumberFormatException e) {
throw new IllegalArgumentException(
"Invalid genesis block configuration, "
+ name
+ " must be a number but was '"
+ value
+ "'");
}
}

/* Converts the {@link JsonObject} describing the Genesis Block to a {@link Map}. This method
* converts all nested {@link JsonObject} to {@link Map} as well. Also, note that all keys are
* converted to lowercase for easier lookup since the keys in a 'genesis.json' file are assumed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,12 +106,12 @@ public void shouldDefaultMixHashToEmptyString() {

@Test
public void shouldGetNonce() {
assertThat(configWithProperty("nonce", "ABCD").getNonce()).isEqualTo("ABCD");
assertThat(configWithProperty("nonce", "0x10").getNonce()).isEqualTo("0x10");
}

@Test
public void shouldDefaultNonceToEmptyString() {
assertThat(EMPTY_CONFIG.getNonce()).isEmpty();
public void shouldDefaultNonceToZero() {
assertThat(EMPTY_CONFIG.getNonce()).isEqualTo("0x0");
}

@Test
Expand Down Expand Up @@ -164,13 +164,19 @@ public void shouldGetAllocations() {
entry("f17f52151ebef6c7334fad080c5704d77216b732", "90000000000000000000000"));
}

@Test
public void shouldGetEmptyAllocationsWhenAllocNotPresent() {
final GenesisConfigFile config = GenesisConfigFile.fromConfig("{}");
assertThat(config.getAllocations()).isEmpty();
}

private GenesisConfigFile configWithProperty(final String key, final String value) {
return GenesisConfigFile.fromConfig("{\"" + key + "\":\"" + value + "\"}");
}

private void assertInvalidConfiguration(final ThrowingCallable getter) {
assertThatThrownBy(getter)
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Invalid Genesis block configuration");
.hasMessageContaining("Invalid genesis block configuration");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
import tech.pegasys.pantheon.ethereum.storage.keyvalue.KeyValueStorageWorldStateStorage;
import tech.pegasys.pantheon.ethereum.worldstate.DefaultMutableWorldState;
import tech.pegasys.pantheon.services.kvstore.InMemoryKeyValueStorage;
import tech.pegasys.pantheon.util.bytes.Bytes32;
import tech.pegasys.pantheon.util.bytes.BytesValue;
import tech.pegasys.pantheon.util.uint.UInt256;

Expand All @@ -40,6 +39,7 @@
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand Down Expand Up @@ -153,38 +153,58 @@ private static <C> BlockHeader buildHeader(
private static Address parseCoinbase(final GenesisConfigFile genesis) {
return genesis
.getCoinbase()
.map(Address::fromHexString)
.map(str -> withNiceErrorMessage("coinbase", str, Address::fromHexString))
.orElseGet(() -> Address.wrap(BytesValue.wrap(new byte[Address.SIZE])));
}

private static <T> T withNiceErrorMessage(
final String name, final String value, final Function<String, T> parser) {
try {
return parser.apply(value);
} catch (final IllegalArgumentException e) {
throw createInvalidBlockConfigException(name, value, e);
}
}

private static IllegalArgumentException createInvalidBlockConfigException(
final String name, final String value, final IllegalArgumentException e) {
return new IllegalArgumentException(
"Invalid " + name + " in genesis block configuration: " + value, e);
}

private static Hash parseParentHash(final GenesisConfigFile genesis) {
return Hash.wrap(Bytes32.fromHexString(genesis.getParentHash()));
return withNiceErrorMessage("parentHash", genesis.getParentHash(), Hash::fromHexStringLenient);
}

private static BytesValue parseExtraData(final GenesisConfigFile genesis) {
return BytesValue.fromHexString(genesis.getExtraData());
return withNiceErrorMessage("extraData", genesis.getExtraData(), BytesValue::fromHexString);
}

private static UInt256 parseDifficulty(final GenesisConfigFile genesis) {
return UInt256.fromHexString(genesis.getDifficulty());
return withNiceErrorMessage("difficulty", genesis.getDifficulty(), UInt256::fromHexString);
}

private static Hash parseMixHash(final GenesisConfigFile genesis) {
return Hash.wrap(Bytes32.fromHexString(genesis.getMixHash()));
}

private static long parseNonce(final GenesisConfigFile genesis) {
String nonce = genesis.getNonce().toLowerCase(Locale.US);
if (nonce.startsWith("0x")) {
nonce = nonce.substring(2);
}
return Long.parseUnsignedLong(nonce, 16);
return withNiceErrorMessage("mixHash", genesis.getMixHash(), Hash::fromHexStringLenient);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you meant to make the mixHash parsing more lenient here^ ? Wonder if we should accept any zero-valued hash, and otherwise be strict ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this was deliberate (as with parentHash). It's unlikely that it will make any difference but since the mix hash for the genesis file really doesn't matter (as long as it's the same for all nodes) it seems nice to not have to add all the digits for no real reason. It doesn't seem worth the extra code to specifically allow it only for 0x0.

}

private static Stream<GenesisAccount> parseAllocations(final GenesisConfigFile genesis) {
return genesis.getAllocations().map(GenesisAccount::fromAllocation);
}

private static long parseNonce(final GenesisConfigFile genesis) {
return withNiceErrorMessage(
"nonce",
genesis.getNonce(),
value -> {
String nonce = value.toLowerCase(Locale.US);
if (nonce.startsWith("0x")) {
nonce = nonce.substring(2);
}
return Long.parseUnsignedLong(nonce, 16);
});
}

@Override
public String toString() {
return MoreObjects.toStringHelper(this)
Expand Down Expand Up @@ -213,8 +233,8 @@ private GenesisAccount(
final String balance,
final String hexCode,
final Map<String, Object> storage) {
this.address = Address.fromHexString(hexAddress);
this.balance = parseBalance(balance);
this.address = withNiceErrorMessage("address", hexAddress, Address::fromHexString);
this.balance = withNiceErrorMessage("balance", balance, this::parseBalance);
this.code = hexCode != null ? BytesValue.fromHexString(hexCode) : null;
this.storage = parseStorage(storage);
}
Expand All @@ -234,7 +254,10 @@ private Map<UInt256, UInt256> parseStorage(final Map<String, Object> storage) {
final Map<UInt256, UInt256> parsedStorage = new HashMap<>();
storage.forEach(
(key, value) ->
parsedStorage.put(UInt256.fromHexString(key), UInt256.fromHexString((String) value)));
parsedStorage.put(
withNiceErrorMessage("storage key", key, UInt256::fromHexString),
withNiceErrorMessage(
"storage value", String.valueOf(value), UInt256::fromHexString)));
return parsedStorage;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
"eip155Block": 0,
"eip158Block": 0
},
"alloc": {},
"coinbase": "0x0000000000000000000000000000000000000000",
"difficulty": "0x0000001",
"extraData": "",
Expand Down