Skip to content

Refactor SyncState for zcash tests#33971

Open
supermassive wants to merge 4 commits intomasterfrom
refactor_zcash_tests
Open

Refactor SyncState for zcash tests#33971
supermassive wants to merge 4 commits intomasterfrom
refactor_zcash_tests

Conversation

@supermassive
Copy link
Collaborator

@supermassive supermassive commented Feb 16, 2026 •
edited
Loading

Resolves brave/brave-browser#53026

  • Remove duplicated instance of rpc, sync_state, etc in tests.
  • Properly wait for sync_state to finish in tests.

@supermassive supermassive requested a review from a team as a code owner February 16, 2026 16:44
@github-actions
Copy link
Contributor

github-actions bot commented Feb 16, 2026 •
edited
Loading

📋 Code Owners Summary

28 file(s) changed, 28 with assigned owners

1 team(s) affected: @brave/crypto-wallets-core

Owners and Their Files

@brave/crypto-wallets-core — 28 file(s)

... and 23 more files

bbondy
bbondy reviewed Feb 17, 2026
View reviewed changes
Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

Review via brave-core-bot: nit: inline is redundant inside an anonymous namespace since the namespace already provides internal linkage. Use constexpr char kOrchardDatabaseName[] = "orchard.db"; without the inline specifier.

(re: components/brave_wallet/browser/internal/orchard_sync_state.cc)

bbondy
bbondy reviewed Feb 17, 2026
View reviewed changes
Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

Review via brave-core-bot: Per Chromium guidelines, explicitly specify base::TaskPriority and shutdown behavior when creating thread pool task runners rather than relying on defaults. See OrchardSyncState::CreateSyncStateSequence() in components/brave_wallet/browser/internal/orchard_sync_state.cc.

@supermassive supermassive mentioned this pull request Feb 19, 2026
Open
@supermassive
Copy link
Collaborator Author

supermassive commented Feb 19, 2026

Review via brave-core-bot: Per Chromium guidelines, explicitly specify base::TaskPriority and shutdown behavior when creating thread pool task runners rather than relying on defaults. See OrchardSyncState::CreateSyncStateSequence() in components/brave_wallet/browser/internal/orchard_sync_state.cc.

This better be tracked by separate issue brave/brave-browser#52985

@supermassive supermassive force-pushed the refactor_zcash_tests branch 2 times, most recently from 3d03c86 to 29317c2 Compare February 20, 2026 15:37
bbondy
bbondy reviewed Feb 20, 2026
View reviewed changes
Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

Review via brave-core-bot

@supermassive supermassive force-pushed the refactor_zcash_tests branch 2 times, most recently from dc75776 to a76b89b Compare February 25, 2026 14:05
@supermassive supermassive enabled auto-merge (squash) February 25, 2026 14:05
@github-actions
Copy link
Contributor

github-actions bot commented Feb 26, 2026

Chromium major version is behind target branch (145.0.7632.120 vs 146.0.7680.32). Please rebase.

@github-actions github-actions bot added the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Feb 26, 2026
@github-actions github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Feb 28, 2026
@supermassive supermassive force-pushed the refactor_zcash_tests branch from 237ced6 to a4b46ba Compare March 2, 2026 05:57
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2026

[puLL-Merge] - brave/brave-core@33971

Description

This PR refactors the ZCashWalletService to decouple the construction of OrchardSyncState and its dedicated task runner sequence from the service's constructor. Instead of creating OrchardSyncState internally, the service now receives it via a SetupSyncState() method called after construction. This also introduces a TestingZCashWalletService helper class that simplifies test setup by managing the sync state lifecycle and eliminating the need for MockOrchardSyncStateProxy classes that were previously used to delegate calls from sequence-bound objects to mock objects on the test thread.

The key change is that base::SequenceBound<OrchardSyncState> is replaced with base::SequenceBound<std::unique_ptr<OrchardSyncState>> (via OrchardSyncState::SequenceBound type alias), which allows constructing the OrchardSyncState (or a mock subclass) on the main thread and then transferring ownership to the bound sequence. This eliminates the proxy pattern entirely, as mocks can now be directly passed into the sequence-bound wrapper.

Possible Issues

  1. sync_state_ used before SetupSyncState() called: The sync_state() accessor has a CHECK(sync_state_) guard, but there's a window between construction and SetupSyncState() where any method that accesses sync state would crash. If any code path (e.g., observer callbacks, keyring events) triggers sync state access before setup is complete, this would be a hard crash.

  2. Sequence checker DETACH_FROM_SEQUENCE pattern: Both OrchardStorage and OrchardSyncState now use DETACH_FROM_SEQUENCE in their constructors because they're constructed on one sequence and used on another. While this is a valid pattern, it means the sequence safety guarantee only kicks in after first use on the target sequence, reducing the protection window.

  3. TestingZCashWalletService::sync_state_ptr is a raw pointer: The sync_state_ptr member holds a raw pointer to the OrchardSyncState that lives inside a SequenceBound wrapper on another thread. Tests access this pointer (e.g., mock_orchard_sync_state()) from the test thread while the object nominally lives on a different sequence. This works because mock expectations are set up before tasks run, but it's fragile and could lead to data races if test patterns change.

  4. Database path construction moved: OrchardSyncState now appends "orchard.db" to the provided folder path internally, but OrchardStorage still receives a full file path. The OrchardSyncState constructor parameter was renamed from path_to_database to path_to_database_folder, but callers in tests pass temp_dir_.GetPath() which is correct, while the production code in BraveWalletService passes a kZCashDataFolderName-appended path. This split responsibility could lead to confusion.

Security Hotspots

  1. Cross-thread raw pointer access via sync_state_ptr: In TestingZCashWalletService, sync_state_ptr is a raw pointer to an object owned by a SequenceBound on a different sequence. While test-only, this pattern could mask real threading issues if tests evolve to run tasks concurrently.
Changes

Changes

  • brave_wallet_service.cc: Updated ZCashWalletService construction to use the new two-step pattern — construct service, then call SetupSyncState() with a separately created sequence and sync state.

  • orchard_storage.cc/h: Added DETACH_FROM_SEQUENCE in constructor to support construction on one sequence and operation on another.

  • orchard_sync_state.cc/h: Added static factory methods CreateSyncStateSequence() and CreateSyncState(). Changed constructor parameter to accept a folder path (appends DB filename internally). Added SequenceBound type alias as base::SequenceBound<std::unique_ptr<OrchardSyncState>>. Added DETACH_FROM_SEQUENCE.

  • zcash_wallet_service.cc/h: Removed second constructor and zcash_data_path_ member. Consolidated to single constructor. Added virtual SetupSyncState(). Moved sync_state(), zcash_rpc(), and CreateActionContext() to protected. Removed SetZCashRpcForTesting() and OverrideSyncStateForTesting().

  • zcash_action_context.cc/h: Updated sync_state type from base::SequenceBound<OrchardSyncState> to OrchardSyncState::SequenceBound.

  • zcash_test_utils.cc/h: Added TestingZCashWalletService class that manages sync state pointer for mocking and handles cleanup in destructor.

  • zcash_rpc.cc: Added CHECK_IS_TEST() guard when network_manager or url_loader_factory are null.

  • Multiple test files (zcash_wallet_service_unittest.cc, zcash_shield_sync_service_unittest.cc, zcash_auto_sync_manager_unittest.cc, zcash_create_*_unittest.cc, zcash_get_chaintip_status_task_unittest.cc, zcash_verify_chain_state_task_unittest.cc, zcash_resolve_transaction_status_task_unittest.cc, zcash_scan_blocks_task_unittest.cc, zcash_blocks_batch_scan_task_unittest.cc): Removed all MockOrchardSyncStateProxy classes. Replaced direct base::SequenceBound<OrchardSyncState> management with TestingZCashWalletService. Removed manual TearDown cleanup. Moved task_environment_ to be declared before other members for proper destruction order. Mock services now inherit from TestingZCashWalletService.

sequenceDiagram
    participant BW as BraveWalletService
    participant ZWS as ZCashWalletService
    participant OSS as OrchardSyncState
    participant OS as OrchardStorage
    participant Seq as SequencedTaskRunner

    BW->>ZWS: new ZCashWalletService(keyring_service, zcash_rpc)
    BW->>OSS: OrchardSyncState::CreateSyncStateSequence()
    OSS-->>BW: scoped_refptr<SequencedTaskRunner>
    BW->>OSS: OrchardSyncState::CreateSyncState(zcash_data_path)
    OSS->>OS: new OrchardStorage(path/orchard.db)
    Note over OS: DETACH_FROM_SEQUENCE
    OSS-->>BW: unique_ptr<OrchardSyncState>
    BW->>ZWS: SetupSyncState(sequence, sync_state)
    ZWS->>Seq: SequenceBound(sequence, move(sync_state))
    Note over Seq: OrchardSyncState now lives on dedicated sequence
    
    Note over ZWS: Later, during operation:
    ZWS->>ZWS: CreateActionContext(account_id)
    ZWS-->>ZWS: ZCashActionContext{rpc, addr, sync_state, account}
    ZWS->>Seq: sync_state.AsyncCall(...)
    Seq->>OSS: method call on dedicated sequence
    OSS->>OS: database operation
    OS-->>OSS: result
    OSS-->>Seq: result
    Seq-->>ZWS: callback with result
Loading

@supermassive supermassive force-pushed the refactor_zcash_tests branch from a4b46ba to 05154d0 Compare March 2, 2026 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbondy bbondy bbondy left review comments

@cypt4 cypt4 cypt4 approved these changes

+1 more reviewer

@netzenbot netzenbot netzenbot approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@supermassive supermassive

Labels

feature/web3/wallet/core feature/web3/wallet puLL-Merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor ZCash tests

4 participants

@supermassive @bbondy @cypt4 @netzenbot