-
Notifications
You must be signed in to change notification settings - Fork 781
Remove proof details from x/sync #4247
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
base: master
Are you sure you want to change the base?
Conversation
83e15dc
to
9ab50fe
Compare
// which means [workItem.end] isn't Nothing. Contradiction. | ||
lastReceivedKey := largestHandledKey.Value() | ||
|
||
if len(endProof) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All changes this line and below are cut-and-paste from x/sync/manager.go
|
||
ctx := context.Background() | ||
syncer, err := NewManager(ManagerConfig{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these tests that don't require the manager will be moved to a different package eventually
3f6b925
to
582d1e1
Compare
582d1e1
to
c48fa13
Compare
There was a problem hiding this comment.
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 moves proof verification and handling logic from the x/sync package to the x/merkledb package. The main purpose is to improve separation of concerns by moving database-specific operations closer to the database implementation and simplifying the sync manager interface.
- Removes database configuration attributes (hasher, tokenSize, BranchFactor) from sync manager and uses database methods instead
- Moves range proof verification and findNextKey functionality to database methods
- Updates proof marshaling to use binary format instead of protobuf in tests
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
x/sync/sync_test.go | Updates tests to call database methods directly instead of sync manager methods |
x/sync/network_server_test.go | Changes proof marshaling from protobuf to binary format |
x/sync/manager.go | Removes database configuration fields and moves proof handling to database |
x/sync/client_test.go | Updates proof handling to use binary marshaling |
x/merkledb/proof_test.go | Adds maxLength parameter to proof verification methods |
x/merkledb/proof.go | Updates proof methods to use internal protobuf marshaling |
x/merkledb/history_test.go | Adds maxLength parameter to verification calls |
x/merkledb/db_test.go | Updates database tests with new return values from commit methods |
x/merkledb/db.go | Implements proof verification and findNextKey methods in database |
proto/sync/sync.proto | Removes unused gRPC service definitions and related messages |
proto/pb/sync/sync.pb.go | Generated code reflecting protobuf changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
f51a039
to
8e1c529
Compare
c48fa13
to
942f6da
Compare
942f6da
to
f6fa493
Compare
Why this should be merged
Some of the manager attributes (e.g. hasher, tokenSize) really belong to the database. Additionally, the manager explicitly uses merkledb proof fields for range proof verification and finding the next key.
How this works
Moves range proof verification to a database method. The entire find next key functionality is also moved, with a subtle change to
completeWorkItem
. All the proto changes are related to the gRPC stuff inx/sync/g_db
How this was tested
Existing UT
Need to be documented in RELEASES.md?
No