consensus: correctly save reference to previous height precommits #9760

williambanfield · 2022-11-23T23:52:52Z

This change reduces the number of Precommit messages sent to peers by 50%.

During the ApplyNewRoundStepMessage, we update the known state of the peer sending us the message.

We set the value of ps.PRS.Precommits to nil in this method if the peer is entering a new height or round.

Line 1368 in 34ca3fb

ps.PRS.Precommits = nil

We then assign ps.PRS.LastCommit = ps.PRS.Precommits if the peer is entering a new height only - this does not happen during just a round change. This therefore results in ps.PRS.LastCommit having the value nil.

When the LastCommit bit field is seen as nil in the reactor, an empty bit field is initialized.

tendermint/consensus/reactor.go

Line 1273 in 34ca3fb

ps.PRS.LastCommit = bits.NewBitArray(numValidators)

The code responsible for gossiping votes from the previous height uses this LastCommit value and, seeing an empty LastCommit will resend every Precommit from the previous height since it lost the information it previously had detailing which precommits from the previous height the peer had.

This can be seen in the code responsible for gossiping precommits from the previous height:

tendermint/consensus/reactor.go

Line 773 in 34ca3fb

if ps.PickSendVote(rs.LastCommit) {

Where this code grabs the, previously nil, LastCommit bit field:

tendermint/consensus/reactor.go

Lines 1204 to 1212 in 34ca3fb

    
           if ps.PRS.Height == height+1 { 
        
           	if ps.PRS.LastCommitRound == round { 
        
           		switch votesType { 
        
           		case tmproto.PrevoteType: 
        
           			return nil 
        
           		case tmproto.PrecommitType: 
        
           			return ps.PRS.LastCommit 
        
           		} 
        
           	}

PR checklist

Tests written/updated, or no tests needed
CHANGELOG_PENDING.md updated, or no changelog entry needed
Updated relevant documentation (docs/) and code comments, or no
documentation updates needed

cmwaters

Nice catch 🎉

We should definitely backport this as well (and add a changelog)

) This change reduces the number of Precommit messages sent to peers by 50%. During the `ApplyNewRoundStepMessage`, we update the known state of the peer sending us the message. We set the value of `ps.PRS.Precommits` to nil in this method if the peer is entering a new height or round. https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L1368 We then assign `ps.PRS.LastCommit = ps.PRS.Precommits` if the peer is entering a new height only - this does not happen during just a round change. This therefore results in `ps.PRS.LastCommit` having the value `nil`. When the `LastCommit` bit field is seen as `nil` in the reactor, an empty bit field is initialized. https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L1273 The code responsible for gossiping votes from the previous height uses this `LastCommit` value and, seeing an empty `LastCommit` will resend every `Precommit` from the previous height since it lost the information it previously had detailing which precommits from the previous height the peer had. This can be seen in the code responsible for gossiping precommits from the previous height: https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L773 Where this code grabs the, previously `nil`, `LastCommit` bit field: https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L1204-L1212 --- #### PR checklist - [ ] Tests written/updated, or no tests needed - [ ] `CHANGELOG_PENDING.md` updated, or no changelog entry needed - [ ] Updated relevant documentation (`docs/`) and code comments, or no documentation updates needed (cherry picked from commit da204d3) # Conflicts: # CHANGELOG_PENDING.md

…ckport #9760) (#9775) * consensus: correctly save reference to previous height precommits (#9760) This change reduces the number of Precommit messages sent to peers by 50%. During the `ApplyNewRoundStepMessage`, we update the known state of the peer sending us the message. We set the value of `ps.PRS.Precommits` to nil in this method if the peer is entering a new height or round. https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L1368 We then assign `ps.PRS.LastCommit = ps.PRS.Precommits` if the peer is entering a new height only - this does not happen during just a round change. This therefore results in `ps.PRS.LastCommit` having the value `nil`. When the `LastCommit` bit field is seen as `nil` in the reactor, an empty bit field is initialized. https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L1273 The code responsible for gossiping votes from the previous height uses this `LastCommit` value and, seeing an empty `LastCommit` will resend every `Precommit` from the previous height since it lost the information it previously had detailing which precommits from the previous height the peer had. This can be seen in the code responsible for gossiping precommits from the previous height: https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L773 Where this code grabs the, previously `nil`, `LastCommit` bit field: https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L1204-L1212 --- #### PR checklist - [ ] Tests written/updated, or no tests needed - [ ] `CHANGELOG_PENDING.md` updated, or no changelog entry needed - [ ] Updated relevant documentation (`docs/`) and code comments, or no documentation updates needed (cherry picked from commit da204d3) # Conflicts: # CHANGELOG_PENDING.md * changelog Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: William Banfield <wbanfield@gmail.com>

…ckport #9760) (#9776) * consensus: correctly save reference to previous height precommits (#9760) This change reduces the number of Precommit messages sent to peers by 50%. During the `ApplyNewRoundStepMessage`, we update the known state of the peer sending us the message. We set the value of `ps.PRS.Precommits` to nil in this method if the peer is entering a new height or round. https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L1368 We then assign `ps.PRS.LastCommit = ps.PRS.Precommits` if the peer is entering a new height only - this does not happen during just a round change. This therefore results in `ps.PRS.LastCommit` having the value `nil`. When the `LastCommit` bit field is seen as `nil` in the reactor, an empty bit field is initialized. https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L1273 The code responsible for gossiping votes from the previous height uses this `LastCommit` value and, seeing an empty `LastCommit` will resend every `Precommit` from the previous height since it lost the information it previously had detailing which precommits from the previous height the peer had. This can be seen in the code responsible for gossiping precommits from the previous height: https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L773 Where this code grabs the, previously `nil`, `LastCommit` bit field: https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L1204-L1212 Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: William Banfield <wbanfield@gmail.com>

ValarDragon · 2022-12-09T19:38:29Z

Awesome, nice job!

…mits (backport #9760) (#9775)" This reverts commit 5703cc3.

…mits (backport #9760) (#9775)" (#9945) This reverts commit 5703cc3.

* Revert "Fixed ordering of match.events in light client RPC (tendermint#9877)" * Revert "state/kvindexer: associate event attributes with events (tendermint#9759)" * Revert "consensus: correctly save reference to previous height precommits (backport tendermint#9760) (tendermint#9776)" * add peer gossip sleep (tendermint#241) (tendermint#245) * add peer gossip sleep * add changelog * Update .changelog/unreleased/bug-fixes/4-busy-loop-send-block-part.md Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> * Update .changelog/unreleased/bug-fixes/4-busy-loop-send-block-part.md --------- Co-authored-by: jhernandezb <contact@jhernandez.me> Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> (cherry picked from commit 5954e75) Co-authored-by: Sergio Mena <sergio@informal.systems> * Build changelog for v0.34.25 release Signed-off-by: Thane Thomson <connect@thanethomson.com> * Bump version --------- Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Thane Thomson <connect@thanethomson.com>

…ckport tendermint#9760) (#214) * consensus: correctly save reference to previous height precommits (backport tendermint#9760) (tendermint#9775) * consensus: correctly save reference to previous height precommits (tendermint#9760) This change reduces the number of Precommit messages sent to peers by 50%. During the `ApplyNewRoundStepMessage`, we update the known state of the peer sending us the message. We set the value of `ps.PRS.Precommits` to nil in this method if the peer is entering a new height or round. https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L1368 We then assign `ps.PRS.LastCommit = ps.PRS.Precommits` if the peer is entering a new height only - this does not happen during just a round change. This therefore results in `ps.PRS.LastCommit` having the value `nil`. When the `LastCommit` bit field is seen as `nil` in the reactor, an empty bit field is initialized. https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L1273 The code responsible for gossiping votes from the previous height uses this `LastCommit` value and, seeing an empty `LastCommit` will resend every `Precommit` from the previous height since it lost the information it previously had detailing which precommits from the previous height the peer had. This can be seen in the code responsible for gossiping precommits from the previous height: https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L773 Where this code grabs the, previously `nil`, `LastCommit` bit field: https://github.com/tendermint/tendermint/blob/34ca3fb4741cdb205a4714d345218a0d5e9fefd0/consensus/reactor.go#L1204-L1212 --- - [ ] Tests written/updated, or no tests needed - [ ] `CHANGELOG_PENDING.md` updated, or no changelog entry needed - [ ] Updated relevant documentation (`docs/`) and code comments, or no documentation updates needed (cherry picked from commit da204d3) * changelog Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: William Banfield <wbanfield@gmail.com> * Applied changes from 9760 --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: William Banfield <wbanfield@gmail.com>

save reference to previous round precommits

88e1de7

williambanfield requested a review from ebuchman as a code owner November 23, 2022 23:52

williambanfield requested a review from a team November 23, 2022 23:52

williambanfield requested review from adizere and lasarojc as code owners November 23, 2022 23:52

williambanfield changed the title ~~consensus: correctly save reference to previous round precommits~~ consensus: correctly save reference to previous height precommits Nov 24, 2022

cmwaters approved these changes Nov 24, 2022

View reviewed changes

thanethomson added S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x S:backport-to-v0.37.x Tell mergify to backport the PR to v0.37.x labels Nov 24, 2022

williambanfield and others added 2 commits November 28, 2022 15:04

changelog

3f9d731

Merge branch 'main' into wb/reduce-precommit-sends

9a989a0

williambanfield added the S:automerge Automatically merge PR when requirements pass label Nov 28, 2022

mergify bot merged commit da204d3 into main Nov 28, 2022

mergify bot deleted the wb/reduce-precommit-sends branch November 28, 2022 20:13

mergify bot mentioned this pull request Nov 28, 2022

consensus: correctly save reference to previous height precommits (backport #9760) #9775

Merged

mergify bot mentioned this pull request Nov 28, 2022

consensus: correctly save reference to previous height precommits (backport #9760) #9776

Merged

cmwaters mentioned this pull request Dec 13, 2022

consensus: correctly save reference to previous height precommits (backport #9760) (#9776) celestiaorg/celestia-core#911

Merged

thanethomson mentioned this pull request Dec 14, 2022

Bandwidth optimization #9880

Closed

16 tasks

thanethomson added a commit that referenced this pull request Dec 21, 2022

Revert "consensus: correctly save reference to previous height precom…

2deee10

…mits (backport #9760) (#9775)" This reverts commit 5703cc3.

thanethomson added a commit that referenced this pull request Dec 22, 2022

Revert "consensus: correctly save reference to previous height precom…

9169ea2

…mits (backport #9760) (#9775)" (#9945) This reverts commit 5703cc3.

lasarojc mentioned this pull request Dec 23, 2022

Bandwidth optimization cometbft/cometbft#30

Closed

3 tasks

jmalicevic mentioned this pull request Jan 11, 2023

consensus:correctly save reference to previous height precommits (backport #9760) cometbft/cometbft#96

Closed

jmalicevic mentioned this pull request Jan 26, 2023

consensus: correctly save reference to previous height precommits (backport #9760) cometbft/cometbft#214

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

consensus: correctly save reference to previous height precommits #9760

consensus: correctly save reference to previous height precommits #9760

williambanfield commented Nov 23, 2022 •

edited

Loading

cmwaters left a comment

ValarDragon commented Dec 9, 2022

	if ps.PRS.Height == height+1 {
	if ps.PRS.LastCommitRound == round {
	switch votesType {
	case tmproto.PrevoteType:
	return nil
	case tmproto.PrecommitType:
	return ps.PRS.LastCommit
	}
	}

consensus: correctly save reference to previous height precommits #9760

consensus: correctly save reference to previous height precommits #9760

Conversation

williambanfield commented Nov 23, 2022 • edited Loading

PR checklist

cmwaters left a comment

Choose a reason for hiding this comment

ValarDragon commented Dec 9, 2022

williambanfield commented Nov 23, 2022 •

edited

Loading