Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPackage.json dependency versions are updated for bitcore-lib and bitcore-mnemonic from 8.25.10 to 10.10.7. No changes to scripts, devDependencies, or other configuration fields. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1033 +/- ##
=======================================
Coverage 87.82% 87.82%
=======================================
Files 114 114
Lines 8818 8818
Branches 2004 1994 -10
=======================================
Hits 7744 7744
Misses 1046 1046
Partials 28 28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Future consideration: removing the bitcore-lib dependency entirelyThis upgrade fixes the immediate bug, but it's worth documenting the long-term options for the team. Bitcore-lib is a heavy, monolithic dependency with some concerning characteristics. Here's a quick overview. Why consider removing bitcore-lib?
The modern alternative:
|
| What we need | Current (bitcore) | Replacement | Audited? |
|---|---|---|---|
| ECDSA + secp256k1 | bitcore.crypto.ECDSA → elliptic |
@noble/secp256k1 |
Yes |
| SHA-256, HMAC, RIPEMD-160 | bitcore.crypto.Hash → Node crypto |
@noble/hashes |
Yes |
| HD key derivation (BIP32) | bitcore.HDPrivateKey / HDPublicKey |
@scure/bip32 |
Yes |
| Mnemonic phrases (BIP39) | bitcore-mnemonic |
@scure/bip39 |
Yes |
| Base58 / Base58Check | bitcore.encoding.Base58 → bs58 |
@scure/base |
Yes |
What's left would be ~300-400 lines of custom code for: Hathor Networks config, Address encoding, Message sign/verify, and Script.buildMultisigOut.
Pros of migrating
- Audited crypto —
@noble/*and@scure/*have published audit reports; bitcore-lib andellipticdo not - Native TypeScript — full type safety without
noImplicitAny: falseworkarounds - Smaller bundle —
@noble/secp256k1is 5KB vsellipticat ~90KB - No more patches — eliminates the
versionGuardhack - Active maintenance — noble/scure receive regular updates; bitcore-lib's last meaningful change was cosmetic
Cons / risks of migrating
deriveNonCompliantChild()— this is a bitcore-specific BIP32 extension that skips invalid derived keys instead of throwing.@scure/bip32does not implement this — we'd need to port ~30 lines and verify behavioral parityHDPrivateKey._buffers— tests access internal buffer structures for validation;@scure/bip32has a different internal representation- Address encoding — our custom Hathor address logic and version byte handling are tightly coupled to bitcore's
Networks.add()andAddressclass - Effort — estimated at weeks of work, plus extensive regression testing across all wallet operations
- Risk surface — subtle behavioral differences in edge cases (key derivation, signature encoding) could cause funds-related bugs that only manifest in production
Another option: inline the bitcore source
Instead of migrating to new libraries, we could copy the ~7,000 lines we actually use into the repo directly. This would:
- ✅ Remove the npm dependency and patch requirement
- ✅ Let us fix bugs (like #3860) directly
- ❌ Make us responsible for maintaining 7,000 lines of untyped legacy JS forever
- ❌ Still depend on
ellipticandbn.js@4.11.8underneath
This is probably the worst option unless we plan to immediately start replacing modules with @noble/* incrementally.
Recommendation
Short term (this PR): upgrade to 10.10.7, fix the bug, move on.
Medium term: if we decide to migrate, the @noble/* + @scure/* path is clearly the better long-term choice over inlining. A good incremental approach would be:
- Replace
crypto.Hashwith@noble/hashes(lowest risk, most isolated) - Replace
encoding.Base58with@scure/base - Replace
ECDSA+Signaturewith@noble/secp256k1 - Replace
HDPrivateKey/HDPublicKeywith@scure/bip32(highest risk, needs careful testing) - Replace
Mnemonicwith@scure/bip39 - Write custom
Address,Networks,Message(~300 lines) - Remove
bitcore-libandbitcore-mnemonic
Each step can be a separate PR with its own test validation.
|
I like this but there are other considerations for this update. I like the idea of not depending on bitcore, if not the The main important issue is We should create a plan to migrate this usage of Obs: 0.4% is aprox. |
Summary
bitcore-libandbitcore-mnemonicfrom 8.25.10 to 10.10.7 (latest stable)versionGuardpatch file to match the new version (patch content unchanged)The Bug
ECDSA.sign()intermittently producesrorssignature values shorter than 32 bytes because BN (big number) serialization strips leading zeros. WhenMessage.verify()later callsSignature.fromCompact(), it asserts exactly 32 bytes and throws:This is a ~1-in-256 chance per signature component, making it an intermittent failure that is hard to reproduce deterministically.
The fix (applied in v10.8.9) normalizes
randsthrough proper buffer round-tripping in thesign()function:Why 10.10.7 and not a smaller jump?
latesttag (v11.x isbeta), making it the safest stable target.var→const), JSDoc improvements, and a defensive guard in Point prototype patching.Breaking changes analysis
Every bitcore-lib API used by wallet-lib was verified against the v10 source. Summary:
ECDSA.sign(hash, privkey)andECDSA.verify(hash, sig, pubkey)still work identically — both return/acceptSignatureobjectsNetworks.add()networkMapsstores arrays instead of single valuesSchnorr,TaggedHashcrypto exportsisSchnorrsupportset({nhashtype}),toDER(),SIGHASH_ALLunchangedMessage.MAGIC_BYTESmonkey-patchAll 25+ API entry points used by wallet-lib (HDPrivateKey, HDPublicKey, Address, Script, Message, Mnemonic, encoding, util, etc.) were individually verified to exist with the same signatures in v10.10.7.
What changed in this PR
package.json— bumpedbitcore-libandbitcore-mnemonicto10.10.7patches/bitcore-lib+8.25.10.patch→patches/bitcore-lib+10.10.7.patch(renamed, same content —versionGuardbypass)package-lock.json— regeneratedNo source code changes were needed — all wallet-lib code is fully compatible with v10.10.7.
Summary by CodeRabbit