mpgen: emit return value after output parameters

[w0xlt]

Disclaimer: I am not very familiar with this codebase, so I would appreciate feedback on whether this is a desirable change, or if there is a preferred way to handle this case.

This PR updates proxy generation for methods where the Cap’n Proto result field is declared before other output fields, but the corresponding C++ method has output parameters before its return value.

A concrete example is Bitcoin Core’s BlockTemplate.submitSolution IPC method. It currently returns:

submitSolution (...) -> (result: Bool);

But if we need to add rejection details:

submitSolution (...) -> (result: Bool, reason: Text, debug: Text);

The result field needs to stay first to preserve the existing wire field number for compatibility. But the C++ method naturally looks like:

bool submitSolution(..., std::string& reason, std::string& debug);

The issue is that the Cap’n Proto field order would be result, reason, debug, but the corresponding C++ order is reason, debug, result: reason and debug are output parameters, while result is the method return value. Current mpgen output follows the Cap’n Proto order, so it generates C++ code that passes the fields in the wrong order for this method signature.

This PR changes mpgen to emit non-return fields first, then emit the result return-value field last. That keeps Cap’n Proto schema order independent from C++ invocation order, allowing schemas to preserve wire compatibility while still mapping correctly to C++ signatures.

The test changes add a regression method:

addResultOut @23 (value :Int32) -> (result :Int32, text :Text);

mapped to:

int addResultOut(int value, std::string& text);

The test verifies both the C++ return value and output parameter are transmitted correctly.

[DrahtBot]

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #243 (mpgen: support primitive std::optional struct fields by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[ryanofsky]

This is a reasonable idea but I'd want to think about it.

In general there isn't any backwards binary compatibility when you change method parameters or default arguments. If you want to change a method while keeping binary compatibility, you can give the method a new ordinal and use a different name for the old one like:

diff

--- a/src/ipc/capnp/mining.capnp
+++ b/src/ipc/capnp/mining.capnp
@@ -36,9 +36,12 @@ interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") {
     getTxSigops @4 (context: Proxy.Context) -> (result: List(Int64));
     getCoinbaseTx @5 (context: Proxy.Context) -> (result: CoinbaseTx);
     getCoinbaseMerklePath @6 (context: Proxy.Context) -> (result: List(Data));
-    submitSolution @7 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (result: Bool);
+    submitSolution @10 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (reason: Text, debug: Text, result: Bool);
     waitNext @8 (context: Proxy.Context, options: BlockWaitOptions) -> (result: BlockTemplate);
     interruptWait @9() -> ();
+
+    # deprecated older methods
+    submitSolutionOld7 @7 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (result: Bool);
 }
 
 struct BlockCreateOptions $Proxy.wrap("node::BlockCreateOptions") {
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -75,7 +75,7 @@ public:
      *       the solved block is constructed and broadcast by multiple nodes
      *       (e.g. both the miner who constructed the template and the pool).
      */
-    virtual bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) = 0;
+    virtual bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase, std::string& reason, std::string& debug) = 0;
 
     /**
      * Waits for fees in the next block to rise, a new tip or the timeout.
@@ -94,6 +94,13 @@ public:
      * Interrupts the current wait for the next block template.
     */
     virtual void interruptWait() = 0;
+
+    // deprecated older methods
+    virtual bool submitSolutionOld7(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase)
+    {
+        std::string reason, debug;
+        return submitSolution(version, timestamp, nonce, coinbase, reason, debug);
+    }
 };
 
 //! Interface giving clients (RPC, Stratum v2 Template Provider in the future)

This approach was previously described https://mirror.b10c.me/bitcoin-bitcoin/34184/#discussion_r2770232149, and I think it's a pretty good solution because it allows making many different types of changes, not just adding output parameters.

This approach would also keep capnproto parameters in the same order as c++ parameters, instead of allowing the return value to be declared between input and output parameters. (I'm not sure it is better to allow different orders instead of requiring a single consistent order.)

I do think there are cases where we might want to allow capnproto and c++ declarations to diverge more. Libmultiprocess was originally written to forward c++ method calls between processes, not to allow c++ methods to be called from other languages. Now that this is a goal, it makes sense to allow capnproto declarations to be written more freely and not directly mirror the c++ declarations. But I'd lean towards allowing this through $Proxy.xxx(...) annotations, so by default there's still strictly enforced consistency between c++ and capnproto, but it's possible to override those requirements. For example there could be a $Proxy.retval annotation allowing a result field to be mapped to the c++ return value while not needing to be called result or be the last field.

[DrahtBot]

🐙 This pull request conflicts with the target branch and needs rebase.