[PAN-2946] - changes in core JSON-RPC method to support ReTestEth #1815

shemnon · 2019-08-01T20:55:48Z

PR description

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'ss namespaced as debug.

Also, rename the getter methods to align with standards.

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.

* DebugStorageRange is needed too. * ethGetBlockByNumber needs the coinbase in a field labeled "author" * Set a flag for the previous to by default not send the coinbase in response * jsonrpc spec tests to verify author not sent. * On a rejected transaciton eth_sendRawTransaction wants an hash of Empty * again, controlled by a flag with option off *

mbaxter · 2019-08-02T14:43:12Z

...c/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/DebugAccountRangeAt.java

+  @Override
+  protected Object resultByBlockNumber(final JsonRpcRequest request, final long blockNumber) {
+    final Object[] params = request.getParams();
+    final String addressHash = parameters.required(params, 2, String.class);


Suggested change

final String addressHash = parameters.required(params, 2, String.class);

final Hash addressHash = parameters.required(params, 2, Hash.class);

Retesteth likes to pass in odd length addresses, particularly zero addresses. Which is why I take it in as a string and at line 81 parse it in a lenient fashion. Going straight to hash throws Caused by: java.lang.IllegalArgumentException: Invalid odd-length hex binary representation 0

mbaxter · 2019-08-02T14:54:40Z

...c/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/DebugAccountRangeAt.java

+
+  @Override
+  protected BlockParameter blockParameter(final JsonRpcRequest request) {
+    return parameters.required(request.getParams(), 0, BlockParameter.class);


The docs say the first parameter should be a number or hash. As is, this will only handle numbers or tags ("latest", "earliest", etc).

If we want to match the spec, we could extend BlockParameter with a new BlockParameterOrHash class. Or we could push back since none of the other JSON-RPC API's work like this.

The docs say hash, but we never see a hash in practice. If it becomes a retesteth requirement I will probably go the BlockParameterOrHash route, but as of now the docs require a form that is not used.

LooksLike DebugStorageRangeAt will need the BlockParameterOrHash treatment. Recent change pushed into ReTestEth needs it now. :(

mbaxter · 2019-08-02T15:00:25Z

...c/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/DebugAccountRangeAt.java

+              .get()
+              .streamAccounts(Bytes32.fromHexStringLenient(addressHash), maxResults + 1)
+              .collect(Collectors.toList());
+      Bytes32 nextKey = Bytes32.ZERO;


For symmetry with debug_storageRangeAt, I think nextKey should be null (not returned) in this case.

retesteh requires the zeros return. Not the most fault tolerant code.

mbaxter · 2019-08-02T15:03:47Z

...onrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/results/BlockResult.java

@@ -89,6 +102,7 @@
    this.timestamp = Quantity.create(header.getTimestamp());
    this.ommers = ommers;
    this.transactions = transactions;
+    this.coinbase = includeCoinbase ? header.getCoinbase().toString() : null;


I think it's cleaner to just always include coinbase instead of adding a flag to switch on this. Shouldn't hurt anything if there's extra data returned that users aren't expecting. Kind of odd that it's not a standard field anyway.

Not only is it not in the JSON-RPC Spec, what reteseth wants is an author field which has the same meaning test wise as coinbase. If we expose it for all I would still want a flag and we expose coinbase not author.

mbaxter · 2019-08-02T15:10:52Z

...onrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/results/BlockResult.java

@@ -185,4 +199,10 @@ public String getTimestamp() {
  public List<TransactionResult> getTransactions() {
    return transactions;
  }
+
+  @JsonGetter(value = "author")


This is an odd name for this field 🤔

mbaxter · 2019-08-02T15:13:56Z

...m/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/EthJsonRpcHttpBySpecTest.java

@@ -216,28 +216,8 @@ Formatter will be turned on to make this easier to read (one spec per line)

    specs.put(EthNewPendingTransactionFilter.class, "eth_newPendingTransactionFilter");

-    specs.put(EthUninstallFilter.class, "eth_uninstallFilter_NonexistentFilter");
-    specs.put(EthUninstallFilter.class, "eth_uninstallFilter_FilterIdTooLong");
-    specs.put(EthUninstallFilter.class, "eth_uninstallFilter_FilterIdNegative");


What happened to these - bad merge??

bad merge. Will fix.

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

mbaxter · 2019-08-02T16:46:26Z

...src/test/resources/tech/pegasys/pantheon/ethereum/jsonrpc/debug_accountRangeAt_complete.json

+  "request": {
+    "id": 414,
+    "jsonrpc": "2.0",
+    "method": "debug_accountRangeAt",


This method needs a rename for the tests too

fixed, temporarily. Retesteth is changing the name but is unstable where they changed it.

mbaxter

LGTM pending test fixes

* add BlockParameterOrBlockHash * Wire up debug_storageRangeAt and debug_accountRange to use it * spec tests

shemnon · 2019-08-02T20:28:08Z

BlockParameterOrHash change is in, to both Debug methods.

shemnon added 3 commits August 1, 2019 14:54

Merge branch 'master' into retesteth-jsonrpc

6769ed5

shemnon marked this pull request as ready for review August 2, 2019 06:00

mbaxter reviewed Aug 2, 2019

View reviewed changes

review fixes

c7f73d3

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

mbaxter reviewed Aug 2, 2019

View reviewed changes

mbaxter approved these changes Aug 2, 2019

View reviewed changes

* fix tests names

a119842

* add BlockParameterOrBlockHash * Wire up debug_storageRangeAt and debug_accountRange to use it * spec tests

Merge branch 'master' into retesteth-jsonrpc

94df85d

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PAN-2946] - changes in core JSON-RPC method to support ReTestEth #1815

[PAN-2946] - changes in core JSON-RPC method to support ReTestEth #1815

shemnon commented Aug 1, 2019

mbaxter Aug 2, 2019

shemnon Aug 2, 2019

mbaxter Aug 2, 2019

shemnon Aug 2, 2019

shemnon Aug 2, 2019 •

edited

Loading

mbaxter Aug 2, 2019

shemnon Aug 2, 2019

mbaxter Aug 2, 2019

shemnon Aug 2, 2019

mbaxter Aug 2, 2019

shemnon Aug 2, 2019

mbaxter Aug 2, 2019

shemnon Aug 2, 2019

mbaxter Aug 2, 2019

shemnon Aug 2, 2019

mbaxter left a comment

shemnon commented Aug 2, 2019

	final String addressHash = parameters.required(params, 2, String.class);
	final Hash addressHash = parameters.required(params, 2, Hash.class);

[PAN-2946] - changes in core JSON-RPC method to support ReTestEth #1815

[PAN-2946] - changes in core JSON-RPC method to support ReTestEth #1815

Conversation

shemnon commented Aug 1, 2019

PR description

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 2, 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

mbaxter left a comment

Choose a reason for hiding this comment

shemnon commented Aug 2, 2019

shemnon Aug 2, 2019 •

edited

Loading