💬 Sjors commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2291425839)
Concept ACK, but afaik our own mining needs to be updated to take this rule into account.

https://github.com/bitcoin/bitcoin/blob/2f7d9aec4d049701fccfc029f44934d187467432/src/node/miner.cpp#L31-L43

💬 maflcko commented on pull request "wallet: fix UnloadWallet thread safety assumptions":
(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.

💬 furszy commented on pull request "wallet: fix UnloadWallet thread safety assumptions":
(https://github.com/bitcoin/bitcoin/pull/30659#discussion_r1718520186)
I didn't think much about it. Just made https://github.com/bitcoin/bitcoin/pull/30659#discussion_r1718356981 happy while was tackling https://github.com/bitcoin/bitcoin/pull/30659#discussion_r1718396246. Either way is fine for me.

💬 furszy commented on pull request "wallet: fix UnloadWallet thread safety assumptions":
(https://github.com/bitcoin/bitcoin/pull/30659#discussion_r1718527166)
And.. reverted per https://github.com/bitcoin/bitcoin/pull/30659#discussion_r1718510823.

💬 furszy commented on pull request "wallet: fix UnloadWallet thread safety assumptions":
(https://github.com/bitcoin/bitcoin/pull/30659#discussion_r1718527539)
Done. Reverted.

💬 Sjors commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1718533770)
```
$ du -h ~/.bitcoin/testnet3
13G /home/sjors/.bitcoin/testnet3/chainstate
103G /home/sjors/.bitcoin/testnet3
```

💬 achow101 commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1718538977)
Hmm, that's odd. Across 3 of my nodes, it's 9.9G

```
$ du -sh ~/.bitcoin/testnet3/chainstate/ ~/.bitcoin/testnet3/blocks/
9.9G /home/ava/.bitcoin/testnet3/chainstate/
39G /home/ava/.bitcoin/testnet3/blocks/
```

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2291509848)
> > The test target is generated by CMake. Such targets [cannot](https://cmake.org/cmake/help/latest/command/add_dependencies.html) be subject to top-level target dependencies.
>
> Ok. So what about `deploy`? If it's a custom target of ours, it should know to build `bitcoin-qt` first?

Fixed in https://github.com/hebasto/bitcoin/pull/330.

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2291521586)
> The main change is commit b4b923565b4adaa5e3bcb22a6bc03f1f7ac4cdde "util: Add util::HexLiteral and util::Vec functions". The other commits have only minor changes.

Design note about b4b923565b4adaa5e3bcb22a6bc03f1f7ac4cdde: I spent hours yesterday trying many ways to implement `VectorFromHex(...)` and `ScriptFromHex(...)` hybrid compile/runtime functions that would be equivalent to `Vec(HexLiteral(...))` and `Script(HexLiteral(...)` in this commit and concluded it was impossible because:

...

📝 marcofleon opened a pull request: "fuzz: Test headers pre-sync through p2p"
(https://github.com/bitcoin/bitcoin/pull/30661)
This PR reopens https://github.com/bitcoin/bitcoin/pull/28043. It's a regression fuzz test for https://github.com/bitcoin/bitcoin/pull/26355 and [a couple bugs](https://github.com/bitcoin/bitcoin/pull/25717/commits/ed6cddd98e32263fc116a4380af6d66da20da990) that were addressed in https://github.com/bitcoin/bitcoin/pull/25717. This should help us move forward with the [removal of mainnet checkpoints](https://github.com/bitcoin/bitcoin/pull/25725).

It seems like the main concern in https://githu
...

🤔 pablomartin4btc reviewed a pull request: "Migrate legacy wallets that are not loaded"
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2240673487)
post-merge tACK 8f2522d242961ceb9e79672aa43e856863a1a6dd

Checked changes proposed by @furszy.

<details>
<summary>Perhaps the PR's description needs to be updated (or a note added) since the previous extra button to enter the passphrase was removed (<code>isEncrypted</code> is in place). Nice solution, less prone to errors too.</summary>


</details>

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