Skip to content
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.

Sign up for GitHub

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

Jump to bottom

rpcserver: decouple from server. #1730

Merged
merged 1 commit into from
Oct 22, 2019
Merged

rpcserver: decouple from server. #1730

merged 1 commit into from
Oct 22, 2019

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented May 3, 2019 •
edited

This pr depends on #1728 and is a port of btcd 981.

This decouples the RPC server from the internal dcrd server to move
closer to being able to split it out into a separate package.

In order to accomplish this, it introduces an rpcserverConfig type and
several new interfaces, named rpcserverPeer, rpcserverConnManager, and
rpcserverBlockManager, which are necessary to break the direct
dependencies on the main server and block manager instances.

It also adds concrete implementations of the new interfaces and uses
them to configure the RPC server.

Ultimately, the RPC server should ideally be decoupled even more such
that all of the types in the configuration struct use interfaces instead
of the concrete types. Doing this would make the RPC server much easier
to internally test since it would allow creating lightweight stubs for
the various pieces.

@dnldd dnldd force-pushed the merge_decouple_rpc_server branch from 18bbd28 to 1ea0d0c Compare May 5, 2019 20:27
@dnldd dnldd changed the title [wip] rpcserver: Decouple from server. rpcserver: decouple from server. May 5, 2019
@dnldd dnldd force-pushed the merge_decouple_rpc_server branch from 1ea0d0c to 1806cb7 Compare May 15, 2019 21:50
@dnldd dnldd force-pushed the merge_decouple_rpc_server branch from 1806cb7 to 2bab1ab Compare June 26, 2019 01:05
@dnldd dnldd mentioned this pull request Jun 26, 2019
Merged
davecgh
davecgh reviewed Jun 26, 2019
View reviewed changes
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed about half of this so far. It's been faithful to my upstream PR up to the point I've reviewed. No additional comments yet.

davecgh
davecgh requested changes Jun 26, 2019
View reviewed changes
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't appear to have been tested hardly at all as it crashes with a very simple startup with only the rpcserver enabled and nothing else.

I'm going to halt all review until it has.

rpcadaptors.go Outdated Show resolved Hide resolved
@dnldd dnldd force-pushed the merge_decouple_rpc_server branch 2 times, most recently from 7b46fa8 to 8c46a9b Compare July 3, 2019 17:47
@davecgh davecgh added this to the 1.6.0 milestone Oct 19, 2019
davecgh
davecgh requested changes Oct 22, 2019
View reviewed changes
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the first round of review. I still need to test all of the affected RPCs to be sure everything is still working properly, but aside from the inline comments, this looks pretty good overall.

rpcadaptors.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
@dnldd
rpcserver: decouple from server.
59c9379 
This decouples the RPC server from the internal dcrd server to move
closer to being able to split it out into a separate package.

In order to accomplish this, it introduces an rpcserverConfig type and
several new interfaces, named rpcserverPeer, rpcserverConnManager, and
rpcserverBlockManager, which are necessary to break the direct
dependencies on the main server and block manager instances.

It also adds concrete implementations of the new interfaces and uses
them to configure the RPC server.

Ultimately, the RPC server should ideally be decoupled even more such
that all of the types in the configuration struct use interfaces instead
of the concrete types.  Doing this would make the RPC server much easier
to internally test since it would allow creating lightweight stubs for
the various pieces.
@davecgh
Copy link
Member

davecgh commented Oct 22, 2019 •
edited

The following is a list of the affected RPCs and relevant additional paths tested:

  • addnode disconnect (both with id and ip:port, should error on attempt to disconnect permanent peer)
  • addnode remove (both with id and ip:port, should error on attempt to remove non-permanent peer)
  • addnode connect (both with perm and temp peers)
  • createrawtransaction
  • createrawsstx
  • createrawssrtx
  • decoderawtransaction
  • decodescript
  • estimatesmartfee
  • estimatestakediff
  • existsaddress, existsaddresses
  • existsexpiredtickets
  • existsmissedtickets
  • existsliveticket, existslivetickets
  • existsmempooltxs
  • generate
  • getaddednodeinfo
  • getbestblock
  • getbestblockhash
  • getblock (both with main and side chain blocks)
  • getblockchaininfo
  • getblockcount
  • getblockhash
  • getblockheader (both with main and side chain blocks)
  • getblocksubsidy
  • getcfilter (with cfilter index on and off both with main and side chain blocks)
  • getcfilterheader (with cfilter index on and off both with main and side chain blocks)
  • getcfilterv2 (both with main and side chain blocks)
  • getchaintips
  • getcoinsupply
  • getconnectioncount
  • getcurrentnet
  • getdifficulty
  • getgenerate
  • gethashespersec
  • getheaders
  • getinfo
  • getmempoolinfo
  • getnettotals
  • getnetworkhashps
  • getnetworkinfo
  • getpeerinfo
  • getrawmempool
  • getrawtransaction (with tx in mempool, then with tx NOT in mempool both with txindex on and off)
  • getstakedifficulty
  • getstakeversioninfo
  • getticketpoolvalue
  • getvoteinfo
  • gettxout (with tx in mempool and NOT in mempool)
  • getwork (request and submission, should error with: cpuminer running, no connections, and when chain is not current)
  • livetickets
  • missedtickets
  • node (same cases as addnode)
  • ping
  • searchrawtransactions (with address index on and off)
  • sendrawtransaction
  • setgenerate (stop, change number of workers)
  • submitblock
  • ticketfeeinfo
  • ticketsforaddress
  • ticketvwap
  • txfeeinfo
  • validateaddress
  • verifychain (level 0 and 1)
  • verifymessage

@dnldd
Copy link
Member Author

dnldd commented Oct 22, 2019

Thanks for the detailed list, should be at it soon 👍

@davecgh davecgh mentioned this pull request Oct 22, 2019
Closed
@dnldd
Copy link
Member Author

dnldd commented Oct 22, 2019

rpcs are looking good at my end as well.

@davecgh davecgh merged commit 284a773 into decred:master Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davecgh davecgh davecgh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.6.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dnldd @davecgh