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

chore: Remove our implementation of NodeId #1070

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

morph-dev
Copy link
Collaborator

@morph-dev morph-dev commented Dec 13, 2023

What was wrong?

We have our own implementation of NodeId, which is no longer needed (see #869)

Closes #869.

Copy link
Collaborator

@njgheorghita njgheorghita left a comment

Choose a reason for hiding this comment

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

🚢 Looks good to me! For some reason, I assumed that there would be a lot more changes as a result of this update, but it seems to me like you've covered everything here. One note, is to just update the pr description to "closes #869" so that the issue will be automatically closed once this pr is merged. Thanks!

@morph-dev
Copy link
Collaborator Author

I was thinking about that, but decided not to do it because bug also says:

Run it against portal-hive

Is that something that should be done as part of this pr before merging (and how?), or something to be done once it's merged (but before closing the bug)?

@njgheorghita
Copy link
Collaborator

@morph-dev Ahh, good catch. Have you run it against portal-hive locally? I'd recommend doing that before merging this pr. Of course, if we go ahead with the merge, we will soon find out if it passes portal-hive, but I think it's better to confirm that it passes portal-hive locally before merging this through

Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

:shipit: 🥳

Copy link
Member

@ogenev ogenev left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@morph-dev
Copy link
Collaborator Author

Which portal-hive tests should I run? I think I ran all of them, and these are the result:

Test Results
rpc-compat all pass
portal-mesh all pass
portal-interop 7 failed 1st run
4 failed 2nd run
ts-sim-test 1 failed
get-block-by-hash 7 failed
trin-bridge 9 failed

This is similar but slightly worse than results at https://portal-hive.ethdevops.io/.
I don't know if those extra failures are caused by me or not. Any advice?

@KolbyML
Copy link
Member

KolbyML commented Dec 14, 2023

Which clients did you run and which tests failed in portal-interop between what clients

@KolbyML
Copy link
Member

KolbyML commented Dec 14, 2023

If the test includes ultralight you can ignore it and we are good to merge if it comes up for tests which are trin/fluffy that is concerning

@morph-dev
Copy link
Collaborator Author

In second run I used this command:

./hive --sim portal-interop --client fluffy,ultralight,trin

These tests failed:
GOSSIP blocks from A:trin --> B:fluffy
GOSSIP blocks from A:trin --> B:ultralight
GOSSIP blocks from A:ultralight --> B:fluffy
GOSSIP blocks from A:ultralight --> B:trin

@KolbyML
Copy link
Member

KolbyML commented Dec 14, 2023

This is good to merge then

@KolbyML
Copy link
Member

KolbyML commented Dec 14, 2023

Wait actually nvm I will look into it more

@KolbyML
Copy link
Member

KolbyML commented Dec 14, 2023

What is the fail reason given for GOSSIP blocks from A:trin --> B:fluffy?

@morph-dev
Copy link
Collaborator Author

    "173": {
      "name": "GOSSIP blocks from A:trin --\u003e B:fluffy",
      "description": "",
      "start": "2023-12-14T16:51:56.389867412+02:00",
      "end": "2023-12-14T16:52:11.282737476+02:00",
      "summaryResult": {
        "pass": false,
        "details": "Client B: [\"Error content for block 17510000 (post-shanghai) block body was absent\", \"Error content for block 17510000 (post-shanghai) receipt was absent\"]"
      },
      "clientInfo": {
        "00343e15": {
          "id": "00343e15",
          "ip": "172.17.0.5",
          "name": "fluffy",
          "instantiatedAt": "2023-12-14T16:51:57.480046961+02:00",
          "logFile": "fluffy/client-00343e15735ccbba69fbeeb4dad3a78139e0d04e06b50603d7a61f54623448c2.log"
        },
        "d1ebef78": {
          "id": "d1ebef78",
          "ip": "172.17.0.4",
          "name": "trin",
          "instantiatedAt": "2023-12-14T16:51:56.87565928+02:00",
          "logFile": "trin/client-d1ebef78a56260c20092203a8e2af0f5857024b0e5064cc6a91f8eae656e15dc.log"
        }
      }
    },

@KolbyML
Copy link
Member

KolbyML commented Dec 14, 2023

Ok yeah this looks good to merge we are increasing the timeout in the PR I am waiting to merge on Portal-Hive.
This doesn't seem like an interop problem or different tests would have failed with this change.

@njgheorghita njgheorghita merged commit 28e0907 into ethereum:master Dec 14, 2023
2 checks passed
@morph-dev morph-dev deleted the nodeid_cleanup branch December 14, 2023 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate native NodeId implementation
4 participants