Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: enable new reactor in the e2e test #1657

Merged
merged 18 commits into from
Mar 24, 2025
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix: tests
evan-forbes committed Mar 10, 2025

Verified

This commit was signed with the committer’s verified signature.
evan-forbes Evan Forbes
commit 7cd22b41996dc3c956b2e9d8abdc000a727ad7cd
33 changes: 17 additions & 16 deletions consensus/propagation/commitment_state.go
Original file line number Diff line number Diff line change
@@ -106,19 +106,19 @@ func (p *ProposalCache) getAllState(height int64, round int32) (*proptypes.Compa
cachedProps, has := p.proposals[height]
cachedProp, hasRound := cachedProps[round]

// // if the round is less than zero, then they're asking for the latest
// // proposal
// if round < -1 && len(cachedProps) > 0 {
// // get the latest round
// var latestRound int32
// for r := range cachedProps {
// if r > latestRound {
// latestRound = r
// }
// }
// cachedProp = cachedProps[latestRound]
// hasRound = true
// }
// if the round is less than zero, then they're asking for the latest
// proposal
if round < -1 && len(cachedProps) > 0 {
// get the latest round
var latestRound int32
for r := range cachedProps {
if r > latestRound {
latestRound = r
}
}
cachedProp = cachedProps[latestRound]
hasRound = true
}

var hasStored *types.BlockMeta
if height < p.currentHeight {
@@ -184,9 +184,11 @@ func (p *ProposalCache) DeleteRound(height int64, round int32) {
}
}

// prune keeps the past X proposals / blocks in memory while deleting the rest.
// prune deletes all cached compact blocks for heights less than the provided
// height and round.
//
// todo: also prune rounds
// todo: also prune rounds. this requires prune in the consensus reactor after
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is pruning rounds too no? since we delete all the proposals for that round

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it won't prune rounds if we're stuck on the same height

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then it's fine to keep it the same way because we don't expect to be stuck on the same height forever and if we do, nodes OOMing will be the least of our issues I guess

// moving rounds.
func (p *ProposalCache) prune(pruneHeight int64) {
p.pmtx.Lock()
defer p.pmtx.Unlock()
@@ -195,5 +197,4 @@ func (p *ProposalCache) prune(pruneHeight int64) {
delete(p.proposals, height)
}
}
// todo: also prune rounds
}
17 changes: 5 additions & 12 deletions consensus/propagation/commitment_state_test.go
Original file line number Diff line number Diff line change
@@ -177,7 +177,7 @@ func TestProposalCache_GetProposalWithRequests(t *testing.T) {
{
name: "Use negative round => fetch the latest round for height=5 which is 1",
height: 5,
round: -1, // meaning "latest round"
round: -2, // meaning "latest round"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need this

wantProposal: prop2,
wantBitArray: bits.NewBitArray(3),
wantOk: true,
@@ -290,25 +290,18 @@ func TestProposalCache_prune(t *testing.T) {
pc.currentHeight = 12
pc.currentRound = 2

pc.prune(12 - 1) // keep only the past 1 height, and past 1 round in memory
pc.prune(11)

// Expect that everything older than height=11 is removed.
// So height=10 is gone
require.Nil(t, pc.proposals[10])

// For height=11, we keep it because it's within 1 height of current (12)
require.NotNil(t, pc.proposals[11])

// For height=11, we also prune the older rounds. Because keepRecentRounds=1
// That means we only keep round=2 if it exists, or the maximum round if that's less.
// But the maximum round we inserted is 2, so we keep only that.
require.NotNil(t, pc.proposals[11][2], "round=2 of height=11 should remain")
require.Nil(t, pc.proposals[11][0], "round=0 of height=11 should have been pruned")
require.Nil(t, pc.proposals[11][1], "round=1 of height=11 should have been pruned")

// For the current height=12, we do not prune all but the last round.
// The code specifically skips pruning the current height's older rounds
require.NotNil(t, pc.proposals[11][0], "round=0 of height=11 should have been pruned")
require.NotNil(t, pc.proposals[11][1], "round=1 of height=11 should have been pruned")
require.NotNil(t, pc.proposals[12][0], "round=0 of current height=12 should remain")
require.NotNil(t, pc.proposals[12][1], "round=1 of current height=12 should remain")
require.NotNil(t, pc.proposals[12][2], "round=2 of current height=12 is definitely still there")
require.NotNil(t, pc.proposals[12][2], "round=2 of current height=12 should remain")
}
2 changes: 2 additions & 0 deletions consensus/state.go
Original file line number Diff line number Diff line change
@@ -538,6 +538,8 @@ func (cs *State) SetProposalAndBlock(

func (cs *State) updateHeight(height int64) {
cs.metrics.Height.Set(float64(height))
cs.mtx.Lock()
defer cs.mtx.Unlock()
cs.Height = height
}

2 changes: 2 additions & 0 deletions libs/bits/bit_array.go
Original file line number Diff line number Diff line change
@@ -36,6 +36,8 @@ func (bA *BitArray) Size() int {
if bA == nil {
return 0
}
bA.mtx.Lock()
defer bA.mtx.Unlock()
return bA.Bits
}