Change signature encoding to 64 bytes fixed on wire#86
Change signature encoding to 64 bytes fixed on wire#86Roasbeef merged 3 commits intolightningnetwork:masterfrom
Conversation
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
Thanks for the PR!
This diff is actually much smaller than smaller then what I had envisioned originally, nice work!
I just have some minor nits related to comments and a bit of refactoring, but once those are addressed I should be able to merge this in pretty quickly.
| sig := e.Serialize() | ||
| if len(sig) > 73 { | ||
| return fmt.Errorf("Signature too long!") | ||
| var b [64]byte |
There was a problem hiding this comment.
Can you extract this section into a distinct function? We may want to add some additional test cases in order to ensure the parsing logic is correct. Additonally this set of future test cases could be added to the spec as test vectors to ensure implementations are encoding/decoding the signatures correctly.
The same goes for the newly added case within readElement.
There was a problem hiding this comment.
ACK. Two new functions in a separate file (signature.go) with tests in signature_test.go. Let me know if you want additional test vectors.
lnwire/lnwire.go
Outdated
| if len(sig) > 73 { | ||
| return fmt.Errorf("Signature too long!") | ||
| var b [64]byte | ||
| // Extract lengths of R and S |
There was a problem hiding this comment.
Minor nit, but can you add a comment here which explains that we grab from an index of 3 since the first three bytes within the DER encoding are 0x03 <length> 0x02, with the length of R being the 4th byte and similarly a comment which explains that the length of R there's an additional 0x02 byte which needs to be skipped.
Referring to the godoc for the Serialize method might be helpful.
There was a problem hiding this comment.
In general having the format of the serialized sig:
0x30 <length> 0x02 <length r> r 0x02 <length s> s
displayed somewhere in the comment can be helpful as it's easier to follow the logic with the full format in mind.
There was a problem hiding this comment.
ACK. I've improved the comments around this with an explanation and the format of the serialized sig.
lnwire/lnwire.go
Outdated
| // Check to make sure each can fit into the intended buffer | ||
| if sLen > 32 { | ||
| if (sLen > 33) || (sig[6+rLen] != 0x00) { | ||
| return fmt.Errorf("S is over 32 bytes long without padding") |
There was a problem hiding this comment.
Can you trim the line to 80 chars here? Thanks!
Same from the error message below on line 193.
There was a problem hiding this comment.
ACK. I had tabs shown as 2 spaces instead of 8 like GitHub shows. Fixed.
lnwire/lnwire.go
Outdated
| } else { | ||
| copy(b[64-sLen:], sig[6+rLen:]) | ||
| } | ||
| if rLen > 32 { |
There was a problem hiding this comment.
Minor nit: I think the code would be easier to follow if this section (for R) was moved to lie before the section that packs S into the buffer.
There was a problem hiding this comment.
R needs to be after S because we subtract one from rLen, which affects the index lookups in the S extraction. I can create new variables to track this but it wouldn't be as efficient. Let me know what you prefer.
There was a problem hiding this comment.
I've added better comments about the logic in this section. If you still want me to switch the code blocks for R and S around, let me know and I'll use more variables to track indices.
lnwire/lnwire.go
Outdated
| // extractCanonicalPadding is a utility function to extract the canonical | ||
| // padding of a big-endian integer from the wire encoding (a 0-padded | ||
| // big-endian integer) such that it passes btcec.canonicalPadding test. | ||
| func extractCanonicalPadding (b []byte) []byte { |
There was a problem hiding this comment.
Has this section been run through gofmt? From a glance it looks like it hasn't been because the indentation looks a bit shallow.
There was a problem hiding this comment.
ACK. Fixed by gofmt.
…-beta Upgrade to LND 0.15.1
2 participants
Formatting changes are due to
go fmt. If you'd like these reverted, please let me know.I have changed the writeElement and readElement functions to represent the signatures as 64 bytes on the wire with R being the first 32 and S being the second 32 bytes, both big-endian and zero-padded. The interaction between these functions and the rest of the code is not changed.