Skip to content
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

[RelayMiner] Supplier rate limiting #895

Merged
merged 80 commits into from
Nov 13, 2024
Merged

[RelayMiner] Supplier rate limiting #895

merged 80 commits into from
Nov 13, 2024

Conversation

red-0ne
Copy link
Contributor

@red-0ne red-0ne commented Oct 28, 2024

Summary

This PR adds an optimistic RelayMeter to the RelayMiner to monitor the consumed stake and stops servicing when maxStake is reached.

It works by

  • Intercepting RelayRequests before being served.
  • Assume the relay will be mined and deduce the corresponding amount from the consumed stake.
  • Send a rate-limited error if app stake share cannot cover of a minable RelayRequest.
  • After a relay has been served check if the Relay is volume applicable and revert the initial deduction if it is not.
  • Updates the known Applications stake if an EventApplicationStaked is observed.
  • Resets the Applications stakes at the beginning of every session.

Issue

RelayMiners need to know how much of the Application's stake they are allowed to consume without over-servicing.
image

Type of change

Select one or more from the following:

  • New feature, functionality or library

Testing

  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have commented my code
  • I have performed a self-review of my own code; both comments & source code

Olshansk and others added 30 commits August 15, 2024 21:39
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@red-0ne Please review all my comments (at a glance) before targeting them one-by-one. Something things made sense only when I finished reviewing anything.

@bryanchriswhite Want to make sure we get your 👀 on this as well.

config.yml Show resolved Hide resolved
pkg/relayer/interface.go Outdated Show resolved Hide resolved
pkg/relayer/interface.go Show resolved Hide resolved
pkg/relayer/interface.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/relay_meter.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/relay_meter.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/relay_meter.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/relay_meter.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/relay_meter.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/relay_meter.go Outdated Show resolved Hide resolved
@red-0ne red-0ne requested a review from Olshansk November 6, 2024 23:15
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@red-0ne Looking good to me w/ no blockers.

  1. PTAL at the lingering nits.
  2. Lmk what you want to do with the unit tests.
  3. We definitely need a review from @bryanchriswhite given the overlap w/ settlement hardening.

config.yml Outdated Show resolved Hide resolved
x/tokenomics/keeper/token_logic_modules.go Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Show resolved Hide resolved
pkg/relayer/proxy/relay_meter.go Show resolved Hide resolved
pkg/relayer/proxy/relay_meter.go Show resolved Hide resolved
pkg/relayer/proxy/relay_meter.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/relay_meter.go Outdated Show resolved Hide resolved

// In case Allowance is positive, add it to the maxCoin to allow no or limited over-servicing.
if !allowUnlimitedOverServicing {
maxAllowedOverServicing := appRelayMeter.maxCoin.Add(rmtr.overServicingAllowanceCoins)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could (should?) this be a value that we cache on initialization or new sessions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be no need to caching or initialization, this is the only place where it's being used.

The maxCoin is updated at the beginning of each session, while overServicingAllowanceCoins are configured at startup and unchanged.

pkg/relayer/proxy/relay_meter.go Show resolved Hide resolved
pkg/relayer/proxy/relay_meter.go Outdated Show resolved Hide resolved
Copy link
Contributor

@bryanchriswhite bryanchriswhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clean and well commented! 🙌 🚀

pkg/relayer/miner/miner.go Outdated Show resolved Hide resolved
pkg/relayer/miner/miner_test.go Outdated Show resolved Hide resolved
pkg/relayer/miner/miner_test.go Outdated Show resolved Hide resolved
pkg/relayer/miner/miner_test.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/proxy.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/token_logic_modules.go Show resolved Hide resolved
x/tokenomics/keeper/token_logic_modules.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/token_logic_modules.go Outdated Show resolved Hide resolved
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@red-0ne Left one comment that really requies attention but approving a prior otherwise!

x/shared/types/session.go Outdated Show resolved Hide resolved
@bryanchriswhite
Copy link
Contributor

@red-0ne I'm confident that you addressed the comments from my prior review and then some. 💪 @Olshansk approved and I'm too distracted with other stuff to re-review this in a timely manner. Please merge! 🙌 🚢

(@Olshansk also approves this message 😉)

@Olshansk
Copy link
Member

(@Olshansk also approves this message 😉)

IMG_7893

@red-0ne red-0ne dismissed bryanchriswhite’s stale review November 13, 2024 21:17

Can't re-review in time.

@red-0ne red-0ne merged commit fac4d6f into main Nov 13, 2024
11 checks passed
bryanchriswhite added a commit that referenced this pull request Nov 14, 2024
…pdates

* pokt/main: (51 commits)
  [RelayMiner] Supplier rate limiting (#895)
  [Tokenomics] refactor: `MintAllocationApplication` var usage to param usage  (#918)
  [Tokenomics] feat: add `mint_allocation_application` param to tokenomics module (#917)
  [Tokenomics] refactor: `MintAllocationSourceOwner` var usage to param usage (#916)
  [Tokenomics] feat: add `mint_allocation_source_owner` param to tokenomics module (#915)
  [Tokenomics] refactor: `MintAllocationSupplier` var usage to param usage (#914)
  [Tokenomics] feat: add `mint_allocation_supplier` param to tokenomics module (#913)
  [Tokenomics] refactor: `MintAllocationProposer` var usage to param usage (#912)
  [Tokenomics] feat: add `mint_allocation_proposer` param to tokenomics module (#911)
  [Tokenomics] refactor: `MintAllocationDao` var usage to param usage (#910)
  [Tokenomics] Add `mint_allocation_dao` tokenomics module param (#909)
  [quick-pr] change full-node.sh script links (#920)
  [Supplier] refactor: `NumSuppliersPerSession` var usage to param usage (#908)
  [Session Params] Add `num_suppliers_per_session` param to session module (#907)
  [Session Params] Add `MsgUpdateParam` to session module (#906)
  Updated some compiled proto files
  [TODOs] Update #2 to `TODO_BETA` to only reflect ACTUAL blockers (#900)
  [Workflow] Enforce `TODO_UPNEXT` comments to have an associated username (#896)
  [Observability] Foundation for load testing telemetry (#832)
  [Tokenomics] Implement Global Mint Reimbursement Request (#878)
  ...
bryanchriswhite added a commit that referenced this pull request Nov 14, 2024
…tlement

* pokt/main:
  [RelayMiner] Supplier rate limiting (#895)
  [Tokenomics] refactor: `MintAllocationApplication` var usage to param usage  (#918)
  [Tokenomics] feat: add `mint_allocation_application` param to tokenomics module (#917)
  [Tokenomics] refactor: `MintAllocationSourceOwner` var usage to param usage (#916)
  [Tokenomics] feat: add `mint_allocation_source_owner` param to tokenomics module (#915)
  [Tokenomics] refactor: `MintAllocationSupplier` var usage to param usage (#914)
  [Tokenomics] feat: add `mint_allocation_supplier` param to tokenomics module (#913)
  [Tokenomics] refactor: `MintAllocationProposer` var usage to param usage (#912)
  [Tokenomics] feat: add `mint_allocation_proposer` param to tokenomics module (#911)
  [Tokenomics] refactor: `MintAllocationDao` var usage to param usage (#910)
  [Tokenomics] Add `mint_allocation_dao` tokenomics module param (#909)
  [quick-pr] change full-node.sh script links (#920)
  [Supplier] refactor: `NumSuppliersPerSession` var usage to param usage (#908)
  [Session Params] Add `num_suppliers_per_session` param to session module (#907)
  [Session Params] Add `MsgUpdateParam` to session module (#906)
  Updated some compiled proto files
  [TODOs] Update #2 to `TODO_BETA` to only reflect ACTUAL blockers (#900)
  [Workflow] Enforce `TODO_UPNEXT` comments to have an associated username (#896)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devnet devnet-test-e2e off-chain Off-chain business logic push-image CI related - pushes images to ghcr.io relayminer Changes related to the Relayminer supplier Changes related to the Supplier actor
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants