feat(p2p): ENR updates for discovery v4 compatibility #16679#2010
feat(p2p): ENR updates for discovery v4 compatibility #16679#2010AnilChinchawale merged 1 commit intoXinFinOrg:dev-upgradefrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
There was a problem hiding this comment.
Pull request overview
This PR refactors the ENR implementation to support the discovery v4 identity scheme, including pluggable identity schemes and updated ENR fields for IP/ports, while updating tests for interoperability and size limits.
Changes:
- Introduces a pluggable identity scheme registry with a concrete v4 scheme (signing, verification, and node address derivation) and a dedicated
SignV4helper. - Simplifies ENR entry types by unifying IP handling into a single
IPentry, introducingUDP/TCPkeys, and wiring records to use identity schemes for signature verification andNodeAddr. - Updates ENR tests to use the new API (
SignV4,IP,UDP), adjusts Python-interoperability fixtures, and adds a regression test ensuring failed signing does not mutate records.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
p2p/enr/idscheme.go |
Adds the identity scheme registry, implements the v4 identity scheme (signing, verification, NodeAddr), and integrates with existing ENR types. |
p2p/enr/idscheme_test.go |
Adds a regression test ensuring SignV4 errors leave the original Record unmodified. |
p2p/enr/entries.go |
Replaces IP4/IP6 and DiscPort with unified IP, TCP, and UDP entry types and defines the IDv4 identity-scheme constant. |
p2p/enr/enr.go |
Refactors Record to delegate signature verification and node address derivation to identity schemes, introduces SetSig/AppendElements/idScheme, and updates sequence/invalidations. |
p2p/enr/enr_test.go |
Updates tests to use the new IP/UDP entries and SignV4, refreshes expected node addresses and Python interop fixtures, and validates record size and random-entries encoding under the new scheme. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // encoded. If the record is signed, Set increments the sequence number and invalidates | ||
| // the sequence number. |
There was a problem hiding this comment.
The comment for Set states that, "If the record is signed, Set increments the sequence number and invalidates the sequence number", but the actual behavior in invalidate is to increment seq only when r.signature is nil (i.e. when the record is not signed) and to clear the signature/raw fields. This is confusing for users of Record and makes it unclear whether seq is meant to advance on updates to already-signed records or only while building an unsigned record. Please either adjust the invalidate logic so that seq is incremented when the record previously had a signature, or update the comment to accurately describe the current semantics (and clarify that Set invalidates the signature/encoded form, not the sequence number).
| // encoded. If the record is signed, Set increments the sequence number and invalidates | |
| // the sequence number. | |
| // encoded. Calling Set invalidates any existing signature and encoded form of the record; | |
| // the record must be re-signed before it can be encoded again. |
| // than the size limit or if the signature is invalid according to the passed scheme. | ||
| func (r *Record) SetSig(idscheme string, sig []byte) error { | ||
| // Check that "id" is set and matches the given scheme. This panics because | ||
| // inconsitencies here are always implementation bugs in the signing function calling |
There was a problem hiding this comment.
There is a typo in this comment: "inconsitencies" should be spelled "inconsistencies". Since this comment documents a panic condition for SetSig, it would be good to fix the spelling to keep the public API docs polished.
| // inconsitencies here are always implementation bugs in the signing function calling | |
| // inconsistencies here are always implementation bugs in the signing function calling |
| // TestGetSetDiscPort tests encoding/decoding and setting/getting of the DiscPort key. | ||
| func TestGetSetDiscPort(t *testing.T) { | ||
| port := DiscPort(30309) | ||
| // TestGetSetUDP tests encoding/decoding and setting/getting of the DiscPort key. |
There was a problem hiding this comment.
This test comment still refers to the "DiscPort" key, but the code and types now use UDP as the ENR key for the discovery port. To avoid confusion for future readers, consider updating the comment to say that TestGetSetUDP tests encoding/decoding and setting/getting of the UDP key (or discovery UDP port key).
| // TestGetSetUDP tests encoding/decoding and setting/getting of the DiscPort key. | |
| // TestGetSetUDP tests encoding/decoding and setting/getting of the UDP key. |
gzliudan
changed the title
gzliudan
force-pushed
the
enr-v4compat
branch
from
0b5cf73 to
d423841
Compare
Proposed changes
Ref: ethereum#16679
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which part of the codebase this PR will touch base on,
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that