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

💬 theStack commented on pull request "test: p2p: check that connecting to ourself leads to disconnect":
(https://github.com/bitcoin/bitcoin/pull/30362#discussion_r1659421007)
Tried that initially, but the log line shows not the destination, but the source address of the connection, which contains an ephemeral port (i.e. randomly assigned by the OS) and hence can't be predicted.

💬 djkarmi commented on issue "Slow sync ":
(https://github.com/bitcoin/bitcoin/issues/30360#issuecomment-2197790240)
So what should I do now?

💬 mzumsande commented on issue "Slow sync ":
(https://github.com/bitcoin/bitcoin/issues/30360#issuecomment-2197795804)
you could just restart the node and see whether the problem persists. If it's the problem I mentioned above, you should connect to new peers and the problem should go away. If it persists, it's probably another problem, e.g. networking issues.

💬 kevkevinpal commented on pull request "test: p2p: check that connecting to ourself leads to disconnect":
(https://github.com/bitcoin/bitcoin/pull/30362#issuecomment-2197886240)
tACK [5d2fb14](https://github.com/bitcoin/bitcoin/pull/30362/commits/5d2fb14bafe4e80c0a482d99e5ebde07c477f000)

looks good to me and I ran it locally on my machine and looks to work well

💬 azazar commented on issue "Internal bug detected: Fee needed > fee paid":
(https://github.com/bitcoin/bitcoin/issues/29398#issuecomment-2198052531)
I didn't find a way to reproduce it after the restart.

💬 djkarmi commented on issue "Slow sync ":
(https://github.com/bitcoin/bitcoin/issues/30360#issuecomment-2198074781)
When using the 32 bit version there was no such problem with synchronization.

💬 alfonsoromanz commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2198179328)
Thanks @mzumsande. I have reverted the change regarding the commit order and have also addressed the feedback from @ryanofsky by calling `LastCommonAncestor()` before `TryDownloadingHistoricalBlocks()`.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659831027)
There's no accessor to `m_prev` though. This is checked below in the random deletion test.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659831168)
Hmm well the `AddFlags` function is in production code, so this is constructing the linked list via production code. The `std::list` we create is a test harness helper that we use to check that our linked list behaves the same.