Identity Provider #1133

synzhu · 2021-08-13T01:54:52Z

closes https://github.com/dapperlabs/flow-go/issues/5735
closes https://github.com/dapperlabs/flow-go/issues/5739

TODO:

Write tests for:

For a follow-up PR (?):

~~- create a default for repeated instances of id.NewFilteredIdentifierProvider(...~~ (made obsolete by the SyncEngineIdentifierProvider in #1186

synzhu · 2021-08-13T01:55:14Z

bors try

bors · 2021-08-13T01:57:31Z

try

Build failed:

Unit Tests (1.15)

vishalchangrani · 2021-08-14T00:15:30Z

cmd/scaffold.go

@@ -203,8 +198,7 @@ func (fnb *FlowNodeBuilder) EnqueueNetworkInit(ctx context.Context) {

 		fnb.Network = net

-		idRefresher := p2p.NewNodeIDRefresher(fnb.Logger, fnb.State, net.SetIDs)
-		idEvents := gadgets.NewIdentityDeltas(idRefresher.OnIdentityTableChanged)
+		idEvents := gadgets.NewIdentityDeltas(fnb.Middleware.UpdateAllowList)


so originally, the gadget used to update the network with the new ids and the network would then update middleware.
Why are we directly updating middleware here, wouldn't the overlay be out of sync at that point with the change.

Instead, if the IdentifierProvider provided a way to update its list, then both middleware and network could use it.

synzhu · 2021-08-18T17:15:13Z

@huitseeker Documenting our offline conversation:

TODO: Not only do staked AN's need to use the HierarchicalIDTranslator, but unstaked nodes also do. Otherwise, they will not be able to translate the peer ID's of staked AN's or other staked nodes into flow ID's since the networking key format is different.

cmd/access/node_builder/staked_access_node_builder.go

synzhu · 2021-08-18T18:55:48Z

network/p2p/libp2pNode.go

+	// timeout for FindPeer queries to the DHT
+	// TODO: is this a sensible value?
+	findPeerQueryTimeout = 15 * time.Second


🤷‍♂️

synzhu · 2021-08-18T18:56:05Z

network/p2p/libp2pNode.go

+		// TODO: why were we doing this? Is it okay to remove?
 		// remove the peer from the peer store if present
-		n.host.Peerstore().ClearAddrs(peerID)
+		// n.host.Peerstore().ClearAddrs(peerID)


@vishalchangrani ?

this was done to avoid libp2p doing any exponential backoffs for a connection it tried earlier and failed but still has an entry for it in the peer store.
I can check if it is still needed or not.

Ahh I see... Okay let me know.

synzhu · 2021-08-18T20:26:18Z

network/p2p/middleware.go

+			// We probably don't need to fail the entire function here, since the other
+			// translations may still succeed
+			m.log.Err(err).Str("flowID", fid.String()).Msg("failed to translate to peer ID")
+			continue


Just wanted to raise awareness here, I'm open to the argument that we actually should fail here.

In particular, because the new behavior will likely differ a bit from the old behavior: https://github.com/onflow/flow-go/pull/1133/files#r691579930

I feel that the new behavior is actually better, but I'm not super confident on this.

In general, my thinking was that returning a partial list of peer IDs is at least better than completely failing because a single flow ID couldn't be translated.

yeah, dont need to fail - I had to change the return earlier on v0.20 and master

synzhu · 2021-08-18T20:26:36Z

network/p2p/peerstore_provider.go

+			p.logger.Err(err).Str("peerID", pid.Pretty()).Msg("failed to translate to Flow ID")
+			continue


synzhu · 2021-08-18T20:26:53Z

network/p2p/protocol_state_provider.go

+			p.logger.Err(err).Interface("identity", identity).Msg("failed to extract peer ID from network key")
+			continue


synzhu · 2021-08-18T20:28:10Z

network/p2p/libp2pConnector.go

-
-	// if some ids didn't translate to peer.AddrInfo, return error
-	if len(invalidIDs) != 0 {
-		return NewUnconvertableIdentitiesError(invalidIDs)


network/p2p/libp2pNode.go

network/p2p/network.go

network/p2p/protocol_state_provider.go

network/p2p/peerManager_test.go

synzhu · 2021-08-21T06:03:03Z

96cf07f

I realized the origin ID checks need to use the ID translator instead of Overlay.Identities, because the latter only returns the protocol state identities.

This would not work for staked AN or unstaked nodes, because the validation would fail for everyone else.

codecov-commenter · 2021-08-21T22:17:36Z

Codecov Report

Merging #1133 (7f2fc0b) into master (0714d5a) will increase coverage by 0.16%.
The diff coverage is 54.00%.

@@            Coverage Diff             @@
##           master    #1133      +/-   ##
==========================================
+ Coverage   56.06%   56.23%   +0.16%     
==========================================
  Files         487      495       +8     
  Lines       29951    30124     +173     
==========================================
+ Hits        16793    16940     +147     
- Misses      10870    10885      +15     
- Partials     2288     2299      +11

Flag	Coverage Δ
unittests	`56.23% <54.00%> (+0.16%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/collection/main.go	`2.85% <0.00%> (ø)`
cmd/consensus/main.go	`3.34% <0.00%> (ø)`
engine/access/ping/engine.go	`0.00% <ø> (ø)`
engine/channels.go	`85.07% <0.00%> (-2.62%)`	⬇️
engine/common/splitter/engine.go	`54.90% <ø> (-2.51%)`	⬇️
model/flow/identifierList.go	`27.02% <0.00%> (-8.69%)`	⬇️
network/p2p/identity_provider_translator.go	`0.00% <0.00%> (ø)`
network/p2p/libp2pConnector.go	`0.00% <0.00%> (ø)`
network/p2p/middleware.go	`0.00% <0.00%> (ø)`
network/p2p/network.go	`0.00% <0.00%> (ø)`
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81ec901...7f2fc0b. Read the comment docs.

Re-generate mocks

…fication-seal-approvals and required-construction-seal-approvals to 1 in the unstaked integration test

Unstaked ANs issue unicast messages on syncrequests and correspondingly need one.

Update unstaked_node_test.go Update core.go

huitseeker

I think there's a bunch of followups, but this otherwise LGTM!
🚀

huitseeker · 2021-09-02T18:08:48Z

cmd/access/node_builder/unstaked_access_node_builder.go

-	builder.bootstrapIdentites = ids
+	dhtOptions = append(dhtOptions, bootstrapPeersOpt)
+
+	connManager := p2p.NewConnManager(builder.Logger, builder.Metrics.Network, p2p.TrackUnstakedConnections(builder.IdentityProvider))


@vishalchangrani we may want to open a follow-up issue to do what you had in mind, i.e. making Stream establishment non-dependent on Protect calls for nodes that don't have connection gating

huitseeker · 2021-09-02T18:25:51Z

network/p2p/libp2pNode.go

+	// timeout for FindPeer queries to the DHT
+	// TODO: is this a sensible value?
+	findPeerQueryTimeout = 10 * time.Second


We're already tracking this in essentially #1184 . I think this value is a good start, but we may want to rename the issue @smnzhu

huitseeker · 2021-09-02T18:28:24Z

network/p2p/protocol_state_provider.go

@@ -0,0 +1,129 @@
+package p2p


I think our friends working on epochs would <3 a heads up (off-PR) on the overlap between this and the epoch_transition_test @smnzhu

synzhu requested review from huitseeker, vishalchangrani and yhassanzadeh13 August 13, 2021 01:54

bors bot added a commit that referenced this pull request Aug 13, 2021

Try #1133:

031eade

vishalchangrani reviewed Aug 14, 2021

View reviewed changes

huitseeker mentioned this pull request Aug 18, 2021

[Access] Fix TODO's for unstaked node #1126

Closed

2 tasks

huitseeker force-pushed the smnzhu/identity-provider branch from bdec2f2 to 8c999e2 Compare August 18, 2021 17:41

synzhu commented Aug 18, 2021

View reviewed changes

cmd/access/node_builder/staked_access_node_builder.go Outdated Show resolved Hide resolved

synzhu commented Aug 18, 2021

View reviewed changes

cmd/access/node_builder/staked_access_node_builder.go Outdated Show resolved Hide resolved

synzhu commented Aug 18, 2021

View reviewed changes

vishalchangrani reviewed Aug 19, 2021

View reviewed changes

network/p2p/libp2pNode.go Show resolved Hide resolved

vishalchangrani reviewed Aug 19, 2021

View reviewed changes

network/p2p/network.go Outdated Show resolved Hide resolved

vishalchangrani reviewed Aug 19, 2021

View reviewed changes

network/p2p/protocol_state_provider.go Outdated Show resolved Hide resolved

huitseeker reviewed Aug 20, 2021

View reviewed changes

network/p2p/peerManager_test.go Outdated Show resolved Hide resolved

huitseeker mentioned this pull request Aug 20, 2021

[bootstrap] Key Generation for Unstaked Access Nodes #1180

Merged

synzhu mentioned this pull request Aug 21, 2021

[Access] Choose a sensible value for findPeerQueryTimeout #1184

Open

1 task

synzhu changed the title ~~Smnzhu/identity provider~~ Identity Provider Aug 21, 2021

synzhu marked this pull request as ready for review August 21, 2021 22:30

synzhu requested review from AlexHentschel and jordanschalm as code owners August 21, 2021 22:30

synzhu and others added 26 commits September 2, 2021 14:05

Remove unneeded files

91d4232

Update connmanager

5e6e413

re-generate mocks

3733f7e

Finish fixing testUtil.go

f3cae1c

Fix DHT test, adapt supportsUnstakedFollower flag

847c112

fix sync request proxy bug

db4fc82

Make goimports happy

944d856

Re-generate mocks

fix unit test

4c4efee

update channel Exists method

1a3f6aa

New test for sync provider

f2722df

Add node role

037d9ee

using default message validators for all nodes; setting required-veri…

59962a3

…fication-seal-approvals and required-construction-seal-approvals to 1 in the unstaked integration test

adding CleanerCollector to metrics in scaffold

ce445d8

Extend the change from #1230 to all nodes

0c87ebd

install a connection manager on unstaked ANs

40aa3dc

Unstaked ANs issue unicast messages on syncrequests and correspondingly need one.

Update middleware options

497ab8b

Added logging for sync engine

c93e1a9

set log level of consensus follower to debug in tests

ac22ab0

Update network.go

3defbf6

Increase integration test timeout

99d90ec

increase num request and add block ids to debug

d4636de

Adding loging for sync requests

a173e19

Update libp2pNode_test.go

43fdfe6

Update unstaked_node_test.go Update core.go

add more logging

43ebbd6

Make CI print integration tests in verbose mode (just like unit tests)

a916831

simplify logging

4ce2800

huitseeker force-pushed the smnzhu/identity-provider branch from 9d8423a to 4ce2800 Compare September 2, 2021 18:07

huitseeker approved these changes Sep 2, 2021

View reviewed changes

synzhu merged commit 4b25ece into master Sep 2, 2021

bors bot deleted the smnzhu/identity-provider branch September 2, 2021 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Identity Provider #1133

Identity Provider #1133

synzhu commented Aug 13, 2021 •

edited by huitseeker

synzhu commented Aug 13, 2021

bors bot commented Aug 13, 2021

vishalchangrani Aug 14, 2021

synzhu commented Aug 18, 2021

synzhu Aug 18, 2021

synzhu Aug 18, 2021

vishalchangrani Aug 19, 2021

synzhu Aug 19, 2021

synzhu Aug 18, 2021

synzhu Aug 18, 2021

synzhu Aug 18, 2021

vishalchangrani Aug 19, 2021

synzhu Aug 18, 2021

synzhu Aug 18, 2021

synzhu Aug 18, 2021

synzhu commented Aug 21, 2021

codecov-commenter commented Aug 21, 2021 •

edited

huitseeker left a comment

huitseeker Sep 2, 2021

huitseeker Sep 2, 2021

huitseeker Sep 2, 2021

		p.logger.Err(err).Str("peerID", pid.Pretty()).Msg("failed to translate to Flow ID")
		continue

		p.logger.Err(err).Interface("identity", identity).Msg("failed to extract peer ID from network key")
		continue

Identity Provider #1133

Identity Provider #1133

Conversation

synzhu commented Aug 13, 2021 • edited by huitseeker

TODO:

For a follow-up PR (?):

synzhu commented Aug 13, 2021

bors bot commented Aug 13, 2021

try

Choose a reason for hiding this comment

synzhu commented Aug 18, 2021

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

synzhu commented Aug 21, 2021

codecov-commenter commented Aug 21, 2021 • edited

Codecov Report

huitseeker 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

synzhu commented Aug 13, 2021 •

edited by huitseeker

codecov-commenter commented Aug 21, 2021 •

edited