proto: deduplicate consensus params #9287

cmwaters · 2022-08-18T09:49:30Z

proto files contain two identical cosnensus params. One in types and the other in abci. This PR aims at consolidating the two. Port of #5994

This PR also includes the depracation of TimeIotaMs (ref: #5792)

sergio-mena · 2022-08-18T10:07:26Z

Good catch. @williambanfield and I had noticed that ConsensusParams had moved from abci to types somewhere after v0.34.x, and decided to leave ConsensusParams and BlockParams in abci for simplicity. I was not aware that ConsensusParams was also present in types in this branch.

My question is: what was the rationale in the first place to move ConsensusParams from abci to types? (looked up the PR, but no info)

sergio-mena · 2022-08-18T10:11:37Z

Follow up: I was intrigued by this duplication, and how it landed on the feature/abci++ppp branch.
Did some digging and realized this duplication is ALSO on v0.34.x, not sure fixing it there entails a risk low enough, though.

williambanfield · 2022-08-18T17:18:59Z

Appears that this was originally done during v0.35 in this pull request: #5987

There is no rationale present on that pull request so it's difficult to understand why it was done.

williambanfield · 2022-08-18T17:26:16Z

@sergio-mena I don't see this as being present in v0.34.x

tendermint/abci/types/types.pb.go

Line 613 in a41c5ee

    
           ConsensusParams *ConsensusParams  `protobuf:"bytes,3,opt,name=consensus_params,json=consensusParams,proto3" json:"consensus_params,omitempty"`

williambanfield

Needs a changelog entry, will break in-process apps.

sergio-mena · 2022-08-18T18:25:41Z

I have added this PR to the Tendermint project, so people have visibility from the project board.

sergio-mena · 2022-08-18T18:29:39Z

I also added a checkbox in #9053 (final adjustments) with a reference to this PR

proto/tendermint/types/params.proto

sergio-mena · 2022-08-18T18:49:06Z

@sergio-mena I don't see this as being present in v0.34.x

@williambanfield not sure I get what you mean. Do you mean that protos and generated code don't match in v0.34.x?

williambanfield · 2022-08-18T19:21:43Z

@sergio-mena
sigh I misread. I thought you were saying that the change present in v0.35 was also present in v0.34 but that's not what you were saying.

tychoish · 2022-08-19T13:00:23Z

consensus/state.go

 	// TODO: We should remove next line in case we don't vote for v in case cs.ProposalBlock == nil,
 	// even if cs.LockedBlock != nil. See https://docs.tendermint.com/master/spec/.


Is this comment now stale too?

It's in v0.36.x so I assume not

tychoish · 2022-08-19T13:02:54Z

state/store.go

+// Allocate empty Consensus params at compile time to avoid multiple allocations during runtime
+var (
+	empty   = types.ConsensusParams{}
+	emptypb = tmproto.ConsensusParams{}
+)


Is this copped from somewhere else? It seems dubious that this would actually save any measurable memory or time, and just opens us up to weird testing flukes. If there's a good way to omit this pattern, I would.

Yeah it's copied from the original PR

Agree with Sam. This is saving us almost nothing. As far as I know, global variables are not allocated at compile time. I'm not really sure I know what that means. They're allocated when the program starts. This just means they're allocated only once which really saves us so little here.

sergio-mena

LGTM

williambanfield · 2022-08-19T14:24:48Z

state/store.go

+				var params types.ConsensusParams
+				params, err = store.LoadConsensusParams(h)


Suggested change

var params types.ConsensusParams

params, err = store.LoadConsensusParams(h)

params, err := store.LoadConsensusParams(h)

Not sure the separated variable declaration gets us anything.

williambanfield · 2022-08-19T14:32:31Z

state/store.go

+// Allocate empty Consensus params at compile time to avoid multiple allocations during runtime
+var (
+	empty   = types.ConsensusParams{}
+	emptypb = tmproto.ConsensusParams{}
+)


Agree with Sam. This is saving us almost nothing. As far as I know, global variables are not allocated at compile time. I'm not really sure I know what that means. They're allocated when the program starts. This just means they're allocated only once which really saves us so little here.

consensus/state.go

williambanfield

all minor points, otherwise looks good.

Cherry pick of tendermint/tendermint#9287

cmwaters added 3 commits August 18, 2022 11:21

deduplicate consensus params

1a39d8e

Merge branch 'feature/abci++ppp' into cal/deduplicate-proto

f68261f

deduplicate consensus params

68e450e

cmwaters changed the base branch from main to feature/abci++ppp August 18, 2022 09:49

cmwaters marked this pull request as ready for review August 18, 2022 09:50

cmwaters requested a review from ebuchman as a code owner August 18, 2022 09:51

cmwaters requested a review from a team August 18, 2022 09:51

cmwaters mentioned this pull request Aug 18, 2022

v0.37 release plan #9091

Closed

39 tasks

williambanfield suggested changes Aug 18, 2022

View reviewed changes

sergio-mena mentioned this pull request Aug 18, 2022

ABCI++ Prepare/Process Proposal #9053

Closed

35 tasks

sergio-mena reviewed Aug 18, 2022

View reviewed changes

proto/tendermint/types/params.proto Show resolved Hide resolved

cmwaters added 3 commits August 19, 2022 13:25

use go native consensus params, add changelog and remove timeiotams

e477de7

Merge branch 'feature/abci++ppp' into cal/deduplicate-proto

518d048

resolve conflicts

73e0f78

cmwaters requested review from williambanfield and sergio-mena August 19, 2022 11:27

cmwaters added 2 commits August 19, 2022 14:23

fix pruning test

f20ec51

add reserved tag

fdcf50e

tychoish reviewed Aug 19, 2022

View reviewed changes

sergio-mena approved these changes Aug 19, 2022

View reviewed changes

williambanfield reviewed Aug 19, 2022

View reviewed changes

williambanfield approved these changes Aug 19, 2022

View reviewed changes

implement suggestions

824294a

cmwaters merged commit 2d8df1b into feature/abci++ppp Aug 22, 2022

cmwaters deleted the cal/deduplicate-proto branch August 22, 2022 08:50

JayT106 mentioned this pull request Aug 24, 2022

regression: block.TimeIotaMs is 0 on exported genesis cosmos/cosmos-sdk#13024

Closed

tnasu mentioned this pull request Dec 20, 2022

Backport e2e-test of the latest tendermint main branch Finschia/ostracon#536

Merged

thanethomson mentioned this pull request Dec 23, 2022

v0.37 release plan cometbft/cometbft#41

Closed

38 tasks

jchappelow mentioned this pull request Sep 13, 2023

split kwild / kwil-admin kwilteam/kwil-db#281

Merged

5 tasks

cmwaters mentioned this pull request Sep 18, 2023

feat!: deduplicate consensus params #9287 celestiaorg/celestia-core#1082

Merged

cmwaters added a commit to celestiaorg/celestia-core that referenced this pull request Sep 20, 2023

feat!: deduplicate consensus params #9287 (#1082)

4132340

Cherry pick of tendermint/tendermint#9287

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proto: deduplicate consensus params #9287

proto: deduplicate consensus params #9287

cmwaters commented Aug 18, 2022 •

edited

sergio-mena commented Aug 18, 2022

sergio-mena commented Aug 18, 2022

williambanfield commented Aug 18, 2022

williambanfield commented Aug 18, 2022

williambanfield left a comment

sergio-mena commented Aug 18, 2022

sergio-mena commented Aug 18, 2022

sergio-mena commented Aug 18, 2022 •

edited

williambanfield commented Aug 18, 2022 •

edited

tychoish Aug 19, 2022

cmwaters Aug 19, 2022

tychoish Aug 19, 2022

cmwaters Aug 19, 2022

williambanfield Aug 19, 2022

sergio-mena left a comment

williambanfield Aug 19, 2022

williambanfield Aug 19, 2022

williambanfield left a comment

		// TODO: We should remove next line in case we don't vote for v in case cs.ProposalBlock == nil,
		// even if cs.LockedBlock != nil. See https://docs.tendermint.com/master/spec/.

		var params types.ConsensusParams
		params, err = store.LoadConsensusParams(h)

	var params types.ConsensusParams
	params, err = store.LoadConsensusParams(h)
	params, err := store.LoadConsensusParams(h)

proto: deduplicate consensus params #9287

proto: deduplicate consensus params #9287

Conversation

cmwaters commented Aug 18, 2022 • edited

sergio-mena commented Aug 18, 2022

sergio-mena commented Aug 18, 2022

williambanfield commented Aug 18, 2022

williambanfield commented Aug 18, 2022

williambanfield left a comment

Choose a reason for hiding this comment

sergio-mena commented Aug 18, 2022

sergio-mena commented Aug 18, 2022

sergio-mena commented Aug 18, 2022 • edited

williambanfield commented Aug 18, 2022 • edited

tychoish Aug 19, 2022

Choose a reason for hiding this comment

cmwaters Aug 19, 2022

Choose a reason for hiding this comment

tychoish Aug 19, 2022

Choose a reason for hiding this comment

cmwaters Aug 19, 2022

Choose a reason for hiding this comment

williambanfield Aug 19, 2022

Choose a reason for hiding this comment

sergio-mena left a comment

Choose a reason for hiding this comment

williambanfield Aug 19, 2022

Choose a reason for hiding this comment

williambanfield Aug 19, 2022

Choose a reason for hiding this comment

williambanfield left a comment

Choose a reason for hiding this comment

cmwaters commented Aug 18, 2022 •

edited

sergio-mena commented Aug 18, 2022 •

edited

williambanfield commented Aug 18, 2022 •

edited