node: implement ADR-52 Tendermint Mode (reopened) #6061

dongsam · 2021-02-06T20:10:38Z

Description

Implementation spec of Tendermint Mode ADR-52 by B-Harvest

Add Tendermint mode

Add --mode flag on tendermint node and mode on config.toml
[ fullnode, validator, seednode ]
default is fullnode, except testnet

Refs

Wrote tests
Updated CHANGELOG_PENDING.md
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Updated relevant documentation (docs/) and code comments
Re-reviewed Files changed in the Github PR explorer
Applied Appropriate Labels

reopened the PR #5212
Since the last commit of the previous PR, added the following commits according to the review

merge latest Master branch
add upgrading.md,
seednode automatically set seed_mode to true
simplify addReactor logic

codecov · 2021-02-06T22:18:46Z

Codecov Report

Merging #6061 (e80715b) into master (26fd158) will increase coverage by 0.01%.
The diff coverage is 62.72%.

@@            Coverage Diff             @@
##           master    #6061      +/-   ##
==========================================
+ Coverage   60.76%   60.77%   +0.01%     
==========================================
  Files         275      275              
  Lines       25345    25455     +110     
==========================================
+ Hits        15401    15471      +70     
- Misses       8341     8371      +30     
- Partials     1603     1613      +10

Impacted Files	Coverage Δ
cmd/tendermint/commands/run_node.go	`0.00% <0.00%> (ø)`
cmd/tendermint/commands/testnet.go	`23.44% <0.00%> (-0.17%)`	⬇️
config/toml.go	`60.86% <ø> (ø)`
rpc/core/status.go	`0.00% <0.00%> (ø)`
config/config.go	`75.93% <66.66%> (-0.18%)`	⬇️
node/node.go	`58.17% <69.65%> (+1.14%)`	⬆️
consensus/wal_generator.go	`70.47% <100.00%> (ø)`
libs/clist/clist.go	`63.74% <0.00%> (-3.51%)`	⬇️
blockchain/v0/pool.go	`74.90% <0.00%> (-1.53%)`	⬇️
p2p/transport_memory.go	`84.93% <0.00%> (-1.37%)`	⬇️
... and 11 more

melekes

great work 👍 left a few comments

config/config.go

melekes · 2021-02-08T06:36:32Z

consensus/wal_generator.go

@@ -89,7 +89,7 @@ func WALGenerateNBlocks(t *testing.T, wr io.Writer, numBlocks int) (err error) {
 	consensusState := NewState(config.Consensus, state.Copy(), blockExec, blockStore, mempool, evpool)
 	consensusState.SetLogger(logger)
 	consensusState.SetEventBus(eventBus)
-	if privValidator != nil {
+	if privValidator != nil && privValidator != (*privval.FilePV)(nil) {


I was trying just checking nil for "no validator", but there were cases privValidator != nil even though privValidator is empty with ModeFullNode, maybe interface typecasting issue

so originally I added the Empty() function to the interface PrivValidator for checking "no validator"

func (pv *FilePV) Empty() bool { return pv == (*FilePV)(nil) }

but then changed it simply to using config because there were too many relevant correction codes.

#5212 (comment)

What is the use case for having a FilePV as empty?

docs/nodes/validators.md

cmd/tendermint/commands/run_node.go

melekes · 2021-02-08T06:46:25Z

rpc/core/status.go

 	privValAddress := env.PubKey.Address()
-	_, val := vals.GetByAddress(privValAddress)
+
+	// If we're still at height h, search in the current validator set.


why do we need to change this? is there a separate issue for this?

this logic is for return empty ValidatorInfo when fullnode mode call rpc /status

#5212 (comment)

cmwaters · 2021-02-08T15:32:27Z

I'm not implying that one or the other is necessary the right default but are we sure we want to default with fullnode rather than validator. If a user is to download the binary and run a single node test net it won't run because it will be a network of one full node. On the other hand, if the user accidentally runs as a validator when just being a full node, they will be treated the exact same as a full node until they join the validator set

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

dongsam · 2021-02-10T09:50:43Z

I'm not implying that one or the other is necessary the right default but are we sure we want to default with fullnode rather than validator. If a user is to download the binary and run a single node test net it won't run because it will be a network of one full node. On the other hand, if the user accidentally runs as a validator when just being a full node, they will be treated the exact same as a full node until they join the validator set

That's right. So the default is fullnode mode, but TestConfig for local testnet is set as validator mode so that config.toml of validator mode is created

alexanderbez

Am I missing something or are all the mode types already implemented? Or does this PR just handle the configuration aspect of it?

UPGRADING.md

cmd/tendermint/commands/run_node.go

alexanderbez · 2021-02-12T20:38:02Z

cmd/tendermint/commands/run_node.go

@@ -102,6 +105,9 @@ func NewRunNodeCmd(nodeProvider nm.Provider) *cobra.Command {
 		Use:     "start",
 		Aliases: []string{"node", "run"},
 		Short:   "Run the tendermint node",
+		Long: `Run the tendermint node
+
+need to set "--mode validator" to run the node as validator`,


I do no think this additional comment is necessary. If we want to describe mode we should have a more elaborate description of all the possible values.

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

dongsam · 2021-02-16T10:40:40Z

Am I missing something or are all the mode types already implemented? Or does this PR just handle the configuration aspect of it?

Thanks for the review, @alexanderbez
the main logic of this PR is implemented in node/node.go

cmwaters

Hey, this is great work 🚀 . Looking forward to getting this over the line.

cmwaters · 2021-02-19T13:47:07Z

config/config.go

@@ -561,6 +586,7 @@ type P2PConfig struct { //nolint: maligned
 	// peers. If another node asks it for addresses, it responds and disconnects.
 	//
 	// Does not work if the peer-exchange reactor is disabled.
+	// automatically set true if Mode of the node is seednode
 	SeedMode bool `mapstructure:"seed-mode"`


Do we want to consider removing this variable. I feel with the introduction of mode, this has been made redundant. Of course you could have a validator with seed mode on but I'm leaning towards separating these i.e. either you're a validator or a seed node. What do people think?

I think this depends on the p2p refactor. Did we decide to pull out the seed mode in a different binary or?

I believe that is still up for discussion, but from the last conversations I heard, moving it to a separate binary is strong contender and there is a possibility that some part of this PR will have to adjusted for that.

Hey @dongsam, we had a call yesterday and briefly discussed this, coming to the agreement that we would like to remove the SeedMode config parameter and rely only on the new mode parameter. This will help draw a clearer distinction between operating a seed node and a full node.

As to the decision about having it as a separate binary, I'm not too familiar with what the advantages and disadvantages would be. Is it just so that is of a smaller size?

If so, then I'm on board with this. However, I would also want to keep it as part of the main Tendermint binary as convenience to me when I am spinning up test nets and what not. Since we are down this road, is there any argument for the light client to also have its own binary?

I think the idea is that it's the same binary, but it runs in either a completely isolated mode -- full-node, validator, or seed-mode, but not any combo. i.e. if seed-mode is enabled, it will only run in seed-mode without full-node functionality.

thanks for the discussion and the feedback, I'll make an additional commit to remove the SeedMode config parameter and rely only on the new mode parameter

cmwaters · 2021-02-19T13:50:33Z

consensus/wal_generator.go

@@ -89,7 +89,7 @@ func WALGenerateNBlocks(t *testing.T, wr io.Writer, numBlocks int) (err error) {
 	consensusState := NewState(config.Consensus, state.Copy(), blockExec, blockStore, mempool, evpool)
 	consensusState.SetLogger(logger)
 	consensusState.SetEventBus(eventBus)
-	if privValidator != nil {
+	if privValidator != nil && privValidator != (*privval.FilePV)(nil) {


What is the use case for having a FilePV as empty?

cmwaters · 2021-02-19T14:09:17Z

node/node.go

+	_, stateDB, err := initDBs(config, dbProvider)
+	if err != nil {
+		return nil, err
+	}
+
+	state, genDoc, err := LoadStateFromDBOrGenesisDocProvider(stateDB, genesisDocProvider)
+	if err != nil {
+		return nil, err
+	}


Suggested change

_, stateDB, err := initDBs(config, dbProvider)

if err != nil {

return nil, err

}

state, genDoc, err := LoadStateFromDBOrGenesisDocProvider(stateDB, genesisDocProvider)

if err != nil {

return nil, err

}

genDoc, err := genesisDocProvider()

if err != nil {

return nil, err

}

state, err := sm.MakeGenesisState(genDoc)

if err != nil {

return nil, err

}

IMO, I don't think it makes sense to have a db that just takes the state derived from genesis and is never updated so I would remove dbProvider and just have it like this

cmwaters · 2021-02-19T14:13:35Z

node/node.go

+			state.Version.Consensus.Block,
+			state.Version.Consensus.App,


I'm not too sure how full nodes handle this, but AFAICS, state is never updated. I can foresee then that there might be a problem if there is a change to the Block and/or App version where the seed node doesn't get updated and can thus no longer communicate with the block chain. I guess this would mean that the operator would also need to stop their seed node and update their genesis.

tac0turtle

Is it possible to increase the total amount of peers a seed node can have? By default, we limit it to 50 because more would use high amounts of memory, but since the seed nodes are already running with reduced memory we could increase this amount.

dongsam · 2021-03-03T20:27:09Z

Is it possible to increase the total amount of peers a seed node can have? By default, we limit it to 50 because more would use high amounts of memory, but since the seed nodes are already running with reduced memory we could increase this amount.

@marbar3778 Does the default limit value 50 mean MaxNumInboundPeers 40 + MaxNumOutboundPeers 10 ?
I think it's okay to increase the default for the lightened seed mode, but those config values are affected by the fullnode or validator, so I'm a little worried about increasing the default values, is that correct?

cmwaters · 2021-03-10T12:37:13Z

but those config values are affected by the fullnode or validator, so I'm a little worried about increasing the default values, is that correct?

Yeah I would just leave the defaults as is. If we really wanted to, we could create a default seed node config with adjusted params that gets used when starting a seed node through the command line. But for now I would just leave it.

As a more general comment, I would like to get this PR in soon. @dongsam when do you think you will be able to work on this. Would be happy to help you with it if you wanted.

dongsam · 2021-03-10T13:30:34Z

As a more general comment, I would like to get this PR in soon. @dongsam when do you think you will be able to work on this. Would be happy to help you with it if you wanted.

@cmwaters Thank you for your words and sorry for the delay. It is being delayed because it overlaps with the Liquidity module schedule. I think I can proceed next weekend, I'd appreciate your help.

cmwaters · 2021-03-15T18:15:35Z

I have created a new PR (#6241) to supersede this as I unfortunately am unable to push to the forked branch

tac0turtle · 2021-03-17T13:42:34Z

closing in favor of #6241

Thank you for the work @dongsam

dongsam added 3 commits February 6, 2021 21:31

Add Tendermint Nodes

097356a

seednode automatically set seed_mode, update docs

46b5287

fix seednode bugs

8843f03

dongsam marked this pull request as ready for review February 6, 2021 22:42

dongsam requested review from alexanderbez, ebuchman, tac0turtle, melekes and tessr as code owners February 6, 2021 22:42

melekes reviewed Feb 8, 2021

View reviewed changes

dongsam and others added 3 commits February 10, 2021 18:18

Apply suggestions from code review

5c801da

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

fix typo

8c250fd

Merge branch 'master' into ADR-52-Tendermint-Mode-reopen-PR

051f838

alexanderbez reviewed Feb 12, 2021

View reviewed changes

Apply suggestions from code review

da2b06b

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Apply suggestions from code review

e80715b

cmwaters reviewed Feb 19, 2021

View reviewed changes

tac0turtle reviewed Mar 2, 2021

View reviewed changes

cmwaters self-assigned this Mar 15, 2021

cmwaters mentioned this pull request Mar 15, 2021

node: implement tendermint modes #6241

Merged

tac0turtle closed this Mar 17, 2021

cmwaters mentioned this pull request Mar 18, 2021

Tendermint "modes" #2237

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node: implement ADR-52 Tendermint Mode (reopened) #6061

node: implement ADR-52 Tendermint Mode (reopened) #6061

dongsam commented Feb 6, 2021

codecov bot commented Feb 6, 2021 •

edited

melekes left a comment

melekes Feb 8, 2021

dongsam Feb 10, 2021

cmwaters Feb 19, 2021

melekes Feb 8, 2021

dongsam Feb 10, 2021

cmwaters commented Feb 8, 2021

dongsam commented Feb 10, 2021

alexanderbez left a comment

alexanderbez Feb 12, 2021

dongsam commented Feb 16, 2021 •

edited

cmwaters left a comment •

edited

cmwaters Feb 19, 2021

tac0turtle Feb 25, 2021

alexanderbez Feb 26, 2021

cmwaters Mar 2, 2021

cmwaters Mar 2, 2021

alexanderbez Mar 2, 2021

dongsam Mar 3, 2021

cmwaters Feb 19, 2021

cmwaters Feb 19, 2021

cmwaters Feb 19, 2021

tac0turtle left a comment

dongsam commented Mar 3, 2021

cmwaters commented Mar 10, 2021

dongsam commented Mar 10, 2021

cmwaters commented Mar 15, 2021

tac0turtle commented Mar 17, 2021 •

edited

node: implement ADR-52 Tendermint Mode (reopened) #6061

node: implement ADR-52 Tendermint Mode (reopened) #6061

Conversation

dongsam commented Feb 6, 2021

Description

codecov bot commented Feb 6, 2021 • edited

Codecov Report

melekes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmwaters commented Feb 8, 2021

dongsam commented Feb 10, 2021

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongsam commented Feb 16, 2021 • edited

cmwaters left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tac0turtle left a comment

Choose a reason for hiding this comment

dongsam commented Mar 3, 2021

cmwaters commented Mar 10, 2021

dongsam commented Mar 10, 2021

cmwaters commented Mar 15, 2021

tac0turtle commented Mar 17, 2021 • edited

codecov bot commented Feb 6, 2021 •

edited

dongsam commented Feb 16, 2021 •

edited

cmwaters left a comment •

edited

tac0turtle commented Mar 17, 2021 •

edited