Skip to content

fix:adjust default paths and gap limits to match android/iOS behavior #156

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

Merged
QuantumExplorer merged 11 commits into from
Oct 14, 2025
Merged

fix:adjust default paths and gap limits to match android/iOS behavior #156

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9f845aa
fix: add a BIP32 path to Default wallet creation options
HashEngineering Oct 3, 2025
0f0978e
fix: increase gap limits to match Android and iOS defaults
HashEngineering Oct 3, 2025
cc8d49c
chore: fix special gap limit comment
HashEngineering Oct 4, 2025
bbf43a9
fix: add BIP32 account 0 to create_accounts_with_passphrase_from_options
HashEngineering Oct 4, 2025
971be53
style: format to pass fmt
HashEngineering Oct 4, 2025
7fc5df7
tests: update tests for adding BIP32 to Default wallet
HashEngineering Oct 4, 2025
430da5b
style: add missing ,
HashEngineering Oct 4, 2025
3cdd496
chore: remove unused import
HashEngineering Oct 4, 2025
7bec305
chore: fix comment in test
HashEngineering Oct 4, 2025
a282394
fix: lower gap limits based on testing with one old wallet from 2019
HashEngineering Oct 7, 2025
c65c6e1
style: fix format
HashEngineering Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion key-wallet-manager/tests/integration_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ fn test_account_management() {
// Get accounts from wallet - Default creates 7 accounts, plus the one we added
let accounts = manager.get_accounts(&wallet_id);
assert!(accounts.is_ok());
assert_eq!(accounts.unwrap().len(), 8); // 7 from Default + 1 we added
assert_eq!(accounts.unwrap().len(), 9); // 8 from Default + 1 we added
}

#[test]
Expand Down
8 changes: 5 additions & 3 deletions key-wallet/src/gap_limit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@ use serde::{Deserialize, Serialize};
use std::collections::HashSet;

/// Standard gap limit for external addresses (BIP44 recommendation)
pub const DEFAULT_EXTERNAL_GAP_LIMIT: u32 = 20;
pub const DEFAULT_EXTERNAL_GAP_LIMIT: u32 = 100;

@QuantumExplorer QuantumExplorer Oct 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This would represent a significant additional computation, and I'm not sure the benefit. I Think iOS used to use 30.

@QuantumExplorer QuantumExplorer Oct 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe GC filters are checked sequentially as well... so the added benefit we got with bloom filters no longer applies.

@PastaPastaPasta PastaPastaPasta Oct 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think he said that android uses 100; and so to confirm compatibility; and to get correct balances in the mobile test wallet we needed to increase. It's possible 30 work would work; @HashEngineering please advise.

@HashEngineering HashEngineering Oct 6, 2025

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.

Let me check 30 and also look up what is in DashSync.

@HashEngineering HashEngineering Oct 6, 2025

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.

DashSync: https://github.com/dashpay/dashsync-iOS/blob/753f9446cfd20ae5da996104a5870df839005d4c/DashSync/shared/Models/Derivation%20Paths/DSFundsDerivationPath.h#L10-L21

#define SEQUENCE_GAP_LIMIT_EXTERNAL 10
#define SEQUENCE_GAP_LIMIT_INTERNAL 5
#define SEQUENCE_GAP_LIMIT_INITIAL 100

#define SEQUENCE_UNUSED_GAP_LIMIT_EXTERNAL 10
#define SEQUENCE_UNUSED_GAP_LIMIT_INTERNAL 5
#define SEQUENCE_UNUSED_GAP_LIMIT_INITIAL 15

#define SEQUENCE_DASHPAY_GAP_LIMIT_INCOMING 6
#define SEQUENCE_DASHPAY_GAP_LIMIT_OUTGOING 3
#define SEQUENCE_DASHPAY_GAP_LIMIT_INITIAL 10

@HashEngineering HashEngineering Oct 7, 2025

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.

Using the test wallet that was created with iOS, 30 external and 30 internal gap limits resulted in successful restoration. In this particular wallet, 20 internal was not sufficient.


/// Standard gap limit for internal (change) addresses
pub const DEFAULT_INTERNAL_GAP_LIMIT: u32 = 10;
pub const DEFAULT_INTERNAL_GAP_LIMIT: u32 = 100;

@QuantumExplorer QuantumExplorer Oct 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is no point in having 100 here, it would mean we skipped 99 internal addresses. I'm not even sure how you can miss 1.

@QuantumExplorer QuantumExplorer Oct 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess we should have it at 25, because that's how many transactions you can have in a block from one ancestor. But honestly with some good coding we could have this at 5.

@HashEngineering HashEngineering Oct 6, 2025

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.

I agree, internal can be less because the addresses are generated by the app when spending and not by the user.

@HashEngineering HashEngineering Oct 7, 2025

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.

Using the test wallet that was created with iOS, 30 external and 30 internal gap limits resulted in successful restoration. In this particular wallet, 20 internal was not sufficient.


/// Standard gap limit for CoinJoin addresses
pub const DEFAULT_COINJOIN_GAP_LIMIT: u32 = 10;
pub const DEFAULT_COINJOIN_GAP_LIMIT: u32 = 500;

@QuantumExplorer QuantumExplorer Oct 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I really don't think we need more than 30 here.

@HashEngineering HashEngineering Oct 6, 2025

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.

We had problems with lower numbers, previously, but some of those problems were due not updating the bloom filters often enough.

With coinjoin, on Android, we support up to 6 active sessions and each session could be using up to 9 keys. That means 54 keys could be used at a time.

The number can be lower than 500 -- I can do testing to see what would work based on some of my wallets.

@HashEngineering HashEngineering Oct 7, 2025

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.

Using a different test wallet which has 4300 transactions, mostly coinjoin, I found that a gap limit of 30 is sufficient.

The number of keys used in those 4300 transactions was 12068. With the DashJ library, there is an algorithm that searches for unused keys and reuses them. Unused keys would be from failed mixing sessions where some keys were allocated, but not used due to a timeout or disconnection.


/// Standard gap limit for special purpose keys (identity, provider keys)
pub const DEFAULT_SPECIAL_GAP_LIMIT: u32 = 20;

@QuantumExplorer QuantumExplorer Oct 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should probably have this less.

@QuantumExplorer QuantumExplorer Oct 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The benefit to having high gap limits is significantly lower with bip157/158 compared to bloom filters.

@HashEngineering HashEngineering Oct 6, 2025

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.

Agreed, these could be 5.

/// Maximum gap limit to prevent excessive address generation
pub const MAX_GAP_LIMIT: u32 = 1000;

Expand Down
82 changes: 63 additions & 19 deletions key-wallet/src/managed_account/managed_account_collection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
//! across different networks in a hierarchical manner.

use crate::account::account_type::AccountType;
use crate::gap_limit::{
DEFAULT_COINJOIN_GAP_LIMIT, DEFAULT_EXTERNAL_GAP_LIMIT, DEFAULT_INTERNAL_GAP_LIMIT,
DEFAULT_SPECIAL_GAP_LIMIT,
};
use crate::managed_account::address_pool::{AddressPool, AddressPoolType};
use crate::managed_account::managed_account_type::ManagedAccountType;
use crate::managed_account::ManagedAccount;
Expand Down Expand Up @@ -329,7 +333,7 @@ impl ManagedAccountCollection {
let external_pool = AddressPool::new(
external_path,
AddressPoolType::External,
20,
DEFAULT_EXTERNAL_GAP_LIMIT,
network,
key_source,
)?;
Expand All @@ -339,7 +343,7 @@ impl ManagedAccountCollection {
let internal_pool = AddressPool::new(
internal_path,
AddressPoolType::Internal,
20,
DEFAULT_INTERNAL_GAP_LIMIT,
network,
key_source,
)?;
Expand All @@ -356,61 +360,101 @@ impl ManagedAccountCollection {
AccountType::CoinJoin {
index,
} => {
let addresses =
AddressPool::new(base_path, AddressPoolType::Absent, 20, network, key_source)?;
let addresses = AddressPool::new(
base_path,
AddressPoolType::Absent,
DEFAULT_COINJOIN_GAP_LIMIT,
network,
key_source,
)?;
ManagedAccountType::CoinJoin {
index,
addresses,
}
}
AccountType::IdentityRegistration => {
let addresses =
AddressPool::new(base_path, AddressPoolType::Absent, 20, network, key_source)?;
let addresses = AddressPool::new(
base_path,
AddressPoolType::Absent,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;
ManagedAccountType::IdentityRegistration {
addresses,
}
}
AccountType::IdentityTopUp {
registration_index,
} => {
let addresses =
AddressPool::new(base_path, AddressPoolType::Absent, 20, network, key_source)?;
let addresses = AddressPool::new(
base_path,
AddressPoolType::Absent,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;
ManagedAccountType::IdentityTopUp {
registration_index,
addresses,
}
}
AccountType::IdentityTopUpNotBoundToIdentity => {
let addresses =
AddressPool::new(base_path, AddressPoolType::Absent, 20, network, key_source)?;
let addresses = AddressPool::new(
base_path,
AddressPoolType::Absent,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;
ManagedAccountType::IdentityTopUpNotBoundToIdentity {
addresses,
}
}
AccountType::IdentityInvitation => {
let addresses =
AddressPool::new(base_path, AddressPoolType::Absent, 20, network, key_source)?;
let addresses = AddressPool::new(
base_path,
AddressPoolType::Absent,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;
ManagedAccountType::IdentityInvitation {
addresses,
}
}
AccountType::ProviderVotingKeys => {
let addresses =
AddressPool::new(base_path, AddressPoolType::Absent, 20, network, key_source)?;
let addresses = AddressPool::new(
base_path,
AddressPoolType::Absent,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;
ManagedAccountType::ProviderVotingKeys {
addresses,
}
}
AccountType::ProviderOwnerKeys => {
let addresses =
AddressPool::new(base_path, AddressPoolType::Absent, 20, network, key_source)?;
let addresses = AddressPool::new(
base_path,
AddressPoolType::Absent,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;
ManagedAccountType::ProviderOwnerKeys {
addresses,
}
}
AccountType::ProviderOperatorKeys => {
let addresses =
AddressPool::new(base_path, AddressPoolType::Absent, 20, network, key_source)?;
let addresses = AddressPool::new(
base_path,
AddressPoolType::Absent,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;
ManagedAccountType::ProviderOperatorKeys {
addresses,
}
Expand All @@ -419,7 +463,7 @@ impl ManagedAccountCollection {
let addresses = AddressPool::new(
base_path,
AddressPoolType::AbsentHardened,
20,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;
Expand Down
83 changes: 64 additions & 19 deletions key-wallet/src/managed_account/managed_account_type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
use crate::account::StandardAccountType;
use crate::gap_limit::{
DEFAULT_COINJOIN_GAP_LIMIT, DEFAULT_EXTERNAL_GAP_LIMIT, DEFAULT_INTERNAL_GAP_LIMIT,
DEFAULT_SPECIAL_GAP_LIMIT,
};

use crate::{AccountType, AddressPool, DerivationPath};
#[cfg(feature = "bincode")]
use bincode_derive::{Decode, Encode};
Expand Down Expand Up @@ -356,7 +361,7 @@ impl ManagedAccountType {
let external_pool = AddressPool::new(
external_path,
AddressPoolType::External,
20,
DEFAULT_EXTERNAL_GAP_LIMIT,
network,
key_source,
)?;
Expand All @@ -366,7 +371,7 @@ impl ManagedAccountType {
let internal_pool = AddressPool::new(
internal_path,
AddressPoolType::Internal,
20,
DEFAULT_INTERNAL_GAP_LIMIT,
network,
key_source,
)?;
Expand All @@ -384,8 +389,13 @@ impl ManagedAccountType {
let path = account_type
.derivation_path(network)
.unwrap_or_else(|_| DerivationPath::master());
let pool =
AddressPool::new(path, AddressPoolType::Absent, 20, network, key_source)?;
let pool = AddressPool::new(
path,
AddressPoolType::Absent,
DEFAULT_COINJOIN_GAP_LIMIT,
network,
key_source,
)?;

Ok(Self::CoinJoin {
index,
Expand All @@ -396,8 +406,13 @@ impl ManagedAccountType {
let path = account_type
.derivation_path(network)
.unwrap_or_else(|_| DerivationPath::master());
let pool =
AddressPool::new(path, AddressPoolType::Absent, 20, network, key_source)?;
let pool = AddressPool::new(
path,
AddressPoolType::Absent,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;

Ok(Self::IdentityRegistration {
addresses: pool,
Expand All @@ -409,8 +424,13 @@ impl ManagedAccountType {
let path = account_type
.derivation_path(network)
.unwrap_or_else(|_| DerivationPath::master());
let pool =
AddressPool::new(path, AddressPoolType::Absent, 20, network, key_source)?;
let pool = AddressPool::new(
path,
AddressPoolType::Absent,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;

Ok(Self::IdentityTopUp {
registration_index,
Expand All @@ -421,8 +441,13 @@ impl ManagedAccountType {
let path = account_type
.derivation_path(network)
.unwrap_or_else(|_| DerivationPath::master());
let pool =
AddressPool::new(path, AddressPoolType::Absent, 20, network, key_source)?;
let pool = AddressPool::new(
path,
AddressPoolType::Absent,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;

Ok(Self::IdentityTopUpNotBoundToIdentity {
addresses: pool,
Expand All @@ -432,8 +457,13 @@ impl ManagedAccountType {
let path = account_type
.derivation_path(network)
.unwrap_or_else(|_| DerivationPath::master());
let pool =
AddressPool::new(path, AddressPoolType::Absent, 20, network, key_source)?;
let pool = AddressPool::new(
path,
AddressPoolType::Absent,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;

Ok(Self::IdentityInvitation {
addresses: pool,
Expand All @@ -443,8 +473,13 @@ impl ManagedAccountType {
let path = account_type
.derivation_path(network)
.unwrap_or_else(|_| DerivationPath::master());
let pool =
AddressPool::new(path, AddressPoolType::Absent, 20, network, key_source)?;
let pool = AddressPool::new(
path,
AddressPoolType::Absent,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;

Ok(Self::ProviderVotingKeys {
addresses: pool,
Expand All @@ -454,8 +489,13 @@ impl ManagedAccountType {
let path = account_type
.derivation_path(network)
.unwrap_or_else(|_| DerivationPath::master());
let pool =
AddressPool::new(path, AddressPoolType::Absent, 20, network, key_source)?;
let pool = AddressPool::new(
path,
AddressPoolType::Absent,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;

Ok(Self::ProviderOwnerKeys {
addresses: pool,
Expand All @@ -465,8 +505,13 @@ impl ManagedAccountType {
let path = account_type
.derivation_path(network)
.unwrap_or_else(|_| DerivationPath::master());
let pool =
AddressPool::new(path, AddressPoolType::Absent, 20, network, key_source)?;
let pool = AddressPool::new(
path,
AddressPoolType::Absent,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;

Ok(Self::ProviderOperatorKeys {
addresses: pool,
Expand All @@ -479,7 +524,7 @@ impl ManagedAccountType {
let pool = AddressPool::new(
path,
AddressPoolType::AbsentHardened,
20,
DEFAULT_SPECIAL_GAP_LIMIT,
network,
key_source,
)?;
Expand Down
4 changes: 2 additions & 2 deletions key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,9 +306,9 @@ fn test_transaction_affects_multiple_accounts() {
};
wallet.add_account(account_type, network, None).expect("Failed to add account to wallet");

// Add a BIP32 account
// Add another BIP32 account

@QuantumExplorer QuantumExplorer Oct 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the first one was BIP44

@HashEngineering HashEngineering Oct 7, 2025

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.

To maintain compatibility with old wallets, BIP32 was added to Default. That is why these tests were updated.

However, if we wanted Default to not have BIP32 (for new wallets), but have a another enum item that does contain BIP32 for restoring a wallet (as in the test wallet we are using), then this test could be returned to its original code. And I could add a RestoreDefault item for the case of restoring a wallet.

let account_type = AccountType::Standard {
index: 0,
index: 1,

@QuantumExplorer QuantumExplorer Oct 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

okay, but I don't think this matters.

@HashEngineering HashEngineering Oct 7, 2025

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.

It matters because the Default wallet already contains a BIP32 account 0, based on my changes. See above comment.

standard_account_type: StandardAccountType::BIP32Account,
};
wallet.add_account(account_type, network, None).expect("Failed to add account to wallet");
Expand Down
20 changes: 20 additions & 0 deletions key-wallet/src/wallet/helper.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,16 @@ impl Wallet {
}
match options {
WalletAccountCreationOptions::Default => {
// Create default BIP32 account 0
self.add_account(
AccountType::Standard {
index: 0,
standard_account_type: StandardAccountType::BIP32Account,
},
network,
None,
)?;
Comment thread
HashEngineering marked this conversation as resolved.

// Create default BIP44 account 0
self.add_account(
AccountType::Standard {
Expand Down Expand Up @@ -347,6 +357,16 @@ impl Wallet {
}
match options {
WalletAccountCreationOptions::Default => {
// Create default BIP32 account 0
self.add_account_with_passphrase(
AccountType::Standard {
index: 0,
standard_account_type: StandardAccountType::BIP32Account,
},
network,
passphrase,
)?;

// Create default BIP44 account 0
self.add_account_with_passphrase(
AccountType::Standard {
Expand Down
Loading
Loading