-
Notifications
You must be signed in to change notification settings - Fork 110
bzzeth: initial support for bzz-eth protocol #1571
Conversation
@zelig let's seal phase 0, clean up and merge before moving on with more functionality? |
bafb94c
to
2045ee2
Compare
b8b6b9a
to
c0de2b7
Compare
@zelig I have rebased this branch with master and with p2p-propagate-context. Removed phase 1 related stuff. Please check if this is okay. |
eaceb09
to
9e6b2f0
Compare
a6e4f4c
to
e04406b
Compare
6b2cb2b
to
a8d7780
Compare
bp := NewPeer(peer) | ||
|
||
// perform handshake and register if peer serves headers | ||
handshake, err := bp.Handshake(context.TODO(), Handshake{ServeHeaders: true}, nil) |
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.
Isn't this an example of potential Capabilities
usage?
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 should we add this as Capability?
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.
later if consensus
bp.serveHeaders = handshake.(*Handshake).ServeHeaders | ||
log.Warn("handshake", "hs", handshake, "peer", bp) | ||
// with another swarm node the protocol goes into idle | ||
if isSwarmNodeFunc(bp) { |
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 seems like a very strange choice to me. Services in nodes should be kept as independent units. As I see it, this is an additional step away from exactly the modularization I wanted to achieve with #1676
For example, what if we were here talking to a different node implementation that magically muxes eth
and bzz
like a boss?
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.
ok i know but this is how we can do it now oder?
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.
Let's expedite the split-up? :)
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.
we'll address this later
} | ||
} | ||
|
||
// peers represents the bzzeth specific peer pool |
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.
In my opinion this would similarly be a case of Capabilities filtering of existing kademlia (PR is coming up on that after the API one https://github.com/nolash/swarm/tree/adaptive-kademlia )
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.
great. anything I need to do now
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.
Let's expedite the capabilities PRs ;)
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.
@nolash let's please defer this requirement to the phase 1 PR. this is ready to be merged
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.
Looks good for phase 0. I share the same concerns for isSwarmNodeFunc as @nolash does. But for now, I see no alternative.
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.
Approved from my side (I can't approve since I am the author of the PR)
I believe capabilities fits for this kind of purpose. But I leave it up to you to decide what to do. |
* 'master' of github.com:ethersphere/swarm: pss: Modularize crypto and remove Whisper. Step 1 - isolate whisper code (ethersphere#1698) pss: Improve pressure backstop queue handling - no mutex (ethersphere#1695) cmd/swarm-snapshot: if 2 nodes to create snapshot use connectChain (ethersphere#1709) network: Add API for Capabilities (ethersphere#1675) pss: fixed flaky test that was using a global variable instead of a local one (ethersphere#1702) pss: Port tests to `network/simulation` (ethersphere#1682) storage: fix hasherstore seen check to happen when error is nil (ethersphere#1700) vendor: upgrade go-ethereum to 1.9.2 (ethersphere#1689) bzzeth: initial support for bzz-eth protocol (ethersphere#1571) network/stream: terminate runUpdateSyncing on peer quit (ethersphere#1696) all: first working SWAP version (ethersphere#1554) version: update to v0.5.0 unstable (ethersphere#1694) chunk, storage: storage with multi chunk Set method (ethersphere#1684) chunk, storage: add HasMulti to chunk.Store (ethersphere#1686) chunk, shed, storage: chunk.Store GetMulti method (ethersphere#1691) api, chunk: progress bar support (ethersphere#1649)
This PR implements phase 0 of the header chain on swarm epic ethersphere/user-stories#9
the code is tested with local trinity client and swarm client having successful handshake.
dependency: #1648