Skip to content

librustzcash APIs for wallet key manipulation#9

Merged
str4d merged 5 commits into
zcash:masterfrom
arcalinea:wallet-apis
May 31, 2018
Merged

librustzcash APIs for wallet key manipulation#9
str4d merged 5 commits into
zcash:masterfrom
arcalinea:wallet-apis

Conversation

@arcalinea
Copy link
Copy Markdown

@arcalinea arcalinea commented May 14, 2018

  • librustzcash_to_scalar
  • librustzcash_ask_to_ak
  • librustzcash_nsk_to_nk
  • librustzcash_crh_ivk
  • librustzcash_check_diversifier
  • librustzcash_ivk_to_pkd

@arcalinea arcalinea requested a review from str4d May 14, 2018 12:01
@ebfull
Copy link
Copy Markdown
Collaborator

ebfull commented May 14, 2018

Can you rebase this on top of the latest librustzcash?

@arcalinea
Copy link
Copy Markdown
Author

Rebased

@arcalinea arcalinea force-pushed the wallet-apis branch 4 times, most recently from 77b724f to b184cc6 Compare May 15, 2018 03:49
@str4d str4d changed the title WIP: Sapling to_scalar wallet API librustzcash APIs for wallet key manipulation May 15, 2018
@arcalinea arcalinea force-pushed the wallet-apis branch 2 times, most recently from 115f152 to 06990af Compare May 15, 2018 06:00
Comment thread src/rustzcash.rs Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I can see why you might not want to use it though. It looks like you do the right thing here, so meh, it's okay.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Well, kinda. I think you should do what my code does, interpret it as a scalar (it should always work) and then use a write_fs afterwards, which should be implemented to write out in little endian bit order.

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.

Using that API requires six additional conversion read/write operations (to convert ak and nk to scalars and back, and to convert ivk to a Point and back). It didn't seem worth the cycles.

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.

Once this has been rebased, we should test this function against the test vectors.

Comment thread src/rustzcash.rs Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should be doing some kind of write_fs here instead.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

So extract it into a separate function like we did for read_fs?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I tried this in latest commit

Comment thread src/rustzcash.rs Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This will read from bytes into an Fs in little-endian bit order, which is what we want, but the function librustzcash_crh_ivk produces a little-endian byte order result and writes that out. Then, when you call librustzcash_ivk_to_pkd, it's not going to work correctly.

Copy link
Copy Markdown
Contributor

@str4d str4d May 16, 2018

Choose a reason for hiding this comment

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

Does it? I thought the intention was that the output of BLAKE2s was exactly the representation we wanted. More precisely, I think this is fine, because:

  • Here we are reading a little-endian byte array in big-endian, and then flipping, which means we need to flip both the u64s and their bits.
  • In ViewingKey.ivk() we flip the little-endian byte array before reading it as big endian, and since the endianness of the bits within each byte doesn't matter for reading, that should mean the result is the same as we get here by flipping the bits above.

I'll try and generate some test vectors to confirm.

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.

Now that #12 has been merged, this function needs to be updated (to use read_le and not swap bits).

Copy link
Copy Markdown
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

All new APIs should be tested against the test vectors that are now in master.

Comment thread src/rustzcash.rs Outdated
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.

Now that #12 has been merged, this function needs to be updated (to use read_le and not swap bits).

Comment thread src/rustzcash.rs Outdated
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.

Now that #12 has been merged, this function needs to be updated (to use read_le and not swap bits).

Comment thread src/rustzcash.rs Outdated
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.

Once this has been rebased, we should test this function against the test vectors.

@arcalinea arcalinea self-assigned this May 23, 2018
Copy link
Copy Markdown
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

Also needs tests (calling these functions inside the key component test function, and checking the output, should suffice).

Comment thread src/rustzcash.rs Outdated
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.

This needed to be removed as well.

Comment thread src/rustzcash.rs Outdated
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.

This needed to be removed as well.

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 the reverse() on line 80 also be removed?

@str4d
Copy link
Copy Markdown
Contributor

str4d commented May 30, 2018

I rebased this PR on master to fix merge conflicts and remove some stray commits that had crept in.

Copy link
Copy Markdown
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

ACK+cov

Comment thread src/rustzcash.rs
) {
// Should be okay, because caller is responsible for ensuring
// the pointer is a valid pointer to 32 bytes, and that is the
// size of the representation
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.

Comment is out-of-date (I think it might be intended to apply to the unsafe conversion of result).

@str4d str4d merged commit 18f4945 into zcash:master May 31, 2018
zkbot added a commit to zcash/zcash that referenced this pull request Jun 5, 2018
Add Sapling key classes to wallet

Leverages new librustzcash APIs added in zcash/librustzcash#9
str4d referenced this pull request in str4d/librustzcash Aug 28, 2018
Various improvements to project structure and implementation

This makes wNAF/multiexp more modularized and abstract (for use later in remodeling groth) and starts moving other things around.

Also, good chance to start working on buildbot.
nuttycom added a commit that referenced this pull request Dec 8, 2025
…14868de..23f0768ea

23f0768ea Release lightwallet-protocol v0.4.0
41156c767 Merge pull request #11 from zcash/feature/get_mempool_tx_pools
7c130e883 Add `lightwalletProtocolVersion` field to `LightdInfo` struct.
edbb726d7 Apply suggestion from code review
38fddd73b Apply suggestions from code review
0250f2720 Add pool type filtering to `GetMempoolTx` argument.
54ccaadd5 Change semantics of pool-based pruning of compact transactions from "may prune" to "must prune".
b0667ec99 Merge pull request #9 from zcash/2025-11-doc-TransparentAddressBlockFilter
f3fea7bd4 doc: TransparentAddressBlockFilter doesn't include mempool
a67dd323a Merge pull request #8 from zcash/2025-11-lightdinfo-upgrade-info
11da4b7e3 add next upgrade info to LightdInfo structure (GetLightdInfo)
42cd8f720 Transparent data docs update (#7)
c0cf957ac Merge pull request #5 from zcash/2025-11-comments
912fc3609 Minor clarification in GetBlockRange documentation.
6b03f2cce Documentation (comments) only
d978256a2 Merge pull request #1 from zcash/compact_tx_transparent
7eeb82e7c Merge pull request #4 from zcash/add_changelog
a95359dc9 Apply suggestions from code review
592b637a8 Add transparent data to the `CompactBlock` format.
9d1fb2c41 Add a CHANGELOG.md that documents the evolution of the light client protocol.
180717dfa Merge pull request #3 from zcash/merge_librustzcash_history
450bd4181 Merge the history of the .proto files from `librustzcash` for complete history preservation.
a4859d11d Move protobuf files into place for use in `zcash/lightwallet-protocol`
2e66cdd9e Update zcash_client_backend/proto/service.proto
eda012519 fix comment
f838d10ad Add gRPC LightdInfo Donation Address
db12c0415 Merge pull request #1473 from nuttycom/wallet/enrichment_queue
698feba96 Apply suggestions from code review
20ce57ab3 zcash_client_backend: Add `block_height` argument to `decrypt_and_store_transaction`
a6dea1da8 Merge pull request #1482 from zancas/doc_tweak
4d2d45fc9 fix incorrect doc-comment
e826f4740 update CompactBlock doc-comment, to cover non-Sapling shielded notes, and addresses
e9a6c00bf Various documentation improvements
988bc7214 Merge pull request #872 from nuttycom/feature/pre_dag_sync-suggest_scan_ranges
58d07d469 Implement `suggest_scan_ranges` and `update_chain_tip`
a9222b338 Address comments from code review.
e20310857 Rename proto::compact::{BlockMetadata => ChainMetadata}
ac63418c5 Reorganize Sapling and Orchard note commitment tree sizes in CompactBlock.
0fdca14f1 zcash_client_backend: Add note commitment tree sizes to `CompactBlock` serialization.
2a0c2b8b7 zcash_client_backend: Add gRPC bindings behind feature flag
1342f0480 zcash_client_backend: Address compact_formats.proto comments
68aa4e01b zcash_client_backend: Bring in latest `compact_formats.proto`
e712eb1bc Add prevHash field to CompactBlock
440384c3e Build protobufs for compact formats

git-subtree-dir: zcash_client_backend/lightwallet-protocol
git-subtree-split: 23f0768ea4471b63285f3c0e9b6fbb361674aa2b
greg0x referenced this pull request in valargroup/librustzcash Mar 12, 2026
feat: Elgamal Cosmos SDK wiring & tests
michaeltout pushed a commit to VerusCoin/librustzcash that referenced this pull request Mar 17, 2026
Align librustzcash in 'ka_agree' branch with current PR branch
TyphooN- added a commit to TyphooN-/junocash that referenced this pull request May 24, 2026
Add Sapling key classes to wallet

Leverages new librustzcash APIs added in zcash/librustzcash#9
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.

4 participants