This will allow a upcomming modularization of the wallet or other components. Currently there is no use of the flexible RPC dispatch table.
Because #5686 is getting to big, it might be a better approach to slice out independent changes which can be easily reviewed and tested.
367@@ -368,6 +368,16 @@ const CRPCCommand *CRPCTable::operator[](string name) const
368 return (*it).second;
369 }
370
371+void CRPCTable::AddOrReplaceCommand(const CRPCCommand command)
372+{
373+ // search after existing key in hashmap and remove it
374+ map<string, const CRPCCommand*>::iterator it = mapCommands.find(command.name);
367@@ -368,6 +368,16 @@ const CRPCCommand *CRPCTable::operator[](string name) const
368 return (*it).second;
369 }
370
371+void CRPCTable::AddOrReplaceCommand(const CRPCCommand command)
372+{
373+ // search after existing key in hashmap and remove it
Hey @jonasschnelli,
this is a very interesting idea. I faced some issues (segfaults and std::bad_alloc exceptions), probably related to object lifetime, and ended up passing const CRPCCommand* command, where the pointer is maintaned externally, which seemed more intuitive.
From a practical perspective, value conversion is also a relevant topic, and editing vRPCConvertParams in rpcclient.cpp fails the goal of modularity, so I created json_conversion.h, to skip conversion tables and it can be used directly on Value objects with Cast<bool>(param[0]), Cast<int16_t>(param[1]), …
vRPCConvertParams needs also modularization. I’ll check your implementation approach and will try to update this PR.
@dexX7 i played around with your json_conversion.h implementation and it seems to be useful. How would you solution to the call RPCTypeCheck look like? I assume when skipping the vRPCConvertParams conversion the type check no longer works?
We could get rid of the conversion via vRPCConvertParams but this would generate some changes (problem of testing/reviewing). Maybe the json_conversion.h should only be used for new calls.
Good point, I didn’t really intend to replace the old system, but it was more like a quick workaround-ish solution, although I have to admit, I don’t like having a conversion table and still be required specify data types (e.g. get_str(), get_int(), …).
I pushed three more conservative and similar, but slightly different each, approaches:
As you may see, I’m a bit lost and undecided, design wise, but I hope it helps to push this further. :)
pulled in @dexX7 conversion table work. Fixed code nits mentioned by @sipa .
A flexible RPC interface is required if we like to add capability to switch on/off certain modules. It’s also totally required to factor out/decouple the wallet from the rest of the code. It could be a intermediate stop in case of a new RPC implementation. But for now i think this is a feasible way. Reviewing this means helping bitcoind to get a new wallet-implenentation while a legacy wallet could still be enabled.
- this will allow a upcomming modularization of the wallet or other components
- add method to insert custom conversion entries
Conflicts:
src/test/rpc_tests.cpp
@dexX7 i reviewed you gist https://gist.github.com/dexX7/32accc4b04cb2e7dd496. I would say the PR is acting correct.
When adding a rpc command over the new call AddOrReplaceCommand() you have to make sure that your CRPCCommand struct stays in memory. In your gist you create your CRPCCommand within your function void addTestCommand() but then test it in another block. const CRPCCommand newCmd get freed after exiting void addTestCommand().
I think one need to take the same approach to add commands to the vRPCCommands[] table as we do it for the standard rpc calls: static const CRPCCommand vRPCCommands[] = ....
This is not perfect but would give a RPC flexibility with a diff-size of +82/−2.
Using pointers signals the responsibility of managing object lifetime is in the hand of the caller.
113@@ -114,6 +114,12 @@ class CRPCTable
114 std::string help(std::string name) const;
115
116 /**
117+ * add or replace a CRPCCommand to the dispatch table
118+ * @param command rpc command to add or replace
119+ */
120+ void AddOrReplaceCommand(const CRPCCommand command);
The devil is in the details here, I think. :) const CRPCCommand command creates copy of the original command, and the caller can maintain the original for the whole execution lifetime, but it would still break (try: the updated gist).
It’s a slightly different story with RPCAddConversion(const std::string& method, int idx), because the lifetime the method name seems to be extended, where Temporary object lifetime, note 1 appears to apply to my understanding.
Upfront: using const CRPCCommand& command works, but the caller must still take care of the lifetime, which is not very intuitive imho, as one might be tempted to believe it’s behavior is similar to the const std::string& s case, thus my comment about passing a pointer const CRPCCommand* instead, which makes it very clear that it’s the caller’s responsibility (in code: https://github.com/dexX7/bitcoin/commit/57b252e4c7665542dd6ff92b7e9aafab77d8811a).
Messing around with object lifetime and pointers is a bit cumbersome and error prone in general in my opinion, so it might be worth to overthink the whole approach of storing pointers in mapCommands, but on the other hand, keeping changes minimal and doing it step by step probably yields results more quickly.
const CRPCCommand& command (passing pointers). I just pulled in your commit.
I’m also aware that this pointer-passing-approche is not intuitive. But i’m still trying to keep the changeset-size at minimum. I also think a rpc calls will be added when the app starts (modules get loaded) then they stay alive until the app detects a shutdown so the lifetime management tend to be not so complex.
Labels
RPC/REST/ZMQ