💬 theuni commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104782068)
This introduces the possibility of a departure from Autotools behavior, and is THE reason I was so opposed to switching to CMake for so long. After the "Autotools Hell" years, it mostly froze in time and (i'm oversimplifying) modules/features became locked-in. That means, in practice, you don't have to worry about whether autoconf's m4's are new enough for feature X.
CMake, however, takes us back in time in this regard. We now have to worry about its version and the feature-set that it provid
...
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104782068)
This introduces the possibility of a departure from Autotools behavior, and is THE reason I was so opposed to switching to CMake for so long. After the "Autotools Hell" years, it mostly froze in time and (i'm oversimplifying) modules/features became locked-in. That means, in practice, you don't have to worry about whether autoconf's m4's are new enough for feature X.
CMake, however, takes us back in time in this regard. We now have to worry about its version and the feature-set that it provid
...
💬 theuni commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104752652)
Why is c++20 turned on automatically for MSVC? Afaics autotools doesn't do that.
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104752652)
Why is c++20 turned on automatically for MSVC? Afaics autotools doesn't do that.
💬 theuni commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104821340)
Does this need CMP0083 ?
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104821340)
Does this need CMP0083 ?
💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1428383348)
> I think of `netbase` as a module for mostly context-free basic low-level methods - this PR moves `IsReachable` to it and saves its state in a global variable,
I was thinking about the same, but `netbase` is already stateful - `proxyInfo`, `nameProxy`, `nConnectTimeout`, `fNameLookup`, `g_socks5_recv_timeout`, `interruptSocks5Recv` are global variables that keep state, which influence the output of the functions defined in `netbase`, making them context-dependent.
`SetProxy()`, `GetProxy(
...
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1428383348)
> I think of `netbase` as a module for mostly context-free basic low-level methods - this PR moves `IsReachable` to it and saves its state in a global variable,
I was thinking about the same, but `netbase` is already stateful - `proxyInfo`, `nameProxy`, `nConnectTimeout`, `fNameLookup`, `g_socks5_recv_timeout`, `interruptSocks5Recv` are global variables that keep state, which influence the output of the functions defined in `netbase`, making them context-dependent.
`SetProxy()`, `GetProxy(
...
💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1104836123)
Oh, well. I feel like `MaybeFlipIPv6toCJDNS()` is warranted in some of the lower level `Lookup*()` functions. But they are called from so many places, the change would be very invasive. I have to gather some bravery in order to look into that. If that is to be done, maybe it should be preceded by some simplification of the `Lookup*()` family of functions. I get a headache every time I have to deal with:
```cpp
bool LookupHost(const std::string& name, std::vector<CNetAddr>& vIP, unsigned int
...
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1104836123)
Oh, well. I feel like `MaybeFlipIPv6toCJDNS()` is warranted in some of the lower level `Lookup*()` functions. But they are called from so many places, the change would be very invasive. I have to gather some bravery in order to look into that. If that is to be done, maybe it should be preceded by some simplification of the `Lookup*()` family of functions. I get a headache every time I have to deal with:
```cpp
bool LookupHost(const std::string& name, std::vector<CNetAddr>& vIP, unsigned int
...
💬 hebasto commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104841866)
> Why is c++20 turned on automatically for MSVC? Afaics autotools doesn't do that.
https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1101149891:
MSVC build configuration is defined in the `build_msvc` directory, not in `configure.ac`.
And C++20 for MSVC was introduced in 88ec5d40dcf5d9f95217b123b48203b2f334c0a1 from #25472.
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104841866)
> Why is c++20 turned on automatically for MSVC? Afaics autotools doesn't do that.
https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1101149891:
MSVC build configuration is defined in the `build_msvc` directory, not in `configure.ac`.
And C++20 for MSVC was introduced in 88ec5d40dcf5d9f95217b123b48203b2f334c0a1 from #25472.
💬 hebasto commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104844686)
https://cmake.org/cmake/help/latest/manual/cmake-policies.7.html:
> Policies in CMake are used to preserve backward compatible behavior across multiple releases.
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104844686)
https://cmake.org/cmake/help/latest/manual/cmake-policies.7.html:
> Policies in CMake are used to preserve backward compatible behavior across multiple releases.
💬 pinheadmz commented on pull request "wallet: reuse change dest when re-creating TX with avoidpartialspends":
(https://github.com/bitcoin/bitcoin/pull/27053#discussion_r1104850084)
Yeah great catch thanks, sorry I didn't notice that already. I added commit 80ad44d10ca0831869a5e50c36e07e7ff10902ec which includes a test for the edge case:
- user requests tx with specific `change_position`
- wallet creates default tx that does not require change (`change_pos` is overwritten with `-1`)
- wallet creates APS tx as second attempt, that does require change (change is placed in random position)
- APS tx is selected and returned, change position is wrong
Looking for feedbac
...
(https://github.com/bitcoin/bitcoin/pull/27053#discussion_r1104850084)
Yeah great catch thanks, sorry I didn't notice that already. I added commit 80ad44d10ca0831869a5e50c36e07e7ff10902ec which includes a test for the edge case:
- user requests tx with specific `change_position`
- wallet creates default tx that does not require change (`change_pos` is overwritten with `-1`)
- wallet creates APS tx as second attempt, that does require change (change is placed in random position)
- APS tx is selected and returned, change position is wrong
Looking for feedbac
...
💬 hebasto commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104852487)
> ```
> export PATH=/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin:$PATH
> cmake -S . -B ./build2
> ```
>
> Rather than picking up `/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang`, it finds `/usr/bin/cc`.
```
cmake -S . -B ./build2 \
-DCMAKE_C_COMPILER=/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang \
-DCMAKE_CXX_COMPILER=/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang++
```
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104852487)
> ```
> export PATH=/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin:$PATH
> cmake -S . -B ./build2
> ```
>
> Rather than picking up `/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang`, it finds `/usr/bin/cc`.
```
cmake -S . -B ./build2 \
-DCMAKE_C_COMPILER=/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang \
-DCMAKE_CXX_COMPILER=/opt/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang++
```
💬 mzumsande commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1428473660)
> I am not sure `IsReachable()` fits into that. It is not like that in `master` where there is the global variable `vfLimited[]`.
Yes, within `Node` it's still one of several global variables, so this is not a change wrt master.
However, `vfLimited` wasn't accessed by the GUI because it wasn't part of `LookupSubnet()` before, while `g_reachable_nets` is. So I'm curious if this would work well together with multiprocess (see #10102, fyi @ryanofsky ).
I think most of the data in `vfLimited` i
...
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1428473660)
> I am not sure `IsReachable()` fits into that. It is not like that in `master` where there is the global variable `vfLimited[]`.
Yes, within `Node` it's still one of several global variables, so this is not a change wrt master.
However, `vfLimited` wasn't accessed by the GUI because it wasn't part of `LookupSubnet()` before, while `g_reachable_nets` is. So I'm curious if this would work well together with multiprocess (see #10102, fyi @ryanofsky ).
I think most of the data in `vfLimited` i
...
💬 theuni commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104899261)
Thanks. I don't understand that change, but I'll take my complaints to that PR :)
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104899261)
Thanks. I don't understand that change, but I'll take my complaints to that PR :)
💬 theuni commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104903746)
Of course. I was simply pointing out that in this case, `./configure && make` wouldn't map to `cmake -S . -B ./build && make -C build` as one might expect. So it's a question of expectations of current build parity vs CMake-ness.
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104903746)
Of course. I was simply pointing out that in this case, `./configure && make` wouldn't map to `cmake -S . -B ./build && make -C build` as one might expect. So it's a question of expectations of current build parity vs CMake-ness.
💬 theuni commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104910467)
Understood for policies. But that's not to mean that it's not possible to misuse or misunderstand, which is why I'm suggesting the note. For example, one could imagine something naive/broken like:
```CMake
if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.20)
set(CMAKE_POSITION_INDEPENDENT_CODE ON)
endif()
```
which would provide better binaries for builders with newer CMake.
Maybe the note could say (if it's true?) that only `cmake_policy` should be set as a result of `CMAKE_VERSION` testing?
...
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104910467)
Understood for policies. But that's not to mean that it's not possible to misuse or misunderstand, which is why I'm suggesting the note. For example, one could imagine something naive/broken like:
```CMake
if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.20)
set(CMAKE_POSITION_INDEPENDENT_CODE ON)
endif()
```
which would provide better binaries for builders with newer CMake.
Maybe the note could say (if it's true?) that only `cmake_policy` should be set as a result of `CMAKE_VERSION` testing?
...
💬 hebasto commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1428512812)
Updated f9e1e7264f925690db74abd3efe7f1a3e10f2542 -> 75c4333e484ffea433f32b48ca7d4a1dc819bb36 ([pr27060.04](https://github.com/hebasto/bitcoin/commits/pr27060.04) -> [pr27060.05](https://github.com/hebasto/bitcoin/commits/pr27060.05), [diff](https://github.com/hebasto/bitcoin/compare/pr27060.04..pr27060.05)):
- addressed @theuni's [comment](https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104821340)
(https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1428512812)
Updated f9e1e7264f925690db74abd3efe7f1a3e10f2542 -> 75c4333e484ffea433f32b48ca7d4a1dc819bb36 ([pr27060.04](https://github.com/hebasto/bitcoin/commits/pr27060.04) -> [pr27060.05](https://github.com/hebasto/bitcoin/commits/pr27060.05), [diff](https://github.com/hebasto/bitcoin/compare/pr27060.04..pr27060.05)):
- addressed @theuni's [comment](https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104821340)
💬 hebasto commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104914724)
Thanks! [Fixed](https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1428512812).
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104914724)
Thanks! [Fixed](https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1428512812).
💬 theuni commented on pull request "build: Increase MS Visual Studio minimum version":
(https://github.com/bitcoin/bitcoin/pull/25472#issuecomment-1428515119)
I'm confused about this PR. If I'm reading correctly, MSVC now gets built as c++20 while other platforms are c++17. Seemingly so that we could drop our designated initializer code because it's non-standard for c++17.
That seems like a huge hammer for a small problem. I suspect I'm missing something?
(https://github.com/bitcoin/bitcoin/pull/25472#issuecomment-1428515119)
I'm confused about this PR. If I'm reading correctly, MSVC now gets built as c++20 while other platforms are c++17. Seemingly so that we could drop our designated initializer code because it's non-standard for c++17.
That seems like a huge hammer for a small problem. I suspect I'm missing something?
💬 mzumsande commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#issuecomment-1428518885)
I was busy with other things in the last weeks, but I will get back to address feedback later this week!
(https://github.com/bitcoin/bitcoin/pull/24170#issuecomment-1428518885)
I was busy with other things in the last weeks, but I will get back to address feedback later this week!
💬 hebasto commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104918952)
It originates from #24531.
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104918952)
It originates from #24531.
💬 hebasto commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104921040)
> But IMO we need a note to the effect of: "Changes which affect binary results may _not_ be quietly gated by CMake version".
Ah, you mean a comment in CMAke's code, right?
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104921040)
> But IMO we need a note to the effect of: "Changes which affect binary results may _not_ be quietly gated by CMake version".
Ah, you mean a comment in CMAke's code, right?
💬 hebasto commented on pull request "build: Increase MS Visual Studio minimum version":
(https://github.com/bitcoin/bitcoin/pull/25472#issuecomment-1428551425)
> I'm confused about this PR. If I'm reading correctly, MSVC now gets built as c++20 while other platforms are c++17. Seemingly so that we could drop our designated initializer code because it's non-standard for c++17.
>
> That seems like a huge hammer for a small problem. I suspect I'm missing something?
I took a liberty to cross [reference](https://github.com/bitcoin/bitcoin/pull/24531#issue-1166402005):
> Designated initializers are supported since gcc 4.7 (Our minimum required is 8) a
...
(https://github.com/bitcoin/bitcoin/pull/25472#issuecomment-1428551425)
> I'm confused about this PR. If I'm reading correctly, MSVC now gets built as c++20 while other platforms are c++17. Seemingly so that we could drop our designated initializer code because it's non-standard for c++17.
>
> That seems like a huge hammer for a small problem. I suspect I'm missing something?
I took a liberty to cross [reference](https://github.com/bitcoin/bitcoin/pull/24531#issue-1166402005):
> Designated initializers are supported since gcc 4.7 (Our minimum required is 8) a
...