[Merged by Bors] - Libp2p integration #2801

dshulyak · 2021-09-20T07:25:31Z

Motivation

Open issues

Internal queuing

Gossipsub uses non blocking channels for internal queueing. In general this is reasonable approach, but we could use priority queue in place.

Approach that i am thinking about requires small change in the codebase to make queue instantion replaceable. I will do some experimentation and then open a discussion in go-libp2p-pubsub repo.

type Queue interface {
    Next(context.Context) (*RPC, error)
    Send(context.Context, *RPC) error
}

type NewQueue() func(buffer int) Queue

And use this interface in router.

No delayed confirmations

Gossipsub launches async validation in a separate goroutine (limited by a parameter), and it expects goroutine to return before processing a message.

Some data can be validated only in certain time window. This time window is weakly synchronized between peers. So it is natural that some peers will send data earlier than others and dropping this data is not an option. We want to buffer such data and validate it after time window arrives. Buffering it using goroutines is not optimal.

Instead we could use delayed confirmation using callback (similar to what is done in current codebase).

We use this approach for tortoise beacon proposals.

I don't see blockers in the codebase that will prevent us from doing it, but requires more work.

Library doesn't wait for goroutines to exit

It doesn't mean that it leaks them, but it definitely doesn't wait for exit. More important resources (e.g. sockets) are cleaned up appropriately,

Global logging

We will need to overwrite logger configuration in ipfs/go-log library which will affect all libp2p subsystems.

Changes

Test Plan

TODO

DevOps Notes

This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
This PR does not affect public APIs
This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
This PR does not make changes to log messages (which monitoring infrastructure may rely on)

bors · 2021-10-22T09:19:41Z

try

Build succeeded:

ci-stage2

dshulyak · 2021-10-22T09:21:00Z

bors try

bors · 2021-10-22T10:14:30Z

try

Build failed:

ci-stage2

dshulyak · 2021-10-22T10:14:57Z

in the last test kibana failed to setup on time, but otherwise tests look stable

countvonzero · 2021-10-22T07:35:12Z

lp2p/bootstrap/bootstrap.go

+			if !ok {
+				panic("event must be of type EventHandshakeComplete")
+			}
+			if _, exist := peers[hs.PID]; exist {


what does this mean? is it expected to be notified multiple times of the same event? or that the node maybe complete handshake with the same peer multiple times?

yes, connection with the same peer is not prohibited, the likelihood of it very low, but there is no explicit mechanism to enforce single connection if two peers will try to establish connection simultaneously, or if peer will create multiple connections by intention

lp2p/bootstrap/bootstrap.go

lp2p/bootstrap/peers.go

lp2p/peerexchange/crawler.go

lp2p/peerexchange/discovery.go

lp2p/peerexchange/protocol.go

countvonzero · 2021-10-22T11:16:21Z

lp2p/peerexchange/store.go

+		}
+	}
+
+	w, err := os.Create(path)


i wonder if we should create a tmp file and swap after the write is successful.

that would be correct, i will open an issue after merge

countvonzero

thanks for the explanations.

countvonzero · 2021-10-23T04:38:52Z

tortoisebeacon/tortoise_beacon.go

+	// NOTE(dshulyak) moved to goroutine because self-broadcast is applied synchronously
+	tb.eg.Go(func() error {
+		if err := tb.publisher.Publish(ctx, protocol, serialized); err != nil {
+			return fmt.Errorf("broadcast: %w", err)


from the application logic point of view, a single failure should not stop the tortoise beacon from working, regardless of how unlikely Publish should fail.

countvonzero · 2021-10-23T04:41:49Z

tortoisebeacon/weakcoin/weak_coin.go

 		return nil
 	}

-	if err := wc.net.Broadcast(ctx, GossipProtocol, broadcast); err != nil {
+	if err := wc.publisher.Publish(ctx, GossipProtocol, msg); err != nil {


actually i think it may be a good practice to simply put all Publish on goroutine, since it validate synchronously.

lp2p/bootstrap/bootstrap.go

countvonzero · 2021-10-23T04:57:44Z

lp2p/peerexchange/crawler.go

+			}
+			pending++
+
+			go func() {


understood. maybe leave a TODO?

countvonzero · 2021-10-23T05:01:05Z

lp2p/host.go

+		if err != nil {
+			return nil, fmt.Errorf("can't create peer addr from %s: %w", p, err)
+		}
+		cm.Protect(addr.ID, "bootstrap")


as discussed during your walk-through, i think we should not special-case bootnode

countvonzero · 2021-10-23T05:08:20Z

lp2p/peerexchange/addrbook.go

-		a.logger.Debug("skipping adding non routable address%v", netAddr.String())
-		// XXX: this makes tests work with unroutable addresses(loopback)
+func (a *addrBook) updateAddress(addr, src *addrInfo) {
+	if !IsRoutable(addr.IP) && IsRoutable(src.IP) {


still don't understand the rationale. does that mean we won't allow src as one of the peers?
say there is a private network, and it has a public node acting as its only gateway to the public network, we will not connect to it?

just want to understand. i understand its old code.

…eived

dshulyak · 2021-10-25T05:22:03Z

bors try

bors · 2021-10-25T06:13:16Z

try

Build succeeded:

ci-stage2

dshulyak · 2021-10-25T06:17:24Z

bors merge

## Motivation ## Open issues ### Internal queuing Gossipsub uses non blocking channels for internal queueing. In general this is reasonable approach, but we could use priority queue in place. Approach that i am thinking about requires small change in the codebase to make queue instantion replaceable. I will do some experimentation and then open a discussion in go-libp2p-pubsub repo. ```go type Queue interface { Next(context.Context) (*RPC, error) Send(context.Context, *RPC) error } type NewQueue() func(buffer int) Queue ``` And use this interface in [router](https://github.com/libp2p/go-libp2p-pubsub/blob/628353661b1292a682ddd898da1594f9795e7670/pubsub.go#L659-L662). ### No delayed confirmations Gossipsub launches async validation in a separate goroutine (limited by a parameter), and it expects goroutine to return before processing a message. Some data can be validated only in certain time window. This time window is weakly synchronized between peers. So it is natural that some peers will send data earlier than others and dropping this data is not an option. We want to buffer such data and validate it after time window arrives. Buffering it using goroutines is not optimal. Instead we could use delayed confirmation using callback (similar to what is done in current codebase). We use this approach for tortoise beacon proposals. I don't see blockers in the codebase that will prevent us from doing it, but requires more work. ### Library doesn't wait for goroutines to exit It doesn't mean that it leaks them, but it definitely doesn't wait for exit. More important resources (e.g. sockets) are cleaned up appropriately, ### Global logging We will need to overwrite logger configuration in ipfs/go-log library which will affect all libp2p subsystems. ## Changes  ## Test Plan  ## TODO - [x] save identity into the file currently only private key is saved - [x] tests refactoring some interfaces are very different, try to find common interface that is easy to mock. - [x] activation - [x] tortoise beacon - [x] syncer - [x] miner - [x] fetch - [x] layerfetcher - [x] api - [x] cmd - [x] hare - [x] nodes are connecting too eagerly with other peers in devnet all nodes have 49 connections (with 50 nodes in total) modify peerexchange.Bootstrap to accept required number of peers, terminate early if connections were eastablished during the round. maybe thats not an issue actually, redundant connections are purged using another component (connection manager) based on separate configuration manager. and the gossip size is bounded. - [x] revert changes to p2p tests i removed some of the redundant tests for a faster debug, need to check if they are needed and re-enable them if thats the case. - [x] tests for bootstrap.Peers and bootstrap.Bootstrap - [x] submit sync binary removal as a separate request - [x] submit http server refactoring as a separate request ## DevOps Notes  - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)

bors · 2021-10-25T07:13:32Z

Pull request successfully merged into develop.

Build succeeded:

ci-stage2

dshulyak marked this pull request as draft September 20, 2021 10:57

dshulyak mentioned this pull request Sep 22, 2021

Libp2p POC in go-spacemesh spacemeshos/pm#85

Closed

nkryuchkov mentioned this pull request Oct 6, 2021

timesync: /peersync/1.0/ handler is missing for request #2841

Closed

moshababo assigned dshulyak Oct 11, 2021

dshulyak force-pushed the libp2p branch from 7ffaaf0 to dfb405a Compare October 13, 2021 04:22

Libp2p

06031d7

dshulyak force-pushed the libp2p branch from e0fffdb to 06031d7 Compare October 13, 2021 05:02

dshulyak added 23 commits October 13, 2021 08:58

Fix sync,api tests and hare binary

73ab67a

Fix weakcoin

8cc6369

Remove mocks from weakcoin

cb08efb

mapstructure for p2p config

c55fe7b

Update p2p tests

c708638

Remove p2p prefix from target-outbound for now

6ed3d72

Update configuration in tests

914c08e

Fix directory in hare and p2p binary

79ef215

Move routable check to original palce

685bd2c

Enable p2p debug logs

622f392

Use routable address intests

4f09277

Setup lp2p global loggin

5697a05

Connect in crawler

a2bec45

Change expectations

18b388c

Retry api_call

a18b4f4

go mod tidy

047a0be

Clear deadline

1f72e2c

Trigger bootstrap on timer

0071947

Add bufio strems to peerexchange protocol

c95f748

Build with newer grpc

dc550ca

Remove retries

64ff63c

Update grpc-gateeway

bbf52ee

Set logger core for libp2p log

85a2288

Add missing

ae86d59

bors bot added a commit that referenced this pull request Oct 22, 2021

Try #2801:

bc80b46

countvonzero reviewed Oct 22, 2021

View reviewed changes

dshulyak added 2 commits October 22, 2021 15:41

newPeers is not not used change it to private

fba0d43

Log peer id and address when network id mismatches

6c6ba23

countvonzero approved these changes Oct 23, 2021

View reviewed changes

dshulyak added 8 commits October 25, 2021 06:17

Dont terminate on publish error

6c8faff

Don't track peer if it was disconnected by the time when event is rec…

45a38f7

…eived

Resolve bootstrap/peers.go feedback

039a8bc

Rename Wrap to Upgrade

0590b4f

Wait for goroutines to exit in peerexchange.crawler

5b714d0

Log protocol

c3c68de

Enable NodeStatus notifications

8577818

Rename lp2p to p2p

a3410f6

dshulyak force-pushed the libp2p branch from 0475ad5 to a3410f6 Compare October 25, 2021 03:55

dshulyak added 2 commits October 25, 2021 06:57

Replace outdated mocks

f2cea1d

Revert sync binary removal

4d2da06

bors bot added a commit that referenced this pull request Oct 25, 2021

Try #2801:

98d699c

bors bot changed the title ~~Libp2p integration~~ [Merged by Bors] - Libp2p integration Oct 25, 2021

bors bot closed this Oct 25, 2021

huly-for-github bot unassigned nkryuchkov Aug 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Libp2p integration #2801

[Merged by Bors] - Libp2p integration #2801

dshulyak commented Sep 20, 2021 •

edited

Loading

bors bot commented Oct 22, 2021

dshulyak commented Oct 22, 2021

bors bot commented Oct 22, 2021

dshulyak commented Oct 22, 2021

countvonzero Oct 22, 2021

dshulyak Oct 22, 2021 •

edited

Loading

countvonzero Oct 22, 2021

dshulyak Oct 22, 2021

countvonzero left a comment

countvonzero Oct 23, 2021

countvonzero Oct 23, 2021

countvonzero Oct 23, 2021

countvonzero Oct 23, 2021

countvonzero Oct 23, 2021

dshulyak commented Oct 25, 2021

bors bot commented Oct 25, 2021

dshulyak commented Oct 25, 2021

bors bot commented Oct 25, 2021

[Merged by Bors] - Libp2p integration #2801

[Merged by Bors] - Libp2p integration #2801

Conversation

dshulyak commented Sep 20, 2021 • edited Loading

Motivation

Open issues

Internal queuing

No delayed confirmations

Library doesn't wait for goroutines to exit

Global logging

Changes

Test Plan

TODO

DevOps Notes

bors bot commented Oct 22, 2021

try

dshulyak commented Oct 22, 2021

bors bot commented Oct 22, 2021

try

dshulyak commented Oct 22, 2021

Choose a reason for hiding this comment

dshulyak Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

countvonzero 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

dshulyak commented Oct 25, 2021

bors bot commented Oct 25, 2021

try

dshulyak commented Oct 25, 2021

bors bot commented Oct 25, 2021

dshulyak commented Sep 20, 2021 •

edited

Loading

dshulyak Oct 22, 2021 •

edited

Loading