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

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

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

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659854749)
Not sure I want to touch this right now. C

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

💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1659878816)
Thank you, let's see if others want this change too. I think it could be acceptable to have more example lines.

💬 darosior commented on pull request "Wallet: don't underestimate the fees when spending a Taproot output":
(https://github.com/bitcoin/bitcoin/pull/26573#issuecomment-2198239776)
Force-pushed to add some `Assume`s. Looks like the CI issue was unrelated.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659918531)
Before this patch, the iteration is `it = erase ? mapCoins.erase(it) : std::next(it);`. So this is basically the same behavior, is it not?

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659919946)
We want to tie adding a flag to adding the entry to the linked list. We don't want to have an interface that lets something add a flag without adding it to the linked list. I think splitting that functionality into multiple methods would be dangerous.

`ClearFlags` is similar. Does it matter to external callers how these methods modify the linked list? That should be transparent to callers. They just know the calling `Next()` from the head gets all flagged entries. The how should not be expose
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2198280480)
@mzumsande @paplorinc thank you very much for your reviews! I've rebased and updated with all your suggested changes.

I also updated the commits for easier review. First, `flags` is encapsulated whenever used, then made private. Then linked list head and self references are added to `AddFlags`, followed by `AddFlags` and `ClearFlags` actually maintaining a linked list of entries. Lastly, the linked list is used for `BatchWrite` instead of passing the entire map of entries.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659947871)
Got it, thanks!

💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1659967321)
Yeah, I could accept either way. You have shown good persistence with this issue @am-sq!

👍 Christewart approved a pull request: "policy: Add `OP_TRUE <0x4e73>` as a standard output script"
(https://github.com/bitcoin/bitcoin/pull/30352#pullrequestreview-2149824011)
utack 99e9d3c7bc45e1266c6592b2afd47b8999745e04

🤔 paplorinc reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2149804075)
Added a few more comments, it's a lot more understandable to me now - though I'm still lacking a few important details, I'll do those next week.
Thanks for all your effort!

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659952944)
I don't mind the current one either, but now that you have helper method, maybe we don't even need to expose the internals:
```suggestion
if (it != cacheCoins.end() && !it->second.IsDirty() && !it->second.IsFresh()) {
```

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659956700)
this seems like a change in behavior (`=` to `|=`)

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

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659956441)
hmmm, shouldn't this be `this->flags |= flags;`? I wonder how the tests passed...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659971494)
so this basically inserts "in the middle" of the linked list all the time, i.e. not at the beginning or the end, right?
I also find that quite confusing, i.e.
```
head <-> newest node <-> second newest <-> third newest <-> oldest node -> nullptr
```

what if we iterated the initialized nodes backwards so that they reflect the order of insertion, something like:
```C++
std::list<CoinsCachePair> CreatePairs(CoinsCachePair& head)
{
std::list<CoinsCachePair> nodes(NUM_NODES);
for
...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659966077)
Could the overall intent be better expressed here, something like `SetCacheStateAndLink`/`ResetCacheStateAndUnlink` or `Sync`/`Reset` or something?
It's obviously doing more after this commit than what it announces...