Skip to content

doc: Fix and expand design.md#278

Open
ViniciusCestarii wants to merge 1 commit into
bitcoin-core:masterfrom
ViniciusCestarii:doc-design-md-corrections
Open

doc: Fix and expand design.md#278
ViniciusCestarii wants to merge 1 commit into
bitcoin-core:masterfrom
ViniciusCestarii:doc-design-md-corrections

Conversation

@ViniciusCestarii
Copy link
Copy Markdown
Contributor

Improve two points on design.md:

  • Clarify that only ProxyClient is directly exposed to users and meant to be used directly and inherits from the C++ interface class so it can be used as a pointer to it.
  • Document the special destroy capnp method, which handles synchronous server-side object destruction.

@DrahtBot
Copy link
Copy Markdown

DrahtBot commented May 24, 2026

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

Reviews

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

Copy link
Copy Markdown
Collaborator

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review 6314aa1. Nice catches calling out two special cases that don't match the more general descriptions in the design doc.

I think the changes should be edited a little more for clarify, though, so left some suggestions below to consider

Comment thread doc/design.md Outdated
## Core Architecture

The `ProxyClient` and `ProxyServer` generated classes are not directly exposed to the user, as described in [usage.md](usage.md). Instead, they wrap C++ interfaces and appear to the user as pointers to an interface. They are first instantiated when calling `ConnectStream` and `ServeStream` respectively for creating the `InitInterface`. These methods establish connections through sockets, internally creating `Connection` objects wrapping a `capnp::RpcSystem` configured for client and server mode respectively.
The `ProxyServer` generated class is not directly exposed to the user. The `ProxyClient` generated class is exposed and made to be used directly, as described in [usage.md](usage.md), and inherits from the C++ interface class so it can be used as a pointer to it. They are first instantiated when calling `ConnectStream` and `ServeStream` respectively for creating the `InitInterface`. These methods establish connections through sockets, internally creating `Connection` objects wrapping a `capnp::RpcSystem` configured for client and server mode respectively.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "doc: Fix and expand design.md" (6314aa1)

Nice catch. Current text is inaccurate in claiming the ProxyClient type is not exposed at all, because the pointer returned to the initial remote object returned by ConnectStream is a ProxyClient<InitInterface> object. But this object isn't really meant to do anything other than inherit from InitInterface and implement its methods. So I think potentially ConnectStream could be changed to return an InitInterface pointer instead of a ProxyClient<InitInterface> and it wouldn't affect anything.

All other remote objects obtained by calling InitInterface methods will be ProxyClient<Foo> objects, but only exposed to user code as Foo pointers, so that aligns with current documentation. Current documentation should be mostly accurate except for the ConnectStream exception, and it is misleading to change it to say ProxyClient generated class meant to be is exposed and used more directly.

LLM suggests following which I think would be more accurate:

The ProxyServer generated class is not directly exposed to the user. The ProxyClient generated class inherits from the C++ interface class, so user code interacts with it through the abstract interface type — the ProxyClient type itself is generally not visible or accessible without a cast. ConnectStream returns a unique_ptr<ProxyClient<InitInterface>> as an exception for the root Init interface, but even there users typically treat it as a pointer to the abstract InitInterface. For all interfaces returned by Init methods (e.g., Printer, Calculator), the return type is the abstract class pointer, hiding the underlying ProxyClient entirely.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I wanted to make it not misleading but added another misleading

Fixed on (used your suggestion with a tweak: replaced the em dash with ", and"): 18db0ab

Comment thread doc/design.md Outdated

`ServerCall` uses the generated `ProxyMethod<MethodParams>::impl` pointer-to-member to invoke the actual C++ method on the wrapped implementation object.

A capnp interface can also declare a special `destroy` method, handled by `ServerDestroy` instead of `ServerCall`. Rather than dispatching to a C++ interface method, `ServerDestroy` calls `invokeDestroy()` on the `ProxyServer`, which resets `m_impl` and runs any registered cleanup functions, giving the client a way to synchronously destroy the wrapped object on the server side.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "doc: Fix and expand design.md" (6314aa1)

This is worth mentioning, but placement is awkward since it is interrupting the description of the flow for server method calls by describing a special case before the general case is described and shown in the diagram.

Would suggest moving the new paragraph after the mermaid diagram and tweaking text to connect it to previously described flow. LLM suggests following which I think is an improvement:

Destroy methods are a special case: a capnp interface can declare a destroy method that is handled by ServerDestroy instead of ServerCall. Rather than dispatching through the ServerField/ServerRet/ServerCall chain to a C++ interface method, ServerDestroy calls invokeDestroy() on the ProxyServer, which resets m_impl and runs any registered cleanup functions, giving the client a way to synchronously destroy the wrapped object on the server side.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree

Fixed on: 18db0ab

@ViniciusCestarii ViniciusCestarii force-pushed the doc-design-md-corrections branch from 6314aa1 to 18db0ab Compare June 3, 2026 13:05
@ViniciusCestarii
Copy link
Copy Markdown
Contributor Author

Forced-push 18db0ab to add suggested changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants