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

💬 txxlt commented on issue "importaddress failed, Only legacy wallets are supported by this command.":
(https://github.com/bitcoin/bitcoin/issues/29772#issuecomment-2198568437)
损失dumpprivkey是令人难以接受的，自以为是的蠢蛋们正在毁灭BTC

🤔 paplorinc reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2150053762)
I still think we can get rid of `m_prev` by always using an immutable `SENTINEL` as the last element, inserting new elements at the head, and handling deletions by copying the next node's content and deleting that instead (unless it's the `SENTINEL`).
What do you think?

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660183610)
nit: it seems to me we're really pushing it with the loop iteration + deletion here, could we do the deletion at the end of the loop instead to have a cleaner separation or the responsibilities? It may make the `Next` method cleaner as well (I know it was similar before as well).

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660189469)
This kept me up at night, we should be able to get rid of `m_prev` by doing insertions and deletions slightly differently:
* we'd have an immutable `SENTINEL` value always serving as the last element (instesd of `nullptr`);
* insertion would always be in front of the previous element, which would serve as the new head (iterating from which will always get us to the `SENTINEL`);
* deletion would copy the content (`node`, `flag` and `next`) of the next node instead (unless it's a `SENTINEL`), a
...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660184345)
after iteration we may still have elements left in `node`, right? Can we assert that we don't?

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660182664)
we're inside `CCoinsCacheEntry`, might as well omit the prefix:
```suggestion
inline bool IsDirty() const { return m_flags & DIRTY; }
inline bool IsFresh() const { return m_flags & FRESH; }
```

💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2198587045)
> I could be off, but the regtest powlimit may be:
>
> https://github.com/bitcoin/bitcoin/blob/2f6dca4d1c01ee47275a4292f128d714736837a1/src/kernel/chainparams.cpp#L423
>
> which means we would want to use a value higher than that for the test (i.e. a hash that would have no chance of colliding with a generated block). Someone should correct if I'm mistaken.
>
> As a sanity check, I generated 10,000 blocks on regtest and none had a block hash greater than 0x7ff...

Thats a good idea! I
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2198587395)
> we should be able to get rid of m_prev by doing insertions and deletions slightly differently

But we can't move the coin from the next entry to us. The `coinsCache` map will still point to the next entry for that outpoint.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2198592345)
> The coinsCache map will still point to the next entry for that outpoint

Yeah, but we can update the map in constant time, right? Do you think that would measurably slow down execution?
Could the memory savings of one less pointer allow us to increase cache hit rate, or am I completely off here?

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2198593956)
Using a singly linked list would be better, agreed. I just don't see how this design would work. How do we clear the flags but keep the coin? That is the purpose of this patch, to *not* delete an entry after clearing the flags.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2198594749)
Oh wait, we only need random access for deletion! Otherwise we can clear the flags on iteration!

👍 tdb3 approved a pull request: "test: Added coverage to Block not found error using gettxoutsetinfo"
(https://github.com/bitcoin/bitcoin/pull/30340#pullrequestreview-2150073353)
ACK 8ec24bdad89e2a72c394060ba5661a91f374b874

📝 ryanofsky opened a pull request: "logging: Replace LogError and LogWarning with LogAlert"
(https://github.com/bitcoin/bitcoin/pull/30364)
Replace `LogError` and `LogWarning` with `LogAlert` to make it easier to choose the correct logging levels to use and avoid log spam.

This PR implements an idea ajtowns came up with in [#30347 (comment)](https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659096893) and it reduces the decision tree for chosing log levels to:

```mermaid
flowchart TD
start-->|No|dbg
start{{Is this critical information?}}-->|Yes|crit

dbg-->|No|logtrace(Trace + Category)
dbg{{Is
...

💬 ryanofsky commented on pull request "doc: Drop description of LogError messages as fatal":
(https://github.com/bitcoin/bitcoin/pull/30361#issuecomment-2198605417)
re: https://github.com/bitcoin/bitcoin/pull/30361#issuecomment-2197781492

Thanks, added back examples and "severe".

I think probably just looking at AbortNode callers would be as effective as as searching for fatal error messages, if looking for places to make error handling more robust.

I do like the alert level idea and posted an implementation in #30364.

Updated 5175cbcbb00d6212cf548de428f43cb0c2a1be4b -> b7aae361b273f2f439d3b278214b7e37908c8cb0 ([`pr/nofat.1`](https://github.com/
...

👋 ryanofsky's pull request is ready for review: "doc: Drop description of LogError messages as fatal"
(https://github.com/bitcoin/bitcoin/pull/30361)