Migrate to latest `darkside.proto` to help fix Darkside tests #1191

str4d · 2023-08-08T18:14:11Z

This code review checklist is intended to serve as a starting point for the author and reviewer, although it may not be appropriate for all types of changes (e.g. fixing a spelling typo in documentation). For more in-depth discussion of how we think about code review, please see Code Review Guidelines.

Author

Self-review: Did you review your own code in GitHub's web interface? Code often looks different when reviewing the diff in a browser, making it easier to spot potential bugs.
Automated tests: Did you add appropriate automated tests for any code changes?
Code coverage: Did you check the code coverage report for the automated tests? While we are not looking for perfect coverage, the tool can point out potential cases that have been missed.
Documentation: Did you update Docs as appropiate? (E.g README.md, etc.)
Run the app: Did you run the app and try the changes?
Did you provide Screenshots of what the App looks like before and after your changes as part of the description of this PR? (only applicable to UI Changes)
Rebase and squash: Did you pull in the latest changes from the main branch and squash your commits before assigning a reviewer? Having your code up to date and squashed will make it easier for others to review. Use best judgement when squashing commits, as some changes (such as refactoring) might be easier to review as a separate commit.

Reviewer

Checklist review: Did you go through the code with the Code Review Guidelines checklist?
Ad hoc review: Did you perform an ad hoc review? In addition to a first pass using the code review guidelines, do a second pass using your best judgement and experience which may identify additional questions or comments. Research shows that code review is most effective when done in multiple passes, where reviewers look for different things through each pass.
Automated tests: Did you review the automated tests?
Manual tests: Did you review the manual tests?You will find manual testing guidelines under our manual testing section
How is Code Coverage affected by this PR? We encourage you to compare coverage befor and after your changes and when possible, leave it in a better place. Learn More...
Documentation: Did you review Docs, README.md, LICENSE.md, and Architecture.md as appropriate?
Run the app: Did you run the app and try the changes? While the CI server runs the app to look for build failures or crashes, humans running the app are more likely to notice unexpected log messages, UI inconsistencies, or bad output data.

str4d · 2023-08-08T18:24:21Z

Tests/DarksideTests/AdvancedReOrgTests.swift

@@ -33,7 +33,7 @@ class AdvancedReOrgTests: ZcashTestCase {
            walletBirthday: birthday + 50,
            network: network
        )
-        try await coordinator.reset(saplingActivation: 663150, branchID: self.branchID, chainName: self.chainName)
+        try await coordinator.reset(saplingActivation: 663150, startSaplingTreeSize: 128607, startOrchardTreeSize: 0, branchID: self.branchID, chainName: self.chainName)


A bunch of the tests reference block height 663150, which corresponds to a persisted mainnet block elsewhere in the codebase. So everywhere I see that reference, I applied the Sapling tree size corresponding to it. There's likely a nicer way to do this, but that's refactoring I don't know how to do in Swift.

This is coupled to the datasets for these tests here https://github.com/zcash-hackworks/darksidewalletd-test-data

These tests start from 663150 onwards. that's why they have that height. If you were to use another height then you need to reference a new dataset.

I think that this change makes sense.

Does Sbs require the wallet to get TreeStates to make node spendable? If so you should get those into dlwd as well

@str4d how do you obtain this tree size?

Obtaining the tree states is what enables fast spendability; I believe that @str4d has already made the required change to lightwalletd here: zcash/lightwalletd#448

That being said, the wallets should still function when connected to a LWD that doesn't support retrieval of tree states, but they'll be limited to linear scanning.

@pacu I fetched the tree size for that block height from my mainnet zcashd node. One of the RPCs exposes it.

str4d · 2023-08-18T13:38:48Z

Rebased on current main. I think this PR can be reviewed and merged (as the corresponding lightwalletd PR has been), and then we can continue fixing the Darkside tests in subsequent PRs.

LukasKorba · 2023-08-24T12:11:04Z

I pulled lightwalletd, used make to build the latest server and ran all dark side test with this result:

I don't know if this is expected or not, I remember str4d to talk about fixes of fundamental issues but still, after thiese fixes the tests need to be iterated to pass...

pacu

first pass of review.

Will attempt to run the test and check their state

pacu · 2023-08-25T14:53:01Z

Tests/DarksideTests/AdvancedReOrgTests.swift

@@ -33,7 +33,7 @@ class AdvancedReOrgTests: ZcashTestCase {
            walletBirthday: birthday + 50,
            network: network
        )
-        try await coordinator.reset(saplingActivation: 663150, branchID: self.branchID, chainName: self.chainName)
+        try await coordinator.reset(saplingActivation: 663150, startSaplingTreeSize: 128607, startOrchardTreeSize: 0, branchID: self.branchID, chainName: self.chainName)


This is coupled to the datasets for these tests here https://github.com/zcash-hackworks/darksidewalletd-test-data

These tests start from 663150 onwards. that's why they have that height. If you were to use another height then you need to reference a new dataset.

I think that this change makes sense.

Does Sbs require the wallet to get TreeStates to make node spendable? If so you should get those into dlwd as well

pacu · 2023-08-25T14:58:14Z

Tests/DarksideTests/AdvancedReOrgTests.swift

@@ -33,7 +33,7 @@ class AdvancedReOrgTests: ZcashTestCase {
            walletBirthday: birthday + 50,
            network: network
        )
-        try await coordinator.reset(saplingActivation: 663150, branchID: self.branchID, chainName: self.chainName)
+        try await coordinator.reset(saplingActivation: 663150, startSaplingTreeSize: 128607, startOrchardTreeSize: 0, branchID: self.branchID, chainName: self.chainName)


@str4d how do you obtain this tree size?

pacu

I ran the tests using SDK 4c74e4d2041d5432bb25696b6bbf658b321fbf55 and LWD current master branch commit 8269810eeefac56ce326b0f6878c1ea3821b8ad5.

The dLWD tests are able to start and reset the chain.

The Tests fail for other reasons that appear not to be related to darkside.proto failures.

Sync failed with error: rustScanBlocks("Error while scanning blocks: The underlying block store produced the following error: Requested height 663199 does not exist in the block cache")

If this is the sole purpose of the PR as it appears to be, then I consider it approved.

but I can't approve per the Repo's policy

LukasKorba · 2023-08-28T10:28:26Z

Approving as I also think the purpose of this PR is fulfilled.

str4d commented Aug 8, 2023

View reviewed changes

str4d force-pushed the darkside-bugfix-tree-sizes branch from b09e0a0 to 585082c Compare August 9, 2023 13:07

Migrate to latest darkside.proto to fix Darkside tests

4c74e4d

str4d force-pushed the darkside-bugfix-tree-sizes branch from 585082c to 4c74e4d Compare August 18, 2023 13:37

str4d changed the title ~~Migrate to latest darkside.proto to fix Darkside tests~~ Migrate to latest darkside.proto to help fix Darkside tests Aug 18, 2023

str4d marked this pull request as ready for review August 18, 2023 13:38

nuttycom requested a review from LukasKorba August 24, 2023 00:48

pacu mentioned this pull request Aug 25, 2023

Review Darksidewalletd updates for Sbs on iOS SDK pacu/zwcd#24

Closed

pacu reviewed Aug 25, 2023

View reviewed changes

LukasKorba approved these changes Aug 28, 2023

View reviewed changes

str4d merged commit 42214bd into main Aug 28, 2023
1 check passed

str4d deleted the darkside-bugfix-tree-sizes branch August 28, 2023 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to latest `darkside.proto` to help fix Darkside tests #1191

Migrate to latest `darkside.proto` to help fix Darkside tests #1191

str4d commented Aug 8, 2023 •

edited

str4d Aug 8, 2023

pacu Aug 25, 2023

pacu Aug 25, 2023

nuttycom Aug 25, 2023

nuttycom Aug 25, 2023

str4d Aug 25, 2023 •

edited

str4d commented Aug 18, 2023

LukasKorba commented Aug 24, 2023

pacu left a comment

pacu Aug 25, 2023

pacu Aug 25, 2023

pacu left a comment

LukasKorba commented Aug 28, 2023

Migrate to latest darkside.proto to help fix Darkside tests #1191

Migrate to latest darkside.proto to help fix Darkside tests #1191

Conversation

str4d commented Aug 8, 2023 • edited

Author

Reviewer

str4d Aug 8, 2023

Choose a reason for hiding this comment

pacu Aug 25, 2023

Choose a reason for hiding this comment

pacu Aug 25, 2023

Choose a reason for hiding this comment

nuttycom Aug 25, 2023

Choose a reason for hiding this comment

nuttycom Aug 25, 2023

Choose a reason for hiding this comment

str4d Aug 25, 2023 • edited

Choose a reason for hiding this comment

str4d commented Aug 18, 2023

LukasKorba commented Aug 24, 2023

pacu left a comment

Choose a reason for hiding this comment

pacu Aug 25, 2023

Choose a reason for hiding this comment

pacu Aug 25, 2023

Choose a reason for hiding this comment

pacu left a comment

Choose a reason for hiding this comment

LukasKorba commented Aug 28, 2023

Migrate to latest `darkside.proto` to help fix Darkside tests #1191

Migrate to latest `darkside.proto` to help fix Darkside tests #1191

str4d commented Aug 8, 2023 •

edited

str4d Aug 25, 2023 •

edited