addrmgr: Allow filtering by type, and remove DNS. #3409
Merged
Conversation
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
Prior to this fix, the calculation for numAddresses was a floor value. This resulted in sharing 0 addresses if less than 5 were known. Sharing 0 addresses is not good behavior when 1-4 are known, so the calculation has been changed to be a ceiling value. This results in always sharing at least one address if at least one is known.
matthawkins90
force-pushed
the
addrmgr_filter_and_dns
branch
from
1a6de05 to
93e9f55
Compare
davecgh
requested changes
Aug 2, 2024
This commit introduces a network address type filter that allows a caller to indicate the types of addresses that will be returned from the address manager. This allows finer control of what types of addresses may be broadcast to a particular peer. The filters are decided by the protocol version in a way that allows the address manager's internal implementation to remain agnostic of the protocol versioning logic.
matthawkins90
force-pushed
the
addrmgr_filter_and_dns
branch
from
93e9f55 to
8d458a7
Compare
davecgh
requested changes
Aug 6, 2024
Member
davecgh
left a comment
davecgh
left a comment
There was a problem hiding this comment.
Updates look good. Just a couple of nits left and I think this will be good to merge.
This removes the need to perform DNS lookups in the address manager. The direction forward is similar to that of BIP 155: addrv2. The address manager stores encoded versions of each address type, and doesn't need to know how to dial each type.
matthawkins90
force-pushed
the
addrmgr_filter_and_dns
branch
from
8d458a7 to
2ad6676
Compare
davecgh
approved these changes
Aug 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Similar to #3406, this work heavily references the work started in #2627, with the ultimate goal of adding support for TorV3 and other network address types.
The first commit fixes a minor bug where, if a node is asked to share addresses, but it only knows a very tiny number of addresses (less than five), it wouldn't share any. This isn't considered good behavior. I found this bug while trying to write tests for the second commit, where I added the ability to ask for addresses that match a certain filter. The bugfix is to always share at least one address (of the specified type) if any are known.
The second commit adds the ability to filter net addresses by specific address types. Eventually, this can be changed based on wire protocol versions which support different address types.
The third commit removes DNS from the address manager. With this proposed plan, the address manager will only be responsible for storing and managing addresses, encoded so that they're serialized compactly. The responsibility of dialing DNS has been moved to
server.go.