btcd
btcd copied to clipboard
btcsuite
Faster operations on the elliptical curve exponent by using nfield instead of big.Int
Very nice! Thanks a ton for working on this. It's obviously going to take a lot of careful review and there are some Travis issues to take care of, but it will be nice to get this in not only for the speed up, but also because we can stop relying on the math/big package which has been rather buggy.
Review status: 0 of 8 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.
btcec/nfield.go, line 156 [r1] (raw file):
Can the nfieldVal type be declared as type nfieldValue [10]uint32 instead seeing as it only has one member? Not sure if it's possible but it might make the struct even more memory compact.
Comments from the review on Reviewable.io
I am wonder if it would useful for the big.Ints mode to be available to run in tandem with the field elements mode for testing and verification of results.
There are several issues reported by Travis that need to be resolved before this can be merged as well:
btcec/btcec.go:643:2: should replace c2.n[0] += 1 with c2.n[0]++
btcec/internal_test.go:91:21: exported func NewNfieldVal returns unexported type *btcec.nfieldVal, which can be annoying to use
btcec/internal_test.go:96:53: exported func TstNonceRFC6979 returns unexported type *btcec.nfieldVal, which can be annoying to use
btcec/signature.go:73:1: exported method Signature.Verify should have comment or be unexported
btcec/btcec.go:643:2: should replace c2.n[0] += 1 with c2.n[0]++
btcec/internal_test.go:91:21: exported func NewNfieldVal returns unexported type *btcec.nfieldVal, which can be annoying to use
btcec/internal_test.go:96:53: exported func TstNonceRFC6979 returns unexported type *btcec.nfieldVal, which can be annoying to use
btcec/signature.go:73:1: exported method Signature.Verify should have comment or be unexported
rebased, have addressed some of the comments. will put on my todo list.
This is a time-consuming PR to put into mind again. I can't promise this will be there that soon.
@jimmysong: Thanks. If you would prefer, I can take this over and drive it to completion. I will of course leave your name on the commit and give you the opportunity to review any modifications I might make.
@davecgh I would like that very much. It'll be easier for me to review than to code right now.
I tried to sync testnet with this PR and got this:
00:16:15 2017-01-22 [INF] BMGR: Rejected block 00000000036334b7b4272c4d3fc1a72e07159c7938859d1580aca99807232243 from 127.0.0.1:18333 (outbound):
failed to validate input 95ec9f464983a63f393f3e7b20a974668213ad76b816206343718a13d9ce897a:0
which references output 19845d028aebcbfd19538fe84887c5a4f29bbf0629c68327b77dca95dc61acde:0
- false stack entry at end of script execution
(input script bytes 493046022100d390d20333811dfdac7aa582649532ce02cc95b878d5fbd8f7906cf890d56515022100f442a1aeb43d665e31ae6021f9f596cd06c91ede3c3344f1ace0e61cc45ff0610121029c1d751235217819e6942d8f929ce56fe5e80bb9a12f34fddc9ed889df5a8e30,
prev output script bytes 76a91400938bf234076a5ef3cf8936bbc4fa53c3e2661f88ac)
@stevenroose: Thanks for pasting results. I haven't been able to go over this in great detail yet, so that will be handy to track down the issue when I eventually get to it.
@davecgh I wanted to fix some of the remarks you made on the code in order to get this PR included. It seems like it could be a good performance bonus.
I figured you'd walk into the same issue yourself anyway, though.
I wondered how one would go about making a tree of if-blocks like this one into a constant-time implementation.. I tried, but the double if's at the end make it really messy.
@stevenroose: You'd need to convert them to a series of bit manipulations with addition and/or subtraction as necessary to ensure that there are no differences in the branches (at least as much as possible, there is no way to full guarantee what the CPU will do even if the assembler is using constant time bitwise ops, but you still want to give it the best possible chance and in practice bitwise ops are the best bet for that) .
For example, consider the constant time implementation of IsZero in field.go:
bits := f.n[0] | f.n[1] | f.n[2] | f.n[3] | f.n[4] | f.n[5] | f.n[6] | f.n[7] | f.n[8] | f.n[9]
return bits == 0
Now compare that against the typical implementation that is not constant time:
return f.n[0] == 0 && f.n[1] == 0 && ... f.n[9] == 0
That will short circuit and return on the first non-zero word that is encountered, so different values will result in a different running time. This is easy to see when looking at the generated assembler since it will employ a bunch of "jump if equal" comparison branches.
Just out of curiosity, where is the limit? crypto/subtle provides a method ConstantTimeEq(x, y int32) int, which could be used to do all the comparisons like t9 == nfieldMSBMask.
I took a turn at resolving the rest of the comments and rebased - changes here. To aid in some of the constant time calculations I introduced a little library following crypto/subtle for uint32. I would appreciate any feedback - I talked to @jimmysong elsewhere and hope to get his approval on my changes soon to move this PR along.
Also - in response to @stevenroose , I think that an effort to use crypto/subtle in place of any branching to avoid timing attacks based on branch prediction would be good for this repo. To check in on the relative performance of this approach I made a little example repo benchmarking the crypto/subtle functions against their branching counterparts. For all but equality checks there is no measurable performance cost to using functions like in crypto/subtle in place of built-in comparisons with equal-time branches. I will recommend we convert some other constant time function to use these tools in another issue.
@jakesyl
Btcd was abandoned by the original authors as they went off to make an altcoin. Safe to say this feature is abandoned.
@kcalvinalvin as far as I can tell @Roasbeef is still active as of November. Decred split off a while ago