Skip to content

Migrate to getwalletstatus RPC for syncing wallets#56

Merged
nullcopy merged 4 commits into
mainfrom
zallet-getwalletstatus
Jun 10, 2026
Merged

Migrate to getwalletstatus RPC for syncing wallets#56
nullcopy merged 4 commits into
mainfrom
zallet-getwalletstatus

Conversation

@str4d

@str4d str4d commented Mar 12, 2026 •
edited by oxarbitrage
Loading

Copy link
Copy Markdown
Collaborator

Closes #63.
Closes COR-1088.
Closes #105.

Depends on zcash/wallet#367 and zcash/wallet#455.

@str4d str4d force-pushed the zallet-getwalletstatus branch from bbdc8ac to 6489d41 Compare March 12, 2026 09:30
@str4d str4d mentioned this pull request Mar 12, 2026
Merged
@str4d str4d force-pushed the zallet-getwalletstatus branch 3 times, most recently from 79c082b to 5f84b1f Compare March 16, 2026 00:20
@linear

linear Bot commented Mar 16, 2026

Copy link
Copy Markdown

COR-1088 Migrate to `getwalletstatus` RPC for syncing wallets

@str4d str4d marked this pull request as draft March 16, 2026 00:26
@oxarbitrage

oxarbitrage commented Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Picking this up and pushing a follow-up commit (721b513).

Two issues surfaced when validating end-to-end against a zallet carrying zcash/wallet#367 (getwalletstatus) + zcash/wallet#455 (empty shielded-tree fix):

  1. KeyError: 'wallet_tip' in the new sync barrier. getwalletstatus declares wallet_tip as Option<ChainTip> with skip_serializing_if = "Option::is_none", so the field is omitted until the wallet has a committed tip — which happens transiently right after blocks are mined. The barrier indexed it unconditionally and crashed ~1 run in 4, exactly during the window it's meant to poll through. Now sync_blocks/sync_mempools/rebuild_cache treat an absent wallet_tip as "not synced yet" and keep polling.
  2. wallet.py still raced because it read wallet state immediately / with a fixed time.sleep(1) rather than calling the now wallet-aware self.sync_all(). Switched it over.

Validated 10/10 green locally with the fix (was ~22% flaky before).

This supersedes #106 (the bespoke wait_for_* pollers there are no longer needed now that getwalletstatus exists) and Closes #105. Closing #106 in favor of this.

⚠️ CI here will stay red until zcash/wallet#367 + #455 land upstream (stock zallet has no getwalletstatus yet).

This was referenced Jun 5, 2026
Closed
Open
str4d and others added 2 commits June 10, 2026 19:56
@str4d @dannywillems
Migrate to getwalletstatus RPC for syncing wallets
698acf9
@oxarbitrage @claude @dannywillems
test framework: handle absent getwalletstatus wallet_tip; sync wallet…
b0eade2 
…s in wallet.py

The getwalletstatus sync barrier indexed `wallet_tip` unconditionally, but the
RPC omits that field (Option + skip_serializing_if) until the wallet has a
committed tip — which happens transiently right after blocks are mined. The
barrier therefore raised `KeyError: 'wallet_tip'` ~1 run in 4, exactly during
the window it is meant to poll through. Treat an absent wallet_tip as "not
synced yet" and keep polling, in sync_blocks, sync_mempools, and rebuild_cache.

Also make wallet.py use the now wallet-aware self.sync_all() instead of an
immediate read and a fixed time.sleep(1), removing the remaining flaky
wallet-scan race (and the unused `import time`). This supersedes the bespoke
pollers in #106.

Validated 10/10 green locally against a zallet carrying zcash/wallet#367
(getwalletstatus) + zcash/wallet#455 (empty shielded tree fix).

Closes #105

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dannywillems dannywillems force-pushed the zallet-getwalletstatus branch from 721b513 to b0eade2 Compare June 10, 2026 19:53
@dannywillems dannywillems marked this pull request as ready for review June 10, 2026 19:53
@dannywillems dannywillems requested a review from nullcopy June 10, 2026 19:57
dannywillems
dannywillems approved these changes Jun 10, 2026
View reviewed changes

@dannywillems dannywillems left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

utACK c0fd062, with utACK of zcash/wallet#367 at 7404cbf5b044500f4f5a9233e855c56a137f6feb.

We will have more visibility on the overall impact of this change when #125 will have been merged.

@dannywillems

dannywillems commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

utACK c0fd062, with utACK of zcash/wallet#367 at 7404cbf5b044500f4f5a9233e855c56a137f6feb.

We will have more visibility on the overall impact of this change when #125 will have been merged.

I approved based on the hypothesis that zcash/wallet#367 is supposed to be implemented to rely less on calls to time.sleep for wallet syncing, which this patch does.

@dannywillems dannywillems force-pushed the zallet-getwalletstatus branch from c0fd062 to 4cf2be8 Compare June 10, 2026 20:09
nullcopy
nullcopy approved these changes Jun 10, 2026
View reviewed changes

@nullcopy nullcopy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

utACK 4cf2be8

@nullcopy nullcopy merged commit b51aa23 into main Jun 10, 2026
24 of 41 checks passed
@nullcopy nullcopy deleted the zallet-getwalletstatus branch June 10, 2026 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dannywillems dannywillems dannywillems approved these changes

@nullcopy nullcopy nullcopy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

4 participants

@str4d @oxarbitrage @dannywillems @nullcopy