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

Produce exclusively low-R signatures from wallet keys #7238

Merged

Conversation

stejbac
Copy link
Contributor

@stejbac stejbac commented Sep 5, 2024

Implement low-R nonce grinding with the class LowRSigningKey extending org.bitcoinj.core.ECKey, which always produces low-R signatures, consistent with the behaviour of Bitcoin Core/Knots (post-2018), Sparrow, Coldcard and possibly other wallets with recent updates, for hopefully improved privacy and slightly lower and more predictable tx fees. Canonical DER-encoded signatures are usually either 71 or 70 bytes (starting with hex 3045 or 3044 resp.), with roughly 50-50 odds, depending on whether they are high-R or low-R. (Less than 1% of the time, they will be shorter than 70 bytes, because of the variable length bigint R & S encodings.) So trying different nonces for low-R saves half-a-byte on average, at the cost of doubling the average signing time.

To this end, provide the class CountingHMacDSAKCalculator to supply a custom nonce to the Bouncy Castle class ECDSASigner. The first invocation of the k-calculator instance matches the output of the RFC 6979 compliant Bouncy Castle version, but subsequent invocations increment an internal counter supplied as additional data/entropy to the HMAC, as mentioned in section 3.6 of the RFC, until a low-R signature results. In this way, a deterministic signing algorithm exactly matching that of (post-2018 versions of) Bitcoin Core results.

Also add unit tests, with test vectors taken from the RFC (which only covers the NIST curves, unfortunately, not secp256k1), and test vectors generated from signed txs created by the bitcoin-tx command.

--

This PR should make all tx signatures produced by Bisq low-R, which could save maybe a few satoshis in fees per tx on average (or more during congestion), though none of the fee calculation logic has been changed. Other uses of ECKey have been changed to produce low-R signatures as well, such as on merit lists for blind votes and proofs of burn.

While Bitcoin Core. Knots, Sparrow and Coldcard produce low-R signatures, I couldn't find any indication in the firmware source code that Trezor currently does, and LND's on-chain wallet still seems to produce high-R signatures in my version (17.3), so there are still obviously quite a few wallets producing high-R signatures. Nevertheless, I expect that there would be an increase in the anonymity set and hence privacy by moving to low-R signatures. However, most of these wallets also seem to produce version 2 txs by default, whereas bitcoinj still produces version 1 txs by default, so perhaps a separate PR could be made to move all or most of Bisq's txs to version 2, to ensure a real improvement in privacy.

--

It might have been simpler to change the signing algorithm in our fork of bitcoinj itself, and there is a draft PR from 2022 upstream to do that: bitcoinj/bitcoinj#2302. However, it probably couldn't be used as-is because it appears to be based on a different version of Bouncy Castle and currently breaks the bitcoinj tests.

Also, part of the motivation behind this PR is to later add the capacity to hide private data in signature nonces and support non-deterministically random pre-chosen nonces, through further custom k-calculators and signing methods added to the class LowRSigningKey. This is to enable the emergency recovery of the redirect tx from a user's seed phrase for the v5 trade protocol, in the event that the peer publishes the warning tx but the user has lost all his trade data. I have already developed and tested (on regtest) such backup & recovery logic, in the personal branch: https://github.com/stejbac/bisq/tree/v5-trade-protocol-redirect-tx-recovery.

Implement low-R nonce grinding with the class 'LowRSigningKey', which
always produces low-R signatures, consistent with the behaviour of
Bitcoin Core/Knots (post-2018), Sparrow, Coldcard and possibly other
wallets with recent updates, for hopefully improved privacy and slightly
lower and more predictable tx fees. Canonical DER-encoded signatures are
usually either 71 or 70 bytes (starting with hex 3045 or 3044 resp.),
with roughly 50-50 odds, depending on whether they are high-R or low-R.
(Less than 1% of the time, they will be shorter than 70 bytes, because
of the variable length bigint R & S encodings.) So trying different
nonces for low-R saves half-a-byte on average, at the cost of doubling
the average signing time.

To this end, provide the class 'CountingHMacDSAKCalculator' to supply a
custom nonce to 'o.b.c.s.ECDSASigner'. The first invocation of the
k-calculator instance matches the output of the RFC 6979 compliant
Bouncy Castle version, but subsequent invocations increment an internal
counter supplied as additional data/entropy to the HMAC, as mentioned in
section 3.6 of the RFC, until a low-R signature results. In this way, a
deterministic signing algorithm exactly matching that of (post-2018
versions of) Bitcoin Core results.

Also add unit tests, with test vectors taken from the RFC (which only
covers the NIST curves, unfortunately, not secp256k1), and test vectors
generated from signed txs created by the 'bitcoin-tx' command.
Provide a 'BisqWallet' subclass of 'o.b.w.Wallet', in order to override
the 'signTransaction' method used internally by the bitcoinj SendRequest
API, so that it always produces txs with low-R signatures.

Also modify 'WalletService.signTransactionInput' to likewise wrap wallet
keys, so that the provided tx input is given a low-R signature.

Finally, modify the manual signing logic in TradeWalletService to wrap
multisig & deposit input keys, which should cover all tx signing in the
Bisq application.
To slightly save storage & bandwidth, create low-R signatures in places
besides tx ScriptSigs & witnesses, such as merit lists, proofs of burn,
filter, alert & accounting authentication data.

(Also, to make setup of mock keys a little easier, bypass wrapping of
keys that are already instances of 'LowRSigningKey' in the factory
method, 'LowRSigningKey.from(ECKey)'.)
Copy link
Collaborator

@HenrikJannsen HenrikJannsen left a comment

Choose a reason for hiding this comment

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

utACK

@alejandrogarcia83 alejandrogarcia83 merged commit 7dab669 into bisq-network:master Sep 8, 2024
3 checks passed
@alejandrogarcia83 alejandrogarcia83 added this to the v1.9.18 milestone Sep 8, 2024
@ChrisSon15
Copy link

The Length of ECDSA Signatures seems to be 63% with low-r and 36% with high-r. So the privacy gain is in that range. Right now in 50% of cases bisq produces a high-r, so the gains for fess will be half a byte on average. If I understand correcly, you are changing the signature code for every signature in bisq not only backup. Given that bisq1 is being replaced some time soon, are there other gains I overlooked for introducing low-r values?

@stejbac
Copy link
Contributor Author

stejbac commented Sep 10, 2024

If 36% of ECDSA signatures are high-R, that would mean 72% are still being created without nonce grinding, which is a majority. So there may not actually be a privacy advantage at present and instead a small reduction in privacy, which is disappointing. I wonder which popular wallets are currently producing the bulk of the high-R signatures. It's also interesting that the percentage of low-R signatures is trending downwards in the last couple of years, in the linked graph.

Every new signature created from the wallet keys, not just signatures on the transactions, should be made low-R by this PR. Other signatures not created via bitcoinj, such as the DSA signatures on trade contracts or signed witnesses, are unaffected.

Aside from the small reduction in tx size and possibility for easier fee estimation (no need to overestimate), there aren't any other direct gains I can think of. But I needed custom nonce generation for the redirect tx recovery signature-data-hiding approach that I took for the v5 protocol, and I guessed nonce grinding was the right thing to do for that, given that's what Sparrow & Core (& apparently Electrum) are currently doing with their signatures. But if some tx signatures generated by Bisq are exclusively low-R, probably all of them should be.

stejbac added a commit to stejbac/bisq that referenced this pull request Sep 13, 2024
Prevent 'KeyIsEncryptedException' from being thrown when signing with a
'LowRSigningKey'-wrapped, encrypted HD key, due to breakage of the
apparent invariant that the 'keyCrypter' field of 'ECKey' should be null
whenever the key isn't encrypted.

When signing with a wrapped, encrypted HD key, the original key is
decrypted and then re-wrapped as a 'LowRSigningKey' instance. This was
blindly copying the 'keyCrypter' property of the decrypted key. But
'DeterministicKey::getKeyCrypter' returns non-null if its parent does,
even if the actual field is null, and the decrypted HD key has the same
parent as the encrypted original. Thus, blindly copying the property
(rather than the field) breaks the above invariant.

Fixes issue bisq-network#7241 with blind voting, caused by the earlier PR bisq-network#7238
which introduced low-R nonce grinding.
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.

4 participants