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

2019 02 14 fix digital signature bug ln invoice #337

Conversation

Christewart
Copy link
Contributor

@Christewart Christewart commented Feb 15, 2019

This fixes #297

The root problem was making sure we were dealing with unsigned numbers inside of BigInt when reading a reading a ECDigitalSignature from a raw r,s value. Depending on how large the number was, previously it could be interpreted as a negative number which would produce an invalid digital signature when reading the invoice from another source.

https://github.com/bitcoin-s/bitcoin-s-core/pull/337/files#diff-ce7c7a075ef8c7f783017f3a133b7cd7R154

This PR also adds more tests for LnUtil, LnInvoiceSignature, and LnInvoice.

@Christewart Christewart force-pushed the 2019-02-14-fix-digital-signature-bug-ln-invoice branch from fdb1e58 to 6b8074e Compare February 15, 2019 18:07
@Christewart Christewart requested review from nkohen and torkelrogstad and removed request for nkohen February 15, 2019 18:08
Copy link
Collaborator

@nkohen nkohen left a comment

Choose a reason for hiding this comment

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

I'm not going to pretend I really know what's going on here but assuming tests pass I didn't see any code things that looked wrong


val u8sAgain = Bech32.from5bitTo8bit(u5s)

u8s == u8sAgain
Copy link
Contributor Author

Choose a reason for hiding this comment

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

assert(u8s == u8sAgain)

@@ -58,11 +58,6 @@ class LnCurrencyUnitSpec extends Properties("LnCurrencyUnitSpec") {
else Try(num1 * num2).isFailure
}

property("Convert negative LnCurrencyUnit value to Satoshis") =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed this because it doesn't really make sense to have a negative ln currency unit

@@ -99,7 +99,7 @@ sealed abstract class LnInvoice {
override def toString: String = {
val b = new StringBuilder
b.append(hrp.toString)
b.append(Bech32.separator)
b.append('1')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why i had to use the hard coded 1 but the initilization problem persisted if I didn't add it. When invoices were being serialized in some cases the separator would be missing

@@ -16,19 +17,19 @@ trait LnUtil {
* [[https://github.com/lightningnetwork/lightning-rfc/blob/master/11-payment-encoding.md#examples Bolt11]]
* for examples.
*/
def createDataLength(bech32String: String): Vector[UInt5] = {
def createDataLength(bech32String: String): List[UInt5] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most changes in this file are because List has way better performance for construction and destruction of collections one by one.

http://www.lihaoyi.com/post/BenchmarkingScalaCollections.html#lists-vs-vectors

Constructing a List item-by-item is 5-15x (!) faster than constructing a Vector item-by-item.

De-constructing a List item-by-item using .tail is 50-60x (!) faster than de-constructing a Vector item-by-item using .tail

val n = BigInt(32).pow(vector.size - 1)
val newAccum = vector.head.toBigInt * n + accum
decodeNumber(vector.tail, newAccum.toLong)
val b32 = BigInt(32)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be moved back inside the h :: t case otherwise it is pointless construction every time we decode

@Christewart Christewart force-pushed the 2019-02-14-fix-digital-signature-bug-ln-invoice branch from d3154f2 to 1e14474 Compare February 16, 2019 21:26
@Christewart Christewart merged commit 44ea4e1 into bitcoin-s:master Feb 16, 2019
Christewart added a commit that referenced this pull request May 1, 2021
* Fix bug of sign issue when serializing r,s ln invoice signatures, we weren't using the proper sign in some rare cases. We always want the BigInteger to be non-negative for r,s

* Fixing initialization problem with EmptyDigitalSignature

* revert default Deps.scala bitcoinsV to published version

* Address code review nits

* remove negative ln currency unit test case
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.

Invalid digital signature on invoice returned from eclair
2 participants