Regression: CNetAddr scoped id/zone indices functionality

jonatack commented at 12:20 pm on May 17, 2021: member

It’s not clear to me if dropping support for this functionality was intentional.

Chain of events:

#21689 removed test coverage due to a misunderstanding due, I believe, to this functionality being generally supported by all platforms except macOS which does not and returns a different format
#21756 removed support and caused this regression
#21690 restores the test coverage that would have caught this regression

Currently, neither the test coverage before #21689 nor the coverage restored by #21690 pass since the merge of #21756.

jonatack added the label Bug on May 17, 2021

practicalswift commented at 12:38 pm on May 17, 2021: contributor

My question from the issue/PR referenced above:

And perhaps more fundamentally: why would we even want to have %zone_index in our textual ToString() output? I think the expectation is to get say fe80::1ff:fe23:4567:890a (without zone index) and not say fe80::1ff:fe23:4567:890a%eth2 or fe80::1ff:fe23:4567:890a%3 when doing ipv6_addr.ToString() :)

Why would we want to include %zone_id in CNetAddr::ToString() output? :)

What “functionality” are you current missing? :)

jonatack commented at 12:42 pm on May 17, 2021: member

The functionality in those tests or CNetAddr::m_scope_id:

0        /**
1         * Scope id if scoped/link-local IPV6 address.
2         * See https://tools.ietf.org/html/rfc4007
3         */
4        uint32_t m_scope_id{0};

Why remove it? Was it removed intentionally?

jonatack commented at 12:47 pm on May 17, 2021: member

I added this test coverage precisely to cover this code. If we don’t want it, I’ll remove this from the codebase along with the rest of the test coverage.

practicalswift commented at 12:54 pm on May 17, 2021: contributor

Perhaps you can think of a way to test what you want to test without relying on how CNetAddr::ToString() formats IP-addresses as strings?

As I’ve said numerous times: I cannot see any reason to include %zone_index in CNetAddr::ToString() output, and I don’t think that was ever the intention to include it historically: it just got to be that way due to our previous unfortunate OS dependence when formatting IP addresses. On some platforms %zone_index was included and one some it did not.

As you know on macOS we did not print %zone_index when doing CNetAddr::ToString(). Has that caused any problems that I’m missing?

I could be missing some use case so that’s why I’m asking you explicitly: setting aside your testing needs (which can be solved by not using CNetAddr::ToString() in the test), can you think of a single use case where one would like to have %zone_index being part of CNetAddr::ToString() output?

jonatack commented at 1:08 pm on May 17, 2021: member

Did you drop the functionality intentionally?

practicalswift commented at 1:29 pm on May 17, 2021: contributor

@jonatack

Did you drop the functionality intentionally?

Again: I’m not sure what “functionality” you’re referring to:

If you by “functionality” mean not appending an unwanted %zone_index string to CNetAddr::ToString(), then yes that non-appending of an unwanted string is obviously intentional. The new implementation of IPv6ToString (which is called from CNetAddr::ToString) follows Rust’s std::net::Ipv6Addr which does not append %zone_index. FWIW that is how the previous implementation of CNetAddr::ToString worked under macOS.
If you by “functionality” mean anything else, then no.

FWIW this is what I said back in April 14 (https://github.com/bitcoin/bitcoin/issues/21682#issuecomment-819897122) about this:

1.) I don’t understand why we test if ToString() output includes %zone_index: it clearly doesn’t on some platforms, so we cannot rely on it anyways. Then why test it?

2.) And perhaps more fundamentally: why would we even want to have %zone_index in our textual ToString() output? I think the expectation is to get say fe80::1ff:fe23:4567:890a (without zone index) and not say fe80::1ff:fe23:4567:890a%eth2 or fe80::1ff:fe23:4567:890a%3 when doing ipv6_addr.ToString() :)

It seems clear that you think something should be changed: feel free to open a PR making the changes you wish to see.

jonatack commented at 1:30 pm on May 17, 2021: member

Before the test coverage was removed, it demonstrated that we supported the expected per-platform functionality according to these resources:

I don’t presume to know if no one used it and if we can safely drop coverage willy-nilly and then functionality. I only tried to have test coverage and some insight on expected behavior. Maybe someone else knows if it’s prudent to remove this.

jonatack commented at 1:31 pm on May 17, 2021: member

FWIW that is how the previous implementation of IPv6ToString worked under macOS.

Yes, macOS did not support it, so that was expected.

practicalswift commented at 1:32 pm on May 17, 2021: contributor

FWIW that is how the previous implementation of IPv6ToString worked under macOS.

Yes, macOS did not support it, so that was expected.

Support what?

jonatack commented at 1:41 pm on May 17, 2021: member

The functionality was added in eda3d9248971a1c3df by @mruddy.

jonatack commented at 1:44 pm on May 17, 2021: member

#7570: “Adds support for binding and listening, as well as connecting to, scoped IPv6 addresses. The motivation for this change is to make it easier to deploy and manage a cluster of nodes on a local network. For example, I was spinning up multiple nodes on an adhoc WIFI network for a simulation and IPv6 link-local addressing made that process easier by a few steps for each node.”

practicalswift commented at 1:46 pm on May 17, 2021: contributor

@jonatack

Assuming you’re talking about IPv6 address text formatting which is the only thing changed:

Personally I think Rust’s std::net::Ipv6Addr does the right thing in this regard.

The new IPv6ToString is largely following Rust’s std::net::Ipv6Addr.

If you think that is a bad decision then feel free to open a PR correcting what you feel is currently broken.

jonatack commented at 2:37 pm on May 17, 2021: member

I don’t plan to look more into it, unless someone is affected or cares about it.

laanwj referenced this in commit bf955dc59b on May 17, 2021

laanwj referenced this in commit 81bd71c9fa on May 17, 2021

laanwj referenced this in commit 90972a8962 on May 17, 2021

laanwj referenced this in commit c7d461b014 on May 17, 2021

laanwj referenced this in commit 6c280adcd8 on May 18, 2021

laanwj closed this on May 19, 2021

laanwj referenced this in commit 2fc111b6e3 on May 19, 2021

sidhujag referenced this in commit b2255c1f29 on May 19, 2021

rebroad referenced this in commit 168702607b on Jun 23, 2021

DrahtBot locked this on Aug 18, 2022

Regression: CNetAddr scoped id/zone indices functionality #21982