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

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