Fix condition check for tx key dissemination #149
Conversation
internal/consensus/state.go
Outdated
| @@ -1049,7 +1049,7 @@ func (cs *State) handleMsg(ctx context.Context, mi msgInfo, fsyncUponCompletion | |||
| // once proposal is set, we can receive block parts | |||
| err = cs.setProposal(msg.Proposal, mi.ReceiveTime) | |||
| // See if we can try creating the proposal block if keys exist | |||
| if err != nil && cs.config.GossipTransactionKeyOnly && cs.privValidatorPubKey != nil { | |||
| if err == nil && cs.config.GossipTransactionKeyOnly && cs.privValidatorPubKey != nil { | |||
There was a problem hiding this comment.
Oh, interesting...
Could we add a unit test for this to assert if Tx key Dissemination was used? Could even just refactor out this snippet to be a helper if that makes it easier
There was a problem hiding this comment.
Yeah +1 to maybe find a way to test if gossip is on or not under given condition
internal/consensus/state.go
Outdated
| @@ -1049,7 +1049,7 @@ func (cs *State) handleMsg(ctx context.Context, mi msgInfo, fsyncUponCompletion | |||
| // once proposal is set, we can receive block parts | |||
| err = cs.setProposal(msg.Proposal, mi.ReceiveTime) | |||
| // See if we can try creating the proposal block if keys exist | |||
| if err != nil && cs.config.GossipTransactionKeyOnly && cs.privValidatorPubKey != nil { | |||
| if err == nil && cs.config.GossipTransactionKeyOnly && cs.privValidatorPubKey != nil { | |||
There was a problem hiding this comment.
error == nil means there is a valid proposal already
error != nil means we don't have a valid proposal
So this change means we will try build the blocks and complete for the proposal only if the we have a valid proposal, which make sense to me
There was a problem hiding this comment.
exactly - error == nil means we have a valid proposal, so we should try to build the block from mempool. otherwise we fall back to old behavior where we wait for blockparts to be gossiped
internal/consensus/state.go
Outdated
| @@ -1049,7 +1049,7 @@ func (cs *State) handleMsg(ctx context.Context, mi msgInfo, fsyncUponCompletion | |||
| // once proposal is set, we can receive block parts | |||
| err = cs.setProposal(msg.Proposal, mi.ReceiveTime) | |||
| // See if we can try creating the proposal block if keys exist | |||
| if err != nil && cs.config.GossipTransactionKeyOnly && cs.privValidatorPubKey != nil { | |||
| if err == nil && cs.config.GossipTransactionKeyOnly && cs.privValidatorPubKey != nil { | |||
There was a problem hiding this comment.
Yeah +1 to maybe find a way to test if gossip is on or not under given condition
| @@ -2924,6 +2924,49 @@ func TestStateOutputsBlockPartsStats(t *testing.T) { | |||
|
|
|||
| } | |||
|
|
|||
| func TestGossipTransactionKeyOnlyConfig(t *testing.T) { | |||
Describe your changes and provide context
In a previous PR, the intent was:
If there is an error in setProposal then it doesn't make sense to continue with handling proposalHowever, it was incorrectly set here: https://github.com/sei-protocol/sei-tendermint/pull/87/files#diff-c27a58d5a8b3c15166169543b3cdb94da91bb60efa27609ef446719474af78c5R1059 meaning we haven't been using tx-key dissemination for a while.
Testing performed to validate your change
Deployed on loadtest cluster, verify metrics work:
http://grafana.sei-loadtest-testnet.seinetwork.io:9090/graph?g0.expr=tendermint_consensus_proposal_block_created_on_propose&g0.tab=0&g0.stacked=0&g0.show_exemplars=0&g0.range_input=1h
Chain ran fine over weekend:http://grafana.sei-loadtest-testnet.seinetwork.io/d/ecVP_k37z/sei-chain-monitoring?orgId=1&refresh=5s&var-chain_id=psu-gossip-tx-test&var-public_dns=All&from=now-2d&to=now-1h