Skip to content

Conversation

rrazvan1
Copy link
Contributor

@rrazvan1 rrazvan1 commented Jan 31, 2025

Why this should be merged

Fixing proof verification bugs:

  1. exclusion proofs -> passing when providing a proof path which has the last node: a sibling of an ancestor of the existing node
  2. exclusion proofs -> passing when providing not fully extended proof
    This issues were mainly affecting the simple proofs verification, but also change/range proofs.

And also, simplifying the code for better readability and maintainability.

How this works

  • moving the main logic of proofs validation inside verifyProofPath(proofPath []ProofNode, key Key) and simplifying the specific proof verifications.
  • added new specific errors
  • more details within comments
  • unifying change/range proof flows

How this was tested

UTs

Need to be documented in RELEASES.md?

@rrazvan1 rrazvan1 marked this pull request as draft January 31, 2025 13:39
@rrazvan1 rrazvan1 force-pushed the fix-proof-verification branch 2 times, most recently from 33e69e1 to 923f088 Compare February 3, 2025 14:20
@rrazvan1 rrazvan1 marked this pull request as ready for review February 3, 2025 16:33
Copy link

This PR has become stale because it has been open for 30 days with no activity. Adding the lifecycle/frozen label will cause this PR to ignore lifecycle events.

{
name: "happy path",
malform: func(*Proof) {},
expectedErr: nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We can remove this line since the zero value is already nil... or not if you want to be explicit (I would personally leave it as nil but I can see the reasoning for not doing so). Leaving this to developer preference.

@rrazvan1 rrazvan1 force-pushed the fix-proof-verification branch from 1fa3412 to e087730 Compare March 19, 2025 16:59
@rrazvan1 rrazvan1 requested a review from joshua-kim March 24, 2025 16:25
@rrazvan1 rrazvan1 requested a review from joshua-kim April 11, 2025 14:55
@rrazvan1 rrazvan1 force-pushed the fix-proof-verification branch from 8ec99b0 to a278f75 Compare April 11, 2025 14:58
@rrazvan1 rrazvan1 force-pushed the fix-proof-verification branch from 430e031 to 842f70d Compare April 22, 2025 15:57
@rrazvan1 rrazvan1 force-pushed the fix-proof-verification branch from d64460a to 76ab1b8 Compare April 24, 2025 08:18
@rrazvan1 rrazvan1 requested a review from joshua-kim April 24, 2025 08:34
@rrazvan1 rrazvan1 moved this from Backlog 🗄️ to In Review 👀 in avalanchego Apr 28, 2025
@joshua-kim joshua-kim enabled auto-merge April 30, 2025 16:53
@joshua-kim joshua-kim removed the request for review from StephenButtolph April 30, 2025 16:54
Copy link
Contributor

@StephenButtolph StephenButtolph left a comment

Choose a reason for hiding this comment

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

(didn't actually review, x/ shouldn't require multiple reviews.)

@joshua-kim joshua-kim added this pull request to the merge queue Apr 30, 2025
Merged via the queue into master with commit 2bf5dc4 Apr 30, 2025
24 checks passed
@joshua-kim joshua-kim deleted the fix-proof-verification branch April 30, 2025 17:15
@github-project-automation github-project-automation bot moved this from In Review 👀 to Done ✅ in avalanchego Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants