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

added registry index for fetched records and updated proof endpoints to use the log length and registry index #187

Merged
merged 26 commits into from
Aug 28, 2023

Conversation

calvinrp
Copy link
Collaborator

@calvinrp calvinrp commented Aug 25, 2023

The registry index for a published record is going to be different than the VecLog's Node(usize) value that is required for the log inclusion proof. For now, keeping the existing pattern of using a HashMap (could be a Vec if added back in the same order) to lookup the value in memory.

To make it easier to adjust the size of the integer for the log_length and registry_index, I added a type alias RegistryIndex.

As always, suggestions appreciated. This was more of a change than I expected.

@calvinrp
Copy link
Collaborator Author

I need to investigate why it is failing on the inclusion proof on the Postgres test. Trying to reproduce.

Copy link
Collaborator

@lann lann left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, pending CI failure.

crates/api/src/v1/proof.rs Outdated Show resolved Hide resolved
crates/server/src/api/v1/proof.rs Outdated Show resolved Hide resolved
@calvinrp
Copy link
Collaborator Author

calvinrp commented Aug 26, 2023

Should be good to go now.

The Postgres test failure was tricky to track down, since I thought it was failing on the inclusion of the log. It was the map inclusion proof that was failing intermittently. When constructing the proof bundle, the leafs need to be added in the exact same order as the client provided the log leafs. It was easier to enforce the ordering in Rust than mess with the diesel Rust Postgres query.

@calvinrp
Copy link
Collaborator Author

calvinrp commented Aug 26, 2023

Also, updated the /fetch/logs endpoint to use logLength instead of checkpointId. Hopefully, this is not a controversial change. It should make things easier.

The updated rendered OpenAPI spec.

This PR should be compatible with the existing Postgres DB data and shouldn't require starting fresh with the Dogfood Registry deployment.

@calvinrp calvinrp requested a review from lann August 26, 2023 18:23
Copy link
Collaborator

@lann lann left a comment

Choose a reason for hiding this comment

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

Could you give this a pass through cargo clippy? If you use vscode you can make it the default linter with "rust-analyzer.check.command": "clippy"

crates/api/src/v1/fetch.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@lann lann left a comment

Choose a reason for hiding this comment

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

Not sure I understand the new RegistryIndex param threaded through CoreService but otherwise lgtm.

@calvinrp calvinrp merged commit 99f1b62 into bytecodealliance:main Aug 28, 2023
6 checks passed
@calvinrp calvinrp deleted the log-index branch August 28, 2023 14:41
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.

2 participants