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

[WIP] - Creates cached state for SubTreeRoots in darksidewalletd #468

Closed
wants to merge 6 commits into from

Conversation

pacu
Copy link
Contributor

@pacu pacu commented Dec 28, 2023

(When completed) Closes #464

I'm following the same reasoning under GetTreeState, let darksidewalletd provide RPC methods
for clients to stage SubTreeRoots when needing to create a spend before sync test scenario
provides:

  • add single root for any shielded protocol
  • add many by a streaming call
  • clear All
  • get a subtree root for a given shielded protocol and height

Please ensure this checklist is followed for any pull requests for this repo. This checklist must be checked by both the PR creator and by anyone who reviews the PR.

  • Relevant documentation for this PR has to be completed before the PR can be merged
  • A test plan for the PR must be documented in the PR notes and included in the test plan for the next regular release

As a note, all CI tests need to be passing and all appropriate code reviews need to be done before this PR can be merged

@pacu pacu marked this pull request as draft December 28, 2023 22:58
@LarryRuane
Copy link
Collaborator

I had been implementing a solution to #464, apologies for not mentioning this before you took up this work. My PR is #469, please review that.

A couple of comments on this PR, the description above says that the subtree roots are returned for a given block height, but they're returned for a given index. Secondly, it doesn't look like you're returning the block hash with each root (probably because z_getsubtreesbyindex doesn't return it); this is necessary. For example, on mainnet (production):

$ grpcurl -plaintext -d '{"startIndex": 0, "shieldedProtocol": 0, "maxEntries": 2}' localhost:9067 cash.z.wallet.sdk.rpc.CompactTxStreamer/GetSubtreeRoots
{
  "rootHash": "dUu1k+pC0jGn3fNnZA8Ju/WdwA8sHSADzDQODAFrWxM=",
  "completingBlockHash": "AAAAAACPmsBjbYHceVyw9Ulab9B06BWhnhmfDaSRUf4=",
  "completingBlockHeight": "558822"
}
{
  "rootHash": "A2VMPqy7m5PhIs9td7YG6uKWEPTzikd5hTaBl/1o4C0=",
  "completingBlockHash": "AAAAAAIFzQaAI1LPzYkhgVvQqdc+ta/D7AnzXHGARqk=",
  "completingBlockHeight": "670209"
}

@pacu
Copy link
Contributor Author

pacu commented Jan 3, 2024

I had been implementing a solution to #464, apologies for not mentioning this before you took up this work. My PR is #469, please review that.

A couple of comments on this PR, the description above says that the subtree roots are returned for a given block height, but they're returned for a given index. Secondly, it doesn't look like you're returning the block hash with each root (probably because z_getsubtreesbyindex doesn't return it); this is necessary. For example, on mainnet (production):

$ grpcurl -plaintext -d '{"startIndex": 0, "shieldedProtocol": 0, "maxEntries": 2}' localhost:9067 cash.z.wallet.sdk.rpc.CompactTxStreamer/GetSubtreeRoots
{
  "rootHash": "dUu1k+pC0jGn3fNnZA8Ju/WdwA8sHSADzDQODAFrWxM=",
  "completingBlockHash": "AAAAAACPmsBjbYHceVyw9Ulab9B06BWhnhmfDaSRUf4=",
  "completingBlockHeight": "558822"
}
{
  "rootHash": "A2VMPqy7m5PhIs9td7YG6uKWEPTzikd5hTaBl/1o4C0=",
  "completingBlockHash": "AAAAAAIFzQaAI1LPzYkhgVvQqdc+ta/D7AnzXHGARqk=",
  "completingBlockHeight": "670209"
}

Oh! thanks for catching that Larry!

@pacu
Copy link
Contributor Author

pacu commented Jan 4, 2024

Secondly, it doesn't look like you're returning the block hash with each root (probably because z_getsubtreesbyindex doesn't return it); this is necessary. For example, on mainnet (production):

@LarryRuane I mimicked the models in the GRPC proto files. So the blockhash should be returned

type DarksideSubtree struct {
	RootHash              []byte
	CompletingBlockHash   []byte
	CompletingBlockHeight uint64
}

This will mean that the provided tree states will have to contain the block hash. Should that be ok?

@pacu pacu marked this pull request as ready for review January 4, 2024 17:04
@LarryRuane
Copy link
Collaborator

Well, in production mode, the block hash is returned and was considered an important part of the interface. So I would think that in darkside mode, it should be provided so that we're fully simulating production mode.

I was hoping you would agree to close this PR and just go with #469. (But the interface is slightly different, so maybe it's not what you want or need.)

@pacu
Copy link
Contributor Author

pacu commented Jan 4, 2024

I was hoping you would agree to close this PR and just go with #469. (But the interface is slightly different, so maybe it's not what you want or need.)

Idk how I missed that part of the post. Apologize not reviewing yours 🙏

I think this works out. I'm currently updating the Advanced Re Org Tests Regtest datasets to have this new info.

This is closed in favor of #469

@pacu pacu closed this Jan 4, 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
2 participants