💬 fjahr commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#issuecomment-2211146256)
Rebased and addressed comments

> Nit: IMO, https://github.com/bitcoin/bitcoin/commit/64d35be20c43f764e5c22657085a790d0adcfe22 message should be amended to also include nTxCount and nTxDiff, since it's not just nChainTx that's changing to uint64_t.

Naming all the local variables that are touched isn't necessary I think but I added a comment in the commit description.

👍 stickies-v approved a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2161063572)
re-ACK 606a7ab862470413ced400aa68a94fd37c8ad3d3

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1667015153)
nit: having `DEFAULT_VALIDATION_CACHE_BYTES` and `DEFAULT_SCRIPT_EXECUTION_CACHE_BYTES` defined in `sigcache.h` is not ideal. One alternative is to rename `script/sigcache.h` to `script/cache.h` and move `ValidationCache` in it, but that touches quite a few lines so I'm not convinced that's the best thing to do for this PR to make progress.

👍 dergoegge approved a pull request: "psbt: Check non witness utxo outpoint early"
(https://github.com/bitcoin/bitcoin/pull/29855#pullrequestreview-2161086689)
utACK 9e13ccc50eec9d2efe0f472e6d50dc822df70d84

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2211245443)
> We've seen time and time again "just run this sidecar software next to Bitcoin Core" is not a thing that anyone does.

Yet another approach for said sidecar is to use IPC. It offers first class access to the node, e.g. via the newly added mining interface. It can probably automagically find a running Bitcoin Core process or spin one up. This would require more (review) progress in #10102 and additional tooling so external application can easily use IPC.

🤔 BrandonOdiwuor reviewed a pull request: "Fix cases of calls to `FillPSBT` errantly returning `complete=true`"
(https://github.com/bitcoin/bitcoin/pull/30357#pullrequestreview-2161119839)
Concept ACK

💬 fjahr commented on pull request "rpc: Use untranslated error strings in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/30395#issuecomment-2211301127)
re-ACK fa5b8920be041380fbfa4c7b443918637423d7a0

💬 fjahr commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + test":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2211315292)
tACK 5b7f70ba2661a194a3c476b81e03785feddb4e1c

Reviewed the code and confirmed the test fails without the behavior change.

💬 fjahr commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1667079909)
Can remove the TODO `Valid snapshot file and snapshot block, but the block is not on the most-work chain` here now as well.

💬 hodlinator commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#discussion_r1667076256)
Should template value be `<100>`?

💬 hodlinator commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#discussion_r1667073020)
`251438` seems to be a fine hook for manhole covers. :)
Worth documenting the choice of seed initialization value?

🤔 hodlinator reviewed a pull request: "random: add benchmarks and drop unnecessary Shuffle function"
(https://github.com/bitcoin/bitcoin/pull/30396#pullrequestreview-2161156126)
Is determinism of `std::shuffle` controlled/controllable? If we don't control it maybe it would be better to keep our custom one to ease reproducibility of test failures, despite it being slower.

💬 hodlinator commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#discussion_r1667077868)
`InsecureRandom_Shuffle100` combined with `<1000>` here as well.

👍 stickies-v approved a pull request: "build: Bump clang minimum supported version to 16"
(https://github.com/bitcoin/bitcoin/pull/30263#pullrequestreview-2161177627)
ACK fa8f53273c7e5965620d31a8c3fe5f223cb76888

My review shouldn't count for much as I'm not too familiar with build system stuff, but:
- I want clang-16
- it works for me locally
- it doesn't seem to break anything
- the changes look good

📝 hodlinator opened a pull request: "refactor: Use designated initializer in test/util/net.cpp"
(https://github.com/bitcoin/bitcoin/pull/30397)
Block was recently touched (e2d1f84858485650ff743753ffa5c679f210a992) and the codebase recently switched to C++20 which allows this to improve robustness.

Follow-up suggested in https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664818014

💬 hodlinator commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1667103367)
Follow-up: #30397

💬 TheBlueMatt commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2211372904)
I don't think the particular software design really impacts it nearly as much as people simply not wanting to deal with two or three pieces of software.

💬 sipa commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#discussion_r1667109704)
It's the alphabetic position of the letters of bench (2-5-14-3-8), but it's really just an arbitrary constant.

👍 marcofleon approved a pull request: "fuzz: fix key size in `crypter`"
(https://github.com/bitcoin/bitcoin/pull/30373#pullrequestreview-2161255501)
Tested ACK 4383dc90bac1b5def73352fe222f99807d8ca4dd. I ran this:

`FUZZ=crypter src/test/fuzz/fuzz -set_cover_merge=1 -shuffle=0 -prefer_small=1 -use_value_profile=0 ../qa-assets/fuzz_seed_corpus/crypter ../qa-assets/fuzz_seed_corpus/*`

It successfully merges in 1 attempt with no errors.

📝 Deuces9ers opened a pull request: "Rename doc/release-notes/release-notes-0.9.0.md to doc/release-notes/…"
(https://github.com/bitcoin/bitcoin/pull/30398)
…deuces9ers/release-notes-0.9.0.md

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test
...