Problem: recent optimizations not included #1669
Conversation
Solution: - add several optimizations in dependencies: a) async fireEvents, b) faster prepare proposal when using NopMempool
WalkthroughThe pull request introduces multiple updates across several files, primarily focusing on enhancements to the proposal handling mechanism, transaction validation, and dependency management. Key changes include the addition of support for registering payees in the relayer precompile, optimizations in proposal handling, and updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (2)integration_tests/ibc_utils.py (2)
Moving the CLI initialization outside the conditional block makes it available throughout the function scope, improving code organization and reducing redundancy.
The addition of explicit waits for new blocks on both chains is a good defensive programming practice. This ensures that both networks are stable and synchronized before proceeding with further operations, preventing potential race conditions in the tests. 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1669 +/- ##
===========================================
+ Coverage 16.88% 35.27% +18.38%
===========================================
Files 72 123 +51
Lines 6149 11752 +5603
===========================================
+ Hits 1038 4145 +3107
- Misses 4988 7193 +2205
- Partials 123 414 +291
|
![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 (3)
app/proposal.go (2)
143-143: Good optimization: Lazy transaction decoding.The addition of lazy transaction decoding is a good optimization that avoids unnecessary decoding when the blocklist is empty or when the transaction object is already available.
Consider adding debug logging to help track validation failures:
if tx == nil { + // Debug logging to track validation performance + logger := ctx.Logger() + logger.Debug("decoding transaction for validation") tx, err = h.TxDecoder(txBz) if err != nil { + logger.Debug("transaction decoding failed", "error", err) return err } }Also applies to: 149-156
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 143-143: app/proposal.go#L143
Added line #L143 was not covered by tests
Missing test coverage for new transaction validation optimizations
The code introduces performance optimizations in transaction validation but lacks corresponding test coverage. Specifically:
- No tests for
ExtTxSelectorand its methodsSelectTxForProposalandSelectTxForProposalFast- No tests for
ProposalHandlerand its transaction validation logic- No tests for blocklist functionality
Please add unit tests covering:
- Transaction selection with valid/invalid transactions
- Fast transaction selection with batch processing
- Blocklist validation scenarios
- Edge cases like empty blocklist and nil transactions
🔗 Analysis chain
Line range hint
1-200: Architectural Review: Sound optimizations need test coverage.The changes introduce well-thought-out optimizations that align with the PR objectives:
- Lazy transaction decoding reduces unnecessary processing
- Batch transaction processing with
SelectTxForProposalFast- Raw byte-level validation support
However, the lack of test coverage for these critical paths is concerning. Consider adding comprehensive tests before merging.
Let's check the current test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing test files fd -e go -e _test.go . | grep -i proposalLength of output: 266
Script:
#!/bin/bash # Search for test files specifically in app directory fd -e go -e _test.go . app/ # Look for any test functions related to proposal handling rg -l "func.*Test.*Proposal" # Check for any existing test implementations for ExtTxSelector or ProposalHandler ast-grep --pattern 'func Test$_(t *testing.T) { $$$ }'Length of output: 358
Script:
#!/bin/bash # Let's specifically look for any test content in app/bench_test.go and app/sim_test.go cat app/bench_test.go app/sim_test.go # Search for any existing test implementations related to transaction validation or proposal handling rg -A 5 "func.*Test.*" app/ # Check if there are any test utilities or setup code in test_helpers.go cat app/test_helpers.goLength of output: 39170
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 50-55: app/proposal.go#L50-L55
Added lines #L50 - L55 were not covered by tests
[warning] 58-64: app/proposal.go#L58-L64
Added lines #L58 - L64 were not covered by tests
[warning] 66-66: app/proposal.go#L66
Added line #L66 was not covered by tests
[warning] 69-69: app/proposal.go#L69
Added line #L69 was not covered by tests
[warning] 72-72: app/proposal.go#L72
Added line #L72 was not covered by testsgo.mod (1)
Line range hint
3-5: Fix Go version mismatch and invalid version numberThere are two critical issues with the Go version configuration:
- Go 1.23.1 doesn't exist yet (latest stable version is 1.22.1 as of April 2024)
- The module version (1.22.7) and toolchain version (1.23.1) are mismatched, which could lead to compatibility issues
Apply this fix:
go 1.22.7 -toolchain go1.23.1 +toolchain go1.22.1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)app/app.go(1 hunks)app/proposal.go(5 hunks)go.mod(5 hunks)gomod2nix.toml(7 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/proposal.go
[warning] 41-41: app/proposal.go#L41
Added line #L41 was not covered by tests
[warning] 50-55: app/proposal.go#L50-L55
Added lines #L50 - L55 were not covered by tests
[warning] 58-64: app/proposal.go#L58-L64
Added lines #L58 - L64 were not covered by tests
[warning] 66-66: app/proposal.go#L66
Added line #L66 was not covered by tests
[warning] 69-69: app/proposal.go#L69
Added line #L69 was not covered by tests
[warning] 72-72: app/proposal.go#L72
Added line #L72 was not covered by tests
[warning] 143-143: app/proposal.go#L143
Added line #L143 was not covered by tests
[warning] 149-154: app/proposal.go#L149-L154
Added lines #L149 - L154 were not covered by tests
[warning] 186-186: app/proposal.go#L186
Added line #L186 was not covered by tests
🔇 Additional comments (10)
app/proposal.go (2)
186-189: LGTM: Efficient proposal validation.
The update to use nil for the transaction parameter in ValidateTransaction is correct, as it leverages the lazy decoding optimization.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 186-186: app/proposal.go#L186
Added line #L186 was not covered by tests
29-29: LGTM: Signature change enhances validation flexibility.
The addition of the []byte parameter to ValidateTx allows for efficient validation of raw transaction bytes, which aligns with the optimization goals.
Let's verify this change is consistently applied across the codebase:
#!/bin/bash
# Search for any remaining old-style ValidateTx calls that might need updating
ast-grep --pattern 'ValidateTx($x)'Also applies to: 32-37
go.mod (3)
44-44: Verify security implications of dependency updates
The updates to security-sensitive packages need verification:
golang.org/x/crypto→ v0.28.0google.golang.org/grpc→ v1.67.1
Also applies to: 46-46
259-259: Review implications of using custom repository forks
The project is using custom forks for critical dependencies:
- cosmos-sdk → crypto-org-chain/cosmos-sdk
- cometbft → crypto-org-chain/cometbft
- ethermint → crypto-org-chain/ethermint
This approach requires careful maintenance to:
- Keep track of upstream security fixes
- Maintain compatibility with the ecosystem
- Handle potential breaking changes
Consider:
- Documenting the reasons for using forks
- Setting up automated monitoring for upstream changes
- Planning for eventual migration back to upstream versions
Also applies to: 271-272, 278-278
193-193: Verify compatibility of golang.org/x/ package versions*
The updates to golang.org/x/* packages should maintain version alignment:
- sys v0.26.0
- term v0.25.0
- text v0.19.0
These version differences might indicate potential compatibility issues.
Also applies to: 238-240
gomod2nix.toml (3)
656-657: Standard library updates look appropriate.
The updates to golang.org/x packages follow semantic versioning and are standard updates:
- crypto: v0.28.0
- sys: v0.26.0
- term: v0.25.0
- text: v0.19.0
Also applies to: 674-675, 677-678, 680-681
519-520: Minor version bumps for monitoring and RPC dependencies.
The updates to prometheus client (v1.20.5) and gRPC (v1.67.1) are minor version bumps that should maintain backward compatibility.
Also applies to: 701-702
161-163: Verify stability of development versions and forks.
The changes include using development versions and forks for critical dependencies:
- cometbft:
v0.0.0-20241101071018-f2b043f6eebe - cosmos-sdk:
v0.50.6-0.20241101065553-a70c19a03a81 - ethermint:
v0.6.1-0.20241101053009-8afdc6924206
While using forks allows for custom modifications, it requires careful tracking of upstream changes and potential merge conflicts.
Also applies to: 177-178, 266-267
✅ Verification successful
Let me analyze the fork information:
-
cometbft fork:
- Last updated: Oct 23, 2024
- Parent: cometbft/cometbft
-
cosmos-sdk fork:
- Last updated: Apr 26, 2024
- Parent: cosmos/cosmos-sdk
-
ethermint fork:
- Last updated: Nov 1, 2024
- Parent: evmos/ethermint
Let's verify if these development versions correspond to the latest commits in these forks:
Let's verify if these are the latest commits in each fork:
Development versions are pinned to specific commits and actively maintained
The development versions referenced in gomod2nix.toml are pinned to specific commits that are:
- All verifiable in their respective forks
- Part of actively maintained forks (last updates: ethermint Nov 1, cometbft Oct 23, cosmos-sdk Apr 26)
- Using intentionally chosen commits rather than latest main branch commits
While using development versions requires careful tracking, the current setup shows a controlled approach with specific commit pins rather than floating versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the forked repositories are in sync with upstream
# Test: Look for recent commits in the forks that haven't been merged upstream
# Check cometbft fork
echo "Checking cometbft fork..."
gh repo view crypto-org-chain/cometbft --json updatedAt,parent
# Check cosmos-sdk fork
echo "Checking cosmos-sdk fork..."
gh repo view crypto-org-chain/cosmos-sdk --json updatedAt,parent
# Check ethermint fork
echo "Checking ethermint fork..."
gh repo view crypto-org-chain/ethermint --json updatedAt,parent
Length of output: 849
Script:
#!/bin/bash
# Check if the commit hashes in gomod2nix.toml match the latest commits in the forks
# Extract commit hashes from version strings
echo "Checking cometbft commit..."
gh api repos/crypto-org-chain/cometbft/commits/f2b043f6eebe --jq '.sha'
echo "Checking cosmos-sdk commit..."
gh api repos/crypto-org-chain/cosmos-sdk/commits/a70c19a03a81 --jq '.sha'
echo "Checking ethermint commit..."
gh api repos/crypto-org-chain/ethermint/commits/8afdc6924206 --jq '.sha'
Length of output: 542
Script:
#!/bin/bash
# Check if these commits are the latest in their respective branches
echo "Latest cometbft commit:"
gh api repos/crypto-org-chain/cometbft/commits/main --jq '.sha'
echo "Latest cosmos-sdk commit:"
gh api repos/crypto-org-chain/cosmos-sdk/commits/main --jq '.sha'
echo "Latest ethermint commit:"
gh api repos/crypto-org-chain/ethermint/commits/main --jq '.sha'
Length of output: 494
CHANGELOG.md (1)
14-14: LGTM: Changelog entry accurately reflects the PR changes.
The changelog entry properly documents both performance optimizations mentioned in the PR objectives:
- async fireEvents
- faster prepare proposal when using NopMempool
app/app.go (1)
423-423: Performance optimization: Using fast proposal handler
The change from NewDefaultProposalHandler to NewDefaultProposalHandlerFast aligns with the PR objective of optimizing proposal preparation when using NopMempool. This should improve performance by using a more efficient implementation.
Let's verify the implementation of the fast proposal handler:
✅ Verification successful
Performance optimization confirmed with additional validation
The switch to NewDefaultProposalHandlerFast is part of a well-structured optimization that:
- Uses a faster proposal handler from the Cosmos SDK
- Extends transaction validation through a custom
ExtTxSelectorthat adds extra security checks - Maintains compatibility with the existing mempool while improving performance
The implementation is properly integrated with additional validation logic through ExtTxSelector, which wraps the default selector and adds custom transaction validation, making this a safe optimization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of NewDefaultProposalHandlerFast to understand the optimizations
rg -A 10 "func NewDefaultProposalHandlerFast"
Length of output: 47
Script:
#!/bin/bash
# Let's try searching in a broader context to find the proposal handler implementation
rg -i "defaultproposalhandlerfast"
# Also search for any related proposal handler types or interfaces
rg -i "proposalhandler"
# Let's also check for any imports that might contain the implementation
rg "baseapp" app/app.go
Length of output: 3215
Script:
#!/bin/bash
# Let's check the import path to locate baseapp's implementation
rg -l "cosmos-sdk/baseapp"
# Let's also check for any documentation or comments about the proposal handlers
rg -B 3 -A 3 "NewDefaultProposalHandlerFast|NewDefaultProposalHandler"
# Let's examine the ExtTxSelector implementation since it's used with the fast handler
rg -A 10 "func NewExtTxSelector"
Length of output: 1266
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/proposal_test.go (1)
13-20: Enhance mock implementation documentation and initialization.The current mock implementation could benefit from:
- Documentation explaining the purpose of the mock and why returning unmodified transactions is sufficient for testing
- Proper initialization check for the embedded TxSelector
Consider applying these improvements:
type mockTxSelector struct { baseapp.TxSelector } +// SelectTxForProposalFast implements a test-specific version that returns unmodified +// transactions. This is sufficient for testing the ExtTxSelector's validation logic +// as the actual selection is handled by the validation function in the test cases. func (mts *mockTxSelector) SelectTxForProposalFast(ctx context.Context, txs [][]byte) [][]byte { - // For testing purposes, simply return the txs as is + if mts.TxSelector == nil { + panic("mockTxSelector: TxSelector not initialized") + } return txs }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/proposal_test.go(1 hunks)
🔇 Additional comments (2)
app/proposal_test.go (2)
1-11: LGTM: Package and imports are well-structured.
The necessary testing dependencies are properly imported, and the package declaration is correct.
1-74: Verify test coverage aligns with optimization goals.
The test file implements validation for the proposal preparation optimization mentioned in the PR objectives. However, let's verify that all optimization-related code paths are covered.
✅ Verification successful
Test coverage adequately verifies optimization implementation
The test file properly covers the SelectTxForProposalFast optimization with comprehensive test cases:
- Empty transaction list handling
- All valid transactions scenario
- All invalid transactions filtering
- Mixed valid/invalid transactions processing
- Edge cases with invalid transactions at boundaries
The implementation in proposal.go shows the optimization matches the test coverage:
- Fast path for empty blocklist
- Efficient filtering of invalid transactions
- Proper delegation to parent selector for final selection
The test coverage aligns well with the optimization goals mentioned in the PR objectives, providing sufficient verification of the proposal preparation improvements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related proposal preparation and NopMempool code
echo "Searching for related proposal preparation code..."
rg -l "SelectTxForProposal|NopMempool" --type go
echo "Checking for any TODO or FIXME comments related to optimizations..."
rg -i "todo.*optim|fixme.*optim" --type go
Length of output: 388
Script:
#!/bin/bash
# Let's examine the implementation details in proposal.go
echo "=== Content of proposal.go ==="
cat app/proposal.go
# Let's also check for any other test files that might be related
echo -e "\n=== Searching for other test files with 'proposal' or 'mempool' in their name ==="
fd -t f ".*test.*" -E "*_test.go" | grep -i "proposal\|mempool"
# Check for any mempool-related implementations
echo -e "\n=== Searching for mempool implementations ==="
rg "type.*Mempool" --type go
Length of output: 5676
Signed-off-by: yihuang <[email protected]>
Solution:
👮🏻👮🏻👮🏻 !!!! 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
These changes enhance the application's functionality and performance, providing a more reliable user experience.