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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31288.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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.
@ryanofsky can you further clarify what the difference in behavior is with an without a 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.
I moved 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
ACK b28972cd85e4472b386349d6cda8c233faeffd4f
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
Added the space.
Code review ACK b28972cd85e4472b386349d6cda8c233faeffd4f
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.
Re-ACK 9aa50152c1cfa1c41215b2b51ed7a516aa67137a
Code review ACK 9aa50152c1cfa1c41215b2b51ed7a516aa67137a
Will go ahead and merge even though it only has 2 acks, since it's basically just adding a line to a schema file. Having this merged should make the related PR easier to maintain and review.