💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667583978)
Done! Thank you for your review, I think it's pretty clean now. I just had to add a `SelfRef` method on `CCoinsCacheEntry` for the sentinel to init its pointers to itself.
Also, we need to not call `Next()` randomly on any `CCoinsCacheEntry`, only from the sentinel and then iterating through. This isn't done anywhere in the patch, but it's not protected against if anyone does. Now instead of returning a `nullptr` if it's not in the linked list it could potentially return a pointer to an already
...

📝 achow101 locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/30405)
.

👍 TheCharlatan approved a pull request: "Use `WITH_LOCK` in `Warnings::Set`"
(https://github.com/bitcoin/bitcoin/pull/30404#pullrequestreview-2161816646)
ACK 6af51e819847e737449609daa214e16f9453e85d

👍 rkrux approved a pull request: "[test]: raise an exception in `_bulk_tx_` when `target_weight` is too low."
(https://github.com/bitcoin/bitcoin/pull/30322#pullrequestreview-2161826096)
reACK [5e19ff8e](https://github.com/bitcoin/bitcoin/pull/30322/commits/5e19ff8e29628bc362eb8191f387b3f1ad514d48)

Ran make and functional tests again, all successful.

👍 rkrux approved a pull request: "Wallet: Add `max_tx_weight` to transaction funding options (take 2)"
(https://github.com/bitcoin/bitcoin/pull/29523#pullrequestreview-2161838801)
reACK [734076c](https://github.com/bitcoin/bitcoin/pull/29523/commits/734076c6de1781f957c8bc3bf7ed6951920cfcf6)

Ran make and functional tests, all successful.

@ismaelsadeeq This [diff comparison b3ac117..734076c](https://github.com/bitcoin/bitcoin/compare/b3ac1179ff90fe261af4e6dc9c7af64e1518b435..734076c6de1781f957c8bc3bf7ed6951920cfcf6) contains a lot of unrelated changes though.

💬 paplorinc commented on pull request "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin":
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2212413726)
Thanks for the review @andrewtoth and @luke-jr!

> I think you should benchmark `bitcoind -reindex-chainstate -stopatheight=820000`

I don't have that much space yet, but ran it with:
```bash
sudo hyperfine \
--runs 3 \
--show-output \
--export-markdown bench.md \
--parameter-list commit bd5d16,00052ec \
--prepare "sudo purge; sync; git checkout {commit}; git reset --hard; make -j10" \
"./src/bitcoind -reindex-chainstate -stopatheight=400000 -printtoconsole=0"
```
resulting in
| C
...

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#issuecomment-2212416250)

> @ismaelsadeeq This [diff comparison b3ac117..734076c](https://github.com/bitcoin/bitcoin/compare/b3ac1179ff90fe261af4e6dc9c7af64e1518b435..734076c6de1781f957c8bc3bf7ed6951920cfcf6) contains a lot of unrelated changes though.

Yes that's because of the recent rebase.

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667680249)
In commit "coins: track flagged cache entries in linked list"

Would it make sense to add code to `CCoinsViewCache`'s destructor to explicitly wipe the map, and then test that `m_sentinel.m_next == &m_sentinel` in an `Assume` or so? To ascertain that no deleted `CCoinsCacheEntrys` remain in the list?

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667679725)
In commit "coins: track flagged cache entries in linked list"

This doesn't matter much, but if you'd swap all prevs/nexts here, the entry would get inserted at the end of the list rather than the beginning, making the list be ordered old-to-new. Unless there is a reason to write things out from new to old?

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

Maybe turn this into an `Assume`, so it doesn't have a runtime cost in non-debug builds?

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667681297)
In commit "coins: pass linked list of flagged entries to BatchWrite"

It's a bit unfortunate that this means iterating through spent entries twice; wouldn't it be possible to let BatchWrite do the deletion too in the same loop? This will probably mean passing both `cacheCoins` and `m_sentinel` in.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667703546)
I see there's also an `assert(!coin.IsSpent());` at the beginning of `AddCoin`, which would also be in the hot path. When should we use `assert` vs `Assume` then?

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667703693)
Yeah it also means passing in the `cachedCoinsUsage` so we can decrement that when we erase.

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667704346)
I guess that predates the introduction of `Assume`.

`assert` runs always; `Assume` only runs when `ABORT_ON_FAILED_ASSUME` is defined, which is the case in debug builds and in fuzz tests.

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667705233)
Hmm, yeah, that's pretty ugly. It's fine to leave exploring this for a follow-up.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667721192)
I ended up doing it. I pass in sentinel and cachedCoinsUsage and delete the spent entries when not erasing.
I think calling erase with the outpoint only is slower than erasing with the iterator itself, so I only erase when calling `Sync`, and clear the map at the end of the loop if `erase=true`.

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

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

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

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667721501)
I moved the iteration down so the logic should be more clear now.