Skip to content
This repository was archived by the owner on Aug 2, 2021. It is now read-only.

bzzeth: initial support for bzz-eth protocol#1571

Merged
zelig merged 4 commits intomasterfrom
bzz-eth
Aug 29, 2019
Merged

bzzeth: initial support for bzz-eth protocol#1571
zelig merged 4 commits intomasterfrom
bzz-eth

Conversation

@acud
Copy link
Copy Markdown
Contributor

@acud acud commented Jul 15, 2019

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

@acud acud marked this pull request as ready for review July 15, 2019 15:35
@acud
Copy link
Copy Markdown
Contributor Author

acud commented Jul 19, 2019

@zelig let's seal phase 0, clean up and merge before moving on with more functionality?

@zelig zelig changed the base branch from master to p2p-propagate-context August 3, 2019 09:39
@zelig
Copy link
Copy Markdown
Member

zelig commented Aug 3, 2019

@acud , @jmozah this is where i am at before going on holidays.

@zelig let's seal phase 0, clean up and merge before moving on with more functionality?

let's merge #1647 and #1648 first

then i am happy to extract phase 0 for review after 12th or you guys could do it

@zelig zelig force-pushed the p2p-propagate-context branch from bafb94c to 2045ee2 Compare August 3, 2019 09:50
@zelig zelig force-pushed the p2p-propagate-context branch 2 times, most recently from b8b6b9a to c0de2b7 Compare August 14, 2019 14:08
@jmozah
Copy link
Copy Markdown
Collaborator

jmozah commented Aug 21, 2019

@zelig I have rebased this branch with master and with p2p-propagate-context. Removed phase 1 related stuff. Please check if this is okay.

@jmozah jmozah requested a review from zelig August 21, 2019 09:29
@jmozah jmozah self-assigned this Aug 21, 2019
@zelig zelig changed the base branch from p2p-propagate-context to master August 22, 2019 08:36
@zelig zelig force-pushed the bzz-eth branch 2 times, most recently from eaceb09 to 9e6b2f0 Compare August 22, 2019 08:52
@jmozah jmozah force-pushed the bzz-eth branch 2 times, most recently from a6e4f4c to e04406b Compare August 22, 2019 09:40
@zelig zelig force-pushed the bzz-eth branch 2 times, most recently from 6b2cb2b to a8d7780 Compare August 22, 2019 10:23
@zelig zelig requested review from janos and nolash and removed request for zelig August 24, 2019 19:40
@zelig zelig added this to the 0.5.0 milestone Aug 24, 2019
bp := NewPeer(peer)

// perform handshake and register if peer serves headers
handshake, err := bp.Handshake(context.TODO(), Handshake{ServeHeaders: true}, nil)
Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Collaborator

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?

Copy link
Copy Markdown
Member

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) {
Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Contributor

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? :)

Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Contributor

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 )

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor

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 ;)

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

@janos janos left a 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.

Copy link
Copy Markdown
Contributor Author

@acud acud left a 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)

@nolash
Copy link
Copy Markdown
Contributor

nolash commented Aug 28, 2019

I believe capabilities fits for this kind of purpose. But I leave it up to you to decide what to do.

@zelig zelig merged commit fe77fee into master Aug 29, 2019
chadsr added a commit to chadsr/swarm that referenced this pull request Sep 23, 2019
* '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)
@acud acud deleted the bzz-eth branch September 25, 2019 10:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants