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

pss: Distill whisper elements in pss to a custom fallback crypto #1731

Merged
merged 13 commits into from
Sep 12, 2019

Conversation

kortatu
Copy link
Contributor

@kortatu kortatu commented Sep 6, 2019

Removed message.go file contained references to whisper message and envelope code. All that whisper code has been copied and distilled into swarm/pss/crypto as the default crypto implementation.
Crypto interface now is composed of 3 interfaces.

  • Message (contains message wrapping unwrapping and validation)
  • Keystore (contains Key manipulation, storage and conversion)
  • CryptoUtils (utilities for tests)

Sealing the message (poW and Nonce) has been completely removed.

We keep the whisper structure of encrypted message with the format:
flags+payloadSize+payload+padding+signature
but only inside the default crypto implementation. Now the payload of the whisper message is a simple []byte.

Padding is now an internal thing of the crypto implementation. Currently we have kept whisper padding, but maybe we can remove it. We are using fixed padding (as previous version) but left a method for implementing flexible padding.

Moved crypto.go to new package pss/crypto.

Moved cryptoUtils to crypto/utils.go

Benchmark tests have improved a bit mainly in larger keys:

This branch:

12:17 $ go test -cpu 4 -run=^$ -bench=^*$
goos: linux
goarch: amd64
pkg: github.com/ethersphere/swarm/pss
BenchmarkSymkeySend/256-4                           5000            270974 ns/op
BenchmarkSymkeySend/1024-4                          5000            281318 ns/op
BenchmarkSymkeySend/1048576-4                        100          12807973 ns/op
BenchmarkSymkeySend/10485760-4                        10         130643575 ns/op
BenchmarkSymkeySend/104857600-4                        2         637537803 ns/op
BenchmarkAsymkeySend/256-4                          2000           1016503 ns/op
BenchmarkAsymkeySend/1024-4                         2000            801877 ns/op
BenchmarkAsymkeySend/1048576-4                       100          19147333 ns/op
BenchmarkAsymkeySend/10485760-4                       10         208490108 ns/op
BenchmarkAsymkeySend/104857600-4                       1        1429648152 ns/op
PASS
ok      github.com/ethersphere/swarm/pss        18.953s

Master:

09:45 $ go test -cpu 4 -run=^$ -bench=^*$
goos: linux
goarch: amd64
pkg: github.com/ethersphere/swarm/pss
BenchmarkSymkeySend/256-4                           5000            280375 ns/op
BenchmarkSymkeySend/1024-4                          5000            289331 ns/op
BenchmarkSymkeySend/1048576-4                         50          23784979 ns/op
BenchmarkSymkeySend/10485760-4                         2         658122235 ns/op
BenchmarkSymkeySend/104857600-4                        1        4254846571 ns/op
BenchmarkAsymkeySend/256-4                          2000            818449 ns/op
BenchmarkAsymkeySend/1024-4                         2000            891359 ns/op
BenchmarkAsymkeySend/1048576-4                       100          31572557 ns/op
BenchmarkAsymkeySend/10485760-4                        2         521175499 ns/op
BenchmarkAsymkeySend/104857600-4                       1       33035929420 ns/op
PASS
ok      github.com/ethersphere/swarm/pss        54.628s

Implement subtask #1703

Distilled whisper message structure and encryption to default crypto implementation.
Added Topic to PssMsg
Simplified PssMsg Payload to []byte
@kortatu kortatu requested a review from nolash September 6, 2019 10:21
@jpeletier jpeletier changed the title Distill whisper elements in pss to a custom fallback crypto pss: Distill whisper elements in pss to a custom fallback crypto Sep 6, 2019
@jpeletier
Copy link
Contributor

After this is merged, please ping me so I rebase #1734, as it builds on this work.

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.

I am a bit skeptical whether this is not premature abstraction. I got the feeling that if it comes to actually use other crypto, it may not fit the interfaces designed here.

For sepration of concerns and code readability, I like the PR though, just suggest that the implementations are grouped by Asymmetric, Symmetric and Message.
and the interfaces i would define where they are used.

i also recommend naming which reads well.

crypto.Symmetric.Create() instead of crypto.cryptoBackend.GenerateSym()
crypto.Assymmetric.Get(id) instead of cryptoUtils.GetPrivate()
cache.Has(..) instead of checkFwdCache()
cache.Put(..) instead of addFwdCache()

pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/keystore.go Outdated Show resolved Hide resolved
@kortatu
Copy link
Contributor Author

kortatu commented Sep 9, 2019

I am a bit skeptical whether this is not premature abstraction. I got the feeling that if it comes to actually use other crypto, it may not fit the interfaces designed here.

About the "early abstraction", there is a second and third step in #1655 to define a crypto API ( Define a modular crypto API based on a selection of reference message crypto implementations). In factr @nolash talked about deciding the API in a SWIP.
This "distilling" only pretends to show which features should contain a crypto API. Saying that, I'm sure this first step could be improved, so I will try to implement all the comments and hints in this PR.

Copy link
Contributor

@nolash nolash left a comment

Choose a reason for hiding this comment

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

This is first half of review. I will continue later today.

pss/client/client_test.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Show resolved Hide resolved
pss/crypto/crypto.go Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
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.

great work! a few minor comments

pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/utils.go Outdated Show resolved Hide resolved
pss/crypto/utils.go Outdated Show resolved Hide resolved
@zelig zelig added this to Backlog in Swarm Core - Sprint planning via automation Sep 10, 2019
Copy link
Contributor

@nolash nolash left a comment

Choose a reason for hiding this comment

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

I will make another pass on this, not on the delta, but on the code itself as a whole now.

pss/crypto/utils.go Outdated Show resolved Hide resolved
pss/crypto/utils.go Outdated Show resolved Hide resolved
pss/crypto/utils.go Outdated Show resolved Hide resolved
pss/crypto/utils.go Outdated Show resolved Hide resolved
pss/keystore.go Outdated Show resolved Hide resolved
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.

agree, better, simpler

@nolash nolash moved this from Backlog to In progress in Swarm Core - Sprint planning Sep 12, 2019
@nolash nolash moved this from In progress to In review (includes Documentation) in Swarm Core - Sprint planning Sep 12, 2019
Copy link
Contributor

@nolash nolash left a comment

Choose a reason for hiding this comment

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

Most of this review was done before our call where we agreed rather merge sooner than later and define further issues for relevant comments that are not addressed. Can we please go through the comments and identify which should be future issues? The more trivial changes should be done now, IMO.

pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/crypto/crypto.go Show resolved Hide resolved
pss/crypto/crypto.go Outdated Show resolved Hide resolved
pss/keystore.go Show resolved Hide resolved
pss/crypto/crypto.go Show resolved Hide resolved
pss/crypto/crypto.go Show resolved Hide resolved
@jpeletier jpeletier added this to Ready for Review in Epic Labs Sprint planning Sep 12, 2019
@nolash nolash merged commit a5adcc3 into ethersphere:master Sep 12, 2019
Swarm Core - Sprint planning automation moved this from In review (includes Documentation) to Done Sep 12, 2019
Epic Labs Sprint planning automation moved this from Ready for Review to Done Sep 12, 2019
@skylenet skylenet added this to the 0.5.0 milestone Sep 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants