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

> ;;

Thanks fixed

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

> Same?

Thanks, fixed

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

> [af7f90d](https://github.com/bitcoin/bitcoin/commit/af7f90da74b935c17d986b0f4d21e4bce6f56343): Thanks for removing it, but I think it can be a separate (trivial) 4 line diff commit?

Sure, moved to separate commit

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

> nit in [af7f90d](https://github.com/bitcoin/bitcoin/commit/af7f90da74b935c17d986b0f4d21e4bce6f56343): Would it matter if all of these were updated to use `GetStream` over `m_substream`? I guess the only difference would be that `std::stacktrace::current()` would be smaller if called inside of one of the substream methods?

Nice idea. Replaced m_substream with GetStream here. It might make it easier for compiler to in
...

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

> nit: Space after `template` according to clang-format?

Thanks, fixed

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

> nit in [af7f90d](https://github.com/bitcoin/bitcoin/commit/af7f90da74b935c17d986b0f4d21e4bce6f56343): I wonder if the `nested` bool could somehow be derived with a C++20 concept [...]

I'll try that out. I haven't really experimented with concepts much.

💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1497439570)
> What is the point of LIFETIMEBOUND in deduction guides? Does it do anything?

Good catch, they can't do anything, removed these

💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1497504953)
.

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