node: implement tendermint modes #6241

cmwaters · 2021-03-15T18:12:37Z

Reference PR: #6061

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

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

codecov · 2021-03-16T09:46:40Z

Codecov Report

Merging #6241 (8432063) into master (c52de48) will decrease coverage by 0.10%.
The diff coverage is 56.02%.

@@            Coverage Diff             @@
##           master    #6241      +/-   ##
==========================================
- Coverage   60.84%   60.74%   -0.11%     
==========================================
  Files         277      277              
  Lines       25943    26060     +117     
==========================================
+ Hits        15785    15830      +45     
- Misses       8530     8590      +60     
- Partials     1628     1640      +12

Impacted Files	Coverage Δ
cmd/tendermint/commands/run_node.go	`0.00% <0.00%> (ø)`
cmd/tendermint/commands/testnet.go	`22.22% <0.00%> (-0.15%)`	⬇️
config/toml.go	`60.86% <ø> (ø)`
rpc/core/status.go	`0.00% <0.00%> (ø)`
node/node.go	`58.35% <60.36%> (+0.79%)`	⬆️
config/config.go	`75.86% <77.77%> (-0.26%)`	⬇️
consensus/wal_generator.go	`70.47% <100.00%> (ø)`
privval/signer_server.go	`89.47% <0.00%> (-5.27%)`	⬇️
types/tx.go	`82.97% <0.00%> (-4.26%)`	⬇️
blockchain/v0/pool.go	`73.00% <0.00%> (-3.43%)`	⬇️
... and 10 more

CHANGELOG_PENDING.md

node/node.go

tac0turtle

Thanks for pushing this over the finish line!!

cmwaters · 2021-03-17T13:53:16Z

There are a few points / ideas that came to mind whilst working on this that I would like to document (I may do also in a separate issue). This PR (and the prior work and ADR) ultimately move us in the direction of making clearer separations in the types of nodes and the roles they play in the network. Specifically:

Validator Node -> proposes and votes on blocks
Full Node -> gossips txs, participates in consensus, validates and stores blocks, provides endpoints for data availability
Seed Node -> keeps an address book of a large part / the entire network, providing peers that nodes can connect with.

I find it compelling to add a fourth type of node:

Light Node -> runs a light client and an RPC client.

This is something that of course already exists but I believe needs to be explicitly named in a way that groups it with the rest of the nodes and is more intuitive for people familiarising themselves with Tendermint / Cosmos SDK. To make the distinction clear:

IBC uses a light client
A wallet runs a light node

The next point I would like to address is the Config file. Given this growing separation between the different nodes in a network, I see two paths we can go down: 1) A "one config to rule them all" approach where one large config file can be used for every node. 2) A config file specific to the node. Currently we side with the former. What this means is that an operator could stop a full node and run a seed node from the exact config. It also means that If I'm running a seed node, my config.toml file is filled with variables that have no relation to a seed node i.e. MempoolConfig or ConsensusConfig. I think this is just bad UX and may confuse end users. I would also argue that it would be more beneficial to this separation if an operator couldn't simply switch modes but would have to start a new process.

I would propose that all node types share a BaseConfig(this already exists). The BaseConfig would indicate the mode and thus how the rest of the toml file should be read. Seed nodes would have a SeedConfig and light nodes would have a LightConfig

…open-PR

tac0turtle · 2021-03-17T14:39:02Z

I find it compelling to add a fourth type of node:
* Light Node -> runs a light client and an RPC client.

I would say this is more up to the application (cosmos/cosmos-sdk#6563). Reason we can not offer this type of node is because we don't know the apps proof design making queries through the light client not usable. IMO we shouldn't have a cmd for it either, it should be app specific.

I agree with your second point. Having two configs will cause less confusion and better the UX for outr users.

cmwaters · 2021-03-17T15:02:18Z

I would say this is more up to the application (cosmos/cosmos-sdk#6563). Reason we can not offer this type of node is because we don't know the apps proof design making queries through the light client not usable. IMO we shouldn't have a cmd for it either, it should be app specific.

Right, the verification of ABCI queries is a difficult one although I believe any app through the cosmos sdk has the same proof design which is the default of the TM light node. Also, aren't the other uses of a light node, verification of headers and txs also valuable?

tac0turtle · 2021-03-17T15:51:53Z

Right, the verification of ABCI queries is a difficult one although I believe any app through the cosmos sdk has the same proof design which is the default of the TM light node. Also, aren't the other uses of a light node, verification of headers and txs also valuable?

Does it work? Last I checked it was still broken. What is the use case for verifying headers via cli? Verifying txs is possible, but it's more valuable to have a single light client defined by the app to do both queries and tx verification. We can add it, but I would guess it would cause confusion with so many options.

…open-PR

cmwaters · 2021-03-17T16:11:58Z

Last I checked it was still broken.

Maybe, I thought we fixed it. At least there is a test for it. 😬

What is the use case for verifying headers via cli?

Well technically it's done via the RPC

more valuable to have a single light client defined by the app to do both queries and tx verification

Totally agree. I think I need to do some more thinking about it because I would like to make light clients/nodes something that provides greater utility to applications. Currently, the model is to have light clients quite separate from application. The light client doesn't store any app state or have any connection with the app. Perhaps we could have an ABCI Light (😅 ) that gives the application the app hash and a proof for verification.

I think a valuable use case for a light node would be to track partial state. I.e. perhaps I just want to know all the transactions that occur with a specific type or attribute (a logical example would be tracking transactions to and from an account). A light node could listen for events on a full node and when the tx was made, verify it and update that specific part of state. I don't know, I'm just thinking aloud.

tac0turtle · 2021-03-17T16:27:13Z

Totally agree. I think I need to do some more thinking about it because I would like to make light clients/nodes something that provides greater utility to applications. Currently, the model is to have light clients quite separate from application. The light client doesn't store any app state or have any connection with the app. Perhaps we could have an ABCI Light (😅 ) that gives the application the app hash and a proof for verification.

I think a valuable use case for a light node would be to track partial state. I.e. perhaps I just want to know all the transactions that occur with a specific type or attribute (a logical example would be tracking transactions to and from an account). A light node could listen for events on a full node and when the tx was made, verify it and update that specific part of state. I don't know, I'm just thinking aloud.

I like some of these ideas. We should do a brainstorm session on these and other ideas, the scope can easily get large, but it would be fun to see what we come up with.

tychoish · 2021-03-17T16:48:16Z

node/node.go

+
+	err = sw.AddPersistentPeers(splitAndTrimEmpty(config.P2P.PersistentPeers, ",", " "))
+	if err != nil {
+


nit, blank line

tychoish · 2021-03-17T16:51:14Z

node/node.go

-		if err := n.bcReactor.Start(); err != nil {
-			return err
+	if n.config.Mode != cfg.ModeSeed {
+


tychoish · 2021-03-17T16:51:27Z

node/node.go

-			return err
+	if n.config.Mode != cfg.ModeSeed {
+
+		if n.config.FastSync.Version == "v0" {


is "v0" a constant somewhere that we could use?

Nope, I don't think so. We could add one

tychoish · 2021-03-17T16:52:22Z

node/node.go

+	if n.config.Mode == cfg.ModeValidator {
+		pubKey, err := n.privValidator.GetPubKey(context.TODO())
+		if pubKey == nil || err != nil {
+			return fmt.Errorf("can't get pubkey: %w", err)


is this error salient when pubkey is nil and error is nil?

yup. I'm not sure if there is a case where it happens, but if it does we should still error

tychoish · 2021-03-17T16:52:53Z

node/node.go

+	nodeInfo.ListenAddr = lAddr
+
+	err := nodeInfo.Validate()
+	return nodeInfo, err


should we return a non-nil node info when err is non nil?

The assumption is generally to read the error first, so I think it should be fine

tychoish · 2021-03-17T16:54:17Z

node/node_test.go

+func TestNodeNewSeedNode(t *testing.T) {
+	config := cfg.ResetTestRoot("node_new_node_custom_reactors_test")
+	config.Mode = cfg.ModeSeed
+	defer os.RemoveAll(config.RootDir)


I'm not sure, but it seems like t.Cleanup() might be better at cleanup but I'm not sure.

dongsam and others added 11 commits February 6, 2021 21:31

Add Tendermint Nodes

097356a

seednode automatically set seed_mode, update docs

46b5287

fix seednode bugs

8843f03

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

Apply suggestions from code review

da2b06b

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

Apply suggestions from code review

e80715b

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

79fb53d

remove seed mode config param

5f230f8

remove db's from seed nodes

6226fc5

cmwaters requested review from alexanderbez, ebuchman, tac0turtle, melekes and tessr as code owners March 15, 2021 18:12

cmwaters mentioned this pull request Mar 15, 2021

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

Closed

6 tasks

cmwaters marked this pull request as draft March 15, 2021 18:17

fix default template config

5d1f6b9

cmwaters added 2 commits March 16, 2021 19:25

adapt e2e suite to seed nodes

22801af

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

9136ce0

cmwaters marked this pull request as ready for review March 16, 2021 18:48

tac0turtle reviewed Mar 17, 2021

View reviewed changes

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved

tac0turtle reviewed Mar 17, 2021

View reviewed changes

node/node.go Outdated Show resolved Hide resolved

tac0turtle approved these changes Mar 17, 2021

View reviewed changes

implement suggestions

8f5fb63

cmwaters closed this Mar 17, 2021

cmwaters reopened this Mar 17, 2021

Merge github.com:tendermint/tendermint into ADR-52-Tendermint-Mode-re…

0f21867

…open-PR

cmwaters added 2 commits March 17, 2021 15:50

make terminology consistent

80562bd

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

457cd5a

cmwaters force-pushed the ADR-52-Tendermint-Mode-reopen-PR branch from dab9b90 to 457cd5a Compare March 17, 2021 14:52

cmwaters added 2 commits March 17, 2021 16:52

make terminology consistent v2

9e8ded8

Merge github.com:tendermint/tendermint into ADR-52-Tendermint-Mode-re…

9e4cc4b

…open-PR

cmwaters force-pushed the ADR-52-Tendermint-Mode-reopen-PR branch from 6a29bed to 9e4cc4b Compare March 17, 2021 15:54

tac0turtle assigned cmwaters Mar 17, 2021

tychoish reviewed Mar 17, 2021

View reviewed changes

cmwaters added 2 commits March 18, 2021 11:05

apply some cosmetic changes

073e92e

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

8432063

cmwaters merged commit 9f7051d into master Mar 18, 2021

cmwaters deleted the ADR-52-Tendermint-Mode-reopen-PR branch March 18, 2021 10:17

This was referenced Mar 18, 2021

config: revision to accommodate node modes #6255

Closed

Tendermint "modes" #2237

Closed

liamsi mentioned this pull request May 1, 2021

light-client with DAS (rpc) celestiaorg/celestia-core#307

Closed

5 tasks

aleem1314 mentioned this pull request Jan 27, 2022

Simapp doesn't create blocks on start cosmos/cosmos-sdk#11042

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node: implement tendermint modes #6241

node: implement tendermint modes #6241

cmwaters commented Mar 15, 2021

codecov bot commented Mar 16, 2021 •

edited

tac0turtle left a comment

cmwaters commented Mar 17, 2021 •

edited

tac0turtle commented Mar 17, 2021

cmwaters commented Mar 17, 2021

tac0turtle commented Mar 17, 2021

cmwaters commented Mar 17, 2021

tac0turtle commented Mar 17, 2021

tychoish Mar 17, 2021

tychoish Mar 17, 2021

tychoish Mar 17, 2021

cmwaters Mar 17, 2021

tychoish Mar 17, 2021

cmwaters Mar 18, 2021

tychoish Mar 17, 2021

cmwaters Mar 18, 2021

tychoish Mar 17, 2021


		err = sw.AddPersistentPeers(splitAndTrimEmpty(config.P2P.PersistentPeers, ",", " "))
		if err != nil {

node: implement tendermint modes #6241

node: implement tendermint modes #6241

Conversation

cmwaters commented Mar 15, 2021

codecov bot commented Mar 16, 2021 • edited

Codecov Report

tac0turtle left a comment

Choose a reason for hiding this comment

cmwaters commented Mar 17, 2021 • edited

tac0turtle commented Mar 17, 2021

cmwaters commented Mar 17, 2021

tac0turtle commented Mar 17, 2021

cmwaters commented Mar 17, 2021

tac0turtle commented Mar 17, 2021

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

codecov bot commented Mar 16, 2021 •

edited

cmwaters commented Mar 17, 2021 •

edited