This ensures that if a client no longer needs a block template, the node can clear its memory as soon as possible.
A block template may hold on to transactions that are no longer in the mempool, so this can be significant.
This has a trivial silent merge conflict with #31283 because it also used the number @9 (gaps are not allowed). I’ll rebase whichever is merged last.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31288.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | TheCharlatan, ryanofsky |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
destroy method?
32@@ -33,6 +33,7 @@ interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") {
33 getWitnessCommitmentIndex @6 (context: Proxy.Context) -> (result: Int32);
34 getCoinbaseMerklePath @7 (context: Proxy.Context) -> (result: List(Data));
35 submitSolution@8 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (result: Bool);
36+ destroy @9 (context :Proxy.Context) -> ();
In commit “Add destroy to BlockTemplate schema” (5f7dc0dbe64eb38afc74e9c4cba489382ea373bd)
Would suggest making destroy the first method because it affect semantics of the object as a whole, and thats where I placed destroy in all the other interfaces. It is fine to keep the number @9 for it, or renumber if you prefer.
destroy to the top and renumbered.
Code review ACK 5f7dc0dbe64eb38afc74e9c4cba489382ea373bd
@ryanofsky can you further clarify what the difference in behavior is with an without a
destroymethod?
I think it probably doesn’t matter too much in this case, since the only thing the destroy method will currently do is free memory. But this does help future proof the interface, in case we want the destructor to do more things in the future, or if we add memory limits and want to give clients a way to ensure that previous block templates were completely freed before they create new ones.
The difference technically between having a destroy method and not having one is that when a libmultiprocess c++ IPC client destroys an object, it will automatically call the destroy method and wait for the server object to be destroyed before returning to client code. Without a destroy method, the C++ client object will be destroyed immediately without waiting for the server object to be destroyed. There’s some documentation about this in https://github.com/chaincodelabs/libmultiprocess/blob/abe254b9734f2e2b220d1456de195532d6e6ac1e/include/mp/proxy.h#L80-L91
40+ getTxSigops @4 (context: Proxy.Context) -> (result: List(Int64));
41+ getCoinbaseTx @5 (context: Proxy.Context) -> (result: Data);
42+ getCoinbaseCommitment @6 (context: Proxy.Context) -> (result: Data);
43+ getWitnessCommitmentIndex @7 (context: Proxy.Context) -> (result: Int32);
44+ getCoinbaseMerklePath @8 (context: Proxy.Context) -> (result: List(Data));
45+ submitSolution@9 (context: Proxy.Context, version: UInt32, timestamp: UInt32, nonce: UInt32, coinbase :Data) -> (result: Bool);
In commit “Add destroy to BlockTemplate schema” (b28972cd85e4472b386349d6cda8c233faeffd4f)
note: Missing a space before the number this line
This ensures that if a client no longer needs a block template,
the node can clear its memory as soon as possible.
A block template may hold on to transactions that are no longer
in the mempool, so this can be significant.