💬 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

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

🚀 fanquake merged a pull request: "chore: remove redundant word"
(https://github.com/bitcoin/bitcoin/pull/31855)