Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Derive within spending key alphanet v5 compat pr #317

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dan-da
Copy link
Collaborator

@dan-da dan-da commented Jan 9, 2025

Supersedes #304

This modifies generation_address so that the same addresses will
be derived from a given wallet secret as happened at tag alphanet-v5 (Jan 6, 2024)

A test exists which verifies this:
wallet_tests::generation_key_derivation::verify_derived_generation_addrs()

Significant changes since #304:

  • compatibility with alphanet-v5 addresses:

    • DerivationIndex: u128 --> u64
    • replace GenerationSpendingKey::seed with secret, index
    • rename GenerationSpendingKey::from_seed() --> from_secret_and_index()
    • add GenerationSpendingKey::seed(secret, index) which uses same
      hash(secret, generation_flag, index) construction as
      WalletSecret::nth_generation_key() used to.
    • WalletSecret::master_generation_key is now DerivationIndex::MAX so
      that the 0th key can have a parent.
    • GenerationSpendingKey::derive_child() now uses wrapping_add() so that
      master key (index = DerivationIndex::MAX) can derive 0th key.
  • add KeyTypeSeed to abstract over seed types

Minor changes:

  • update a couple Block tests to use mainnet.
  • rename SpendingKeyIter::curr --> curr_fwd
  • rename SpendingKeyIter::curr_back --> curr_rev
  • updates test verify_derived_generation_keys with newly generated keys
    because serialized key format has changed.
  • add some missing doc-comments for key/addr in generation_address.rs
  • update GenerationSpendingKey deserialization impl
  • add impl From<&GenerationSpendingKey> for GenerationReceivingAddress
  • remove GenerationReceivingAddress::from_spending_key()
  • update Arbitrary impls for GenerationSpendingKey seeds
  • add a couple of Arbitrary impl to the feature flag.
  • adapt tests to changes

Of special note for review:

  1. note the fields that changed in GenerationSpendingKey and the method from_seed() became from_secret_and_index().

  2. consider security impiications that every key now embeds the wallet master secret. This means that if any key is compromised every key in the wallet is compromised. If this is not acceptable (and it probably shouldn't be), I believe it is a deal-breaker for backwards compatibility. I don't believe there's any other way to have both self-deriving keys and backwards compatibility, but feel free to prove me wrong....

  3. compare impl of GenerationSpendingKey::seed() with WalletSecret::nth_generation_spending_key() in master.

  4. also see GenerationSpendingKey::derive_child()

  5. note how the master generation key in WalletSecret is DerivationIndex::MAX, and that GenerationSpendingKey::derive_child() allows wrapping add, so that the master key can be parent of the 0th key. I initially made an impl that did not do this, and instead the master-key is the 0th key, but this complicated the implementation of the nth_* methods and made them a bit ugly and non-obvious. I feel this impl is overall more elegant.

  6. the DerivationIndex is a u64 instead of u128 -- for all key types. I think it's fine.... that's still a huge amount of possible addresses for a wallet.

  7. review test: wallet_tests::generation_key_derivation::verify_derived_generation_addrs()


with regards to point 2, see #317 (comment)

With regards to point 2: my conclusion after some reflection is that the weakened security risk is not worth it, and we should prefer #304 rather than this PR. If we are not willing to accept the weakened security, we have a choice:

a. implement self-derived keys in a backwards incompatible way, eg #304.
b. prioritize backwards compatiiblity and do not use self-deriving keys at all.

I very much prefer (a) even if it means some short-term pain because we have to obtain new addresses from 3rd parties. I think that self-deriving keys bring a lot of value for the ecosystem that will be lost if every piece of software must have access to the wallet secret directly or via RPC.

dan-da added 3 commits January 9, 2025 14:35
Primary features:
1. SpendingKey can now derive its own children without any wallet state.
   This enables RPC clients to derive keys themselves.

2. Derivation iterators.  SpendingKeyIter and SpendingKeyParallelIter.
   The latter leverages rayon to utilize all cpu cores.

3. /known_keys rpc now returns a SpendingKeyRange instead of a list of
   addresses.  This is a big perf win for large wallets.

changelog:

  refactor spending-key types so each key can derive its own children
  known_keys rpcs now return iterators instead of Vec
  DerivationIndex: u64 -> u128.
  fix premine_distribution(). separate test vs non-test.  make it aware of current network
  impl IntoIterator for SpendingKey
  GenerationReceivingAddress::derive_from_seed() -> from_seed()
  impl rayon parallel iterators
  SpendingKeyIter now starts at child 0, instead of parent_key.  fix some DoubleEndedIter issues
  iteration tests
  add test double_ended_range_iterator_meet_middle
  add test double_ended_iterator_to_first_elem
  add tests double_ended_range_iterator_to_first_elem, range_iterator_to_last_elem, iterator_nth
  separate SpendingKeyParallelIter from SpendingKeyIter
  add par_iter tests
  iterator accepts any impl RangeBounds
  tests to validate RangeBounds conversions
  known_keys() rpc returns SpendingKeyRange instead of SpendingKeyIter
  only perform extra GenerationSpendingKey assertions for debug build. add comments
fixes a panic in SpendingKeyParallelIter::collect().

also adds a test assertion that count() is correct. An incorrect
count was another symptom of the bug.

big thanks to @cuviper for spotting the problem.

rayon-rs/rayon#1224 (comment)
This modifies generation_address so that the same addresses will
be derived from a given wallet secret as happened at tag alphanet-v5.

A test exists which verifies this:
  wallet_tests::generation_key_derivation::verify_derived_generation_addrs()

Significant changes:

* compatibility with alphanet-v5 addresses:
    * DerivationIndex: u128 --> u64
    * replace GenerationSpendingKey::seed with secret, index
    * rename GenerationSpendingKey::from_seed() --> from_secret_and_index()
    * add GenerationSpendingKey::seed(secret, index) which uses same
       hash(secret, generation_flag, index) construction as
       WalletSecret::nth_generation_key() used to.
    * WalletSecret::master_generation_key is now DerivationIndex::MAX so
      that the 0th key can have a parent.
    * GenerationSpendingKey::derive_child() now uses wrapping_add() so that
      master key (index = DerivationIndex::MAX) can derive 0th key.

* add KeyTypeSeed to abstract over seed types

Minor changes:
* update a couple Block tests to use mainnet.
* rename SpendingKeyIter::curr      --> curr_fwd
* rename SpendingKeyIter::curr_back --> curr_rev
* updates test verify_derived_generation_keys with newly generated keys
  because serialized key format has changed.
* add some missing doc-comments for key/addr in generation_address.rs
* update GenerationSpendingKey deserialization impl
* add impl From<&GenerationSpendingKey> for GenerationReceivingAddress
* remove GenerationReceivingAddress::from_spending_key()
* update Arbitrary impls for GenerationSpendingKey seeds
* add a couple of Arbitrary impl to the feature flag.
* adapt tests to changes
@dan-da
Copy link
Collaborator Author

dan-da commented Jan 10, 2025

I have another idea how to impl self-deriving keys.

first, a review of the implementations thus far.


historical impl, in master (not self deriving):

key = (digest)

// performed by WalletSecret
child_key = hash(wallet_secret, generation_flag, child_index)
0th_key = hash(wallet_secret, generation_flag, 0)

This has the property that if any single key is compromised, no other key is compromised. No key can be derived without access to wallet-secret, which is never supplied to client(s) via RPC.


first self-deriving impl from #304:

key = (digest)
child_key = hash(digest, child_index)

master_key = hash(wallet_secret, generation_flag)
0th_key = hash(master_key, 0)

This has property/problem that any key can derive all its children. So if key with index = 5 is compromised then all index > 5 are likewise compromised.

Also, these keys/addrs do not match legacy keys from alphanet-v5.


2nd self-deriving impl, in this pr:

key = (wallet_secret, index)
key::seed() = hash(wallet_secret, generation_flag, index)
child_key = (wallet_secret, key.index.wrapping_add(1))

master_key = (wallet_secret, DerivationIndex::MAX)
0th_key = (wallet_secret, master_key.index.wrapping_add(1))

This has property/problem that any key can derive all keys. So if key with index = 2005 is compromised then all index <= 2005 and >= 2005 are compromised.

these keys/addrs match legacy keys from alphanet-v5.


finally, a possible 3rd construction, master-key-only derivation:

master_key = (wallet_secret)
key = (digest)

// performed by master_key
child_key = hash(wallet_secret, generation_flag, child_index)
0th_key = hash(wallet_secret, generation_flag, 0)

Here the master-key would store wallet_secret while the sub-keys would store only a digest. This is the original/legacy impl except that the master-key would be a serializable type intended for use by client software.

This has the property that if the master key is compromised all keys are compromised. However if any spending key is compromised, no other key is compromised.

WIth this construction spending keys are not self-deriving, but the master-key can derive all child keys. I believe this is similar to xprv in bip32.


@aszepieniec I leave it to you to consider. Please let me know if any q's.

@aszepieniec
Copy link
Contributor

I can't pass judgment because I'm confused about this summary. Please clarify and/or confirm.

  • There are at least four constructions being discussed: (1) historical/current master; (2) first self-deriving impl from feat: SpendingKey self derivation, parallel iteration #304; (3) second self-deriving impl / this PR; (4) master-key-only.
  • The keys and addresses from (3) / this PR match with legacy keys from alphanet-v5.
  • The keys and addresses from (1) / current master do not match with legacy keys from alphanet-v5.
  • In proposal (1), child keys are derived by/from WalletSecret whereas proposals (2), (3), (4) they are derived from other keys.
  • The line key = ... describes the type of keys under that proposal.
  • The method wrapping_add modifies the zeroth argument (the thing before the dot) and returns the result of the wrapping add.
  • master_key denotes the secret string of random bits that lives in WalletSecret and is the thing that is imported through the CLI command import-seed-phrase.
  • What does key::seed() = hash(wallet_secret, generation_flag, index) do?

Based on abstract consideration of the properties (ignoring how they arise from the concrete implementations) I tend towards this position. (But I reserve the right to change my mind after understanding the matter better.)

  • Supporting legacy keys is the top priority.
  • Given that legacy keys are supported, self-derivation of keys is a feature even if it comes with the property that a compromised child compromised the entire family.
  • Self-deriving keys without that property are preferable.
  • I think it is silly and needlessly complex to limit self-derivation to distinguished keys based on the above bullet point.

@aszepieniec
Copy link
Contributor

I wonder whether you ever considered this scheme? If so, what what made you decide against it?

key_i = Hash(key_{i >> 1}, i % 1)
master_key = key_0

It arranges all keys into a balanced binary tree such that every key is the root of some branch, and every key has two immediate children. If a key is compromised, only its branch is compromised.

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.

2 participants