💬 laanwj commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2435926822)
> It is not entirely clear whether it should be called after every RegQueryValueExA or if it's fine as-is, after the loop.

FWIW the idea that i got from reading that was that you should close the key when done with the API. Not at any specific point. This will unload the performance collection DLLs. The next time you query the key again they will be loaded again.

i concluded that we do the right thing (never close it) because, we're never done with it, we keep queryiing them time after tim
...

👍 laanwj approved a pull request: "util: Remove RandAddSeedPerfmon"
(https://github.com/bitcoin/bitcoin/pull/31124#pullrequestreview-2393333397)
Code review ACK 9bb92c0e7ffe2426b4afb80ddd4b4c96968316b5

📝 maflcko converted_to_draft a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061)
All translatable format strings are fixed. This change surfaces errors in them at compile-time.

The implementation achieves this by allowing to delay the translation (or `std::string` construction) that previously happened in `Untranslated()` or `_()` by returning a new type from those functions. The new type can be converted to `bilingual_str` where needed.

This can be tested by adding a format string error in an original string literal and observing a new compile-time failure.

💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1815481584)
I don't like this suggestion. If you disagree, you can continue discussion in https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1801093580

🚀 achow101 merged a pull request: "refactor: migrate `bool GetCoin` to return `optional<Coin>`"
(https://github.com/bitcoin/bitcoin/pull/30849)

💬 laanwj commented on pull request "RFC: build: support for pre-compiled headers.":
(https://github.com/bitcoin/bitcoin/pull/31053#issuecomment-2436025444)
Complete build on RPi5 with NVME hat, 8GB memory:
```
$ cmake -B build -G Ninja --preset=dev-mode -DWITH_CCACHE=OFF --toolchain /data/tmp/bitcoin/depends/aarch64-unknown-linux-gnu/toolchain.cmake
$ time ninja -C build
```

(PR with pch)
real 33m33.235s
user 127m24.712s
sys 4m22.753s

(base commit)
real 37m39.927s
user 144m17.888s
sys 3m26.133s

About 10% faster (in real time), not as impressive as some results on faster machines, but still nice to have.

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-2436046062)
Finished benching on a HDD until 860k on Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz, CPU 8:

Summary
'COMMIT=f278ca4ec3f0a90c285e640f1a270869ca594d20 ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=860000 -dbcache=10000 -printtoconsole=0' ran
1.02 times faster than 'COMMIT=e9e23b59f8eedb8dfae75aa660328299fba92b50 ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=860000 -dbcache=10000 -printtoconsole=
0'

f278ca4ec3 coins: allow emplacing no
...

👍 maflcko approved a pull request: "Introduce `g_fuzzing` global for fuzzing checks"
(https://github.com/bitcoin/bitcoin/pull/31093#pullrequestreview-2393422314)
lgtm

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-2436080216)
> So likely on HDD we shouldn't use so many threads, apparently it slows down IBD.

I'm not sure we can conclude that from your benchmark. It used a very high dbcache setting, which makes the effect of this change less important. It also is syncing from untrusted network peers, so there is some variance which could also account for the 2% difference.

🤔 nymius reviewed a pull request: "Refactor BnB tests"
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2393479212)
nit: I have read the test and I don't have any code related comment yet, but I have noticed that some of the commits are structured with some not written conventions and beside the actual comments about commit message quality in the `CONTRIBUTIONS.md` file, there are some other conventions used by other contributors too, in a similar fashion to [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/) with (`wallet:`, `ci:`, `rpc:`). In that spirit, wouldn't be a good idea to, in ca
...

✅ jonatack closed a pull request: "rpc, cli: return "verificationprogress" of 1 when up to date"
(https://github.com/bitcoin/bitcoin/pull/31135)

💬 jonatack commented on pull request "rpc, cli: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2436139570)
I agree with the approaches suggested by @sipa and @laanwj above and will be happy to review them. However, there are no concept ACKs and I am not confident of garnering sufficient ACKs if I implement those approaches here myself, so leaving it up for grabs.

📝 instagibbs opened a pull request: "functional test: Additional package evaluation coverage"
(https://github.com/bitcoin/bitcoin/pull/31152)
Current test coverage doesn't ensure that mempool trimming doesn't appear prior to the entire package, and not just the subpackage, is finished being submitted.

Add a scenario that covers this case, where package ancestors can make it in individually, but would be immadiately evicted if not for the package CPFP.

in response to https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813272637 where if applied onto that PR, the test fails due to package failure.

💬 maflcko commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2436158760)
It would be good to explain the jpegs in the description, or even remove them. They will be excluded from the merge commit and aren't shown, unless GitHub happens to be reachable and online. Are they saying that IBD was 4% faster? Also, I think they were created with the UB version of this pull, so may be outdated either way?

I did a quick check on my laptop and it seems the `XorSmall` (1+4 bytes) is slower with this pull. The `Xor` (modified to check 40 bytes) was twice as fast. Overall, I'd
...

💬 maflcko commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1815599531)
> Maybe this can be documented in `doc/developer-notes.md`?

(An alternative would be to add the CI configs to the cmake presets file, because the ISan is part of one. )

💬 maflcko commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1815601596)
Yes. At least for me locally, but I am getting widely different bench results anyway: https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2436158760

With this one reverted, `XorSmall` is back to being just slightly slower than current master for me.

💬 edilmedeiros commented on pull request "rpc, cli: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2436213630)
Maybe #31127 might be marked as Good First Issue?

👍 hodlinator approved a pull request: "tinyformat: enforce compile-time checks for format string literals"
(https://github.com/bitcoin/bitcoin/pull/31149#pullrequestreview-2393618400)
ACK 4cf12216951e91550c81db807fe0ecfafe6834c9

Only dropped commit I had [concerns about](https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2431257072). [l0rinc too](https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2389270316).

Does not entirely prevent throwing of exceptions from incorrect format strings (notably translated strings), but is a step in the right direction (validating more format strings at compile time).

💬 hodlinator commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2436261655)
> > Worth bringing back the try/catch in src/test/fuzz/strprintf.cpp for Debug builds?
>
> We should definitely be able to see easily what the invalid values were (and add unit tests for those cases)

(Not sure that would add much value, we know the fuzzing will produce mostly garbage format strings that will trigger errors).

💬 maflcko commented on pull request "doc: replace `-?` with `-h` and `-help`":
(https://github.com/bitcoin/bitcoin/pull/31118#issuecomment-2436264728)
lgtm ACK 33a28e252a7349c0aa284005aee97873b965fcfe