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.

Sign up for GitHub

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

Jump to bottom

use go-ethereum ecies due to instability with ecies/go/v2 #178

Merged
merged 6 commits into from
Jul 19, 2023
Merged

use go-ethereum ecies due to instability with ecies/go/v2 #178

merged 6 commits into from
Jul 19, 2023

Conversation

agouin
Copy link
Member

@agouin agouin commented Jul 18, 2023 •
edited
Loading

The new interchaintest e2e suite caught a bug with the ecies/go/v2 lib. It can fail to encrypt properly when used concurrently.

Swaps out ecies functionality with go ethereum lib instead, and tests updated to catch the bug in the future.

@agouin agouin requested a review from DavidNix July 18, 2023 21:19
@agouin agouin marked this pull request as ready for review July 18, 2023 21:54
DavidNix
DavidNix approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@DavidNix DavidNix left a comment

Choose a reason for hiding this comment

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

Do we need to support ECIES and RSA? Can we only support ECIES? (Maybe not if we've committed to v3.) Would make things simpler.

cmd/horcrux/cmd/leader_election.go Outdated Show resolved Hide resolved
signer/cosigner_security_ecies_test.go Outdated Show resolved Hide resolved
signer/threshold_validator.go Show resolved Hide resolved
signer/cosigner_security_rsa_test.go Outdated Show resolved Hide resolved
@agouin
Copy link
Member Author

agouin commented Jul 19, 2023 •
edited
Loading

Do we need to support ECIES and RSA? Can we only support ECIES?

For backwards compatibility, and to avoid the need to rush to v4, I'd like to keep rsa support. Removing it in a future release will be nice to clean things up.

mark-rushakoff
mark-rushakoff approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@mark-rushakoff mark-rushakoff left a comment

Choose a reason for hiding this comment

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

I left a few small suggestions but nothing blocking merge.

go.work.sum
@@ -28,6 +28,7 @@ cloud.google.com/go/cloudtasks v1.10.0/go.mod h1:NDSoTLkZ3+vExFEWu2UJV1arUyzVDAi
cloud.google.com/go/compute v1.14.0/go.mod h1:YfLtxrj9sU4Yxv+sXzZkyPjEyPBZfXHUvjxega5vAdo=
cloud.google.com/go/compute v1.18.0/go.mod h1:1X7yHxec2Ga+Ss6jPyjxRxpu2uu7PLgsOVXvgU0yacs=
cloud.google.com/go/compute v1.19.0/go.mod h1:rikpw2y+UMidAe9tISo04EHNOIf42RLYF/q8Bs93scU=
cloud.google.com/go/compute/metadata v0.2.0/go.mod h1:zFmK7XCadkQkj6TtorcaGlCW1hT1fIilQDwofLpJ20k=
Copy link
Member

Choose a reason for hiding this comment

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

I was a little surprised to see go.work.sum changing without go.work changing.

As a side note, the general reference seems to be that go.work and go.work.sum typically are not committed to source control. I haven't looked closely enough to see if horcrux meets the standard of exception to that rule. golang/go#53502

Copy link
Member Author

Choose a reason for hiding this comment

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

VSCode requires the go.work in order to have the IDE smarts within multiple modules without opening multiple workspaces with the go.mod at the root.

I suppose I am fine to remove and gitignore it, but it is nice for the IDE to work with everything off a fresh clone.

signer/cosigner_security_ecies.go Outdated Show resolved Hide resolved
signer/cosigner_security_ecies_test.go Outdated Show resolved Hide resolved
signer/cosigner_security_ecies_test.go Outdated Show resolved Hide resolved
signer/cosigner_security_rsa_test.go Outdated Show resolved Hide resolved
signer/threshold_validator.go Show resolved Hide resolved
signer/threshold_validator.go Outdated Show resolved Hide resolved
signer/threshold_validator.go Outdated Show resolved Hide resolved
@agouin agouin merged commit ac0a811 into main Jul 19, 2023
12 checks passed
@agouin agouin deleted the andrew/ecies_eth branch July 19, 2023 16:38
vimystic added a commit that referenced this pull request Jul 20, 2023
@vimystic @agouin @dependabot
merge main into branch (#182)
128d79d 
* use go-ethereum ecies due to instability with ecies/go/v2 (#178)

* use go-ethereum ecies due to instability with ecies/go/v2

* remove extra logging

* check ECIES key also in get leader cmd

* compile error

* tidy logic

* Handle feedback

* Bump github.com/gin-gonic/gin from 1.8.1 to 1.9.1 in /test (#179)

Bumps [github.com/gin-gonic/gin](https://github.com/gin-gonic/gin) from 1.8.1 to 1.9.1.
- [Release notes](https://github.com/gin-gonic/gin/releases)
- [Changelog](https://github.com/gin-gonic/gin/blob/master/CHANGELOG.md)
- [Commits](gin-gonic/gin@v1.8.1...v1.9.1)

---
updated-dependencies:
- dependency-name: github.com/gin-gonic/gin
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Andrew Gouin <andrew@gouin.io>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
vimystic added a commit that referenced this pull request Jul 25, 2023
@vimystic @agouin @dependabot
Vivek/156 : Downgrade golang version from 1.20 to 1.19 (#183)
6cf45a6 
* Add metrics capture for sidecars / cosigners

* merge main into branch (#182)

* use go-ethereum ecies due to instability with ecies/go/v2 (#178)

* use go-ethereum ecies due to instability with ecies/go/v2

* remove extra logging

* check ECIES key also in get leader cmd

* compile error

* tidy logic

* Handle feedback

* Bump github.com/gin-gonic/gin from 1.8.1 to 1.9.1 in /test (#179)

Bumps [github.com/gin-gonic/gin](https://github.com/gin-gonic/gin) from 1.8.1 to 1.9.1.
- [Release notes](https://github.com/gin-gonic/gin/releases)
- [Changelog](https://github.com/gin-gonic/gin/blob/master/CHANGELOG.md)
- [Commits](gin-gonic/gin@v1.8.1...v1.9.1)

---
updated-dependencies:
- dependency-name: github.com/gin-gonic/gin
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Andrew Gouin <andrew@gouin.io>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Remove rouge file

* Downgrade golang to 1.19

* Revert "Remove rouge file"

This reverts commit e2b926a.

* Update based on review

* Update workflow & Docker files to use golang 1.19

* Update to remove appending errors

* Fix

* update to remove error appends from config.go & treshold.go

* Update mod files to 1.19

* remove go.work.sum

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Andrew Gouin <andrew@gouin.io>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavidNix DavidNix DavidNix approved these changes

@mark-rushakoff mark-rushakoff mark-rushakoff approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@agouin @DavidNix @mark-rushakoff