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

Add format checks with fixed subtree values from zcashd #7446

Closed
Tracked by #6642 ...
teor2345 opened this issue Sep 1, 2023 · 10 comments · Fixed by #7515
Closed
Tracked by #6642 ...

Add format checks with fixed subtree values from zcashd #7446

teor2345 opened this issue Sep 1, 2023 · 10 comments · Fixed by #7515
Assignees
Labels
A-compatibility Area: Compatibility with other nodes or wallets, or standard rules A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes C-testing Category: These are tests I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data

Comments

@teor2345
Copy link
Contributor

teor2345 commented Sep 1, 2023

Motivation

We are calculating subtree roots, but we don't know if they match zcashd.

Specifications

The RPC format isn't on the RPC website yet, but it is in the zcashd PR at https://github.com/zcash/zcash/pull/6677/files#diff-decae4be02fb8a47ab4557fe74a9cb853bdfa3ec0fa1b515c0a1e5de91f4ad0bR1458-R1475

Suggestion

Add a list of subtree indexes and subtree roots for testing, and check that they match here:
https://github.com/ZcashFoundation/zebra/pull/7437/files#diff-55a5fe0b23f2b84d8ef7fbd30e96d1abade1bfe4980ddf5a61abd8065da18012R209-R214

Testing Matrix

We need to test the 8 combinations of these different code paths:

  • Sapling & Orchard
  • Completed Subtree at the end of the block & Incomplete subtree at the end of the block
  • Subtrees upgraded from existing blocks & subtrees added with new tip blocks (one way to do this is to test indexes near zero, and indexes near the tip)
@mpguerra mpguerra added this to Zebra Sep 1, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New in Zebra Sep 1, 2023
@teor2345 teor2345 added P-Medium ⚡ I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data C-testing Category: These are tests A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes A-compatibility Area: Compatibility with other nodes or wallets, or standard rules labels Sep 1, 2023
@mpguerra
Copy link
Contributor

mpguerra commented Sep 6, 2023

Hey team! Please add your planning poker estimate with Zenhub @arya2 @oxarbitrage @teor2345 @upbqdn

@oxarbitrage
Copy link
Contributor

Do we know what argument combination we should use to generate this test vectors:

  • Completed Subtree at the end of the block & Incomplete subtree at the end of the block
  • Subtrees upgraded from existing blocks & subtrees added with new tip blocks (one way to do this is to test indexes near zero, and indexes near the tip)

@teor2345
Copy link
Contributor Author

Do we know what argument combination we should use to generate this test vectors:

I don't know, sorry. Finding them is part of the work in this ticket.

The upgrade code changes in PR #7531 will make it a lot easier to search the entire database for specific test vectors. I can help with that after I get that PR working. But it might not be until later this week, because I have to do ZIP editor work today.

@oxarbitrage
Copy link
Contributor

That is ok, i can do the searching. What is not clear to me is what i am looking for.

@teor2345

This comment was marked as outdated.

@teor2345
Copy link
Contributor Author

I understand that's a lot of test vectors, so I was trying to cut it down to 4 or 6 different code paths in PR #7531. But I haven't managed to make it work yet.

@teor2345
Copy link
Contributor Author

You could log the subtree pool and index when Zebra runs the 8 different kinds of subtree code

There's a bug in one of these code paths, or in the subtree validation:
#7349 (comment)

As part of the fix I am trying to reduce the number of code paths. Any test vectors you generate will still be useful, but the instructions above will become outdated, and I'll need to make new ones.

@teor2345
Copy link
Contributor Author

  • Completed Subtree at the end of the block

There is a full list of these subtrees here:
#7532 (comment)

And their test vectors:
#7532 (comment)

All the other existing subtrees are in the middle of the block. (The existing subtrees have indexes less than 1092 for Sapling, or 584 for Orchard.)

Zebra logged all the subtrees at the end of a block because the sync code generates incorrect heights for them. So I will have to replace that code to fix bug #7532. We didn't catch this bug in the tests because the upgrade code works fine.

@oxarbitrage
Copy link
Contributor

I was checking the code paths that are mentioned in the outdated comment. I know this is going to change so i am not sure if this is still relevant:

I am not able to see how to trigger the code paths 1, 2, 3 and 4 that are located in the state. It seems this code is only executed when we are updating the tree state which i don't think we will be able to test in the context of this ticket.

Maybe i don't have enough context but the upgrade is done when zebra starts, we could block the rpc sever entirely during that stage OR send an error message from the rpc method.

@teor2345
Copy link
Contributor Author

I know this is going to change so i am not sure if this is still relevant:

Most of the code paths are combined as part of fixing bug #7532, but the block selection code has to be different, and so does a small part of the "end of block" check. So I think that means we still need to test all those code paths.

I am not able to see how to trigger the code paths 1, 2, 3 and 4 that are located in the state. It seems this code is only executed when we are updating the tree state which i don't think we will be able to test in the context of this ticket.

The goal of this ticket is to add subtree test vectors that were created by:

  • sapling middle of block
  • sapling end of block
  • orchard middle of block
  • orchard end of block

Ideally we want to run the test on both "sync from empty" states, and "upgraded from existing" states. But that's not currently something a test can control in our CI. So maybe it is out of scope for this ticket?

I opened #7570, let's see what everyone thinks tomorrow.

Maybe i don't have enough context but the upgrade is done when zebra starts, we could block the rpc sever entirely during that stage OR send an error message from the rpc method.

That sounds like compatibility with lightwalletd, which is handled by ticket #7407, so it's out of scope for this ticket.

@mergify mergify bot closed this as completed in #7515 Sep 20, 2023
@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in Zebra Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compatibility Area: Compatibility with other nodes or wallets, or standard rules A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes C-testing Category: These are tests I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data
Projects
Archived in project
3 participants