💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1658972919)
Went ahead with 2 example types displayed.

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