📝 vasild opened a pull request: "net: reduce CAddress usage to CService or CNetAddr"
(https://github.com/bitcoin/bitcoin/pull/31854)
Using `CAddress` when only `CService` or `CNetAddr` is needed is excessive and confusing. Fix those occurrences to use the class they need:

* `CConnman::CalculateKeyedNetGroup()` needs `CNetAddr`, not `CAddress`, thus change its argument.

* Both callers of `CConnman::CreateNodeFromAcceptedSocket()` create a dummy `CAddress` from `CService`, so use `CService` instead.

* `GetBindAddress()` only needs to return `CService`.

* `CNode::addrBind` only needs to be `CService`.

💬 Eunovo commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2656212656)
Rebased https://github.com/bitcoin/bitcoin/commit/1c6b886465df0f00549e7d10c3bfefd27be7f1c2 to https://github.com/bitcoin/bitcoin/pull/31689/commits/bd27a83efcc3d678f33041ee34eeb019777a2405

I improved some variable and function names based on received feedback, and also made style changes to align with `doc/developer-notes.md`.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2656216742)
Extracted the first commit into https://github.com/bitcoin/bitcoin/pull/31854. It is not strictly related to this PR and makes sense on its own. If merged will reduce the size of this PR.

📝 tianzedavid opened a pull request: "chore: remove redundant word"
(https://github.com/bitcoin/bitcoin/pull/31855)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954288305)
I'm introducing a new `RPC_SHUTDOWN_ERROR` which makes sense here. I don't think the message itself matters, since the user knows they shut down the node - and the exact details of where and why the RPC call fails aren't important.

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954295417)
Indeed, but on the other hand code changes over time, stuff appears between the if statement and the usage of `tip`, then someone changes the if statement, and now we have a crash...

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954297095)
Yes, this scenario I had in mind.

> in that case to ignore the timeout and instead keep waiting for the node to set a tip

Hmm. What if the tip is not being connected for whatever reason? Wait forever? The current behavior of respecting the timeout looks safer. Would be an odd user experience to provide a timeout and the program to decide to wait longer for whatever reason.

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954298757)
It seems the compiler doesn't know about this default inside the implementation file.

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954300543)
Ok

💬 willcl-ark commented on pull request "chore: remove redundant word":
(https://github.com/bitcoin/bitcoin/pull/31855#issuecomment-2656266441)
Thanks for the contribution, but this should probably be closed.

In this project we prefer typos to be fixed organically as that area of the codebase is worked on to avoid diluting precious developer review time. See our https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#pull-request-philosophy

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2656278649)
I changed `waitTipChanged()` to only return `nullopt` during shutdown. This is achieved by extending the `timeout` in the unlikely event that node initialization takes longer.

I introduced a new `RPC_SHUTDOWN_ERROR` code and use it in case `waitfornewblock` and friends are called really early and the node shuts down (probably very hard to reach in practice). See https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1952373326

On question about long polling I still need to investigate:
...

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954310804)
I ended up with something similar...

💬 fanquake commented on pull request "ci: switch MSAN to use prebuilt Clang binaries":
(https://github.com/bitcoin/bitcoin/pull/31850#issuecomment-2656283257)
Swapped to rc2. Needs a fixup for MSAN fuzz.

💬 willcl-ark commented on issue "Wallet passpharse":
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2656290307)
> but now it doesn’t work as usual

Which previous version(s) of bitcoin core did this particular wallet and password work with?

Does it still work with that version (thereby implying a regression in newer versions)?

💬 willcl-ark commented on pull request "ci: switch MSAN to use prebuilt Clang binaries":
(https://github.com/bitcoin/bitcoin/pull/31850#issuecomment-2656293223)
For fuzzing end-users, you might consider touching up the wording in:

https://github.com/bitcoin/bitcoin/blob/55cf39e4c54da6639a8f1f7c813c2909454cada1/doc/fuzzing.md?plain=1#L104-L112

...to directly mention using pre-built llvm bins (although it does already say to use the CI job as a reference which is and will remain accurate).

👍 maflcko approved a pull request: "logging: Ensure -debug=0/none behaves consistently with -nodebug"
(https://github.com/bitcoin/bitcoin/pull/31767#pullrequestreview-2614664083)
left two nits

review ACK 7afeaa24693039730c9ae00c325c2b9d0e2deda1 👡

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review
...

💬 maflcko commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1954308137)
Nit in a8fedb36a71ac193fc158284b14d4eb474e5ab62: Why is this needed? It seems like a no-op:

* If the list of debug args is empty, the code below can handle it. In fact, it must handle the case anyway. For example, if someone passes just `-nodebug`, `IsArgSet` will be true, but `GetArgs` will be empty.
* If the list of debug args is not empty, it is just a redundant check?

So my recommendation would be to remove it. If you want, you can keep the `{}` for the scope.

💬 maflcko commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1954318238)
Nit in https://github.com/bitcoin/bitcoin/commit/a8fedb36a71ac193fc158284b14d4eb474e5ab62: Would be nice to explain why this is done: Something like: `// GetArgs disregards any debugging categories appearing before -nodebug, so do the same for -debug=0/none`

💬 fanquake commented on pull request "net: reduce CAddress usage to CService or CNetAddr":
(https://github.com/bitcoin/bitcoin/pull/31854#discussion_r1954320762)
Still documented as returning a `CAddress`?

💬 willcl-ark commented on pull request "test, tracing: don't use problematic `bpf_usdt_readarg_p()`":
(https://github.com/bitcoin/bitcoin/pull/31848#issuecomment-2656312092)
Approach ACK

So if I understand the change correctly, the new (documented/recommended) behaviour is to always first `read_arg()` which gets the memory location of the data, then `read_user[_str]()` which then actually performs the copy from user memory to the BPF memory space.

If I have this correct, then using the documented, seemingly more-reliable, and more explicit approach seems fine to me.

Not sure how I could test the reliability of this myself, but https://github.com/bitcoin/bit
...