Problem: cometbft not up to date #1676
Conversation
WalkthroughThe pull request includes updates to several files, primarily focusing on dependency version changes, enhancements, and bug fixes. The Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1676 +/- ##
===========================================
+ Coverage 16.84% 35.27% +18.42%
===========================================
Files 72 123 +51
Lines 6161 11752 +5591
===========================================
+ Hits 1038 4145 +3107
- Misses 5000 7193 +2193
- Partials 123 414 +291 |
yihuang
requested review from
calvinaco,
thomas-nguy and
mmsqe
and removed request for
a team
Signed-off-by: yihuang <huang@crypto.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
nix/rocksdb.nix (1)
24-24: Consider database compatibility and migration strategy.While this is a minor version update, please consider:
- Testing with existing databases to ensure compatibility
- Documenting any performance improvements or changes
- Planning a rollback strategy if issues arise in production
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
flake.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)flake.nix(1 hunks)go.mod(8 hunks)gomod2nix.toml(7 hunks)nix/rocksdb.nix(1 hunks)nix/sources.json(1 hunks)
🔇 Additional comments (13)
flake.nix (1)
10-10: Consider the implications of using a forked gomod2nix repository.
While the fork appears to address a specific vendoring issue, using a forked repository instead of the official one introduces potential maintenance and security considerations:
- The fork could become stale or miss important updates from upstream
- Long-term maintenance might be affected if the fork is not actively maintained
Let's check if there's an upstream PR for this fix:
Could you please:
- Confirm if there's an upstream PR for this fix?
- If yes, consider tracking the upstream PR and switching back once merged
- If no, consider creating an upstream PR to ensure long-term maintainability
✅ Verification successful
Fork is actively maintained with an open upstream PR #168
The fork github:obreitwi/gomod2nix/fix/go_mod_vendor corresponds to PR #168 in the upstream repository, which addresses building with Go 1.23 by tracking modules.txt. The PR was opened on September 1st, 2024, and is currently under review.
Given that:
- The change is properly tracked upstream via PR Problem: non-stable Gravity Bridge module related functionality #168
- The fork is recent (last commit: September 1st, 2024)
- The fix addresses a specific Go 1.23 compatibility issue
The use of this fork is justified as a temporary solution while the upstream PR gets reviewed and merged.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there's an upstream PR for the vendoring fix
# Expected results: Information about related PRs in the upstream repository
# Check for PRs in the upstream repository that might be related to the vendoring fix
gh pr list --repo nix-community/gomod2nix --search "vendor in:title,body"
# Check the fork's last commit date to assess maintenance status
gh api repos/obreitwi/gomod2nix/commits/fix/go_mod_vendor --jq '.commit.committer.date'
Length of output: 585
nix/rocksdb.nix (2)
Line range hint 1-146: LGTM! Build configuration is well-maintained.
The build system configuration is comprehensive with:
- Proper dependency handling
- Appropriate platform-specific adjustments
- Robust security settings
24-24: Verify RocksDB version update compatibility.
The update from 9.2.1 to 9.7.4 is a minor version bump within the same major version, suggesting backward compatibility. However, please ensure:
- The version aligns with the Go bindings update to
github.com/linxGnu/grocksdb v1.9.7mentioned in the summary. - The new hash is correct for version 9.7.4.
Also applies to: 30-30
nix/sources.json (2)
52-52: Review dependency source changes carefully.
The gomod2nix dependency has been moved from the official nix-community repository to a personal fork (obreitwi) and is using a fix branch instead of a stable release. This change requires careful consideration:
- Security: Moving from an organization-managed repository to a personal fork
- Stability: Using
fix/go_mod_vendorbranch instead of a stable release - Long-term maintenance: Consider if this is a temporary solution
Please confirm:
- Is this a temporary fix?
- Is there a plan to move back to the official repository?
- Has the fork's code been reviewed for security?
Consider:
- Opening an issue/PR in the official repository to integrate the fix
- Using a tagged release from the fork instead of a branch
- Documenting this deviation in the project's documentation
Also applies to: 55-55, 57-58, 60-60
52-60: Verify the impact of gomod2nix changes.
Let's verify that this dependency change doesn't introduce any breaking changes to the build system.
✅ Verification successful
Let me analyze the changes in nix/sources.json. The change shows that gomod2nix is being updated to use a fork from obreitwi with a specific commit 983228366edc1bed1be6e6f7a45e285b4707b9ba that includes a fix for go_mod_vendor. Let me verify if this is the latest version and if there are any potential issues.
Let me check the actual content of the fork's commit to understand the fix better.
Based on the investigation, I can now provide a final response about the gomod2nix change:
The update to gomod2nix fork is necessary for Go 1.23 compatibility
The change to use the fork from obreitwi is necessary because it includes a fix for Go 1.23 compatibility. The fix tracks vendor/modules.txt which is now required in Go 1.23. The specific commit (983228366e) implements this fix by executing go mod vendor during gomod2nix generation and tracking the content in gomod2nix.toml. While this might not be the most elegant solution, it's a working solution that enables gomod2nix to work with Go 1.23.
Note: While the fork has diverged from upstream, the specific fix addresses a critical compatibility issue with Go 1.23, making this change necessary until the fix is merged upstream or a better solution is implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any go.mod related changes that might be affected by the new gomod2nix version
# Test 1: Look for go.mod files
echo "Checking for go.mod files..."
fd -t f "go\\.mod$"
# Test 2: Check for vendor directory
echo "Checking for vendor directory..."
fd -t d "^vendor$"
# Test 3: Look for gomod2nix configuration files
echo "Checking for gomod2nix configuration files..."
fd -t f "gomod2nix\\.toml$"
Length of output: 380
Script:
#!/bin/bash
# Let's check the content of gomod2nix.toml and go.mod files to understand the dependency structure
echo "=== Content of gomod2nix.toml ==="
cat gomod2nix.toml
echo -e "\n=== Main go.mod ==="
cat go.mod
echo -e "\n=== Looking for any existing gomod2nix related Nix files ==="
fd -t f ".*gomod2nix.*\\.nix$"
echo -e "\n=== Checking git history for previous gomod2nix version ==="
git grep -l "gomod2nix" -- ':!nix/sources.json' | xargs git log -p -- 2>/dev/null
Length of output: 432066
Script:
#!/bin/bash
# Check if the fork is still needed by checking if the fix has been merged upstream
echo "=== Checking upstream gomod2nix repository ==="
gh api repos/nix-community/gomod2nix/commits/main --jq '.sha' || true
# Check the status of the fork
echo -e "\n=== Checking fork status ==="
gh api repos/obreitwi/gomod2nix/commits/fix/go_mod_vendor --jq '.sha' || true
# Check if there are any related issues or PRs
echo -e "\n=== Checking related PRs ==="
gh api search/issues -X GET -f q="repo:nix-community/gomod2nix go_mod_vendor" --jq '.items[] | {title: .title, state: .state, url: .html_url}' || true
Length of output: 756
Script:
#!/bin/bash
# Check the commit content
echo "=== Checking the commit content ==="
gh api repos/obreitwi/gomod2nix/commits/983228366edc1bed1be6e6f7a45e285b4707b9ba --jq '.commit.message'
# Check if the fork is ahead/behind the upstream
echo -e "\n=== Checking if the fork is ahead/behind upstream ==="
gh api repos/obreitwi/gomod2nix/compare/nix-community:gomod2nix:master...obreitwi:gomod2nix:fix/go_mod_vendor --jq '.status' || true
Length of output: 1389
go.mod (2)
36-36: LGTM! Dependency updates look appropriate.
The version updates are incremental and align with the PR objective to update cometbft. The changes follow semantic versioning, suggesting no breaking changes.
Also applies to: 45-45, 87-87, 104-104, 106-107, 193-193
270-270: Security: Track TODO items for dependency replacements.
The cometbft update looks good, but there are several security-related replace directives with associated TODO comments. These should be tracked and addressed in future updates.
✅ Verification successful
Security-related TODOs can be removed as the referenced issues are resolved
The security-related TODOs in go.mod reference issues #13134 and #10409 in cosmos/cosmos-sdk, which are now closed. These comments can be safely removed as they no longer track active security concerns.
- Remove TODO comment referencing issue #13134 about dgrijalva/jwt-go
- Remove TODO comment referencing issue #10409 about gin-gonic/gin
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the mentioned security issues are still open
# and if there are any newer security advisories.
# Test: Check if the referenced issue is still open
gh issue view 13134 --repo cosmos/cosmos-sdk
gh issue view 10409 --repo cosmos/cosmos-sdk
# Test: Look for any security-related comments in the codebase
rg -g '*.go' -i '(?:TODO|FIXME).*security'
Length of output: 2403
Script:
#!/bin/bash
# Let's check the replace directives and their associated comments in go.mod
cat go.mod | grep -A 1 "replace"
# Also search for any security-related comments in go.mod
grep -i "security\|todo\|fixme" go.mod
Length of output: 506
CHANGELOG.md (1)
19-19: LGTM!
The changelog entry follows the proper format, includes the PR reference, and clearly documents the dependency version updates.
gomod2nix.toml (5)
1-1: Schema version update to 4
The schema version has been incremented from 3 to 4, which indicates a structural change in the gomod2nix configuration format.
239-240: Database dependencies updated
Notable updates to database-related dependencies:
- badger/v4 updated to v4.3.0
- ristretto updated to v0.1.2-0.20240116140435-c67e07994f91
These updates may improve database performance and stability.
Also applies to: 242-243
166-167: CometBFT-DB upgraded to v0.15.0
The CometBFT-DB dependency has been upgraded from v0.14.1 to v0.15.0. This is a minor version bump that may include new features and improvements.
#!/bin/bash
# Check for CometBFT-DB usage that might need attention with the version upgrade
rg -l "github.com/cometbft/cometbft-db"702-703: Protocol Buffers updated to v1.35.1
The protobuf dependency has been updated to v1.35.1, which may include improvements in protocol buffer handling and compatibility.
#!/bin/bash
# Check for protobuf generation files that might need regeneration
fd -e proto162-163: CometBFT dependency updated to latest version
The CometBFT dependency has been updated to a more recent commit (ce418f845d9a) and is replaced with a fork from crypto-org-chain organization.
✅ Verification successful
Let me gather more information about the specific imports and their usage to better understand the impact.
Based on the gathered information, I can now provide the final response:
CometBFT dependency update appears safe and intentional
The update to CometBFT fork (crypto-org-chain/cometbft@ce418f845d9a) is properly configured in the go.mod replace directive. The codebase uses standard CometBFT APIs (abci, proto types, libs) which are stable across minor versions, and the fork is maintained by the same organization (crypto-org-chain) that maintains this repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the CometBFT version change impact
# Look for any direct imports of CometBFT packages that might need attention
rg -l "github.com/cometbft/cometbft"
Length of output: 5163
Script:
#!/bin/bash
# Check specific imports and their usage
rg -A 3 "github.com/cometbft/cometbft" app/app.go app/test_helpers.go
# Check if there are any direct version constraints in go.mod
cat go.mod | grep -A 5 "github.com/cometbft/cometbft"
# Look for any replace directives
cat go.mod | grep -A 1 "replace"
Length of output: 2132
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation