network: Add adaptive capabilities message #1529

nolash · 2019-06-29T18:16:04Z

This PR is the implementation of part of https://github.com/ethersphere/SWIP/blob/master/lightnode_caps_msg.md (ethersphere/SWIPs#7)

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)

network/adaptive.go

network/protocol.go

nolash · 2019-07-03T06:14:04Z

I've reverted the changes I made to use the bitvector message. It turned a lot messier and difficult to understand, and its design is unintuitive for the case of capabilities:

In bitvector the Get(..) and set(..) use index which indexes the total bit number (2 bytes = 0-15). Commands treat order of bits within bytes as Right-To-Left, and the bytes slices as Left-To-Right.

Exampe: If I NewFromBytes([]byte{0x80, 0x08}) (10000000 00001000) then I'd expect set(1, true) (turn bit idx 1 on) to result no change, but instead I get 0x8108 (10000001 00001000). If I set(12, false) (turn bit idx 12 off) the result is 0x8000 (10000000 00000000).

Merely changing the way Get(..) and set(..) interprets the underlying vector (0x01<<(8-(idx%8)) instead of 0x01<<(idx%8)) messes up the tests in network/stream so it seems that package interprets byte slices passed to and/or gotten from it (NewFromBytes(..),Bytes(..)) in this "inside out" fashion.

Perhaps the additions I made to bitvector previously - the SetBytes, UnsetBytes and Stringshould be reverted, as they will not now be used.

nolash · 2019-07-03T06:38:18Z

I understand why it's made this way, of course; it's more efficient to avoid the extra subtraction in the index calculation. But for the sake of the capabilities this is not really a factor since the interface only operates on fully-defined bit vectors rather than individual bits.

holisticode · 2019-07-03T16:56:00Z

Maybe it's a good idea to add the capability swap_enabled, but if does not fit this PR, it's fine to add it later

nolash · 2019-07-03T17:48:33Z

@holisticode swap is a separate module right? You notice PSS also hasn't been included in this PR. Only the strictly bzz related stuff... I would imagine Swap would use API (upcoming PR) to add itself as a module to the capabilities vector.

holisticode · 2019-07-03T17:50:44Z

It is however an interesting question to ponder: when swap will be available, then how to deal with nodes who do not support swap? Do we simply drop them? Is there some way between?

acud

@nolash i have my traditional aesthetic code-style disagreements. otherwise i am a bit afraid of this whole notification concept - i think this is more dangerous than constructive at this point as our configuration infrastructure does not support this, thus developers should not get the possibility to mingle with run time configuration when the underlying infrastructure does not support it.
my 2 cents

acud · 2019-07-05T13:07:59Z

network/adaptive.go

+}
+
+// NewCapabilities creates a new Capabilities container
+//


remove this line, unnecessary

acud · 2019-07-05T13:16:34Z

network/adaptive.go

+// get a capability bitvector for the module
+// the first two bytes returned are id and length, the following bytes contain the flags
+// returns nil if id is not registered
+func (c Capabilities) get(id uint8) capability {


i'd like to see a dedicated type here, in a similar manner to what is done in sctx/sctx.go.
i.e. func (c Capabilities) get (id CapabilityModule) capability

Maybe sctx/sctx.go could use some comments itself? There's no description of what that package does...

acud · 2019-07-05T13:20:04Z

network/adaptive.go

+
+// Capabilities is a container that holds all capability bitvector flags for all registered modules
+type Capabilities struct {
+	Flags   []capability      // the bitvector flags


why are you exporting a field with an underlying unexported type?
if Capabilities is exported I'd assume that a particular Capability should be too, no?
if you intend to use this only within the framework of the network package than please unexport both of them

acud · 2019-07-05T13:22:09Z

network/adaptive.go

+// NewCapabilities creates a new Capabilities container
+//
+// It optionally takes a change notification channel as argument. If this is nil, notifications will not be issued.
+func NewCapabilities(changeC chan<- capability) *Capabilities {


please describe a situation when a node capability would change during runtime.
as much as i'd love to support such an approach it would be dangerous with our current design. a lot of our configuration needs the node to be started afresh to take place. thus, without rewriting our configuration infrastructure I find it unfeasible to allow developers to change capabilities during runtime.

please describe a situation when a node capability would change during runtime.

Changing during runtime is an explicit goal of the project with Status.

acud · 2019-07-05T13:25:23Z

network/adaptive.go

+}
+
+// adds a capability module to the bitvector collection
+// does not protect against duplicate ids. Calling code should use registerModule instead


then why do you need this function in the first place? i don't see any reason to have an extra function call just to abstract the builtin append

acud · 2019-07-05T13:52:58Z

network/adaptive.go

+	return m
+}
+
+func (m CapabilitiesMsg) get(idx int) capability {


do you really need another function in order to abstract a native functionality of go? there's no array size check here, so the extra function provides no added value

acud · 2019-07-05T13:53:29Z

network/adaptive.go

+
+func capArrayToString(b []capability) string {
+	var caps []string
+	for _, cap := range b {


what happens when len(b)==0?

acud · 2019-07-05T13:59:19Z

network/protocol.go

 )

+var (
+	capabilitiesFlagRetrieve      = []byte{0x00, 0x01} // node retrieves data for itself


as mentioned i'd be happy to see this in a more constructed manner with concrete types

acud · 2019-07-05T14:13:47Z

network/protocol.go

+	capabilitiesFlagStorer        = []byte{0x80, 0x00} // node is part of network storage (sync)
+
+	// temporary presets to emulate the legacy LightNode/full node regime
+	fullCapability  capability


you're using the term fullCapability but above you use capability to refer to a certain capability within a certain module but as far as i understand this might infer capabilities over several modules...?

The "full" in this context is only about the bzz capabilities. Swap, pss etc will have other distinct capabilities.

acud · 2019-07-05T14:15:53Z

network/protocol.go

+// temporary convenience functions for legacy "full node"
+func newFullCapability() capability {
+	c := newCapability(0, 2)
+	c.set(capabilitiesFlagRetrieve)


consider variadic function...?

zelig

This PR is a lot of code and complexity.
Agree with @acud that caps change may be premature to add now.
I would prefer binary caps implemented as bool fields on a simple struct.
that allows for get, set transparently without code.
You can then write a custom serialisation with a function implementing rlpEncoder interface. Then business logic is can remain readable and separated from bandwidth optimisation measures like bitvector representation.

nolash · 2019-07-28T20:51:10Z

thus developers should not get the possibility to mingle with run time configuration when the underlying infrastructure does not support it.

@acud I think this functionality is so exotic and obscure at this point that no developer would have endeavoured to try to hack with it.

However, since the code likewise turned out too exotic for everyone's taste, I'm rewriting the entire implementation. And (just) to make you happy I'll leave out the channel stuff in the rewrite ... for now :)

nolash · 2019-07-29T17:29:37Z

#obsoleted by #1619

nolash added enhancement in progress light node labels Jun 29, 2019

nolash self-assigned this Jun 29, 2019

nolash force-pushed the adaptive-capabilities-msg branch from 1d2e6f7 to 212c90c Compare July 2, 2019 00:01

nolash requested review from acud and nonsense July 2, 2019 08:01

nolash added ready for review and removed in progress labels Jul 2, 2019

nolash requested review from agazso and nagydani and removed request for nonsense July 2, 2019 08:01

nonsense reviewed Jul 2, 2019

View reviewed changes

network/adaptive.go Outdated Show resolved Hide resolved

agazso reviewed Jul 2, 2019

View reviewed changes

network/protocol.go Outdated Show resolved Hide resolved

nolash force-pushed the adaptive-capabilities-msg branch 2 times, most recently from 40b4581 to ac5d56c Compare July 3, 2019 06:03

nolash force-pushed the adaptive-capabilities-msg branch from c8fb856 to 057d742 Compare July 4, 2019 13:42

acud suggested changes Jul 5, 2019

View reviewed changes

zelig reviewed Jul 24, 2019

View reviewed changes

nolash added in progress and removed ready for review labels Jul 28, 2019

nolash added 3 commits July 29, 2019 17:25

network: Add Capability and Capabilities types

6529934

network: Tests pass (but cleanup needed)

6f76c21

network: Remove useless pointers

95931c1

nolash added 23 commits July 29, 2019 17:25

network: Move capabilities to BzzAddr

551169c

network: Remove redundant Capabilities member on BzzPeer

7af6307

network: Add API and test, move adaptive tests to sep file

511b1a1

network: Conceal capability type

5286a73

network: Change Capabilities to struct, intro change chan

b3420aa

network: Add event channel subscribe

273397c

network: Add cleanup on service stop

bba0cd3

network: Add test for notify in API test

776e895

network: Add rpc module Subscribe

27ed8b0

network: Reinstate internal channel notify test

1ea689d

network: Extract API to separate file

29ed01b

network: Remove API files (will be PRd separately)

2f1cf1b

network: Add comments to code

c26a387

pss: Remove stray alteration in pss

ff9ec84

network: Speling

6d9259a

network: Bump protocol version, as rebase updates crossed last one

4e674f7

network: Delint

a7cde4c

network: Make Capabilities in HandshakeMsg serializable

d9c79dc

network: CapabilitiesMsg add get() instead of fromMsg()

1ac7367

network: WIP Simplify capabilities objects

eb5efd0

network: WIP Rehabilitate tests after simplification

32db8eb

network: WIP revert to rlp encode and export cap/caps fields

fb9c2e9

network: Fixed full and light comparisons

967ba7c

nolash force-pushed the adaptive-capabilities-msg branch from 20cf3b0 to 967ba7c Compare July 29, 2019 15:25

nolash added 3 commits July 29, 2019 19:04

network: WIP custom RLP decoder for Capabilities collection

45f8efc

network: Capabilities RLP Decoder now re-populates id to array idx map

86140a5

network: Tidy up a bit

4b7972b

nolash mentioned this pull request Jul 29, 2019

network: Add adaptive capabilities message #1619

Merged

nolash closed this Jul 29, 2019

network: Add adaptive capabilities message #1529

network: Add adaptive capabilities message #1529

Uh oh!

Conversation

nolash commented Jun 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nolash commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nolash commented Jul 3, 2019

Uh oh!

holisticode commented Jul 3, 2019

Uh oh!

nolash commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

holisticode commented Jul 3, 2019

Uh oh!

acud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zelig left a comment

Choose a reason for hiding this comment

Uh oh!

nolash commented Jul 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nolash commented Jul 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nolash commented Jun 29, 2019 •

edited

Loading

nolash commented Jul 3, 2019 •

edited

Loading

nolash commented Jul 3, 2019 •

edited

Loading

nolash commented Jul 28, 2019 •

edited

Loading