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

Checked coverage per https://corecheck.dev/bitcoin/bitcoin/pulls/29460

💬 maflcko commented on pull request "test: assert rpc error for addnode v2transport not enabled":
(https://github.com/bitcoin/bitcoin/pull/29460#discussion_r1497075255)
```suggestion
assert_raises_rpc_error,
```

nit (only if you re-touch for other reasons): Please always add the trailing comma, to avoid having to touch this line again in the future when a new item is added to the list. Also, could sort.

💬 maflcko commented on pull request "test: check_mempool_result negative feerate":
(https://github.com/bitcoin/bitcoin/pull/29459#discussion_r1497080191)
The comment is now wrongly placed

💬 maflcko commented on pull request "Update rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-1956121171)
Are you still working on this? https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-1943705614

💬 willcl-ark commented on pull request "docs: ci multi-arch requires qemu":
(https://github.com/bitcoin/bitcoin/pull/29456#issuecomment-1956191576)
Seems like a useful addition.

I do vaguely recall installing a `binfmt` package of some sort when I last did this, but could be misremembering something else I was doing with docker and qemu (apologies if so)...

👍 willcl-ark approved a pull request: "docs: ci multi-arch requires qemu"
(https://github.com/bitcoin/bitcoin/pull/29456#pullrequestreview-1892465034)
utACK 540282905dc6137a307273188d6d9291809f0ee9

💬 maflcko commented on pull request "docs: ci multi-arch requires qemu":
(https://github.com/bitcoin/bitcoin/pull/29456#issuecomment-1956206891)
lgtm ACK 540282905dc6137a307273188d6d9291809f0ee9

👍 brunoerg approved a pull request: "test: assert rpc error for addnode v2transport not enabled"
(https://github.com/bitcoin/bitcoin/pull/29460#pullrequestreview-1892780591)
utACK 345169a7523f00209985da88e0171e8687589c25

🤔 ajtowns reviewed a pull request: "refactor: Allow CScript construction from any std::input_iterator"
(https://github.com/bitcoin/bitcoin/pull/29369#pullrequestreview-1892789962)
utACK fa9ef95b4c1114e2d8f7c3307fdf01446efee6fd

LGTM. Could change `template<typename InputIterator>prevector(...)` to also expect `std::input_iterator`.

The prevector functions that take two iterators first increase the capacity by `last - first` then run `fill(ptr, first, last)` -- that's buggy if the passed in iterators specify a greater range than a `size_type` (or `difference_type` for `insert()`) can hold. Could perhaps be worth changing:

```diff
- template<typename InputIterat
...

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1497241949)
Same?

👍 maflcko approved a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929#pullrequestreview-1892577456)
left some comments/nits. Feel free to ignore.

very nice. lgtm ACK 33d99bad01 👆

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

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1497241697)
nit: Space after `template` according to clang-format?

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1497229478)
nit in 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?

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

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1497210553)
af7f90da74b935c17d986b0f4d21e4bce6f56343: Thanks for removing it, but I think it can be a separate (trivial) 4 line diff commit?

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

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1497303755)
nit in https://github.com/bitcoin/bitcoin/commit/af7f90da74b935c17d986b0f4d21e4bce6f56343: I wonder if the `nested` bool could somehow be derived with a C++20 concept, so that it is true, whenever `SubStream::m_substream` exists (or whenever `SubStream{}.GetStream()` exists). This would avoid issues where a dev "manually" nests params-streams and would have to call `GetStream` for the number of manual nests they did. Using the compiler to deduce the bool makes sure that `GetStream` would have to
...

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1497272449)
> The language already doesn't allow passing temporaries as rvalues so there is probably not anything the compiler can helpfully warn about in that case.

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:

```cpp
ParamsStream<const UncopyableStream&,const BaseFormat&> pstream{UncopyableStream {MakeByteSpan(std::string_view{"abc"})}, RAW};
BOOST_CHECK_EQUAL(pstream.GetStream().str(), "abc");
...

💬 maflcko commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1497361706)
what is the point of re-doing the check for every binary?

💬 bstin commented on pull request "rpcauth.py - Add new option (-json) to output text in json format":
(https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-1956461349)
Apologies for delay - PR now has changed prefix / title. I also included update to README.md to document change.