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

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659957585)
can you please explain why this isn't a change in behavior? (in the commit it was introduced in)

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659968285)
same, it seems to me that it's not the head that's tested, but its next pointer

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659967194)
instead of exposing `GetFlags`, we could check that it's not fresh and not dirty

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660017722)
It isn't because entry is constructed the line above, and we know from default constructor `flags` will be initialized to `0`. So `0 | flags` is same as `flags`.

💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2198357675)
> > thanks @tdb3 and @alfonsoromanz I added a log in [82c454c](https://github.com/bitcoin/bitcoin/pull/30340/commits/82c454c65658439481b50fe71ed097ecb8d70737)
> > Also @tdb3 I tried doing
> > ```
> > b1hash = index_node.getblockhash(1)
> > index_node.invalidateblock(b1hash)
> >
> > self.log.info("Test obtaining info for a non-existent block hash")
> > assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height=b1hash, use_index=True)
> >
...

📝 Balcoin-BLC opened a pull request: "Update README.md"
(https://github.com/bitcoin/bitcoin/pull/30363)
<!--
*** 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 test improvements or new tests that improv
...

✅ achow101 closed a pull request: "Update README.md"
(https://github.com/bitcoin/bitcoin/pull/30363)

📝 achow101 locked a pull request: "Update README.md"
(https://github.com/bitcoin/bitcoin/pull/30363)
<!--
*** 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 test improvements or new tests that improv
...

💬 tdb3 commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2198387274)

> > Regarding the hash used for hash_or_height, from what I can tell regtest blocks are allowed a higher target. While using 00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09 (block hash of mainnet block 1000) would be extremely improbably to collide, wouldn't it be better to use something guaranteed not to collide? Something above the max target for regtest?
>
> sounds good that makes sense I just pushed [837af6e](https://github.com/bitcoin/bitcoin/pull/30340/commits/837af6e
...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660083440)
Indeed, same for line 130

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660086532)
Not sure if it helps, just an observation: `m_prev` only uses `.second`, so it might as well be a `CCoinsCacheEntry*`
```patch
Index: src/coins.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/coins.h b/src/coins.h
--- a/src/coins.h (revision c36363f6b24c7ab2afe198d9855f507ddf096e1f)
+++ b/src/coins.h (date 1719730674870)
@@ -123,7 +123,7 @@
* parent cac
...

💬 Fi3 commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2198494395)
@itornaza I agree with you about the implication of integrating noise into the TP. Just want to point out 2 minors issues [in your sketch](https://github.com/user-attachments/files/16033413/sv2-draft-sketch.pdf):
1. Pool and JDS do not need to communicate (and if they do they do not use the job declaration protocol)
2. JDS and TP do not communicate using the template distribution protocol. JDS <-> TP communication is not part of Sv2. For example the JDS on SRI just use bitcoind RPC API.

💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2198513621)
@Fi3 thank you for clarifying it really helped a lot! Would you also think this is a good point to start reviewing the Noise Protocol implementation on this PR compared to it's [specification](https://noiseprotocol.org/noise.html)?

💬 fjahr commented on pull request "test: fix inconsistency in fundrawtransaction weight limits test":
(https://github.com/bitcoin/bitcoin/pull/30353#issuecomment-2198529040)
Code review ACK 4f571fb73526b55a83af3c04fbc662ccdd1307ae

This looks like the right fix for the observed test failure to me.