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

✅ ryanofsky closed a pull request: "logging: Use LogFatal instead LogError for fatal errors"
(https://github.com/bitcoin/bitcoin/pull/30347)

💬 ryanofsky commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2198607175)
Closing this PR as #30361 is a more direct fix for the issue, assuming we are ok with log levels not making a distinction between fatal errors and other types of errors. It [seems](https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659096893) like it never might have been anybody's intention to even make this distinction in the first place and it just got written in when new documentation was added.

💬 jonatack commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#issuecomment-2198630776)
I think this makes the logging less useful, as it's less clear if the alert is a warning, an error, or fatal (or possibly if action by the user is required), and this would affect non-developer users who don't change the logging from default settings.

I would suggest simplifying the logging code, rules/restrictions and logging API, and making the latter consistent along with the printed logs. Reducing end-user utility doesn't seem the way to go in reducing the existing complexity.

💬 hebasto commented on pull request "depends: remove `FORCE_USE_SYSTEM_CLANG`":
(https://github.com/bitcoin/bitcoin/pull/30201#issuecomment-2198632337)
CI related stuff has to be ported to the CMake staging branch.