Skip to content

addrmgr: Decouple address manager from wire.NetAddress #2596

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.

Sign up for GitHub

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

Jump to bottom
Merged
davecgh merged 5 commits into from
Sep 14, 2021
Merged

addrmgr: Decouple address manager from wire.NetAddress #2596

davecgh merged 5 commits into from
Sep 14, 2021

Conversation

@sefbkn
Copy link
Member

@sefbkn sefbkn commented Feb 18, 2021 •
edited
Loading

This change decouples the address manager's internal handling of network addresses to use a type owned by the address manager.

@sefbkn sefbkn force-pushed the decouple_addrmgr_from_wire branch 3 times, most recently from 6e4af32 to fb75e54 Compare February 18, 2021 08:50
dnldd
dnldd reviewed Feb 18, 2021
View reviewed changes
dnldd
dnldd reviewed Feb 18, 2021
View reviewed changes
@sefbkn sefbkn force-pushed the decouple_addrmgr_from_wire branch from fb75e54 to 4c04c7b Compare February 18, 2021 23:30
dnldd
dnldd reviewed Feb 19, 2021
View reviewed changes
dnldd
dnldd reviewed Feb 19, 2021
View reviewed changes
dnldd
dnldd reviewed Feb 19, 2021
View reviewed changes
dnldd
dnldd reviewed Feb 19, 2021
View reviewed changes
Copy link
Member

@dnldd dnldd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks good overall.

@davecgh davecgh added this to the 1.7.0 milestone Feb 19, 2021
rstaudt2
rstaudt2 reviewed Feb 21, 2021
View reviewed changes
Copy link
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see more test coverage being added as part of this decoupling!

@sefbkn
Copy link
Member Author

sefbkn commented Mar 2, 2021

Thanks for the feedback @dnldd @rstaudt2. I've not had the opportunity to wrap up applying the suggested changes yet, but will be able to shortly.

@sefbkn sefbkn force-pushed the decouple_addrmgr_from_wire branch from 4c04c7b to af86580 Compare March 11, 2021 10:32
@sefbkn
Copy link
Member Author

sefbkn commented Mar 12, 2021

Since there are changes in this PR that were not in the initial commits, I want to call out some high-level differences although most of the functional changes are the same plus a little more to fully decouple the addrmgr module. The aim was to help make reviewing it easier since a lot of changes were squashed into a single commit prevoiously

  • Added more detailed comments in the address manager
  • Remove some unnecessary tests and move others to netaddress_test.go for methods that moved to NetAddress
  • Split out the changes into more commits rather than squashing too many changes in one larger commit at the end
  • Completely decouple the address manager module from wire

@sefbkn sefbkn requested review from dnldd and rstaudt2 March 12, 2021 01:01
sefbkn
sefbkn commented Mar 12, 2021
View reviewed changes
dnldd
dnldd reviewed Mar 12, 2021
View reviewed changes
dnldd
dnldd reviewed Mar 12, 2021
View reviewed changes
rstaudt2
rstaudt2 reviewed Mar 12, 2021
View reviewed changes
Copy link
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates look good to me! I also ran ./run_tests.sh on each commit and tested various scenarios manually and did not see any issues.

@sefbkn sefbkn force-pushed the decouple_addrmgr_from_wire branch 3 times, most recently from e3378ea to c2034bf Compare March 14, 2021 20:32
@sefbkn sefbkn mentioned this pull request Mar 17, 2021
Open
davecgh
davecgh reviewed Mar 18, 2021
View reviewed changes
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this on! I'll be reviewing this in stages. First round of comments inline.

@sefbkn sefbkn force-pushed the decouple_addrmgr_from_wire branch 3 times, most recently from 36187b2 to 48de119 Compare March 20, 2021 19:40
@sefbkn sefbkn force-pushed the decouple_addrmgr_from_wire branch from 48de119 to 1a398c5 Compare March 20, 2021 20:06
dnldd
dnldd reviewed Mar 22, 2021
View reviewed changes
dnldd
dnldd approved these changes Mar 22, 2021
View reviewed changes
Copy link
Member

@dnldd dnldd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment related issues to fix but overall I think this is good to go.

@sefbkn sefbkn force-pushed the decouple_addrmgr_from_wire branch 2 times, most recently from 51404cf to edd4cea Compare April 4, 2021 02:55
davecgh
davecgh requested changes Apr 7, 2021
View reviewed changes
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've finished reviewing the remaining pieces. Very nice job overall! I've called out a few minor things inline, but overall the changes make sense and the increased test coverage is a welcome change. I also like how various methods like GroupKey are now defined on the new address type itself instead of being standalone funcs.

The only remaining thing that I think really isn't correct is the addition of the ServiceFlag type as called out inline.

Once these last bits are resolved, I'll do some live testing and barring any issues found there, this will be good to go from my viewpoint. Thanks for your work on this and to everyone who helped review.

@davecgh davecgh added the waiting for changes Pull requests that are waiting for changes from the submitter. label Apr 30, 2021
@sefbkn sefbkn force-pushed the decouple_addrmgr_from_wire branch from edd4cea to 7bf736c Compare September 14, 2021 08:37
@sefbkn sefbkn changed the title multi: Decouple address manager from wire.NetAddress addrmgr: Decouple address manager from wire.NetAddress Sep 14, 2021
@sefbkn
addrmgr: Improve test coverage.
940c92d 
This change increases test coverage within the address manager in
addition to improving comments throughout related code.

- Add tests for method ValidatePeerNa.
- Add tests for method HostToNetAddress.
- Add additional tests for methods Connected, Attempt, and Good.
- Improve coverage around local address handling.
- Improve coverage for internal behavior between tried and new buckets.
@sefbkn
addrmgr: Remove unnecessary test cases.
2a49087 
This commit removes unnecessary tests cases around the test helper
addAddressByIP. Since the values passed to this method are always
valid, there is no need for the error handling around it since that
results in code that serves to test a test helper.
@sefbkn
addrmgr: Decouple IP network checks from wire.
8b3b9d1 
This commit modifies the IP network checks to accept a net.IP rather
than a wire.NetAddress.
@sefbkn
addrmgr: Rename network address type.
ba5de8d 
This commit renames the address manager's type
NetworkAddress to NetAddressType and also introduces
a new NetAddressReach type.
@sefbkn sefbkn force-pushed the decouple_addrmgr_from_wire branch from 7bf736c to e121d36 Compare September 14, 2021 09:09
@sefbkn
addrmgr: Decouple addrmgr from wire NetAddress.
d11fb35 
This change removes the wire NetAddress type as a dependency of the address
manager module by introducing a new NetAddress type owned by the address
manager, in preparation for upcoming changes to the wire protocol.

- Introduce a new NetAddress type in the address manager.
- Modify KnownAddress struct to use address manager NetAddress.
- Replace DeserializeNetAddress with newNetAddressFromString.
@sefbkn sefbkn force-pushed the decouple_addrmgr_from_wire branch from e121d36 to d11fb35 Compare September 14, 2021 09:15
@sefbkn sefbkn mentioned this pull request Sep 14, 2021
Merged
@davecgh davecgh removed the waiting for changes Pull requests that are waiting for changes from the submitter. label Sep 14, 2021
davecgh
davecgh approved these changes Sep 14, 2021
View reviewed changes
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed all the updates. This looks good to go!

@davecgh davecgh merged commit 38c187f into decred:master Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davecgh davecgh davecgh approved these changes

+2 more reviewers

@dnldd dnldd dnldd approved these changes

@rstaudt2 rstaudt2 rstaudt2 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.7.0

Development

Successfully merging this pull request may close these issues.

4 participants

@sefbkn @dnldd @rstaudt2 @davecgh