👍 luke-jr approved a pull request: "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin"
(https://github.com/bitcoin/bitcoin/pull/30326#pullrequestreview-2161585202)
utACK 00052ecc0402f2c53e3602b901a937b64aa7f7cc

There's a potential race condition for negative lookups, but I assume it's fine since the old code would also have a (different) race.

Performance might be slightly worse for negative lookups, but that seems an acceptable tradeoff since the positive lookup path is much more likely, especially during IBD.

🤔 hodlinator reviewed a pull request: "random: add benchmarks and drop unnecessary Shuffle function"
(https://github.com/bitcoin/bitcoin/pull/30396#pullrequestreview-2161613367)
Ran benchmarks on da28a26aae3178fb7663efbe20bb650857ace775 with..

### Clang, non-debug, Linux

```console
$ src/bench/bench_bitcoin --filter=".*Random.*"

| ns/number | number/s | err% | ins/number | cyc/number | IPC | bra/number | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 12.84 | 77,8
...

💬 sipa commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2211965086)
@hodlinator FWIW, my motivation here is just FastRandomContext non-debug performance. I did notice that `std::shuffle` is a bit slower than the custom Shuffle on my system too (AMD 5950X, GCC 13.2) , but not the huge (5x-10x) slowdown it once was. With 2% or 26% or even 50% slowdown, I don't think it's worth keeping a custom function around (I don't think any of the `Shuffle` uses are *that* performance critical). Things like `randbool` and `randrange` are probably more relevant overall.

I do
...

📝 achow101 opened a pull request: "Use `WITH_LOCK` in `Warnings::Set`"
(https://github.com/bitcoin/bitcoin/pull/30404)
The scope of the lock should be limited to just guarding m_warnings as anything listening on `NotifyAlertChanged` may execute code that requires the lock as well.

Fixes #30400

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667444775)
In commit "refactor: encapsulate flags setting with AddFlags and ClearFlags"

I don't think this test helps; testing a value is likely slower than just overwriting it. Testing with godbolt does seem like it doesn't get optimized out, which surprises me a bit.

EDIT: I see this matters in a future commit. Feel free to ignore, or move the conditional there.

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667444981)
In commit "refactor: require self and sentinel parameters for AddFlags"

This seems like a strange name for a getter for the sentinel?

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667488129)
If I can suggest a slightly different linked-list design, which would avoid some conditionals.

A doubly-linked, circular, list. The sentinel is both the first and the last element of the list of flagged entries. Every non-flagged entry is also a linked list, containing just itself. In this case, adding/removing from the flagged list is just concatenating/splicing linked lists, which can be done without conditions/edge cases.

```c++
/** Insert this after other. */
void InsertAfter(CCoinsC
...

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667445032)
In commit "refactor: require self and sentinel parameters for AddFlags"

Perhaps also use the name sentinel here? I see you're changing the name in a later commit, but it would be cleaner to do it here.

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667499414)
Actually, even simpler. By delaying the setting of `m_next = this` and `m_prev = this` to just before insertion, inlining that, and not bothering with getting `m_next` and `m_prev` correct for taken-out elements:

```c++
void InsertAfter(CCoinsCacheEntry* other) noexcept
{
m_prev = other->m_next->m_prev;
other->m_next->m_prev = this;
m_next = other->m_next;
other->m_next = this;
}

void InsertBefore(CCoinsCacheEntry* other) noexcept
{
m_next = other->m_prev->m_ne
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667501865)
Yeah I thought about doing a self-referencing sentinel, but for iteration then in BatchWrite we would have to pass the sentinel and do something like
```C++
for (auto it{sentinel.Next()}; &it->second != &sentinel; it = it->Next()) {
```
I thought just passing a pointer of the first item and checking `it != nullptr` was cleaner. But I can update to do it this way too.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667502352)
Also, `m_prev` and `m_next` would have to be `CoinsCachePair`s, not `CCoinsCacheEntry`s, so we can't set the pointers in the constructor since we don't have a pair.

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667502681)
No need for anything in the constructor with my latest suggestion.

💬 petertodd commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2212087833)
> Concept NACK. This seems to encourage and promote spam. We should be doing the exact opposite.

This proposal is merely a way of doing something that is already done - signed anchor outputs - in a more efficient way with less overall bytes consumed. The creating and spending of keyless anchor outputs is completely fixed, with no ability to encode any data. It has nothing to do with spam.

> Curious why you say "creation was already standard" - I don't see why that would be the case.

Be
...

📝 zxramozx opened a pull request: "Patch 4"
(https://github.com/bitcoin/bitcoin/pull/30405)
`d0db9ca047f8575cd5b344c16f39dad875c8eefe<!--`
*** 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
...

🤔 zxramozx reviewed a pull request: "Patch 4"
(https://github.com/bitcoin/bitcoin/pull/30405#pullrequestreview-2161741589)
gh pr checkout 30405

> gh pr checkout 30405

✅ zxramozx closed a pull request: "Patch 4"
(https://github.com/bitcoin/bitcoin/pull/30405)

💬 zxramozx commented on pull request "Patch 4":
(https://github.com/bitcoin/bitcoin/pull/30405#issuecomment-2212211013)
.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667582047)
Moved to later commit.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667582156)
Changed to `sentinel()`.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667582337)
Fixed.