[Modularization] add possibility to extend or overwrite RPC calls #5744

jonasschnelli · 2015-02-03T19:46:07Z

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.

According to bitcoin#3440 it would make sense to decouple the wallet and the miner (and maybe other things) from the init-/shutdown-process, etc. This is related to bitcoin#5686, bitcoin#5744, bitcoin#5745

jtimon · 2015-02-13T19:55:56Z

ut ACK

sipa · 2015-02-13T19:57:32Z

src/rpcserver.cpp

+void CRPCTable::AddOrReplaceCommand(const CRPCCommand command)
+{
+    // search after existing key in hashmap and remove it
+    map<string, const CRPCCommand*>::iterator it = mapCommands.find(command.name);


Why is this lookup needed? You can just overwrite using the line you have below?

Hmm... yes. Should be sufficient to just overwrite. Will change this.

dexX7 · 2015-03-04T22:07:43Z

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]), ...

jonasschnelli · 2015-03-05T08:14:53Z

@dexX7 Indeed. The vRPCConvertParams needs also modularization. I'll check your implementation approach and will try to update this PR.

jonasschnelli · 2015-03-06T10:03:03Z

@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.

dexX7 · 2015-03-07T09:51:19Z

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:

modularization-json-conversion-3: one function to add new entries
modularization-json-conversion-2: expose conversion class and table without param class
modularization-json-conversion-1: expose conversion classes and table

As you may see, I'm a bit lost and undecided, design wise, but I hope it helps to push this further. :)

jonasschnelli · 2015-03-13T15:53:56Z

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 · 2015-03-13T19:14:36Z

Adding new commands is still an issue for me, if the scope is left. For example: https://gist.github.com/dexX7/32accc4b04cb2e7dd496

jonasschnelli · 2015-03-13T21:03:59Z

@dexX7 Thanks for the response. Yes. Bad memory management. Will have a look at it.

jonasschnelli · 2015-03-14T08:25:54Z

@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.

dexX7 · 2015-03-14T11:49:25Z

src/rpcserver.h

+     * add or replace a CRPCCommand to the dispatch table
+     * @param command rpc command to add or replace
+     */ 
+    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: dexX7@57b252e).

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.

You are totally right. It must be 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.

Using pointers signals the responsibility of managing object lifetime is in the hand of the caller.

includes bitcoin#5744

sipa · 2015-06-16T15:05:01Z

Needs rebase.

jonasschnelli · 2015-06-16T15:23:58Z

@sipa: i think this solution is more flexible (and also less risky): #6006. Closing this.

includes bitcoin#5744

jonasschnelli force-pushed the 2015/02/rpc_flex_1 branch from 2e08088 to ea66fb4 Compare February 3, 2015 20:22

laanwj added the RPC/REST/ZMQ label Feb 4, 2015

jonasschnelli mentioned this pull request Feb 4, 2015

Introduce basic modularization #5752

Closed

This was referenced Feb 5, 2015

[Wallet] improve/replace wallet #5761

Closed

[Wallet] replace BDB with internal append only (logdb) backend #5686

Closed

sipa reviewed Feb 13, 2015
View reviewed changes

arnuschky mentioned this pull request Feb 20, 2015

[Wallet] Wallet-pruning #5389

Closed

jonasschnelli and others added 3 commits March 13, 2015 17:45

[Modularization] add possibility to extend or overwrite RPC calls

3c3711e

- this will allow a upcomming modularization of the wallet or other components

[Modularization] allow to add entries to the JSON conversion table

5537c08

- add method to insert custom conversion entries Conflicts: src/test/rpc_tests.cpp

[Modularization] flexible RPC table overhaul

bbee2eb

jonasschnelli force-pushed the 2015/02/rpc_flex_1 branch from f5a0e15 to bbee2eb Compare March 13, 2015 16:45

dexX7 reviewed Mar 14, 2015
View reviewed changes

[Modularization] use pointers when adding or replacing RPC commands

dac9278

Using pointers signals the responsibility of managing object lifetime is in the hand of the caller.

jonasschnelli added a commit to jonasschnelli/bitcoin that referenced this pull request Mar 21, 2015

[Refactor] decouple wallet rpc commands

674fc82

includes bitcoin#5744

jonasschnelli mentioned this pull request Mar 21, 2015

[Refactor] decouple wallet rpc commands #5934

Closed

jonasschnelli added a commit to jonasschnelli/bitcoin that referenced this pull request Mar 21, 2015

[Refactor] decouple wallet rpc commands

07c3027

includes bitcoin#5744

jonasschnelli added a commit to jonasschnelli/bitcoin that referenced this pull request Apr 12, 2015

[Refactor] decouple wallet rpc commands

d41f25a

includes bitcoin#5744

This was referenced Apr 12, 2015

[RPC] add possibility to extend the JSONRPC URI schemas #6006

Closed

[PoC/Concept review] add 2nd wallet "corewallet" #6008

Closed

jonasschnelli closed this Jun 16, 2015

jonasschnelli added a commit to jonasschnelli/bitcoin that referenced this pull request Jun 24, 2015

[Refactor] decouple wallet rpc commands

a6b659a

includes bitcoin#5744

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Modularization] add possibility to extend or overwrite RPC calls #5744

[Modularization] add possibility to extend or overwrite RPC calls #5744

Uh oh!

jonasschnelli commented Feb 3, 2015

Uh oh!

jtimon commented Feb 13, 2015

Uh oh!

sipa Feb 13, 2015

Uh oh!

jonasschnelli Feb 13, 2015

Uh oh!

dexX7 commented Mar 4, 2015

Uh oh!

jonasschnelli commented Mar 5, 2015

Uh oh!

jonasschnelli commented Mar 6, 2015

Uh oh!

dexX7 commented Mar 7, 2015

Uh oh!

jonasschnelli commented Mar 13, 2015

Uh oh!

dexX7 commented Mar 13, 2015

Uh oh!

jonasschnelli commented Mar 13, 2015

Uh oh!

jonasschnelli commented Mar 14, 2015

Uh oh!

dexX7 Mar 14, 2015

Uh oh!

jonasschnelli Mar 14, 2015

Uh oh!

sipa commented Jun 16, 2015

Uh oh!

jonasschnelli commented Jun 16, 2015

Uh oh!

Uh oh!

[Modularization] add possibility to extend or overwrite RPC calls #5744

[Modularization] add possibility to extend or overwrite RPC calls #5744

Uh oh!

Conversation

jonasschnelli commented Feb 3, 2015

Uh oh!

jtimon commented Feb 13, 2015

Uh oh!

sipa Feb 13, 2015

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Feb 13, 2015

Choose a reason for hiding this comment

Uh oh!

dexX7 commented Mar 4, 2015

Uh oh!

jonasschnelli commented Mar 5, 2015

Uh oh!

jonasschnelli commented Mar 6, 2015

Uh oh!

dexX7 commented Mar 7, 2015

Uh oh!

jonasschnelli commented Mar 13, 2015

Uh oh!

dexX7 commented Mar 13, 2015

Uh oh!

jonasschnelli commented Mar 13, 2015

Uh oh!

jonasschnelli commented Mar 14, 2015

Uh oh!

dexX7 Mar 14, 2015

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Mar 14, 2015

Choose a reason for hiding this comment

Uh oh!

sipa commented Jun 16, 2015

Uh oh!

jonasschnelli commented Jun 16, 2015

Uh oh!

Uh oh!