-
Notifications
You must be signed in to change notification settings - Fork 112
cmd/swarm, p2p, swarm: Enable ENR in binary/execadapter #1302
Conversation
@nolash build is broken however I am unsure if this is related to the content of the PR |
@justelad I would say not, since the fails are using sim inproc, and the changes only concern sim exec and swarm binary. |
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.
Change request: handle error
of Config.Init()
in test cases. Others are optional.
That would be nice if you could increase the abstraction level of Config.Init()
by extracting low level functionality into higher level methods. That the Init()
would become easier to understand and shorter.
General style:
I strongly prefer of using the if err := f(); err != nil
format. It comes as a builtin language feature for a reason. By using the proposed format - where possible - the reader can immediately know the scope of the error - thus less code to understand - and the surrounding environment does not get polluted.
Sorry, I disagree. I can add a couple of comments if you want. But I don't think it will be clearer by splitting it up. |
@frncmx on second thoughts, I believe the |
@nolash I would rather not take up on a refactoring task just because I just wanted to prove one specific function could be cleaner. I came up with the following code:
|
@nolash I can post you the patch if you would like to apply my changes. Just looking at the code I moved around I also noticed: we use both
|
Just one more quick iteration, so that we really "pick one level of abstraction per method and stick with it":
The above code makes it easy to read it through in a few seconds and get a high level understanding of what is going on without bothered by details. |
Right, I didn't mean to push anything on you. I merely thought since you said you were writing it anyway, that it literally would have been replace code with that new function call you were making in two places. Don't worry. I can take care of merging this in. Thanks for the contribution. |
Yeah |
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.
Thank you! I like Config.Init()
much better now. 👍
|
||
prvkey, err := crypto.HexToECDSA(hexprvkey) | ||
if err != nil { | ||
t.Fatalf("failed to load private key: %v", err) | ||
} | ||
nodekey, err := crypto.HexToECDSA(hexnodekey) |
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 is always there whenever you call config.Init() could we not just put the nodkey generation into Init
?
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.
@zelig no because we want to generate it with node/Config.NodeKey()
when in cmd/swarm
Thanks for reviews @frncmx @zelig submitted upstream ethereum/go-ethereum#19309 |
Merged as ethereum/go-ethereum#19309 |
This PR contains some preliminary changes to allow for using ENR to transmit the swarm overlay address between peers.
Currently, ENR is not being sent over the p2p handshake. The necessary changes for this to happen is pending an EIP submission (and approval, and implementation). This is described in this PR by @fjl which incidentally also contains a hack by him that provides a functional demonstration on how ENR record will be available through
PeerInfo
and on thep2p.Peer
object:ethereum/go-ethereum#19220
In the meantime, the aim is to include the maximum amount of changes here that can be made towards the goal without relying on the implementation of the EIP. Some of the changes are cherry-picked from an experimental branch https://github.com/ethersphere/go-ethereum/tree/enr-bzz-poc building on the in the above mentioned PR.
In the experimental branch all handshake related code has been removed, and includes a temporary POC test
swarm/network/simulation/protocol_new_test.go:TestENR
that demonstrates a successfulWaitTillHealthy()
only using ENR.My proposed steps towards the basic but sound transition to handshake-free Swarm are:
BEFORE EIP
Preliminary changes in swarm/network and embed ENR in simulatioñ(done)AFTER EIP
swarm/network
swarm/network:BzzAddr
swarm/network:BzzPeer
andswarm/network:Peer
p2p/protocols
p2p/protocols
toswarm
package