[TODOs] refactor: proof path calculation #659
Conversation
bryanchriswhite
added
protocol
WalkthroughThe changes introduce a new Changes
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
changed the base branch from
issues/584/refactor/count-difficulty-bits
to
main
bryanchriswhite
force-pushed
the
issues/584/refactor/proot-path
branch
from
1429fe8 to
6d831f7
Compare
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 659) |
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- pkg/crypto/protocol/proof_path.go (1 hunks)
- pkg/relayer/session/proof.go (3 hunks)
- tests/integration/tokenomics/relay_mining_difficulty_test.go (4 hunks)
- x/proof/keeper/msg_server_submit_proof.go (5 hunks)
- x/proof/keeper/msg_server_submit_proof_test.go (13 hunks)
- x/tokenomics/keeper/update_relay_mining_difficulty.go (2 hunks)
Additional comments not posted (22)
pkg/crypto/protocol/proof_path.go (1)
15-22: LGTM! Verify SMT specification initialization.The initialization logic for the SMT specification looks good. Ensure that the
SmtSpecis correctly used in the rest of the codebase.x/tokenomics/keeper/update_relay_mining_difficulty.go (2)
13-13: LGTM! Verify usage ofprotocol.SmtSpec.The logic for updating relay mining difficulty looks good. Ensure that the
protocol.SmtSpecis correctly used throughout the function.
150-152: LGTM! Verify usage ofprotocol.SmtSpec.The logic for returning the default difficulty target hash looks good. Ensure that the
protocol.SmtSpecis correctly used in this function.tests/integration/tokenomics/relay_mining_difficulty_test.go (2)
32-32: LGTM! Verify test coverage.The test logic for updating relay mining difficulty for a new service looks good. Ensure that the test covers all relevant scenarios.
206-215: LGTM! Verify proof generation logic.The proof generation logic looks good. Ensure that the proof is correctly generated and used in the tests.
pkg/relayer/session/proof.go (2)
8-8: LGTM! Verify proof submission logic.The logic for submitting proofs looks good. Ensure that the proof submission is correctly handled and tested.
Line range hint
219-241:
LGTM! Verify proof generation logic.The proof generation logic looks good. Ensure that the proof is correctly generated and used in the proof submission process.
x/proof/keeper/msg_server_submit_proof.go (4)
154-154: Correct usage ofprotocol.SmtSpecinGetValueHash.The change to use
protocol.SmtSpecinGetValueHashis correct and aligns with the centralization of SMT specifications.
517-520: Correct usage ofprotocol.GetPathForProof.The change to use
protocol.GetPathForProoffor proof path validation is correct and aligns with the centralization of proof path calculations.
443-443: Correct usage ofprotocol.SmtSpecinVerifyClosestProof.The change to use
protocol.SmtSpecinVerifyClosestProofis correct and aligns with the centralization of SMT specifications.
Line range hint
197-198:
Correct usage ofprotocol.GetPathForProof.The change to use
protocol.GetPathForProoffor computing the expected proof path is correct and aligns with the centralization of proof path calculations.x/proof/keeper/msg_server_submit_proof_test.go (11)
58-58: Correct usage ofprotocol.GetPathForProofininit.The change to use
protocol.GetPathForProofin theinitfunction is correct and aligns with the centralization of proof path calculations.
76-76: Correct usage ofshared.GetEarliestSupplierProofCommitHeight.The change to use
shared.GetEarliestSupplierProofCommitHeightfor proof message height calculation is correct and aligns with the new logic.
166-169: Correct block height advancement for claim message height.The change to advance the block height for claim message height is correct and ensures proper timing for proof submission.
183-195: Correct block height advancement and block hash storage for proof path seed height.The changes to advance the block height to the proof path seed height and store the block hash are correct and ensure proper proof path validation.
197-198: Correct usage ofprotocol.GetPathForProoffor expected proof path calculation.The change to use
protocol.GetPathForProoffor computing the expected proof path is correct and aligns with the centralization of proof path calculations.
201-201: Correct block height advancement for proof message height.The change to advance the block height for proof message height is correct and ensures proper timing for proof submission.
406-406: Correct usage ofkeepertest.WithBlockHash.The change to use
keepertest.WithBlockHashfor block hash setup is correct and ensures proper proof path validation.
520-520: Correct block height advancement for claim message height.The change to advance the block height for claim message height is correct and ensures proper timing for proof submission.
1087-1091: Correct usage ofprotocol.GetPathForProoffor expected proof path calculation.The change to use
protocol.GetPathForProoffor computing the expected proof path is correct and aligns with the centralization of proof path calculations.
1201-1211: Correct block height advancement and block hash storage for proof path seed height.The changes to advance the block height to the proof path seed height and store the block hash are correct and ensure proper proof path validation.
1428-1428: Correct usage ofprotocol.SmtSpecinGetValueHash.The change to use
protocol.SmtSpecinGetValueHashis correct and aligns with the centralization of SMT specifications.
| // GetPathForProof computes the path to be used for proof validation by hashing | ||
| // the block hash and session id. | ||
| func GetPathForProof(blockHash []byte, sessionId string) []byte { | ||
| hasher := newHasher() | ||
| if _, err := hasher.Write(append(blockHash, []byte(sessionId)...)); err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| return hasher.Sum(nil) |
There was a problem hiding this comment.
Consider handling potential errors more gracefully.
The hash computation logic looks good. However, consider handling potential errors more gracefully instead of using panic.
- panic(err)
+ log.Fatalf("failed to write hash: %v", err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // GetPathForProof computes the path to be used for proof validation by hashing | |
| // the block hash and session id. | |
| func GetPathForProof(blockHash []byte, sessionId string) []byte { | |
| hasher := newHasher() | |
| if _, err := hasher.Write(append(blockHash, []byte(sessionId)...)); err != nil { | |
| panic(err) | |
| } | |
| return hasher.Sum(nil) | |
| // GetPathForProof computes the path to be used for proof validation by hashing | |
| // the block hash and session id. | |
| func GetPathForProof(blockHash []byte, sessionId string) []byte { | |
| hasher := newHasher() | |
| if _, err := hasher.Write(append(blockHash, []byte(sessionId)...)); err != nil { | |
| log.Fatalf("failed to write hash: %v", err) | |
| } | |
| return hasher.Sum(nil) |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- pkg/relayer/session/proof.go (3 hunks)
- x/proof/keeper/msg_server_submit_proof.go (5 hunks)
- x/tokenomics/keeper/update_relay_mining_difficulty.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- pkg/relayer/session/proof.go
- x/proof/keeper/msg_server_submit_proof.go
- x/tokenomics/keeper/update_relay_mining_difficulty.go
|
E2E tests passing locally but failed to re-run in CI:
|
…ent-balances * pokt/main: [TODOs] refactor: proof path calculation (#659) [Dependencies] bump go-getter and ibc-go (#691) [Relayminer] refactor: `relayerSessionsManager#waitForBlock()` (#648) [Observables] chore: add `ReplayObservable#SubscribeFromLatestBufferedOffset()` (#647) [Observability] Add claim relays counter (#644) [Code Health] chore: log unused error when updating relay mining difficulty (#683) [Testing] chore: uncomment proof CLI query tests (#668) build(deps): bump ws from 7.5.9 to 7.5.10 in /docusaurus (#686) build(deps): bump webpack-dev-middleware from 5.3.3 to 5.3.4 in /docusaurus (#688) build(deps): bump express from 4.18.2 to 4.19.2 in /docusaurus (#687) build(deps): bump follow-redirects from 1.15.3 to 1.15.6 in /docusaurus (#685) build(deps): bump braces from 3.0.2 to 3.0.3 in /docusaurus (#689) [CosmosSDK] Bump to v0.50.7 (#682)
…ation-overserviced * pokt/main: [TODOs] refactor: proof path calculation (#659) [Dependencies] bump go-getter and ibc-go (#691) [Relayminer] refactor: `relayerSessionsManager#waitForBlock()` (#648) [Observables] chore: add `ReplayObservable#SubscribeFromLatestBufferedOffset()` (#647) [Observability] Add claim relays counter (#644) [Code Health] chore: log unused error when updating relay mining difficulty (#683) [Testing] chore: uncomment proof CLI query tests (#668) build(deps): bump ws from 7.5.9 to 7.5.10 in /docusaurus (#686) build(deps): bump webpack-dev-middleware from 5.3.3 to 5.3.4 in /docusaurus (#688) build(deps): bump express from 4.18.2 to 4.19.2 in /docusaurus (#687) build(deps): bump follow-redirects from 1.15.3 to 1.15.6 in /docusaurus (#685) build(deps): bump braces from 3.0.2 to 3.0.3 in /docusaurus (#689) [CosmosSDK] Bump to v0.50.7 (#682)
…ation-use-index * pokt/main: [TODOs] refactor: proof path calculation (#659) [Dependencies] bump go-getter and ibc-go (#691) [Relayminer] refactor: `relayerSessionsManager#waitForBlock()` (#648) [Observables] chore: add `ReplayObservable#SubscribeFromLatestBufferedOffset()` (#647) [Observability] Add claim relays counter (#644) [Code Health] chore: log unused error when updating relay mining difficulty (#683) [Testing] chore: uncomment proof CLI query tests (#668) build(deps): bump ws from 7.5.9 to 7.5.10 in /docusaurus (#686) build(deps): bump webpack-dev-middleware from 5.3.3 to 5.3.4 in /docusaurus (#688) build(deps): bump express from 4.18.2 to 4.19.2 in /docusaurus (#687) build(deps): bump follow-redirects from 1.15.3 to 1.15.6 in /docusaurus (#685) build(deps): bump braces from 3.0.2 to 3.0.3 in /docusaurus (#689) [CosmosSDK] Bump to v0.50.7 (#682)
Summary
GetProofPath()static function out of the proof keeper pkg intopkg/crypto/protocol. This should streamline the relayminer's dependency tree.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
Refactor
protocolpackage.Tests
These changes enhance the cryptographic proof verification process, improving security and reliability.