[Code Health] refactor: simplify protocol.CountHashDifficultyBits()
#656
Conversation
WalkthroughThe updates introduce a new function Changes
Sequence Diagram(s)Sequence diagrams are not included as the changes are straightforward and function-focused without significant modifications to the control flow. Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
bryanchriswhite
added
push-image
|
The image is going to be pushed after the next commit. You can use If you also want to run E2E tests, please add |
|
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. You may need to run GCP workloads (requires changing the namespace to 656) |
bryanchriswhite
changed the title
bryanchriswhite
changed the title
protocol.CountHashDifficultyBits()
|
|
||
| // CountHashDifficultyBits returns the number of leading zero bits in the given byte | ||
| // slice. It returns an error if the byte slice is all zero bits. | ||
| func CountHashDifficultyBits(bz [32]byte) int { |
There was a problem hiding this comment.
cc @red-0ne who I believe has similar changes elsewhere.
bryanchriswhite
changed the base branch from
issues/584/refactor/miner-query-params
to
main
Co-authored-by: Daniel Olshansky <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
pkg/crypto/protocol/difficulty.go (1)
8-8: Improve documentation clarity.The documentation is clear, but it can be improved by explaining why BigEndian is chosen for bit/byte ordering.
- // CountHashDifficultyBits returns the number of leading zero bits in the given byte slice. + // CountHashDifficultyBits returns the number of leading zero bits in the given byte slice. + // BigEndian is chosen for bit/byte ordering to ensure that leading zeros accumulate across adjacent bytes.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- pkg/crypto/protocol/difficulty.go (1 hunks)
- pkg/crypto/protocol/difficulty_test.go (1 hunks)
- pkg/crypto/protocol/errors.go (1 hunks)
- pkg/relayer/miner/gen/gen_fixtures.go (2 hunks)
- pkg/relayer/miner/miner.go (2 hunks)
- x/proof/keeper/msg_server_submit_proof.go (2 hunks)
- x/proof/keeper/msg_server_submit_proof_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
- pkg/crypto/protocol/errors.go
Additional comments not posted (7)
pkg/crypto/protocol/difficulty.go (1)
9-15: LGTM!The function implementation is correct and efficient.
pkg/crypto/protocol/difficulty_test.go (1)
12-50: LGTM!The test cases are comprehensive and cover various scenarios for the
CountHashDifficultyBitsfunction.pkg/relayer/miner/miner.go (1)
115-126: LGTM!The changes are correct and consistent. The function
mapMineRelayusesCountHashDifficultyBitscorrectly.pkg/relayer/miner/gen/gen_fixtures.go (2)
203-203: LGTM!The changes are correct and consistent. The function
difficultyGTEusesCountHashDifficultyBitscorrectly.
209-209: LGTM!The changes are correct and consistent. The function
difficultyLTusesCountHashDifficultyBitscorrectly.x/proof/keeper/msg_server_submit_proof.go (1)
479-479: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
validateMiningDifficultymatch the new implementation.Verification successful
Function usage verified.
The function
validateMiningDifficultyis consistently used with the new implementation inx/proof/keeper/msg_server_submit_proof.go.
x/proof/keeper/msg_server_submit_proof.go: Verified usage ofvalidateMiningDifficultyat lines 479 and 483.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `validateMiningDifficulty` match the new implementation. # Test: Search for the function usage. Expect: Only occurrences of the new implementation. rg --type go $'validateMiningDifficulty'Length of output: 551
Script:
#!/bin/bash # Extract the definition and usage instances of `validateMiningDifficulty` in `x/proof/keeper/msg_server_submit_proof.go` # Extract the function definition echo "Function Definition:" rg --type go --context 10 'func validateMiningDifficulty' x/proof/keeper/msg_server_submit_proof.go # Extract the function usage instances echo "Function Usage Instances:" rg --type go --context 5 'validateMiningDifficulty(relayBz, params.MinRelayDifficultyBits)' x/proof/keeper/msg_server_submit_proof.goLength of output: 1247
Script:
#!/bin/bash # Extract the function usage instances of `validateMiningDifficulty` in `x/proof/keeper/msg_server_submit_proof.go` # Extract the function usage instances rg --type go --context 5 'validateMiningDifficulty' x/proof/keeper/msg_server_submit_proof.goLength of output: 1592
x/proof/keeper/msg_server_submit_proof_test.go (1)
1397-1397: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
getClosestRelayDifficultyBitsmatch the new implementation.Verification successful
Verified: The function
getClosestRelayDifficultyBitsis only used within the test filex/proof/keeper/msg_server_submit_proof_test.goand matches the new implementation.
x/proof/keeper/msg_server_submit_proof_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `getClosestRelayDifficultyBits` match the new implementation. # Test: Search for the function usage. Expect: Only occurrences of the new implementation. rg --type go $'getClosestRelayDifficultyBits'Length of output: 411
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- pkg/crypto/protocol/difficulty.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- pkg/crypto/protocol/difficulty.go
|
E2E tests passing locally but failed to run in CI: |
…#656) Co-authored-by: Daniel Olshansky <[email protected]>
Summary
Simplifies
protocol.CountHashDifficultyBits()and eliminatesMustCountHashDifficultyBits().Issue
Type of change
Select one or more:
Testing
Documentation changes (only if making doc changes)
make docusaurus_start; only needed if you make doc changesLocal Testing (only if making code changes)
make go_develop_and_testmake test_e2ePR Testing (only if making code changes)
devnet-test-e2elabel to the PR.make trigger_ciif you want to re-trigger tests without any code changesSanity Checklist
Summary by CodeRabbit
New Features
CountHashDifficultyBitsfunction to calculate leading zero bits in a hash.Refactor
CountHashDifficultyBitsfor difficulty calculations instead of the previous method.protocolto use the new subpackagecrypto/protocol.Tests
CountHashDifficultyBitsfunction to ensure accuracy in difficulty bit counting.