Skip to content
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

Merged
merged 168 commits into from
Jun 19, 2023

Conversation

yhassanzadeh13
Copy link
Contributor

@yhassanzadeh13 yhassanzadeh13 commented Jun 7, 2023

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:

  • Centralization of Decision-making: The logic for disallow-listing decisions has been integrated into the libp2p node, centralizing this aspect.
  • Streamlining Notification Handling: By relocating and simplifying the disallow-listing notification logic to the middleware, the need for a separate pubsub architecture by the notification distributor has been eliminated. This is because the middleware is the sole entity that should consume these notifications to prevent conflicts.
  • Terminology Standardization: All terms related to blocklisting have been updated to the more consistent "disallow-listing".
  • Race Condition Mitigation in Tests: A race condition that was present among connection gaters in the test utilities has been addressed. This was achieved by ensuring that each node is provided with a unique and isolated instance of a connection gater during testing.
  • Middleware Initialization Refinement: Middleware initialization has been streamlined through the incorporation of a configuration structure.
  • Logic Migration for Consistency and Scalability: Certain logics have been moved from the middleware to the libp2p node. This realignment is in accordance with the long-term objective of segregating middleware from the network and libp2p components, contributing to a more scalable and maintainable codebase.

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

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
Copy link
Contributor

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

// add a random delay to initial launch to avoid synchronizing this
// potentially expensive operation across the network

Copy link
Contributor Author

@yhassanzadeh13 yhassanzadeh13 Jun 14, 2023

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.

network/alsp/manager/README.md Outdated Show resolved Hide resolved
network/alsp/manager/README.md Outdated Show resolved Hide resolved
network/alsp/manager/README.md Outdated Show resolved Hide resolved
network/alsp/manager/README.md Outdated Show resolved Hide resolved
network/alsp/manager/README.md Outdated Show resolved Hide resolved
network/alsp/manager/manager.go Outdated Show resolved Hide resolved
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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

network/p2p/p2pnode/libp2pNode.go Show resolved Hide resolved
network/p2p/p2pnode/libp2pNode.go Outdated Show resolved Hide resolved
Copy link
Contributor

@gomisha gomisha left a 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.

yhassanzadeh13 and others added 13 commits June 14, 2023 09:25
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>
@yhassanzadeh13 yhassanzadeh13 merged commit 3ff4476 into master Jun 19, 2023
@yhassanzadeh13 yhassanzadeh13 deleted the yahya/6470-alsp-part-6-pruning branch June 19, 2023 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants