Implement Eth64 support #82

holgerd77 · 2020-07-01T11:55:38Z

Fixes #55

Some note: reconsidered on the Common integration and decided to integrate deeper - see #55 for initial reservations on this. I think this will much more practical and less error prone when implementing in the client when the HF from Common is aligned with the HF set by the latest block download in Common. This information will then be directly taken to doing the right forkId comparisons, otherwise this would have to be updated manually from the outside.

lgtm-com · 2020-07-01T12:06:56Z

This pull request introduces 1 alert when merging 7863e0e into 826a06b - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

evertonfraga · 2020-07-01T13:49:31Z

@holgerd77 sure, this will go along the recent goerli changes.

holgerd77 · 2020-07-01T20:43:29Z

src/eth/index.ts

+      this._forkHash = c.forkHash(this._hardfork)
+      // TODO: functionality to get next fork block number could also be integrated into Common
+      const hfBlock: number = c.hardforks().filter((hf: any) => hf.name === 'chainstart')[0].block
+      // Next fork block number or 0 if none available


@evertonfraga thanks, I think I will open a small PR on this towards Common as well before a release, makes really more sense to add this code along a small nextForkBlockNumber() function to Common which we can then reuse.

holgerd77 · 2020-07-03T17:06:59Z

Ok, I think I have gotten the Eth64 validation passing with another real-life Geth peer when running DEBUG=devp2p:eth:* node -r ts-node/register ./examples/peer-communication.ts, the example is not yet working though, here is some typical log extract:

Add peer: 13.209.88.20:30303 Geth/v1.9.10-stable-58cf5686/linux-amd64/go1.13.6 (eth64) (total: 2)
Remove peer: 61.144.21.203:28303 - we disconnect, reason: SUBPROTOCOL_ERROR (16) (total: 1)
[
  <Buffer 40>,
  <Buffer 01>,
  <Buffer 29 33 d4 dc ba 9e ff 2e d2>,
  <Buffer 9b 30 9f c6 db f7 9a 57 92 9c 0e 2a 4a e0 10 77 49 7b 43 b7 05 49 58 f9 f8 d3 f6 d1 e6 2b 93 1f>,
  <Buffer d4 e5 67 40 f8 76 ae f8 c0 10 b8 6a 40 d5 f5 67 45 a1 18 d0 90 6a 34 e6 9a ec 8c 0d b1 cb 8f a3>,
  [ <Buffer 3e dd 5b 10>, <Buffer 42 ae 50> ]
]
Successful Eth64 validation with 13.209.88.20
13.209.88.20:30303 expected one header for Byzantium Fork verify (received: 0)
Add peer: 163.172.134.155:30303 OpenEthereum/v3.0.1-stable-8ca8089-20200601/x86_64-unknown-linux-gnu/rustc1.43.1 (eth64) (total: 2)
Remove peer: 163.172.134.155:30303 - peer disconnect, reason: TOO_MANY_PEERS (4) (total: 1)
Remove peer: 13.209.88.20:30303 - we disconnect, reason: USELESS_PEER (3) (total: 0)

I'm also not sure about the correct implementation of the various validation rules from the EIP, this is trickier than thought on first glance.

holgerd77 · 2020-07-03T21:28:21Z

Fixed one bug, this is now working and syncing on the example. 😄 Needs a couple of days more for some reevaluation and polishing before review.

evertonfraga · 2020-08-10T17:01:11Z

Update on this issue:

Common 1.5.1 had a commit cherry-pick to get specifically the goerli fix from master. To import this set of changes, i would need to pull not only the forkHash changes, but several others, to avoid a merge conflict rabbit hole.

So I will create branch common/1.6.x based off of master, downgrade the package versions (currently at 2.0.0) and names (currently @ethereumjs/common).

codecov · 2020-11-17T15:55:56Z

Codecov Report

Merging #82 (77625aa) into master (2ecc7df) will decrease coverage by 0.51%.
The diff coverage is 74.13%.

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
- Coverage   83.39%   82.88%   -0.52%     
==========================================
  Files          15       15              
  Lines        1162     1215      +53     
  Branches      191      205      +14     
==========================================
+ Hits          969     1007      +38     
- Misses         73       82       +9     
- Partials      120      126       +6

Impacted Files	Coverage Δ
src/eth/index.ts	`79.38% <71.69%> (-5.62%)`	⬇️
src/les/index.ts	`82.24% <100.00%> (ø)`
src/rlpx/peer.ts	`74.90% <100.00%> (+0.09%)`	⬆️
src/rlpx/rlpx.ts	`79.83% <100.00%> (+0.16%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ecc7df...77625aa. Read the comment docs.

README.md

…mon instance

holgerd77 · 2020-11-20T08:48:59Z

Ok, this is now ready for review. 😄

I gave this a final test run with the peer-communication.ts example (DEBUG=devp2p:* node -r ts-node/register ./examples/peer-communication.ts) with solely ETH/64 enabled and this repeatedly came to a connection and exchanged blocks/txs. So I think this should be relatively safe to merge.

Some example output:

New tx: d1048490fa28d3c0c7b35687081eb935a17ba1f6f6306481664e59e396c70131 (from 168.119.68.101:30303)
New tx: 81642506e684c9f505efda9c2a26c30d0cc695264bb2d5846b6306b7ee554bb3 (from 168.119.68.101:30303)
New tx: 45769f2bcc948668f2badc016ee8e94e554d756f457dc2299371a71df6aa189d (from 168.119.68.101:30303)
New tx: 4236b96a92261c03a32e209c16a5a08ad1fad51438659940b2526093d15537c3 (from 168.119.68.101:30303)
New tx: b53c61acabcc89df4f98ef6991d1cfa8f9dac65a39bf45b7e8de8fc93262281a (from 168.119.68.101:30303)
New tx: 57ce2d23506c150cba675d8196f0cf32f0d4615bf7d4a923a4471127daf443b0 (from 168.119.68.101:30303)
New tx: b8f6a376e186df60d6202a50d69bc8e62d6e572338025af63e9ff94f1dfff177 (from 168.119.68.101:30303)
New tx: 5d6c07f90a5e87540387a0b06c9da99bf40717e38d3519e7064daba7aaa72819 (from 168.119.68.101:30303)
New tx: 9232ca175632c56fa306eaf68ab169dccfb8105a7a5d67d05ab20be061e28359 (from 168.119.68.101:30303)
  devp2p:rlpx:peer Received header 168.119.68.101:30303 +129ms
  devp2p:rlpx:peer Received body 168.119.68.101:30303 16f992d9f992d6f990b6f8708317350a850165a0bc00830186a0945338b0... +0ms
  devp2p:rlpx:peer Received undefined (message code: 22 - 16 = 6) 168.119.68.101:30303 +1ms
  devp2p:eth Received BLOCK_BODIES message from 168.119.68.101:30303: f992d9f992d6f990b6f8708317350a850165a0bc00830186a0945338b001... +1ms
----------------------------------------------------------------------------------------------------------
New block 11293905: 2db452b6965ffd789f08aae755e1425ca7bd181ca2fe62f5c80fb68baded27e9 (from 168.119.68.101:30303)
----------------------------------------------------------------------------------------------------------
New tx: 7dcce6f4cefa79b54b44688782348603dc266f7169fbd727ef53a1f563031d66 (from 168.119.68.101:30303)
New tx: 5aa156aac123350861075150e061d9546515de4da6f32eaaccc46eac31527472 (from 168.119.68.101:30303)
New tx: f6641f4efb10fc170f7bf8413c2736083d180ee06992cbb1cef1628d4ae10e29 (from 168.119.68.101:30303)
  devp2p:rlpx refill connections.. queue size: 2, peers: 1, open slots: 24 +2s
  devp2p:dpt:server ping timeout: 3.209.45.79:30303 - +49ms
  devp2p:dpt:server ping timeout: 34.255.23.113:30303 - +81ms
  devp2p:dpt:server ping timeout: 13.52.56.43:30304 - +64ms
  devp2p:dpt:server ping timeout: 211.23.221.214:42371 - +39ms
  devp2p:dpt:server ping timeout: 119.23.30.242:30303 - +54ms
  devp2p:dpt:server ping timeout: :30303 - +156ms
  devp2p:dpt:server ping timeout: :::59997 - +178ms

I first wanted to remove the lint exception '@typescript-eslint/no-unnecessary-condition': 'off' again, but this doesn't seem to be as easy a side task as I would have anticipated, rule has some problems with options setting and mentioning lines like this._timeout = options.timeout || ms('10s') as not able to be undefined while TypeScript shows me that options.timeout can very well be undefined. Also tried nullish coalescing operator, also no luck. So this seems to be dedicated task to me for a separate PR, and also doesn't have that much of an urgency since we mainly want to merge here to get the repo prepared for the monorepo switch.

jochem-brouwer

Had to lookup this EIP, 2 small comments, one question about the tests.

jochem-brouwer · 2020-11-23T22:08:57Z

src/eth/index.ts

+    const peerNextFork = buffer2int(forkId[1])
+
+    if (this._forkHash === peerForkHash) {
+      if (peerNextFork) {


Style nit: assumes knowledge that 0 is regarded as false.

Updated (next push).

Are you otherwise confident that the validation rules (see equally named section in the EIP-2124 specs are adequately covered? I had my problems here since the rules are stating the success cases and are very vague on the error cases ("Reject in all other cases") and I had to do somewhat of the inverse (implement the error cases and succeed in all other cases 😋 ) way around.

A bit unfortunate that this is not more exactly covered by the spec, would assume it wouldn't be that hard to just state all possible cases there.

I will re-check this 👍

jochem-brouwer · 2020-11-23T22:17:52Z

test/integration/eth-simulator.ts

-test('ETH: send allowed eth62', async t => {
-  const cap = [devp2p.ETH.eth62]
+test('ETH: should work with allowed eth64', async t => {
+  sendWithProtocolVersion(t, 64)


This test is exactly the same as the one above?

Yes, the test cases are semantically different and I wanted to have it in a way that someone updating to a new protocol version (ETH/65) is triggered to on that occasion update the first test case.

jochem-brouwer · 2020-11-23T22:20:45Z

test/integration/eth-simulator.ts

-      t.end()
-    }
+  opts.onPeerError0 = function(err: Error, rlpxs: any) {
+    const msg = 'Remote is advertising a future fork that passed locally'


We might want to create some kind of global error-string object, such that this test doesn't suddenly fail if we change the error string. We can then test if the msg is of a certain Error (an enum or something just like we did in VM). (Not a blocker)

Yeah, that's a good idea, @rumkin had also already been working on a similar proposal on the VM ethereumjs/ethereumjs-monorepo#487 (that one is further going though - just wanted to mention here. For a first round it definitely makes sense to just start with the ERROR strings).

Yes, I agree, this is something we should do midterm, or at the point where these "magic strings" are in "too many" places.

holgerd77 · 2020-11-24T10:52:16Z

@jochem-brouwer ok, this is now ready for re-review. 😄

jochem-brouwer

OK, as of now, I am pretty much convinced that the implementation follows the 4 rules of the validation correctly 👍 Great work! 😄

holgerd77 · 2020-11-24T18:31:15Z

Nice, thanks!

holgerd77 added the PR state: WIP label Jul 1, 2020

holgerd77 commented Jul 1, 2020

View reviewed changes

holgerd77 mentioned this pull request Jul 2, 2020

New utility methods for Common (loosely connected to Eth64 implementation) ethereumjs/ethereumjs-monorepo#805

Merged

holgerd77 force-pushed the implement-eth64-support branch from 7863e0e to e5b1f62 Compare July 3, 2020 17:02

holgerd77 force-pushed the implement-eth64-support branch from e5b1f62 to d4a3627 Compare July 3, 2020 21:27

holgerd77 force-pushed the implement-eth64-support branch from d4a3627 to 61f52e0 Compare July 3, 2020 21:33

holgerd77 force-pushed the implement-eth64-support branch 2 times, most recently from cb0f2e3 to 66c8ca0 Compare November 17, 2020 15:50

ryanio reviewed Nov 17, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

ryanio reviewed Nov 17, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

holgerd77 force-pushed the implement-eth64-support branch from fb2a99b to 64a2758 Compare November 17, 2020 19:59

holgerd77 added 4 commits November 17, 2020 21:00

Use Common for networkId in ETH and LES protocols, init Rlpx with Com…

ac09ad2

…mon instance

Added support for Eth64 protocol version

aa7f6f0

Updated @ethereumjs/common to v2.0.0-beta.2

9cfba1e

Fixed build command, rebase fixes

05ecd8d

holgerd77 force-pushed the implement-eth64-support branch from 64a2758 to 05ecd8d Compare November 17, 2020 20:00

holgerd77 added PR state: needs review and removed PR state: WIP labels Nov 23, 2020

holgerd77 requested review from cgewecke and jochem-brouwer November 23, 2020 21:49

jochem-brouwer reviewed Nov 23, 2020

View reviewed changes

eth64: minor fixes

77625aa

holgerd77 force-pushed the implement-eth64-support branch from 676dd99 to 77625aa Compare November 24, 2020 10:45

jochem-brouwer approved these changes Nov 24, 2020

View reviewed changes

holgerd77 merged commit 1402808 into master Nov 24, 2020

holgerd77 deleted the implement-eth64-support branch November 24, 2020 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Eth64 support #82

Implement Eth64 support #82

holgerd77 commented Jul 1, 2020 •

edited

Loading

lgtm-com bot commented Jul 1, 2020

evertonfraga commented Jul 1, 2020

holgerd77 Jul 1, 2020

holgerd77 commented Jul 3, 2020

holgerd77 commented Jul 3, 2020

evertonfraga commented Aug 10, 2020 •

edited

Loading

codecov bot commented Nov 17, 2020 •

edited

Loading

holgerd77 commented Nov 20, 2020

jochem-brouwer left a comment

jochem-brouwer Nov 23, 2020

holgerd77 Nov 24, 2020

holgerd77 Nov 24, 2020

jochem-brouwer Nov 24, 2020

jochem-brouwer Nov 23, 2020

holgerd77 Nov 24, 2020

jochem-brouwer Nov 23, 2020

holgerd77 Nov 24, 2020

jochem-brouwer Nov 24, 2020

holgerd77 commented Nov 24, 2020

jochem-brouwer left a comment

holgerd77 commented Nov 24, 2020

Implement Eth64 support #82

Implement Eth64 support #82

Conversation

holgerd77 commented Jul 1, 2020 • edited Loading

lgtm-com bot commented Jul 1, 2020

evertonfraga commented Jul 1, 2020

Choose a reason for hiding this comment

holgerd77 commented Jul 3, 2020

holgerd77 commented Jul 3, 2020

evertonfraga commented Aug 10, 2020 • edited Loading

codecov bot commented Nov 17, 2020 • edited Loading

Codecov Report

holgerd77 commented Nov 20, 2020

jochem-brouwer 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

holgerd77 commented Nov 24, 2020

jochem-brouwer left a comment

Choose a reason for hiding this comment

holgerd77 commented Nov 24, 2020

holgerd77 commented Jul 1, 2020 •

edited

Loading

evertonfraga commented Aug 10, 2020 •

edited

Loading

codecov bot commented Nov 17, 2020 •

edited

Loading