Skip to content

crypto/secp256k1: fix .String()#137

Merged
liamsi merged 1 commit intocelestiaorg:masterfrom
tac0turtle:secp-string
Jan 12, 2021
Merged

crypto/secp256k1: fix .String()#137
liamsi merged 1 commit intocelestiaorg:masterfrom
tac0turtle:secp-string

Conversation

@tac0turtle
Copy link
Collaborator

@tac0turtle tac0turtle commented Jan 12, 2021

Description

  • wrap the key in []Byte() to avoid overflow.
  • fixes e2e failure

This is already in Tendermint just hasn't been added to LL

@tac0turtle tac0turtle self-assigned this Jan 12, 2021
@codecov-io
Copy link

Codecov Report

Merging #137 (dd229b6) into master (0332760) will increase coverage by 0.50%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
+ Coverage   60.33%   60.84%   +0.50%     
==========================================
  Files         262      261       -1     
  Lines       23888    23642     -246     
==========================================
- Hits        14414    14384      -30     
+ Misses       7979     7766     -213     
+ Partials     1495     1492       -3     
Impacted Files Coverage Δ
crypto/secp256k1/secp256k1.go 71.11% <0.00%> (ø)
libs/events/events.go 92.94% <0.00%> (-5.89%) ⬇️
privval/signer_server.go 89.47% <0.00%> (-5.27%) ⬇️
consensus/ticker.go 90.00% <0.00%> (-5.00%) ⬇️
types/tx.go 81.13% <0.00%> (-3.78%) ⬇️
privval/signer_endpoint.go 78.78% <0.00%> (-3.04%) ⬇️
privval/signer_listener_endpoint.go 87.05% <0.00%> (-2.36%) ⬇️
p2p/switch.go 63.88% <0.00%> (-2.17%) ⬇️
blockchain/v0/pool.go 79.08% <0.00%> (-1.53%) ⬇️
blockchain/v0/reactor.go 62.16% <0.00%> (-0.91%) ⬇️
... and 12 more

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Saw the fix in tendermint a while ago. Was a bit surprised that slicing causes the stack to overflow tbh.
(ref: tendermint/tendermint#5707)

LGTM

@liamsi liamsi merged commit 626c194 into celestiaorg:master Jan 12, 2021
evan-forbes pushed a commit that referenced this pull request Mar 11, 2025
* Replace tm-db with cometbft-db

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Update build workflow to use private cometbft-db repo

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Build Docker image on PRs to ensure this works

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Enable private module access for E2E workflows

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Enable private module access for mock generation check

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Enable private module access for tests

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Cosmetic update to build workflow

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Enable private module access for vulncheck

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Enable private module access for linter

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* ci: Build Docker image on PRs to ensure this works

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* docker: Add support for usage of cometbft-db as private module

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Replace TENDERMINT_BUILD_OPTIONS with COMETBFT_BUILD_OPTIONS to fix building of database support

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* deps: Update cometbft-db to latest unreleased commit

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* deps: Update cometbft-db to latest unreleased commit

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Bump cometbft-db version to v0.7.0

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Minor formatting fixes

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>
evan-forbes pushed a commit that referenced this pull request Mar 11, 2025
Follow-up to #137.

Fixes the Docker builds that were failing to be parsed by GitHub Actions.

Also fixes our `make build-docker` target to build images in the same way we do in CI. On Linux, I have to explicitly tell Docker to use BuildKit to pass the target/build platform args in:

```bash
DOCKER_BUILDKIT=1 make build-docker
```

Given that I've also proven now that the images do build in CI (see [this workflow](https://github.com/cometbft/cometbft/actions/runs/3964436268/jobs/6793294390) and [this one](https://github.com/cometbft/cometbft/actions/runs/3964436273/jobs/6793294238)), we no longer need to build on every change to every pull request. The E2E node Docker image in particular takes nearly 30mins to build, so that would slow us down tremendously. It's far better to build those images only once PRs get merged.

---

#### PR checklist

- [x] Tests written/updated, or no tests needed
- [x] `CHANGELOG_PENDING.md` updated, or no changelog entry needed
- [x] Updated relevant documentation (`docs/`) and code comments, or no
      documentation updates needed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants