Conversation
|
Replaces: #1269 |
|
| Branch | feat/new-whitelist-policy |
| Testbed | ubuntu-22.04 |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result minutes (m) (Result Δ%) | Lower Boundary minutes (m) (Limit %) | Upper Boundary minutes (m) (Limit %) |
|---|---|---|---|---|
| sync-v2 (up to 20000 blocks) | 📈 view plot 🚷 view threshold | 1.71 m(-0.10%)Baseline: 1.71 m | 1.54 m (90.09%) | 2.06 m (83.25%) |
a414e74 to
812f883
Compare
df4d9d6 to
e5c9d1b
Compare
c35a8f9 to
743a986
Compare
743a986 to
4519d69
Compare
hathor/p2p/manager.py
Outdated
| """Suspend the active whitelist (sysctl 'off').""" | ||
| if not self.peers_whitelist: | ||
| return | ||
| self.peers_whitelist.stop() |
There was a problem hiding this comment.
Should suspended whitelists keep updating?
There was a problem hiding this comment.
No, but require a refresh when reactivating.
4519d69 to
d944ce0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1544 +/- ##
==========================================
+ Coverage 85.65% 85.67% +0.02%
==========================================
Files 439 445 +6
Lines 33515 33817 +302
Branches 5264 5302 +38
==========================================
+ Hits 28707 28974 +267
- Misses 3797 3831 +34
- Partials 1011 1012 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a60aa15 to
5e76bfa
Compare
LFRezende
left a comment
There was a problem hiding this comment.
Review - Luis Felipe/LFRezende
Core:
- Possible oversight which does not add bootstrap peers at connect_with_bootstrap_registration, lines 268-275
- Discussion and further recommendation to delete --x-p2p-whitelist-only flag.
- Discussion on expanding mechanism of suspended_peers.
The rest are smaller refactor suggestions.
hathor/p2p/whitelist/factory.py
Outdated
| PeersWhitelist instance or None if disabled | ||
| """ | ||
| peers_whitelist: PeersWhitelist | None = None | ||
| spec_lower = whitelist_spec.lower() |
There was a problem hiding this comment.
Perhaps a spec = whitelist_spec.lower().strip() to eliminate any accidental whitespaces and forcing it into default by accident.
|
|
||
| if self._url is None: | ||
| return | ||
| if self._url.lower() == 'none': |
There was a problem hiding this comment.
Same as before - perhaps .lower().strip() would add a further caution to detail.
| def connect_with_bootstrap_registration(entrypoint: PeerEndpoint) -> None: | ||
| if self.peers_whitelist and entrypoint.peer_id is not None: | ||
| self.peers_whitelist.add_bootstrap_peer(entrypoint.peer_id) | ||
| self.connect_to_endpoint(entrypoint) |
There was a problem hiding this comment.
At the beginning of the bootstrap, when peers_whitelist = set(), each entrypoint provided for bootstrapping peers will connect, but it will not add the peer_id of that entrypoint to peers_whitelist.
peers_whitelist AND entrypoint.peer_id is not None --> At the beginning, peers_whitelist == None, hence bootstrap will not add peerId to the set, hence none will be added, just have its entrypoint connected via connect_to_endpoint.
There was a problem hiding this comment.
Perhaps we could do something like:
def connect_with_bootstrap_registration(entrypoint: PeerEndpoint) -> None:
if not self.peers_whitelist:
return # Or raise exception
if entrypoint.peer_id is not None:
self.peers_whitelist.add_bootstrap_peer(entrypoint.peer_id)
self.connect_to_endpoint(entrypoint)
It ensures that, if no whitelist, there will be no connection, since there are no peers to bootstrap to.
And if there is, check whether they yield an entrypoint peer id. If so, then, and only then shall you add them to the bootstrap peer.
If you want only to connect to entrypoints with a peer_id, then connect to them.
If not, just put self.connect_to_endpoint(entrypoint) out of the if clause.
Makes sense?
There was a problem hiding this comment.
I fixed it using another approach: All bootstrap nodes skip the whitelist verification. Can you review it again, please?
There was a problem hiding this comment.
Just did - seems fair: bootstrap peer ids are ones we are completely knowable about, so it makes sense to skip whitelist verification. Additionally, the flag "has_successful_fetch" was ingenious as it blocks unintended peers from connecting before bootstrap peers have connected.
hathor/p2p/manager.py
Outdated
| peer_id = conn.get_peer_id() | ||
| if peer_id is None: | ||
| continue | ||
| if not self.peers_whitelist.is_peer_whitelisted(peer_id): |
There was a problem hiding this comment.
Small suggestion: perhaps whitelist = self.peers_whitelist would make the function less verbose.
--> if not whitelist;
--> if not whitelist.is_peer_whitelisted(peer_id)
| if option == 'on': | ||
| if self._suspended_whitelist is None: | ||
| return | ||
| self.connections.set_peers_whitelist(self._suspended_whitelist) | ||
| self._suspended_whitelist = None | ||
| return | ||
| if option == 'off': | ||
| if self.connections.peers_whitelist is None: | ||
| return | ||
| self._suspended_whitelist = self.connections.peers_whitelist | ||
| self.connections.set_peers_whitelist(None) | ||
| return |
There was a problem hiding this comment.
So the mechanism proposed is to put the whitelist peers in suspension and turn the whitelist None (Off) and then take these peers from the "suspended set" to whitelist if ON? Interesting.
Perhaps we could expand on this idea and suspend peers in general (not just whitelist) in case we detect misbehavior from other peers - like a "blacklist" or "quarantine" of sorts.
There was a problem hiding this comment.
I have a PR open for this blacklist, if this is what you were thinking about: #1554
It's a manual blacklist, we need to add peers there on demand.
My PR actually only adds support in sysctl to a blacklist logic we had already implemented in hathor-core.
There was a problem hiding this comment.
Blacklisting can help when a specific full node is misbehaving. However, it’s not very effective against targeted attacks. Generating new peer IDs is extremely cheap, so an attacker can simply create a new identity and reconnect to your node almost immediately.
hathor/sysctl/p2p/manager.py
Outdated
| def set_whitelist(self, new_whitelist: str) -> None: | ||
| """Set the whitelist-only mode. If 'on' or 'off', simply changes the | ||
| following status of current whitelist. If an URL or Filepath, changes | ||
| the whitelist object, following it by default. | ||
| It does not support eliminating the whitelist (passing None).""" | ||
|
|
||
| option: str = new_whitelist.lower().strip() | ||
| if option == 'on': | ||
| if self._suspended_whitelist is None: | ||
| return | ||
| self.connections.set_peers_whitelist(self._suspended_whitelist) | ||
| self._suspended_whitelist = None | ||
| return | ||
| if option == 'off': | ||
| if self.connections.peers_whitelist is None: | ||
| return | ||
| self._suspended_whitelist = self.connections.peers_whitelist | ||
| self.connections.set_peers_whitelist(None) |
There was a problem hiding this comment.
Small nitpick: perhaps connections = self.connections would reduce verbose, as it is a term used frequently.
| def test_whitelist_cli_args(self): | ||
| """Test --x-p2p-whitelist and --x-p2p-whitelist-only CLI arguments.""" | ||
| # Test with whitelist URL | ||
| manager = self._build(['--temp-data', '--x-p2p-whitelist', 'https://example.com/whitelist']) | ||
| self.assertIsNotNone(manager.connections.peers_whitelist) | ||
| self.assertIsInstance(manager.connections.peers_whitelist, URLPeersWhitelist) | ||
|
|
||
| # Test with whitelist-only flag (now a no-op, whitelist always enforces) | ||
| manager2 = self._build([ | ||
| '--temp-data', '--x-p2p-whitelist', 'https://example.com/whitelist', | ||
| '--x-p2p-whitelist-only', | ||
| ]) | ||
| self.assertIsNotNone(manager2.connections.peers_whitelist) | ||
|
|
||
| # Test with disabled whitelist | ||
| manager3 = self._build(['--temp-data', '--x-p2p-whitelist', 'none']) | ||
| self.assertIsNone(manager3.connections.peers_whitelist) |
There was a problem hiding this comment.
Since we're removing --x-p2p-whitelist-only flag from cli, we may delete them here.
5b8b5fd to
e623233
Compare
b361dc6 to
0fdcd2e
Compare
LFRezende
left a comment
There was a problem hiding this comment.
Last changes on bootstrap peers and their derived tests seem reasonable.
Ran linter and tests locally:
- All linters work fine
- Two tests failed, and the last one seems to be connected to bootstrap (already communicated msbrogli).
It seems to be flaking - I'll approve.
| parser.add_argument('--x-disable-ipv4', action='store_true', | ||
| help='Disables connecting to IPv4 peers') | ||
|
|
||
| parser.add_argument("--x-p2p-whitelist", help="Add whitelist to follow from since boot.") |
| # Whitelist specification constants | ||
| WHITELIST_SPEC_DEFAULT = 'default' | ||
| WHITELIST_SPEC_HATHORLABS = 'hathorlabs' | ||
| WHITELIST_SPEC_NONE = 'none' | ||
| WHITELIST_SPEC_DISABLED = 'disabled' |
| if spec_lower in (WHITELIST_SPEC_DEFAULT, WHITELIST_SPEC_HATHORLABS): | ||
| peers_whitelist = URLPeersWhitelist(reactor, str(settings.WHITELIST_URL), True) | ||
| elif spec_lower in (WHITELIST_SPEC_NONE, WHITELIST_SPEC_DISABLED): | ||
| peers_whitelist = None | ||
| elif _looks_like_url(whitelist_spec): | ||
| peers_whitelist = URLPeersWhitelist(reactor, whitelist_spec, True) |
There was a problem hiding this comment.
Why are you passing mainnet=True in the constructors? Also it would be better if those args were kw only.
| parse_whitelist('''hathor-whitelist | ||
| # node1 | ||
| 2ffdfbbfd6d869a0742cff2b054af1cf364ae4298660c0e42fa8b00a66a30367 | ||
|
|
||
| 2ffdfbbfd6d869a0742cff2b054af1cf364ae4298660c0e42fa8b00a66a30367 | ||
|
|
||
| # node3 | ||
| G2ffdfbbfd6d869a0742cff2b054af1cf364ae4298660c0e42fa8b00a66a30367 | ||
| 2ffdfbbfd6d869a0742cff2b054af1cf364ae4298660c0e42fa8b00a66a30367 | ||
| ''') | ||
| {'2ffdfbbfd6d869a0742cff2b054af1cf364ae4298660c0e42fa8b00a66a30367'} |
There was a problem hiding this comment.
This comment is wrong, use doctests instead.
|
|
||
| """ | ||
| lines = parse_file(text, header=header) | ||
| return {PeerId(line.split()[0]) for line in lines} |
|
|
||
| peers_to_remove = current_whitelist - new_whitelist | ||
| for peer_id in peers_to_remove: | ||
| if self._on_remove_callback: |
| def refresh(self) -> Deferred[None]: | ||
| return self._unsafe_update() |
There was a problem hiding this comment.
It looks like this is only used in a test, it could be removed.
| if mainnet: | ||
| if result.scheme != 'https': | ||
| raise ValueError(f'invalid scheme: {self._url}') |
There was a problem hiding this comment.
Should this come from a setting in HathorSettings instead of hardcoding it for mainnet?
| if not result.netloc: | ||
| raise ValueError(f'invalid url: {self._url}') |
There was a problem hiding this comment.
This should be checked even for non-mainnet, right?
| d: Deferred[None] = Deferred() | ||
| d.callback(None) | ||
| return d |
Co-authored-by: LFRezende <lfsrsprofessional@gmail.com>
0fdcd2e to
30a7cac
Compare
Co-Author: LFRezende lfsrsprofessional@gmail.com
Motivation
The previous peer whitelist implementation was tightly coupled across
HathorManager,ConnectionsManager, andHathorSettings, mixing concerns and making it difficult to change the whitelist source (URL, local file) or policy at runtime. The whitelist was controlled by a globalENABLE_PEER_WHITELISTsetting with special-case logic for sync-v1 vs. sync-v2, and the peer ID list lived onHathorManager. This refactor extracts the whitelist into a self-contained, pluggable module (hathor.p2p.whitelist) that supports multiple backends (URL-based, file-based), configurable policies (allow-allvs.only-whitelisted-peers), runtime switching via sysctl, and a grace period for bootstrap peers before the first successful fetch completes. It also removes the now-unusedSyncVersion.V1_1variant and theENABLE_PEER_WHITELISTsetting.Acceptance Criteria
The whitelist is modeled as an abstract
PeersWhitelistbase class with two concrete implementations:URLPeersWhitelist(fetches from a remote URL) andFilePeersWhitelist(reads from a local file), both supporting periodic refresh with exponential backoff on failureWhitelist files support an optional
policy:directive that controls connection behavior:allow-allpermits any peer,only-whitelisted-peers(default) restricts connections to listed peer IDs onlyA factory function (
create_peers_whitelist) and CLI argument--x-p2p-whitelistallow selecting the whitelist source at startup:default/hathorlabs(uses settings URL),none/disabled(no whitelist), a file path, or a custom URLBefore the first successful whitelist fetch, a grace period allows only bootstrap-discovered peers to connect, preventing connections to arbitrary peers while the whitelist is still loading
The
ENABLE_PEER_WHITELISTsetting andSyncVersion.V1_1are removed; whitelist capability is now always required in the HELLO handshake, and the whitelist is enabled/disabled purely by whether aPeersWhitelistinstance is provided toConnectionsManagerWhitelist state is owned by
ConnectionsManager.peers_whitelistinstead ofHathorManager.peers_whitelist, andpeer_id.py's_is_peer_allowedsimply delegates to the whitelist objectThe sysctl interface exposes
whitelist(get/set the active whitelist source, or toggleon/offto suspend/resume) andwhitelist.status(returns structured state, policy, peer count, and source)When a new whitelist is set at runtime, peers not in the updated list are immediately disconnected
Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged