💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717828979)
In commit "refactor: Add consteval ArrayFromHex and VecFromHex"
(b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795)

I think it would be safer and more explicit to use `static_cast<Byte>(...)` instead of functional cast `Byte(...)`. Not actually sure about this particular situation, but in general there are unsafe conversions that C casts allow which static_cast does not allow.

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717490226)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717352300

> In the PR summary you mean?

I do actually mean the commit message. The current commit message ddd06a0ec0a3b2c1d128fe5986a1363e7cf8e365 makes it sounds like it only switching to BOOST_CHECK_EQUAL_COLLECTIONS, but in reality is doing that and also making a seemingly unrelated test cleanup. Would suggest mentioning the other cleanup in the commit message, so for example, if someone is debugging something and wants to s
...

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717492122)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717426365

> Do we really need to support both?

It seems easier to support both than to add an error message explaining that the last byte has to be null, when there is no particular reason it needs to be null. But mainly I didn't think the last byte should be silently ignored, so at least that problem is fixed now.

🤔 ismaelsadeeq reviewed a pull request: "wallet: fix UnloadWallet thread safety assumptions"
(https://github.com/bitcoin/bitcoin/pull/30659#pullrequestreview-2240311191)
Code review ACK 7a73b0b7d5e690235bcefca9b82f01442b37ad5e

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

🤔 TheCharlatan reviewed a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2240344135)
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?

current HEAD:
```
#10000 DONE cov: 2794 ft: 3029 corp: 66/642b lim: 33 exec/s: 161 rss: 406Mb
Done 10000 runs in 62 second(s)
```
+ min validation cache:
```
#10000 DONE cov: 2788 ft: 3011 corp: 54/431b lim: 68 exec/s: 333 rss: 350Mb
Done 10000 runs in 30 second(s)
```

👍 ismaelsadeeq approved a pull request: "test: add functional test for XORed block/undo files (`-blocksxor` option)"
(https://github.com/bitcoin/bitcoin/pull/30657#pullrequestreview-2240361497)
Tested ACK faa1b9b0e6de7d213699fbdbc2e25a2a81c35cdc

💬 hebasto commented on pull request "test: Do not write Python bytecode to source directory":
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2291245181)
Closing until the migration to CMake is complete.

✅ 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?