NET: Yahya/5098-repackage-network-layer #183

thep2p · 2020-11-26T01:25:05Z

Note: this PR basically repackages the network layer. It does not add any new features or changes in functionalities.

Prior to this PR, the entire network package of flow was under /network/gossip/libp2p/, which would expose the following downsides:
1- There were two unnecessary subdirectories of /gossip/libp2p.
2- Libp2p was fully integrated and interlaced with the network package of flow and encapsulation of it was fading out.
3- There were several circular dependencies underneath /gossip/libp2p.
Additionally, the package structure was not fully compliant with golang best practice as well as the rest of the project.

Per conversation with @vishalchangrani we decided to repackage the network layer to pave the way for further libp2p encapsulation and dependency injection. To do this, in this PR we address the followings:
1- /network/gossip/libp2p now turned into /network/ with following packages:

codec: encapsulates encoding/decoding functionalities.
internal: a golang-reserved package name for shared tools we want exportable within network package and unexportable externally.
message: we keep it as a reserved package for our protobuf messages.
mock: encapsulates auto-generated mocks.
p2p: implementations of network-layer protocols (e.g., middleware)
queue: encapsulates message queue implementations
stub: encapsulates a network stub implementations for unit tests.
test: encapsulates end-to-end integration tests of network layer.
topology: encapsulates topology protocols implementation.
validator: encapsulates message validator implementations.

All interfaces have been moved to network/ package and their implementations moved to their corresponding packages. In case implementation details were noticeable, we dedicate a separate package for the interface, e.g., topology.

…/master"

…er' into yahya/5098-repackage-network-layer

jordanschalm

Nice work 👏

jordanschalm · 2020-11-26T20:51:18Z

Makefile

-	GO111MODULE=on mockery -name '.*' -dir=network/gossip/libp2p/middleware -case=underscore -output="./network/gossip/libp2p/mock" -outpkg="mock"
-	GO111MODULE=on mockery -name 'Connector' -dir=network/gossip/libp2p -case=underscore -output="./network/gossip/libp2p/mock" -outpkg="mock"
-	GO111MODULE=on mockery -name 'SubscriptionManager' -dir=network/gossip/libp2p/channel -case=underscore -output="./network/gossip/libp2p/mock" -outpkg="mock"
+	GO111MODULE=on mockery -name '.*' -dir=network/p2p -case=underscore -output="./network/mock" -outpkg="mock"


Out of scope here, but it might be a good idea to start outputting these mocks to packages named eg. mocknetwork. When they're all called mock we end up manually overwriting the package name for most tests, and not always consistentlyl.

Addresses in 1009da8

jordanschalm · 2020-11-26T20:52:11Z

network/internal/errors.go

@@ -1,4 +1,4 @@
-package errors
+package internal


Goland is telling me this error isn't used anywhere, maybe we can just remove? InvalidEngineError

Fixed in e69cb2f. Removed InvalidEngineError and made other error inline in code as it was only utilized on a test file.

jordanschalm · 2020-11-26T20:52:38Z

network/internal/keyTranslator.go

@@ -1,4 +1,4 @@
-package libp2p
+package internal


jordanschalm · 2020-11-26T20:53:45Z

network/p2p/libp2pNode.go

@@ -28,7 +28,7 @@ import (
 	"github.com/rs/zerolog"

 	"github.com/onflow/flow-go/module"
-	netwk "github.com/onflow/flow-go/network"
+	network2 "github.com/onflow/flow-go/network"


Maybe libp2pnet and flownet for these rather than network and network2?

applied in 1305edd

jordanschalm · 2020-11-26T20:54:07Z

network/p2p/libp2pNode.go

-func (p *P2PNode) Start(ctx context.Context,
-	n NodeAddress,
+func (n *Node) Start(ctx context.Context,
+	nodeAddress NodeAddress,


vishalchangrani · 2020-11-27T00:02:17Z

network/test/testUtil.go

@@ -233,3 +234,40 @@ func stopNetworks(t *testing.T, nets []*libp2p.Network, duration time.Duration)
 	unittest.RequireCloseBefore(t, lifecycle.AllDone(comps...), duration,
 		"could not stop the networks")
 }
+
+// networkPayloadFixture creates a blob of random bytes with the given size (in bytes) and returns it.


…e-network-layer

vishalchangrani · 2020-11-27T01:29:38Z

network/p2p/keyTranslator.go

@@ -1,4 +1,4 @@
-package libp2p
+package p2p


The diff contains: - a lot of changes that concern parts of the library we don't use (other curves a la BN, BLS24-X, BLS12-383 ...), integer protocols (ETRS), field extension machinery ... - otherwise irrelevant changes, e.g. CI/CD - some memory bug fixing [Full Changeset](https://github.com/relic-toolkit/relic/compare/7a9bba7f..9206ae5) **Fixed bugs:** - Unexpected failure of ep2\_mul\[\_lwnaf\] above the prime group order [\onflow#64](relic-toolkit/relic#64) **Closed issues:** - Other way to construct towered extension fields [\onflow#203](relic-toolkit/relic#203) - blake2.h:101:5: error: size of array element is not a multiple of its alignment [\onflow#202](relic-toolkit/relic#202) - ECIES 160bit [\onflow#201](relic-toolkit/relic#201) - Compilation with "ARITH gmp" fails [\onflow#200](relic-toolkit/relic#200) - Support for armv8-a ? [\onflow#198](relic-toolkit/relic#198) - Function name bn\_init conflicts with OpenSSL when used in tandem [\onflow#196](relic-toolkit/relic#196) - 16-bit MSP430 [\onflow#193](relic-toolkit/relic#193) - Modular exponentiation returns 1 if exponent is 0 and modulo is 1 [\onflow#185](relic-toolkit/relic#185) - Compilation of RELIC with bls12-446 and bls12-455 fails [\onflow#182](relic-toolkit/relic#182) - test\_bn fails with BLS12-381 preset [\onflow#181](relic-toolkit/relic#181) - \[BUG\] undefined reference to `bench_init', `bench\_clean' [\onflow#180](relic-toolkit/relic#180) - Tests FTBFS because of missing symbol in header [\onflow#179](relic-toolkit/relic#179) - Builds are broken [\onflow#178](relic-toolkit/relic#178) - compile error inlining failed in call to always\_inline ‘\_mm\_alignr\_epi8’ on unbantu20.04 gcc9 [\onflow#177](relic-toolkit/relic#177) - bn\_write\_str buffer overflow [\onflow#176](relic-toolkit/relic#176) - ECDSA verify succeeds when it should fail [\onflow#175](relic-toolkit/relic#175) - ec\_mul\_gen hangs with curve SECG\_K256 [\onflow#174](relic-toolkit/relic#174) - Wrong square root computation [\onflow#173](relic-toolkit/relic#173) - Out-of-bounds read via bn\_sqr\_basic [\onflow#172](relic-toolkit/relic#172) - OSS-Fuzz integration [\onflow#171](relic-toolkit/relic#171) - Building Relic with Curve NIST\_P256 throws FATAL ERROR in relic\_fp\_prime.c:120 [\onflow#170](relic-toolkit/relic#170) - Compressing \(packing\) a point to binary array does not comply with X9.62 standard [\onflow#169](relic-toolkit/relic#169) - ‘ctx\_t’ {aka ‘struct \_ctx\_t’} has no member named ‘total’ [\onflow#168](relic-toolkit/relic#168) - relic does not work with C++ [\onflow#167](relic-toolkit/relic#167) - Memory leak in ep2\_curve\_init/clean with ALLOC=DYNAMIC [\onflow#166](relic-toolkit/relic#166) - \*\_is\_valid\(\) functions produce false negative for not normalized points [\onflow#147](relic-toolkit/relic#147) - Bench and Test doesnt build [\onflow#122](relic-toolkit/relic#122) **Merged pull requests:** - Add pairing delegation protocols [\onflow#199](relic-toolkit/relic#199) ([dfaranha](https://github.com/dfaranha)) - Fix support for Win64/MSVC targets. [\onflow#197](relic-toolkit/relic#197) ([dfaranha](https://github.com/dfaranha)) - Simplify generator getting for Gt. [\onflow#194](relic-toolkit/relic#194) ([luozejiaqun](https://github.com/luozejiaqun)) - cmake: Always use user defined CFLAGS, not only for release builds [\onflow#187](relic-toolkit/relic#187) ([xdustinface](https://github.com/xdustinface)) - Fix MinGW build [\onflow#186](relic-toolkit/relic#186) ([xdustinface](https://github.com/xdustinface)) - Remove debug printf in bn\_mxp\_slide [\onflow#184](relic-toolkit/relic#184) ([guidovranken](https://github.com/guidovranken)) - Remove ALLOC = STACK to simplify memory allocation. [\onflow#183](relic-toolkit/relic#183) ([dfaranha](https://github.com/dfaranha)) - Update relic\_alloc.h [\onflow#165](relic-toolkit/relic#165) ([aguycalled](https://github.com/aguycalled)) - Add correct support for FreeBSD and NetBSD [\onflow#164](relic-toolkit/relic#164) ([hoffmang9](https://github.com/hoffmang9))

thep2p added 30 commits November 24, 2020 16:45

refactors /gossip/libp2p

14d1aba

fixes makefile for mock

8d86881

updates go module

eed9290

moves conduit to protocol package

0a3c2e6

moves middleware implementation to protocol package

e666393

renames P2PNode to Node

4f7f8b0

renames receivers of P2PNode

78349c9

moves read connection and subscription to internals

ee87367

adds Host method for node

d69064e

fixes middleware errors

398e1d8

encapsulates test into test package

79e94f3

moves interfaces to the root package

fa77318

fixes topology package

d46792d

packages and fixes internals

0c8d127

packages and fixes internals

20b54bc

fixes mocks

2f0611e

fixes cmd

e3f4b50

fixes makefile for mocks

1b013c4

makes libp2p connector unexportable

43c3f89

fixes test in libp2p

f89fe64

fixes errors in network tests

4afc548

renames protocol into p2p

b881694

refactors makefile

9a9120a

refactors protocol to p2p in files

d53f39c

Auto stash before merge of "yahya/4864-optimize-topology" and "origin…

0d5ff6b

…/master"

fixes go module

cd31a69

refactors message validator

127ca57

renames subscription manager to subscription

44b032d

fixes compilation problem in validator

4beaf5b

moves topology to network

c0e4fd4

thep2p requested a review from psiemens as a code owner November 26, 2020 01:25

thep2p requested review from vishalchangrani and jordanschalm November 26, 2020 01:27

thep2p and others added 3 commits November 26, 2020 09:49

Merge branch 'master' into yahya/5098-repackage-network-layer

d6f27d4

Merge remote-tracking branch 'origin/yahya/5098-repackage-network-lay…

827e460

…er' into yahya/5098-repackage-network-layer

renames bacl network

c2411ff

jordanschalm approved these changes Nov 26, 2020

View reviewed changes

vishalchangrani approved these changes Nov 26, 2020

View reviewed changes

vishalchangrani reviewed Nov 27, 2020

View reviewed changes

thep2p added 6 commits November 26, 2020 16:31

makes error inline and removes error file

e69cb2f

merges internals into p2p

5b5ca86

Merge remote-tracking branch 'origin/master' into yahya/5098-repackag…

53857d6

…e-network-layer

updates mocknetwork

29421b8

updates makefile

9360d13

generates mocks with mocknetwork

1009da8

vishalchangrani reviewed Nov 27, 2020

View reviewed changes

network/p2p/keyTranslator.go

@@ -1,4 +1,4 @@

package libp2p

package p2p

Copy link

Contributor

vishalchangrani Nov 27, 2020

Choose a reason for hiding this comment

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

✅

refactors mock networks in packages

967b3cd

thep2p requested review from AlexHentschel, ramtinms and zhangchiqing as code owners November 27, 2020 01:30

thep2p added 3 commits November 26, 2020 18:06

fixes broken test

c693647

renames network2 to flownetwork

32e1168

refactors to libp2pnet and flownet

1305edd

thep2p merged commit 069e286 into master Nov 27, 2020

thep2p deleted the yahya/5098-repackage-network-layer branch November 27, 2020 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NET: Yahya/5098-repackage-network-layer #183

NET: Yahya/5098-repackage-network-layer #183

thep2p commented Nov 26, 2020

jordanschalm left a comment

jordanschalm Nov 26, 2020

thep2p Nov 27, 2020

jordanschalm Nov 26, 2020

thep2p Nov 27, 2020

jordanschalm Nov 26, 2020

jordanschalm Nov 26, 2020

thep2p Nov 27, 2020

jordanschalm Nov 26, 2020

vishalchangrani Nov 27, 2020

vishalchangrani Nov 27, 2020

NET: Yahya/5098-repackage-network-layer #183

NET: Yahya/5098-repackage-network-layer #183

Conversation

thep2p commented Nov 26, 2020

jordanschalm 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

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