proxy: fix BuildList to use non-const iteration for interface types

[ryanofsky]

Needed for bitcoin/bitcoin#10102 since #277 was merged.

Since #277, it is no longer possible to return vector<unique_ptr<ExternalSigner>> from Node::listExternalSigners() in bitcoin/bitcoin#10102, because the proxy server objects libmultiprocess creates to wrap the ExternalSigner objects need to take ownership of the objects to keep them alive, so they need to be moved out of the returned vector, which can only be done if BuildField is passed a non-const vector element.

This PR restores previous behavior before #277 making it possible to mutate container elements while serializing them, in cases like this where it is necessary.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline and AI policy for information on the review process.
A summary of reviews will appear here.

[ryanofsky]

Updated a0b01d7 -> 46deb5e (pr/buildlist.2 -> pr/buildlist.3, compare) to fix IWYU CI failures: add #include <mp/proxy-io.h> to generated proxy-client files and reserve() call in listCallbacks https://github.com/bitcoin-core/libmultiprocess/actions/runs/28966350534/job/85950397301

[Sjors]

IIUC this isn't really a callback flow, so the terminology had me confused about the test. Might be better to add another (Bar) interface to make it more clear.

diff --git a/test/mp/test/foo-types.h b/test/mp/test/foo-types.h
index b96eabfcfb..96506a9f50 100644
--- a/test/mp/test/foo-types.h
+++ b/test/mp/test/foo-types.h
@@ -40,4 +40,5 @@ struct FooCallback; // IWYU pragma: export
 struct FooFn; // IWYU pragma: export
 struct FooInterface; // IWYU pragma: export
+struct BarInterface; // IWYU pragma: export
 } // namespace messages

diff --git a/test/mp/test/foo.capnp b/test/mp/test/foo.capnp
index edf7841c8a..0896274209 100644
--- a/test/mp/test/foo.capnp
+++ b/test/mp/test/foo.capnp
@@ -37,5 +37,5 @@ interface FooInterface $Proxy.wrap("mp::test::FooImplementation") {
     callIntFnAsync @21 (context :Proxy.Context, arg :Int32) -> (result :Int32);
     passDataPointers @22 (arg :List(Data)) -> (result :List(Data));
-    listCallbacks @24 (context :Proxy.Context, n :Int32) -> (result :List(FooCallback));
+    listBars @24 (context :Proxy.Context, n :Int32) -> (result :List(BarInterface));
 }

@@ -49,4 +49,9 @@ interface ExtendedCallback extends(FooCallback) $Proxy.wrap("mp::test::ExtendedC
 }

+interface BarInterface $Proxy.wrap("mp::test::Bar") {
+    destroy @0 (context :Proxy.Context) -> ();
+    value @1 (context :Proxy.Context) -> (result :Int32);
+}
+
 interface FooFn $Proxy.wrap("ProxyCallback<std::function<int()>>") {
     destroy @0 (context :Proxy.Context) -> ();
diff --git a/test/mp/test/foo.h b/test/mp/test/foo.h
index 17aeeb8388..5c881117ce 100644
--- a/test/mp/test/foo.h
+++ b/test/mp/test/foo.h
@@ -60,13 +60,4 @@ public:
 };

-//! Concrete FooCallback that returns a fixed value, used by listCallbacks tests.
-class SimpleCallback : public FooCallback
-{
-public:
-    explicit SimpleCallback(int value) : m_value(value) {}
-    int call(int) override { return m_value; }
-    int m_value;
-};
-
 class ExtendedCallback : public FooCallback
 {
@@ -75,4 +66,27 @@ public:
 };

+//! A second, arbitrary interface used to test returning a
+//! list of interface objects.
+class Bar
+{
+public:
+    virtual ~Bar() = default;
+    virtual int value() = 0;
+};
+
+//! Concrete Bar that returns a fixed value, used by listBars tests.
+class SimpleBar : public Bar
+{
+public:
+    explicit SimpleBar(int value) : m_value(value) {}
+    int value() override { return m_value; }
+    int m_value;
+};
+
 class FooImplementation
 {
@@ -99,9 +113,9 @@ public:
     int passFn(std::function<int()> fn) { return fn(); }
     std::vector<FooDataRef> passDataPointers(std::vector<FooDataRef> values) { return values; }
-    std::vector<std::unique_ptr<FooCallback>> listCallbacks(int n)
+    std::vector<std::unique_ptr<Bar>> listBars(int n)
     {
-        std::vector<std::unique_ptr<FooCallback>> result;
+        std::vector<std::unique_ptr<Bar>> result;
         result.reserve(n);
-        for (int i = 0; i < n; ++i) result.push_back(std::make_unique<SimpleCallback>(i));
+        for (int i = 0; i < n; ++i) result.push_back(std::make_unique<SimpleBar>(i));
         return result;
     }
diff --git a/test/mp/test/test.cpp b/test/mp/test/test.cpp
index 41ca506c62..41aa4d67fd 100644
--- a/test/mp/test/test.cpp
+++ b/test/mp/test/test.cpp
@@ -263,9 +263,9 @@ KJ_TEST("Call FooInterface methods")
     // BuildList with interface element types, which requires non-const iteration
     // so unique_ptr::release() can transfer ownership to the proxy server.
-    std::vector<std::unique_ptr<FooCallback>> callbacks{foo->listCallbacks(3)};
-    KJ_REQUIRE(callbacks.size() == 3u);
+    std::vector<std::unique_ptr<Bar>> bars{foo->listBars(3)};
+    KJ_REQUIRE(bars.size() == 3u);
     for (int i = 0; i < 3; ++i) {
-        KJ_REQUIRE(callbacks[i] != nullptr);
-        KJ_EXPECT(callbacks[i]->call(0) == i);
+        KJ_REQUIRE(bars[i] != nullptr);
+        KJ_EXPECT(bars[i]->value() == i);
     }
 }

[ryanofsky]

re: #304 (comment)

IIUC this isn't really a callback flow, so the terminology had me confused about the test. Might be better to add another (Bar) interface to make it more clear.

Yeah you are right. The server is returning callback objects in the sense of "you can call me back later by using these objects" not taking callback objects that it can use to call into the client later. So your example makes more sense and its better to avoid the callback term here.

[ryanofsky]

Thanks for review. Looks like there are some things that should be improved here so I will make this a draft

[ryanofsky]

In commit "proxy: fix BuildList to use non-const iteration for interface types" (a0b01d7)

Note, while this change mostly restores behavior to what it was before #277, passing elem to BuildField below as an lvalue reference regardless of whether the container is lvalue or rvalue, it might make more sense to pass elem as an rvalue if the container is an rvalue, or as an lvalue if it's an lvalue with something like:

for (auto&& elem : value) {
    if constexpr (std::is_lvalue_reference_v<Value&&>) {
        BuildField(/*...*/, elem);
    } else {
        BuildField(/*...*/, std::move(elem));
    }
}

This could be safer because it could allow BuildField to distinguish based on the way the container is being used and only move from temporary containers that are about to be destroyed, not containers passed as references.

Also it might be better to replace for (auto&& elem : value) with for (auto it = elem.begin(); it != elem.end(); ++it) like the previous code had since this would could also preserve rvaluedness of dereferencing the iterator, if the iterator deferences as a proxy object instead of a reference like vector`

Which is just to say that a0b01d7 is a minimal change restoring ability to return lists of interface pointers, but it might make sense to actually increase safety here as well.

[ryanofsky]

re: #304 (comment)

IIUC this isn't really a callback flow, so the terminology had me confused about the test. Might be better to add another (Bar) interface to make it more clear.

Yeah you are right. The server is returning callback objects in the sense of "you can call me back later by using these objects" not taking callback objects that it can use to call into the client later. So your example makes more sense and its better to avoid the callback term here.