-
Notifications
You must be signed in to change notification settings - Fork 28
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
Ping/Pong Extensions #706
Ping/Pong Extensions #706
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far! Couple of relatively small things in comments.
Other feedback already noted on discord.
There was a problem hiding this 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 overall though I question whether we need a new dependency to grab the commit hash. I think we can safely assume that anybody running this client locally will either be running it from a cloned git repo or in docker and I'm pretty sure git is installed in our docker image so we should able to just do an exec('git rev-parse HEAD')
or something to that effect and read the output instead of requiring a new dependency for this. I know I'm being persnickety but just my 2 gwei. Happy to be persuaded otherwise.
b18567d
to
3980994
Compare
3980994
to
065084d
Compare
commit 58bd756 implements changes from ethereum/portal-network-specs#358 unit tests will fail until test vectors are updated |
Passing tests with test vectors from: https://github.com/ethereum/portal-network-specs/pull/357/files |
} | ||
const pong = await network2.handlePing(network1NodeAddress, 1234n, pingMsg) | ||
const pongMsg = PortalWireMessageType.deserialize(pong) | ||
// const pongPayload = HistoryRadius.deserialize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this commented out? Shouldn't we validate the content of the error payload too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Kolby Moroz Liebl <[email protected]>
Current the client info is returning the wrong value
needs to be updated to
My tests pass with this change |
Implements changes in ethereum/portal-network-specs#348
Redefines the
CustomPayload
of PING and PONG messages to include atype
parameter.This allows different types of PING messages to provide / request different kinds of data from a peer.
Currently this includes:
Clients will respond with a PONG of the same type, or: