monero icon indicating copy to clipboard operation
monero copied to clipboard

net: bring internal clarity and consistency between IPv4 and IPv6

Open bjacquin opened this issue 10 months ago • 4 comments

Modify all IPv4 variables, function arguments name and daemon arguments to IPv4 specific naming to raise consistency with IPv6. This change is done in order to make source code more legible before addressing #8818.

To ensure retro compatibility, daemon arguments and configuration settings changes are marked as deprecated, but can still be used falling back to new option name when new options are not used.

* --p2p-bind-ip is an alias to --p2p-bind-ipv4-address
* --p2p-bind-port is an alias to --p2p-bind-ipv4-port
* --p2p-bind-port-ipv6 is an alias to --p2p-bind-ipv6-port
* --rpc-bind-ip is an alias to --rpc-bind-ipv4-address
* --rpc-restricted-bind-ip is an alias to --rpc-restricted-bind-ipv4-address

Bug: https://github.com/monero-project/monero/issues/8818

bjacquin avatar Apr 05 '24 18:04 bjacquin

I don't think all of these naming changes are necessary. It certain makes the review a lot longer because I have to look hard to spot what was actually changed.

The change is made in order to raise consistency and legibility so that other changes made in this part of the code are easier to read for the developer. I've been preparing another PR (not published yet) making changes in this code part and I found myself in a lot of difficulty making such changes due to internal naming that is highly confusing and error prone. This PR will help future changes in the part of the code much easier to follow. This is long PR indeed, however I feel is a necessity to make this code more legible as we go.

bjacquin avatar Apr 07 '24 17:04 bjacquin

@moneromooo-monero

I think this something that we wouldn't accept historically, as it changes a lot of lines attributable to you, with minimal benefit. (I don't think the lack of ipv4 everywhere is confusing).

vtnerd avatar Apr 07 '24 18:04 vtnerd

I personally don't think the review and possible breakage is worth the name change. Honestly, to me, the naming is not confusing so long as you think of IPv4 as the "default" protocol.

jeffro256 avatar May 21 '24 15:05 jeffro256

@bjacquin thank you for submitting this PR.

Regarding this change, although I think it is a nice change. But I agree with @jeffro256 . And I think the convention we have is okay, the default flag is ipv4.

It can be a good idea to reach out to members of community, particularly if you want to spend time on specific PR.

0xFFFC0000 avatar Jun 05 '24 11:06 0xFFFC0000