-
Notifications
You must be signed in to change notification settings - Fork 289
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
rpcserver: Move to internal. #2288
Conversation
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.
Looks good.
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.
Thank you for splitting it up into well crafted commits! It makes it so much easier to review these larger changesets that move things around since each step can be logically followed and reviewed for correctness along the way.
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.
I noticed that the init
function in rpcserver.go
still has a call to initialize the math/rand
RNG state despite nothing using it anymore. That should be removed.
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.
I would also rename NewRPCServer
to New
since it'll read rpcserver.New
instead of stuttering. Likewise with RPCserver
-> Server
, so it becomes rpcserver.Server
which is still has a little bit of stutter, but not as bad as rpcserver.RPCServer
.
5cf7e78
to
c9d5e2c
Compare
Thanks for the reviews. Updated per the review comments and also rebased to the latest master. |
Also, I marked this as 1.6.0 since I plan to get it in for that given it will help with the release process. The goal is to avoid leaving anything in the root module that is likely to cause a major semver bump. |
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.
Looks like you accidentally committed .DS_Store
.
With the exception of the aforementioned extra committed file, I've reviewed all the updates to this point and they look good. |
c9d5e2c
to
4eecc21
Compare
Removed the unintentionally committed file and also reworked this commit to provide exported delegated functions for the |
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.
This is looking really good overall.
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.
Works and looks great. Awesome.
// These fields define the username and password for RPC connections and | ||
// limited RPC connections. | ||
RPCUser string | ||
RPCPass string | ||
RPCLimitUser string | ||
RPCLimitPass string |
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.
Not related to this pr, but would it be prudent to store the authsha
that these values become rather than keeping the plain-text in memory?
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.
From my understanding, this is not much of an issue since accessing the RPC server from a remote/untrusted network is not recommended. However, as a best practice, I agree that the exposure of plaintext passwords should be minimized (its also stored in plaintext in the config file and/or passed through the command line).
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.
Not a bad idea, but realistically, if someone has enough access to do that, it's probably game over in short order anyway.
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.
It's been fine for this long, so it's probably not an issue. The rpc password usually is something random anyway, not something personal.
4eecc21
to
c2b0a8f
Compare
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.
Everything looks good to go now.
I've verified that every individual commit builds and works properly and also the the entire PR works as expected via simnet.
Mind rebasing this one last time so I can merge this? I missed that it was going to conflict with the one I just merged. |
This removes the dependency that rpcserver has on the global config in the main module. It accomplishes this by passing the necessary config information through the rpcserverConfig struct. Additionally, this adds a Lookup method to the rpcserver.ConnManager interface. This is part of the effort to move rpcserver into a separate internal package.
This removes the dependency that rpcserver has on the const maxProtocolVersion and var userAgentVersion defined in server.go. It accomplishes this by passing these values through the rpcserverConfig struct. This is part of the effort to move rpcserver into a separate internal package.
This removes the dependency that rpcserver has on the supportedSubsystems and parseAndSetDebugLevels functions defined in config.go. It accomplishes this by creating and implementing a rpcserver.LogManager interface. This is part of the effort to move rpcserver into a separate internal package.
This removes the dependency that rpcserver has on the PeerNotifier interface defined in blockmanager.go. It accomplishes this by removing the call to AnnounceNewTransactions and instead calling the already defined ConnMgr.RelayTransactions interface method, followed by directly calling NotifyNewTransactions to notify websocket clients of the newly accepted transactions. This is part of the effort to move rpcserver into a separate internal package.
This exports `rpcServer`, `rpcserverConfig`, and `newRPCServer` from rpcserver.go and updates all references accordingly. This is part of the effort to move rpcserver into a separate internal package.
This adds the following exported RPCServer functions, which delegate to the corresponding underlying rpcwebsocket notification functions: - NotifyWork - NotifyStakeDifficulty - NotifyNewTickets - NotifyBlockConnected - NotifySpentAndMissedTickets - NotifyBlockDisconnected - NotifyReorganization - NotifyWinningTickets This is done so that blockmanager.go and server.go can still access these functions after rpcserver is moved to a separate internal package. This is part of the effort to move rpcserver into a separate internal package.
This moves `genCertPair` from rpcserver.go to server.go. `genCertPair` is only used by server.go and is being moved there in anticipation of rpcserver being moved to a separate internal package. This is part of the effort to move rpcserver into a separate internal package.
This moves rpcserver from the main module to a standalone internal package. Overview of the major changes: - Move the following files from the main module -> internal/rpcserver: - rpcserver.go - rpcserver_test.go - rpcserverhandlers_test.go - rpcserverhelp.go - rpcserverhelp_test.go - rpcwebsocket.go - Update the README.md and doc.go files - Update all import paths in the repository accordingly - Update `testDataPath` in rpcserverhandlers_test.go - Update all `rpcsLog` references to `log` - Define the following convenience functions and vars in rpcserver.go: - `normalizeAddress` - `directionString` - `zeroHash`
This renames RpcserverConfig to Config since this is now referenced as rpcserver.Config from outside of the rpcserver package.
This renames NewRPCServer to New since this is now referenced as rpcserver.New from outside of the rpcserver package.
This renames RPCServer to Server since this is now referenced as rpcserver.Server from outside of the rpcserver package.
This removes math/rand from rpcserver as it is being initialized but it is not being used anywhere.
c2b0a8f
to
67a6eb2
Compare
Rebased to latest master |
This moves rpcserver from the main module to a standalone internal package.
This is broken up into several commits to make the majority of the dependency updates separately from actually moving the files. Can be split into multiple PRs but leaving it as one for now since this likely won't be merged until post 1.6.0.
Closes #2263