swarm: Added lightnode flag #816

agazso · 2018-07-26T14:20:38Z

Added --lightnode command line parameter
Added LightNode to Handshake message

This is the Task0 of the Light Node implementation.
For more details see https://hackmd.io/O5PvHQVhQnaNCbhJbAJVog?view

Added --lightnode command line parameter Added LightNode to Handshake message

nonsense · 2018-07-26T15:05:09Z

swarm/network/protocol.go

 		}
+
+		log.Info("peer created: %s", handshake.peerAddr.String())


I think this should be a log.Debug, otherwise logs will be full of it. Also use structured logs:

log.Debug("peer created", "addr", handshake.peerAddr.String())

Fixed logging

nonsense · 2018-07-26T15:08:12Z

swarm/network/protocol_test.go

 	)

 	if err != nil {
 		t.Fatal(err)
 	}
+
+	if pt.bzz.handshakes[id].LightNode != peerLightNode {
+		t.Fatal(fmt.Sprintf("peer LightNode flag is %v, should be %v", pt.bzz.handshakes[id].LightNode, peerLightNode))


Please use Fatalf, instead of Fatal + Sprintf

Changed to Fatalf

nonsense · 2018-07-26T15:08:44Z

swarm/network/protocol_test.go

@@ -30,6 +30,8 @@ import (
 	p2ptest "github.com/ethereum/go-ethereum/p2p/testing"
 )

+const DefaultVersion = 5


I think we need a better name for this, and a godoc - what is this DefaultVersion.

Changed it to TestProtocolVersion

nonsense · 2018-07-26T15:11:31Z

swarm/network/protocol_test.go

 	id := s.IDs[0]

 	err := s.testHandshake(
-		correctBzzHandshake(addr),
-		&HandshakeMsg{Version: 0, NetworkID: 3, Addr: NewAddrFromNodeID(id)},


I have mixed feelings about changing the hardcoded values to extracted variables. The idea is - if someone changes the DefaultNetworkID in the code, a test should fail, because this is changing the protocol and the contract. Peers would no longer be able to communicate.

You must know very well what you are doing when changing this, and I am afraid that hacking the actual value we tests against (in this case DefaultNetworkID and Version) has the opposite effect - tests will automatically pass, and you will notice that you changed the contract only once you deploy this.

I created a new const called TestProtocolNetworkID and used that in tests. I understand now that why using the DefaultNetworkID was a bad idea, however generally it's a good practice to not used hardcoded constants in the test code (or for that matter in any code), because it may lead to errors in unrelated tests when you change the constant in NetworkID tests.

What do you think?

I am happy with TestProtocolNetworkID

nonsense · 2018-07-26T15:13:25Z

cmd/swarm/config.go

@@ -303,6 +308,12 @@ func envVarsOverride(currentConfig *bzzapi.Config) (config *bzzapi.Config) {
 		}
 	}

+	if lightnodeenable := os.Getenv(SWARM_ENV_LIGHT_NODE_ENABLE); lightnodeenable != "" {


Nitpick, but this var has a scope of 2 lines, it could very well be just lne ;)

Changed the name to lne

nonsense · 2018-07-26T15:17:16Z

swarm/network/protocol_test.go

@@ -30,6 +30,8 @@ import (
 	p2ptest "github.com/ethereum/go-ethereum/p2p/testing"
 )

+const DefaultVersion = 5


This should be 6, no?

I changed it to TestProtocolNetworkID. As far as I understand it can be anything that is not 0 based on the existing tests. Feel free to correct me if I am wrong.

nonsense · 2018-07-26T15:19:32Z

swarm/network/protocol_test.go

+	}
+}
+
+func TestBzzHandshakeLightNodeOn(t *testing.T) {


What's the point of those 2 tests? I think you only need one of them.

The only reason I did two tests were to make sure it is set correctly independently of the default value. If we only check the On case (where LightNode is True), then it may happen that in the code it's set to True incorrectly buy tests wouldn't catch that.

I don't have a strong opinion about it but generally I think it doesn't hurt to have an extra test and it may cover errors as described above.

janos · 2018-07-30T05:31:52Z

cmd/swarm/main.go

@@ -123,6 +123,11 @@ var (
 		Usage:  "Duration for sync subscriptions update after no new peers are added (default 15s)",
 		EnvVar: SWARM_ENV_SYNC_UPDATE_DELAY,
 	}
+	SwarmLightNodeEnabled = cli.BoolTFlag{


I think that BoolFlag should be used instead BoolTFlag, if the default value is false.

Changed it to BoolFlag

GitCop · 2018-07-30T13:56:02Z

There were the following issues with your Pull Request

Commit: 62deb0ba09e15f3d67340119a4b010cb00fa6192
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/ethersphere/go-ethereum

This message was auto-generated by https://gitcop.com

…rkID

nonsense · 2018-07-31T09:28:02Z

swarm/network/protocol_test.go

@@ -30,6 +30,11 @@ import (
 	p2ptest "github.com/ethereum/go-ethereum/p2p/testing"
 )

+const (
+	TestProtocolVersion   = 5


The current version is 5. You are changing the handshake message, which means that you need to bump this to 6, no?

I bumped it to 6 to reflect the fact that the handshake is changed. However I think changing here has little significance, because it is only used in the scope of these tests and it is only used in one test for comparing with zero.

I think in order to change it in the protocol, one has to change the version in the BzzSpec that is inside swarm/network/protocol.go.

Also I remember in one of our hangout calls that it may not be necessary to bump this, @janos can you chime in?

Well, I was wrong about the significance of changing this in the tests, because if I change it there and not in BzzSpec the tests will fail.

Still the question is valid, should we change it in BzzSpec and therefore here too?

I ran some tests.

If we change the version, then nodes running version 6 won't connect to nodes that are running version 5 (as expected).

However if we just add the LightNode flag to the handshake without changing the version, the nodes are able to connect to each other. This is a bit surprising to me, but this means it's not strictly necessary to change the version at this point, because currently the code doesn't change any functionality.

In any case in the future it seems like a good idea to bump the version when add the actual LightNode functionality to Kademlia, because from that point nodes will definitely work differently, hence the need for the version change.

So I think the question is should we change the version two times (now and when the Kademlia changes are implemented) or just once (only after the Kademlia changes)?

I think that adding a field to a message is not a breaking change for protocol. It just requires that default value should be handled, and since default for light node flag is false, then it is handled implicitly.

OK, I thought nodes won't be able to deserialise the new value. If they can, then perfect!

@janos That's what my experiment suggests too, that just adding a LightNode field to the Handshake message works even when connecting to nodes which do not support the LightNode flag.

So basically you suggest that we keep the version right now at 5 and change it to 6 when we actually introduce breaking change in the functionality in Kademlia? @nonsense wdyt?

Ok, kept the version at 5 right now

Apparently I don't understand exactly the RLP serialization - will play with this locally, and I trust you that it works. Since this is the case, keep it at 5, no need to upgrade.

nonsense · 2018-07-31T09:34:24Z

swarm/network/protocol_test.go

@@ -225,16 +240,59 @@ func TestBzzHandshakeVersionMismatch(t *testing.T) {
 }

 func TestBzzHandshakeSuccess(t *testing.T) {


The following 3 tests are basically identical. I think this is introducing too much code with too little test value. Please create one test with a table-driven params, where you have both options - light node on/off. https://github.com/golang/go/wiki/TableDrivenTests

nonsense · 2018-07-31T10:22:54Z

swarm/network/protocol_test.go

-	if pt.bzz.handshakes[id].LightNode != peerLightNode {
-		t.Fatalf("peer LightNode flag is %v, should be %v", pt.bzz.handshakes[id].LightNode, peerLightNode)
-	}
+var lightNodeTests = []struct {


This var should be scoped within the test, no?

Moved the var inside the function

…scope

nonsense

lgtm

provided @janos approves, you can open on ethereum/go-ethereum and have @gbalint merge it.

agazso · 2018-07-31T14:39:50Z

Is it possible that the Go 1.9 test is flaky and that's why the Travis build failed? Have you seen similar before?

nonsense · 2018-07-31T14:44:27Z

@agazso yes, you are hitting a flaky test on the Go 1.9 build. Ignore or click rebuild. This one in particular is quite rare.

janos

LGTM

nonsense · 2018-08-07T13:59:44Z

@agazso We are getting a lot of errors in the open cluster, which includes this PR:


swarm_30399 DEBUG[08-07\|13:54:42.104\|p2p/server.go:703] Adding p2p peer name=swarm/v0.3.1-225171a... addr=137.117.226.225:57364 peers=4
--
swarm_30399 WARN [08-07\|13:54:42.104\|swarm/network/protocol.go:254] 27991d4d: handshake failed with remote peer fb24c2c5: Handshake error: Invalid message (RLP error): <= msg #0 (203 bytes): rlp: too few elements for network.HandshakeMsg
swarm_30399 DEBUG[08-07\|13:54:42.104\|p2p/server.go:721] Removing p2p peer id=daca9ff0c3176045 conn=inbound duration=444.603µs peers=3 req=false err="27991d4d: stream protocol closed: Handshake error: Invalid message (RLP error): <= msg #0 (203 bytes): rlp: too few elements for network.HandshakeMsg"

This is the backward-incompatible change I mentioned, and this is why I wanted to bump up the protocol version.

How did you test this to come to the conclusion that RLP can handle both versions of the HandshakeMsg?

nonsense · 2018-08-14T10:22:38Z

@gbalint @agazso note that this is backward-incompatible with previous versions. We should probably bump up the protocol versions.

agazso · 2018-08-14T13:41:40Z

@nonsense That is correct, it is indeed backward incompatible, my tests were incomplete before.

I agree that we should bump the protocol version. The question is how should we do it, because depending on the versioning schema swarm uses, the swarm version should be changed to reflect it is a backward incompatible change. For example with semver it's a minor version change.

nonsense · 2018-08-14T14:02:38Z

semver for version 0.x.y does not say we should be backward compatible - we can change APIs at any time. This doesn't mean we should, but we could.

agazso · 2018-08-14T14:41:57Z

@nonsense Sorry, I am a bit confused :)

To my understanding we changed the network API in a way that is backwards incompatible. People who are using Swarm may expect that change is reflected in a Swarm version number change. I don't know the policy of Swarm dealing about these cases.

It's easy just to bump the protocol version, I am asking about the specific steps to do that:

shall we create a new branch for it?
when will it be released? is it when you release a new minor version?

nonsense · 2018-08-14T15:09:43Z

We changed the API in a non-backward compatible way with this PR, and it has already been merged. Nodes no longer can connect to each other, and we are currently still using the same version for the protocol, which is bad.

We should at least increase the protocol version.

This will be released with 0.3.2 , which won't be compatible with 0.3.1, because of this change.

I hope that makes sense.

agazso · 2018-08-14T15:40:32Z

@nonsense I created a PR for the protocol version change: #888

swarm: Added lightnode flag

bb33951

Added --lightnode command line parameter Added LightNode to Handshake message

nonsense reviewed Jul 26, 2018

View reviewed changes

janos reviewed Jul 30, 2018

View reviewed changes

agazso added 4 commits July 30, 2018 15:40

swarm/config: Fixed variable naming

1fec621

cmd/swarm: Changed BoolTFlag to BoolFlag for SwarmLightNodeEnabled

d6a15df

swarm/network: Changed logging

1536e25

swarm/network: Changed protocol version testing

4835322

swarm/network: Renamed DefaultNetworkID variable to TestProtocolNetwo…

3a15c27

…rkID

nonsense reviewed Jul 31, 2018

View reviewed changes

agazso added 2 commits July 31, 2018 12:15

swarm/network: Bumped protocol version

975e96c

swarm/network: Changed LightNode handhsake test to table driven

5270b80

nonsense reviewed Jul 31, 2018

View reviewed changes

agazso added 2 commits July 31, 2018 16:05

swarm/network: Changed back TestProtocolVersion to 5 for now

c807e00

swarm/network: Moved the test configuration inside the test function …

022c7f4

…scope

nonsense approved these changes Jul 31, 2018

View reviewed changes

janos approved these changes Jul 31, 2018

View reviewed changes

agazso mentioned this pull request Jul 31, 2018

swarm: Added lightnode flag ethereum/go-ethereum#17291

Merged

nonsense closed this Aug 14, 2018

This was referenced Aug 14, 2018

Bumped swarm handshake protocol version to 6 #887

Closed

swarm/network: Bumped swarm handshake protocol version to 6 #888

Closed

nonsense mentioned this pull request Aug 20, 2018

swarm/network: bump bzz protocol version ethereum/go-ethereum#17449

Merged

		@@ -225,16 +240,59 @@ func TestBzzHandshakeVersionMismatch(t *testing.T) {
		}

		func TestBzzHandshakeSuccess(t *testing.T) {

swarm: Added lightnode flag #816

swarm: Added lightnode flag #816

Conversation

agazso commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitCop commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nonsense Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nonsense left a comment

Choose a reason for hiding this comment

agazso commented Jul 31, 2018

nonsense commented Jul 31, 2018 • edited Loading

janos left a comment

Choose a reason for hiding this comment

nonsense commented Aug 7, 2018

nonsense commented Aug 14, 2018

agazso commented Aug 14, 2018

nonsense commented Aug 14, 2018

agazso commented Aug 14, 2018

nonsense commented Aug 14, 2018

agazso commented Aug 14, 2018

nonsense Jul 31, 2018 •

edited

Loading

nonsense commented Jul 31, 2018 •

edited

Loading