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

🤔 murchandamus reviewed a pull request: "Move maximum timewarp attack threshold back to 600s from 7200s"
(https://github.com/bitcoin/bitcoin/pull/30647#pullrequestreview-2240965068)
crACK 16e95bda86302af20cfb314a2c0252256d01f750

💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718764031)
Fascinating. What is your compiler, stdlib version, OS and filesystem for `/tmp/` and `./`?

Does the issue happen when you create a non-`/tmp/` folder? (I presume `/tmp/` is a ramdisk?)

```
mkdir no_ramdisk_temp_prefix
./test/functional/test_runner.py --tmpdirprefix ./no_ramdisk_temp_prefix/ feature_blocksxor

💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718767576)
Also, what is `ls --all /tmp/bitcoin_func_test_fj812gkv/node0/regtest/blocks`

💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718769752)
Is it Apple https://en.wikipedia.org/wiki/.DS_Store ?