Skip to content

Report range limits#232

Merged
brunotm merged 9 commits intomasterfrom
bm/DS-2118-add-report-range-limits
Mar 19, 2026
Merged

Report range limits#232
brunotm merged 9 commits intomasterfrom
bm/DS-2118-add-report-range-limits

Conversation

@brunotm
Copy link
Contributor

@brunotm brunotm commented Mar 16, 2026

  • report range limits
  • move disableNilStreamValues from encoder options to ChannelDefinition.DisableNilStreamValues
  • re-add report options cache

@brunotm brunotm requested a review from a team as a code owner March 16, 2026 14:28
@github-actions
Copy link

👋 brunotm, thanks for creating this pull request!

To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team.

Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks!

calvwang9
calvwang9 previously approved these changes Mar 17, 2026
Copy link
Contributor

@calvwang9 calvwang9 left a comment

Choose a reason for hiding this comment

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

looks like we need to update the type in chainlink-common too

@brunotm brunotm requested review from akuzni2, calvwang9 and ro-tex March 17, 2026 15:44
Comment on lines +97 to +99
if p.OptsCache.Len() != len(outcome.ChannelDefinitions) {
p.Logger.Warnw("OptsCache length mismatch with ChannelDefinitions length, resetting OptsCache", "optsCacheLen", p.OptsCache.Len(), "channelDefinitionsLen", len(outcome.ChannelDefinitions), "stage", "Outcome", "seqNr", outctx.SeqNr)
p.OptsCache.ResetTo(outcome.ChannelDefinitions)
Copy link
Contributor

Choose a reason for hiding this comment

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

given that we are introducing the ability to remove tombstoned channels, is relying on the len(outcome.ChannelDefinitions) still safe, i.e. if we add 1, remove 1 channel this wouldnt trigger a cache refresh right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

@brunotm brunotm Mar 18, 2026

Choose a reason for hiding this comment

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

any removal is handled in the same manner after the reset check which is only done to account for cold starts and b/g handovers

removedChannelIDs := make([]llotypes.ChannelID, 0, len(removeChannelVotesByID))
for channelID, voteCount := range removeChannelVotesByID {
if voteCount <= p.F {
continue
}
removedChannelIDs = append(removedChannelIDs, channelID)
delete(outcome.ChannelDefinitions, channelID)
p.OptsCache.Remove(channelID)

@brunotm
Copy link
Contributor Author

brunotm commented Mar 18, 2026

looks like we need to update the type in chainlink-common too

that pr was already merged.

@brunotm brunotm requested a review from calvwang9 March 18, 2026 08:28
akuzni2
akuzni2 previously approved these changes Mar 18, 2026
akuzni2
akuzni2 previously approved these changes Mar 18, 2026
calvwang9
calvwang9 previously approved these changes Mar 19, 2026
…amValues to enable compatible default behaviour
@brunotm brunotm dismissed stale reviews from calvwang9 and akuzni2 via 5337fa9 March 19, 2026 14:24
@brunotm brunotm force-pushed the bm/DS-2118-add-report-range-limits branch from 5337fa9 to bdc028a Compare March 19, 2026 17:12
@brunotm brunotm merged commit 701d472 into master Mar 19, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants