NC-1244 Implement JSON-RPC method "eth_getWork" #70

Shinabyss · 2018-10-16T06:43:02Z

PR Details

Implement JSON-RPC method eth_getWork same as geth

Description

Issue NC-1244

Related Issue

Motivation and Context

Implement JSON-RPC method eth_getWork same as corresponding function as geth

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Special notes for your reviewer:

Release note

NONE

Prerequisites:

I have read the CONTRIBUTING.md document.
This code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added appropriate tests that prove my change is effective
New and existing tests pass locally with my changes

…NC-1244

Debugged

lucassaldanha

Requested a couple of changes. Also, I think it is important to get Trent's review on the mining changes.

lucassaldanha · 2018-10-18T00:51:02Z

...s/src/test/java/tech/pegasys/pantheon/tests/acceptance/jsonrpc/EthGetWorkAcceptanceTest.java

+    assertThat(thrown).isInstanceOf(ClientConnectionException.class);
+    assertThat(thrown.getMessage())
+        .contains(
+            "\"error\" : {\n"


Using \n can break the build on other OS (e.g. Windows). I suggest that you only check if the message contains no mining work available yet instead of trying to match the json object.

...s/src/test/java/tech/pegasys/pantheon/tests/acceptance/jsonrpc/EthGetWorkAcceptanceTest.java

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/EthHash.java

lucassaldanha · 2018-10-18T00:59:00Z

...rpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcError.java

@@ -29,6 +29,7 @@
  // Filter & Subscription Errors
  FILTER_NOT_FOUND(-32000, "Filter not found"),
  SUBSCRIPTION_NOT_FOUND(-32000, "Subscription not found"),
+  NO_MINING_WORK_FOUND(-32000, "no mining work available yet"),


All other error messages start with a capital letter.

geth wasn't capital thus it was intentionally not made capital, but i will change this regardless

lucassaldanha · 2018-10-18T01:00:09Z

...sonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/EthGetWork.java

+      };
+      return new JsonRpcSuccessResponse(req.getId(), result);
+    } else {
+      LOG.error("Mining is not operational, eth_getWork request cannot be processed");


This is not an error (from the application point of view). It is doing exactly what is expected. Change it to a debug/trace level.

rain-on · 2018-10-18T02:46:47Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/DirectAcyclicGraphSeed.java

@@ -0,0 +1,38 @@
+/*


Looks like this is a straight copy from the EthHash?

nit: Nice idea, but maybe not relevant to this changeset.

Taken out of the EthHash because getwork need it

rain-on · 2018-10-18T02:50:19Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/EthHash.java

@@ -58,7 +58,7 @@

  private static final int ACCESSES = 64;

-  private static final ThreadLocal<MessageDigest> KECCAK_256 =
+  public static final ThreadLocal<MessageDigest> KECCAK_256 =


seems like a bad idea making this "ThreadLocal" variable publicly visible - can it be moved into the DagSeed class?

Made this public variable on DagSeed class, the final should prevent it from being altered.

rain-on · 2018-10-18T02:52:11Z

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/EthHashSolverInputs.java

@@ -17,12 +17,13 @@
 public class EthHashSolverInputs {
  private final UInt256 target;
  private final byte[] prePowHash;
-  private final long dagSeed; // typically block number
+  private final long blockNumber;


Not your problem: But is this the right information to be exposed to the JSON RPC? I had figured this was the dagseed hash, but tbh was guessing based on our algo requirements.

from what @CjHare and I could gather, the dagSeed is a hash

…NC-1244

Debugged Change Requests Actioned

NicolasMassart · 2018-10-18T07:15:42Z

This PR needs ad proper description !

lucassaldanha

Thanks for addressing my comments.
Just need to double check with Trent that the mining changes are ok.

Shinabyss added 4 commits October 16, 2018 08:34

EthGetWork Added with Unit and acceptance tests

69ee7d5

EthGetWork Added with Unit and acceptance tests

7413ee0

EthGetWork Added with Unit and acceptance tests

b7e961a

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

96f3739

…NC-1244

NicolasMassart self-requested a review October 17, 2018 09:11

NicolasMassart added the work in progress Work on this pull request is ongoing label Oct 17, 2018

Shinabyss changed the title ~~Nc 1244~~ NC-1244 Implement JSON-RPC method "eth_getWork" Oct 17, 2018

Shinabyss added 2 commits October 17, 2018 20:25

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

21497a4

…NC-1244

EthGetWork Added with Unit and acceptance tests

c201595

Debugged

Shinabyss removed the work in progress Work on this pull request is ongoing label Oct 18, 2018

Shinabyss added 2 commits October 18, 2018 10:05

Merge branch 'master' into NC-1244

cd4b99c

Merge branch 'master' into NC-1244

743418d

lucassaldanha suggested changes Oct 18, 2018

View reviewed changes

lucassaldanha requested a review from rain-on October 18, 2018 01:02

rain-on reviewed Oct 18, 2018

View reviewed changes

Shinabyss added 5 commits October 18, 2018 13:20

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

c0d82dc

…NC-1244

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

9443f24

…NC-1244

EthGetWork Added with Unit and acceptance tests

3a7ddde

Debugged Change Requests Actioned

Merge remote-tracking branch 'origin/NC-1244' into NC-1244

7cfb943

Merge branch 'master' into NC-1244

318cf2f

Merge branch 'master' into NC-1244

5ffdbda

lucassaldanha approved these changes Oct 18, 2018

View reviewed changes

Merge branch 'master' into NC-1244

906cf2a

Shinabyss merged commit 1325a41 into PegaSysEng:master Oct 19, 2018

Shinabyss deleted the NC-1244 branch October 19, 2018 00:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NC-1244 Implement JSON-RPC method "eth_getWork" #70

NC-1244 Implement JSON-RPC method "eth_getWork" #70

Shinabyss commented Oct 16, 2018 •

edited

Loading

lucassaldanha left a comment

lucassaldanha Oct 18, 2018

lucassaldanha Oct 18, 2018

Shinabyss Oct 18, 2018

lucassaldanha Oct 18, 2018

rain-on Oct 18, 2018

Shinabyss Oct 18, 2018

rain-on Oct 18, 2018

Shinabyss Oct 18, 2018

rain-on Oct 18, 2018

Shinabyss Oct 18, 2018

NicolasMassart commented Oct 18, 2018

lucassaldanha left a comment

NC-1244 Implement JSON-RPC method "eth_getWork" #70

NC-1244 Implement JSON-RPC method "eth_getWork" #70

Conversation

Shinabyss commented Oct 16, 2018 • edited Loading

PR Details

Description

Related Issue

Motivation and Context

Types of changes

Release note

Prerequisites:

lucassaldanha 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

NicolasMassart commented Oct 18, 2018

lucassaldanha left a comment

Choose a reason for hiding this comment

Shinabyss commented Oct 16, 2018 •

edited

Loading