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

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

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

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

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

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

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

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

💬 achow101 commented on pull request "Wallet: Zero out wallet master key upon locking so it doesn't persist in memory":
(https://github.com/bitcoin/bitcoin/pull/27080#issuecomment-1428607405)
ACK 3a11adc7004d21b3dfe028b190d83add31691c55

✳️ achow101 pushed commits to a branch: bitcoin/bitcoin:master
(https://github.com/bitcoin/bitcoin/compare/1ad0711d7c10...2c1fe27bf3c1)
Merge bitcoin/bitcoin#27080: Wallet: Zero out wallet master key upon locking so it doesn't persist in memory

3a11adc7004d21b3dfe028b190d83add31691c55 Zero out wallet master key upon lock (John Moffett)

Pull request description:

When an encrypted wallet is locked (for instance via the RPC `walletlock`), the documentation indicates that the key is removed from memory:

https://github.com/bitcoin/bitcoin/blob/b92d609fb25637ccda000e182da854d4b762eee9/src/wallet/rpc/encrypt.cpp#L157-L158

However, the vector (a `std::vector<unsigned char, secure_allocator<unsigned char>>`) is merely _cleared_. As it is a member variable, it also stays in scope as long as the wallet is loaded, preventing the secure allocator from deallocating. This allows the key to persist indefinitely in memory. I confirmed this behavior on my macOS machine by using an open-source third party memory inspector ("Bit Slicer"). I was able to find my wallet's master key in Bit Slicer after unlocking and re-locking my encrypted wallet. I then confirmed the key data was at the address in LLDB.

This PR manually fills the bytes with zeroes before calling `clear()` by using our `memory_cleanse` function, which is designed to prevent the compiler from optimizing it away. I confirmed that it does remove the data from memory on my machine upon locking.

Note: An alternative approach could be to call `vMasterKey.shrink_to_fit()` after the `clear()`, which would trigger the secure allocator's deallocation. However, `shrink_to_fit()` is not _guaranteed_ to actually change the vector's capacity, so I think it's unwise to rely on it.

## Edit: A little more clarity on why this is an improvement.

Since `mlock`ed memory is guaranteed not to be swapped to disk and our threat model doesn't consider a super-user monitoring the memory in realtime, why is this an improvement? Most importantly, consider hibernation. Even `mlock`ed memory may get written to disk. From the `mlock` [manpage](https://man7.org/linux/man-pages/man2/mlock.2.html):

> (But be aware that the suspend mode on laptops and some desktop computers will save a copy of the system's RAM to disk, regardless of memory locks.)

As far as I can tell, this is true of [Windows](https://web.archive.org/web/20190127110059/https://blogs.msdn.microsoft.com/oldnewthing/20140207-00/?p=1833#:~:text=%5BThere%20does%20not%20appear%20to%20be%20any%20guarantee%20that%20the%20memory%20won%27t%20be%20written%20to%20disk%20while%20locked.%20As%20you%20noted%2C%20the%20machine%20may%20be%20hibernated%2C%20or%20it%20may%20be%20running%20in%20a%20VM%20that%20gets%20snapshotted.%20%2DRaymond%5D) and macOS as well.

Therefore, a user with a strong OS password and a strong wallet passphrase could still have their keys stolen if a thief takes their (hibernated) machine and reads the permanent storage.

ACKs for top commit:
S3RK:
Code review ACK 3a11adc7004d21b3dfe028b190d83add31691c55
achow101:
ACK 3a11adc7004d21b3dfe028b190d83add31691c55

Tree-SHA512: c4e3dab452ad051da74855a13aa711892c9b34c43cc43a45a3b1688ab044e75d715b42843c229219761913b4861abccbcc8d5cb6ac54957d74f6e357f04e8730