Review PR for changes to ZcashLightClientKit for fast spendability. #1257

nuttycom · 2023-09-11T19:31:46Z

This PR is not intended to be merged; instead, it's intended to allow for a unified UI for review of the feature/DAG-sync changes that includes changes already merged to main.

Migrate to Rust backend with fast spendability support

- SubtreeRoot placeholder removed, we can rely on the generated one from the gprc

…eeRoot [#1162] Remove duplicated SubtreeRoot

The previous FFI repo revision no longer exists; commits between 87faf91 and here will not build.

epk -> ephemeralKey refactor done so offline tests in the SDK pass, therefore the CI passes as well

Update Rust dependencies with bugfixes

- mainnet & testnet checkpoints

[#1182] Update checkpoints

- firstUnenhancedHeight is now properly used - fixed tests

…ot-called [#1184] FirstUnenhancedHeight not called

The previous FFI repo revisions no longer exist; commits between 87faf91 and here will not build.

Update Rust dependencies with checkpoint and memo bugfixes

Bumps [github.com/apple/swift-nio-extras](https://github.com/apple/swift-nio-extras) from 1.12.1 to 1.19.0. - [Release notes](https://github.com/apple/swift-nio-extras/releases) - [Commits](apple/swift-nio-extras@1.12.1...1.19.0) --- updated-dependencies: - dependency-name: github.com/apple/swift-nio-extras dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>

…/swift-nio-extras-1.19.0 Bump github.com/apple/swift-nio-extras from 1.12.1 to 1.19.0

- the priorities for log levels have been updated [#1196] Check logging level priorities - changelog updated

…orities [#1196] Check logging level priorities

The previous FFI repo revisions no longer exist; commits between 87faf91 and here will not build.

Update Rust dependencies with scanning and spending bugfixes

- hotfix for explicit use of self in a Task, CI fails to build it without it

[#1208] Bitrise fails to build

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

- endpoint updated to the latest and workable server

[#1213] Server update

Update the `zcash-light-client-ffi` dependency version.

The previous FFI repo revisions no longer exist; commits between 87faf91 and here will not build. Update Rust dependencies with account birthdays and scan progress - fixes for SampleApp

Update Rust dependencies with account birthdays and scan progress

draft [#1165] Step 1 - Download note commitment tree data from lightwalletd - code cleanup after draft [#1165] Step 1 - Download note commitment tree data from lightwalletd - UpdateSubtreeRootsAction added, ensuring the roots are downloaded and stored in the DB [#1165] Step 1 - Download note commitment tree data from lightwalletd - added ZcashError for putSaplingSubtreeRoots failure - cleaned up action [#1165] Step 1 - Download note commitment tree data from lightwalletd - demo app config temporarily updated to Nighthawk server [#1165] Step 1 - Download note commitment tree data from lightwalletd - file header updated [#1165] Step 1 - Download note commitment tree data from lightwalletd (#1174) - demo app config cleaned up [#1165] Step 1 - Download note commitment tree data from lightwalletd (#1174) - offline tests fixed

nuttycom

I opened this review-only PR, so I cannot approve or request changes. None of my comments are blocking, but it would be helpful to have the questions addressed and where changes are required, please create issues.

Package.resolved

Example/ZcashLightClientSample/ZcashLightClientSample/ViewController.swift

Sources/ZcashLightClientKit/Block/Actions/UpdateSubtreeRootsAction.swift

Sources/ZcashLightClientKit/Block/CompactBlockProcessor.swift

Sources/ZcashLightClientKit/Initializer.swift

Sources/ZcashLightClientKit/Metrics/SDKMetrics.swift

Sources/ZcashLightClientKit/Repository/TransactionRepository.swift

Sources/ZcashLightClientKit/Rust/ZcashRustBackendWelding.swift

Tests/DarksideTests/AdvancedReOrgTests.swift

str4d

Reviewed as of 2ac984a (excluding some files under Tests/).

Sources/ZcashLightClientKit/Block/Actions/DownloadAction.swift

Sources/ZcashLightClientKit/Model/ScanRange.swift

Sources/ZcashLightClientKit/Resources/checkpoints/mainnet/2092500.json

Tests/TestUtils/proto/darkside.grpc.swift

Tests/TestUtils/Sourcery/GeneratedMocks/AutoMockable.generated.swift

Sources/ZcashLightClientKit/Block/Actions/ProcessSuggestedScanRangesAction.swift

Sources/ZcashLightClientKit/Block/CompactBlockProcessor.swift

Tests/DarksideTests/PendingTransactionUpdatesTest.swift

pacu

can't reject. :/

Reviewed the files and left questions and change request.

Sources/ZcashLightClientKit/Block/Actions/Action.swift

Sources/ZcashLightClientKit/Initializer.swift

Sources/ZcashLightClientKit/Model/ScanProgress.swift

Sources/ZcashLightClientKit/Repository/TransactionRepository.swift

Sources/ZcashLightClientKit/Rust/ZcashRustBackend.swift

Sources/ZcashLightClientKit/Synchronizer.swift

Tests/DarksideTests/AdvancedReOrgTests.swift

Tests/DarksideTests/DarksideSanityCheckTests.swift

- all comments from the review (round 1) resolved

- when isContinuityError occurred, the next action should be rewind, fixed

- adding a context so it's clear why division by 0 is not a concern - pending transactions removed from the DemoApp's UI, #1260 is a followup

- the logic deciding if verify range is present and going to the rewind is no longer valid, so removed - tests updated

Sources/ZcashLightClientKit/Model/ScanRange.swift

- cbp_state_machine.png as well as .puml files updated to reflect the State Machine changes after SBS - one small cleanup of clearCache, no longer needed to be called twice, only after enhance (missed removal of linear sync)

Sources/ZcashLightClientKit/Block/Actions/RewindAction.swift

Sources/ZcashLightClientKit/Block/Actions/UpdateChainTipAction.swift

- the diagram png + puml updated - rewind action continues to the process scan ranges

Sources/ZcashLightClientKit/Block/Actions/ProcessSuggestedScanRangesAction.swift

- more comments resolved - totalProgressRange removed from the SDK - ScanRange now takes into account the given value and properly initializes, + added tests - tests fixed

Sources/ZcashLightClientKit/Block/Actions/Action.swift

- totalProgressRange was removed from the protocol but not from the actual implementation

Sources/ZcashLightClientKit/Model/ScanRange.swift

- only the matching priorities are allowed, otherwise fatalError

str4d

utACK 26fe1f7

Sources/ZcashLightClientKit/Model/ScanRange.swift

nuttycom · 2023-09-13T16:59:46Z

utACK 26fe1f7

pacu

changes are non blocking for an RC, but I would pay specific attention to the Division By Zero and the allocations on the RustBackend not leaking

pacu · 2023-09-13T18:41:01Z

Sources/ZcashLightClientKit/Block/Actions/DownloadAction.swift

If I'm not missing any other, this is the only action that actually implements the stop() method.

It's seems that stop doesn't belong to the protocol itself but to the implementation of the DownloadAction

pacu · 2023-09-13T18:55:43Z

Sources/ZcashLightClientKit/Initializer.swift

@@ -409,35 +409,30 @@ public class Initializer {
    /// - Parameter seed: ZIP-32 Seed bytes for the wallet that will be initialized
    /// - Throws: `InitializerError.dataDbInitFailed` if the creation of the dataDb fails
    /// `InitializerError.accountInitFailed` if the account table can't be initialized. 
-    func initialize(with seed: [UInt8]?, viewingKeys: [UnifiedFullViewingKey], walletBirthday: BlockHeight) async throws -> InitializationResult {


Documentation of the function is outdated and does not match the parameters required. Consider updating them

Sources/ZcashLightClientKit/Initializer.swift

Sources/ZcashLightClientKit/Model/ScanProgress.swift

nuttycom · 2023-09-25T22:55:37Z

Closing as complete with the 2.0.0 release.

str4d and others added 30 commits July 19, 2023 18:13

Migrate to Rust backend with fast spendability support

6eb3956

Merge pull request #1160 from zcash/fast-spendability

87faf91

Migrate to Rust backend with fast spendability support

[#1162] Remove duplicated SubtreeRoot

5e8985d

- SubtreeRoot placeholder removed, we can rely on the generated one from the gprc

Merge pull request #1163 from LukasKorba/1162-Remove-duplicated-Subtr…

27b7e78

…eeRoot [#1162] Remove duplicated SubtreeRoot

Update Rust dependencies with bugfixes

ea3281e

The previous FFI repo revision no longer exists; commits between 87faf91 and here will not build.

epk key refactor

d6507b7

epk -> ephemeralKey refactor done so offline tests in the SDK pass, therefore the CI passes as well

Merge pull request #1173 from zcash/update-rust-deps

e4cea0c

Update Rust dependencies with bugfixes

[#1182] Update checkpoints

a6d0f97

- mainnet & testnet checkpoints

Merge pull request #1183 from LukasKorba/1182-Update-checkpoints

b9524ae

[#1182] Update checkpoints

[#1184] FirstUnenhancedHeight not called

ad99fde

- firstUnenhancedHeight is now properly used - fixed tests

Merge pull request #1185 from LukasKorba/1184-FirstUnenhancedHeight-n…

4d95f2c

…ot-called [#1184] FirstUnenhancedHeight not called

Update Rust dependencies with checkpoint and memo bugfixes

cddf689

The previous FFI repo revisions no longer exist; commits between 87faf91 and here will not build.

Merge pull request #1187 from zcash/rust-checkpoint-and-memo-bugfixes

84620ed

Update Rust dependencies with checkpoint and memo bugfixes

Merge pull request #1194 from zcash/dependabot/swift/github.com/apple…

b92177b

…/swift-nio-extras-1.19.0 Bump github.com/apple/swift-nio-extras from 1.12.1 to 1.19.0

[#1196] Check logging level priorities

5fa1af3

- the priorities for log levels have been updated [#1196] Check logging level priorities - changelog updated

Merge pull request #1198 from LukasKorba/1196-Check-logging-level-pri…

a9f9273

…orities [#1196] Check logging level priorities

Update Rust dependencies with scanning and spending bugfixes

bc05287

The previous FFI repo revisions no longer exist; commits between 87faf91 and here will not build.

Merge pull request #1201 from zcash/rust-scan-spend-fixes

d446c6d

Update Rust dependencies with scanning and spending bugfixes

Migrate to latest darkside.proto to fix Darkside tests

4c74e4d

[#1208] Bitrise fails to build

5ac2121

- hotfix for explicit use of self in a Task, CI fails to build it without it

Merge pull request #1209 from LukasKorba/1208-Bitrise-fails-to-build

9a82acb

[#1208] Bitrise fails to build

Merge pull request #1191 from zcash/darkside-bugfix-tree-sizes

42214bd

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

[#1213] Server update

6f8f7eb

- endpoint updated to the latest and workable server

Merge pull request #1214 from LukasKorba/1213-Server-update

c2b904e

[#1213] Server update

Update the zcash-light-client-ffi dependency version.

8f2962f

Merge pull request #1210 from nuttycom/ffi-update

e389af4

Update the `zcash-light-client-ffi` dependency version.

Update Rust dependencies with account birthdays and scan progress

e762120

The previous FFI repo revisions no longer exist; commits between 87faf91 and here will not build. Update Rust dependencies with account birthdays and scan progress - fixes for SampleApp

Merge pull request #1229 from zcash/rust-account-birthdays

2302dc4

Update Rust dependencies with account birthdays and scan progress

nuttycom commented Sep 12, 2023

View reviewed changes

str4d reviewed Sep 12, 2023

View reviewed changes

pacu reviewed Sep 12, 2023

View reviewed changes

pacu mentioned this pull request Sep 12, 2023

Review Spend Before Sync iOS pacu/zwcd#39

Closed

LukasKorba requested review from pacu and str4d September 13, 2023 07:21

LukasKorba added 4 commits September 13, 2023 09:24

Comments addressed

4b45c15

- all comments from the review (round 1) resolved

Rewind when continuity error

d5d4664

- when isContinuityError occurred, the next action should be rewind, fixed

Cleanup and comment providing context

af595e6

- adding a context so it's clear why division by 0 is not a concern - pending transactions removed from the DemoApp's UI, #1260 is a followup

ProcessSuggestedScanRanges always continues to the download

3d2812c

- the logic deciding if verify range is present and going to the rewind is no longer valid, so removed - tests updated

str4d reviewed Sep 13, 2023

View reviewed changes

Sources/ZcashLightClientKit/Model/ScanRange.swift Outdated Show resolved Hide resolved

cbp_state_machine updated

95c9746

- cbp_state_machine.png as well as .puml files updated to reflect the State Machine changes after SBS - one small cleanup of clearCache, no longer needed to be called twice, only after enhance (missed removal of linear sync)

str4d reviewed Sep 13, 2023

View reviewed changes

Sources/ZcashLightClientKit/Block/Actions/RewindAction.swift Outdated Show resolved Hide resolved

str4d reviewed Sep 13, 2023

View reviewed changes

Sources/ZcashLightClientKit/Block/Actions/UpdateChainTipAction.swift Show resolved Hide resolved

Rewind action continues to the processScanRanges

5600e77

- the diagram png + puml updated - rewind action continues to the process scan ranges

nuttycom commented Sep 13, 2023

View reviewed changes

Sources/ZcashLightClientKit/Block/Actions/ProcessSuggestedScanRangesAction.swift Outdated Show resolved Hide resolved

nuttycom mentioned this pull request Sep 13, 2023

Remove unused totalProgressRange from ActionContext #1265

Closed

Addressed comments batch 2

4e2af5d

- more comments resolved - totalProgressRange removed from the SDK - ScanRange now takes into account the given value and properly initializes, + added tests - tests fixed

LukasKorba requested a review from str4d September 13, 2023 15:35

nuttycom commented Sep 13, 2023

View reviewed changes

Sources/ZcashLightClientKit/Block/Actions/Action.swift Outdated Show resolved Hide resolved

Instance totalProgressRange removed

50afc8f

- totalProgressRange was removed from the protocol but not from the actual implementation

nuttycom commented Sep 13, 2023

View reviewed changes

Sources/ZcashLightClientKit/Model/ScanRange.swift Outdated Show resolved Hide resolved

ScanRange initialization changed

26fe1f7

- only the matching priorities are allowed, otherwise fatalError

str4d approved these changes Sep 13, 2023

View reviewed changes

Sources/ZcashLightClientKit/Model/ScanRange.swift Show resolved Hide resolved

pacu reviewed Sep 13, 2023

View reviewed changes

pacu mentioned this pull request Sep 13, 2023

Avoid Division By Zero in ScanProgress type #1267

Closed

nuttycom closed this Sep 25, 2023

nuttycom deleted the feature/DAG-sync branch September 25, 2023 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review PR for changes to ZcashLightClientKit for fast spendability. #1257

Review PR for changes to ZcashLightClientKit for fast spendability. #1257

nuttycom commented Sep 11, 2023

nuttycom left a comment

str4d left a comment

pacu left a comment

str4d left a comment

nuttycom commented Sep 13, 2023

pacu left a comment

pacu Sep 13, 2023

pacu Sep 13, 2023

nuttycom commented Sep 25, 2023

Review PR for changes to ZcashLightClientKit for fast spendability. #1257

Review PR for changes to ZcashLightClientKit for fast spendability. #1257

Conversation

nuttycom commented Sep 11, 2023

nuttycom left a comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

pacu left a comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

nuttycom commented Sep 13, 2023

pacu left a comment

Choose a reason for hiding this comment

pacu Sep 13, 2023

Choose a reason for hiding this comment

pacu Sep 13, 2023

Choose a reason for hiding this comment

nuttycom commented Sep 25, 2023