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

cgo signature verification not supported & cannot use privkey verify signature #10747

Closed
4 tasks
ningxin18 opened this issue Dec 13, 2021 · 4 comments · Fixed by #11298
Closed
4 tasks

cgo signature verification not supported & cannot use privkey verify signature #10747

ningxin18 opened this issue Dec 13, 2021 · 4 comments · Fixed by #11298
Labels

Comments

@ningxin18
Copy link

Summary of Bug

secp256k1_cgo issue

Although I used +build !libsecp256k1, but secp256k1_cgo signature veryfication not supported.

line 24 in secp256k1_cgo.go, there's cannot use private key verify signature

func (pubKey *PrivKey) VerifySignature(msg []byte, sig []byte) bool {
	return secp256k1.VerifySignature(pubKey.Key, crypto.Sha256(msg), sig)
}

change as below

func(pubkey *PubKey) VerifySignature(msg []byte, sig []byte) bool {
	return secp256k1.VerifySignature(pubkey.Bytes(), crypto.Sha256(msg), sig)
}

secp256k1_cgo sig verifiction issue

when I use ethereum verify signature:

  msg := []byte("We have lingered long enough on the shores of the cosmic ocean.")
  
  msg = crypto.Keccak256(msg)
  key, _ := ecdsa.GenerateKey(secp256k1.S256(), rand.Reader)
  priv2 :=math.PaddedBigBytes(key.D, 32)
  
  signature, err2 := secp256k1.Sign(msg, priv2)
  require.Nil(t, err2)
  
  
  time1 := time.Now()
  pubkey := elliptic.Marshal(secp256k1.S256(), key.X, key.Y)
  ok := secp256k1.VerifySignature(pubkey, msg, signature[:64])
  fmt.Println("time:", time.Now().Sub(time1).Microseconds()) 

ethereum cgo average verification time is 60 ms

but cosmos go sig verification takes 350 ms, cgo sig verification takes 180 ms

  msg := []byte("We have lingered long enough on the shores of the cosmic ocean.")
  
  priv := GenPrivKey()
  sigStr, err := priv.Sign(msg)
  require.NoError(t, err)
  
  pub := priv.PubKey()
  
  time1 := time.Now()
  ok := pub.VerifySignature(msg, sigStr[:64])
  fmt.Println("time:", time.Now().Sub(time1).Microseconds())

Version

Every release

Steps to Reproduce


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@tac0turtle
Copy link
Member

can you explain if you need ego verification? As a team we talked about deprecating and removing the ego version in favour of the native go one, but this is because we did not know of a user for it

@ningxin21
Copy link

can you explain if you need ego verification? As a team we talked about deprecating and removing the ego version in favour of the native go one, but this is because we did not know of a user for it

We use cosmos-sdk develop dex, so we need maximize performance.
As you can see, the test above shows that cgo-version can double the performance.

@tac0turtle
Copy link
Member

cc @ValarDragon do you have any idea why cosmos ego is so much slower as well?

@ValarDragon
Copy link
Contributor

ValarDragon commented Feb 15, 2022

I believe Eth's CGO implementation now takes advantage of GLV optimizations. It was in bitcoin core for ages, but not in Ethereum due to patent issues. The patent expired sometime in the last two years, and then Ethereum switched is my understanding.

@mergify mergify bot closed this as completed in #11298 Mar 10, 2022
mergify bot pushed a commit that referenced this issue Mar 10, 2022
## Description

Closes: #10747 

- update secp256k1 cgo fork, 
- debug verify bytes


```
benchmark                     old ns/op     new ns/op     delta
BenchmarkKeyGeneration-10     407           413           +1.35%
BenchmarkSigning-10           95099         36754         -61.35%
BenchmarkVerification-10      215551        48053         -77.71%

benchmark                     old allocs     new allocs     delta
BenchmarkKeyGeneration-10     2              2              +0.00%
BenchmarkSigning-10           83             4              -95.18%
BenchmarkVerification-10      74             1              -98.65%

benchmark                     old bytes     new bytes     delta
BenchmarkKeyGeneration-10     96            96            +0.00%
BenchmarkSigning-10           5283          196           -96.29%
BenchmarkVerification-10      3537          32            -99.10%
```

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
mergify bot pushed a commit that referenced this issue Mar 10, 2022
## Description

Closes: #10747

- update secp256k1 cgo fork,
- debug verify bytes

```
benchmark                     old ns/op     new ns/op     delta
BenchmarkKeyGeneration-10     407           413           +1.35%
BenchmarkSigning-10           95099         36754         -61.35%
BenchmarkVerification-10      215551        48053         -77.71%

benchmark                     old allocs     new allocs     delta
BenchmarkKeyGeneration-10     2              2              +0.00%
BenchmarkSigning-10           83             4              -95.18%
BenchmarkVerification-10      74             1              -98.65%

benchmark                     old bytes     new bytes     delta
BenchmarkKeyGeneration-10     96            96            +0.00%
BenchmarkSigning-10           5283          196           -96.29%
BenchmarkVerification-10      3537          32            -99.10%
```

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit 361c837)

# Conflicts:
#	CHANGELOG.md
#	crypto/keys/secp256k1/internal/secp256k1/README.md
tac0turtle added a commit that referenced this issue Mar 11, 2022
* fix: cgosecp256k1 verification (#11298)

## Description

Closes: #10747

- update secp256k1 cgo fork,
- debug verify bytes

```
benchmark                     old ns/op     new ns/op     delta
BenchmarkKeyGeneration-10     407           413           +1.35%
BenchmarkSigning-10           95099         36754         -61.35%
BenchmarkVerification-10      215551        48053         -77.71%

benchmark                     old allocs     new allocs     delta
BenchmarkKeyGeneration-10     2              2              +0.00%
BenchmarkSigning-10           83             4              -95.18%
BenchmarkVerification-10      74             1              -98.65%

benchmark                     old bytes     new bytes     delta
BenchmarkKeyGeneration-10     96            96            +0.00%
BenchmarkSigning-10           5283          196           -96.29%
BenchmarkVerification-10      3537          32            -99.10%
```

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit 361c837)

# Conflicts:
#	CHANGELOG.md
#	crypto/keys/secp256k1/internal/secp256k1/README.md

* fix conflicts

Co-authored-by: Marko <marbar3778@yahoo.com>
JimLarson pushed a commit to agoric-labs/cosmos-sdk that referenced this issue Jul 7, 2022
* fix: cgosecp256k1 verification (cosmos#11298)

## Description

Closes: cosmos#10747

- update secp256k1 cgo fork,
- debug verify bytes

```
benchmark                     old ns/op     new ns/op     delta
BenchmarkKeyGeneration-10     407           413           +1.35%
BenchmarkSigning-10           95099         36754         -61.35%
BenchmarkVerification-10      215551        48053         -77.71%

benchmark                     old allocs     new allocs     delta
BenchmarkKeyGeneration-10     2              2              +0.00%
BenchmarkSigning-10           83             4              -95.18%
BenchmarkVerification-10      74             1              -98.65%

benchmark                     old bytes     new bytes     delta
BenchmarkKeyGeneration-10     96            96            +0.00%
BenchmarkSigning-10           5283          196           -96.29%
BenchmarkVerification-10      3537          32            -99.10%
```

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit 361c837)

# Conflicts:
#	CHANGELOG.md
#	crypto/keys/secp256k1/internal/secp256k1/README.md

* fix conflicts

Co-authored-by: Marko <marbar3778@yahoo.com>
randy75828 pushed a commit to Switcheo/cosmos-sdk that referenced this issue Aug 10, 2022
* fix: cgosecp256k1 verification (cosmos#11298)

## Description

Closes: cosmos#10747

- update secp256k1 cgo fork,
- debug verify bytes

```
benchmark                     old ns/op     new ns/op     delta
BenchmarkKeyGeneration-10     407           413           +1.35%
BenchmarkSigning-10           95099         36754         -61.35%
BenchmarkVerification-10      215551        48053         -77.71%

benchmark                     old allocs     new allocs     delta
BenchmarkKeyGeneration-10     2              2              +0.00%
BenchmarkSigning-10           83             4              -95.18%
BenchmarkVerification-10      74             1              -98.65%

benchmark                     old bytes     new bytes     delta
BenchmarkKeyGeneration-10     96            96            +0.00%
BenchmarkSigning-10           5283          196           -96.29%
BenchmarkVerification-10      3537          32            -99.10%
```

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit 361c837)

# Conflicts:
#	CHANGELOG.md
#	crypto/keys/secp256k1/internal/secp256k1/README.md

* fix conflicts

Co-authored-by: Marko <marbar3778@yahoo.com>
randy75828 pushed a commit to Switcheo/cosmos-sdk that referenced this issue Aug 10, 2022
* fix: cgosecp256k1 verification (cosmos#11298)

## Description

Closes: cosmos#10747

- update secp256k1 cgo fork,
- debug verify bytes

```
benchmark                     old ns/op     new ns/op     delta
BenchmarkKeyGeneration-10     407           413           +1.35%
BenchmarkSigning-10           95099         36754         -61.35%
BenchmarkVerification-10      215551        48053         -77.71%

benchmark                     old allocs     new allocs     delta
BenchmarkKeyGeneration-10     2              2              +0.00%
BenchmarkSigning-10           83             4              -95.18%
BenchmarkVerification-10      74             1              -98.65%

benchmark                     old bytes     new bytes     delta
BenchmarkKeyGeneration-10     96            96            +0.00%
BenchmarkSigning-10           5283          196           -96.29%
BenchmarkVerification-10      3537          32            -99.10%
```

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit 361c837)

# Conflicts:
#	CHANGELOG.md
#	crypto/keys/secp256k1/internal/secp256k1/README.md

* fix conflicts

Co-authored-by: Marko <marbar3778@yahoo.com>
Eengineer1 pushed a commit to cheqd/cosmos-sdk that referenced this issue Aug 26, 2022
* fix: cgosecp256k1 verification (cosmos#11298)

Closes: cosmos#10747

- update secp256k1 cgo fork,
- debug verify bytes

```
benchmark                     old ns/op     new ns/op     delta
BenchmarkKeyGeneration-10     407           413           +1.35%
BenchmarkSigning-10           95099         36754         -61.35%
BenchmarkVerification-10      215551        48053         -77.71%

benchmark                     old allocs     new allocs     delta
BenchmarkKeyGeneration-10     2              2              +0.00%
BenchmarkSigning-10           83             4              -95.18%
BenchmarkVerification-10      74             1              -98.65%

benchmark                     old bytes     new bytes     delta
BenchmarkKeyGeneration-10     96            96            +0.00%
BenchmarkSigning-10           5283          196           -96.29%
BenchmarkVerification-10      3537          32            -99.10%
```

---

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit 361c837)

* fix conflicts

Co-authored-by: Marko <marbar3778@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants