💬 MarcoFalke commented on pull request "ci: A few fixes of `ccache` issues":
(https://github.com/bitcoin/bitcoin/pull/27084#discussion_r1104757809)
I am still not sure what this means and which paths are preferred or discouraged

💬 john-moffett commented on pull request "wallet: SecureString to allow null characters":
(https://github.com/bitcoin/bitcoin/pull/27068#issuecomment-1428311028)
> Concept ACK, great catch indeed.

Thank you (and @furszy)!

>> Check to see if the wallet would decrypt using the substring up to the first null and only then emit errors (or even just warnings) like in 1 or 2.

> I think this is the best approach. ... Additionally, I think we could also encourage the user to re-encrypt the wallet with a different passphrase (without null char) for maximum backward compatibility, and perhaps even offer a one-click solution to do so in qt?

Sounds good,
...

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1104785968)
It has inside, `ReachableNets::m_mutex`.

💬 brunoerg commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1104803546)
In d9b95cb1eedb9f165df93dcae0aba96ede40767a, I could check that all CJDNS address starts with [OxFC](https://github.com/cjdelisle/cjdns/blob/master/doc/Whitepaper.md#pulling-it-all-together), so, it switches the first byte to `0x55` to avoid generating it that look like CJDNS. But why specifically `0x55`?

💬 brunoerg commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1104805263)
nit:
```suggestion
const bool reachable{g_reachable_nets.Contains(addr)};
```

💬 ryanofsky commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-1428374812)
re: https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-1399216846

> Not sure if related or not:

Yes, the IPC serialization code wasn't setting the `interfaces::FoundBlock.found` field, which caused wallets not to sync correctly after #26679. (The serialization bug present before #26679, but didn't have an effect until new code tried to use the missing field.)

---

Rebased 05e44e7fd41b32d57379bb6a87c4bebb4a166f9c -> 5e90c75de0d13aedb7e7c06ebe5aa55b11bb398d ([`pr/ipc.183`](https
...

💬 theuni commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#discussion_r1104819743)
Note that this ends up setting:
```
C compiler ............................ /usr/bin/cc
...
C++ compiler .......................... /usr/bin/c++
```
Which is different from what autotools would find. cc/c++ are usually symlinks to a valid compiler, but they don't always exist.

For ex, this would actually break my current workflow because my current llvm+clang toolchain did not ship with these links. So I'd see surprising results with a build like:
```
export PATH=/opt/clang+llvm-14.0.
...

💬 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
...

💬 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.

💬 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 ?

💬 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(
...

💬 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
...

💬 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.

💬 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.

💬 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
...

💬 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++
```

💬 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
...

💬 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 :)

💬 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.

💬 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?
...