Skip to content

Implement z_importkey and z_exportkey for Sapling#400

Merged
oxarbitrage merged 1 commit into
mainfrom
z_importexportkey
Jun 23, 2026
Merged

Implement z_importkey and z_exportkey for Sapling#400
oxarbitrage merged 1 commit into
mainfrom
z_importexportkey

Conversation

@oxarbitrage

@oxarbitrage oxarbitrage commented Mar 20, 2026

Copy link
Copy Markdown
Contributor

Adds two new RPC methods:

  • z_importkey: Imports a Sapling extended spending key into the wallet. The key is encrypted with age and stored in the keystore. A UFVK is derived and imported into the wallet database so the sync engine can track transactions. The rescan parameter controls whether historical blocks are scanned ("yes", "no", or "whenkeyisnew"), and start_height sets the birthday for the imported account. Real treestates are fetched from the chain indexer for non-zero start heights.

  • z_exportkey: Exports the spending key for a Sapling payment address. Requires the wallet to be unlocked. Looks up the key by iterating standalone sapling DFVKs and using decrypt_diversifier to match the address.

Also adds fetch_account_birthday to json_rpc::utils as a shared helper for building an AccountBirthday from a chain treestate.

Includes unit tests for parameter validation, key encoding/decoding roundtrips, and address parsing.

Close #69
Close #79
Closes COR-254 and COR-259

@nuttycom

Copy link
Copy Markdown
Contributor

I'm not sure that it makes sense to implement the legacy versions of these methods; their utility is extremely limited and I would prefer that we avoid introducing API surface that we then have to maintain. Both of these methods got zero points in our survey of what people are using: https://docs.google.com/spreadsheets/d/1UJxH1cowexGqadU32Uei5Qak6jGhXjb18-T_QBPmDAA/edit?gid=0#gid=0

@nuttycom

Copy link
Copy Markdown
Contributor

I guess that I can see implementing z_importkey for Sapling paper wallet users. But key export in particular I'm skeptical of - it seems like an avenue to lose funds if you trust z_exportkey for backup without realizing that there might be Orchard funds associated with the same root of spending authority.

@oxarbitrage

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback.

I'm not sure that it makes sense to implement the legacy versions of these methods; their utility is extremely limited and I would prefer that we avoid introducing API surface that we then have to maintain. Both of these methods got zero points in our survey of what people are using: https://docs.google.com/spreadsheets/d/1UJxH1cowexGqadU32Uei5Qak6jGhXjb18-T_QBPmDAA/edit?gid=0#gid=0

I wasn’t aware of the survey, thanks for sharing it. I’ll consider it for future work. It might also make sense to close or re-evaluate issues in the repository that correspond to methods with zero usage.

That said, I do think import-related APIs may become more valuable as zcashd is deprecated and users look for ways to migrate into the new stack.

I guess that I can see implementing z_importkey for Sapling paper wallet users.

Not only for paper wallets. If you have a legacy Sapling key (with funds) and want to import it into the new stack without relying on zcashd, this provides a viable path. I used this same RPC method in zcashd to achieve that migration.

But key export in particular I'm skeptical of - it seems like an avenue to lose funds if you trust z_exportkey for backup without realizing that there might be Orchard funds associated with the same root of spending authority.

That makes sense. The export functionality was mainly added for completeness. I’m happy to remove it, or alternatively keep it with clearer documentation highlighting the risks.

Let me know how you’d prefer to proceed. Happy to adjust the PR accordingly.

@oxarbitrage

Copy link
Copy Markdown
Contributor Author

By the way, I should have provided more context earlier. The main motivation here was recovering legacy funds that were otherwise inaccessible.

I had the mnemonic, but none of the available tools worked for recovery (zeccavator, zodl/zashi, and ywallet all failed in this case). The only viable path was to derive the Sapling key from the mnemonic (I wrote a small tool for that: https://github.com/oxarbitrage/zcash-sapling-derive) and then import it into zcashd using z_importkey.

That worked for me, but it made me realize that this recovery path would no longer be available once zcashd is deprecated. That’s what motivated porting these calls into zallet.

@ValarDragon

Copy link
Copy Markdown

I suggest we add the import key logic here, so zcashd folks can migrate

@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.

Looks good to me! I left a few minor comments, but nothing blocking (modulo merge conflicts)

utACK 532371e

Comment thread zallet/src/components/json_rpc/utils.rs Outdated
.get_treestate(treestate_height.to_string())
.await
.map_err(|e| {
LegacyCode::InvalidParameter.with_message(format!(

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.

s/LegacyCode::InvalidParameter/RpcErrorCode::InternalError/

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — now InternalError (kept the message via ErrorObjectOwned, since ErrorCode has no with_message).

.map_err(|e| LegacyCode::Database.with_message(e.to_string()))?;

if let Some(tip) = chain_tip {
if start_height > tip {

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.

(not blocking) might be nice to skip this check if rescan="no".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — the start_height > tip check is now skipped when rescan="no".

// - "no" → use the current chain tip so the sync engine only tracks new
// transactions going forward.
//
// TODO: When rescan is "yes" and the key already exists, zcashd would force a

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.

The recently added rewind_to_height() method should do this for you now

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deferred — rewind_to_height rewinds the whole wallet (all accounts), not just this key, so I left the TODO (now referencing it) rather than trigger a global rewind on import.

// does not expose a way to reset scan ranges for an existing account.
let effective_height = match rescan {
"yes" | "whenkeyisnew" => start_height,
"no" => chain_tip.unwrap_or(BlockHeight::from_u32(0)),

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.

Not sure how I feel about silently failing over to 0 here

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.

Thinking on it some more, I'm actually ok with it. I would support logging a warning here, though (not blocking)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Superseded by your follow-up below — added a warning log.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — tracing::warn! when the rescan="no" path falls back to genesis.

) -> RpcResult<AccountBirthday> {
if height == BlockHeight::from_u32(0) {
return Ok(AccountBirthday::from_parts(
ChainState::empty(BlockHeight::from_u32(0), BlockHash([0; 32])),

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.

Should this at least include the real genesis hash?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Left as zeros, with a comment explaining why: the chain state is the state before the birthday, and nothing precedes genesis, so the zero parent hash is the correct anchor — the real genesis hash would be the hash of block 0 itself.

@oxarbitrage

Copy link
Copy Markdown
Contributor Author

Thanks for the review, and apologies for the delay in getting back to this!

Rebased onto main and resolved the keystore.rs conflict from the migration refactor: the encrypt helpers are now Encryptor methods on main, so I dropped the branch's duplicate free functions and kept only the new decrypt_standalone_sapling_extsk; I also dropped the zcashd-import cfg from Encryptor::encrypt_standalone_sapling_key, since the always-built z_importkey path needs it.

Addressed the review comments (replied inline on each thread):

  • fetch_account_birthday: treestate-fetch failure now returns InternalError instead of InvalidParameter.
  • import_key: skip the start_height > tip check when rescan="no" (start_height is unused there); tracing::warn! when the rescan="no" path falls back to genesis; refined the existing-key-rescan TODO to note rewind_to_height (deferred, since it rewinds the whole wallet rather than just this key).
  • Genesis ChainState: left as BlockHash([0; 32]) with a comment explaining why (no block precedes genesis, so the zero parent hash is the correct anchor).

Re @nuttycom's concern: z_exportkey's help text now warns that it is not a complete backup (Orchard funds under the same spending authority are not exported). Added a CHANGELOG entry for both methods.

Note: the two CI failures (Test NU7 and Latest build) look pre-existing and dependency-related (zebra-chain not covering NetworkUpgrade::Nu7, and orchard = "^0.13" failing to resolve), not from this PR.

@linear

linear Bot commented Jun 8, 2026

Copy link
Copy Markdown

COR-254

COR-259

Adds two new RPC methods:

- `z_importkey`: Imports a Sapling extended spending key into the wallet.
  The key is encrypted with age and stored in the keystore. A UFVK is
  derived and imported into the wallet database so the sync engine can
  track transactions. The rescan parameter controls whether historical
  blocks are scanned ("yes", "no", or "whenkeyisnew"), and start_height
  sets the birthday for the imported account. Real treestates are fetched
  from the chain indexer for non-zero start heights.

- `z_exportkey`: Exports the spending key for a Sapling payment address.
  Requires the wallet to be unlocked. Looks up the key by iterating
  standalone sapling DFVKs and using decrypt_diversifier to match the
  address. Its documentation warns that the exported key is not a complete
  backup, as Orchard funds under the same spending authority are not
  represented.

Also adds `fetch_account_birthday` to `json_rpc::utils` as a shared
helper for building an AccountBirthday from a chain treestate.

Includes unit tests for parameter validation, key encoding/decoding
roundtrips, and address parsing, and a CHANGELOG entry for both methods.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@oxarbitrage

Copy link
Copy Markdown
Contributor Author

Validated this PR end-to-end against the dedicated integration test (zcash/integration-tests#76, wallet_import_export_key.py) by running it through the interop CI (ZIT-Revision), building zallet from this branch:

  • wallet_import_export_key.py | True | 16 s — covers import → listaddresses (sapling UA under imported_watchonly) → re-import idempotency → z_exportkey round-trip → cross-wallet isolation → rescan param values → and the error paths (out-of-range height, invalid rescan, invalid key, address-not-in-wallet).

The only red shard in that run was an unrelated, pre-existing failure on nuparams.py (getblocksubsidy subsidy assertion), which this PR's diff doesn't touch — tracking that separately on the integration-tests side.

This is approved + mergeable + green, and now interop-validated. Planning to merge shortly.

@oxarbitrage oxarbitrage merged commit a0f4556 into main Jun 23, 2026
38 checks passed
@nuttycom nuttycom deleted the z_importexportkey branch June 23, 2026 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rpc: Implement z_importkey rpc: Implement z_exportkey

4 participants