refactor: use deterministic k-value for ECDSA signing to fix bad-protx-sig error#303
refactor: use deterministic k-value for ECDSA signing to fix bad-protx-sig error#303LexxXell wants to merge 3 commits into
Conversation
- Introduced `ECDSA.prototype.signDeterministicK` method. - Ensures the use of deterministic k-value during the signing me process.
- Replaced `signRandomK()` with `signDeterministicK()` in `Message.prototype._sign`. - This fixes “bad-protx-sig” error on transaction broadcast. Also ensures consistent and deterministic signing for enhanced security and compliance with best practices.
|
We couldn't make a ProRegTx transaction in the JS for a quite time, because of bad-protx-sig error hapenning each time, and we couldn't understand, but Konstantin helped us with getting legit buffer / hash / signature data that made us find the mistake. It seems that function was ported from bitcore, but nobody really have gotten to use it. The Message class implement signing in the randomK mode, which returns you different signature result each time, while Core is working in the deterministic mode. Once we switched the modes, we have been able to correctly sign the payloadSig of ProRegTx and successfully broadcasted transaction in the testnet network. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two convergent blocking issues confirmed by both agents: (1) the new ECDSA method is misspelled signDeterminicticK (extra 'tic') in both the definition (lib/crypto/ecdsa.js:279) and its only caller (lib/message.js:68), baking a misspelled symbol into the public API surface and breaking the API contract described in the PR/commit message; (2) the PR's stated goal of fixing bad-protx-sig is not achieved — ProTx payload signing flows through AbstractPayload.sign → Signer.signHash → ecdsa.signRandomK() (lib/crypto/signer.js:33), which this PR does not touch. Additionally, no tests were added despite the PR description claiming otherwise, and the new function is missing a trailing semicolon. Recommending REQUEST_CHANGES.
🔴 3 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Migrate
Signer.signHash(and any othersignRandomKcall sites) to a deterministic-k path — After this PR, the only remainingsignRandomKcaller in the library is lib/crypto/signer.js:33, used byAbstractPayload.signfor all ProTx payload signing as well as generic data signing. The same threat model that motivates this PR (a biased or repeating RNG enabling nonce-reuse private-key recovery) applies equally to that path, and migrating it is what would actually address the reportedbad-protx-sigissue. Changing transaction-signing semantics affects downstream fixtures and consumers, so it warrants its own scoped change with regression tests.- Follow-up: Open a separate issue/PR to migrate
Signer.signHash(and audit any othersignRandomKcall sites) to a deterministic-k signing path, with fixture-pinned signature tests for known (privkey, sighash) pairs and a changelog note documenting the determinism change.
- Follow-up: Open a separate issue/PR to migrate
| ECDSA.prototype.signDeterminicticK = function () { | ||
| this.deterministicK(); | ||
| return this.sign(); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Method name is misspelled: signDeterminicticK (extra 'tic') exposed on public ECDSA API
ECDSA.prototype.signDeterminicticK contains an extra 'tic' — the intended name (per the PR title, commit message c5c63cc, and PR description) is signDeterministicK. ECDSA is exported as bitcore.crypto.ECDSA on the library's public surface, so this typo permanently misnames a public method. Any downstream caller following the PR description and invoking ecdsa.signDeterministicK() will receive TypeError: ecdsa.signDeterministicK is not a function and may fall back to ad-hoc nonce generation — re-introducing the very RNG-reuse risk this PR sets out to mitigate. The misspelling is also propagated to the only in-tree caller at lib/message.js:68, which is why local tests didn't catch it. The closing brace also lacks a trailing semicolon (inconsistent with the adjacent signRandomK definition on lines 284–287 and likely to fail npm run lint).
| ECDSA.prototype.signDeterminicticK = function () { | |
| this.deterministicK(); | |
| return this.sign(); | |
| } | |
| ECDSA.prototype.signDeterministicK = function () { | |
| this.deterministicK(); | |
| return this.sign(); | |
| }; |
source: ['claude', 'codex']
| ecdsa.privkey = privateKey; | ||
| ecdsa.pubkey = privateKey.toPublicKey(); | ||
| ecdsa.signRandomK(); | ||
| ecdsa.signDeterminicticK(); |
There was a problem hiding this comment.
🔴 Blocking: Caller in Message._sign uses the misspelled method name and must be updated in lockstep
lib/message.js:68 invokes ecdsa.signDeterminicticK(), matching the typo introduced in lib/crypto/ecdsa.js:279. When the prototype method is renamed to signDeterministicK (per the PR description), this call site must be updated at the same time or Message.prototype._sign will throw TypeError: ecdsa.signDeterministicK is not a function and break all message signing.
| ecdsa.signDeterminicticK(); | |
| ecdsa.signDeterministicK(); |
source: ['claude']
| ecdsa.hashbuf = hash; | ||
| ecdsa.privkey = privateKey; | ||
| ecdsa.pubkey = privateKey.toPublicKey(); | ||
| ecdsa.signRandomK(); | ||
| ecdsa.signDeterminicticK(); | ||
| ecdsa.calci(); | ||
| return ecdsa.sig; | ||
| }; |
There was a problem hiding this comment.
🔴 Blocking: Change does not address bad-protx-sig — wrong signing path is modified
The PR's stated purpose is to fix bad-protx-sig errors during transaction broadcasts. However, ProTx payload signing does not go through Message.prototype._sign — ProRegTx / ProUpServTx / ProUpRegTx / MnHfSignal etc. extend AbstractPayload, and AbstractPayload.prototype.sign (lib/transaction/payload/abstractpayload.js:54-60) delegates to Signer.signHash (lib/crypto/signer.js:25), which still calls ecdsa.signRandomK() at lib/crypto/signer.js:33. Message.prototype._sign is the BIP-322-style "Bitcoin Signed Message" path (uses magicHash() on line 63), unrelated to ProTx payload signatures. If the intent is to eliminate non-deterministic k from ProTx signing, the change needed is in lib/crypto/signer.js:33 (with fixture-pinned signature regression tests). As written, this PR cannot fix the broadcast failure described in the PR body, even if a downstream acceptance test happened to pass for unrelated reasons.
source: ['claude', 'codex']
| ECDSA.prototype.signDeterminicticK = function () { | ||
| this.deterministicK(); | ||
| return this.sign(); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: No tests added for the new deterministic signing method
The PR description states "additional test cases were added to validate deterministic signing behavior", but grep of test/ shows no coverage for signDeterminicticK / signDeterministicK; only the pre-existing #deterministicK tests in test/crypto/ecdsa.js exercise deterministicK() directly. The fact that the misspelling slipped through both the definition and its only call site is itself evidence that no test invokes the new method by name. Add at least one test that directly calls ecdsa.signDeterministicK() on a fixed (privKey, message) pair and asserts a stable signature — this both catches name regressions and pins deterministic behavior as a regression guard.
source: ['claude']
| ecdsa.hashbuf = hash; | ||
| ecdsa.privkey = privateKey; | ||
| ecdsa.pubkey = privateKey.toPublicKey(); | ||
| ecdsa.signRandomK(); | ||
| ecdsa.signDeterminicticK(); | ||
| ecdsa.calci(); | ||
| return ecdsa.sig; | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: Behavior change in Message.sign() is a soft breaking change worth calling out
Before this PR, Message.prototype.sign used random k, so two calls with the same (privKey, message) produced different (but equally valid) signatures. After this PR the signature is deterministic per RFC 6979. The PR description lists "None" under Breaking Changes, but downstream consumers that compare signatures byte-for-byte (test fixtures, cached signatures, signature-uniqueness assumptions) will observe a change. Deterministic signing is the right direction — but call it out in the changelog/release notes and pin the new expected signature for a known fixture so any future accidental regression to random k is caught.
source: ['claude']
| ECDSA.prototype.signDeterminicticK = function () { | ||
| this.deterministicK(); | ||
| return this.sign(); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Missing trailing semicolon and JSDoc on the new prototype method
Compared with the adjacent ECDSA.prototype.signRandomK (lines 284–287), the new function lacks a trailing ; after the closing brace and has no JSDoc. The repository has .eslintrc.json and runs eslint . for npm run lint; with Airbnb-base style semi is on by default, so this is likely to fail lint. Adding a short JSDoc would also be useful since the method is new and on the public API surface.
| } | |
| /** | |
| * Signs `this.hashbuf` using a deterministic k value (RFC 6979). | |
| * @returns {ECDSA} | |
| */ | |
| ECDSA.prototype.signDeterministicK = function () { | |
| this.deterministicK(); | |
| return this.sign(); | |
| }; |
source: ['claude']
3 participants
Issue being fixed or feature implemented
This change fixes the “bad-protx-sig” error that occurred during transaction broadcasts. It improves the signing process by replacing
signRandomK()withsignDeterministicK(), ensuring deterministic k-value generation for enhanced security and compliance with best practices.What was done?
Message.prototype._signto replaceecdsa.signRandomK()withecdsa.signDeterministicK().How Has This Been Tested?
Breaking Changes
None. The change is backward-compatible, and no breaking changes are introduced.
Checklist: