Skip to content

Conversation

@thelazier
Copy link

No description provided.

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

Nice find! 👍

Can confirm the issue, however not 100% happy with the solution, see below.

CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure)
{
if (pszDest == NULL) {
if (IsLocal(addrConnect))
Copy link

@UdjinM6 UdjinM6 Jan 21, 2018

Choose a reason for hiding this comment

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

This changes behavior for "normal" nodes too, should probably avoid that. I think passing and optional param (default to false) fConnectToMasternode to CConnman::ConnectNode and using it here would be a better solution.

@UdjinM6 UdjinM6 added this to the 12.3 milestone Jan 21, 2018
@codablock
Copy link

What about an alternative solution where ManageStateInitial calls OpenNetworkConnection directly with pszDest set to the string version of the address and fFeeler being true?

IMHO, using the CConnman for this at all has a bad taste. We want to only check if the port is accessible if I understand this correctly, so we should probably have a helper function that only does a call to ConnectSocket and immediately closes the socket again.

@UdjinM6
Copy link

UdjinM6 commented Jan 21, 2018

Agree, having a special helper would probably be an even better way.

@thelazier
Copy link
Author

Agree with @codablock

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants