💬 maflcko commented on pull request "Adding security":
(https://github.com/bitcoin/bitcoin/pull/30662#issuecomment-2291583169)
For reference, the cmake replacement pull request is https://github.com/bitcoin/bitcoin/pull/30454 ; Hardening, testing and review of the replacement are very welcome.

💬 pablomartin4btc commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-2291587719)
Updates:
- No functional changes:
- Using GUIUtil::bringToFront() that was recently fixed (and merged) for `Wayland`;
- Removed unnecessary extra line in (`src/qt/transactiondescdialog.h`).

👍 ryanofsky approved a pull request: "wallet: fix UnloadWallet thread safety assumptions"
(https://github.com/bitcoin/bitcoin/pull/30659#pullrequestreview-2240667976)
Code review ACK f550a8e035b4603787273ea250f403f6f0be453f. Just a simple rename since last review

💬 ryanofsky commented on pull request "wallet: fix UnloadWallet thread safety assumptions":
(https://github.com/bitcoin/bitcoin/pull/30659#discussion_r1718594859)
re: https://github.com/bitcoin/bitcoin/pull/30659#discussion_r1718510823

> Can you explain why a four-line diff requires a five-line scripted diff? Seems easier to just have a normal diff when the script is longer than the diff.

Agree that a manual change is fine here, but IMO unless a scripted diff is badly written or fragile (e.g. hardcoding line numbers), I'd always prefer it over a manual change because it:

1. Documents and summarizes changes in a representation verified by CI.
2.
...

👍 ismaelsadeeq approved a pull request: "wallet: fix UnloadWallet thread safety assumptions"
(https://github.com/bitcoin/bitcoin/pull/30659#pullrequestreview-2240732475)
Re-ACK f550a8e035b4603787273ea250f403f6f0be453f

📝 buerbaumer opened a pull request: "3 more security enhancements"
(https://github.com/bitcoin/bitcoin/pull/30663)
Please see the detailed description in the commits below.

💬 maflcko commented on pull request "3 more security enhancements":
(https://github.com/bitcoin/bitcoin/pull/30663#discussion_r1718659330)
Please see my previous reply (https://github.com/bitcoin/bitcoin/pull/30662#issuecomment-2291573530)

The file will be deleted soon. Applying minor cosmetic changes or unspecified "hardening" to a file that will be deleted is pointless.

Only severe bugs can be fixed at this point. However, you will have to explain the bug and add exact steps to reproduce.

💬 brunoerg commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2291686784)
Concept ACK

💬 paplorinc commented on pull request "coins: Add move operations to Coin and CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#discussion_r1718683637)
Fixed, thanks!

💬 Sjors commented on pull request "guix: Drop unused module from manifest":
(https://github.com/bitcoin/bitcoin/pull/30653#issuecomment-2291697575)
```
170df52c2238510bd166f3fb1c4c3c11d2c1480a2e468fd532cb4d0435ac11cf guix-build-c7fb80a08f98/output/aarch64-linux-gnu/SHA256SUMS.part
54e71ef5135464f58e3db4a3b893fa2f26a2c9cfb465699a363bb59a0d1bd94f guix-build-c7fb80a08f98/output/aarch64-linux-gnu/bitcoin-c7fb80a08f98-aarch64-linux-gnu-debug.tar.gz
806d6042151e0af026748379b9bbbfea53d4c91555b2f0d05ed11faf83f429bb guix-build-c7fb80a08f98/output/aarch64-linux-gnu/bitcoin-c7fb80a08f98-aarch64-linux-gnu.tar.gz
96f111f81311b55c805f1ebe74c5a5bc3
...

💬 maflcko commented on pull request "wallet: fix UnloadWallet thread safety assumptions":
(https://github.com/bitcoin/bitcoin/pull/30659#discussion_r1718693413)
> IMO unless a scripted diff is badly written or fragile (e.g. hardcoding line numbers), I'd always prefer it over a manual change because it:

I understand why scripted-diffs are useful. However, you forgot to mention the risks and downsides:

* A script in the scripted-diff must be reviewed.
* Otherwise, it can accidentally or intentionally modify files that are not tracked by git (Which has happened in the past).
* Failing statements in the scripted-diff won't cause the CI to fail, whi
...

💬 glozow commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718700679)
I added a `if (opts.use_xor) assert(xor_key != decltype(xor_key){});` which hit, so not an old bitcoind. Attaching combined log.
```
node0 2024-08-15T16:40:41.093882Z [init] [node/blockstorage.cpp:1184] [InitBlocksdirXorKey] Using obfuscation key for blocksdir *.dat files (/tmp/bitcoin_func_test__c38k83u/node0/regtest/blocks): '0000000000000000'
```
[logs_30657.txt](https://github.com/user-attachments/files/16627687/logs_30657.txt)

💬 gmaxwell commented on pull request "3 more security enhancements":
(https://github.com/bitcoin/bitcoin/pull/30663#discussion_r1718707099)
This is a wrong change.

It's perfectly valid to have a PSBT with a "negative fee", -- as it may not yet have sufficient inputs to add up to the outputs. The code as is works correctly in this respect.

💬 brunoerg commented on pull request "3 more security enhancements":
(https://github.com/bitcoin/bitcoin/pull/30663#issuecomment-2291718315)
NACK

🤔 paplorinc reviewed a pull request: "3 more security enhancements"
(https://github.com/bitcoin/bitcoin/pull/30663#pullrequestreview-2240868725)
NACK 7bbdd5c30bcbd4125395f0adf74501b35831130c

💬 paplorinc commented on pull request "3 more security enhancements":
(https://github.com/bitcoin/bitcoin/pull/30663#discussion_r1718707933)
can you reproduce this with a test? If so, please add it so that we can validate that this is a valid concern and not just the result of a static analysis tool.

💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718716512)
> I added a `if (opts.use_xor) assert(xor_key != decltype(xor_key){});` which hit, so not an old bitcoind. Attaching combined log.

Can you also print/log/debug the result of `fs::is_empty(opts.blocks_dir)` and `fs::exists(xor_key_path)`?

✅ glozow closed a pull request: "3 more security enhancements"
(https://github.com/bitcoin/bitcoin/pull/30663)

💬 glozow commented on pull request "3 more security enhancements":
(https://github.com/bitcoin/bitcoin/pull/30663#issuecomment-2291725972)
Thanks for your interest in contributing. Please see the [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md).

Pull requests should be focused (there seem to be 4-5 changes here that don't relate to one another) and provide rationale in the commit messages. Also, please do not open duplicate pull requests.

Closing this due to lack of conceptual support.

💬 gmaxwell commented on pull request "3 more security enhancements":
(https://github.com/bitcoin/bitcoin/pull/30663#discussion_r1718719021)
Unnecessarily initialization can create performance problems when they happen on tight inner-loop code (though I didn't check if they do in this case).

More importantly, they can conceal serious flaws when something that must be initialized like a random seed is instead used with an unacceptable static initial value. Without the unnecessary initialization valgrind/msan will find these errors, but with it they won't.

In some cases it's possible to default initialize a value to a "safe valu
...