📝 fjahr opened a pull request: "test: Remove already resolved assumeutxo todo comment"
(https://github.com/bitcoin/bitcoin/pull/30403)
The case in the comment seems to be sufficiently addressed by #30267 but I forgot to remove the Todo. Thanks to alfonsoromanz for noticing this.

💬 fjahr commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2211808920)
> Post-merge question: Shouldn't this TODO be deleted because of the new test introduced in this PR?

Duh, you are right, fixed with #30403. Thanks!

💬 luke-jr commented on pull request "refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options":
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2211809968)
Seems more like kernel shouldn't have policy stuff at all, but that's out of scope for this PR. Added your commit

💬 alfonsoromanz commented on pull request "test: Remove already resolved assumeutxo todo comment":
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2211817861)
ACK 9d5a89509031ce924b9685a6e2e4b19cfef50a9e

I wonder if this is a good place to discuss other (potentially addressed) TODOs?

For example, interesting starting states could be loading a snapshot when the current chain tip is:

`- TODO: An ancestor of snapshot block`: isn't this already addressed when successfully loading the snapshot into node1 [here](https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_assumeutxo.py#L310)? Node1's chain tip is at `START_HEIGTH` (199) b
...

💬 luke-jr 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-2211820698)
Concept NACK. This seems to encourage and promote spam. We should be doing the exact opposite.

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

👍 luke-jr approved a pull request: "wallet: use LogTrace for walletdb log messages at trace level"
(https://github.com/bitcoin/bitcoin/pull/30355#pullrequestreview-2161554757)
utACK 46819f5df6de2a860c3ec87d55527b617a3b128f

💬 luke-jr commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2211824602)
Weak approach NACK. This is just limiting the max block weight, which is already a configurable parameter. Having two params for the same thing seems like a poor way to do it. I'm not sure it avoids the risk of bugs producing invalid blocks, either - it just moves the bug risk from one place to another equally probable.

Instead, maybe just perform the block weight cap on initialization rather than block assembly?

💬 luke-jr commented on pull request "[WIP] net: return result from addnode RPC":
(https://github.com/bitcoin/bitcoin/pull/30381#issuecomment-2211829520)
The return value seems entirely redundant. Maybe it should just be the new peer id (Object-encapsulated for future extension), and make `getpeerinfo` accept a peer id param to just investigate that one?

💬 luke-jr commented on pull request "fix: increase consistency of rpcauth parsing":
(https://github.com/bitcoin/bitcoin/pull/30401#issuecomment-2211848897)
Does this consider the scenario where bitcoin.conf has valid rpcauth params, but the user specifies -norpcauth or such on the command line to disable them?

👍 tdb3 approved a pull request: "test: Remove already resolved assumeutxo todo comment"
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2161569821)
ACK 9d5a89509031ce924b9685a6e2e4b19cfef50a9e
Double checking https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2211817861 would also be good as well.

💬 tdb3 commented on pull request "fix: increase consistency of rpcauth parsing":
(https://github.com/bitcoin/bitcoin/pull/30401#issuecomment-2211873385)
> Does this consider the scenario where bitcoin.conf has valid rpcauth params, but the user specifies -norpcauth or such on the command line to disable them?

Thanks for taking a look.
Yes.
`conf_setup()` creates `rpcauth` statements in node 0's bitcoin.conf (including for user `rt`).
After restarting the node with `-norpcauth`, the `assert_equal` with `rt` tests that the associated `rpcauth` statement in the conf is disabled.

https://github.com/bitcoin/bitcoin/blob/1ef15ea7120fe3f04853
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667414497)
I think I will refrain from adding these annotations for now, since they are more than just documentation and may have side effects in the generated code. I might experiment with a separate commit to see if they affect performance.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667414536)
Renamed all `head` to `sentinel`. Hopefully that makes everything clearer :).

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667415387)
Thanks, still working on reproducing the benchmarks before I can properly ACK it

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