Conversation
c6e4e8c to
7f39373
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements key derivation hardening by restricting BIP-32 derivation paths for cryptocurrency apps to prevent unauthorized key access across different coins.
Changes:
- Restricts
HAVE_APPLICATION_FLAG_DERIVE_MASTERflag to only Bitcoin Legacy and Bitcoin Test Legacy apps - Adds BIP-44 path restrictions (
PATH_APP_LOAD_PARAMS) for each supported cryptocurrency - Updates the lib-app-bitcoin submodule to support the hardening changes
Reviewed changes
Copilot reviewed 4 out of 71 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib-app-bitcoin | Updates submodule commit to version supporting path hardening |
| Makefile | Adds path restrictions for all coins and limits master derivation flag to legacy Bitcoin apps |
| CHANGELOG.md | Documents the derivation path hardening changes |
| .github/workflows/guidelines_enforcer.yml | Updates workflow to use branch-specific guidelines enforcer |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7f39373 to
82d85c6
Compare
|
A separate Claude Opus 4.6 analysis. Review: Derivation Path Hardening —
|
| Coin | BIP44_COIN_TYPE | BIP44_COIN_TYPE_2 | PATH restriction | SLIP-44 Match |
|---|---|---|---|---|
| Bitcoin Test Legacy | 1 | 1 | DERIVE_MASTER (unrestricted) |
✅ |
| Bitcoin Legacy | 0 | 0 | DERIVE_MASTER (unrestricted) |
✅ |
| Bitcoin Cash | 145 | 0 | */145' */0' 4541509' 45' |
✅ 145 |
| Bitcoin Gold | 156 | 0 | */156' */0' 4541509' 45' |
✅ 156 |
| Bitcoin Private | 183 | 0 | */183' */0' 4541509' 45' |
✅ 183 |
| Litecoin | 2 | 2 | */2' |
✅ 2 |
| Dogecoin | 3 | 3 | */3' |
✅ 3 |
| Dash | 5 | 5 | */5' |
✅ 5 |
| Peercoin | 6 | 6 | */6' |
✅ 6 |
| Viacoin | 14 | 14 | */14' |
✅ 14 |
| Digibyte | 20 | 20 | */20' |
✅ 20 |
| Vertcoin | 28 | 128 | */28' */128' |
✅ 28 (128 historical) |
| GameCredits | 101 | 101 | */101' |
✅ 101 |
| PivX | 119 | 77 | */119' */77' |
✅ 119 (77 historical) |
| Horizen | 121 | 121 | */121' |
✅ 121 |
| Firo | 136 | 136 | */136' |
✅ 136 |
| LBRY | 140 | 140 | */140' |
✅ 140 |
| Komodo | 141 | 141 | */141' |
✅ 141 |
| ZClassic | 147 | 147 | */147' |
✅ 147 |
| Ravencoin | 175 | 175 | */175' |
✅ 175 |
| Resistance | 356 | 356 | */356' |
✅ 356 |
| NIX | 400 | 400 | */400' |
✅ 400 |
| Stratis | 105105 | 105105 | */105105' |
✅ 105105 |
| xRhodium | 10291 | 10291 | */10291' |
✅ 10291 |
| Hydra Test | 0 | 0 | 44'/609' |
✅ 609 |
| Hydra | 0 | 0 | 44'/609' |
✅ 609 |
Correctness Analysis
✅ Correct and well-done:
-
Every coin has either
DERIVE_MASTERorPATH_APP_LOAD_PARAMS— no coin is left without any derivation capability. -
Wildcard
*/coin_type'syntax is properly used for most coins, allowing BIP-44 (44'), BIP-49 (49'), BIP-84 (84'), and BIP-86 (86') purposes. -
Dual coin_type support for historical compatibility:
- PivX:
*/119'+*/77'(77 was used before SLIP-44 registration) - Vertcoin:
*/28'+*/128'(128 was used before SLIP-44 registration) - BCH/BTG/BTCP: include
*/0'for Bitcoin-derived legacy paths
- PivX:
-
Bitcoin fork extra paths are justified:
45'= BIP-45 purpose for P2SH multisig wallets (Copay compatibility)4541509'=0x454C45= ASCII "ELE" — Electron Cash / Electrum ecosystem historical path
-
Deprecated coins properly removed (Zcash, Qtum were already
$(error)stubs).
⚠️ Points to consider (not necessarily bugs):
-
Hydra uses fixed
44'/609'instead of wildcard*/609': This means Hydra is restricted to BIP-44 purpose only, despite havingFLAG_SEGWIT_CHANGE_SUPPORT. If Hydra ever uses BIP-49/84/86 SegWit paths, this would need to change to*/609'. However, this matches the existing behavior ondevelop(it was--path "44'/609'"before), so it's not a regression. -
Hydra's
BIP44_COIN_TYPE=0vs path44'/609': Both Hydra and Hydra Test defineBIP44_COIN_TYPE=0andBIP44_COIN_TYPE_2=0, but the path restriction uses609'. TheBIP44_COIN_TYPEcompile-time value0will be available to the C code, which could cause confusion if the code uses it to construct paths internally. This was pre-existing behavior — not introduced by this branch — but is worth noting. -
Bitcoin Legacy as "recovery tool": The tagline "This is a recovery tool. Not for everyday use!" and the retention of
DERIVE_MASTERis appropriate since this is the legacy app that needs unrestricted access for recovery scenarios. This is clearly communicated to the user via the UI.
Verdict
The changes are correct and standards-compliant. All SLIP-44 coin types are accurate, dual coin types for backward compatibility are properly included, Bitcoin fork special paths (BIP-45 and Electron Cash) are justified, and no coin is left without path enforcement. The only minor items are the pre-existing Hydra BIP44_COIN_TYPE=0 mismatch and its fixed-purpose path, neither of which are regressions.
…itcoin_testnet_legacy variants
8cd1dda
82d85c6 to
8cd1dda
Compare
Checklist
=> corresponds to the documentation from the product:
developlib-app-bitcoin@main