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

💬 Sjors commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2291734952)
Here's a branch on top of this PR that add a test for the timewarp attack, and also fixes the miner.

Maybe for this PR we should only take the fix and leave the test for later, because it's quite slow.

https://github.com/bitcoin/bitcoin/compare/master...Sjors:bitcoin:2024/08/conervative-timewarp

💬 achow101 commented on pull request "wallet: fix UnloadWallet thread safety assumptions":
(https://github.com/bitcoin/bitcoin/pull/30659#issuecomment-2291745717)
ACK f550a8e035b4603787273ea250f403f6f0be453f

💬 sipa commented on pull request "3 more security enhancements":
(https://github.com/bitcoin/bitcoin/pull/30663#discussion_r1718742563)
This is true, but in this case actually irrelevant. Since every constructor of the class initializes all the fields, the change here literally has no effect; the initializers for fields are just defaults that get used when a constructor doesn't contain an explicit initialization.

💬 glozow commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718748117)
[logs_30657_2.txt](https://github.com/user-attachments/files/16627939/logs_30657_2.txt)

Ah interesting, `fs::is_empty(opts.blocks_dir)` is 0

🚀 achow101 merged a pull request: "wallet: fix UnloadWallet thread safety assumptions"
(https://github.com/bitcoin/bitcoin/pull/30659)

💬 maflcko commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2291808999)
> because it's quite slow

Possible ideas to speed it up could be to use the "real" testnet4 blocks for the first interval, similar to `./test/functional/data/blockheader_testnet3.hex`. If they are too large, one could "pre-mine" the first interval and recreate it deterministically in the test on each run, which should also be fast.