💬 maflcko commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2020590366)
thx, done

💬 maflcko commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2020590512)
thx, done

💬 maflcko commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2020590604)
> I think it's a good change, I like consistency and though a few other similar ones could be added here.

I don't think it is possible to achieve "consistency" here. There is no automated check to catch those in definitions. The goals here are:

* Performance improvements (by the first commit). However, there are no benchmarks, so this is questionable.
* Readability and style related improvements. However, they are not enforced consistently, so this is questionable as well.

So I'll drop
...

💬 maflcko commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2020591301)
(dropped commit for now)

💬 willcl-ark commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020591415)
Thanks, that infradead page is very handy!

I made a few changes in 6c694c212f8d898dac9cc1c5637381452c91e79b based on your thoughts (and the protocol page):

- set socket to non-blocking mode to avoid hanging if the kernel doesn't send a response
- use a vector to collect all data from multi-part responses
- exit when `recv()` returns 0 (this should handle single-part messages, AFAICT)

I think relying on receiving no more data from `recv()` to break the receive loop should be as (or per
...

💬 hebasto commented on pull request "depends: Switch from multilib to platform-specific toolchains":
(https://github.com/bitcoin/bitcoin/pull/32162#discussion_r2020593162)
Also please note that `depends/README.md` lists triplets after specifically mentioning "for cross compilation". In such cases: https://github.com/bitcoin/bitcoin/blob/4c1906a500cacab385b09e780b54271b0addaf4b/depends/hosts/default.mk#L1-L3 and toolchain prefixes usually do not include the `-pc-` infix.

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r2020635559)
Trying this: https://stackoverflow.com/a/21699210

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r2020647766)
Oh no that makes no sense, this is not yaml.

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r2020660927)
I added the single space indentation, but otherwise no changes. When testing locally, dropping `\` breaks cmake.

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2765616858)
Rebased.

💬 laanwj commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020671105)
> I think relying on receiving no more data from recv() to break the receive loop should be as (or perhaps more) robust than checking for the NLM_F_MULTI flag and exiting after first receive if it's not set, but curious what you think here?

Maybe-is it safe to assume that netlink will never block?

We don't want to end up in the same situation as before where we miss data. But due to say, a threading race condition.

i think the safest thing here is to mimic as closely as possible `ip`'s
...

💬 laanwj commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020674715)
> use a vector to collect all data from multi-part responses

i'm not sure i see the motivation here. Parsing the packets as seperate units is just as valid, avoids dynamic allocation, and is simpler.

💬 l0rinc commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2765643722)
crACK fa9cd7bd4f8b4bad4b01bb3709f91d2a651fe47d

<details>
<summary>Diff compared to previous review</summary>

```patch
diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp
--- a/src/test/argsman_tests.cpp (revision 17cb88fefcc390967091a4f63bd04a1b3cefcda1)
+++ b/src/test/argsman_tests.cpp (date 1743412333616)
@@ -52,7 +52,7 @@
struct TestArgsManager : public ArgsManager
{
TestArgsManager() { m_network_only_args.clear(); }
- void ReadConfigString(const std:
...

🤔 hebasto reviewed a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2728662979)
For now, I've reviewed all commits except the last three that touch the depends build subsystem.

Left a few comments.

💬 hebasto commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2020675936)
88eabc8c2c0fbb62db5cb6248cc8ab770f3ce78b, nit:
```suggestion
# Copyright (c) 2025-present The Bitcoin Core developers
```

💬 hebasto commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2020683263)
c8287113b80f1b1038bc1ba561276f18a55d86ff

When configuring with `-DBUILD_TESTS=OFF`, `mptest` is still being built needlessly.

💬 hebasto commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2020674237)
88eabc8c2c0fbb62db5cb6248cc8ab770f3ce78b

The `WITH_EXTERNAL_LIBMULTIPROCESS` option can depend on `ENABLE_IPC` using the `cmake_dependent_option` command, which allows expressing "when ENABLE_IPC is enabled" directly in the code.

💬 hebasto commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2020678945)
88eabc8c2c0fbb62db5cb6248cc8ab770f3ce78b

Why does building the `mpgen` tool require `core_interface`?

💬 hebasto commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2020697414)
88eabc8c2c0fbb62db5cb6248cc8ab770f3ce78b

I apologies for being pedantic, but the sentence _"BUILD_TESTING is a standard cmake variable that controls whether CTest is used."_ is not 100% correct, as the `BUILD_TESTING` variable is [_created_](https://cmake.org/cmake/help/latest/module/CTest.html) by the `CTest` module. However, I'm not sure how to reword the comment best.

💬 Sjors commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2765679554)
Verified that 625bd576215e82d430998bfa68501db7ba03c3b2 just drops 047e6083b0 compared to my last review, since it was included in https://github.com/bitcoin/bitcoin/pull/31992. I rebased #30975 and #31802 on top of this just in case.

💬 rkrux commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2020669952)
The naming of the `failure` variable is unintuitive imo because the first time I read it, I assumed this is some returned failure object from some operation undergoing the test but that is not the case as per this comment in the `make_spender()`.

https://github.com/bitcoin/bitcoin/blob/4c1906a500cacab385b09e780b54271b0addaf4b/test/functional/feature_taproot.py#L493

At the cost of being verbose, it could be called `failure_overrides` or something similar because seeing around ~90 odd occurr
...