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

💬 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_r1954339722)
TIL https://flylib.com/books/en/2.887.1.53/1/

Possible solutions I see:
* drop the default altogether and have all callers use 2 arguments
* use the "non-virtual interface idiom" as described in the chapter above (it is from the book "Effective C++" by Scott Meyers)
* use overload instead of default arguments. Have two methods `waitTipChanged(hash)` and `waitTipChanged(hash, timeout)` (the first could call the second internally).

💬 fanquake commented on pull request "cmake: Install man pages for configured targets only":
(https://github.com/bitcoin/bitcoin/pull/31765#issuecomment-2656330751)
Close this given #31844 (which also solves #31745)? Or, it'd be good if you could comment either here or there on the alternative approach.

💬 vasild commented on pull request "net: reduce CAddress usage to CService or CNetAddr":
(https://github.com/bitcoin/bitcoin/pull/31854#issuecomment-2656335347)
`74f42a5920...cd4bfaee10`: address https://github.com/bitcoin/bitcoin/pull/31854#discussion_r1954320762

💬 vasild commented on pull request "net: reduce CAddress usage to CService or CNetAddr":
(https://github.com/bitcoin/bitcoin/pull/31854#discussion_r1954343810)
Should be "CService", fixed, thanks!

💬 InnDe commented on issue "Wallet passpharse":
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2656343492)
I think 14 or 15 that was before 12 years
On Thu, 13 Feb 2025 at 14:20 Will Clark ***@***.***> wrote:

> 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)?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2656290307>,
> or unsubscribe
> <h
...

💬 fanquake commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1954349832)
Yes

💬 maflcko commented on pull request "qa debug: Add --debug_runs/-waitfordebugger [DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/31723#issuecomment-2656348436)
> > I think if you want to debug the start-up sequence specifically, it may be easier to do it outside the tests? Then anything else, if needed, can happen without patching Bitcoin Core source code itself.
>
> This is not about debugging the start-up sequence specifically, but rather debugging _from an early stage_. Maybe one has set 5 breakpoints, some of which are hit during _bitcoind_ init, and some which are hit by re-running a Python test which calls a set of RPCs.

I'd still say it is
...

💬 maflcko commented on issue "CI: Failed pulls from docker.io causing jobs to fail":
(https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2656374222)
This still happens: https://cirrus-ci.com/task/4901485584056320

Though, it seems odd, because that run should not have pulled more than two images. Rate-limiting that seems that the new rate limits are stronger that they document themselves?

If that is true, maybe the easiest fix would be to run (or pick) a caching pass-through mirror instead?

💬 maflcko commented on pull request "chore: remove redundant word":
(https://github.com/bitcoin/bitcoin/pull/31855#issuecomment-2656376795)
lgtm ACK 033acdf03da4a77d69fb58f7cab97dd1073fb83e