Add low R signing #1342
Merged
Add low R signing #1342
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benthecarman
requested changes
Apr 16, 2020
core-test/src/test/scala/org/bitcoins/core/crypto/SignTest.scala
Outdated
Show resolved
Hide resolved
benthecarman
approved these changes
Apr 16, 2020
benthecarman
approved these changes
Apr 16, 2020
There was a problem hiding this comment.
This looks good to me, however, to implement this into the wallet will be tricky with how it is. Currently when we get signers, they will use only use the signFunction and not the signLowR function.
Maybe it would be worth creating a LowRSign type that extends Sign and defaults to low r signing.
Christewart
reviewed
Jul 29, 2020
Christewart
approved these changes
Jul 29, 2020
Christewart
pushed a commit
that referenced
this pull request
May 1, 2021
* Added signing with entropy to the JNI and implemented low R signing * Added check for determinism of low R signing * Cleaned up test * Implemented signing with entropy in Bouncy Castle * Added docs for signing with entropy * Fixed things after rebase * De-parallelized signLowR and added scaladoc warnings to signWithEntropy
schildbach
pushed a commit
to schildbach/bitcoinj
that referenced
this pull request
Feb 26, 2025
The goal is to make transaction sizes more predictable, simplifying the fee calculation. This change will make sure signatures will always have DER encodings of 70 bytes or less, without counting the sighash flags byte. Note that one of our tests for a BIP143 native P2WPKH test vector had to be changed, because that vector had been produced pre-grinding and hasn't been updated since. However, the change only affects a P2PK input which isn't really the focus of the test vector. Also see similar changes in other projects: bitcoin/bitcoin#13666 rust-bitcoin/rust-secp256k1#259 bitcoin-s/bitcoin-s#1342 + bitcoin-s/bitcoin-s#2408 bisq-network/bisq#7238 TODO extract HMacDSAKCalculatorWithEntrophy to top level class? TODO make encodeToCompact() its own commit? TODO more test vectors from other projects TODO should sigs with DER encodings *less* than 70 bytes also be retried?
schildbach
pushed a commit
to schildbach/bitcoinj
that referenced
this pull request
Feb 26, 2025
The goal is to make transaction sizes more predictable, simplifying the fee calculation. This change will make sure signatures will always have DER encodings of 70 bytes or less, without counting the sighash flags byte. Note that one of our tests for a BIP143 native P2WPKH test vector had to be changed, because that vector had been produced pre-grinding and hasn't been updated since. However, the change only affects a P2PK input which isn't really the focus of the test vector. Also see similar changes in other projects: bitcoin/bitcoin#13666 rust-bitcoin/rust-secp256k1#259 bitcoin-s/bitcoin-s#1342 + bitcoin-s/bitcoin-s#2408 bisq-network/bisq#7238 TODO extract HMacDSAKCalculatorWithEntrophy to top level class? TODO make encodeToCompact() its own commit? TODO more test vectors from other projects TODO should sigs with DER encodings *less* than 70 bytes also be retried?
schildbach
pushed a commit
to schildbach/bitcoinj
that referenced
this pull request
Apr 24, 2025
The goal is to make transaction sizes more predictable, simplifying the fee calculation. This change will make sure signatures will always have DER encodings of 70 bytes or less, without counting the sighash flags byte. Note that one of our tests for a BIP143 native P2WPKH test vector had to be changed, because that vector had been produced pre-grinding and hasn't been updated since. However, the change only affects a P2PK input which isn't really the focus of the test vector. Also see similar changes in other projects: bitcoin/bitcoin#13666 rust-bitcoin/rust-secp256k1#259 bitcoin-s/bitcoin-s#1342 + bitcoin-s/bitcoin-s#2408 spesmilo/electrum#5820 bisq-network/bisq#7238 TODO extract HMacDSAKCalculatorWithEntrophy to top level class? TODO make encodeToCompact() its own commit? TODO more test vectors from other projects TODO should sigs with DER encodings *less* than 70 bytes also be retried?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduces signing with entropy to the JNI.
Also introduces and tests low R signing as was introduced to bitcoin-core here.
Closes #1312