-
Notifications
You must be signed in to change notification settings - Fork 39.2k
multiprocess: Add capnp wrapper for Chain interface #29409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| // Copyright (c) The Bitcoin Core developers | ||
| // Distributed under the MIT software license, see the accompanying | ||
| // file COPYING or https://opensource.org/license/mit. | ||
|
|
||
| #ifndef BITCOIN_IPC_CAPNP_CHAIN_TYPES_H | ||
| #define BITCOIN_IPC_CAPNP_CHAIN_TYPES_H | ||
|
|
||
| #include <coins.h> | ||
| #include <ipc/capnp/chain.capnp.proxy.h> | ||
| #include <ipc/capnp/common.capnp.proxy-types.h> | ||
| #include <ipc/capnp/handler.capnp.proxy-types.h> | ||
| #include <interfaces/chain.h> | ||
| #include <kernel/types.h> | ||
| #include <policy/fees/block_policy_estimator.h> | ||
| #include <primitives/block.h> | ||
| #include <rpc/server.h> | ||
|
|
||
| //! Specialization of handleRpc needed because it takes a CRPCCommand& reference | ||
| //! argument, so a manual cleanup callback is needed to free the passed | ||
| //! CRPCCommand struct and proxy ActorCallback object. | ||
| template <> | ||
| struct mp::ProxyServerMethodTraits<ipc::capnp::messages::Chain::HandleRpcParams> | ||
| { | ||
| using Context = ServerContext<ipc::capnp::messages::Chain, | ||
| ipc::capnp::messages::Chain::HandleRpcParams, | ||
| ipc::capnp::messages::Chain::HandleRpcResults>; | ||
| static ::capnp::Void invoke(Context& context); | ||
| }; | ||
|
|
||
| //! Specialization of start method needed to provide CScheduler& reference | ||
| //! argument. | ||
| template <> | ||
| struct mp::ProxyServerMethodTraits<ipc::capnp::messages::ChainClient::StartParams> | ||
| { | ||
| using ChainContext = ServerContext<ipc::capnp::messages::ChainClient, | ||
| ipc::capnp::messages::ChainClient::StartParams, | ||
| ipc::capnp::messages::ChainClient::StartResults>; | ||
| static void invoke(ChainContext& context); | ||
| }; | ||
|
|
||
| namespace mp { | ||
| //! Overload CustomBuildMessage, CustomPassMessage, and CustomReadMessage to | ||
| //! serialize interfaces::FoundBlock parameters. Custom conversion functions are | ||
| //! needed because there is not a 1:1 correspondence between members of the C++ | ||
| //! FoundBlock class and the Cap'n Proto FoundBlockParam and FoundBlockResult | ||
| //! structs. Separate param and result structs are needed because Cap'n Proto | ||
| //! only has input and output parameters, not in/out parameters like C++. | ||
| void CustomBuildMessage(InvokeContext& invoke_context, | ||
| const interfaces::FoundBlock& dest, | ||
| ipc::capnp::messages::FoundBlockParam::Builder&& builder); | ||
| void CustomPassMessage(InvokeContext& invoke_context, | ||
| const ipc::capnp::messages::FoundBlockParam::Reader& reader, | ||
| ipc::capnp::messages::FoundBlockResult::Builder&& builder, | ||
| std::function<void(const interfaces::FoundBlock&)>&& fn); | ||
| void CustomReadMessage(InvokeContext& invoke_context, | ||
| const ipc::capnp::messages::FoundBlockResult::Reader& reader, | ||
| const interfaces::FoundBlock& dest); | ||
|
|
||
| //! Overload CustomBuildMessage and CustomPassMessage to serialize | ||
| //! interfaces::BlockInfo parameters. Custom conversion functions are needed | ||
| //! because BlockInfo structs contain pointer and reference members, and custom | ||
| //! code is needed to deal with their lifetimes. Specifics are described in the | ||
| //! implementation of these functions. | ||
| void CustomBuildMessage(InvokeContext& invoke_context, | ||
| const interfaces::BlockInfo& block, | ||
| ipc::capnp::messages::BlockInfo::Builder&& builder); | ||
| void CustomPassMessage(InvokeContext& invoke_context, | ||
| const ipc::capnp::messages::BlockInfo::Reader& reader, | ||
| ::capnp::Void builder, | ||
| std::function<void(const interfaces::BlockInfo&)>&& fn); | ||
|
|
||
| //! CScheduler& server-side argument handling. Skips argument so it can | ||
| //! be handled by ProxyServerMethodTraits ChainClient::Start code. | ||
| template <typename Accessor, typename ServerContext, typename Fn, typename... Args> | ||
| void CustomPassField(TypeList<CScheduler&>, ServerContext& server_context, const Fn& fn, Args&&... args) | ||
| { | ||
| fn.invoke(server_context, std::forward<Args>(args)...); | ||
| } | ||
|
|
||
| //! CRPCCommand& server-side argument handling. Skips argument so it can | ||
| //! be handled by ProxyServerMethodTraits Chain::HandleRpc code. | ||
| template <typename Accessor, typename ServerContext, typename Fn, typename... Args> | ||
| void CustomPassField(TypeList<const CRPCCommand&>, ServerContext& server_context, const Fn& fn, Args&&... args) | ||
| { | ||
| fn.invoke(server_context, std::forward<Args>(args)...); | ||
| } | ||
|
|
||
| //! Override to avoid assert failures that would happen trying to serialize | ||
| //! spent coins. Probably it would be best for Coin serialization code not | ||
| //! to assert, but avoiding serialization in this case is harmless. | ||
| bool CustomHasValue(InvokeContext& invoke_context, const Coin& coin); | ||
|
|
||
| //! Overload CustomBuildMessage and CustomReadMessage to serialize HelpResult. | ||
| inline void CustomBuildMessage(InvokeContext& invoke_context, const HelpResult& dest, ipc::capnp::messages::HelpResult::Builder&& builder) | ||
| { | ||
| builder.setMessage(dest.what()); | ||
| } | ||
|
|
||
| template <typename Input> | ||
| void ThrowField(TypeList<HelpResult>, InvokeContext& invoke_context, Input&& input) | ||
| { | ||
| auto data = input.get().getMessage(); | ||
| throw HelpResult(std::string(CharCast(data.begin()), data.size())); | ||
| } | ||
| } // namespace mp | ||
|
|
||
| #endif // BITCOIN_IPC_CAPNP_CHAIN_TYPES_H |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,173 @@ | ||||||||||
| # Copyright (c) The Bitcoin Core developers | ||||||||||
| # Distributed under the MIT software license, see the accompanying | ||||||||||
| # file COPYING or https://opensource.org/license/mit. | ||||||||||
|
|
||||||||||
| @0x94f21a4864bd2c65; | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add a simple explanation comment for these salts?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #29409 (comment)
Not sure about this. Is there a comment you would suggest? I feel like ids are built into the syntax of Cap'n Proto and ubiquitous. If you aren't familiar with Cap'n Proto, I think a comment here would probably just distract you from more semantically relevant parts of the file and leave you confused. Commenting on IDs in capnproto seems like commenting on #ifndef include guards at the top of c++ header files. It would be hard to know where to begin or what information would be helpful. |
||||||||||
|
|
||||||||||
| using Cxx = import "/capnp/c++.capnp"; | ||||||||||
| $Cxx.namespace("ipc::capnp::messages"); | ||||||||||
|
|
||||||||||
| using Proxy = import "/mp/proxy.capnp"; | ||||||||||
| $Proxy.include("interfaces/chain.h"); | ||||||||||
| $Proxy.include("rpc/server.h"); | ||||||||||
| $Proxy.includeTypes("ipc/capnp/chain-types.h"); | ||||||||||
|
|
||||||||||
| using Common = import "common.capnp"; | ||||||||||
| using Handler = import "handler.capnp"; | ||||||||||
|
|
||||||||||
| interface Chain $Proxy.wrap("interfaces::Chain") { | ||||||||||
| destroy @0 (context :Proxy.Context) -> (); | ||||||||||
| getHeight @1 (context :Proxy.Context) -> (result :Int32, hasResult :Bool); | ||||||||||
|
Comment on lines
+19
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder how schema evolution is handled by Cap'n Proto - what happens if a field is deprecated and removed or new values are inserted, do I have to check all existing values to make sure I can find a new valid id? https://capnproto.org/language.html#evolving-your-protocol claims:
So I guess if you change any value and deprecate the old one you will have to add the new version with max + 1 - wanted to suggest that we could add 10 opportunities between values like: so that when we change but it appears that's not possible.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #29409 (comment)
Yes you do. You need to pick the next unused ID. I think it actually works pretty well in practice, especially if the id's are mostly in order, but it's also possible to include
Yes like you point out, gaps are not allowed. Technically I think the compiler could allow gaps for interfaces, but for structs it couldn't work because the order fields are added to the struct defines its binary layout. For interfaces the restriction also seems reasonable to make interfaces consistent with structs, and make it possible to see what order methods were added over time. Since it's also possible to move and rename deprecated fields, this restriction doesn't get in the way of making capnp files readble. Renaming also lets you break source compatibility and require source code to be updated, while maintaining binary compatibility so old binaries and code using previous definitions continue to work. In general, I think the system is very flexible and functions pretty nicely. |
||||||||||
| getBlockHash @2 (context :Proxy.Context, height :Int32) -> (result :Data); | ||||||||||
| haveBlockOnDisk @3 (context :Proxy.Context, height :Int32) -> (result :Bool); | ||||||||||
| findLocatorFork @4 (context :Proxy.Context, locator :Data) -> (result :Int32, hasResult :Bool); | ||||||||||
| hasBlockFilterIndex @5 (context :Proxy.Context, filterType :UInt8) -> (result :Bool); | ||||||||||
| blockFilterMatchesAny @6 (context :Proxy.Context, filterType :UInt8, blockHash :Data, filterSet :List(Data)) -> (result :Bool, hasResult :Bool); | ||||||||||
| findBlock @7 (context :Proxy.Context, hash :Data, block :FoundBlockParam) -> (block :FoundBlockResult, result :Bool); | ||||||||||
| findFirstBlockWithTimeAndHeight @8 (context :Proxy.Context, minTime :Int64, minHeight :Int32, block :FoundBlockParam) -> (block :FoundBlockResult, result :Bool); | ||||||||||
| findAncestorByHeight @9 (context :Proxy.Context, blockHash :Data, ancestorHeight :Int32, ancestor :FoundBlockParam) -> (ancestor :FoundBlockResult, result :Bool); | ||||||||||
| findAncestorByHash @10 (context :Proxy.Context, blockHash :Data, ancestorHash :Data, ancestor :FoundBlockParam) -> (ancestor :FoundBlockResult, result :Bool); | ||||||||||
| findCommonAncestor @11 (context :Proxy.Context, blockHash1 :Data, blockHash2 :Data, ancestor :FoundBlockParam, block1 :FoundBlockParam, block2 :FoundBlockParam) -> (ancestor :FoundBlockResult, block1 :FoundBlockResult, block2 :FoundBlockResult, result :Bool); | ||||||||||
| findCoins @12 (context :Proxy.Context, coins :List(Common.Pair(Data, Data))) -> (coins :List(Common.Pair(Data, Data))); | ||||||||||
| guessVerificationProgress @13 (context :Proxy.Context, blockHash :Data) -> (result :Float64); | ||||||||||
| hasBlocks @14 (context :Proxy.Context, blockHash :Data, minHeight :Int32, maxHeight: Int32, hasMaxHeight :Bool) -> (result :Bool); | ||||||||||
| isRBFOptIn @15 (context :Proxy.Context, tx :Data) -> (result :Int32); | ||||||||||
| isInMempool @16 (context :Proxy.Context, txid :Data) -> (result :Bool); | ||||||||||
| hasDescendantsInMempool @17 (context :Proxy.Context, txid :Data) -> (result :Bool); | ||||||||||
| broadcastTransaction @18 (context :Proxy.Context, tx: Data, maxTxFee :Int64, broadcastMethod :Int32) -> (error: Text, result :Bool); | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In 1c9dcf2 Also what happens here is
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #29409 (comment)
IIUC, the concern in both of these cases is that non-C++ clients calling this interface (like rust or python clients) could call this method but leave the This is a legitimate concern, and it's not specific to this method or to these parameters. In C++, the compiler enforces at each call site that explicit values need to be passed for all parameters which don't have defaults. But in capnproto, all parameters are optional because fields without explicit defaults have implicit defaults of 0 or null. The But if non-c++ other clients are used, they either need to be careful when they omit parameters, or the interface needs to be changed to allow parameters to be safely omitted. One way to do this is to assign default values in interfaces like c6638fa. Another way is to make method implementations handle invalid values better, for example by throwing exceptions instead of asserting. |
||||||||||
| getTransactionAncestry @19 (context :Proxy.Context, txid :Data) -> (ancestors :UInt64, descendants :UInt64, ancestorsize :UInt64, ancestorfees :Int64); | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In 1c9dcf2 even though these are positional, I think we can rename
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #29409 (comment)
Good catch. It looks like the parameter was renamed in commit 957ae23 and this PR was not updated. Currently there is checking to make sure capnproto parameter types and c++ types are compatible (compilation will fail otherwise) but there it isn't checked that parameters have the same names so sometimes names can get out of sync. |
||||||||||
| calculateIndividualBumpFees @20 (context :Proxy.Context, outpoints :List(Data), targetFeerate :Data) -> (result: List(Common.PairInt64(Data))); | ||||||||||
| calculateCombinedBumpFee @21 (context :Proxy.Context, outpoints :List(Data), targetFeerate :Data) -> (result :Int64, hasResult :Bool); | ||||||||||
| getPackageLimits @22 (context :Proxy.Context) -> (ancestors :UInt32, descendants :UInt32); | ||||||||||
| checkChainLimits @23 (context :Proxy.Context, tx :Data) -> (result :Common.ResultVoid); | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In 1c9dcf2 I wonder if this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #29409 (comment)
Yes this is the same issue. checkChainLimits calls CheckPolicyLimits which calls StageAddition which does not check whether And In this case, the issue applies to C++ clients as well as rust and python clients because C++ clients can also call checkChainLimits with a null pointer. Depending on what the goal is, there are various ways this could be addressed. One way would be to change checkChainLimits to throw an exception if a null pointer is passed. In general it's not a realistic goal for this PR to ensure that it's safe to call all Chain methods with all possible arguments and ensure that the node will not crash, but it should be good to do where feasible. |
||||||||||
| estimateSmartFee @24 (context :Proxy.Context, numBlocks :Int32, conservative :Bool, wantCalc :Bool) -> (calc :Common.FeeCalculation, result :Data); | ||||||||||
| estimateMaxBlocks @25 (context :Proxy.Context) -> (result :UInt32); | ||||||||||
| mempoolMinFee @26 (context :Proxy.Context) -> (result :Data); | ||||||||||
| relayMinFee @27 (context :Proxy.Context) -> (result :Data); | ||||||||||
| relayIncrementalFee @28 (context :Proxy.Context) -> (result :Data); | ||||||||||
| relayDustFee @29 (context :Proxy.Context) -> (result :Data); | ||||||||||
| havePruned @30 (context :Proxy.Context) -> (result :Bool); | ||||||||||
| getPruneHeight @31 (context :Proxy.Context) -> (result: Int32, hasResult: Bool); | ||||||||||
| isReadyToBroadcast @32 (context :Proxy.Context) -> (result :Bool); | ||||||||||
| isInitialBlockDownload @33 (context :Proxy.Context) -> (result :Bool); | ||||||||||
| shutdownRequested @34 (context :Proxy.Context) -> (result :Bool); | ||||||||||
| initMessage @35 (context :Proxy.Context, message :Text) -> (); | ||||||||||
| initWarning @36 (context :Proxy.Context, message :Common.BilingualStr) -> (); | ||||||||||
| initError @37 (context :Proxy.Context, message :Common.BilingualStr) -> (); | ||||||||||
| showProgress @38 (context :Proxy.Context, title :Text, progress :Int32, resumePossible :Bool) -> (); | ||||||||||
| handleNotifications @39 (context :Proxy.Context, notifications :ChainNotifications) -> (result :Handler.Handler); | ||||||||||
| waitForNotificationsIfTipChanged @40 (context :Proxy.Context, oldTip :Data) -> (); | ||||||||||
| waitForNotifications @41 (context :Proxy.Context) -> (); | ||||||||||
| handleRpc @42 (context :Proxy.Context, command :RPCCommand) -> (result :Handler.Handler); | ||||||||||
| rpcEnableDeprecated @43 (context :Proxy.Context, method :Text) -> (result :Bool); | ||||||||||
| getSetting @44 (context :Proxy.Context, name :Text) -> (result :Text); | ||||||||||
| getSettingsList @45 (context :Proxy.Context, name :Text) -> (result :List(Text)); | ||||||||||
| getRwSetting @46 (context :Proxy.Context, name :Text) -> (result :Text); | ||||||||||
| updateRwSetting @47 (context :Proxy.Context, name :Text, update: SettingsUpdateCallback) -> (result :Bool); | ||||||||||
| overwriteRwSetting @48 (context :Proxy.Context, name :Text, value :Text, action :Int32) -> (result :Bool); | ||||||||||
| deleteRwSettings @49 (context :Proxy.Context, name :Text, action: Int32) -> (result :Bool); | ||||||||||
| requestMempoolTransactions @50 (context :Proxy.Context, notifications :ChainNotifications) -> (); | ||||||||||
| hasAssumedValidChain @51 (context :Proxy.Context) -> (result :Bool); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| interface ChainNotifications $Proxy.wrap("interfaces::Chain::Notifications") { | ||||||||||
| destroy @0 (context :Proxy.Context) -> (); | ||||||||||
| transactionAddedToMempool @1 (context :Proxy.Context, tx :Data) -> (); | ||||||||||
| transactionRemovedFromMempool @2 (context :Proxy.Context, tx :Data, reason :Int32) -> (); | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Original has enums for bitcoin/src/interfaces/chain.h Line 323 in 5fe753b
should we make the param
Suggested change
Or vice versa, since But looking at #29409 (comment), you mentioned:
which was merged since - shouldn't it be a compile error now?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #29409 (comment)
Nice find noticing the inconsistency. Both types should be safe because casts between signed and unsigned types are well-defined. I tend to prefer signed types for enums so if the enums use -1, -2, -3 values they will be displayed nicely. But unsigned types could be better for some enums, particularly bitmasks. I updated chainstate role enums variables to be signed for consistency. |
||||||||||
| blockConnected @3 (context :Proxy.Context, role: ChainstateRole, block :BlockInfo) -> (); | ||||||||||
| blockDisconnected @4 (context :Proxy.Context, block :BlockInfo) -> (); | ||||||||||
| updatedBlockTip @5 (context :Proxy.Context) -> (); | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the use for an
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding this here for context: I guess one of the benefits of the |
||||||||||
| chainStateFlushed @6 (context :Proxy.Context, role: ChainstateRole, locator :Data) -> (); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| interface ChainClient $Proxy.wrap("interfaces::ChainClient") { | ||||||||||
| destroy @0 (context :Proxy.Context) -> (); | ||||||||||
| registerRpcs @1 (context :Proxy.Context) -> (); | ||||||||||
| verify @2 (context :Proxy.Context) -> (result :Bool); | ||||||||||
| load @3 (context :Proxy.Context) -> (result :Bool); | ||||||||||
| start @4 (context :Proxy.Context, scheduler :Void) -> (); | ||||||||||
| stop @5 (context :Proxy.Context) -> (); | ||||||||||
| setMockTime @6 (context :Proxy.Context, time :Int64) -> (); | ||||||||||
| schedulerMockForward @7 (context :Proxy.Context, deltaSeconds :Int64) -> (); | ||||||||||
| } | ||||||||||
|
Comment on lines
+83
to
+92
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of scope for this PR, just raising this out of curiosity: I'm working on an IPC extractor for peer-observer, a non-wallet client that consumes
Three of those methods feel pretty wallet-shaped: Would it be worth reducing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #29409 (comment)
It's a good question. The doc comment is too ambiguous and should be improved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I get it now, thanks you! |
||||||||||
|
|
||||||||||
| struct RPCCommand $Proxy.wrap("CRPCCommand") { | ||||||||||
| category @0 :Text; | ||||||||||
| name @1 :Text; | ||||||||||
| actor @2 :ActorCallback; | ||||||||||
| argNames @3 :List(RPCArg); | ||||||||||
| uniqueId @4 :Int64 $Proxy.name("unique_id"); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| # Representation of std::pair<std::string, bool> in CRPCCommand | ||||||||||
| struct RPCArg { | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this corresponds to Line 50 in 5fe753b
we might want to ment why this one doesn't have a corresponding proxy:
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #29409 (comment)
Good suggestion, added a version of this comment |
||||||||||
| name @0 :Text; | ||||||||||
| namedOnly @1: Bool; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| interface ActorCallback $Proxy.wrap("ProxyCallback<CRPCCommand::Actor>") { | ||||||||||
| call @0 (context :Proxy.Context, request :JSONRPCRequest, response :Text, lastCallback :Bool) -> (error :Text $Proxy.exception("std::exception"), rpcError :Text $Proxy.exception("UniValue"), typeError :Text $Proxy.exception("UniValue::type_error"), helpResult :HelpResult $Proxy.exception("HelpResult"), response :Text, result: Bool); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| struct HelpResult { | ||||||||||
| message @0 :Text; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| struct JSONRPCRequest $Proxy.wrap("JSONRPCRequest") { | ||||||||||
| id @0 :Text; | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #29409 (comment)
Yes, it is. The UniValue fields is serialized as JSON text here. And Cap'n Proto allows text fields to be null so |
||||||||||
| method @1 :Text $Proxy.name("strMethod"); | ||||||||||
| params @2 :Text; | ||||||||||
| mode @3 :Int32; | ||||||||||
| uri @4 :Text $Proxy.name("URI"); | ||||||||||
| authUser @5 :Text; | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this include the version as well at this point?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #29409 (comment)
Yes, good catch. Added the version field and added a comment to JSONRPCRequest struct to prevent this type of bug in the future. I think this bug did not matter too much in practice because this capnp struct is only used to forward RPC requests from the node to the wallet and wallet shouldn't respond to requests differently based on JSONRPC version, but it is still a bug in the serialization that should be fixed. |
||||||||||
| peerAddr @6 :Text; | ||||||||||
| version @7: Int32 $Proxy.name("m_json_version"); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| struct FoundBlockParam { | ||||||||||
| wantHash @0 :Bool; | ||||||||||
| wantHeight @1 :Bool; | ||||||||||
| wantTime @2 :Bool; | ||||||||||
| wantMaxTime @3 :Bool; | ||||||||||
| wantMtpTime @4 :Bool; | ||||||||||
| wantInActiveChain @5 :Bool; | ||||||||||
| wantLocator @6 :Bool; | ||||||||||
| nextBlock @7: FoundBlockParam; | ||||||||||
| wantData @8 :Bool; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| struct FoundBlockResult { | ||||||||||
| hash @0 :Data; | ||||||||||
| height @1 :Int32; | ||||||||||
| time @2 :Int64; | ||||||||||
| maxTime @3 :Int64; | ||||||||||
| mtpTime @4 :Int64; | ||||||||||
| inActiveChain @5 :Bool; | ||||||||||
| locator @6 :Data; | ||||||||||
| nextBlock @7: FoundBlockResult; | ||||||||||
| data @8 :Data; | ||||||||||
| found @9 :Bool; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| struct BlockInfo $Proxy.wrap("interfaces::BlockInfo") { | ||||||||||
| # Fields skipped below with Proxy.skip are pointer fields manually handled | ||||||||||
| # by CustomBuildMessage / CustomPassMessage overloads. | ||||||||||
| hash @0 :Data $Proxy.skip; | ||||||||||
| prevHash @1 :Data $Proxy.skip; | ||||||||||
| height @2 :Int32 = -1; | ||||||||||
| fileNumber @3 :Int32 = -1 $Proxy.name("file_number"); | ||||||||||
| dataPos @4 :UInt32 = 0 $Proxy.name("data_pos"); | ||||||||||
| data @5 :Data $Proxy.skip; | ||||||||||
| undoData @6 :Data $Proxy.skip; | ||||||||||
| chainTimeMax @7 :UInt32 = 0 $Proxy.name("chain_time_max"); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| struct ChainstateRole $Proxy.wrap("kernel::ChainstateRole") { | ||||||||||
| validated @0 :Bool; | ||||||||||
| historical @1 :Bool; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| interface SettingsUpdateCallback $Proxy.wrap("ProxyCallback<interfaces::SettingsUpdate>") { | ||||||||||
| destroy @0 (context :Proxy.Context) -> (); | ||||||||||
| call @1 (context :Proxy.Context, value :Text) -> (value :Text, result: Int32, hasResult: Bool); | ||||||||||
| } | ||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to wait for
SettingsValueto be exposed for the other consts to be removed as well?Looks a bit weird that this isn't consted anymore, while other similar ones still are:
bitcoin/src/interfaces/node.h
Lines 111 to 116 in 5336bcd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #29409 (comment)
It looks like const references there could be removed, but did you mean to link to a different interface file? The node.h interface is used by the GUI, unlike the chain.h interface used by wallet. But it could be changed in a separate PR and I noted some other ideas for cleaning up the settings interfaces previously in #30697 (comment) and #30828 (review)