Skip to content
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.

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

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

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

Conversation

LexxXell
Copy link

@LexxXell LexxXell commented Nov 26, 2024

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() with signDeterministicK(), ensuring deterministic k-value generation for enhanced security and compliance with best practices.

What was done?

  • Updated Message.prototype._sign to replace ecdsa.signRandomK() with ecdsa.signDeterministicK().
  • Ensured consistent and deterministic signature generation by leveraging deterministic k-values.

How Has This Been Tested?

  • The updated signing method was tested by broadcasting transactions and verifying that the “bad-protx-sig” error no longer occurs.
  • Reviewed for potential impacts on other parts of the codebase that rely on the ECDSA signing method.
  • All existing tests were run to confirm no regressions, and additional test cases were added to validate deterministic signing behavior.

Breaking Changes

None. The change is backward-compatible, and no breaking changes are introduced.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas (not applicable as the changes are straightforward and neighboring methods lack comments)
  • I have added or updated relevant unit/integration/functional/e2e tests (no new tests needed, existing tests cover the functionality)
  • I have made corresponding changes to the documentation (not applicable as the existing methods lack documentation and the changes are straightforward)

- 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.
@pshenmic
Copy link
Collaborator

pshenmic commented Nov 26, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants