New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Vishal/refactor network #48
Changes from all commits
55a7328
60cf911
9e70c00
30f68c2
25249d8
e87115b
2c14597
f70458d
bac1694
391ad5c
8be8148
2723e55
88bfe16
103c95f
433d305
e0704c4
585bdcc
eac83be
8ddeb7b
84c097b
722a9d4
d347421
c1828ee
70c484e
9b6345a
681e66c
4556b19
71b1370
703d2e9
34b0c0e
2366b67
4d2a5fc
525a320
b2da736
bb9f48f
fe1f949
cc04818
b18fafa
514e12c
a2d5e47
a2a8747
64be110
21a4c7a
baafb04
20fa2f8
a0ff783
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,6 @@ import ( | |
|
||
chmodels "github.com/onflow/flow-go/model/chunks" | ||
"github.com/onflow/flow-go/model/flow" | ||
"github.com/onflow/flow-go/network/gossip/libp2p/test" | ||
protocolMock "github.com/onflow/flow-go/state/protocol/mock" | ||
"github.com/onflow/flow-go/utils/unittest" | ||
) | ||
|
@@ -44,7 +43,7 @@ func TestAssignment(t *testing.T) { | |
func (a *PublicAssignmentTestSuite) TestByNodeID() { | ||
size := 5 | ||
// creates ids and twice chunks of the ids | ||
ids := test.CreateIDs(size) | ||
ids := unittest.IdentityListFixture(size) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using the more standard |
||
chunks := a.CreateChunks(2*size, a.T()) | ||
assignment := chmodels.NewAssignment() | ||
|
||
|
@@ -83,7 +82,7 @@ func (a *PublicAssignmentTestSuite) TestByNodeID() { | |
func (a *PublicAssignmentTestSuite) TestAssignDuplicate() { | ||
size := 5 | ||
// creates ids and twice chunks of the ids | ||
var ids flow.IdentityList = test.CreateIDs(size) | ||
var ids flow.IdentityList = unittest.IdentityListFixture(size) | ||
chunks := a.CreateChunks(2, a.T()) | ||
assignment := chmodels.NewAssignment() | ||
|
||
|
@@ -121,7 +120,7 @@ func (a *PublicAssignmentTestSuite) TestPermuteEntirely() { | |
|
||
// creates random ids | ||
count := 10 | ||
var idList flow.IdentityList = test.CreateIDs(count) | ||
var idList flow.IdentityList = unittest.IdentityListFixture(count) | ||
var ids flow.IdentifierList = idList.NodeIDs() | ||
original := make(flow.IdentifierList, count) | ||
copy(original, ids) | ||
|
@@ -166,7 +165,7 @@ func (a *PublicAssignmentTestSuite) TestPermuteSublist() { | |
count := 10 | ||
subset := 4 | ||
|
||
var idList flow.IdentityList = test.CreateIDs(count) | ||
var idList flow.IdentityList = unittest.IdentityListFixture(count) | ||
var ids flow.IdentifierList = idList.NodeIDs() | ||
original := make([]flow.Identifier, count) | ||
copy(original, ids) | ||
|
@@ -200,7 +199,7 @@ func (a *PublicAssignmentTestSuite) TestDeterministicy() { | |
snapshot.On("Seed", mock.Anything, mock.Anything, mock.Anything).Return(seed, nil) | ||
|
||
// creates two set of the same nodes | ||
nodes1 := test.CreateIDs(n) | ||
nodes1 := unittest.IdentityListFixture(n) | ||
nodes2 := make([]*flow.Identity, n) | ||
require.Equal(a.T(), copy(nodes2, nodes1), n) | ||
|
||
|
@@ -267,7 +266,7 @@ func (a *PublicAssignmentTestSuite) ChunkAssignmentScenario(chunkNum, verNum, al | |
snapshot.On("Seed", mock.Anything, mock.Anything, mock.Anything).Return(seed, nil) | ||
|
||
// creates nodes and keeps a copy of them | ||
nodes := test.CreateIDs(verNum) | ||
nodes := unittest.IdentityListFixture(verNum) | ||
original := make([]*flow.Identity, verNum) | ||
require.Equal(a.T(), copy(original, nodes), verNum) | ||
|
||
|
@@ -294,7 +293,7 @@ func (a *PublicAssignmentTestSuite) TestCacheAssignment() { | |
snapshot.On("Seed", mock.Anything, mock.Anything, mock.Anything).Return(seed, nil) | ||
|
||
// creates nodes and keeps a copy of them | ||
nodes := test.CreateIDs(5) | ||
nodes := unittest.IdentityListFixture(5) | ||
assigner, err := NewPublicAssignment(3, state) | ||
require.NoError(a.T(), err) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,84 +3,80 @@ package libp2p | |
import ( | ||
"context" | ||
"fmt" | ||
"net" | ||
"time" | ||
|
||
"github.com/libp2p/go-libp2p-core/discovery" | ||
"github.com/libp2p/go-libp2p-core/peer" | ||
"github.com/rs/zerolog" | ||
|
||
"github.com/onflow/flow-go/model/flow" | ||
"github.com/onflow/flow-go/model/flow/filter" | ||
"github.com/onflow/flow-go/network/gossip/libp2p/middleware" | ||
) | ||
|
||
// Discovery implements the discovery.Discovery interface to provide libp2p a way to discover other nodes | ||
type Discovery struct { | ||
ctx context.Context | ||
log zerolog.Logger | ||
overlay middleware.Overlay | ||
me flow.Identifier | ||
done chan struct{} | ||
} | ||
|
||
func NewDiscovery(log zerolog.Logger, overlay middleware.Overlay, me flow.Identifier, done chan struct{}) *Discovery { | ||
d := &Discovery{overlay: overlay, log: log, me: me, done: done} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
func NewDiscovery(ctx context.Context, log zerolog.Logger, overlay middleware.Overlay, me flow.Identifier) *Discovery { | ||
d := &Discovery{ | ||
ctx: ctx, | ||
overlay: overlay, | ||
log: log, | ||
me: me, | ||
} | ||
return d | ||
} | ||
|
||
// Advertise is suppose to advertise this node's interest in a topic to a discovery service. However, we are not using | ||
// a discovery service hence this function just returns a long duration to reduce the frequency with which libp2p calls it. | ||
func (d *Discovery) Advertise(_ context.Context, _ string, _ ...discovery.Option) (time.Duration, error) { | ||
select { | ||
case <-d.done: | ||
return 0, fmt.Errorf("middleware stopped") | ||
default: | ||
return time.Hour, nil | ||
func (d *Discovery) Advertise(ctx context.Context, _ string, _ ...discovery.Option) (time.Duration, error) { | ||
err := ctx.Err() | ||
if err != nil { | ||
return 0, err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would be good to append some metadata information on the place where the error happens in both this and subsequent functions: |
||
} | ||
return time.Hour, nil | ||
} | ||
|
||
// FindPeers returns a channel providing all peers of the node. No parameters are needed as of now since the overlay.Identity | ||
// provides all the information about the other nodes. | ||
func (d *Discovery) FindPeers(_ context.Context, _ string, _ ...discovery.Option) (<-chan peer.AddrInfo, error) { | ||
func (d *Discovery) FindPeers(ctx context.Context, topic string, _ ...discovery.Option) (<-chan peer.AddrInfo, error) { | ||
|
||
// if middleware has been stopped, don't return any peers | ||
select { | ||
case <-d.done: | ||
d.log.Debug().Str("topic", topic).Msg("initiating peer discovery") | ||
|
||
err := ctx.Err() | ||
if err != nil { | ||
emptyCh := make(chan peer.AddrInfo) | ||
close(emptyCh) | ||
return emptyCh, fmt.Errorf("middleware stopped") | ||
default: | ||
return emptyCh, err | ||
} | ||
|
||
// query the overlay to get all the other nodes that should be directly connected to this node for 1-k messaging | ||
// call the callback to get all the other nodes that should be directly connected to this node for 1-k messaging | ||
ids, err := d.overlay.Topology() | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to get ids: %w", err) | ||
} | ||
|
||
// remove self from list of nodes to avoid self dial | ||
delete(ids, d.me) | ||
ids = ids.Filter(filter.Not(filter.HasNodeID(d.me))) | ||
|
||
// create a channel of peer.AddrInfo that needs to be returned | ||
ch := make(chan peer.AddrInfo, len(ids)) | ||
|
||
// send each id to the channel after converting it to a peer.AddrInfo | ||
for _, id := range ids { | ||
// create a new NodeAddress | ||
ip, port, err := net.SplitHostPort(id.Address) | ||
if err != nil { | ||
return nil, fmt.Errorf("could not parse address %s: %w", id.Address, err) | ||
} | ||
|
||
// convert the Flow key to a LibP2P key | ||
lkey, err := PublicKey(id.NetworkPubKey) | ||
nodeAddress, err := nodeAddressFromIdentity(*id) | ||
if err != nil { | ||
return nil, fmt.Errorf("could not convert flow public key to libp2p public key: %v", err) | ||
return nil, fmt.Errorf(" invalid node address: %s, %w", id.String(), err) | ||
} | ||
|
||
nodeAddress := NodeAddress{Name: id.NodeID.String(), IP: ip, Port: port, PubKey: lkey} | ||
addrInfo, err := GetPeerInfo(nodeAddress) | ||
if err != nil { | ||
return nil, fmt.Errorf(" invalid node address: %s, %w", nodeAddress.Name, err) | ||
return nil, fmt.Errorf(" invalid node address: %s, %w", id.String(), err) | ||
} | ||
|
||
// add the address to the channel | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -526,6 +526,17 @@ func (p *P2PNode) UpdateAllowlist(allowListAddrs ...NodeAddress) error { | |
return nil | ||
} | ||
|
||
// IsConnected returns true is address is a direct peer of this node else false | ||
func (p *P2PNode) IsConnected(address NodeAddress) (bool, error) { | ||
pInfo, err := GetPeerInfo(address) | ||
if err != nil { | ||
return false, err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same error appending here. |
||
} | ||
// query libp2p for connectedness status of this peer | ||
isConnected := p.libP2PHost.Network().Connectedness(pInfo.ID) == network.Connected | ||
return isConnected, nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe do this: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think both of us think alike - I had exactly that way and Jordan suggested adding a new variable |
||
} | ||
|
||
func generateProtocolID(rootBlockID string) protocol.ID { | ||
return protocol.ID(FlowLibP2PProtocolIDPrefix + rootBlockID) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the past, when the network unit test ran, the ports were automatically selected when the libp2p host started by specifying the listen address of
0.0.0.0:0
. While that was convenient in terms of getting a free local port to listen on, the test setup became overly complex. The test had to create a half baked network, start the libp2p host and go back and update the peer ids.I changed it so that the test asks for open ports on a system using this third party library and uses those passing it down as the flow.Identity from network -> middleware -> libp2p.