-
Notifications
You must be signed in to change notification settings - Fork 37.7k
doc: comment "add only reachable addresses to addrman" #26040
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
Conversation
ACK 9094951 |
9094951
to
a7b4f97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crACK a7b4f97
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK a7b4f97
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also mention the caveat in #26035, unless that's fixed.
I would hope "until it's fixed". :) |
a7b4f97
to
ce42570
Compare
Addressed additional review comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused about the broken checks, all I see is a doc string.
@@ -1648,6 +1648,14 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect) | |||
|
|||
if (add_fixed_seeds_now) { | |||
std::vector<CAddress> seed_addrs{ConvertSeeds(Params().FixedSeeds())}; | |||
// We will not make outgoing connections to peers that are unreachable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it wants:
/* Blah
* blah
* blah */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there are two random funcional tests failing in each failed CI run for some reason (different one in each case), should not be related with this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second one is new to me, I opened #26051 - but yes, obviously unrelated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK ce42570
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK ce42570
ACK ce42570 |
…drman" ce42570 doc: comment "add only reachable addresses to addrman" (Kristaps Kaupe) Pull request description: Proposed by Sjors during review of bitcoin#25678, was likely just missed, as it also for me looks a code where comment will not hurt. bitcoin#25678 (comment) ACKs for top commit: mzumsande: ACK ce42570 vasild: ACK ce42570 Zero-1729: re-ACK ce42570 Tree-SHA512: ef085d527349de07c1b43ed39e55e34b29cb0137c9509bd14a1af88206f7d4aa7dfec1dca53a9deaed67a2d0f32fa21e0b1a04d4d5d7f8a265dfab3b62bf8c54
Proposed by Sjors during review of #25678, was likely just missed, as it also for me looks a code where comment will not hurt.
#25678 (comment)