💬 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

🚀 achow101 merged a pull request: "Wallet: Zero out wallet master key upon locking so it doesn't persist in memory"
(https://github.com/bitcoin/bitcoin/pull/27080)

💬 ryanofsky commented on pull request "Add settings.json prune-prev, proxy-prev, onion-prev settings ":
(https://github.com/bitcoin-core/gui/pull/603#issuecomment-1428630595)
Just to summarize discussion above, I think that as long we believe it is useful to store disabled prune and proxy values, then `settings.json` is a better place to store them than QSettings, so this PR is implementing the right approach, and is ready for review/testing/merge.

If we think it's ok to reset disabled settings to default instead of saving them, then that is the current behavior, and this PR could just be closed. I'm beginning to think more that it is good to store disabled settin
...

💬 achow101 commented on pull request "wallet: SecureString to allow null characters":
(https://github.com/bitcoin/bitcoin/pull/27068#issuecomment-1428632058)
> > 3. 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. It's not _that_ far-fetched for a user to have used a null character in their password (e.g. through random password generator) so this will quite likely actually affect some users?

I highly doubt it. I would be surprised if any password manager produced a passphrase that included non-typeable chara
...