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

💬 rkrux commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2020681949)
A note here on the `key` object could be helpful because it appears to be a commonly used arg as there are around ~75 occurrences of this object in this file. Moreover, it's not even a named argument in the `make_spender()` but a passthrough. The only mention of it is here:
https://github.com/bitcoin/bitcoin/blob/4c1906a500cacab385b09e780b54271b0addaf4b/test/functional/feature_taproot.py#L425

An indirect mention is here:
https://github.com/bitcoin/bitcoin/blob/4c1906a500cacab385b09e780b5427
...

🤔 rkrux reviewed a pull request: "test: Add encodable PUSHDATA1 examples to feature_taproot"
(https://github.com/bitcoin/bitcoin/pull/32114#pullrequestreview-2728655786)
Sorry if these comments are coming in a bit late.

✅ hebasto closed an issue: "oss-fuzz build fails"
(https://github.com/bitcoin/bitcoin/issues/32167)

👍 TheCharlatan approved a pull request: "validation: set BLOCK_FAILED_CHILD correctly"
(https://github.com/bitcoin/bitcoin/pull/31835#pullrequestreview-2728780393)
Re-ACK c99667583dd9b57612edf4c04611cd4857250600

💬 pablomartin4btc commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020752757)
Thanks! I'll overload `HasLegacyRecords` so it'd make it even cleaner I think.

💬 pablomartin4btc commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020753904)
Yeah, I'll tweak it a bit. Thanks!

💬 rkrux commented on pull request "wallet: migration, avoid loading legacy wallet after failure when BDB isn't compiled":
(https://github.com/bitcoin/bitcoin/pull/31451#discussion_r2020760391)
I'll let furszy confirm but I believe conceptually this^ is what he meant. You can open a PR for others to review and provide suggestions. Also, you can keep the PR in draft mode if you prefer so, which lets the reviewers know the PR is not ready for review yet.

But it's easier and better to give suggestions in a PR than it is on a standalone commit - you will also get CI feedback while you're waiting for a response.

✅ maflcko closed a pull request: "qt: Add addressList field to SendCoinsRecipient for multiple addresses"
(https://github.com/bitcoin-core/gui/pull/857)

💬 maflcko commented on pull request "qt: Add addressList field to SendCoinsRecipient for multiple addresses":
(https://github.com/bitcoin-core/gui/pull/857#issuecomment-2765787428)
Closing for now, per previous comment. Feel free to open a fresh pull request, which passes all tests, and the CI. Also, a screenshot and steps to reproduce or a test would be good for a new feature.

💬 pinheadmz commented on pull request "[draft] Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2765788869)
I rebased this branch on a single squashed commit from #30988 essentially just cherry-picking `sockman.{h,cpp}` by @vasild and leaving out the p2p refactor. This will make rebase maintenance on master a lot easier by reducing conflicting scope, and hopefully also makes review easier. It also means to some extent this PR can be merged independently of #30988, and also gives @theuni some room to rewrite a specific HTTP sockman if a more efficient purpose-focused module can be written. (Will updat
...