💬 maflcko commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-1956486790)
> LGTM. Could change `template<typename InputIterator>prevector(...)` to also expect `std::input_iterator`.

Sure, done for all InputIterators in this file.



> that's buggy if the passed in iterators specify a greater range than a `size_type` (or `difference_type` for `insert()`) can hold.

I think it will in all cases be buggy if the range can not be held in `difference_type`, as `difference_type` (aka `int32_t`) is used to represent the subtraction of two pointers in the prevector it
...

💬 BrandonOdiwuor commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1497400170)
fixed

💬 m3dwards commented on pull request "ci: Avoid CI failures from temp env file reuse":
(https://github.com/bitcoin/bitcoin/pull/29441#issuecomment-1956501825)
Post merge ACK https://github.com/bitcoin/bitcoin/commit/fa91bf2559d2e839592bf1dc1d423d5fb1c3573e.

I ran multiple CI jobs at the same time and observed the nicer naming in the /tmp directory.

👋 BrandonOdiwuor's pull request is ready for review: "doc: Fix gen-manpages to check build options"
(https://github.com/bitcoin/bitcoin/pull/29457)

👍 maflcko approved a pull request: "consensus: Store transaction nVersion as uint32_t"
(https://github.com/bitcoin/bitcoin/pull/29325#pullrequestreview-1893031424)
Left some nits. Lgtm

re-ACK 4542a4d76e7598ac67fa4f154b13b2fa32050dd2 🌷

<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: re
...

💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1497420900)
:eyes:

💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1497420458)
:eyes:

💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1497425647)
Need to change it in the comment in the header as well

💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1497432560)
:eyes:

💬 stickies-v commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#discussion_r1497485628)
Thanks for spotting, fixed!

💬 stickies-v commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#issuecomment-1956590859)
Force pushed to address[ indentation issue](https://github.com/bitcoin/bitcoin/pull/29277#discussion_r1493816010) (and removed the unintuitive "developer mistake" comment in docstring)

> @stickies-v have you considered preparing a map once when an RPC is called rather than doing the search for the index every time an arg is accessed?

I agree with @maflcko's reasoning, so going to leave this as is.

🤔 ryanofsky reviewed a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929#pullrequestreview-1893028346)
Updated 33d99bad01637a3454358841b5c1aaf099002afb -> 9dc839a7fd1c8c7648f9e0a025d5ebecee8d289a ([`pr/iparams.6`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.6) -> [`pr/iparams.7`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.7), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/iparams.6..pr/iparams.7)) applying various suggestions and splitting commits

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