Skip to content

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Mar 7, 2020

libevent uses getaddrinfo when available, and falls back to gethostbyname
Windows has both, but gethostbyname only supports IPv4
libevent fails to detect Windows's getaddrinfo due to not including the right headers
This patches libevent's configure script to check it correctly

Upstream issue: libevent/libevent#966

libevent uses getaddrinfo when available, and falls back to gethostbyname
Windows has both, but gethostbyname only supports IPv4
libevent fails to detect Windows's getaddrinfo due to not including the right headers
This patches libevent's configure script to check it correctly
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 7, 2020
libevent uses getaddrinfo when available, and falls back to gethostbyname
Windows has both, but gethostbyname only supports IPv4
libevent fails to detect Windows's getaddrinfo due to not including the right headers
This patches libevent's configure script to check it correctly

Github-Pull: bitcoin#18287
Rebased-From: e918138
@laanwj
Copy link
Member

laanwj commented Mar 11, 2020

Good catch. It's really sneaky that including a header can make this difference.

I think it'd be good to have a test for this case to detect regression.

@luke-jr
Copy link
Member Author

luke-jr commented Mar 11, 2020

Who maintains the Windows CI stuff? That's the only place I can see a test making sense...

@maflcko
Copy link
Member

maflcko commented Mar 11, 2020

How would a test case look like? Can't it be added as a normal unit or functional test?

@luke-jr
Copy link
Member Author

luke-jr commented Mar 11, 2020

I think just specifying -rpcbind=::1 would fail at startup. Does AppVeyor run the functional tests?

@maflcko
Copy link
Member

maflcko commented Mar 11, 2020

Appveyor uses msvc to compile, not sure if that changes anything. @sipsorcery

Though, we have a windows cross build that runs the unit tests:

export RUN_FUNCTIONAL_TESTS=false

@sipsorcery
Copy link
Contributor

I've just tested starting bitcoind built with msvc using:

..\src\bitcoind -rpcbind=::1 -rpcallowip=0.0.0.0/0 -debug

It failed with the relevant log messages being:

libevent: event_add: event: 000001F4D284AA20 (fd 1764), EV_READ    call 00007FF6DB2244A0
2020-03-11T18:48:42Z libevent: win32_add: adding event for 1764
2020-03-11T18:48:42Z Binding RPC on address ::1 port 8332
2020-03-11T18:48:42Z libevent: Detected an IPv4 interface
2020-03-11T18:48:42Z libevent: getaddrinfo: nodename nor servname provided, or not known
2020-03-11T18:48:42Z Binding RPC on address ::1 port 8332 failed.
2020-03-11T18:48:42Z Unable to bind any endpoint for RPC server
2020-03-11T18:48:42Z libevent: event_del: 000001F4D284AA20 (fd 1764), callback 00007FF6DB2244A0
2020-03-11T18:48:42Z libevent: win32_del: Removing event for 1764
2020-03-11T18:48:42Z libevent: event_base_free_: 0 events freed

This does appear to support Luke's diagnosis.

The patch won't solve the problem for msvc builds because the libevent dependency isn't installed with the Bitcoin Core depends scripts (it uses vcpkg in the .appveyor file).

If it's possible it would seem sensible to add a unit test that attempts to start bitcoind with the rpc listener on ::1. I can't recall if the appveyor vm's have IPv6 enabled or not, I'll check.

@DrahtBot
Copy link
Contributor

Gitian builds

File commit d20d5dc
(master)
commit fa9cd0d6e9abdb94cc80a7da33fc5a6ca3d07e00
(master and this pull)
bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz 73956d4944191255... 5153f27de9e1bdb0...
bitcoin-0.19.99-aarch64-linux-gnu.tar.gz fb88fd609c8c1b1c... 68d05a4e1726538a...
bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz b86040afa2b48137... 63f3136fd23b58cd...
bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz 21e193b0f898c969... 5fc8d06154eac9ac...
bitcoin-0.19.99-osx-unsigned.dmg a6be9f0646feef1c... d5b0f35b107edf84...
bitcoin-0.19.99-osx64.tar.gz 5a94089a3dbc5f3f... 6c38970879bed33c...
bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz c0716e8ef89e4725... 7b5144f87f4826e4...
bitcoin-0.19.99-riscv64-linux-gnu.tar.gz bdc5d76d88c0e8e6... 7e64bd3645fc7770...
bitcoin-0.19.99-win64-debug.zip 7d5c7682cd2045ac... b1e6eb754222ddfb...
bitcoin-0.19.99-win64-setup-unsigned.exe 4c39acb810bed5ec... f00f9502110f5d68...
bitcoin-0.19.99-win64.zip 997e3870a32aa396... bccc5740f929692a...
bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz d7558a4738ae2d5b... 2c0017a385697b18...
bitcoin-0.19.99-x86_64-linux-gnu.tar.gz c590269f431ae2c7... d8cc2df7c7b421ff...
bitcoin-0.19.99.tar.gz 00d9de8fe9c555a8... b021e8c901c7f711...
bitcoin-core-linux-0.20-res.yml 33781663516c3d60... a3d2a9b40285a29e...
bitcoin-core-osx-0.20-res.yml 7f84fbc82c109580... 7a7bd3ed3cd8765a...
bitcoin-core-win-0.20-res.yml a189b51cd52c859b... cd9dfedefa2a3678...
linux-build.log 9a596858a2901993... b4e5b0ee54840c22...
osx-build.log e57b1e14546f1b12... adf8a07151993479...
win-build.log 145b30399f10aeb9... 062b9ed339740d12...
bitcoin-core-linux-0.20-res.yml.diff 24d88d8542fd99fe...
bitcoin-core-osx-0.20-res.yml.diff 85aa4477ed2530e5...
bitcoin-core-win-0.20-res.yml.diff 901764e9f9fb15d1...
linux-build.log.diff 160ec3d126e7b5c3...
osx-build.log.diff 10cfa58da5449f7d...
win-build.log.diff 85d2d8eca1349b53...

@sipsorcery
Copy link
Contributor

@luke-jr I'm attempting to verify this fix but so far the Windows executable I produce still fails to bind the RPC port on ::1.

It looks like the patch is working insofar as if I do:

cd depends
make libevent

the output does indicate getaddrinfo is available.

checking for getservbyname... yes
checking for getaddrinfo... yes
checking for F_SETFD in fcntl.h... yes
checking for select... yes

I'll keep digging but if you know of any obvious things I should check I'd be happy to hear them.

@fanquake
Copy link
Member

This patch (part of libevent/libevent@6d54be2) looks ok and the inclusion of the additional headers definitely fixes libevents detection of getaddrinfo. Note that the same headers are also used here: https://github.com/libevent/libevent/blob/4c908dde58ef780eeefcc9df4db3063ca62ea862/include/event2/util.h#L65

However I have also tried to verify the fix, and after cross-compiling this PR and taking bitcoind.exe into a Windows 10 VM I cannot get it to run using the same command as @sipsorcery. i.e bitcoind.exe -rpcbind=::1 -rpcallowip=0.0.0.0/0 -debug.

Obviously there is an issue here, but at this point I don't think this can be a blocker for 0.20.0 unless someone can verify a fix.

@fanquake
Copy link
Member

Going to close this in favour of #19375, which includes this change.

@fanquake fanquake closed this Jun 25, 2020
@fanquake fanquake removed this from the 0.20.1 milestone Jun 25, 2020
laanwj added a commit that referenced this pull request Jul 1, 2020
…ipv6 usage

eb6b735 build: pass _WIN32_WINNT=0x0601 when building libevent for Windows (fanquake)
03e056e depends: Patch libevent build to fix IPv6 -rpcbind on Windows (Luke Dashjr)

Pull request description:

  TLDR: This poaches a commit from #18287 and adds one more to adjust the Windows version targeted when building libevent. These changes combined should fully fix ipv6 usage with the RPC server on Windows.

  ---

  Binding the RPC server to a ipv6 address does not currently work on Windows.
  We currently try and bind to `127.0.0.1` and `::1` [by default](https://github.com/bitcoin/bitcoin/blob/master/src/httpserver.cpp#L304).

  On Windows you'll see lines like this in debug.log:
  ```bash
  2020-06-24T01:49:04Z libevent: getaddrinfo: nodename nor servname provided, or not known
  2020-06-24T01:49:04Z Binding RPC on address ::1 port 8332 failed
  ```

  This issue was bought up in, and supposedly fixed by #18287, however the two people that tested it, both said that it didn't fix the problem. I think I now understand why that change alone is incomplete.

  Our call into libevent starts with [evhttp_bind_socket_with_handle()](https://github.com/bitcoin/bitcoin/blob/master/src/httpserver.cpp#L325):
  ```bash
  evhttp_bind_socket_with_handle()
  	bind_socket()
  		make_addrinfo()
  			evutil_getaddrinfo()
  				if #USE_NATIVE_GETADDRINFO
  					#ifndef AI_ADDRCONFIG
  						evutil_adjust_hints_for_addrconfig_()
  							evutil_check_interfaces()
  								evutil_check_ifaddrs()
  									evutil_found_ifaddr()
  										// miss identifies ipv6 as ipv4?
  					#endif
  					evutil_getaddrinfo_common_()
  ```

  The problem is falling into ["#ifndef AI_ADDRCONFIG"](https://github.com/libevent/libevent/blob/master/evutil.c#L1580):

  ```cpp
  #ifndef AI_ADDRCONFIG
  	/* Not every system has AI_ADDRCONFIG, so fake it. */
  	if (hints.ai_family == PF_UNSPEC &&
  	    (hints.ai_flags & EVUTIL_AI_ADDRCONFIG)) {
  		evutil_adjust_hints_for_addrconfig_(&hints);
  	}
  #endif
  ```

  When this occurs, hints end up being adjusted, and it seems that ipv6 addresses end up being mis-identified as ipv4?

  However this shouldn't happen, as these `AI_` definitions are available on Windows.
  The issue is that in evutil.c, `_WIN32_WINNT` [is set to `0x501`](https://github.com/libevent/libevent/blob/master/evutil.c#L45) (XP).

  This obviously predates Vista (`0x0600`), which is when the `AI_ADDRCONFIG` definition (and others) became [available](https://docs.microsoft.com/en-us/windows/win32/api/ws2def/ns-ws2def-addrinfoa).

  The change here will override libevents internal D_WIN32_WINNT defines. This should be ok, because it's only making "more" of the Windows API available. It's also aligned with what we do in our own configure, we pass [`D_WIN32_WINNT=0x0601`](https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L610). We also now use linker flags to restrict our binary from running on a Windows version [earlier than Windows 7](https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L621).

  The combined fixes can be tested by running:

  `bitcoind -rpcbind=::1 rpcallowip='0.0.0.0/0' -debug=http`

  and then querying it using:

  `bitcoin-cli -rpcconnect=::1 getblockchaininfo`

  TODO:
  - [x] Open an issue upstream. libevent/libevent#1041

ACKs for top commit:
  laanwj:
    ACK eb6b735

Tree-SHA512: e1e50f194911301981edaed0c216ed4efb9ebd4a1f9bc9b9f85bec7140b66c45c8666fd5db4aad359596559d4a08ab7c920e9d9736f3ecdbb841afc54e40586e
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants