fix: Only reply with mnauth to peers that are connected to the address registered in DML

[UdjinM6]

A node can be bound to multiple addresses. Send mnauth to nodes connected to the right address only to avoid confusion.

Thanks @kxcd 👍

[PastaPastaPasta]

I'm pretty sure I'd like some tests on this one please :)

[kxcd]

I can test it on testnet, just get me a build.

[UdjinM6]

I can test it on testnet, just get me a build.

You should be able to download it from CI build artefacts e.g. https://gitlab.com/dashpay/dash/-/jobs/2893007730/artifacts/browse/build-ci/dashcore-linux64/src/dashd for linux64-build

[kxcd]

404 on that link, I am guessing because the build failed, I will check back later.

[UdjinM6]

404 on that link, I am guessing because the build failed, I will check back later.

oh, sorry, my bad
results (../browse/..): https://gitlab.com/dashpay/dash/-/jobs/2893007730/artifacts/browse/build-ci/dashcore-linux64/src/
link to dashd (../file/..): https://gitlab.com/dashpay/dash/-/jobs/2893007730/artifacts/file/build-ci/dashcore-linux64/src/dashd

[MrDefacto]

What about nodes that uses local addresses to bind? Local IP that is translated 1:1 to public? There are certain DMZ setup or other cases when nodes are running like that.
There bind address doesn't match MN registered service IP.

EDIT: of course externalip is set to public ip

[kxcd]

My test will be on a node whose P2P is port forwarded into the VM, so I assume the dashd will bind to some private address eg, 10.x.y.z however, I believe it is still a condition that a Masternode specify the correct externalip= paramater to work and as such if this patch fails, maybe the comparison should be done against that property?

[PastaPastaPasta]

Just for clarity, I'm asking for functional tests

[kxcd]

I tested this fix first with my Masternode configured for TOR, I then checked on a node with onlynet=onion and another node without TOR, the result was that on the onion network, the the MN was not disclosed and on the plain net node, it was shown as a verified MN as expected. From the logs, the top IPs that were not the same as mine where.

Num   IP
914 185.220.100.243
856 185.220.100.255
732 185.220.100.241
707 185.220.100.253
705 185.220.100.252

and
358 ::
and a few IPv6 IPs. Are these IPs TOR node IPs?
I then re-ran the test with no masternode connected to TOR and checked the log and the message still prints, I got 294 hits for :: in about 30 mins. All my masternodes are connected on behind a NAT F/W and presumably bind to a private IP. I was not able to test on VM with a public IP at this time. Just one thing, the log message says 'differents' instead of 'different', otherwise this fix seems to work.

[thephez]

@PastaPastaPasta @UdjinM6 Are we planning to merge this at some point? It's still assigned to the 18.2 milestone

[PastaPastaPasta]

I'm not sure... I'd really like to see a functional test on this, but if we let it sit this long w/o adding a test we may have to wait a very long time before Udjin or I decide to write the test. That makes me inclined to be okay merging it. Any thoughts Udjin?

[UdjinM6]

I'm not sure how to make it testable cause local addresses aren't passed via version message, only routable ones are... Squashed and reworked the fix a bit - you can change https://github.com/dashpay/dash/pull/4974/files#diff-0f59ac68e534009ef1063c98c5df0b13d16771ad367cfae36850f5f2fc035d1fR34 to smth else (e.g. 127.0.0.2) and all mn-related tests should fail. That's probably the best option I have in my mind 🤷

EDIT: @kxcd could you test the new version on your nodes pls?

[github-actions]

This pull request has conflicts, please rebase.

[knst]

Is it still relevant? Can it be merged without tests?