chore(deps): update btcec to the latest version #1329

ajnavarro · 2023-11-02T14:29:02Z

Apart from taking care of security issues, it is slightly faster.

Some context:
After doing some profiling to be sure that the actual storage implementation was a bottleneck when executing Gno code, I found that signature verification is taking ~70% of the execution time. Updating the dependency makes it 68% faster and reduces by 3 the amount of bytes used per operation.

Benchmarks before and after:

Running tool: /usr/local/go/bin/go test -benchmem -run=^$ -bench ^BenchmarkVerify$ github.com/gnolang/gno/tm2/pkg/crypto/secp256k1 -v -count=1

goos: linux
goarch: amd64
pkg: github.com/gnolang/gno/tm2/pkg/crypto/secp256k1
cpu: 11th Gen Intel(R) Core(TM) i7-1195G7 @ 2.90GHz
BenchmarkVerify
BenchmarkVerify-8   	    5726	    206442 ns/op	    4177 B/op	      88 allocs/op
PASS
ok  	github.com/gnolang/gno/tm2/pkg/crypto/secp256k1	1.218s

///////////////////////////////////////////////////////////////////////////

goos: linux
goarch: amd64
pkg: github.com/gnolang/gno/tm2/pkg/crypto/secp256k1
cpu: 11th Gen Intel(R) Core(TM) i7-1195G7 @ 2.90GHz
BenchmarkVerify
BenchmarkVerify-8   	    8354	    141906 ns/op	    1585 B/op	      30 allocs/op
PASS
ok  	github.com/gnolang/gno/tm2/pkg/crypto/secp256k1	1.212s

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

codecov · 2023-11-02T14:56:47Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (5aa8676) 55.59% compared to head (12492f2) 55.95%.

Files	Patch %	Lines
tm2/pkg/crypto/secp256k1/secp256k1_nocgo.go	55.00%	5 Missing and 4 partials ⚠️
tm2/pkg/crypto/ledger/ledger_secp256k1.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1329      +/-   ##
==========================================
+ Coverage   55.59%   55.95%   +0.36%     
==========================================
  Files         420      420              
  Lines       65470    65481      +11     
==========================================
+ Hits        36395    36642     +247     
+ Misses      26222    25975     -247     
- Partials     2853     2864      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gfanton

LGTM 👍

(just a small question)

go.mod

zivkovicmilos

Thank you for adding this fix 🙏

Time to make this rocket fly 🚀

go.mod

netlify · 2023-11-10T09:40:00Z

✅ Deploy Preview for gno-docs2 canceled.

Name	Link
🔨 Latest commit	`d3e6922`
🔍 Latest deploy log	https://app.netlify.com/sites/gno-docs2/deploys/654dfa6e9ca73900089c2d2c

thehowl

LGTM.

This is crypto code so I'd like @moul to merge this.

tm2/pkg/crypto/secp256k1/secp256k1_nocgo.go

tm2/pkg/crypto/secp256k1/secp256k1_nocgo_test.go

D4ryl00 · 2023-11-16T11:19:02Z

I tested this PR against the issue #1063 and I have no compilation error 👍

thehowl

Blocked pending addition of golden tests to verify new library doesn't change anything in signature verification, etc.

Apart from taking care of security issues, it is slightly faster. Signed-off-by: Antonio Navarro <antnavper@gmail.com>

Signed-off-by: Antonio Navarro <antnavper@gmail.com>

Checked with previous version, it is working fine. Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>

ajnavarro · 2023-11-20T13:53:49Z

@thehowl added golden tests. Tested also with the previous btcec version

Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>

thehowl

LGTM. I would say we can merge after @moul's eventual approval

piux2 · 2023-11-28T21:44:17Z

tm2/pkg/crypto/secp256k1/secp256k1_nocgo.go

-	return &secp256k1.Signature{
-		R: new(big.Int).SetBytes(sigStr[:32]),
-		S: new(big.Int).SetBytes(sigStr[32:64]),
+func signatureFromBytes(sigStr []byte) (*ecdsa.Signature, bool) {


Is this safe to make the changes here?

piux2 · 2023-11-28T22:08:51Z

@jaekwon can you also review this PR?

In this PR, We directly call crypto APIs from Bitcoin, Decred's library, and Ethereum's signing mechanism. Typically, mixing crypto APIs requires thorough scrutiny for the underline implication.

jaekwon · 2023-11-30T23:39:30Z

go.sum

 github.com/btcsuite/btcd v0.22.0-beta.0.20220111032746-97732e52810c/go.mod h1:tjmYdS6MLJ5/s0Fj4DbLgSbDHbEqLJrtnHecBFkdz5M=
-github.com/btcsuite/btcd/btcutil v1.0.0 h1:dB36qRTOucIh6NUe40UCieOS+axPhP6VNyRtYkTUKKk=
+github.com/btcsuite/btcd v0.23.0/go.mod h1:0QJIIN1wwIXF/3G/m87gIwGniDMDQqjVn4SZgnFpsYY=
+github.com/btcsuite/btcd/btcec/v2 v2.1.0/go.mod h1:2VzYrv4Gm4apmbVVsSq5bqf1Ec8v56E48Vt0Y/umPgA=


can these be compressed into the latest one?

jaekwon · 2023-11-30T23:42:49Z

tm2/pkg/crypto/ledger/ledger_secp256k1.go

-	sigBER := btcec.Signature{R: sigDER.R, S: sigDER.S}
-	return sigBER.Serialize(), nil
+
+	return sigDER.Serialize(), nil


but there's no BER conversion?
What does that even mean?

ajnavarro requested review from jaekwon, moul and a team as code owners November 2, 2023 14:29

github-actions bot assigned ajnavarro Nov 2, 2023

github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Nov 2, 2023

zivkovicmilos requested review from zivkovicmilos and gfanton November 7, 2023 11:11

gfanton approved these changes Nov 7, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

ajnavarro force-pushed the chore/update-btcd-latest-version branch from c834f11 to 8ad1b8d Compare November 7, 2023 14:07

zivkovicmilos approved these changes Nov 7, 2023

View reviewed changes

thehowl reviewed Nov 7, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

thehowl reviewed Nov 7, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

ajnavarro force-pushed the chore/update-btcd-latest-version branch 2 times, most recently from 5695493 to d3e6922 Compare November 10, 2023 09:39

thehowl approved these changes Nov 13, 2023

View reviewed changes

thehowl changed the title ~~chore: Update btcec to the latest version.~~ chore(deps): update btcec to the latest version Nov 13, 2023

moul reviewed Nov 14, 2023

View reviewed changes

tm2/pkg/crypto/secp256k1/secp256k1_nocgo.go Show resolved Hide resolved

moul reviewed Nov 14, 2023

View reviewed changes

tm2/pkg/crypto/secp256k1/secp256k1_nocgo_test.go Show resolved Hide resolved

moul approved these changes Nov 14, 2023

View reviewed changes

thehowl requested changes Nov 16, 2023

View reviewed changes

thehowl mentioned this pull request Nov 16, 2023

Minutes: GitHub Review Meetings [every Thursday] gnolang/meetings#18

Open

3 tasks

ajnavarro added 6 commits November 20, 2023 14:43

chore: Update btcec to the latest version.

3595ec5

Apart from taking care of security issues, it is slightly faster. Signed-off-by: Antonio Navarro <antnavper@gmail.com>

fix mispelled words

ff0b049

Signed-off-by: Antonio Navarro <antnavper@gmail.com>

linter

d0b56c7

Signed-off-by: Antonio Navarro <antnavper@gmail.com>

go mod tidy

6f392b0

Signed-off-by: Antonio Navarro <antnavper@gmail.com>

Refactor gomod

6b9ef98

Signed-off-by: Antonio Navarro <antnavper@gmail.com>

Update btcutil too

673bcdb

Signed-off-by: Antonio Navarro <antnavper@gmail.com>

Add golden test.

1ae6215

Checked with previous version, it is working fine. Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>

ajnavarro force-pushed the chore/update-btcd-latest-version branch from d3e6922 to 1ae6215 Compare November 20, 2023 13:53

ajnavarro added 2 commits November 20, 2023 15:12

gofumpt

22d96e2

Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>

go mod tidy other go.mod files

dc676da

Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>

moul mentioned this pull request Nov 21, 2023

Minutes: Core Staff Weekly Syncs [every Monday] gnolang/meetings#36

Open

thehowl reviewed Nov 23, 2023

View reviewed changes

thehowl approved these changes Nov 23, 2023

View reviewed changes

Merge branch 'master' into chore/update-btcd-latest-version

12492f2

gfanton merged commit 6d05a37 into gnolang:master Nov 23, 2023
194 checks passed

ajnavarro deleted the chore/update-btcd-latest-version branch November 24, 2023 09:25

piux2 reviewed Nov 28, 2023

View reviewed changes

michelleellen mentioned this pull request Nov 29, 2023

Minutes: Public Dev Calls [every Wednesday] gnolang/meetings#37

Open

jaekwon reviewed Nov 30, 2023

View reviewed changes

gfanton pushed a commit to moul/gno that referenced this pull request Jan 18, 2024

chore(deps): update btcec to the latest version (gnolang#1329)

37124c9

kristovatlas requested a review from a team April 11, 2024 03:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(deps): update btcec to the latest version #1329

chore(deps): update btcec to the latest version #1329

ajnavarro commented Nov 2, 2023 •

edited

Loading

codecov bot commented Nov 2, 2023 •

edited

Loading

gfanton left a comment

zivkovicmilos left a comment

netlify bot commented Nov 10, 2023 •

edited

Loading

thehowl left a comment

D4ryl00 commented Nov 16, 2023

thehowl left a comment

ajnavarro commented Nov 20, 2023

thehowl left a comment

piux2 Nov 28, 2023 •

edited

Loading

piux2 commented Nov 28, 2023 •

edited

Loading

jaekwon Nov 30, 2023

jaekwon Nov 30, 2023

chore(deps): update btcec to the latest version #1329

chore(deps): update btcec to the latest version #1329

Conversation

ajnavarro commented Nov 2, 2023 • edited Loading

codecov bot commented Nov 2, 2023 • edited Loading

Codecov Report

gfanton left a comment

Choose a reason for hiding this comment

zivkovicmilos left a comment

Choose a reason for hiding this comment

netlify bot commented Nov 10, 2023 • edited Loading

✅ Deploy Preview for gno-docs2 canceled.

thehowl left a comment

Choose a reason for hiding this comment

D4ryl00 commented Nov 16, 2023

thehowl left a comment

Choose a reason for hiding this comment

ajnavarro commented Nov 20, 2023

thehowl left a comment

Choose a reason for hiding this comment

piux2 Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

piux2 commented Nov 28, 2023 • edited Loading

jaekwon Nov 30, 2023

Choose a reason for hiding this comment

jaekwon Nov 30, 2023

Choose a reason for hiding this comment

ajnavarro commented Nov 2, 2023 •

edited

Loading

codecov bot commented Nov 2, 2023 •

edited

Loading

netlify bot commented Nov 10, 2023 •

edited

Loading

piux2 Nov 28, 2023 •

edited

Loading

piux2 commented Nov 28, 2023 •

edited

Loading