Skip to content

Revert "Deduplicate partial and full GMessage validation logic (#1010)"#1014

Closed
Kubuxu wants to merge 1 commit into
mainfrom
revert/1010
Closed

Revert "Deduplicate partial and full GMessage validation logic (#1010)"#1014
Kubuxu wants to merge 1 commit into
mainfrom
revert/1010

Conversation

@Kubuxu

@Kubuxu Kubuxu commented Jun 12, 2025

Copy link
Copy Markdown
Contributor

This reverts commit 54abe92.

We've observed an anomaly in metrics when running this code.
Reverting it temporarily to prevent it from being included in a release until the root cause is identified.

This reverts commit 54abe92.

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
Copilot AI review requested due to automatic review settings June 12, 2025 16:28
@github-project-automation github-project-automation Bot moved this to Todo in F3 Jun 12, 2025

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reverts commit 54abe92 that deduplicated partial and full GMessage validation logic in order to address anomalies observed in metrics. The changes restore separate validation implementations and associated types across the codebase.

  • Restores the standalone partial validator in pmsg/partial_validator.go.
  • Reverts type and pointer changes from gpbft.PartialGMessage to pmsg.PartialGMessage in multiple files.
  • Reinstates the original validator and participant implementations with separate handling for full and partial messages.

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.

File Description
pmsg/partial_validator.go Adds back the dedicated partial validator implementation and reintroduces its comments.
pmsg/partial_msg.go Reverts type changes and pointer usage for partial messages, ensuring consistency.
gpbft/validator.go Reverts changes in validation logic and error/wrapping messages to restore original behavior.
(Other files) Update references from gpbft.PartialGMessage to pmsg.PartialGMessage across the project.
Comments suppressed due to low confidence (3)

pmsg/partial_validator.go:33

  • [nitpick] Consider expanding this TODO comment with a reference to a tracked issue or an outline of the future refactoring plan so that the technical debt is clearly documented for future improvements.
// TODO: Reduce duplicate code between this and the full validator. Doing this ...

pmsg/partial_msg.go:139

  • [nitpick] Verify that the conversion to using pointers for PartialGMessage in the LRU cache and associated helper functions is fully consistent, ensuring no nil dereference issues occur during concurrent processing.
inferJustificationVoteValue(pgmsg.PartialGMessage)

gpbft/validator.go:90

  • [nitpick] Double-check that the revised error wrapping and logging messages maintain our standard format across all validators, so that future debugging and metric attribution remain consistent.
if cacheMessage { if _, err := v.cache.Add(msg.Vote.Instance, messageCacheNamespace, buf.Bytes()); err != nil {

@Kubuxu Kubuxu requested a review from BigLep June 12, 2025 16:29
Kubuxu added a commit that referenced this pull request Jun 12, 2025
This commit retracts 0.8.5 (a brand new release) due to anomaly in
metrics caused by #1010 (reverted in #1014).
The 0.8.5 release is not used by any implementation at the time of
retraction.
Follow up release 0.8.6 is on its way.

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
Kubuxu added a commit that referenced this pull request Jun 12, 2025
This commit retracts 0.8.5 (a brand new release) due to anomaly in
metrics caused by #1010 (reverted in #1014).
The 0.8.5 release is not used by any implementation at the time of
retraction.
Follow up release 0.8.6 is on its way.

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
@github-project-automation github-project-automation Bot moved this from Todo to In review in F3 Jun 12, 2025
@Kubuxu

Kubuxu commented Jun 12, 2025

Copy link
Copy Markdown
Contributor Author

After trawling through the logs, it seems like input to signature validation is wrong somewhere in the new code.
I don't think there is a need to revert all of it right now as it seems it fails closed. We can fix it and then retract the release.

@Kubuxu Kubuxu closed this Jun 12, 2025
@github-project-automation github-project-automation Bot moved this from In review to Done in F3 Jun 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants