-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Resolve v6 addresses #8861
Resolve v6 addresses #8861
Conversation
Okay, clearly the way I'm loading config doesn't work for tests. This echo's back to my question in #8552 : Adding [the prefer_ipv6 preference] to the initial-config.yaml is clear, but is there code that already handles putting that into configs that don't have it, or is there a way that a default value is set in memory somehow if it's not in the config? Actually, it's a little different. Based on @hoffmang9 's comment in the discussion, I will set a default for this if it's not in the config. The question is, what if there is no config at all? It looks like the run-time tests have no chia config in the filesystem when these are running? |
and write it back to config.yaml
I could change the code in |
The tests do create a config.yaml file - if you look at create_block_tools() and the "BlockTools" class in block_tools.py. That creates a config.yaml in a temp directory You can also look at the simpler TestConfig class in test_config.py |
Looking at https://github.com/Chia-Network/chia-blockchain/runs/3929558844?check_suite_focus=true it seems many of the core CI runs are failing because they aren't able to load the config.yaml when they hit my new method. So, something must not be initialized yet. |
for it in a few places that call get_host_addr.
many times as it would in has_full_node.
It looks like the tests are failing The code seems to be using the ::1 IPv6 address for localhost, but perhaps the server is not listening properly for that. |
Thanks for that detail. I've been wondering how my change could've been causing these issues, but now it's clear. Let me know if you can confirm that the environment the tests are running in are listening IPv4-only, and can be changed. And let me know if you have any more feedback on the PR as it is. |
I can, one option is you can try switching the default to false and see if the tests pass. Meanwhile I'll see what I can dig up. I'm thinking one way to handle the default is to leverage Optional and None - so rather than specify the default in the various "get" calls, let the get return None, and then change get_host_addr is None is passed in that means True or False. One line then to switch the default. |
Certainly no third party IPv6 available on the runners. Lame... |
Wow. This is super lame. And, that says it was marked "wontfix" and closed 17 months ago. I'm very disappointed. |
Okay. And, I think this may also bring to light a concern I had about making "ipv6 preferred" the default. There will be many people, sadly, who don't have IPv6 working and will get burned in the same way as the test runners. This is among the reasons I wanted it to be in the on-disk config, to be easier to find and switch-flip. |
It's interesting however that the socket.getaddrinfo() call returns ::1 for localhost in this case - so the OS stack has IPv6 enabled? |
Yes. If you look at the ticket Gene referenced, you can see that the |
…e have only that one place where the coded default lives. also fix an oversight where we were building a PeerInfo from a PeerInfo in some cases.
…into resolve-v6
I'm not much of a python expert, but this is a case where I think the only reasonable option is change the parameter type to Optional[Any] |
All modern OSes have ipv6 enabled even if it can't get to outside addresses. |
Yeah. In which case, I'd expect chia's tests to be able to listen on ::1 and connect that way. The cancellation Earle found suggests that isn't working. Again, I'm not sure how key that is to pursue at this point. I changed the in-code default to False, so I hope that avoids this issue on githubs test runners. |
…py. It seems that github testers can't handle trying to use IPv6 and this may be easier for average users (sadly)
So, having "fixed" the code I added to always use IPv4, GitHub is okay with it. frown. But, this at least gives a config variable that people can use. |
I also have another concern. When I wrote this code on my Ubuntu system, I was wondering if Any recommendations? I can take that test out. I could try "localhost" with IPv6, except that it seems that doesn't always contain both family addresses, so would also fail a "localhost",True == "::1" test. |
I'd probably just remove it |
Hey @emlowe . Can you and others take a look at this, and pull it in if it meets requirements? |
…issues figuring out where/how they're defined. So, quote them here.
I'm pretty happy as it is now without adding a bunch more classes I'm not sure add much in terms of code correctness or readability. |
@emlowe can you get anyone else you want to review this attached, and then we can get it merged in? Thanks. |
I approved it, but we had some other more pressing issues, so it will likely wait until after the next release. It will get in though. |
The last test that failed failed to retrieve blocks from localhost, but I don't think my changes affect that flow. Let me know, but I'm assuming that updating my branch didn't introduce this, but that it's a testing infra oddity. |
I just kicked that workflow for you. |
wanted to merge but conflicts need to be resolved. sorry its taken so long! |
I assume: |
For the record on the above error... https://github.com/Chia-Network/chia-blockchain/runs/4268413400?check_suite_focus=true
But I don't think that's the interesting bit. https://gist.github.com/altendky/cdfcf9aae28e1449863008b4819466b7#file-gistfile1-txt-L2147-L2183
I started to address something maybe similar for another test over in #9206. I'll link here and note that this test should be reviewed as well. Rerunning the failed workflow now. |
Replace calls to
socket.gethostbyname
to a new resolver method inutil/network.py
that uses a config pref to return IPv4 or IPv6 addresses appropriately. Discussed in #8552