💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659308223)
Sorry, can you elaborate on what benefit that would have here, and any possible downsides? I'm not 100% sure what all the differences are between the two methods, so preferred to stick to status quo.

💬 Emzy commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2197644218)
Tested ACK aba5047

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659322903)
In that case stick to it and I'll add it in a separate PR once this is merged!

🤔 tdb3 reviewed a pull request: "refactor: add coinbase constraints to BlockAssembler::Options"
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2148873843)
Approach ACK
This adds flexibility for SV2, having less magic numbers is better. I support adding a configuration parameter for this (would prevent others from needing to fork).

Left a couple small nits for now, but plan to test/exercise in more detail.

💬 tdb3 commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1659263337)
nit: Is it comprehensive using the term "pool" here, or would it be better to keep things generic (and perhaps add an example)? This is reserving space in the template for the coinbase and the entity requesting the template could be a pool or a solo miner (albeit less commonly), right? Maybe something like `maximum additional serialized bytes the block requestor can add in coinbase transaction outputs (for example, to enable pool payouts)`.

nit: weight and bytes seem to be used interchang
...

💬 ajtowns commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197683031)
> Re [#30338 (comment)](https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197436060)
>
> Thanks for going through this. Having something like `LaunchSettings` seems like a good thing.
>
> > Fixing the formatting of buffered logs is a bit of work, but not very complicated. It doesn't seem particularly related to the kernel, though. Would you review a PR to do that?
>
> yes :)

Have a look at https://github.com/ajtowns/bitcoin/pull/9 and see what you think; obviously needs a l
...

💬 Mariuszok12 commented on pull request "wallet: use LogTrace for walletdb log messages at trace level":
(https://github.com/bitcoin/bitcoin/pull/30355#discussion_r1659360805)
3H9yBBp3fmaZTf1iyg6Vjv1WTAENeva5Qh

🤔 Mariuszok12 reviewed a pull request: "wallet: use LogTrace for walletdb log messages at trace level"
(https://github.com/bitcoin/bitcoin/pull/30355#pullrequestreview-2149041661)
3H9yBBp3fmaZTf1iyg6Vjv1WTAENeva5Qh

💬 Mariuszok12 commented on pull request "policy: Add `OP_TRUE <0x4e73>` as a standard output script":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1659361958)
3H9yBBp3fmaZTf1iyg6Vjv1WTAENeva5Qh

💬 Mariuszok12 commented on pull request "policy: Add `OP_TRUE <0x4e73>` as a standard output script":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1659362071)
3H9yBBp3fmaZTf1iyg6Vjv1WTAENeva5Qh

👍 hodlinator approved a pull request: "scripted-diff: Log parameter interaction not thrice"
(https://github.com/bitcoin/bitcoin/pull/30358#pullrequestreview-2149051207)
utACK fa1bc7c88bb720f02955dbe4e2a174b5a52f3af8

Should have already been made less repetitive when moving them into a dedicated function in 411b05ac9511395923976bfbd0c153ddabf2ebcf.

📝 ryanofsky opened a pull request: "doc: Drop description of LogError messages as fatal"
(https://github.com/bitcoin/bitcoin/pull/30361)
Remove documentation that says `LogError` should be used for "severe problems that require the node (or a subsystem) to shut down entirely" because:

- It is not how `LogError` and `Level::Error` are used currently. Of 129 current uses only 58 cases are fatal according to [#30348](https://github.com/bitcoin/bitcoin/issues/30348)

- "[T]here's not much benefit in a log that says "hey this error is about to cause the node to stop" -- you already get that information by seeing "Shutdown: in pro
...

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

I like your idea to collapse the log levels further and I am curious about ways you think we can make progress towards that goal.

> However, there's not much benefit in a log that says "hey this error is about to cause the node to stop" -- you already get that information by seeing "Shutdown: in progress..." immediately following.

If we don't need to have a dedicated level for fatal errors, it suggests a possible a
...

💬 hodlinator commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2197730404)
(For what it is worth, I'm used to thin convenience macros for logging from other codebases. In the ecosystem we also have https://github.com/ElementsProject/lightning/blob/master/lightningd/log.h which follows the same pattern).

> I implemented that restriction in 44afae705a12252149adbe7ac6d3646052b8bf3b and would be ok with adding it this PR if it solves problems for you and AJ, even if I don't personally see a need for the API to be so restrictive.

The API feels good to me, nice touch w
...

👍 ryanofsky approved a pull request: "wallet: use LogTrace for walletdb log messages at trace level"
(https://github.com/bitcoin/bitcoin/pull/30355#pullrequestreview-2149071007)
Code review ACK 46819f5df6de2a860c3ec87d55527b617a3b128f. Nice catch!

📝 theStack opened a pull request: "test: p2p: check that connecting to ourself leads to disconnect"
(https://github.com/bitcoin/bitcoin/pull/30362)
This small PR adds test coverage for the scenario of connecting to ourself, leading to an immediate disconnect:
https://github.com/bitcoin/bitcoin/blob/2f6dca4d1c01ee47275a4292f128d714736837a1/src/net_processing.cpp#L3729-L3735

This logic has been first introduced by Satoshi in October 2009, together with a couple of other changes and a version bump to "v0.1.6 BETA" (see commit cc0b4c3b62367a2aebe5fc1f4d0ed4b97e9c2ac9).

🤔 fjahr reviewed a pull request: "test: p2p: check that connecting to ourself leads to disconnect"
(https://github.com/bitcoin/bitcoin/pull/30362#pullrequestreview-2149081709)
tACK 5d2fb14bafe4e80c0a482d99e5ebde07c477f000

Nice find!

💬 fjahr commented on pull request "test: p2p: check that connecting to ourself leads to disconnect":
(https://github.com/bitcoin/bitcoin/pull/30362#discussion_r1659388331)
nit: Could pull this up two lines and use it to check the exact log line I think, but not a blocker for me

💬 ajtowns commented on pull request "doc: Drop description of LogError messages as fatal":
(https://github.com/bitcoin/bitcoin/pull/30361#issuecomment-2197781492)
I think both entries should continue to include examples.

I think removing "severe" from the LogWarning description doesn't make sense -- anything the admin might have to address is a severe (potential) problem, isn't it?

Having LogError mean "the node admin **will** need to address" vs LogWarning mean "the node admin **may** need to address" seems to argue towards retaining the "LogError is fatal" interpretation to me -- fatal errors are ones the admin does need address; anything else the
...

💬 ajtowns commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197782050)
> > Touching every single line of logging code for a "nice to have" doesn't make much sense to me.
>
> AJ I know we are disagreeing about a lot of things, and I do not fault you in any way for this, but your reactions seem very strong relative the actual scope of changes we would like to make.

I don't know what you want me to say here? I didn't say "THAT'S CRAZY YOU SHOULD ALL BE PUT IN AN ASYLUM"; I said "it doesn't make much sense to me". The only way to be less strong about my opinion i
...