Skip to content
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

net: bring internal clarity and consistency between IPv4 and IPv6 #9279

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bjacquin
Copy link
Contributor

@bjacquin bjacquin commented Apr 5, 2024

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: #8818

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 monero-project#8818.

* --p2p-bind-ip is replaced with --p2p-bind-ipv4-address
* --p2p-bind-port is replaced with --p2p-bind-port-ipv4
* --rpc-bind-ip is replaced with --rpc-bind-ipv4-address
* --rpc-restricted-bind-ip is replaced with --rpc-restricted-bind-ipv4-address

Bug: monero-project#8818
* --p2p-bind-port-ipv4 is replaced with --p2p-bind-ipv4-port
* --p2p-bind-port-ipv6 is replaced with --p2p-bind-ipv6-port
To ensure retro compatibility, legacy IPv4 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.

Raise warning in case legacy option are 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
Copy link
Contributor

@vtnerd vtnerd left a comment

Choose a reason for hiding this comment

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

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.

@bjacquin
Copy link
Contributor Author

bjacquin commented Apr 7, 2024

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.

@vtnerd
Copy link
Contributor

vtnerd commented Apr 7, 2024

@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).

@@ -62,5 +62,5 @@ EXPOSE 18081
USER monero

ENTRYPOINT ["monerod"]
CMD ["--p2p-bind-ip=0.0.0.0", "--p2p-bind-port=18080", "--rpc-bind-ip=0.0.0.0", "--rpc-bind-port=18081", "--non-interactive", "--confirm-external-bind"]
Copy link
Contributor

@vtnerd vtnerd Apr 7, 2024

Choose a reason for hiding this comment

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

The change in runtime flags is an almost certain no, as this requires everyone change their run scripts for minimal benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

4175d5b adds retro compatibility with such options

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

Successfully merging this pull request may close these issues.

None yet

3 participants