-
Notifications
You must be signed in to change notification settings - Fork 80
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
p2p/enr: parse ENRs with enode #1011
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1011 +/- ##
==========================================
- Coverage 54.16% 53.56% -0.61%
==========================================
Files 118 119 +1
Lines 13215 13282 +67
==========================================
- Hits 7158 7114 -44
- Misses 5030 5143 +113
+ Partials 1027 1025 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
p2p/enr_test.go
Outdated
require.Error(t, err) | ||
require.Contains(t, err.Error(), "input contains more than one value") | ||
} | ||
|
||
func TestBackwardsENR(t *testing.T) { | ||
oldVersionENR := "enr:-HW4QHwvU8usykhaKQ_cxyAaAck_TlS0zr9nLNXDODQll87ifLECDsnczJz7vC_aVTKq9Q8aC3UIKWwls1i4TmEXLAuAgmlkgnY0iXNlY3AyNTZrMaEDhry2ryLc-Y39laN92yfixHghkWIjLeQ4orBvDQy20jA=" |
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.
better do 100 random old ENRs (copy old EncodeENR to test file)
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.
also see encoding/base64/base64_test.go:62
p2p/enr.go
Outdated
if strings.HasSuffix(enrStr, "=") { | ||
enrStr = enrStr[:len(enrStr)-1] |
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.
this isn't correct
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.
got it
Just adding "do not merge" until we fully understand why things were not breaking sooner |
cmd/createcluster.go
Outdated
p2pConfig := p2p.Config{ | ||
TCPAddrs: []string{"127.0.0.1:3610"}, | ||
UDPAddr: "127.0.0.1:3630", | ||
} |
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.
don't understand why this is required
if !ok { | ||
return enr.Record{}, errors.New("invalid ENR with no prefix (enr:)") | ||
} | ||
// Ensure backwards compatibility with older versions with encoded ENR strings. |
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.
add comment what that older actually did
} | ||
} | ||
|
||
func encodeOldVersion(t *testing.T, r enr.Record) string { |
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.
add comment what this is
p2p/enr_test.go
Outdated
|
||
func TestBackwardsENR(t *testing.T) { | ||
random := rand.New(rand.NewSource(time.Now().Unix())) | ||
for i := 0; i < 1000; i++ { |
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.
100 is probably fine
testutil/compose/define.go
Outdated
config := p2p.Config{ | ||
TCPAddrs: []string{"127.0.0.1:3610"}, | ||
UDPAddr: "127.0.0.1:3630", | ||
} |
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.
doesn't seem right
Corrects ENR encode/decode base64 format with the help of enode.
category: bug
ticket: #970