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(tests): Add snapshot tests for sprout database formats #7057

Merged
merged 9 commits into from
Jun 27, 2023

Conversation

teor2345
Copy link
Collaborator

@teor2345 teor2345 commented Jun 23, 2023

Motivation

We're changing our database note commitment tree format code in PR #7053, but we don't have good coverage for sprout trees. (We have snapshots for the raw data, but not converting it into Zebra's data structures.)

Reference

RocksDB Iterator Mode:
https://docs.rs/rocksdb/latest/rocksdb/enum.IteratorMode.html

Snapshot redactions:
https://insta.rs/docs/redactions/#sorted-redactions

Complex Code or Requirements

Using database iterators like this has caused hangs before, but so far these methods are only used in tests.

Solution

Empty Note Commitment Tree snapshot tests:

Sprout Note Commitment Tree manual snapshot tests:

Tree Root tests:

Related Fixes:

Review

@oxarbitrage might find this handy for testing PR #7053.

Reviewer Checklist

  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

We still don't have some snapshot tests for the entire database, or for some shielded data. (We only use 10 blocks, but the first Sprout transaction is after 100+ blocks.)

We can add them as we change the database format.

@teor2345 teor2345 added P-Medium ⚡ C-testing Category: These are tests A-state Area: State / database changes C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG A-concurrency Area: Async code, needs extra work to make it work properly. labels Jun 23, 2023
@teor2345 teor2345 self-assigned this Jun 23, 2023
@teor2345 teor2345 requested a review from a team as a code owner June 23, 2023 06:56
@teor2345 teor2345 requested review from dconnolly and removed request for a team June 23, 2023 06:56
@github-actions github-actions bot added the C-feature Category: New features label Jun 23, 2023
@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Merging #7057 (ea8f5fd) into main (a6f35af) will decrease coverage by 0.07%.
The diff coverage is 82.19%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7057      +/-   ##
==========================================
- Coverage   77.52%   77.46%   -0.07%     
==========================================
  Files         310      310              
  Lines       41694    41767      +73     
==========================================
+ Hits        32325    32353      +28     
- Misses       9369     9414      +45     

@oxarbitrage
Copy link
Contributor

Failed by container issue. Restarting all jobs

@teor2345
Copy link
Collaborator Author

Failed by container issue. Restarting all jobs

It first failed at the create instance stage:

ERROR: (gcloud.compute.instances.create-with-container) Could not fetch resource:

<title>Error 502 (Server Error)!!1</title>

502. That’s an error.

The server encountered a temporary error and could not complete your request.

Please try again in 30 seconds. That’s all we know.

https://github.com/ZcashFoundation/zebra/actions/runs/5353701092/jobs/9711197294?pr=7057#step:11:91

@teor2345 teor2345 requested a review from a team as a code owner June 26, 2023 00:34
@teor2345 teor2345 requested review from upbqdn and removed request for a team and upbqdn June 26, 2023 00:34
@teor2345
Copy link
Collaborator Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jun 27, 2023

update

✅ Branch has been successfully updated

Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

This looks and works very good, it will be needed to test the ECC dependency upgrades of #7053

mergify bot added a commit that referenced this pull request Jun 27, 2023
@mergify mergify bot merged commit 5324e5a into main Jun 27, 2023
@mergify mergify bot deleted the sprout-zebra-db-snapshot branch June 27, 2023 15:32
@teor2345 teor2345 mentioned this pull request Jun 28, 2023
44 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Async code, needs extra work to make it work properly. A-state Area: State / database changes C-feature Category: New features C-testing Category: These are tests C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants