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

Thanks for picking this up!

📝 buerbaumer opened a pull request: "Adding security"
(https://github.com/bitcoin/bitcoin/pull/30662)
Adding two security enhancements. Please see the detailed description in the two commits below.
Thanks.

👋 pablomartin4btc's pull request is ready for review: "Bugfix - don't allow multiple dialogs for same tx in TransactionView"
(https://github.com/bitcoin-core/gui/pull/817)

💬 instagibbs commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2291572839)
I started considering adding a fuzzing harness for timewarp attacks(and mitigations). It would also rely on this type of PoW-checking avoidance.

concept ACK

✅ maflcko closed a pull request: "Adding security"
(https://github.com/bitcoin/bitcoin/pull/30662)

💬 maflcko commented on pull request "Adding security":
(https://github.com/bitcoin/bitcoin/pull/30662#issuecomment-2291573530)
Not sure about patching the legacy `autogen.sh`, which will be removed in a few days.

💬 TheBlueMatt commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2291575272)
> Makes sense to me, sanity checked you can sync with this. Is https://github.com/TheBlueMatt/bips/blob/7f9670b643b7c943a0cc6d2197d3eabe661050c2/bip-XXXX.mediawiki still the right place to see great consensus cleanup spec?

Uhh, I mean that's the right place for the original version, but I believe the new version(s?) live elsewhere. Dunno if there's a formal spec for it yet but haven't been following it super closely.

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