Refactor node.NewNode to simplify by thanethomson · Pull Request #3456 · tendermint/tendermint

thanethomson · 2019-03-20T15:31:13Z

The node.NewNode method is pretty complex at the moment, an in order to address issues like #3156, we need to simplify the interface for partial node instantiation. In some places, we don't need to build up a full node (like in the node.TestCreateProposalBlock test), but the complexity of such partial instantiation needs to be reduced.

This PR aims to eventually make this easier/simpler.

See also this gist for some background work done when starting to refactor here.

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

The `node.NewNode` method is pretty complex at the moment, an in order to address issues like #3156, we need to simplify the interface for partial node instantiation. In some places, we don't need to build up a full node (like in the `node.TestCreateProposalBlock` test), but the complexity of such partial instantiation needs to be reduced. This PR aims to eventually make this easier/simpler.

melekes · 2019-04-18T09:55:27Z

What's left to do here?

thanethomson · 2019-04-19T02:07:07Z

Ideally I wanted to find a way to better group the inner construction of the node to simplify it further (perhaps into higher-level constructs), but that's proven to be quite difficult. This might be the best I can do for now.

melekes · 2019-04-22T06:33:16Z

node/node.go


+func loadStateFromDBOrGenesisDoc(stateDB dbm.DB, genesisDocProvider GenesisDocProvider) (sm.State, *types.GenesisDoc, error) {
 	// Get genesis doc
 	// TODO: move to state package?


can we move this to state/store.go?

We can. It does, however, involve moving the GenesisDocProvider-related code from node/node.go to state/store.go to prevent circular dependencies. This changes the Go API a bit. Do you think that'd be a problem?

Technically the genesis doc represents a form of state, so it would make sense to have it in the state package, right?

Hmm.. will respond tomorrow

node/node.go

melekes · 2019-04-22T06:56:14Z

node/node.go

+
+	// If an address is provided, listen on the socket for a connection from an
+	// external signing process.
+	privValidator, err = optionallyCreateAndStartPrivValidator(config, privValidator, logger)


If we're going to use this optionally prefix, we should document the expected behavior. Like if the address is not provided, privValidator and err are both nil. Note this won't prevent developers from making a mistake where they would assume privValidator can't ever be nil later in the code.

In the case when both are nil we just asking for unexpected panics. Better to encode this in an explicit error which the caller can react on.

codecov-io · 2019-04-24T17:10:34Z

Codecov Report

Merging #3456 into develop will decrease coverage by <.01%.
The diff coverage is 58.22%.

@@             Coverage Diff             @@
##           develop    #3456      +/-   ##
===========================================
- Coverage    64.29%   64.28%   -0.01%     
===========================================
  Files          213      213              
  Lines        17447    17501      +54     
===========================================
+ Hits         11217    11250      +33     
- Misses        5308     5332      +24     
+ Partials       922      919       -3

Impacted Files	Coverage Δ
state/store.go	`59.77% <0%> (-11.47%)`	⬇️
node/node.go	`64.07% <70.76%> (+0.19%)`	⬆️
privval/signer_service_endpoint.go	`83.63% <0%> (-5.46%)`	⬇️
p2p/pex/pex_reactor.go	`80.64% <0%> (-1.76%)`	⬇️
libs/clist/clist.go	`66.66% <0%> (-1.52%)`	⬇️
blockchain/reactor.go	`70.56% <0%> (-0.94%)`	⬇️
libs/db/remotedb/grpcdb/server.go	`0% <0%> (ø)`	⬆️
consensus/state.go	`79.05% <0%> (+0.11%)`	⬆️
consensus/reactor.go	`72.25% <0%> (+1.41%)`	⬆️
privval/socket_listeners.go	`96.55% <0%> (+10.34%)`	⬆️
... and 1 more

node/node.go

melekes · 2019-04-25T07:24:52Z

state/store.go

+// panics if failed to unmarshal bytes
+func loadGenesisDoc(db dbm.DB) (*types.GenesisDoc, error) {
+	bytes := db.Get(genesisDocKey)
+	if len(bytes) == 0 {


Suggested change

cmn.PanicCrisis(fmt.Sprintf("Failed to load genesis doc due to unmarshaling error: %v (bytes: %X)", err, bytes))

panic(fmt.Sprintf("Failed to load genesis doc due to unmarshaling error: %v (bytes: %X)", err, bytes))

If we change this here, shouldn't we just replace it in the whole codebase and take out the deprecated functions altogether?

I don't mind, but should probably be a separate PR

Done in #3595

state/store.go

As per discussion over [here](#3456 (comment)), we need to remove these `PanicXXX` functions and eliminate our dependence on them. In this PR, each and every `PanicXXX` function call is replaced with a simple `panic` call.

melekes

🥑

* Remove deprecated PanicXXX functions from codebase As per discussion over [here](#3456 (comment)), we need to remove these `PanicXXX` functions and eliminate our dependence on them. In this PR, each and every `PanicXXX` function call is replaced with a simple `panic` call. * add a changelog entry

node/node.go

plus, expose PEXReactor on node

* Remove deprecated PanicXXX functions from codebase As per discussion over [here](tendermint#3456 (comment)), we need to remove these `PanicXXX` functions and eliminate our dependence on them. In this PR, each and every `PanicXXX` function call is replaced with a simple `panic` call. * add a changelog entry

The node.NewNode method is pretty complex at the moment, an in order to address issues like tendermint#3156, we need to simplify the interface for partial node instantiation. In some places, we don't need to build up a full node (like in the node.TestCreateProposalBlock test), but the complexity of such partial instantiation needs to be reduced. This PR aims to eventually make this easier/simpler. See also this gist https://gist.github.com/thanethomson/56e1640d057a26186e38ad678a1d114c for some background work done when starting to refactor here. ## Commits: * [WIP] Refactor node.NewNode to simplify The `node.NewNode` method is pretty complex at the moment, an in order to address issues like tendermint#3156, we need to simplify the interface for partial node instantiation. In some places, we don't need to build up a full node (like in the `node.TestCreateProposalBlock` test), but the complexity of such partial instantiation needs to be reduced. This PR aims to eventually make this easier/simpler. * Refactor state loading and genesis doc provider into state package * Refactor for clarity of return parameters * Fix incorrect capitalization of error messages * Simplify extracted functions' names * Document optionally-prefixed functions * Refactor optionallyFastSync for clarity of separation of concerns * Restructure function for early return * Restructure function for early return * Remove dependence on deprecated panic functions * refactor code a bit more plus, expose PEXReactor on node * align logger names * add a changelog entry * align logger names 2 * add a note about PEXReactor returning nil

…tendermint#3456) (tendermint#3461) Closes tendermint#3401 --- #### PR checklist - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request tendermint#3456 done by [Mergify](https://mergify.com). --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

thanethomson self-assigned this Mar 20, 2019

thanethomson requested review from ebuchman and melekes March 20, 2019 15:31

Merge branch 'develop' into refactor/new-node

b3f9ac5

thanethomson changed the title ~~[WIP] Refactor node.NewNode to simplify~~ Refactor node.NewNode to simplify Apr 19, 2019

melekes reviewed Apr 22, 2019

View reviewed changes

thanethomson added 6 commits April 24, 2019 11:48

Merge branch 'develop' into refactor/new-node

e51c3a6

Refactor state loading and genesis doc provider into state package

fa75cb0

Refactor for clarity of return parameters

2af6a5e

Fix incorrect capitalization of error messages

bd1924a

Simplify extracted functions' names

9de7092

Document optionally-prefixed functions

f45139a

melekes reviewed Apr 25, 2019

View reviewed changes

thanethomson added 4 commits April 25, 2019 08:25

Refactor optionallyFastSync for clarity of separation of concerns

f3e0478

Restructure function for early return

a2ce28b

Restructure function for early return

e6195cf

Remove dependence on deprecated panic functions

d233302

thanethomson mentioned this pull request Apr 25, 2019

Remove deprecated PanicXXX functions from codebase #3595

Merged

4 tasks

melekes approved these changes Apr 26, 2019

View reviewed changes

Merge branch 'develop' into refactor/new-node

1f6d020

melekes reviewed Apr 26, 2019

View reviewed changes

node/node.go Outdated Show resolved Hide resolved

melekes added 3 commits April 26, 2019 15:07

refactor code a bit more

23799b4

plus, expose PEXReactor on node

align logger names

145740a

add a changelog entry

f292a7d

melekes added 2 commits April 26, 2019 15:25

align logger names 2

fd64835

add a note about PEXReactor returning nil

eb4573f

melekes merged commit 7b162f5 into develop Apr 26, 2019

melekes deleted the refactor/new-node branch April 26, 2019 12:05

ebuchman mentioned this pull request Apr 26, 2019

Better support for experimenting with reactors #3176

Closed

melekes mentioned this pull request May 7, 2019

v0.31.6 release preview #3637

Closed

36 tasks

melekes mentioned this pull request May 30, 2019

temporary #3684

Closed

44 tasks

thanethomson mentioned this pull request May 30, 2019

Move GenesisDocProvider and DefaultGenesisDocProviderFunc back to node package #3691

Closed

	cmn.PanicCrisis(fmt.Sprintf("Failed to load genesis doc due to unmarshaling error: %v (bytes: %X)", err, bytes))
	panic(fmt.Sprintf("Failed to load genesis doc due to unmarshaling error: %v (bytes: %X)", err, bytes))

Conversation

thanethomson commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melekes commented Apr 18, 2019

Uh oh!

thanethomson commented Apr 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

melekes Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thanethomson commented Mar 20, 2019 •

edited

Loading

melekes Apr 22, 2019 •

edited

Loading

codecov-io commented Apr 24, 2019 •

edited

Loading