Tolerate individual malformed ENRs in discv5#10941
Open
MysticRyuujin wants to merge 2 commits into
Open
Conversation
An out-of-range port in a peer ENR made getTcpAddress throw, and because convertToDiscoveryPeer is consumed via flatMap the exception aborted the whole peer-conversion batch. Catch it per record and skip only that record.
One un-decodable --p2p-discovery-bootnodes entry threw from the DiscV5Service constructor, preventing node startup. Parse each bootnode independently and skip the bad one.
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Two related robustness fixes so that a single malformed ENR cannot disrupt discv5 beyond that one record:
NodeRecordConverter.convertToDiscoveryPeer: address resolution (getTcpAddress/getQuicAddress) can throw on a record with an out-of-range port. Because the conversion is consumed viaflatMap(streamKnownPeers/convertToDiscoveryPeers), one throwing record aborted the entire peer-conversion batch. It now catches the failure per record and skips only that record.DiscV5Serviceconstructor: bootnodes were decoded with.map(NodeRecordFactory.DEFAULT::fromEnr), so one un-decodable--p2p-discovery-bootnodesentry threw and prevented node startup. Bootnodes are now parsed individually, skipping a bad entry.Found during a cross-client discv5 audit.
Testing
Unit tests added to
NodeRecordConverterTest(out-of-range port → skipped) andDiscV5ServiceTest(malformed bootnode → startup does not fail); both fail before the change and pass after.spotlessApply+:networking:p2p:testrun clean.Note
Low Risk
Localized defensive error handling in discovery; no auth, data, or protocol semantics changes beyond ignoring invalid ENRs.
Overview
Hardens discv5 so one bad ENR cannot take down startup or an entire peer batch.
Bootnodes:
--p2p-discovery-bootnodesentries are decoded via a newparseBootnodehelper that catches decode failures, logs a warning, and drops only that entry; valid bootnodes still load.Peer conversion:
NodeRecordConverter.convertToDiscoveryPeerwraps TCP/QUIC address resolution in a try/catch so records with unusable addresses (e.g. out-of-range ports) return empty instead of abortingstreamKnownPeers/convertToDiscoveryPeersflatMap batches.Unit tests cover malformed bootnode startup and out-of-range TCP port skipping.
Reviewed by Cursor Bugbot for commit 6c19079. Bugbot is set up for automated code reviews on this repo. Configure here.