Skip to content

refactor: use deterministic k-value for ECDSA signing to fix bad-protx-sig error #303

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

Open
LexxXell wants to merge 3 commits into
base: master
Choose a base branch
from
Open

refactor: use deterministic k-value for ECDSA signing to fix bad-protx-sig error #303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c5c63cc
feat: added deterministic signing method to ECDSA
LexxXell Nov 26, 2024
8ddb2fc
refactor: updated ECDSA signing to use deterministic k-value
LexxXell Nov 26, 2024
3734337
Updated package version
LexxXell Nov 26, 2024
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
5 changes: 5 additions & 0 deletions lib/crypto/ecdsa.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,11 @@ ECDSA.prototype.sign = function () {
return this;
};

ECDSA.prototype.signDeterminicticK = function () {
this.deterministicK();
return this.sign();
}
Comment on lines +279 to +282

@thepastaclaw thepastaclaw May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 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).

Suggested change
ECDSA.prototype.signDeterminicticK = function () {
this.deterministicK();
return this.sign();
}
ECDSA.prototype.signDeterministicK = function () {
this.deterministicK();
return this.sign();
};

source: ['claude', 'codex']

Comment on lines +279 to +282

@thepastaclaw thepastaclaw May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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']

@thepastaclaw thepastaclaw May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💬 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.

Suggested change
}
/**
* Signs `this.hashbuf` using a deterministic k value (RFC 6979).
* @returns {ECDSA}
*/
ECDSA.prototype.signDeterministicK = function () {
this.deterministicK();
return this.sign();
};

source: ['claude']


ECDSA.prototype.signRandomK = function () {
this.randomK();
return this.sign();
Expand Down
2 changes: 1 addition & 1 deletion lib/message.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ Message.prototype._sign = function _sign(privateKey) {
ecdsa.hashbuf = hash;
ecdsa.privkey = privateKey;
ecdsa.pubkey = privateKey.toPublicKey();
ecdsa.signRandomK();
ecdsa.signDeterminicticK();

@thepastaclaw thepastaclaw May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 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.

Suggested change
ecdsa.signDeterminicticK();
ecdsa.signDeterministicK();

source: ['claude']

ecdsa.calci();
return ecdsa.sig;
};
Comment on lines 65 to 71

@thepastaclaw thepastaclaw May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 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']

Comment on lines 65 to 71

@thepastaclaw thepastaclaw May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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']

Expand Down
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@dashevo/dashcore-lib",
"version": "0.22.0",
"version": "0.22.1",
"description": "A pure and powerful JavaScript Dash library.",
"author": "Dash Core Group, Inc. <dev@dash.org>",
"main": "index.js",
Expand Down