p2p: fix DiscReason encoding/decoding #30855
Conversation
|
I noticed that the test However when closing a connection the client simply uses If my understanding is correct, I will fix that test too. |
p2p/peer_test.go
Outdated
| "github.com/ethereum/go-ethereum/rlp" | ||
| "github.com/stretchr/testify/require" |
There was a problem hiding this comment.
these should be added to the import-paragraph further down, not up here
There was a problem hiding this comment.
These changes are not present anymore
|
Could you please provide some background why you are reverting #24507 |
|
Ahh I see why. So the server sends the reason as a byte array... In the spec it is defined to be an RLP list. The fix needs to be applied on the writing side, i.e. in p2p/transport.go. The decoding part is actually correct. Though we could handle both cases as well. |
fjl
changed the title
@fjl ah I see! I will implement the fix in the writing side today then! |
|
@fjl should be good now, if you like the approach I will squash all commits |
lorenzo-dev1
changed the title
And update some unit tests, since they were still using the wrong format.
| @@ -86,6 +88,7 @@ var discReasonToString = [...]string{ | |||
| DiscSelf: "connected to self", | |||
| DiscReadTimeout: "read timeout", | |||
| DiscSubprotocolError: "subprotocol error", | |||
| DiscInvalid: "invalid disconnect reason", | |||
There was a problem hiding this comment.
@fjl I think this bumps the length of the discReasonToString to 256... So it means that the String() func will never return the uknown reason error. it will return empty string instaed
fjl
changed the title
|
About |
|
Seems like I cannot merge the PR even if approved, should I wait for other approvals? |
|
Only team can merge. I was waiting for tests to finish. |
Ok I see, sorry it is my first PR on go-ethereum so I am not familiar with the process. Could you also point me to where the CI is run? |
|
It runs on AppVeyor. I will merge your PR later, still want to verify something about it. |
This PR reverts back the change to the
DiscReasonrlp decoding done in #24507It also adds a test to check for correctness. The current code in
masterwill fail the test since the rlp decoding of DiscReason will fail. However the error returned fromrlp.Decodeis not checked, so the client proceeds using the default value for DiscReason, which isDiscRequested.This PR also prevents a possible nil pointer dereference when closing an rlpx connection.