💬 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
...
💬 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
(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
(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)
(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
...
(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
...
(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
...
📝 Harshil-Jani opened a pull request: "Check for the awk script before executing it"
(https://github.com/bitcoin/bitcoin/pull/27095)
Hi ! I am new to the Bitcoin contributions. I was taking a look at the possible TODOs and I found a TODO in the file `ci/retry/retry` where, awk script was being used and we need to check if it already exist or not. This patch completes the TODO and would possibly be my very first contribution to the Bitcoin Protocol.
Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear
...
(https://github.com/bitcoin/bitcoin/pull/27095)
Hi ! I am new to the Bitcoin contributions. I was taking a look at the possible TODOs and I found a TODO in the file `ci/retry/retry` where, awk script was being used and we need to check if it already exist or not. This patch completes the TODO and would possibly be my very first contribution to the Bitcoin Protocol.
Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear
...
💬 achow101 commented on pull request "refactor: Move coin_control variable to test setup section":
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1428659487)
NACK
While this isn't incorrect, it's not meaningfully helpful. It doesn't really add anything useful to the codebase. The code as it is now is not broken nor necessarily wrong either.
Yes, it could be a problem if the test is removed, but it's unlikely that the test will be removed, and if it is removed, the line can be just as easily moved at that time.
PRs like this just end up being noise both in terms of open PRs, and in code churn. While this probably won't cause anything to need
...
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1428659487)
NACK
While this isn't incorrect, it's not meaningfully helpful. It doesn't really add anything useful to the codebase. The code as it is now is not broken nor necessarily wrong either.
Yes, it could be a problem if the test is removed, but it's unlikely that the test will be removed, and if it is removed, the line can be just as easily moved at that time.
PRs like this just end up being noise both in terms of open PRs, and in code churn. While this probably won't cause anything to need
...
💬 john-moffett commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1105010018)
While you're touching, maybe a good idea to change to `std::vector<unsigned char, secure_allocator<unsigned char>>`?
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1105010018)
While you're touching, maybe a good idea to change to `std::vector<unsigned char, secure_allocator<unsigned char>>`?
💬 sipa commented on pull request "Signing support for Miniscript Descriptors":
(https://github.com/bitcoin/bitcoin/pull/24149#discussion_r1105008572)
Given the need for all these dummy `SignatureData` objects that need to be created in calls, would it make sense to add another overload of `SignSignature` that has no `SignaturaData&` argument?
(https://github.com/bitcoin/bitcoin/pull/24149#discussion_r1105008572)
Given the need for all these dummy `SignatureData` objects that need to be created in calls, would it make sense to add another overload of `SignSignature` that has no `SignaturaData&` argument?
💬 mzumsande commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#issuecomment-1428684154)
[f5724d1 ](https://github.com/bitcoin/bitcoin/commit/f5724d18878c292e4cdc766ac93d28d4252dd157)to [080f83e](https://github.com/bitcoin/bitcoin/commit/080f83e43ddc1fd2ddc57f6590b9166a373d38c2):
I added a commit to change the make the RPC return `false` in the case of unavailable data and improved the error messages. I left `init` behavior unchanged in this case (no aborts).
(https://github.com/bitcoin/bitcoin/pull/25574#issuecomment-1428684154)
[f5724d1 ](https://github.com/bitcoin/bitcoin/commit/f5724d18878c292e4cdc766ac93d28d4252dd157)to [080f83e](https://github.com/bitcoin/bitcoin/commit/080f83e43ddc1fd2ddc57f6590b9166a373d38c2):
I added a commit to change the make the RPC return `false` in the case of unavailable data and improved the error messages. I left `init` behavior unchanged in this case (no aborts).
💬 ryanofsky commented on pull request "refactor, kernel: Remove gArgs accesses from dbwrapper and txdb":
(https://github.com/bitcoin/bitcoin/pull/25862#discussion_r1105021213)
re: https://github.com/bitcoin/bitcoin/pull/25862#discussion_r1087877059
In commit "refactor, validation: Add ChainstateManagerOpts db options" (cde3882cd4d4d1ee5682cab47d05e786cc798eb8)
> Not going against this change, but this line removal doesn't look to be part of a "non-behavior change" commit.
Good catch. It looks like this line got dropped due to a bad rebase. It was not supposed to be deleted.
(https://github.com/bitcoin/bitcoin/pull/25862#discussion_r1105021213)
re: https://github.com/bitcoin/bitcoin/pull/25862#discussion_r1087877059
In commit "refactor, validation: Add ChainstateManagerOpts db options" (cde3882cd4d4d1ee5682cab47d05e786cc798eb8)
> Not going against this change, but this line removal doesn't look to be part of a "non-behavior change" commit.
Good catch. It looks like this line got dropped due to a bad rebase. It was not supposed to be deleted.
💬 achow101 commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1428731690)
ACK 0bb90cabc8e2c6bbbc3c52ec1170e725435b625d
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1428731690)
ACK 0bb90cabc8e2c6bbbc3c52ec1170e725435b625d