📝 Jianru-Lin opened a pull request: "Correct Error Code in OP_IF/OP_NOTIF Empty Stack Check"
(https://github.com/bitcoin/bitcoin/pull/30359)
### Description

This pull request addresses an error code inconsistency in the handling of empty stacks for the `OP_IF` and `OP_NOTIF` Bitcoin Script opcodes.

Currently, if the stack is empty when evaluating these opcodes, the script returns `SCRIPT_ERR_UNBALANCED_CONDITIONAL`, which is misleading. The correct error code for this scenario should be `SCRIPT_ERR_INVALID_STACK_OPERATION`, as the issue lies in insufficient stack elements rather than mismatched conditionals.

This change modi
...

💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1658977149)
Made this change.

💬 jonatack commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1658995029)
> I don't think it makes sense for us to have 7 levels of logging -- even 5 is a lot.

Meta:

It's not clear why people are 👍 on that comment. Are 5 "a lot", despite what other projects do, or based on our needs? Or is the 👍 about 5 being ok, but not having more than 5?

I've seen drive-by 👍 on other comments recently where the comment made multiple points, and it was similarly not clear: which of the points the 👍 pertains to? All? Some?

If a reviewer has a comment, I think it would
...

💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2197273180)
After going through https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2134614071, [sv2-spec](https://github.com/stratum-mining/sv2-spec/blob/main/03-Protocol-Overview.md), https://github.com/stratum-mining/sv2-spec/discussions/85 time and time again, I put all the relevant information in [sv2-draft-sketch.pdf](https://github.com/user-attachments/files/16033413/sv2-draft-sketch.pdf) for the Config A case, to make better sense of the trade offs involved.

With the assumption that I unde
...

💬 jonatack commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659004164)
@ryanofsky

> A description of the actual log levels in our code base is

I agree. When I was working on #25203, for instance, it was often unclear to me whether to use error, warning, or info, and your classification would have made the choice clear. I am also not sure that we can usefully eliminate all helpful logging in one or more of the categories you described.

💬 cobratbq commented on pull request "doc: external-signer, example 'createwallet' RPC call requires eight argument":
(https://github.com/bitcoin/bitcoin/pull/30354#discussion_r1659007178)
Done. I hope you like it ... it's become a bit convoluted, but it certainly is more descriptive.

🤔 paplorinc reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2148425344)
Fantastic change, would be helpful if you could make it a bit more digestible by separating it into smaller commits (especially splitting high-risk changes from low-risk refactoring), explaining the changes in commit messages.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659000088)
```suggestion
auto attr = entry.GetFlags();
```

(or just remove the assert, doesn't make a lot of sense to me)

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659006186)
Yeah, too many changes in one commit

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658985403)
doing refactoring and logic changes in the same commit makes reviewing slightly harder - could you extract all non-critical changes in a separate commit so that we can concentrate on high-risk code more than on stylistic changes (which are important, but shouldn't distract us)

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659005029)
this `it->second.GetFlags() & CCoinsCacheEntry::DIRTY` appears quite often, maybe it would make sense to extract it to `it->second.IsDirty()` (and `it->second.IsFresh()`, of course)

💬 ryanofsky commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2197288729)
re: https://github.com/bitcoin/bitcoin/issues/30348#issue-2378034511

> Current `LogWarning` usages looked appropriate, but were rare, with only 8 usages currently.

This turned out to be incorrect, so I edited this part of the issue description. wilcl-ark found a case where `LogWarning` is currently being misused (https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2194685490) to warn about an unexpectedly large `-checkblocks` value. I also found more incorrect uses that I missed bef
...

💬 paplorinc commented on pull request "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin":
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2197292114)
> your time might be better spent reviewing Pull Requests like https://github.com/bitcoin/bitcoin/pull/28280

Thanks for the hint @fanquake, it's a tough PR, I have added my two sats to it: https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2138727793

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659023545)
I don't fully understand how we need `m_prev` here, but have you investigated whether it's possible to get rid of it and rely on `m_next` only?
Sometimes we still have a reference to both, but I assume that wasn't always the case, right?

💬 ryanofsky commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659025992)
re: https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659004164

> Meta:
>
> It's not clear why people are 👍 on that comment.

I kind of like the mystery of the reactions. But I 👍 AJ's chart because I think it is a good recommendation for how to choose log levels in new code, even if it doesn't really tell you what log levels mean in existing code. I also broadly agree with having few log levels and using few log levels. I think by a large margin most log statements should be u
...

💬 jonatack commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2197318437)

> All consistent, now accepting the same parameters
> All compatible with log categories, to make it possible to filter or highlight log messages from a particular component
> All compatible with wallet logging, which includes the wallet name in log messages
> Require less verbosity, by not needing category constants to be repeated in log prints from a common source.
> Not tied to one hardcoded global Logger instance


I think I'd be Concept ACK on these, though I've some
...

💬 tdb3 commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2197343758)
> 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)
>
> index_n
...

💬 ajtowns commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2197354127)
Concept NACK; introducing more different levels just makes it require more effort to use the logging system, and increases the chances they will be used incorrectly.

💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2197361518)
> We've just over-complicated everything when it could be simple and open. I know a number of other developers think the same but don't speak up much because it seems pointless to do so.

Curious, when you say "over-complicated", are you referring to the implementation of the logging code, or to the usage of the logging API? Or to effects of logging settings?

I don't think the logging API is too complicated. I think #28318 made it more self documenting and simpler.

On the other hand, I w
...

💬 ajtowns commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659082202)
> Your [flowchart](https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657886163) is a good idealization, but it does not describe the realities of current code, which is using logging for ordinary error reporting, not just the most critical errors.

The current code mostly hasn't migrated to the multiple severity levels from the original LogPrintf/LogPrint split. It would be nice to spend time working on that, but we're instead constantly bikeshedding the API instead and treating that
...