💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1497505462)
> It's because after successfully locking the lockdir directory, I don't want to release the lock (you had caught that problem earlier), which means we can't delete .lock, but I do want to start with a clean data directory. This extra level makes it easy to delete the old data directory (if it exists) without disturbing the .lock file so it can remain locked the entire time.

What if we use the test group directory level instead?. See 6697933c.

💬 tdb3 commented on pull request "rpcauth.py - Add new option (-json) to output text in json format":
(https://github.com/bitcoin/bitcoin/pull/29433#discussion_r1497514698)
ACK for 713ab485d7b0aa86ea5af35e44917ef15a5fb22a

💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1497520893)
Also `doc/policy/mempool-replacements.md`

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1956631999)
ACK 9dc839a7fd1c8c7648f9e0a025d5ebecee8d289a 📇

<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 9dc839a7fd1c8c7648f9e0a02
...

💬 BrandonOdiwuor commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#issuecomment-1956640728)
@luke-jr you mean away to override the changes or the help2man generation?

💬 theuni commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1956654181)
@fanquake Do you think it's too late for this for v27? It would imply require getting https://github.com/bitcoin-core/crc32c-subtree/pull/6 in, I think.

💬 theuni commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1956665685)
> 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.

🤔 ryanofsky reviewed a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929#pullrequestreview-1893217667)
Updated 9dc839a7fd1c8c7648f9e0a025d5ebecee8d289a -> 40f505583f4edeb2859aeb70da20c6374d331a4f ([`pr/iparams.7`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.7) -> [`pr/iparams.8`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/iparams.7..pr/iparams.8)) using concept to replace `bool nested` as suggested

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

> I wonder if the `nested` bool could somehow be derived with a C++20 concept

Nice suggestion, implemented this

💬 maflcko commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#discussion_r1497547735)
nit in 0260f84ce07a7795b57cd84dc51d7c31a1ee15bd: `s/man/rpc/g`?

👍 maflcko approved a pull request: "RPC: access RPC arguments by name"
(https://github.com/bitcoin/bitcoin/pull/29277#pullrequestreview-1893195194)
ACK 391873694137f241dfe1f57ed9058a438e147218 🍔

<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 391873694137f241dfe1f57ed9
...

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