-
Notifications
You must be signed in to change notification settings - Fork 180
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
[ALSP] Implements disallowlisting logic #4441
Conversation
…to yahya/6470-alsp-part-5-decay
var DefaultPeerUpdateInterval = 10 * time.Minute | ||
// DefaultPeerUpdateInterval is default duration for which the peer manager waits in between attempts to update peer connections. | ||
// We set it to 1 second to be aligned with the heartbeat intervals of libp2p, alsp, and gossipsub. | ||
var DefaultPeerUpdateInterval = time.Second |
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.
This is a 600x increase in the number of peer updates. Is that safe to do? I was under the impression this was an expensive operation
flow-go/network/p2p/connection/peerManager.go
Lines 87 to 88 in 0ee0a2a
// add a random delay to initial launch to avoid synchronizing this | |
// potentially expensive operation across the network |
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.
Summarizing our discussion:
Shortening the peer update intervals is safe due to two factors. Firstly, with this PR's changes, connections are established with nodes only if they don’t exist already, making this update inconsequential if there are no changes in the allow-listed or staked peers. Secondly, it's vital for the network layer's safety measures that all heartbeats are synchronized. This ensures, for example, that if the ALSP disallow-lists a node within a heartbeat, the disallow-listing is guaranteed to be completed by the next heartbeat.
if err != nil { | ||
m.log.Error().Err(err).Str("peer_id", pid.String()).Msg("failed to disconnect from blocklisted peer") | ||
} | ||
func (m *Middleware) OnDisallowListNotification(notification *network.DisallowListingUpdate) { |
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.
Using a core component like Middleware
as the distributor for these notifications is going to cause weird dependency chains, like you flagged in NodeDisallowListingWrapper
.
What do you think about keeping the distributor as a stand along component instead? That way it can be built and passed into components separately.
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 this PR, we observed that distributing the disallow-listing notification is unnecessary since it has only one consumer, the middleware
. Therefore, this PR directly connects the consumer to the notification producer. Given that there's just one consumer, having a distributor broker seems excessive. Additionally, as we’re phasing out the middleware
in favor of integrating it into the LibP2PNode
and network
components. Eventually, only the network
layer will consume these notifications. This aligns well architecturally with the network layer handling notifications regarding node disallow-listing at the network level.
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.
LGTM, just a few minor typos.
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
Co-authored-by: Misha <misha.rybalov@dapperlabs.com>
Co-authored-by: Misha <misha.rybalov@dapperlabs.com>
…into yahya/6470-alsp-part-6-pruning
This PR concludes the development of the Application Layer Spam Prevention (ALSP) Reputation Management System (RMS) within the Flow blockchain by implementing its sixth and final part. The key objective of this PR is to equip the ALSP manager with the capability to allow-list and disallow-list nodes. A node is disallow-listed if its accumulated penalty drops below a specified threshold, resulting in termination of the existing connection and prevention of any future connections. Once the penalties for a disallow-listed node have fully decayed, it is reinstated to the allow-list, allowing the establishment of a connection with it.
Furthermore, this PR introduces several enhancements and refinements:
This PR not only concludes the development of the ALSP RMS component but also enhances the overall system's robustness, efficiency, and maintainability.
Follow-up issues:
[Application Layer Spam Prevention] Implement Batch Mode for Sending Disallow-Listing Notifications in MisbehaviorReportManage