test: Use zero compressed addresses for values in map #21960

pull shobitb wants to merge 1 commits into bitcoin:master from shobitb:test-cnetaddr-canonical changing 1 files +5 −5

shobitb commented at 7:19 AM on May 16, 2021: none

This change fixes a breaking test (cnetaddr_tostring_canonical_ipv6) in net_tests. The test uses a map "canonical_representations_ipv6" whose values are supposed to be zero-compressed IPv6 addresses, but a few values in the map were not zero-compressed. This caused the make check step in build-osx.md to fail on macOS 10.14. See logs --

test/net_tests.cpp:446: error: in "net_tests/cnetaddr_tostring_canonical_ipv6": check net_addr.ToString() == expected_canonical_representation_output has failed [2001:db8::1:1:1:1:1 != 2001:db8:0:1:1:1:1:1]
test/net_tests.cpp:446: error: in "net_tests/cnetaddr_tostring_canonical_ipv6": check net_addr.ToString() == expected_canonical_representation_output has failed [2001:db8::1:1:1:1:1 != 2001:db8:0:1:1:1:1:1]
test/net_tests.cpp:446: error: in "net_tests/cnetaddr_tostring_canonical_ipv6": check net_addr.ToString() == expected_canonical_representation_output has failed [2001:db8::1:1:1:1:1 != 2001:db8:0:1:1:1:1:1]
test/net_tests.cpp:446: error: in "net_tests/cnetaddr_tostring_canonical_ipv6": check net_addr.ToString() == expected_canonical_representation_output has failed [2001:db8:aaaa:bbbb:cccc:dddd::1 != 2001:db8:aaaa:bbbb:cccc:dddd:0:1]
test/net_tests.cpp:446: error: in "net_tests/cnetaddr_tostring_canonical_ipv6": check net_addr.ToString() == expected_canonical_representation_output has failed [2001:db8:aaaa:bbbb:cccc:dddd::1 != 2001:db8:aaaa:bbbb:cccc:dddd:0:1]
...
...

Leaving test module "Bitcoin Core Test Suite"; testing time: 75790us

*** 5 failures are detected in the test module "Bitcoin Core Test Suite"
make[3]: *** [test/net_tests.cpp.test] Error 1
make[2]: *** [check-am] Error 2
make[1]: *** [check-recursive] Error 1
make: *** [check-recursive] Error 1

Verified that make check completes successfully after this change.

test: Use zero compressed addresses for values in map

Without this change, the build-osx build instructions fail at the `make check` step (on macOS 10.14).

4bc97706ca

fanquake added the label Tests on May 16, 2021
MarcoFalke requested review from practicalswift on May 16, 2021
MarcoFalke commented at 7:39 AM on May 16, 2021: member

Might be another one of those issues where the tests assume too much of the system they run on? (#21682)
jonatack commented at 9:47 AM on May 16, 2021: member

Pinging @practicalswift, this test was committed on March 19, 2021 in 732c7bddeb9.
practicalswift commented at 4:32 PM on May 16, 2021: contributor
First, thanks a lot for reporting this @ssb402 :)

What you've found is the combination of two bugs: one bug in macOS and one bug in Bitcoin Core.
- macOS bug: In older versions of macOS Apple engineers simply got IPv6 zero compression wrong. More specifically they missed the sentence The symbol "::" MUST NOT be used to shorten just one 16-bit 0 field in RFC 5952. Luckily this issue has been resolved in later versions of macOS.
- Bitcoin Core bug: In Bitcoin Core we did the mistake of using the operating system for something cosmetic like formatting a IPv6 address that should not need any kind of interaction with the operating system. That unnecessarily exposed us to mistakes in code bases that are typically not as thoroughly reviewed as ours. This issue is fixed in #21756 which removes this unnecessary OS dependence. Review welcome! :)
The RFC (RFC 5952: "A Recommendation for IPv6 Address Text Representation") is very clear:
```
4.2.2.  Handling One 16-Bit 0 Field

   The symbol "::" MUST NOT be used to shorten just one 16-bit 0 field.
   For example, the representation 2001:db8:0:1:1:1:1:1 is correct, but
   2001:db8::1:1:1:1:1 is not correct.
```
Note that this is the exact test vector used in our unit test :)

There are three things you can do to help resolve this:
- Would you be willing to open an issue documenting this? That would be useful to point to in case other macOS users run in to the same problem.
- Would you be willing to apply the patch suggested in #21756 and tell if that solves the issue you're experiencing? (It should :))
- If possible, would you be willing to review #21756? :)
Again, thanks a lot for reporting issues: that's how Bitcoin Core gets harder, better, faster, stronger, etc :)
shobitb commented at 7:51 PM on May 16, 2021: none

Thanks @practicalswift for the review and explaining the details so clearly! I appreciate it.

Yes, I will review and use the patch from #21756 and see if it resolves my issue. Then I can create an issue documenting this and discard my PR.
shobitb commented at 3:19 AM on May 17, 2021: none

@practicalswift #21756 resolved my issue. Thanks! I will close this PR. Also, created an issue #21967 in case other macOS devs run into this problem.
shobitb closed this on May 17, 2021
shobitb deleted the branch on May 17, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Review Requested
practicalswift

Labels

Tests