/go-ethereum

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

rlp, p2p, p2p/discover: EIP-8 changes #2091

Merged
merged 12 commits into from Feb 19, 2016
+764 −182

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@fjl @robotally @codecov-io @obscuren @karalabe
@fjl
Contributor

fjl commented Dec 24, 2015

Implements ethereum/EIPs#49

@fjl fjl added the network label Dec 24, 2015

@fjl fjl added this to the Homestead milestone Dec 24, 2015

@robotally

This comment has been minimized.

Sign in to view

robotally commented Dec 24, 2015

Vote Count Reviewers
👍 2 @karalabe @obscuren
👎 0

Updated: Fri Feb 19 10:01:31 UTC 2016

@fjl fjl added the in progress label Dec 24, 2015

@fjl fjl force-pushed the fjl:eip-8 branch Dec 24, 2015

@codecov-io

This comment has been minimized.

Sign in to view

codecov-io commented Jan 4, 2016

Current coverage is 48.19%

Merging #2091 into develop will increase coverage by +0.31% as of 506e27e

Powered by Codecov. Updated on successful CI builds.

@fjl fjl force-pushed the fjl:eip-8 branch Jan 18, 2016

@fjl fjl force-pushed the fjl:eip-8 branch 2 times, most recently Feb 10, 2016

@fjl
crypto/ecies: make authenticated shared data work 
The s2 parameter was not actually written to the MAC.
fdb936e

@fjl fjl force-pushed the fjl:eip-8 branch Feb 12, 2016

@fjl fjl added please review and removed in progress labels Feb 12, 2016

@fjl

This comment has been minimized.

Sign in to view
Contributor

fjl commented Feb 12, 2016

@obscuren @karalabe @Gustav-Simonsson please review

@fjl fjl force-pushed the fjl:eip-8 branch to 2817d1d Feb 12, 2016

@obscuren obscuren modified the milestones: Homestead, 1.3.4 Feb 15, 2016

crypto/ecies/ecies.go
@@ -192,11 +192,9 @@ func concatKDF(hash hash.Hash, z, s1 []byte, kdLen int) (k []byte, err error) {
// messageTag computes the MAC of a message (called the tag) as per
// SEC 1, 3.5.
func messageTag(hash func() hash.Hash, km, msg, shared []byte) []byte {
if shared == nil {
shared = make([]byte, 0)
}

This comment has been minimized.

Sign in to view
Copy link
@karalabe

karalabe Feb 18, 2016

Member

Was this shared thing never used until now? Why is it only added to the hash now?

This comment has been minimized.

Sign in to view
Copy link
@fjl

fjl Feb 18, 2016

Contributor

We didn't use it until now.
rlp/decode.go
// decode as a nil pointer. This tag can be useful when decoding recursive
// types.
// decode as a nil pointer. This tag can be useful when decoding
// recursive types.

This comment has been minimized.

Sign in to view
Copy link
@karalabe

karalabe Feb 18, 2016

Member

Since you explain here how the nil tag works, please also add how the .. works.
rlp/decode_test.go Outdated
input: "C3010203",
ptr: new(dotdotRaw),
value: dotdotRaw{A: 1, DotDot: []RawValue{unhex("02"), unhex("03")}},
},

This comment has been minimized.

Sign in to view
Copy link
@karalabe

karalabe Feb 18, 2016

Member

Please add a test case where the tag is correctly set, but the input RLP contains something wrong. E.g. you want to parse into a list of strings and one of the elements it an int, not a string. Or something like this, to verify that mixing elements are caught and does not crash or similar.

This comment has been minimized.

Sign in to view
Copy link
@karalabe

karalabe Feb 18, 2016

Member

Also it would be nice to see examples tests where:

  • .. is used, but only one element is supplied
  • .. is used, but 0 elements are supplied

This comment has been minimized.

Sign in to view
Copy link
@fjl

fjl Feb 18, 2016

Contributor

The example already checks this, right?

This comment has been minimized.

Sign in to view
Copy link
@karalabe

karalabe Feb 18, 2016

Member

I'm not seeing either of the three cases checked?

Edit: The example fails when B is missing too. It doesn't check what happens when all the obligatory (A, B) fields are present, but C is missing or C contains only a single element or if C contains mixed elements. Those are the issues I'd like tested.
rlp/encode.go
etypeinfo, err := cachedTypeInfo1(typ.Elem(), tags{})
if err != nil {
return nil, err
}
writer := func(val reflect.Value, w *encbuf) error {
lh := w.list()

This comment has been minimized.

Sign in to view
Copy link
@karalabe

karalabe Feb 18, 2016

Member

Where to you start the list in the new code? I can only see the listEnd being used.
rlp/typecache.go Outdated
return tags{
nilOK: strings.Contains(tag, "nil"),
dotdot: strings.Contains(tag, ".."),
}

This comment has been minimized.

Sign in to view
Copy link
@karalabe

karalabe Feb 18, 2016

Member

I think it would catch potential errors if these would do strict checks and not contains. I.e. split the tag by commas, and check each element for nil, or ... equality and also error out if there's something else in there.

This comment has been minimized.

Sign in to view
Copy link
@obscuren

obscuren Feb 18, 2016

Member

What kind of potential errors?

Hmm thinking about this and I see what you mean. rlp:"nilling" rlp:"nonil" would also yield a positive.
rlp/encode_test.go Outdated
@@ -214,6 +214,7 @@ var encTests = []encTest{
{val: simplestruct{A: 3, B: "foo"}, output: "C50383666F6F"},
{val: &recstruct{5, nil}, output: "C205C0"},
{val: &recstruct{5, &recstruct{4, &recstruct{3, nil}}}, output: "C605C404C203C0"},
{val: &dotdotRaw{A: 1, DotDot: []RawValue{unhex("02"), unhex("03")}}, output: "C3010203"},

This comment has been minimized.

Sign in to view
Copy link
@karalabe

karalabe Feb 18, 2016

Member

Could you also add a case here when you only have one element and when zero elements? :D I would really like these corner cases covered by tests.
rlp/encode.go Outdated
etypeinfo, err := cachedTypeInfo1(typ.Elem(), tags{})
if err != nil {
return nil, err
}
writer := func(val reflect.Value, w *encbuf) error {
lh := w.list()
if !ts.dotdot {
defer w.listEnd(w.list())

This comment has been minimized.

Sign in to view
Copy link
@obscuren

obscuren Feb 18, 2016

Member

@karalabe here. w.list() is executed prior to the rest of the code.

EDIT: nifty pattern BTW :-D cool trick!
@obscuren

This comment has been minimized.

Sign in to view
Member

obscuren commented Feb 18, 2016

As discussed on gitter change the .. to tail to make it more descriptive what the tag does.
p2p/discover/udp.go Outdated
@@ -460,6 +468,29 @@ func isTemporaryError(err error) bool {
return ok && tempErr.Temporary() || isPacketTooBig(err)
}
func encodePacket1(priv *ecdsa.PrivateKey, ptype byte, req interface{}, additional []byte) ([]byte, error) {

This comment has been minimized.

Sign in to view
Copy link
@karalabe

karalabe Feb 18, 2016

Member

What does this method do? I don't see it used in any of the code.
p2p/rlpx.go Outdated
@@ -234,7 +256,9 @@ func (h *encHandshake) secrets(auth, authResp []byte) (secrets, error) {
return s, nil
}
func (h *encHandshake) ecdhShared(prv *ecdsa.PrivateKey) ([]byte, error) {
// ecdhShared returns the static shared secret, the result

This comment has been minimized.

Sign in to view
Copy link
@karalabe

karalabe Feb 18, 2016

Member

Method name in the doc is wrong.
p2p/rlpx.go Outdated
if err != nil {
return s, err
}
auth, err := h.authMsg(prv, token)
var authPacket []byte
if os.Getenv("RLPX_EIP8") != "" {

This comment has been minimized.

Sign in to view
Copy link
@karalabe

karalabe Feb 18, 2016

Member

Wasn't this only used for testing? Shouldn't EIP-8 be used always?

This comment has been minimized.

Sign in to view
Copy link
@karalabe

karalabe Feb 18, 2016

Member

Hmmm, or is this here so we push out the change to first only accept EIP-8, but don't initiate it and when people upgraded then switch over to use it by everyone?

This comment has been minimized.

Sign in to view
Copy link
@fjl

fjl Feb 19, 2016

Contributor

Yes, it's more like the latter comment. I used the env variable for testing with other clients.
Would be nice to ensure that the majority of the network will send EIP-8 soon-ish, maybe through
something like

if time.Now().After(switchoverDate) {
    // send EIP-8
}
@karalabe

This comment has been minimized.

Sign in to view
Member

karalabe commented Feb 18, 2016

Code looks solid to me. The crypto aspects I cannot verify, but given the thorough tests + the cross client interop experiments, I guess those parts are correct. Please address the issues I've raised and the it's a Go from my part.
@fjl

This comment has been minimized.

Sign in to view
Contributor

fjl commented Feb 19, 2016

PTAL. will squash when you're done reviewing.
@karalabe

This comment has been minimized.

Sign in to view
Member

karalabe commented Feb 19, 2016

LGTM 👍
@obscuren

This comment has been minimized.

Sign in to view
Member

obscuren commented Feb 19, 2016

👍

@obscuren obscuren added the merge imminent label Feb 19, 2016

@karalabe

This comment has been minimized.

Sign in to view
Member

karalabe commented Feb 19, 2016

@fjl Please squash.

@obscuren obscuren merged this pull request into ethereum:develop Feb 19, 2016
5 checks passed

5 checks passed

buildbot/ARM Go pull requests DEV build done.
Details
buildbot/Linux Go pull requests DEV build done.
Details
buildbot/OSX Go pull requests DEV build done.
Details
buildbot/Windows Go pull requests DEV build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@obscuren obscuren removed the please review label Feb 19, 2016

@obscuren

This comment has been minimized.

Sign in to view
Member

obscuren commented Feb 19, 2016

@fjl please re-open.

@fjl fjl referenced this pull request Feb 19, 2016

Merged

p2p, p2p/discover: EIP-8 changes #2226

@fjl fjl removed merge imminent network labels Feb 24, 2016

@fjl fjl removed this from the 1.3.4 milestone Feb 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment