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.

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

node: refactor package node #21105

Merged
merged 160 commits into from
Aug 3, 2020
Merged

node: refactor package node #21105

merged 160 commits into from
Aug 3, 2020

Conversation

renaynay
Copy link
Contributor

@renaynay renaynay commented May 19, 2020

This PR significantly changes the APIs for instantiating Ethereum nodes in a Go program. The new APIs are not backwards-compatible, but we feel that this is made up for by the much simpler way of registering services on node.Node. You can find more information and rationale in the design document: https://gist.github.com/renaynay/5bec2de19fde66f4d04c535fd24f0775.

There is also a new feature in Node's Go API: it is now possible to register arbitrary handlers on the user-facing HTTP server. In geth, this facility is used to enable GraphQL.

There is a single minor change relevant for geth users in this PR: The GraphQL API is no longer available separately from the JSON-RPC HTTP server. If you want GraphQL, you need to enable it using the --http --graphql flag combination. The --graphql.port and --graphql.addr flags are no longer available.


[ + ] Introduces Lifecycle interface
[ + ] Introduces RegisterAPIs() method
[ + ] Introduces RegisterLifecycle() method
[ + ] Introduces RegisterHTTP() method
[ + ] Introduces RegisterProtocols() method

[<>] Changes the way information about node's HTTP servers is stored
[<>] Separates LES server management from eth.Ethereum, now managed by node
[<>] GraphQL can only be started on the node's canonical HTTP server (to enable graphql --graphql, the user must also enable http with the --http flag)

[ - ] Removes Service interface
[ - ] Removes ServiceContext
[ - ] Removes the responsibility of service construction

TODO

  • functional implementation of design
  • fix RegisterHTTP methods
  • general code clean-up
    • clean up code in cmd/geth/main.go
    • clean up services to which 2 backends are passed (where one is nil)
  • tests
  • make sure documentation is accurate
  • rebase
  • clean-up

@renaynay renaynay force-pushed the draft-refactor-node branch 2 times, most recently from 582c0aa to 124539d Compare June 9, 2020 12:57
@renaynay renaynay requested a review from fjl June 22, 2020 15:21
eth/backend.go Outdated Show resolved Hide resolved
node/service.go Outdated Show resolved Hide resolved
eth/backend.go Outdated Show resolved Hide resolved
@renaynay
Copy link
Contributor Author

renaynay commented Jun 24, 2020

Notes from review:

  • either add a method to get consensus.Engine from the ethapi.Backend or get it another way.
  • remove all "fetch backend" instances and try to remove the Lifecycle method where it fetches a lifecycle based on its type.
  • remove ServiceContext
  • change the panic of missing backend to a nicer fatal err
  • take another look at WSAllowed because there could be a resulting race condition
  • RegisterHTTP could possibly be done better and could maybe still follow this signature:
func (n *Node) RegisterHTTP(path string, h http.Handler) error
  • Also fix GQL server tests, they're still failing on some builds on travis.

ethstats/ethstats.go Outdated Show resolved Hide resolved
ethstats/ethstats.go Outdated Show resolved Hide resolved
internal/ethapi/backend.go Outdated Show resolved Hide resolved
cmd/geth/main.go Outdated Show resolved Hide resolved
cmd/geth/main.go Outdated Show resolved Hide resolved
@@ -457,24 +450,23 @@ func startNode(ctx *cli.Context, stack *node.Node) {
if ctx.GlobalString(utils.SyncModeFlag.Name) == "light" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think everything in this block is code that only works for full nodes.
We should create an auxiliary function for setting up *eth.Ethereum specifically
and then put a type assertion for *eth.Ethereum here.

@renaynay
Copy link
Contributor Author

renaynay commented Jul 1, 2020

HTTP Rule:

flags:
--http
--http.port
--http.cors
--http.vhosts
--http.graphql enables graphql on http server

  • can graphql be enabled without http enabled? NO
  • graphql can only be on http server, not on separate endpoint.

console:

  • if I do a stopRPC() in the console, what happens to enabled gql server? It also stops as well.

@renaynay
Copy link
Contributor Author

renaynay commented Jul 1, 2020

TODO:

  • ethstats gets own backend interface + full node backend then do type assertion (don't import internal ethapi)
  • atomic int32 wsAllowed
  • real type assertion for main.go so that configure eth vs les stuff in startNode
  • graphql can only be on http

extra:

  • SetContractBackend move this to eth / les configuration (ipc endpoint)

ethclient/ethclient.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
@renaynay renaynay requested a review from fjl July 10, 2020 13:51
@renaynay renaynay marked this pull request as ready for review July 10, 2020 13:51
@fjl fjl mentioned this pull request Aug 3, 2020
fjl added 2 commits August 3, 2020 15:01
I'm not happy with 'path' for this because we also have ResolvePath
to find file paths in the datadir.
@fjl fjl merged commit c0c0161 into ethereum:master Aug 3, 2020
@fjl fjl removed the status:triage label Aug 3, 2020
@fjl fjl added this to the 1.9.19 milestone Aug 4, 2020
enriquefynn pushed a commit to enriquefynn/go-ethereum that referenced this pull request Mar 10, 2021
This PR significantly changes the APIs for instantiating Ethereum nodes in
a Go program. The new APIs are not backwards-compatible, but we feel that
this is made up for by the much simpler way of registering services on
node.Node. You can find more information and rationale in the design
document: https://gist.github.com/renaynay/5bec2de19fde66f4d04c535fd24f0775.

There is also a new feature in Node's Go API: it is now possible to
register arbitrary handlers on the user-facing HTTP server. In geth, this
facility is used to enable GraphQL.

There is a single minor change relevant for geth users in this PR: The
GraphQL API is no longer available separately from the JSON-RPC HTTP
server. If you want GraphQL, you need to enable it using the
./geth --http --graphql flag combination.

The --graphql.port and --graphql.addr flags are no longer available.
gastonponti added a commit to celo-org/celo-blockchain that referenced this pull request Jul 13, 2021
Merge from upstream, go-ethereum v 1.9.19 (https://github.com/ethereum/go-ethereum/releases/tag/v1.9.19)

Geth v1.9.19 is our regular maintenance release

Note, if you were using GraphQL previously, it was moved to the HTTP RPC endpoint. The old --graphql.host and --graphql.port flags will not work any more. You might need to adjust your TOML config files accordingly too.

1.9.19 Notes:

Deprecated flags:
GraphQLListenAddrFlag
GraphQLPortFlag

Added flags:
CacheTrieJournalFlag
CacheTrieRejournalFlag

28c5a8a
fetcher.go/mainLoop
check ancestor, otherwise the lightest sync could panic
fetcher.go/newLightFetcher
Add downloader.SyncMode as parameter

c0c0161 (ethereum/go-ethereum#21105)
node.go:
startNetworking should server stop if proxyServer failed
main.go
startNode runnerFactory replaced

90dedea
this commit tried to fusion the consensus/istanbul/core/event.go file with an erased tests (signed_data_internal_test.go)
Restored that test, and add the changes from the signer/core/signed_data.go, to the shared/signer/signed_data.go

c28fd9c
make Berlin a copy of Istanbul instead of YoloV1

Backwards compatibility
The commit c0c0161 (ethereum/go-ethereum#21105) "changes the APIs for instantiating Ethereum nodes in a Go program. The new APIs are not backwards-compatible" (following this proposal https://gist.github.com/renaynay/5bec2de19fde66f4d04c535fd24f0775)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants