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

network: Add adaptive capabilities message #1619

Merged
merged 42 commits into from
Aug 16, 2019

Conversation

nolash
Copy link
Contributor

@nolash nolash commented Jul 29, 2019

This PR is the implementation of part of https://github.com/nolash/SWIPs/blob/SWIP-ADAPTIVE-NODE-PROTOCOL/SWIPs/swip-adaptive-node-protocol.md

  • introduce capabilities types
  • remove the lightnode field in handshake, replace with capabilities
  • test that capabilities are set as expected through the provided calls

(API will be added in separate PR)

#obsoletes #1529

nolash added 30 commits July 29, 2019 17:25
@nolash
Copy link
Contributor Author

nolash commented Jul 30, 2019

@zelig

If we want these finegrained caps, it is because they can be directly referenced in conditionals at the appropriate place they are relevant, i.e. have an interface where i can write if peer.Can(caps.Retrieve) or something, oder?

Yeah API is next PR.

if caps are a fixed list you enlist in protocol, then caps could just be sequential (iota) and be represented as a boolarray and serialised as bytes interpreted as your favourite bitvector :)

probably we want a "FromBools" or/and "FromBytes" in the API?

If caps are meant to be extensible i dont see how you take care of id clashes.

By convention was the idea.

Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the complexity and indirection here is too much for my taste.
do we have use cases of anything beyond a fix set of boolean flags?
I would prefer introducing simple flags first as and when they are needed with a functionality and only then see what if any abstraction needs to emerge, no?

network/adaptive.go Show resolved Hide resolved
@nolash
Copy link
Contributor Author

nolash commented Aug 5, 2019

do we have use cases of anything beyond a fix set of boolean flags?

As I said before in the last PR, the whole adaptive concept with Status suggests that capabilities will change. This provides a way to change them. Incremental small changes.

Also, the idea is that modules (pss, swap, ...) define and set their own capabilities. That would require an API.

So if we assume that it's not only "setting static bit flags," is it still too complex?

acud
acud previously approved these changes Aug 5, 2019
Copy link
Member

@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.

@nolash LGTM.
A few minor comments/questions in the body of the PR but this is a major improvement over the previous PR. No blockers from my side.
I have a separate question though about the Capabilities``&|``Capability types - there seems to be no methods to check whether a certain capability actually exists within the data structure.
Example: I am a full node and I want to know if the other node is not just a light node, but if it also possesses capabilities of stream protocol, or what not. I am not sure if this is intentional but I think we will need this pretty soon. For your consideration.
Peace

network/adaptive.go Outdated Show resolved Hide resolved
network/adaptive.go Outdated Show resolved Hide resolved
network/adaptive.go Outdated Show resolved Hide resolved
network/adaptive_test.go Outdated Show resolved Hide resolved
network/protocol.go Show resolved Hide resolved
@nolash
Copy link
Contributor Author

nolash commented Aug 5, 2019

there seems to be no methods to check whether a certain capability actually exists within the data structure.

Plan is to add API in which this is possible. But that's for the next PR.

Actually I was thinking probably we shouldn't even have sync as a flag in the bzz core, it should be controlled by the streaming package?

@janos
Copy link
Member

janos commented Aug 6, 2019

Imported but not used package https://travis-ci.org/ethersphere/swarm/jobs/567808411.

@nolash
Copy link
Contributor Author

nolash commented Aug 6, 2019

Imported but not used package https://travis-ci.org/ethersphere/swarm/jobs/567808411.

@janos fixed thanks

network/adaptive.go Outdated Show resolved Hide resolved
network/adaptive.go Outdated Show resolved Hide resolved
network/adaptive.go Outdated Show resolved Hide resolved
network/adaptive.go Outdated Show resolved Hide resolved
@nolash
Copy link
Contributor Author

nolash commented Aug 15, 2019

@janos thanks changed

Copy link
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.

Thanks @nolash. LGTM.

@zelig zelig merged commit c535b27 into ethersphere:master Aug 16, 2019
@skylenet skylenet added this to the 0.5.0 milestone Aug 16, 2019
@nolash nolash deleted the adaptive-capabilities-msg-nonotify branch August 16, 2019 09:34
chadsr added a commit to chadsr/swarm that referenced this pull request Sep 23, 2019
* 'master' of github.com:ethersphere/swarm:
  chunk, storage: chunk.Store multiple chunk Put (ethersphere#1681)
  storage: fix pyramid chunker and hasherstore possible deadlocks (ethersphere#1679)
  pss: Use distance to determine single guaranteed recipient (ethersphere#1672)
  storage: fix possible hasherstore deadlock on waitC channel (ethersphere#1674)
  network: Add adaptive capabilities message (ethersphere#1619)
  p2p/protocols, p2p/testing; conditional propagation of context (ethersphere#1648)
  api/http: remove unnecessary conversion (ethersphere#1673)
  storage: fix LazyChunkReader.join potential deadlock (ethersphere#1670)
  HTTP API support for pinning contents (ethersphere#1658)
  pot: Add Distance methods with tests (ethersphere#1621)
  README.md: Update Vendored Dependencies section (ethersphere#1667)
  network, p2p, vendor: move vendored p2p/testing under swarm (ethersphere#1647)
  build, vendor: use go modules for vendoring (ethersphere#1532)
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.

None yet

5 participants