swarm/pss: Allow transmission of raw messages #376

nolash · 2018-04-06T07:30:43Z

This PR enables sending and receiving of raw messages, bypassing the builtin pss encryption mechanisms.

The intended usage is for applications that prefer to handle encryption themselves.

It also makes it possible to use the pss node as a multiplexing node, in essence handling several clients with different public keys, something that is not currently possible in a pss node since the public key is unique for this node.

It also introduces a protected topic value 0x0 which, if set, denotes an unencrypted message, and is passed to the raw message handler before decryption is attempted.

The PR also introduces outbox capacity handling code, which errs on failed attempt of message enqueueing.

zelig · 2018-04-06T12:50:14Z

swarm/pss/pss.go

+}
+
+func (self *Pss) executeHandlers(topic Topic, payload []byte, from *PssAddress, asymmetric bool, keyid string) {
+	handlers := self.getHandlers(topic)


why is this peer created can you remind me?

we need the peer passed to the PssProtocol handler.

zelig · 2018-04-06T17:04:12Z

swarm/pss/pss.go

@@ -365,17 +371,25 @@ func (self *Pss) process(pssmsg *PssMsg) bool {
 			self.outbox <- pssmsg
 		}()
 	}
-	handlers := self.getHandlers(psstopic)
+	if psstopic == rawTopic {


so lets talk about this. I dont understand why for no encryption there would be a specific topic rather than a bool field on the message or nothing (just the first assumption)

nothing sounds better. Let's try that. nothing is not possible, because whisper topics are 4 byte fixed arrays. If we detach from whisper (to allow for more bytes in topic) we can do this.

Bool on message, well, yeah;

That's one more byte pr msg.

If we later want to use empty topic instead and would want to revert, that will change the pss message structure itself, which means version change in protocol and incompatible nodes, I guess.

Disallowing certain hashes is not so elegant either, I know.

I don't have a strong opinion here, but I would like to avoid changing the message structure in the future.

nonsense · 2018-04-10T08:43:01Z

swarm/pss/pss_test.go

@@ -1172,13 +1254,13 @@ func benchmarkSymkeyBruteforceSameaddr(b *testing.B) {
 }

 // setup simulated network and connect nodes in circle
-func setupNetwork(numnodes int) (clients []*rpc.Client, err error) {
+func setupNetwork(numnodes int, allowRaw bool) (clients []*rpc.Client, err error) {


please document setupNetwork explaining about allowRaw... something along the lines of

setupNetwork is creating a simulated network with `numnodes` number of nodes. `allowRaw` is ...

nonsense · 2018-04-10T08:44:38Z

swarm/pss/pss.go

@@ -71,6 +72,7 @@ type PssParams struct {
 	CacheTTL            time.Duration
 	privateKey          *ecdsa.PrivateKey
 	SymKeyCacheCapacity int
+	AllowRaw            bool


We should start to document the params in structs like this one. Please add a comment next to AllowRaw explaining its purpose and usage.

nonsense · 2018-04-10T12:29:57Z

swarm/pss/pss.go

@@ -365,17 +371,25 @@ func (self *Pss) process(pssmsg *PssMsg) bool {
 			self.outbox <- pssmsg
 		}()
 	}
-	handlers := self.getHandlers(psstopic)
+	if psstopic == rawTopic {


I am not sure I understand this - how is it possible to end up in this if-condition? If we are using rawTopic for psstopic wouldn't we always end up in the err != nil block above?

This if-condition looks redundant to me.

If the message is encrypted but has topic 0x0 it will hit this check.

Then I don't understand the raw concept... from the description of this PR - It also introduces a protected topic value 0x0 which, if set, denotes an unencrypted message, and is passed through to the raw message handler in case of failed decryption.

@nolash maybe we do an interactive review, on a call, so that we don't have another 100 comments on this PR - I just want to understand the background a bit better :)

You're right. I'll move the topic check up. We don't need to waste cycles trying to decrypt the message if it has topic 0x0

nonsense · 2018-04-10T12:33:14Z

swarm/pss/pss.go

@@ -365,17 +371,25 @@ func (self *Pss) process(pssmsg *PssMsg) bool {
 			self.outbox <- pssmsg


I think it is quite error-prone to create a go-routine for adding messages to the outbox channel. We should have some other mechanism to signal to the user if the outbox size is reached, and we should do something about it, not create unlimited number of go-routines that are blocked in memory.

I would just remove this goroutine altogether as a start.

I originally was using arrays and counters which was quite a bit slower according to benchmarks, but which had error on capacity issues. @zelig suggested use channels instead. See #354

So you suggest to introduce a counter?

using channels for the outbox is good, but when you have a buffered channel you should address what happens when the limit of the buffer is reached, and address this pressure. AFAIK you cannot have an unlimited buffer, as this basically means unlimited memory and this doesn't exist.

what you are doing now (goroutine which blocks once you reach the size of the outbox), means that you essentially have an unlimited number of goroutines that might block and you have a memory problem. we should not have that, but rather return an error and block if the outbox size is reached.

here is a summary of the questions we should answer when we have a buffered channel - https://www.ardanlabs.com/blog/2017/10/the-behavior-of-channels.html

nonsense · 2018-04-10T23:47:55Z

@nolash you don't have to maintain a counter to keep track whether the buffered channel is full or not. you can do the same with a select statement and a default - see https://stackoverflow.com/questions/25657207/golang-how-to-know-a-buffered-channel-is-full

zelig · 2018-04-11T01:06:29Z

swarm/pss/pss.go

-		}()
+		if !self.enqueue(pssmsg) {
+			log.Error("outbox full!")
+			return false


why not return error?

zelig · 2018-04-11T01:07:43Z

swarm/pss/pss.go

+		return true
+	default:
+	}
+	return true


return false
or better errOutboxFull

zelig · 2018-04-11T01:08:53Z

swarm/pss/pss.go

@@ -745,11 +798,14 @@ func (self *Pss) forward(msg *PssMsg) {
 	if sent == 0 {
 		log.Debug("unable to forward to any peers")
 		time.Sleep(time.Millisecond)
-		self.outbox <- msg
+		if !self.enqueue(msg) {
+			return errors.New("outbox full!")


3rd way of calling enqueue

zelig · 2018-04-11T01:13:19Z

still dont understand rawTopic

nolash · 2018-04-11T06:44:26Z

@zelig did you read my comment on your comment on rawTopic before?

nonsense

lgtm

nolash added enhancement ready for review pss labels Apr 6, 2018

nolash self-assigned this Apr 6, 2018

nolash requested review from gbalint, zelig and nonsense and removed request for gbalint April 6, 2018 07:39

nolash force-pushed the pss-raw branch from e01d5d8 to 6e93648 Compare April 6, 2018 07:41

zelig reviewed Apr 6, 2018

View reviewed changes

nonsense reviewed Apr 10, 2018

View reviewed changes

nolash force-pushed the pss-raw branch from eddf5cc to a3eda34 Compare April 10, 2018 19:45

nolash added ready for another review and removed ready for review labels Apr 10, 2018

zelig reviewed Apr 11, 2018

View reviewed changes

nolash added 9 commits April 11, 2018 09:00

swarm/pss: WIP outbox

580d116

swarm/pss: WIP deduplication

509a22e

swarm/pss: Remove pss DPA + leaner cache

48d957d

swarm/pss: Enable raw message sending

c6ab940

swarm/pss: Rebase after deduplication changes

e501442

swarm/pss: Add documentation for AllowRaw param

221e55b

swarm/pss: Add counter and error on full outbox

538c311

swarm/pss: Move rawtopic test before decryption attempt

3e10f30

swarm/pss: Add error to outbox

ecde730

nolash force-pushed the pss-raw branch from a3eda34 to ecde730 Compare April 11, 2018 07:15

nonsense approved these changes Apr 11, 2018

View reviewed changes

gbalint approved these changes Apr 11, 2018

View reviewed changes

nolash merged commit 5b7c6da into swarm-network-rewrite Apr 11, 2018

nolash deleted the pss-raw branch April 11, 2018 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swarm/pss: Allow transmission of raw messages #376

swarm/pss: Allow transmission of raw messages #376

nolash commented Apr 6, 2018 •

edited

Loading

zelig Apr 6, 2018

nolash Apr 6, 2018

zelig Apr 6, 2018

nolash Apr 6, 2018 •

edited

Loading

nonsense Apr 10, 2018

nonsense Apr 10, 2018

nonsense Apr 10, 2018

nolash Apr 10, 2018

nonsense Apr 10, 2018 •

edited

Loading

nolash Apr 10, 2018

nonsense Apr 10, 2018

nolash Apr 10, 2018 •

edited

Loading

nonsense Apr 10, 2018 •

edited

Loading

nonsense commented Apr 10, 2018

zelig Apr 11, 2018

zelig Apr 11, 2018

zelig Apr 11, 2018

nolash Apr 11, 2018

zelig commented Apr 11, 2018

nolash commented Apr 11, 2018

nonsense left a comment

		@@ -365,17 +371,25 @@ func (self Pss) process(pssmsg PssMsg) bool {
		self.outbox <- pssmsg

swarm/pss: Allow transmission of raw messages #376

swarm/pss: Allow transmission of raw messages #376

Conversation

nolash commented Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolash Apr 6, 2018 • edited Loading

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 Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolash Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

nonsense Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

nonsense commented Apr 10, 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

zelig commented Apr 11, 2018

nolash commented Apr 11, 2018

nonsense left a comment

Choose a reason for hiding this comment

nolash commented Apr 6, 2018 •

edited

Loading

nolash Apr 6, 2018 •

edited

Loading

nonsense Apr 10, 2018 •

edited

Loading

nolash Apr 10, 2018 •

edited

Loading

nonsense Apr 10, 2018 •

edited

Loading