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.

Sign up for GitHub

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

Jump to bottom

Byron addresses have derivation indexes that are not only hardened. #1041

Closed
3 tasks done
KtorZ opened this issue Nov 15, 2019 · 1 comment
Closed
3 tasks done

Byron addresses have derivation indexes that are not only hardened. #1041

KtorZ opened this issue Nov 15, 2019 · 1 comment
Assignees
@KtorZ
Milestone
Usability & Compa...

Comments

@KtorZ
Copy link
Member

KtorZ commented Nov 15, 2019 •
edited
Loading

Release Operating System Cause
v2019-11-14 Windows & OSX & Linux) Code v Human

Context

Addresses in Byron uses HD derivation with 3 levels: root, account and addresses.
It was believed that Byron addresses indexes used only hardened derivation, i.e. they would range from 2^31=2147483648 to 2^32=4294967296.

However, some addresses and in particular change addresses embed derivation indexes that range from outside of the hardened domain (e.g. 1594290073 was found in a mainnet address).

This appeared to has been fixed in cardano-sl in:

commit 4ebc883d4f08ffa13d3bce74e71792a3a54aeb42
Author: Edward Tate <[email protected]>
Date:   Tue Nov 28 12:01:26 2017 -0400

    [CSL-1955] Updated generateUnique to always generate hardened addresses and removed orM.

released somewhere between March 2018 and June 2018. So any old wallet before these dates have been using non-hardened indexes for addresses.

Steps to Reproduce

  1. Not all wallets are affected as they need to have addresses with indexes outside of the hardened domain.
  2. Wallets need to be old enough, when Daedalus was still running on v0 in [email protected], or [email protected] or [email protected].

Expected behavior

  1. Addresses that belong to a wallet should be discovered!

Actual behavior

  1. Some addresses are missing.

Resolution Plan

  • Use another type of derivation index specially for Byron addresses that can go over the whole range (hardened and non hardened).

  • Assess whether this is also true for the account path? Confirmed by looking at old versions of Cardano, accounts indexes were used using the same buggy function and are therefore subject to the same rule. We need to reflect this in code.

  • Curse Serokell one more time.

PR

Number Base
#1042 master
#1055 master

QA

  • Hard to reproduce since this occurred on old addresses that were carrying a wrong payload. Payloads are however encrypted with a passphrase derived from the root public key. Without this, we can't reliably know whether or not the fix solved issues of users.

  • We've however tried out with @Anviking's wallet for which the fix has been successful.

  • Additionally, the fixed version shipped to users has solved most user issues but a few. From those, it appears that they've been trying again using an old version of Daedalus which didn't include the fix.

  • Added an extra golden test in the address discovery spec for random derivation about an address which carries non-hardened derivation index. This more as a regression test to make sure that this bug isn't re-introduced in the future. (see: Golden test for Random discovery and addresses with non-hardened indexes #1055)
@KtorZ KtorZ self-assigned this Nov 15, 2019
@KtorZ KtorZ mentioned this issue Nov 15, 2019
Merged
1 task
iohk-bors bot added a commit that referenced this issue Nov 15, 2019
@iohk-bors @KtorZ
Merge #1042
4359edb 
1042: use a special 'ByronHybrid' index with range going from 0 to 2^32  r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#1041 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have used a special 'ByronHybrid' index with range going from 0 to 2^32 for the address path.

# Comments

<!-- Additional comments or screenshots to attach if any -->

:question: Should we also worry about the account path :question: 

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: KtorZ <[email protected]>
iohk-bors bot added a commit that referenced this issue Nov 15, 2019
@iohk-bors @KtorZ
Merge #1042
c7306e6 
1042: use a special 'ByronHybrid' index with range going from 0 to 2^32  r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#1041 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have used a special 'ByronHybrid' index with range going from 0 to 2^32 for the address path.

# Comments

<!-- Additional comments or screenshots to attach if any -->

:question: Should we also worry about the account path :question: 

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: KtorZ <[email protected]>
iohk-bors bot added a commit that referenced this issue Nov 15, 2019
@iohk-bors @KtorZ
Merge #1042
e00affe 
1042: use a special 'ByronHybrid' index with range going from 0 to 2^32  r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#1041 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have used a special 'ByronHybrid' index with range going from 0 to 2^32 for the address path.

# Comments

<!-- Additional comments or screenshots to attach if any -->

:question: Should we also worry about the account path :question: 

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: KtorZ <[email protected]>
@KtorZ KtorZ added this to the Usability & Compatibility milestone Nov 15, 2019
KtorZ added a commit that referenced this issue Nov 20, 2019
@KtorZ
add extra golden test in Random discovery for addresses including non…
87b685f 
… hardened indexes

As a regression test for #1041
@KtorZ KtorZ mentioned this issue Nov 20, 2019
Merged
1 task
KtorZ added a commit that referenced this issue Nov 20, 2019
@KtorZ
add extra golden test in Random discovery for addresses including non…
ff8b4f2 
… hardened indexes

As a regression test for #1041
@piotr-iohk
Copy link
Contributor

lgtm.

iohk-bors bot added a commit that referenced this issue Nov 21, 2019
@iohk-bors @KtorZ
Merge #1055
a318f20 
1055: Golden test for Random discovery and addresses with non-hardened indexes r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#1041 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have added an additional golden test to verify that we can correctly identify addresses that have non-hardened indexes in their derivation path.

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: KtorZ <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@KtorZ KtorZ

Labels
None yet
Projects
None yet
Milestone
Usability & Compatibility
Development

No branches or pull requests
2 participants
@KtorZ @piotr-iohk