-
Notifications
You must be signed in to change notification settings - Fork 8
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
[Relay Mining] Use per-service difficulty in the relayer #745
Conversation
…ficulty-per-service
WalkthroughThis update introduces significant enhancements across multiple components of the system, focusing on improved service management, tokenomics querying, and error handling. Key additions include new interfaces and query clients for tokenomics, refined miner functionalities, and robust testing infrastructure. The changes streamline data retrieval processes and enhance the application's capability to handle service-related computations, contributing to a more modular and maintainable codebase. Changes
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 Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
testutil/testclient/testqueryclients/tokenomicsquerier.go (1)
18-22
: Ensure thread-safe access torelayDifficultyTargets
.The
relayDifficultyTargets
map is accessed without any synchronization mechanisms, which could lead to race conditions if accessed concurrently. Consider using async.Mutex
orsync.RWMutex
to protect access to this map.
- File:
testutil/testclient/testqueryclients/tokenomicsquerier.go
- Lines: Access operations on
relayDifficultyTargets
(e.g., lines involvingrelayDifficultyTargets[serviceId]
).Analysis chain
Initialize
relayDifficultyTargets
safely.The
relayDifficultyTargets
map is initialized in theinit
function. Ensure that this map is thread-safe if accessed concurrently in tests, possibly by using sync mechanisms if needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for concurrent access to `relayDifficultyTargets`. # Test: Search for concurrent access patterns. Expect: Safe access patterns or synchronization mechanisms. rg --type go 'relayDifficultyTargets'Length of output: 799
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one @adshmh! 🔥
Co-authored-by: Bryan White <[email protected]>
Co-authored-by: Bryan White <[email protected]>
Co-authored-by: Bryan White <[email protected]>
Co-authored-by: Bryan White <[email protected]>
Co-authored-by: Bryan White <[email protected]>
Co-authored-by: Bryan White <[email protected]>
Co-authored-by: Bryan White <[email protected]>
Co-authored-by: Bryan White <[email protected]>
Co-authored-by: Bryan White <[email protected]>
Co-authored-by: Bryan White <[email protected]>
Co-authored-by: Bryan White <[email protected]>
…-service' into feat-relay-mining-difficulty-per-service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adshmh Great job. This was squeaky clean!
Doing a preliminary approval but just make sure to:
- See my suggestions on comments & tests
- I left a couple questions that may need a TODO added
- After (1) & (2), make sure to run E2E tests by adding the
devnet-test-e2e
and runningmake trigger_ci
after like ten seconds - Feel free to merge after if everything is 🟢
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
…ficulty-per-service
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 745) |
…ke-transfer * pokt/main: [Relay Mining] Use per-service difficulty in the relayer (#745) [Tilt] Enable rest service by default in Tilt (#793) [Docs] Minor doc updates across the board (#792) build(deps): bump webpack from 5.89.0 to 5.94.0 in /docusaurus (#775) [Docs] Tokenomics Documentation (#750) fix: ensure stable_marshaler_all option on all proto files (#772) build(deps): bump micromatch from 4.0.5 to 4.0.8 in /docusaurus (#774) [Tooling] Add `protocheck` CLI tool (#770) Update `adding_params.md` to use `ignite` (#764) [Docs] Cleanup & Deprecation (#766)
…actor/transfer-msg_period-param * issues/657/feat/app-stake-transfer: chore: regenerate protobufs [Relay Mining] Use per-service difficulty in the relayer (#745) [Tilt] Enable rest service by default in Tilt (#793) [Docs] Minor doc updates across the board (#792) build(deps): bump webpack from 5.89.0 to 5.94.0 in /docusaurus (#775) [Docs] Tokenomics Documentation (#750) fix: ensure stable_marshaler_all option on all proto files (#772) build(deps): bump micromatch from 4.0.5 to 4.0.8 in /docusaurus (#774) [Tooling] Add `protocheck` CLI tool (#770) Update `adding_params.md` to use `ignite` (#764) [Docs] Cleanup & Deprecation (#766)
…ues/657/chore/app-transfer-period * issues/657/refactor/transfer-msg_period-param: chore: regenerate protobufs chore: regenerate protobufs fix: failing test chore: regenerate protobufs Revert "refactor: rename param to application_transfer_and_unboding_period_sessions" [Relay Mining] Use per-service difficulty in the relayer (#745) [Tilt] Enable rest service by default in Tilt (#793) [Docs] Minor doc updates across the board (#792) build(deps): bump webpack from 5.89.0 to 5.94.0 in /docusaurus (#775) [Docs] Tokenomics Documentation (#750) fix: ensure stable_marshaler_all option on all proto files (#772) build(deps): bump micromatch from 4.0.5 to 4.0.8 in /docusaurus (#774) [Tooling] Add `protocheck` CLI tool (#770) Update `adding_params.md` to use `ignite` (#764) [Docs] Cleanup & Deprecation (#766)
Summary
Relayer is updated to query the tokenmoics module to get the relay difficulty target hash specific to the service corresponding to the relay. This is used to determine if a relay is volume applicable.
Relayer is updated to query the service module to get the compute units per relay specific to the service corresponding to the relay. This is used as the weight of the relay when updating the Relayer's SMT for the session corresponding to the relay.
Claims are validated at the time of submission to ensure the total claimed compute units is equal to the number of relays multiplied by the compute units per relay for the corresponding service.
All [Relay Mining] Maintain per service difficulty on the relayer #705 references in the code have been addressed.
Relay test fixture generator has been updated to use the difficulty threshold hash to be consistent with how the relayer now operates.
Issue
Relayer should use a service-specific difficulty target hash.
Relayer should use the service-specific ComputeUnitsPerRelay when adding a relay to the SMT for claiming.
[Relay Mining] Maintain per service difficulty on the relayer #705
UPNEXT
TODO_FOLLOWUP(@olshansk, #690)
These have been skipped to: 1) align on the PR before making further changes and 2) restrict the PR size and scope, as these were not strictly required for the above enhancements to function.
Depending on the review results, I can add commits to address the above in this PR as well.
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_test
make test_e2e
PR Testing (only if making code changes)
devnet-test-e2e
label to the PR.make trigger_ci
if you want to re-trigger tests without any code changesSanity Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores