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.

Sign up for GitHub

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

Jump to bottom

feat(store): GetNoteAuthenticationInfo endpoint #421

Merged
merged 5 commits into from
Aug 23, 2024
Merged

feat(store): GetNoteAuthenticationInfo endpoint #421

merged 5 commits into from
Aug 23, 2024

Conversation

polydez
Copy link
Contributor

@polydez polydez commented Jul 26, 2024 •
edited by Mirko-von-Leipzig
Loading

Resolves: #395

This PR adds GetNoteAuthenticationInfo endpoint. Prior this we had to use GetNotesById endpoint for getting inclusion proofs, but it returns a lot of unnecessary information for such task.

Also GetBlockInputs endpoint now returns not only found requested note IDs, but also their inclusion proofs.

@polydez polydez requested a review from bobbinth July 26, 2024 14:59
@polydez polydez marked this pull request as ready for review July 26, 2024 15:49
@polydez polydez force-pushed the polydez-note-inclusion-proofs branch from ee7efd8 to 27f421b Compare July 26, 2024 16:19
bobbinth
bobbinth reviewed Jul 26, 2024
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good. This is not a full review yet - but I left a couple of comments inline. The main one is that note inclusion proof is actually a bit more complex than just a single Merkle path.

proto/note.proto Outdated Show resolved Hide resolved
proto/rpc.proto Outdated Show resolved Hide resolved
proto/store.proto Outdated Show resolved Hide resolved
@polydez polydez mentioned this pull request Aug 1, 2024
Merged
@polydez polydez requested a review from bobbinth August 2, 2024 09:13
Mirko-von-Leipzig
Mirko-von-Leipzig approved these changes Aug 7, 2024
View reviewed changes
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

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

LGTM

crates/store/src/db/sql.rs Outdated Show resolved Hide resolved
crates/store/src/db/sql.rs Outdated Show resolved Hide resolved
crates/store/src/db/sql.rs Show resolved Hide resolved
bobbinth
bobbinth requested changes Aug 8, 2024
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good. Not a full review but I did leave some comments inline. The main one is I think just NoteInclusionProof objects are not sufficient we are missing the MMR part of the proofs.

proto/rpc.proto Outdated Show resolved Hide resolved
proto/responses.proto Outdated Show resolved Hide resolved
proto/responses.proto Outdated Show resolved Hide resolved
crates/block-producer/src/batch_builder/batch.rs Outdated Show resolved Hide resolved
@Mirko-von-Leipzig Mirko-von-Leipzig linked an issue Aug 13, 2024 that may be closed by this pull request
[Feature]: Add specialized endpoint for getting note inclusion paths #395
Closed
@Mirko-von-Leipzig Mirko-von-Leipzig changed the title Implemented GetNoteInclusionProofs endpoint in both Store and RPC feat(store): GetNoteInclusionProofs endpoint Aug 14, 2024
@Mirko-von-Leipzig Mirko-von-Leipzig mentioned this pull request Aug 14, 2024
Open
Mirko-von-Leipzig
Mirko-von-Leipzig reviewed Aug 14, 2024
View reviewed changes
crates/rpc-proto/proto/responses.proto Outdated Show resolved Hide resolved
crates/rpc-proto/proto/responses.proto Outdated Show resolved Hide resolved
crates/store/src/server/api.rs Outdated Show resolved Hide resolved
bobbinth
bobbinth requested changes Aug 17, 2024
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left some comments inline (and I think answered all outstanding questions - but let me know if I missed something).

proto/mmr.proto Outdated Show resolved Hide resolved
proto/responses.proto Outdated Show resolved Hide resolved
crates/store/src/types.rs Outdated Show resolved Hide resolved
crates/rpc-proto/proto/responses.proto Outdated Show resolved Hide resolved
crates/store/src/db/sql.rs Outdated Show resolved Hide resolved
crates/store/src/state.rs Outdated Show resolved Hide resolved
crates/store/src/state.rs Outdated Show resolved Hide resolved
crates/proto/src/domain/mod.rs Show resolved Hide resolved
crates/proto/src/domain/notes.rs Outdated Show resolved Hide resolved
crates/block-producer/src/store/mod.rs Outdated Show resolved Hide resolved
@Mirko-von-Leipzig Mirko-von-Leipzig mentioned this pull request Aug 20, 2024
Open
bobbinth
bobbinth reviewed Aug 20, 2024
View reviewed changes
crates/store/src/state.rs Outdated Show resolved Hide resolved
@Mirko-von-Leipzig Mirko-von-Leipzig marked this pull request as draft August 21, 2024 14:50
Mirko-von-Leipzig
Mirko-von-Leipzig reviewed Aug 22, 2024
View reviewed changes
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

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

I've rebased and I think did the block producer portion as well.

proto/store.proto Outdated Show resolved Hide resolved
proto/responses.proto Outdated
Comment on lines 139 to 141
message GetNoteInclusionProofsResponse {
note.NoteInclusionProofs proofs = 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming feels weird. Elsewhere in the comments there was a NoteAuthenticationInfo suggestion, which I have used in parts of the code. I've gotten a bit lost wrt to the naming - should NoteInclusionProofs become NoteAuthenticationInfo as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - I think I suggested it somewhere. I think NoteAuthenticationInfo may be a bit better (one reason is that we use it in the context of authenticating "unauthenticated notes") - but also not a strong preference by any means.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've made this change -- should the proto method name change as well to GetNoteAuthenticationInfo?

I'm still a bit murky on the nomenclature

Copy link
Contributor

Choose a reason for hiding this comment

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

I could go either way - so, basically, up to you :)

Copy link
Contributor

Choose a reason for hiding this comment

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

changed it so that it isn't the only outlier :)

proto/block.proto Outdated Show resolved Hide resolved
crates/block-producer/src/batch_builder/tests/mod.rs Show resolved Hide resolved
crates/block-producer/src/batch_builder/batch.rs Show resolved Hide resolved
crates/block-producer/src/test_utils/block.rs Show resolved Hide resolved
@Mirko-von-Leipzig Mirko-von-Leipzig marked this pull request as ready for review August 22, 2024 13:56
bobbinth
bobbinth approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a few small comments inline. Once these are addressed, we can merge.

I also left one comment for a subsequent PR. I think we should do it right after this PR (it shouldn't be too complex).

crates/proto/src/domain/blocks.rs Show resolved Hide resolved
crates/store/src/state.rs Outdated Show resolved Hide resolved
crates/block-producer/src/block_builder/mod.rs Outdated Show resolved Hide resolved
crates/block-producer/src/batch_builder/tests/mod.rs Show resolved Hide resolved
crates/block-producer/src/batch_builder/batch.rs Show resolved Hide resolved
This was referenced Aug 23, 2024
Open
Closed
@Mirko-von-Leipzig
Copy link
Contributor

Mirko-von-Leipzig commented Aug 23, 2024 •
edited
Loading

Changes made - one final naming question here @bobbinth and then we're good to go I think.

@Mirko-von-Leipzig Mirko-von-Leipzig changed the title feat(store): GetNoteInclusionProofs endpoint feat(store): GetNoteAuthenticationInfo endpoint Aug 23, 2024
@Mirko-von-Leipzig Mirko-von-Leipzig merged commit 4633fc9 into next Aug 23, 2024
8 checks passed
@Mirko-von-Leipzig Mirko-von-Leipzig deleted the polydez-note-inclusion-proofs branch August 23, 2024 16:03
@bobbinth bobbinth mentioned this pull request Aug 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobbinth bobbinth bobbinth approved these changes

@Mirko-von-Leipzig Mirko-von-Leipzig Mirko-von-Leipzig approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature]: Add specialized endpoint for getting note inclusion paths
3 participants
@polydez @Mirko-von-Leipzig @bobbinth