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: ignore arbitrary data stored in multiaddrs enr key #2853

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

richard-ramos
Copy link
Member

Description

This would allow reading the multiaddrs key and not fail if one of the values encoded in that key is not a multiaddress.
The reason why this change is being proposed is that it would allow storing some data which can be translated to multiaddresses, for example circuit relay addresses which could be stored with this format:

- 4 bytes for the IP address
- 2 bytes for the Port
- 33 bytes for a secp256 compressed public key

which would mean storing 39 bytes compared to storing 50 bytes which would be the bytes encoding of a multiaddress containing the same information. (In ENRs the less bytes we use the better)

cc: @fryorcraken

Copy link

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2853

Built from afacddb

@fryorcraken
Copy link
Collaborator

I don't think we should store arbitrary data in mutliaddr, if we go for the new format, we should use a new key.

However, I agree with this change in the way that if not able to read a field, we shouldn't fully fail the enr decoding and just attempt to use the other fields to connect to a node.

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

This makes sense to me. We shouldn't fail on multiaddrs decode failure.

Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@richard-ramos richard-ramos merged commit 76d5b26 into master Jun 27, 2024
9 of 11 checks passed
@richard-ramos richard-ramos deleted the multiaddrs branch June 27, 2024 14:01
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.

4 participants