Skip to content

Conversation

kristapsk
Copy link
Contributor

@kristapsk kristapsk commented Sep 7, 2022

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)

@DrahtBot DrahtBot added the Docs label Sep 7, 2022
@mzumsande
Copy link
Contributor

ACK 9094951

Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

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

crACK a7b4f97

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK a7b4f97

Copy link
Member

@Sjors Sjors left a 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.

@kristapsk
Copy link
Contributor Author

We should also mention the caveat in #26035, unless that's fixed.

I would hope "until it's fixed". :)

@kristapsk
Copy link
Contributor Author

Addressed additional review comments.

Copy link

@amovfx amovfx left a 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
Copy link
Member

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 */

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK ce42570

Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

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

re-ACK ce42570

@mzumsande
Copy link
Contributor

ACK ce42570

@maflcko maflcko merged commit ef5bb74 into bitcoin:master Sep 9, 2022
@kristapsk kristapsk deleted the onlynet_dns-comment branch September 9, 2022 15:34
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 11, 2022
…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
@bitcoin bitcoin locked and limited conversation to collaborators Sep 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants