💬 maflcko commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#discussion_r1497576027)
why remove this?

💬 maflcko commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#discussion_r1497549765)
0260f84ce07a7795b57cd84dc51d7c31a1ee15bd: Why `char[]`? Could use `auto`?

💬 maflcko commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#discussion_r1497572844)
28a33e1f9891a4ea56ba843001db84c878b0e2ea: No? It does not return a falsy value?

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1956703003)
ACK 40f505583f4edeb2859aeb70da20c6374d331a4f 🎦

<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: ACK 40f505583f4edeb2859aeb70d
...

👍 BrandonOdiwuor approved a pull request: "test: assert rpc error for addnode v2transport not enabled"
(https://github.com/bitcoin/bitcoin/pull/29460#pullrequestreview-1893298478)
Code Review ACK 345169a7523f00209985da88e0171e8687589c25

💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1497623320)
re: https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1497272449

> I think you meant to say "lvalues"? Also, const lvalues should be fine by the language. For example, the following should cause a use-after-free:

Thanks that was supposed to say lvalues, and that's a good point, so it's possible lifetimebound could still help avoid bugs in some cases. I think it should be possible to add back lifetimebound by overloading the constructor using `std::is_reference`. But it would add a
...

💬 BrandonOdiwuor commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1956804728)
@fanquake fixed by disabling `--descriptors` flag when running `create_cache.py` if `--legacy-wallet` flag is provided

💬 BrandonOdiwuor commented on pull request "test: Add test case for spending bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#issuecomment-1956816638)
@maflcko could you elaborate more on this

https://github.com/bitcoin/bitcoin/pull/29120#issuecomment-1895628355
> Could also update the documentation of the setting to clarify that it refers to the relay of transactions which create bare outputs, and not to the relay of transactions spending them?

🚀 fanquake merged a pull request: "docs: ci multi-arch requires qemu"
(https://github.com/bitcoin/bitcoin/pull/29456)

💬 BrandonOdiwuor commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#issuecomment-1956840825)
@epiccurious moved aborting the tests before the Unit Tests for Test Framework Modules.

💬 luke-jr commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1497744510)
What is this supposed to do? :/

💬 maflcko commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1497755511)
```suggestion
if testdir_disk_usage_stats.free < MIN_FREE_SPACE:
```

nit: no need for `()` in python

💬 maflcko commented on pull request "test: Add test case for spending bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#issuecomment-1956886011)
> @maflcko could you elaborate more on this

Sure, what is the question?

💬 hebasto commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1956889076)
> > In the libsecp repo, the `--with-asm=no` option is still required to avoid MSan warnings.
>
> Can you point to where this is an issue? It looks as though secp has proper `__msan_*` annotations in place these days.

Sure. For example, https://github.com/bitcoin-core/secp256k1/pull/1169#issuecomment-1370003449. It holds for the current libsecp master branch @ https://github.com/bitcoin-core/secp256k1/commit/0653a25d50f67c68bd2d196ecc7eddab067d95ef as well.

💬 BrandonOdiwuor commented on pull request "test: Add test case for spending bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#issuecomment-1956893424)
was asking what you meant by this?

> Could also update the documentation of the setting to clarify that it refers to the relay of transactions which create bare outputs, and not to the relay of transactions spending them?

https://github.com/bitcoin/bitcoin/pull/29120#issuecomment-1895628355

💬 maflcko commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1956896591)
Can you test what happens when the wallet is completely disabled in configure?

📝 ryanofsky opened a pull request: "ci: remove unnecessary git diff after applying patch"
(https://github.com/bitcoin/bitcoin/pull/29461)
Drop `git diff` command so it is easier to run CI locally if git checkout is a worktree. Currently this fails because the directory is not recognized as a git repository.

The `git diff` command was added recently in #28359 commit fa07ac48d804beac38a98d23be2167f78cadefae and isn't necessary. It is also probably not useful for debugging.

💬 maflcko commented on pull request "test: Add test case for spending bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#issuecomment-1956903325)
I mean that the documentation in `init.cpp` can be clarified that it refers to outputs, not inputs.

💬 BrandonOdiwuor commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1497766575)
fixed

💬 starius commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r1497768481)
@Eunovo That's a good idea, thanks! I added the text of my comment above to the code and pushed in a separate temporal commit to track changes.