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

test: add darkside support for GetSubtreeRoots gRPC #469

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

LarryRuane
Copy link
Collaborator

Closes #464
One new darksidewallet gRPC added, SetSubtreeRoots()

Example usage and output, after starting lightwalletd in darksidewallet mode:

$ grpcurl -plaintext -d '{"saplingActivation": 1,"branchID": "abc", "chainName":"x"}' \
    localhost:9067 cash.z.wallet.sdk.rpc.DarksideStreamer/Reset

$ grpcurl -plaintext -d '{"shieldedProtocol": 0,"startIndex": 2, "subtreeRoots":[
    {"rootHash": "qg==", "completingBlockHash": "uw==", "completingBlockHeight": 99},
    {"rootHash": "zA==", "completingBlockHash": "3Q==", "completingBlockHeight": 999}
]}' localhost:9067 cash.z.wallet.sdk.rpc.DarksideStreamer/SetSubtreeRoots

$ grpcurl -plaintext -d '{"startIndex": 2, "shieldedProtocol": 0, "maxEntries": 2}' \
    localhost:9067 cash.z.wallet.sdk.rpc.CompactTxStreamer/GetSubtreeRoots
{
  "rootHash": "qg==",
  "completingBlockHash": "uw==",
  "completingBlockHeight": "99"
}
{
  "rootHash": "zA==",
  "completingBlockHash": "3Q==",
  "completingBlockHeight": "999"
}
$

Each call to SetSubtreeRoots completely replaces all of the roots. A call to Reset clears all the roots. It's not necessary to create or stage blocks or transactions; this is its own independent "cache".

Design discussion:

The original plan was to allow subtree roots to be added and deleted individually, but that was more complex to implement and was semantically confusing. For example, what if you had created subtree roots at index 2, then 3, then 4. Now you can request starting index 2 and length 3, and get all 3 of these back. But what if you now remove index 3 and make the same request (or never created an entry at index 3 in the first place)? What should it return for the middle entry (index 3)? Should that be an error? Such an error would never occur in production (it would only be a test error).

It really only makes sense to have a contiguous range of entries. I can't imagine a test wanting to surgically update specific entries.

In return for this lack of flexibility, the code and the mental model are much simpler to understand. It's very important for test code to be as trivial as possible to understand, or else you start to doubt if the production code is actually being tested.

Copy link
Contributor

@pacu pacu 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 to me!

@pacu
Copy link
Contributor

pacu commented Jan 11, 2024

ACK!

Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

Question about the change to the "errors" dependency.

frontend/service.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

utACK modulo the answer to the "errors" question.

One new darksidewallet gRPC added, SetSubtreeRoots()
@LarryRuane LarryRuane force-pushed the 2023-12-darkside-GetSubtreeRoots branch from 64231e7 to bfd4e0a Compare January 11, 2024 19:49
@LarryRuane
Copy link
Collaborator Author

Force pushed to resolve review comment from @nuttycom (thanks). There are still some references to github.com/pkg/errors but I'll fix them in a separate PR.

Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

utACK bfd4e0a

@nuttycom
Copy link
Contributor

@LarryRuane do you know why the CI run here doesn't seem to be responding? I don't see an error, but I also don't see the option to admin-merge functionality.

@LarryRuane
Copy link
Collaborator Author

@LarryRuane do you know why the CI run here doesn't seem to be responding? I don't see an error, but I also don't see the option to admin-merge functionality.

No, sorry, I don't know how CI works at all. As I mentioned, Marshall tried to get CI to work on this repo but wasn't able to. I don't know what problem he was running into.

@nuttycom nuttycom merged commit e91d882 into zcash:master Jan 11, 2024
@LarryRuane LarryRuane mentioned this pull request Jan 24, 2024
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.

Implement darkside counterpart for z_getsubtreesbyindex method
3 participants