🤔 pablomartin4btc reviewed a pull request: "Feature: Use different datadirs for different signets"
(https://github.com/bitcoin/bitcoin/pull/29838#pullrequestreview-2160987527)
Concept ACK and light tACK 50f4ca2363e008aacb9f8a8aa1bb7abba04c0b33

Tried different challenges and custom `-datadir` option.

👍 tdb3 approved a pull request: "rpc: Use untranslated error strings in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/30395#pullrequestreview-2160988753)
ACK fa5b8920be041380fbfa4c7b443918637423d7a0

Re-ran check in https://github.com/bitcoin/bitcoin/pull/30395#discussion_r1666952404 . No extra period seen.
Ran unit/functional tests locally, including `feature_assumeutxo` (all passed).

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