swarm/pss: Message handler refactor #965

nolash · 2018-10-18T08:04:02Z

This is the first in a series of PR providing the following enhancements to PSS:

Send fully addressed messages to neighbourhoods

The possibility of sending a message with full PssAddress to a neighbourhood of nodes, by interactive with the Kademlia. The primary use case is chunk syncing, where the chunk will have full address but nodes that have responsibility for storing the chunk should still regard themselves as recipients of it.

Mitigate external influence of resource expenditure

If receiving raw messages, currently all raw messages will be passed on to handling code, effectively passing the responsibility of limiting resource expenditure to the handlers. This can be risky, in case of implementations that do not protect against request flooding. Whether to handle raw messages or not should therefore be explicitly set on the handler.

This also pertains to symmetric decryptions, were all registered symmetric keys (for decryption) share the same LRU cache which is tried sequentially on the decrypt. Instead, there should be symkey decrypt caches per topic.

Symkey filter in message type

To reduce resource expenditure in symmetric decryption, an extra field in the message type will be introduced where up to 32 bytes of symmetric key hint can be added. The hint is a hash of the symmetric key, indexed on both sides. The symkey cache must be refactored to be able to take advantage of this hint. A zero-value in this field would indicate public key crypto.

This also clears the way for upgrading whisper support to v6, as one of the changes in whisper v6 is omission of the symkey nonce in the whisper envelope; with this field it's still possible to identify incoming symmetrically encrypted messages, to avoid a forced attempt of asymmetric decryption on every reception.

Part 1

protocols, handshakes, pss/client and notify have all been edited to pass tests with the new Handler struct, but need to be revisited for implementation details. This will follow in a later PR

- Change Handler from func to struct with necessary params
- Set raw message allowance per handler
- Add possible recipient logic to use kademlia depth
- Handler capabilities tests
- Prox destinations tests
- Make sender handle message if topic has prox handler and is possible recipient

The PR also replaces the -v[v] verbosity flags for tests with --loglevel flag that sets the loglevel explicitly, and default level to 2 (log.Warn).

zelig · 2018-10-19T15:54:41Z

https://travis-ci.org/ethersphere/go-ethereum/jobs/443683817#L439
pss/notify tests fail @nolash

swarm/network/kademlia.go

zelig · 2018-10-19T17:17:02Z

swarm/pss/api.go

+		},
+	}
+	if raw {
+		hndlr.caps |= handlerCapRaw


swarm/pss/pss.go

swarm/pss/types.go

swarm/pss/api.go

zelig · 2018-10-20T04:29:13Z

swarm/pss/pss.go

+	// - no prox handler on message and partial address matches
+	// - prox handler on message and we are in prox regardless of partial address match
+	// store this result so we don't calculate again on every handler
+	var isProx bool


Now I think you execute a raw handler for a message if there is another handler that allows prox even if that handler does not allow prox.

I think this logic should be different now, instead of isSelf calls we should just have a call to func getHandlersFor(topic, type) *handler when the type enum is

raw if the message is raw

sym if the message has a symkey hint

asym if there is no hint.

the handlers would be in a map indexed by topic+type serialised.
this would return a list of handlers where the handler would have an address matcher
to be called with address and sym key hint.

func (*handler) match(address, symhint) (payload []byte, handlers []*handlerFunc, forward bool)
this would

lookup decryption key from hint and open for sym

it would match on address

or partial address if allowed

or proximity if allowed

or it would decrypt with asym if there was no hint

and return the decrypted payload and the handlers that matched and a bool to indicate if the message needs to be forwarded (only false if full destination address matches)

Now I think you execute a raw handler for a message if there is another handler that allows prox even if that handler does not allow prox.

It's not clear to mean what you mean here, but the condition in pss.go:380 stops handling of raw message if no raw handlers are registered for the topic.

sure but it still tries it even though we could know better by then....

swarm/pss/pss.go

swarm/pss/types.go

zelig · 2018-10-21T07:42:42Z

@nolash https://travis-ci.org/ethersphere/go-ethereum/jobs/443796697#L508

zelig

unused var https://travis-ci.org/ethersphere/go-ethereum/jobs/452808218#L503

swarm/network/kademlia.go

swarm/pss/protocol_test.go

swarm/pss/pss.go

swarm/pss/pss_test.go

zelig · 2018-11-10T06:21:06Z

swarm/pss/pss_test.go

+	// remote address distances from localAddr to try and the expected outcomes if we use prox handler
+	remoteDistances := []int{
+		255,
+		kad.MinBinSize + 1,


this is very confusing and most likely wrong. How is kad.MinBinSize relate to proximity order (you call distance)?

This works only becuse you set peerCount := kad.MinBinSize + 2 , kad.MinProxBinSize happens to be 2 and you fill the table with peers in PO bins staring from 0.
This is way too arcane for a test.

I wanted to make sure that I have one peer in each bin, two more peers than NNs. The NNs in this test will be in two separate bins.

Are you arguing that the test will break if kad.MinBinSize would change? I'm not sure how?

I do agree however that the naming in this test makes it confusing. How about I copy the kad.MinBinSize to a local var, and only use local var names thereafter?

swarm/pss/types.go

zelig · 2018-11-10T10:43:18Z

Also, the algo for forwarding needs fixing as per our convo (see my code)
https://github.com/ethersphere/go-ethereum/blob/11f152819632d3c7a09de5f468e2f506280dd75d/swarm/pss/pss.go#L875

nolash · 2018-11-20T10:55:16Z

@zelig I think this was wrong before. Can you evaluate too please?

https://github.com/ethersphere/go-ethereum/pull/965/files#diff-2d1fd9a8750299d8ae63031e6b30be89R347

zelig

minor things, otherwise LGTM

zelig · 2018-11-21T05:47:28Z

swarm/network/kademlia.go

@@ -430,6 +433,12 @@ func (k *Kademlia) eachAddr(base []byte, o int, f func(*BzzAddr, int, bool) bool
 // the nearest neighbour set with cardinality >= MinProxBinSize
 // if there is altogether less than MinProxBinSize peers it returns 0
 // caller must hold the lock


remove this last comment please

zelig · 2018-11-21T05:48:45Z

swarm/pss/protocol_test.go

@@ -129,7 +129,9 @@ func testProtocol(t *testing.T) {
 	case <-lmsgC:
 		log.Debug("lnode ok")
 	case cerr := <-lctx.Done():
-		t.Fatalf("test message timed out: %v", cerr)
+		_ = cerr


reinstate t.Fatal line

zelig · 2018-11-21T05:51:49Z

swarm/pss/pss.go

+		return false
+	}
+
+	minProx := p.Kademlia.NeighbourhoodDepth()


I would call this depth

zelig · 2018-11-21T05:52:27Z

swarm/pss/pss.go

+	}
+
+	minProx := p.Kademlia.NeighbourhoodDepth()
+	depth, _ := p.Kademlia.Pof(p.Kademlia.BaseAddr(), msg.To, 0)


I would call it po or msgpo

zelig · 2018-11-21T05:53:23Z

swarm/pss/pss.go

+	if err != nil {
+		return err
+	}
+	if p.isSelfPossibleRecipient(pssMsg, true) && p.topicHandlerCaps[topic].prox {


worth a comment that this is loopback

zelig · 2018-11-23T00:43:21Z

swarm/pss/pss.go


-	if minProx <= depth {
+	if po <= depth {


but now the other way round :)

lol sorry. thanks for catching it

zelig · 2018-11-23T00:46:40Z

https://travis-ci.org/ethersphere/go-ethereum/jobs/457838997#L506
and tests fail

nolash · 2018-11-23T10:03:01Z

Submitted upstream ethereum/go-ethereum#18169

nolash requested review from gbalint, holisticode, janos, acud, nonsense and zelig as code owners October 18, 2018 08:04

nolash self-assigned this Oct 18, 2018

nolash added enhancement in progress pss labels Oct 18, 2018

nolash removed request for nonsense, janos, holisticode, gbalint, zelig and acud October 18, 2018 08:04

nolash force-pushed the pss-handler-refactor-depth branch from c0eff2c to 588ab17 Compare October 19, 2018 06:03

zelig mentioned this pull request Oct 19, 2018

[WIP] neighbourhood addressing for unencrypted pss #961

Closed

nolash requested review from adamschmideg and zelig and removed request for adamschmideg October 19, 2018 12:52

nolash added ready for review and removed in progress labels Oct 19, 2018

zelig reviewed Oct 20, 2018

View reviewed changes

zelig force-pushed the pss-handler-refactor-depth branch from f42eca7 to 9edcb1c Compare October 24, 2018 18:11

nolash force-pushed the pss-handler-refactor-depth branch from 10def86 to 11f1528 Compare November 9, 2018 10:30

zelig suggested changes Nov 10, 2018

View reviewed changes

nolash mentioned this pull request Nov 20, 2018

swarm/pss: Remove spurious forwarding #1008

Closed

2 tasks

nolash added 14 commits November 20, 2018 13:17

swarm/pss, swarm/network: WIP Rawmsg per handler + handler to struct

325d57f

swarm/pss: Add topic handler capability cache, process conditions

14dd35b

swarm/pss: Fix comments

2d5fc96

swarm/pss: Add prox recipient logic + tests for prox and raw

b6fea80

swarm/pss: Add higher level handler test on prox handler test

3814138

swarm/pss: Fix comments, minor cleanup

be084fc

swarm/pss: Fix test in notify

bc2a4b8

swarm/pss: WIP amend PR comments by @zelig

92bafbf

swarm/pss: Correct params for subscribe, correct hash for digest

5a2eee5

swarm/pss: Add deliver to self on prox topics

6f93440

swarm/pss: Simplify caps check, test cleanup

122d351

swarm/pss: Move topic handler cap add to unconditional add

88eb8c5

swarm/pss: Remove unused constant

3e6d23e

swarm/pss: Exorcise evil bitwise logic and revert kad consts

b13b05c

nolash force-pushed the pss-handler-refactor-depth branch from a46ac96 to b13b05c Compare November 20, 2018 12:18

zelig suggested changes Nov 21, 2018

View reviewed changes

swarm/pss: Minor cleanup

8b7918b

nolash force-pushed the pss-handler-refactor-depth branch from 45e6069 to 8b7918b Compare November 21, 2018 08:51

zelig suggested changes Nov 23, 2018

View reviewed changes

swarm/pss: Complete depth/po comparison + proper loglevel ctrl

91b45cc

zelig approved these changes Nov 23, 2018

View reviewed changes

zelig added ready to submit to ethereum/go-ethereum and removed ready for review labels Nov 23, 2018

nolash closed this Nov 23, 2018

zelig mentioned this pull request Nov 29, 2018

push syncing - meta + timeline #1028

Closed

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swarm/pss: Message handler refactor #965

swarm/pss: Message handler refactor #965

nolash commented Oct 18, 2018 •

edited

Loading

zelig commented Oct 19, 2018

zelig Oct 19, 2018

nolash Oct 20, 2018

zelig Oct 20, 2018

nolash Oct 20, 2018 •

edited

Loading

zelig Oct 21, 2018

zelig commented Oct 21, 2018

zelig left a comment

zelig Nov 10, 2018

nolash Nov 10, 2018 •

edited

Loading

zelig commented Nov 10, 2018

nolash commented Nov 20, 2018

zelig left a comment

zelig Nov 21, 2018

zelig Nov 21, 2018

zelig Nov 21, 2018

zelig Nov 21, 2018

zelig Nov 21, 2018

zelig Nov 23, 2018

nolash Nov 23, 2018 •

edited

Loading

zelig commented Nov 23, 2018

nolash commented Nov 23, 2018

swarm/pss: Message handler refactor #965

swarm/pss: Message handler refactor #965

Conversation

nolash commented Oct 18, 2018 • edited Loading

Send fully addressed messages to neighbourhoods

Mitigate external influence of resource expenditure

Symkey filter in message type

Part 1

zelig commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolash Oct 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zelig commented Oct 21, 2018

zelig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolash Nov 10, 2018 • edited Loading

Choose a reason for hiding this comment

zelig commented Nov 10, 2018

nolash commented Nov 20, 2018

zelig left a 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

nolash Nov 23, 2018 • edited Loading

Choose a reason for hiding this comment

zelig commented Nov 23, 2018

nolash commented Nov 23, 2018

nolash commented Oct 18, 2018 •

edited

Loading

nolash Oct 20, 2018 •

edited

Loading

nolash Nov 10, 2018 •

edited

Loading

nolash Nov 23, 2018 •

edited

Loading