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

go: Switch to CometBFT v0.34.27 #5268

Merged
merged 1 commit into from May 23, 2023
Merged

go: Switch to CometBFT v0.34.27 #5268

merged 1 commit into from May 23, 2023

Conversation

abukosek
Copy link
Contributor

@abukosek abukosek commented May 23, 2023

This PR switches from our fork of Tendermint to our fork of CometBFT, which is a drop-in replacement.

Here is our fork of CometBFT that contains the exact same patches as our Tendermint fork: https://github.com/oasisprotocol/cometbft/tree/v0.34.27-oasis1

@abukosek abukosek marked this pull request as ready for review May 23, 2023 09:24
@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #5268 (821075e) into master (d243d97) will decrease coverage by 0.69%.
The diff coverage is 79.74%.

@@            Coverage Diff             @@
##           master    #5268      +/-   ##
==========================================
- Coverage   67.09%   66.41%   -0.69%     
==========================================
  Files         516      516              
  Lines       54814    54825      +11     
==========================================
- Hits        36780    36411     -369     
- Misses      13574    13912     +338     
- Partials     4460     4502      +42     
Impacted Files Coverage Δ
go/consensus/tendermint/db/badger/badger.go 71.89% <ø> (ø)
go/consensus/tendermint/db/closer.go 100.00% <ø> (ø)
go/consensus/tendermint/db/init.go 78.94% <ø> (ø)
go/consensus/tendermint/full/archive.go 0.00% <0.00%> (ø)
go/consensus/tendermint/full/common.go 62.06% <ø> (-2.48%) ⬇️
go/worker/compute/executor/committee/node.go 72.25% <41.66%> (+0.16%) ⬆️
go/worker/registration/worker.go 64.58% <70.37%> (+3.29%) ⬆️
go/consensus/tendermint/full/full.go 60.85% <100.00%> (ø)
go/consensus/tendermint/light/internal.go 60.60% <100.00%> (ø)
go/consensus/tendermint/light/service.go 54.38% <100.00%> (ø)
... and 3 more

... and 74 files with indirect coverage changes

Comment on lines -78 to +77
github.com/btcsuite/btcd v0.22.1 // indirect
github.com/btcsuite/btcd/btcec/v2 v2.2.1 // indirect
Copy link
Member

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

@peternose peternose left a comment

Choose a reason for hiding this comment

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

Running go mod tidy returns an error.

➜  go git:(821075e18) ✗ go mod tidy
github.com/oasisprotocol/oasis-core/go/consensus/tendermint/abci imports
        github.com/tendermint/tendermint/abci/types imports
        github.com/tendermint/tendermint/crypto/secp256k1 imports
        github.com/btcsuite/btcd/btcec/v2/ecdsa tested by
        github.com/btcsuite/btcd/btcec/v2/ecdsa.test imports
        github.com/btcsuite/btcd/chaincfg/chainhash: ambiguous import: found package github.com/btcsuite/btcd/chaincfg/chainhash in multiple modules:
        github.com/btcsuite/btcd v0.20.1-beta (/home/peter/go/pkg/mod/github.com/btcsuite/btcd@v0.20.1-beta/chaincfg/chainhash)
        github.com/btcsuite/btcd/chaincfg/chainhash v1.0.1 (/home/peter/go/pkg/mod/github.com/btcsuite/btcd/chaincfg/chainhash@v1.0.1)

@@ -9,10 +9,10 @@ import (
"strings"
"sync"

dbm "github.com/cometbft/cometbft-db"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dbm "github.com/cometbft/cometbft-db"
cdb "github.com/cometbft/cometbft-db"

Have no idea where dbm comes from :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use this elsewhere for the Tendermint DB import, so I standardized on it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Your changes make sense, but not sure why was named dbm in the first place. As we switched to cometbft, we could rename to cdb or something similar everywhere.

Copy link
Contributor Author

@abukosek abukosek May 23, 2023

Choose a reason for hiding this comment

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

What if they rename it again?

dbm is probably "database module" or something similar, I think it fits fine :P

@kostko
Copy link
Member

kostko commented May 23, 2023

Running go mod tidy returns an error.

That's strange, we run this as part of CI?

@abukosek
Copy link
Contributor Author

Running go mod tidy returns an error.

I have no idea how to fix this, it seems like a problem in an upstream package or go mod tidy itself.

@abukosek
Copy link
Contributor Author

Running go mod tidy returns an error.

I have no idea how to fix this, it seems like a problem in an upstream package or go mod tidy itself.

This will probably fix it: golang/go#60352
But I'm not waiting for 1.20.5 to be released just for this, going to go ahead with the merge now :)

@peternose
Copy link
Contributor

That's strange, we run this as part of CI?

We should probably fix common.mk (replace ; with &&):

$(GO) mod tidy; \

@abukosek abukosek merged commit 0a5c727 into master May 23, 2023
3 of 4 checks passed
@abukosek abukosek deleted the andrej/feature/cometbft branch May 23, 2023 10:32
@abukosek
Copy link
Contributor Author

We should probably fix common.mk (replace ; with &&):

$(GO) mod tidy; \

We can do this after Go 1.20.5 is released, which should have a fix for this issue, otherwise all our builds will keep failing...

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.

None yet

3 participants