💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2197235201)
If there are further nits, I will address them in a follow-up.

Ready for more (re)review.

💬 TheCharlatan commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197248760)
Re https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2194940918

> I don't understand where this is coming from. Libraries can export globals, eg stdin, so I don't see why you're claiming it's necessary for the kernel when it's not even necessary for libc.

Being forced to eliminate all globals doesn't seem to be the claim here? As long as the globals don't carry over annoying state from previous usage, require annoying initialization, or aren't mostly safe from user misuse, their u
...

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