✅ hebasto closed a pull request: "test: Do not write Python bytecode to source directory"
(https://github.com/bitcoin/bitcoin/pull/30533)

👍 ryanofsky approved a pull request: "wallet: fix UnloadWallet thread safety assumptions"
(https://github.com/bitcoin/bitcoin/pull/30659#pullrequestreview-2240380401)
Code review ACK 7a73b0b7d5e690235bcefca9b82f01442b37ad5e. This is a much cleaner bugfix than I suggested in #29073. Moving the unload notification makes the shutdown sequence easier to understand, more efficient, and safe.

💬 ryanofsky commented on pull request "wallet: fix UnloadWallet thread safety assumptions":
(https://github.com/bitcoin/bitcoin/pull/30659#discussion_r1718396246)
In commit "wallet: rename ReleaseWallet to FlushAndDelete" (7a73b0b7d5e690235bcefca9b82f01442b37ad5e)

Not important, but maybe this should be called FlushAndDeleteWallet to be consistent with WaitForDeleteWallet.

💬 reverse-hash commented on issue "docs: Windows build intructions result in a large binary":
(https://github.com/bitcoin/bitcoin/issues/30593#issuecomment-2291280993)
Same issue when compiling 27.1 to build my own docker images.

```
make HOST=$(gcc -dumpmachine) NO_QT=1 NO_ZMQ=1 NO_WALLET=1 NO_BDB=1 NO_SQLITE=1 \
&& ./autogen.sh \
&& ./configure \
--prefix=$PWD/depends/$(gcc -dumpmachine) LDFLAGS="-static-libstdc++" \
--disable-bench \
--disable-cli \
--disable-debug \
--disable-man \
--disable-tests \
--disable-upnp \
--disable-wallet \
--disable-zmq \
--with-qrencode=no \
--enable-fuzz-binary
...

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718446549)
To be clear, you are suggesting extracting the repeated string literal "04678afdb0fe5548271967f1a67130b7105c..." into a `constexpr` variable?

💬 instagibbs commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2291335588)
is this too hard to make a boundary test?

💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2291393187)
> I still see a doubling in performance of the valid test if the validation cache is initialized with minimal values. Do you want to add it back?

Sure, done. Personally, I didn't care about the "valid" fuzz target much, but making it twice as fast can't hurt.

For reference, the flame graph for the "valid" fuzz target now shows that 80% of the time is spent in "ProcessNewBlockHeader", which I do not know how to avoid, and I don't think it is worth it. The goal for this pull request was to m
...

💬 furszy commented on pull request "wallet: fix UnloadWallet thread safety assumptions":
(https://github.com/bitcoin/bitcoin/pull/30659#discussion_r1718493802)
> nit: should this change be in its own commit? then you can use scripted diff for the rename.

Updated. Scripted both changes within the same commit.

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

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1718495189)
@glozow Good question, I've pushed a change that allows for perhaps a bit more accurate measurements (run the 5000 and 15000 iteration benchmark, and subtract). No other changes.

💬 achow101 commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1718502472)
It's 9.9G for me. Are you looking at the right network datadir?

💬 maflcko commented on pull request "coins: Add move operations to Coin and CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#discussion_r1718503311)
Missing `noexcept`, no?

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