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

📝 TheCharlatan opened a pull request: "test: Add arguments for creating a slimmer TestingSetup"
(https://github.com/bitcoin/bitcoin/pull/30399)
This adds arguments to some of the testing setup constructors for creating an environment without networking and a validation interface. This is useful for improving the performance of the utxo snapshot fuzz test, which constructs a new TestingSetup on each iteration.

Using this slimmed down `TestingSetup` in future might also make the tests a bit faster when run in aggregate.

✅ hebasto closed a pull request: "Rename doc/release-notes/release-notes-0.9.0.md to doc/release-notes/…"
(https://github.com/bitcoin/bitcoin/pull/30398)

📝 hebasto locked a pull request: "."
(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
...

💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2211455306)
> > **transaction_identifier.h** - Fixed dormant bug in `TxidFromString()` where the `string_view` length wasn't respected(!).
>
> This is known, see [#28922 (comment)](https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1404437378). Thanks for picking it up!
>
> Maybe submit the fix first?

Aha, good that you are tracking it! I see at least 4 possible fixes:

1. Switch back to `Txid TxidFromString(const std::string& str)`
2. Make `TxidFromString()` convert from `std::string_view
...

📝 hodlinator converted_to_draft a pull request: "refactor: Make uint256S(const char*) consteval"
(https://github.com/bitcoin/bitcoin/pull/30377)
`uint256S()` calls taking C-string literals are littered throughout the code and were executed at runtime to perform parsing unless a given optimizer was surprisingly efficient. While this may not be a hot spot, it's better hygiene *in C++20* to store the parsed data blob directly in the binary, without any parsing at runtime.

**transaction_identifier.h** - Fixed dormant bug in `TxidFromString()` where the `string_view` length wasn't respected(!).

Added runtime overload taking `std::string
...

⚠️ achow101 opened an issue: "Double lock detected in `Warnings::GetMessages()`"
(https://github.com/bitcoin/bitcoin/issues/30400)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Running `bitcoin-qt -testnet` results in it eventually deadlocking due to a double lock in the warnings code.

### Expected behaviour

Should not double lock.

### Steps to reproduce

1. Build with `--enable-debug` (or define `DEBUG_LOCKORDER`
2. Run `bitcoin-qt -testnet`, and either sync or reindex
3. It will assert and crash on its own (without `DEBUG_LOCKORDER`, it will eventually han
...

💬 achow101 commented on issue "Double lock detected in `Warnings::GetMessages()`":
(https://github.com/bitcoin/bitcoin/issues/30400#issuecomment-2211502915)
cc @stickies-v as you were the last to touch the warnings code.