Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement eth/68 #805

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

feat: implement eth/68 #805

wants to merge 10 commits into from

Conversation

jonastheis
Copy link

@jonastheis jonastheis commented Jun 6, 2024

1. Purpose or design rationale of this PR

This PR implements the eth/68 protocol from upstream.

Cherry-picked from

I made sure that tests in eth/fetcher/tx_fetcher_test.go pass. Tests in eth/fetcher/block_fetcher_test.go failed already on develop.
Similarly, tests in cmd/devp2p/internal/ethtest/chain_test.go pass. Tests in cmd/devp2p/internal/ethtest/suite_test.go failed already on develop.
Extended tests to also test for new eth/68 protocol negotiation.

2. PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • build: Changes that affect the build system or external dependencies (example scopes: yarn, eslint, typescript)
  • ci: Changes to our CI configuration files and scripts (example scopes: vercel, github, cypress)
  • docs: Documentation-only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that doesn't fix a bug, or add a feature, or improves performance
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • test: Adding missing tests or correcting existing tests

3. Deployment tag versioning

Has the version in params/version.go been updated?

  • This PR doesn't involve a new deployment, git tag, docker image tag, and it doesn't affect traces
  • Yes

4. Breaking change label

Does this PR have the breaking-change label?

  • This PR is not a breaking change
  • Yes

@0xmountaintop
Copy link
Member

just wondering:
is f48f501 the same as cherry-picking ethereum#28261 then fixing conflicts?
If so, can we do it this way (cherry-pick + fix conflicts, instead of re-implement by yourself)?

this makes it easier to track the changes

@jonastheis
Copy link
Author

just wondering: is f48f501 the same as cherry-picking ethereum#28261 then fixing conflicts? If so, can we do it this way (cherry-pick + fix conflicts, instead of re-implement by yourself)?

this makes it easier to track the changes

There were just a lot of conflicts so I felt it was easier this way. I went back and cherry-picked that commit and updated the branch with a force push.

0xmountaintop
0xmountaintop previously approved these changes Jun 7, 2024
@0xmountaintop
Copy link
Member

0xmountaintop commented Jun 13, 2024

how to test

  1. push a tag, then it will publish a docker image (see https://github.com/scroll-tech/go-ethereum/blob/develop/.github/workflows/docker.yaml)
  2. update https://github.com/scroll-tech/testnet/blob/sepolia/core/l2geth/Dockerfile#L3 (sepolia branch)
  3. deploy one of the bootnodes (connect to VPN -> login to jenkins -> deploy)
  4. revert the change in https://github.com/scroll-tech/testnet in case other people misdeploy this image to other nodes.
  5. launch a node locally, check whether it could comm with this bootnode and sync
  6. ask @HAOYUatHZ (me) to check whether my branch can comm with this bootnode and sync

0xmountaintop
0xmountaintop previously approved these changes Jul 2, 2024
Copy link
Member

@0xmountaintop 0xmountaintop left a comment

Choose a reason for hiding this comment

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

tested. works good.

@jonastheis
Copy link
Author

Just for completeness: I've also tested this with sepolia-l2geth-bootnode2 (deployed this jt/eth-68 branch) to it. Works both ways:

local: jt/eth-68, remote: scroll-v5.4.8-ccc-0.11.1

> admin.peers
[{
    caps: ["eth/66"],
    enode: "enode://dd1ac5433c5c2b04ca3166f4cb726f8ff6d2da83dbc16d9b68b1ea83b7079b371eb16ef41c00441b6e85e32e33087f3b7753ea9e8b1e3f26d3e4df9208625e7f@54.148.111.168:30303",
    id: "a704bc011f205aecff8a13fcaa18b85bcf0b075de53822a22534337d800ff90c",
    name: "Geth/v5.4.7-mainnet/linux-amd64/go1.21.1",
    network: {
      inbound: false,
      localAddress: "...",
      remoteAddress: "54.148.111.168:30303",
      static: false,
      trusted: false
    },
    protocols: {
      eth: {
        difficulty: 9968979,
        head: "0x67e517959c9b199bd3afa3cdedde1156611b430144ee7d39549d4deac36c3a73",
        version: 66
      }
    }
}]

local: jt/eth-68, remote: jt/eth-68

> admin.peers
[{
    caps: ["eth/66", "eth/67", "eth/68"],
    enode: "enode://dd1ac5433c5c2b04ca3166f4cb726f8ff6d2da83dbc16d9b68b1ea83b7079b371eb16ef41c00441b6e85e32e33087f3b7753ea9e8b1e3f26d3e4df9208625e7f@54.148.111.168:30303",
    id: "a704bc011f205aecff8a13fcaa18b85bcf0b075de53822a22534337d800ff90c",
    name: "Geth/v5.5.1-mainnet/linux-amd64/go1.21.1",
    network: {
      inbound: false,
      localAddress: "...",
      remoteAddress: "54.148.111.168:30303",
      static: false,
      trusted: false
    },
    protocols: {
      eth: {
        difficulty: 9975835,
        head: "0xb448fd75e5c2aa6469106bb21663b202ea0600eaa74ad74086c80839840d1697",
        version: 68
      }
    }
}]

local: develop, remote: jt/eth-68

> admin.peers
[{
    caps: ["eth/66", "eth/67", "eth/68"],
    enode: "enode://dd1ac5433c5c2b04ca3166f4cb726f8ff6d2da83dbc16d9b68b1ea83b7079b371eb16ef41c00441b6e85e32e33087f3b7753ea9e8b1e3f26d3e4df9208625e7f@54.148.111.168:30303",
    id: "a704bc011f205aecff8a13fcaa18b85bcf0b075de53822a22534337d800ff90c",
    name: "Geth/v5.5.1-mainnet/linux-amd64/go1.21.1",
    network: {
      inbound: false,
      localAddress: "...",
      remoteAddress: "54.148.111.168:30303",
      static: false,
      trusted: false
    },
    protocols: {
      eth: {
        difficulty: 10315165,
        head: "0xb52e8f45dff39d82348eb6d31ba67cbc82fcff4f4bbbf17c35a48a3301ba35e7",
        version: 66
      }
    }
}]

vdwijden and others added 8 commits August 6, 2024 10:47
* eth: implement eth/68

* eth/protocols/eth: added tx size to announcement

* eth/protocols/eth: check equal lengths on receiving announcement

* eth/protocols/eth: add +1 to tx size because of the type byte

* eth: happy lint, add eth68 tests, enable eth68

* eth: various nitpick fixes on eth/68

* eth/protocols/eth: fix announced tx size wrt type byte

Co-authored-by: MariusVanDerWijden <[email protected]>
Co-authored-by: Péter Szilágyi <[email protected]>
Conflicts:
	eth/handler_eth_test.go
	eth/protocols/eth/handler.go
	eth/protocols/eth/protocol.go
…ocol (ethereum#28261)

* eth: enforce announcement metadatas and drop peers violating the protocol

* eth/fetcher: relax eth/68 validation a bit for flakey clients

* tests/fuzzers/txfetcher: pull in suggestion from Marius

* eth/fetcher: add tests for peer dropping

* eth/fetcher: linter linter linter linter linter
 Conflicts:
	eth/fetcher/tx_fetcher.go
	eth/handler.go
	eth/handler_eth.go
 Conflicts:
	cmd/devp2p/internal/ethtest/chain_test.go
	cmd/devp2p/internal/ethtest/helpers.go
	cmd/devp2p/internal/ethtest/snapTypes.go
	cmd/devp2p/internal/ethtest/suite.go
	cmd/devp2p/internal/ethtest/types.go
	cmd/devp2p/rlpxcmd.go
Co-authored-by: Felix Lange <[email protected]>
 Conflicts:
	cmd/devp2p/internal/ethtest/helpers.go
omerfirmak
omerfirmak previously approved these changes Aug 6, 2024
@github-actions github-actions bot dismissed stale reviews from omerfirmak and 0xmountaintop via eec54c3 August 6, 2024 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants