-
Notifications
You must be signed in to change notification settings - Fork 726
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
Update deterministic subnets upgrade to allow prefix computation #4959
Update deterministic subnets upgrade to allow prefix computation #4959
Conversation
Tagging @michaelsproul since you have good eye for these things |
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.
These changes look fine to me.
The conversion to u64's I think are fine, but if someone wants to double check it can't hurt us that might be one area to look at.
There is edge cases for panics, but only on wild configuration settings, so i'm cool with that.
Yes, I am assuming that the number of bits we take for subnet prefix + shuffling will never reach 64, since this would require us to store a mapping for 1.8×10¹⁹ (2^64) node id prefixes -> subnet, per epoch of interest (probably at least the current and next one). Note that the previous code has similar assumptions, since I don't think these assumptions are outrageous, since they are controlled by the spec and anyone changing them to crazy values can't cause harm to other nodes but their own. But ofc happy to take suggestions from @michaelsproul when he gets the time |
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.
Merged unstable to fix some conflicts with Dencun variables.
Logic looks good and matches ethereum/consensus-specs#3545
Should add a check at the start compute_subnets_for_epoch
to check that prefix_bits + shuffling_prefix_bits < 64
?
I think we get this one in. @ackintosh has another that is dependent on this one also. |
@AgeManning gentle reminder to use mergify next time 🙏
|
…p#4959) * add new spec field to spec * add spec changes * fix bug and update expected subnets with validation from pyspec * fix values and make test prettier (?) * fix style --------- Co-authored-by: Lion - dapplion <35266934+dapplion@users.noreply.github.com>
Issue Addressed
Implements ethereum/consensus-specs#3545 in preparation for turning on deterministic subnet peers discovery
Proposed Changes
Add the spec values necessary to implement the change so that peer discovery can be tackled afterwards
Additional Info
found the 🐛 and now all the test values are verified with pyspec so I'm certain calculations are correct
The
compute_subscribed_subnet
function required two changes to work:subnet_prefix
andshuffling_bits
uint64
).For the record this is the code: