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

wallet: always create signatures with low r-value #3220

Merged
merged 1 commit into from Oct 29, 2019

Conversation

@gorazdko
Copy link
Contributor

gorazdko commented Oct 28, 2019

Close #3210

  • in contrast to Core I didnt include bool grind as a function argument. I can do that too if wanted - calling sign_hash() would require that arg everywhere, since C doesnt support default arguments.
Copy link
Collaborator

darosior left a comment

Looks good (didn't tested for now though), but you should definitely check out https://github.com/ElementsProject/lightning/blob/master/doc/HACKING.md and https://github.com/ElementsProject/lightning/blob/master/doc/STYLE.md 😉

@@ -75,16 +75,40 @@ static void dump_tx(const char *msg UNUSED,
}
#endif

/* Taken from bitcoin/src/key.cpp:
// Copyright (c) 2009-2018 The Bitcoin Core developers
// Copyright (c) 2017 The Zcash developers

This comment has been minimized.

Copy link
@darosior

darosior Oct 28, 2019

Collaborator

Hmmm

// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
*/
// Check that the sig has a low R value and will be less than 71 bytes

This comment has been minimized.

Copy link
@darosior

darosior Oct 28, 2019

Collaborator

We use /* .... */ comments ;-)

// file COPYING or http://www.opensource.org/licenses/mit-license.php.
*/
// Check that the sig has a low R value and will be less than 71 bytes
static bool SigHasLowR(const secp256k1_ecdsa_signature* sig)

This comment has been minimized.

Copy link
@darosior

darosior Oct 28, 2019

Collaborator

And snake case

@gorazdko gorazdko force-pushed the gorazdko:grindsignatures branch from 4108356 to 7d37021 Oct 29, 2019
@gorazdko

This comment has been minimized.

Copy link
Contributor Author

gorazdko commented Oct 29, 2019

force pushed stylistic corrections

@gorazdko gorazdko force-pushed the gorazdko:grindsignatures branch from 7d37021 to b90b935 Oct 29, 2019
@niftynei

This comment has been minimized.

Copy link
Collaborator

niftynei commented Oct 29, 2019

ACK b90b935

s,
h->sha.u.u8,
privkey->secret.data, NULL, NULL);
unsigned char extra_entropy[32] = {0};

This comment has been minimized.

Copy link
@niftynei

niftynei Oct 29, 2019

Collaborator

pardon my ignorance, but this looks like you're only initializing the first element of a 32-size array. what is the rest of the array set to? do we need to initialize the rest of the array?

This comment has been minimized.

Copy link
@gorazdko

gorazdko Oct 29, 2019

Author Contributor

unsigned char extra_entropy[32] = {0}; initializes all elements of the array to zero

@gorazdko

This comment has been minimized.

Copy link
Contributor Author

gorazdko commented Oct 29, 2019

here's also a couple of my logs from testing, where you can see we make 2 iterations on average:

b'***************[sign_hash]**************'
b'i: 0'
b'extra_entropy: 0'
b'compact_sig[0]: b3'
b'i: 1'
b'extra_entropy: 1'
b'compact_sig[0]: 78'
b''
b''
b'***************[sign_hash]**************'
b'i: 0'
b'extra_entropy: 0'
b'compact_sig[0]: 58'
b''
b''
b'***************[sign_hash]**************'
b'i: 0'
b'extra_entropy: 0'
b'compact_sig[0]: 5d'
b''
b''
b'***************[sign_hash]**************'
b'i: 0'
b'extra_entropy: 0'
b'compact_sig[0]: a8'
b'i: 1'
b'extra_entropy: 1'
b'compact_sig[0]: ff'
b'i: 2'
b'extra_entropy: 2'
b'compact_sig[0]: 59'
b''
b''
b'***************[sign_hash]**************'
b'i: 0'
b'extra_entropy: 0'
b'compact_sig[0]: ea'
b'i: 1'
b'extra_entropy: 1'
b'compact_sig[0]: 13'
b''
b''
b'***************[sign_hash]**************'
b'i: 0'
b'extra_entropy: 0'
b'compact_sig[0]: bf'
b'i: 1'
b'extra_entropy: 1'
b'compact_sig[0]: 78'
b''
b''
@niftynei niftynei merged commit a3851f2 into ElementsProject:master Oct 29, 2019
3 checks passed
3 checks passed
bitcoin-bot/acks Acks by niftynei
bitcoin-bot/fixups PR does not contain unsquashed fixups
continuous-integration/travis-ci/pr The Travis CI build passed
Details
s,
h->sha.u.u8,
privkey->secret.data, NULL, extra_entropy);
((u32 *)extra_entropy)[0]++;

This comment has been minimized.

Copy link
@ysangkok

ysangkok Nov 6, 2019

what's the [0] for? can't the u32 be incremented directly? will this only ever modify the first byte of the u32?

This comment has been minimized.

Copy link
@gorazdko

gorazdko Nov 6, 2019

Author Contributor

it means we are changing the first 32 bits of extra_entropy. This matches the behavior in Core. The other option would be (*((u32 *)extra_entropy))++

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.