[PAN-2946] Retesteth support #1818

shemnon · 2019-08-03T02:32:48Z

PR description

Add support for retesteth, a JSON-RPC based reference testing API.

Fixed Issue(s)

PAN-2949 PAN-2950 PAN-2951 PAN-2935 PAN-2954 PAN-2955

and eth_getBlockByNumber

`./retesteth/retesteth -t BlockchainTests/bcStateTests -- --testpath ../../tests/ --clients pantheon` I suspect DebugStorageRangeAt is most of the problem.

Use GenesisState to do a lot of what was done by hand.

* Add header validation mode based on seal engine * rework account storage range, input is in decimal, so use existing super class * catch another invalid RLP

us from istanbul's chainId requirement.

* set the timestamp of the system (requires a genesis rebuild) * add a raw transaction * then mine the blocks.

# Conflicts: # ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/ProtocolScheduleBuilder.java

All tests run. Not all tests pass. There's some stuff relating to mining and protocol schedules I need to fix.

I nede to intercept several classes to get the block reward change everywhere it needs to be.

# Conflicts: # ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/DebuggableMutableWorldState.java # ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/WorldStateArchive.java # ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/results/DebugStorageRangeAtResult.java # ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/DebugStorageRangeAtTest.java

# Conflicts: # ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/DefaultMutableWorldState.java

# Conflicts: # ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/EthHashBlockCreator.java # ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/headervalidationrules/ProofOfWorkValidationRule.java

# Conflicts: # util/src/main/java/tech/pegasys/pantheon/util/uint/UInt256Value.java

# Conflicts: # ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/AbstractBlockCreator.java # ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetBlockProcessor.java

Some of the methods need to have a changeable reference to stuff like blockchainqueries. For the impacted methods the solution is to wrap them in a Supplier<> interface. This includes new constructors for re-used methods. Also include the debug_accountRangeAt method as it's namespaced as debug. Also, rename the getter methods to align with standards.

# Conflicts: # ethereum/blockcreation/src/test/java/tech/pegasys/pantheon/ethereum/blockcreation/EthHashBlockCreatorTest.java

* restore missing spec tests * rename to debig_accountRange * make sure stack trace come with invalidjsonrpcparams catch

# Conflicts: # ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/chain/DefaultMutableBlockchain.java # ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/DebugAccountRange.java # ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/DebugStorageRangeAt.java # ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/results/BlockResult.java # ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/results/BlockResultFactory.java # ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/results/DebugStorageRangeAtResult.java # ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/EthJsonRpcHttpBySpecTest.java

shemnon · 2019-08-03T16:18:49Z

Currently works with retesteth at branch 9a4609a5d7d84b61734c2dbb5d2d8cf4981b33e0

mbaxter

Haven't finished looking over everything - just dropping in a few comments for now 😄

mbaxter · 2019-08-05T21:04:47Z

ethereum/retesteth/src/main/java/tech/pegasys/pantheon/ethereum/retesteth/NoProofSolver.java

+public class NoProofSolver extends EthHashSolver {
+
+  NoProofSolver(final Iterable<Long> nonceGenerator) {
+    super(nonceGenerator, null);


What about creating an EthHashSolver interface, so we don't have to set null values in the superclass? Doesn't look like you're using anything significant from the superclass? Then the implementation EthHashSolver becomes DefaultEthHashSolver.

An interface already exists, and it's a functional interface. Reduced it to a lambda.

mbaxter · 2019-08-05T21:12:40Z

.../src/main/java/tech/pegasys/pantheon/ethereum/retesteth/NoRewardProtocolScheduleWrapper.java

+    final ProtocolSpec<C> original = delegate.getByBlockNumber(number);
+    // Pre Spurious Dragon we need to "touch" the accounts to create a zero-balance version.  We use
+    // the maximum possible reward as a sentinel value.
+    final boolean skipZeroBlockRewards = !TOUCH_ACCOUNT_SCHEDULES.contains(original.getName());


Can you just use:

Suggested change

final boolean skipZeroBlockRewards = !TOUCH_ACCOUNT_SCHEDULES.contains(original.getName());

final boolean skipZeroBlockRewards = original.isSkipZeroBlockRewards();

?

Yep. Style tweak to eliminate the variable since it's not calculated.

mbaxter · 2019-08-05T21:13:01Z

.../src/main/java/tech/pegasys/pantheon/ethereum/retesteth/NoRewardProtocolScheduleWrapper.java

+  public ProtocolSpec<C> getByBlockNumber(final long number) {
+    final ProtocolSpec<C> original = delegate.getByBlockNumber(number);
+    // Pre Spurious Dragon we need to "touch" the accounts to create a zero-balance version.  We use
+    // the maximum possible reward as a sentinel value.


Looks like an outdated comment here

mbaxter · 2019-08-05T21:14:05Z

.../src/main/java/tech/pegasys/pantheon/ethereum/retesteth/NoRewardProtocolScheduleWrapper.java

+        original.getTransactionReceiptFactory(),
+        original.getDifficultyCalculator(),
+        Wei.ZERO, // block reward
+        null, // transaction receipt type; unused


I think we should remove this param or set something here - thoughts?

Sounds like a reasonable activity, but that kind of refactor should stand on its own PR IMHO.

Turns out this is to support EIP-658, so no it cannot be removed. Perhaps a different comment.

If you look at the ProtocolSpec constructor, this field is never actually used so looks like it can be removed safely. Alternatively a less invasive change might be to add a getter for the field and set the value here to origin.getReceiptType() (plus log a ticket to remove later).

As a general principle, I want to avoid using nulls even if it makes sense right now because over time it will make the code more brittle and we've done a pretty good job of avoiding nulls up to this point.

Done in #1825

mbaxter · 2019-08-05T21:16:20Z

ethereum/retesteth/src/main/java/tech/pegasys/pantheon/ethereum/retesteth/RetestethContext.java

+  private EthHashSolver ethHashSolver;
+
+  public boolean resetContext(
+      final String genesisConfigString, final String sealEngine, final Optional<Long> clockTime) {


How about creating an enum for sealEngine rather than just letting it be any string?

My concern is how do we deal with new random values? The testing team has already shown that they will introduce random undocumented values ("NoReward") and I expect at least one other arbitrary add ("ProgPow" or some variant of the name). If we make an enum then unexpected test evolution becomes a parsing error and we fail with an RPC exception instead of bad data. Retesteh is already bad when it comes to reporting back data on the RPC failures (i.e. which test tripped it) but is much better about state failures.

So if PegaSys was controlling the code I'd say yes, but I don't feel we can trust the community code will behave well.

👍Makes sense to me as-is then

mbaxter · 2019-08-05T21:24:11Z

ethereum/retesteth/src/main/java/tech/pegasys/pantheon/ethereum/retesteth/RetestethContext.java

+
+    protocolSchedule =
+        MainnetProtocolSchedule.fromConfig(
+            JsonGenesisConfigOptions.fromJsonObject((ObjectNode) genesisConfig.get("config")));


(optional) There's also JsonUtil.getObjectNode which avoids the casting :D

rename debug_accountRange

mbaxter · 2019-08-06T20:05:40Z

.../src/main/java/tech/pegasys/pantheon/ethereum/retesteth/NoRewardProtocolScheduleWrapper.java

+        original.getTransactionReceiptFactory(),
+        original.getDifficultyCalculator(),
+        Wei.ZERO, // block reward
+        null, // transaction receipt type; unused


If you look at the ProtocolSpec constructor, this field is never actually used so looks like it can be removed safely. Alternatively a less invasive change might be to add a getter for the field and set the value here to origin.getReceiptType() (plus log a ticket to remove later).

As a general principle, I want to avoid using nulls even if it makes sense right now because over time it will make the code more brittle and we've done a pretty good job of avoiding nulls up to this point.

mbaxter · 2019-08-06T20:19:27Z

ethereum/retesteth/src/main/java/tech/pegasys/pantheon/ethereum/retesteth/RetestethContext.java

+
+    blockchainQueries = new BlockchainQueries(blockchain, worldStateArchive);
+
+    final String sealengine = JsonUtil.getString(genesisConfig, "sealengine", "");


Might be worth normalizing the sealengine value with toLowerCase().

sealEngine values are canonically mixed case, per the docs that do cover it.

Just makes everything more stable if casing inconsistencies crop up even if there's a supposed canonical form for each value.

mbaxter · 2019-08-06T20:21:54Z

ethereum/retesteth/src/main/java/tech/pegasys/pantheon/ethereum/retesteth/RetestethContext.java

+  /* Converts all to lowercase for easier lookup since the keys in a 'genesis.json' file are assumed
+   * case insensitive.
+   */
+  private static ObjectNode normalizeKeys(final ObjectNode genesis) {


Might be worth deduping this method by moving it to JsonUtil.

I'll do this and the receiptType in another PR by the end of the day and then merge them in.

done in #1826

mbaxter · 2019-08-06T20:26:22Z

ethereum/retesteth/src/main/java/tech/pegasys/pantheon/ethereum/retesteth/RetestethService.java

+    final JsonRpcParameter parameters = new JsonRpcParameter();
+    final BlockResultFactory blockResult = new BlockResultFactory();
+    final Map<String, JsonRpcMethod> jsonRpcMethods = new HashMap<>();
+    JsonRpcMethodsFactory.addMethods(


(nit) In retrospect seems unnecessary to publicly expose this method, when it's just looping over some methods and adding them to a hashmap.

It's either expose it publicly or duplicate it.

Personally, I'd duplicate - its not a lot of code 🤷‍♀

mbaxter · 2019-08-06T20:35:14Z

...retesteth/src/main/java/tech/pegasys/pantheon/ethereum/retesteth/methods/TestGetLogHash.java

+        context.getBlockchainQueries().transactionReceiptByTransactionHash(txHash);
+    return new JsonRpcSuccessResponse(
+        request.getId(),
+        receipt.map(this::calculateLogHash).orElse(Hash.EMPTY_LIST_HASH).toString());


Is Hash.EMPTY_LIST_HASH the right default? The general json-rpc pattern has been to return null if a value isn't found. The hash of empty list implies that the receipt was found and has no logs.

This is what retesteth specified when they defined the method. It does feel like an implementation detail leaking through but returning null breaks the RPC tests.

mbaxter · 2019-08-06T20:38:58Z

...retesteth/src/main/java/tech/pegasys/pantheon/ethereum/retesteth/methods/TestMineBlocks.java

+  public JsonRpcResponse response(final JsonRpcRequest request) {
+    long blocksToMine = parameters.required(request.getParams(), 0, Long.class);
+    while (blocksToMine-- > 0) {
+      if (!context.mineNewBlock()) {


Feels like this mineNewBlock method should be a method in this class, rather than sitting in context. When I see a "context" class I don't usually expect it to be doing real work, but just providing access to dependencies.

mbaxter · 2019-08-06T20:40:32Z

...teth/src/main/java/tech/pegasys/pantheon/ethereum/retesteth/methods/TestModifyTimestamp.java

+  @Override
+  public JsonRpcResponse response(final JsonRpcRequest request) {
+    final long epochSeconds = parameters.required(request.getParams(), 0, Long.class);
+    context.setTimestamp(epochSeconds);


Same here - seems like you should be getting the clock from context, and resetting the value in this method.

mbaxter · 2019-08-06T20:42:29Z

...esteth/src/main/java/tech/pegasys/pantheon/ethereum/retesteth/methods/TestRewindToBlock.java

+  public JsonRpcResponse response(final JsonRpcRequest request) {
+    final long blockNumber = parameters.required(request.getParams(), 0, Long.TYPE);
+
+    return new JsonRpcSuccessResponse(request.getId(), context.rewindToBlock(blockNumber));


Same comment as above - seems like you should be getting the blockchain from context and then rewinding here. Or alternatively, maybe context has an "actions" object that holds various retesteth actions? Seems more cohesive to just house the logic here though unless there's a lot of reuse.

mbaxter · 2019-08-06T21:05:41Z

...esteth/src/test/java/tech/pegasys/pantheon/ethereum/retesteth/methods/RetestethFixtures.java

+import com.google.common.base.Charsets;
+import com.google.common.io.Resources;
+
+class RetestethFixtures {


Looks like this class is unused.

Yea, when I duplicated the RPC methods I also duplicated the test classes and testing support. Looks like this is indeed dead and should be removed.

Move the `normalizeKeys` method into JsonUtil as Retesteth will re-use it.

mbaxter

Looks good to me!

mbaxter · 2019-08-07T20:55:52Z

ethereum/retesteth/src/main/java/tech/pegasys/pantheon/ethereum/retesteth/RetestethContext.java

+
+    blockchainQueries = new BlockchainQueries(blockchain, worldStateArchive);
+
+    final String sealengine = JsonUtil.getString(genesisConfig, "sealengine", "");


Just makes everything more stable if casing inconsistencies crop up even if there's a supposed canonical form for each value.

mbaxter · 2019-08-07T20:58:27Z

ethereum/retesteth/src/main/java/tech/pegasys/pantheon/ethereum/retesteth/RetestethService.java

+    final JsonRpcParameter parameters = new JsonRpcParameter();
+    final BlockResultFactory blockResult = new BlockResultFactory();
+    final Map<String, JsonRpcMethod> jsonRpcMethods = new HashMap<>();
+    JsonRpcMethodsFactory.addMethods(


Personally, I'd duplicate - its not a lot of code 🤷‍♀

shemnon added 30 commits July 12, 2019 20:59

rough shell that starts up jsonrpc services.

2e9dd3b

add logging

9d56e43

implement test_setChainParams, test_importRawBlock, eth_getBlockNumber,

331d4ea

and eth_getBlockByNumber

Half done debug account range at.

5ec7d65

enough APIs to get a complete (error-filled) run from

57bd060

`./retesteth/retesteth -t BlockchainTests/bcStateTests -- --testpath ../../tests/ --clients pantheon` I suspect DebugStorageRangeAt is most of the problem.

Quick! Commit! bcStateTests worked!

a3ca31e

need to handle no block found for storage

0835b49

make GenesisState support nonces

77c932b

Use GenesisState to do a lot of what was done by hand.

some test cases

4fec34c

address range tests

a6c808c

continues make it faster

d256ae4

More tests run now.

54daa51

* Add header validation mode based on seal engine * rework account storage range, input is in decimal, so use existing super class * catch another invalid RLP

host whitelist

5db2ee6

Merge branch 'master' of github.com:shemnon/pantheon into retesteth

5c53fb6

Have difficulty 1 mean Int256_MAX_INT instead of 33 bytes of sadness.

2f68a45

retesteth is not JSON-RPC compliant.

8c05c5f

spotless

da667ef

Move the block number check outside the addProtocolSpec call. Shields

a38a7ad

us from istanbul's chainId requirement.

cluster of related apis, with dependencies

87d42bf

* set the timestamp of the system (requires a genesis rebuild) * add a raw transaction * then mine the blocks.

Merge branch 'master' of github.com:PegaSysEng/pantheon into retesteth

5b7f18b

# Conflicts: # ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/ProtocolScheduleBuilder.java

add test_getLogHash

ffdba2f

All tests run. Not all tests pass. There's some stuff relating to mining and protocol schedules I need to fix.

handle "NoReward" seal engines.

00045ed

I nede to intercept several classes to get the block reward change everywhere it needs to be.

rewind to block number.

f09613d

Merge branch 'master' of github.com:PegaSysEng/pantheon into retesteth

518c910

# Conflicts: # ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/DefaultMutableWorldState.java

undo protocolschedule changes

3bc4486

Merge branch 'master' of github.com:PegaSysEng/pantheon into retesteth

33d0f5a

don't tick clock for mining.

f1426f2

redo rewind and set timestamp

d57792a

shemnon added 11 commits August 1, 2019 20:09

javadoc

7673361

Merge branch 'master' of github.com:PegaSysEng/pantheon into retesteth

17cc224

# Conflicts: # util/src/main/java/tech/pegasys/pantheon/util/uint/UInt256Value.java

Merge branch 'retesteth-noreward' into retesteth

168799a

# Conflicts: # ethereum/blockcreation/src/main/java/tech/pegasys/pantheon/ethereum/blockcreation/AbstractBlockCreator.java # ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetBlockProcessor.java

fix skip zero rewards

f46ed0c

reuse core Json-RPC

3e90617

Merge branch 'master' of github.com:PegaSysEng/pantheon into retesteth

f176686

# Conflicts: # ethereum/blockcreation/src/test/java/tech/pegasys/pantheon/ethereum/blockcreation/EthHashBlockCreatorTest.java

review fixes

8eb47f9

* restore missing spec tests * rename to debig_accountRange * make sure stack trace come with invalidjsonrpcparams catch

cleanup

b270d9a

cleanup

826cde2

mbaxter reviewed Aug 5, 2019

View reviewed changes

shemnon added 7 commits August 5, 2019 16:22

update retesteth to commit 61df0467

4283d7d

rename debug_accountRange

revice noProof hasher

f0041f8

advance clock on mining always

a4f27b3

add istanbul fork block

a5a7035

Merge branch 'master' of github.com:PegaSysEng/pantheon into retesteth

683500f

we have the data in the protocol schedules already.

5633736

less casting.

a54acd4

mbaxter reviewed Aug 6, 2019

View reviewed changes

shemnon and others added 6 commits August 6, 2019 17:10

Merge branch 'master' of github.com:PegaSysEng/pantheon into retesteth

5f3d102

[PAN-2946] refactor normalizeKeys method

581523b

Move the `normalizeKeys` method into JsonUtil as Retesteth will re-use it.

javadoc

efc821c

review changes

52ae166

Merge branch 'master' of github.com:PegaSysEng/pantheon into retesteth

f156c0f

Merge branch 'master' into retesteth

46876a1

mbaxter approved these changes Aug 7, 2019

View reviewed changes

Merge branch 'master' into retesteth

def68ad

shemnon merged commit 2d478de into PegaSysEng:master Aug 7, 2019

	final boolean skipZeroBlockRewards = !TOUCH_ACCOUNT_SCHEDULES.contains(original.getName());
	final boolean skipZeroBlockRewards = original.isSkipZeroBlockRewards();


		blockchainQueries = new BlockchainQueries(blockchain, worldStateArchive);

		final String sealengine = JsonUtil.getString(genesisConfig, "sealengine", "");

[PAN-2946] Retesteth support #1818

[PAN-2946] Retesteth support #1818

Conversation

shemnon commented Aug 3, 2019 • edited Loading

PR description

Fixed Issue(s)

shemnon commented Aug 3, 2019

mbaxter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shemnon Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

mbaxter Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbaxter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shemnon commented Aug 3, 2019 •

edited

Loading

shemnon Aug 5, 2019 •

edited

Loading

mbaxter Aug 6, 2019 •

edited

Loading