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

EIP-7594 - Shorten the new introduced ENR record key (custody_subnet_count). #3744

Closed
nalepae opened this issue May 3, 2024 · 7 comments
Closed

Comments

@nalepae
Copy link
Contributor

nalepae commented May 3, 2024

Description of the issue:

EIP-7594 introduces a new ENR record: custody_subnet_count.

As highlighted by @wemeetagain in #3644, the maximum encoded size of a node record is 300 bytes. (Source).

For information here is what an ENR entries looks like with this new record:
image

The custody_subnet_count key by itself consumes 20 bytes, so almost 7% of the total available space in the ENR.

Proposal:
Remplace this key by a shorter one.

@nalepae nalepae changed the title EIP-7594 - Shorten the new introduced ENR record key (curstody_subnet_count). EIP-7594 - Shorten the new introduced ENR record key (custody_subnet_count). May 3, 2024
@ppopth
Copy link
Member

ppopth commented May 3, 2024

That makes sense. I would say csc (custody subnet count).

@jimmygchen
Copy link
Contributor

csc could be a bit cryptic for those not familiar with the context. I'm leaning towards custnets or custnetcnt as it aligns well with our existing keys attnets and syncnets; and it's also slightly easier to read. What do you think?

@dapplion
Copy link
Collaborator

dapplion commented May 6, 2024

Why not just custody? Do we anticipate any future property that may conflict in that namespace?

@nalepae
Copy link
Contributor Author

nalepae commented May 15, 2024

Related:

  1. The specification states: custody_subnet_count is a SSZ uint64 (Source).
    Unless we wish to have more than 255 subnets, we waste 7 bytes compared to uint8.
    We could use uint8 or uint16 to be more conservative.
  2. Why does this number need to be SSZ encoded? We could use standard big endian integer, as already done with, for example, tcp and udp. (Source)

@wemeetagain
Copy link
Contributor

Why does this number need to be SSZ encoded?

Good point. ENR kvs are already RLP encoded, so that natively handles encoding numbers.

For a positive integer, it is converted to the the shortest byte array whose big-endian interpretation is the integer, and then encoded as a string according to the rules below.

nalepae added a commit to nalepae/consensus-specs that referenced this issue May 17, 2024
@ppopth
Copy link
Member

ppopth commented May 24, 2024

csc could be a bit cryptic for those not familiar with the context

I think it's cryptic for now, but people can get familiar with it over time.

@hwwhww
Copy link
Contributor

hwwhww commented Jun 18, 2024

closing via #3772

@hwwhww hwwhww closed this as completed Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants