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

[Supplier] Initial slashing implementation #795

Merged
merged 21 commits into from
Sep 18, 2024
Merged

Conversation

red-0ne
Copy link
Contributor

@red-0ne red-0ne commented Sep 4, 2024

Summary

This PR introduces the initial implementation of Supplier slashing:

  • Adjusts proof parameters to align with the probabilistic proofs paper.
  • Adds comments on potential follow-up improvements and adjustments.
  • Implements Supplier slashing for invalid or missing proofs.
  • Unstakes a Supplier whose stake falls below the minimum due to slashing.
  • Move the ComputeUnitsToTokenMultiplier to the shared module.

~1300LOC is auto-generated code

Issue

To enable permissionless demand, the protocol must discourage the submission of false or invalid claims.

As part of this, Suppliers should be penalized for failing to comply with the C&P life cycle rules.

Type of change

Select one or more from the following:

Testing

  • Documentation: make docusaurus_start; only needed if you make doc changes
  • 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
  • I create and reference any new tickets, if applicable
  • I have left TODOs throughout the codebase, if applicable

@red-0ne red-0ne added supplier Changes related to the Supplier actor on-chain On-chain business logic tokenomics Token Economics - what else do you need? labels Sep 4, 2024
@red-0ne red-0ne self-assigned this Sep 4, 2024
@Olshansk Olshansk added this to the Shannon Beta TestNet Launch milestone Sep 9, 2024
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.

Haven't gone through the meet yet in x/tokenomics/keeper/settle_pending_claims.go but publishing a partial review

pkg/relayer/session/claim.go Outdated Show resolved Hide resolved
app/app_config.go Show resolved Hide resolved
e2e/tests/0_settlement.feature Outdated Show resolved Hide resolved
e2e/tests/0_settlement.feature Show resolved Hide resolved
e2e/tests/session.feature Outdated Show resolved Hide resolved
x/tokenomics/keeper/keeper_settle_pending_claims_test.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/keeper_settle_pending_claims_test.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Show resolved Hide resolved
@red-0ne red-0ne added the consensus-breaking IMPORTANT! If the PR with this tag is merged, next release WILL HAVE TO BE an upgrade. label Sep 12, 2024
app/app_config.go Outdated Show resolved Hide resolved
e2e/tests/0_settlement.feature Outdated Show resolved Hide resolved
e2e/tests/0_settlement.feature Outdated Show resolved Hide resolved
e2e/tests/session.feature Outdated Show resolved Hide resolved
proto/poktroll/proof/params.proto Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Outdated Show resolved Hide resolved
proto/poktroll/tokenomics/event.proto Outdated Show resolved Hide resolved
x/tokenomics/keeper/keeper_settle_pending_claims_test.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Outdated Show resolved Hide resolved
x/tokenomics/tokenomics.go Outdated Show resolved Hide resolved
x/tokenomics/tokenomics.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/token_logic_modules.go Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Outdated Show resolved Hide resolved
@red-0ne
Copy link
Contributor Author

red-0ne commented Sep 17, 2024

@Olshansk

I added an unexpected change due to a conflict (cyclic dependency) resulting from merging main into this branch.

The ComputeUnitsToTokenMultiplier is now moved to the shared module to resolve a conflict.

The conflict itself is due to main using ProofRequirementForClaim from the proof module.

While in this branch, ProofRequirementForClaim needs the ComputeUnitsToTokenMultiplier (from tokenomics module) to check against the ProofRequirementThreshold which results in a cyclic dependency.

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.

Minor comment but LGTM otherwise!

proto/poktroll/shared/params.proto Show resolved Hide resolved
x/proof/types/shared_query_client.go Outdated Show resolved Hide resolved
Copy link

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 make trigger_ci to submit an empty commit that'll trigger the tests.

GCP workloads (requires changing the namespace to 795)
Grafana network dashboard for devnet-issue-795

@github-actions github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels Sep 18, 2024
@red-0ne red-0ne merged commit 09fc4ce into main Sep 18, 2024
10 checks passed
bryanchriswhite added a commit that referenced this pull request Sep 19, 2024
…egration-app

* pokt/main:
  [Supplier] Initial slashing implementation (#795)
  [Container image] Change base image for release (#829)
  [Proof] Prevent proof submission when not required (#822)
bryanchriswhite added a commit that referenced this pull request Sep 19, 2024
…ke-transfer

* pokt/main:
  [Supplier] Initial slashing implementation (#795)
  [Container image] Change base image for release (#829)
  [Proof] Prevent proof submission when not required (#822)
bryanchriswhite added a commit that referenced this pull request Sep 19, 2024
…actor/transfer-msg_period-param

* issues/657/feat/app-stake-transfer:
  [Supplier] Initial slashing implementation (#795)
  [Container image] Change base image for release (#829)
  [Proof] Prevent proof submission when not required (#822)
bryanchriswhite added a commit that referenced this pull request Sep 19, 2024
…ues/657/chore/app-transfer-period

* issues/657/refactor/transfer-msg_period-param:
  [Supplier] Initial slashing implementation (#795)
  [Container image] Change base image for release (#829)
  [Proof] Prevent proof submission when not required (#822)

# Conflicts:
#	app/app_config.go
bryanchriswhite added a commit that referenced this pull request Sep 19, 2024
…nt-stake-supplier

* pokt/main:
  [Supplier] Initial slashing implementation (#795)
bryanchriswhite added a commit that referenced this pull request Sep 19, 2024
…99/e2e/param-helpers

* issues/799/e2e/idempotent-stake-supplier:
  chore: add comment
  chore: sync shared module default params
  fix: failing E2E test
  [Supplier] Initial slashing implementation (#795)
  [Container image] Change base image for release (#829)
  [Proof] Prevent proof submission when not required (#822)
  chore: rename paramsAnyMap type to mitigate variable name shadowing
  [Tooling, Code Health] refactor: split Makefile into multiple files (#816)
  [Tooling] fix and add makefile targets (#814)
  [Docs] Add a debugging tip: using transaction hashes (#786)
bryanchriswhite added a commit that referenced this pull request Sep 19, 2024
…lier-stake-events

* issues/799/e2e/param-helpers:
  chore: review feedback improvements
  chore: add comment
  chore: sync shared module default params
  fix: failing E2E test
  [Supplier] Initial slashing implementation (#795)
  [Container image] Change base image for release (#829)
  [Proof] Prevent proof submission when not required (#822)
  chore: rename paramsAnyMap type to mitigate variable name shadowing
  [Tooling, Code Health] refactor: split Makefile into multiple files (#816)
  [Tooling] fix and add makefile targets (#814)
  [Docs] Add a debugging tip: using transaction hashes (#786)
bryanchriswhite added a commit that referenced this pull request Sep 19, 2024
…tests/params

* issues/799/feat/supplier-stake-events:
  chore: review feedback improvements
  chore: add comment
  chore: sync shared module default params
  fix: failing E2E test
  [Supplier] Initial slashing implementation (#795)
  [Container image] Change base image for release (#829)
  [Proof] Prevent proof submission when not required (#822)
  chore: rename paramsAnyMap type to mitigate variable name shadowing
  [Tooling, Code Health] refactor: split Makefile into multiple files (#816)
  [Tooling] fix and add makefile targets (#814)
  [Docs] Add a debugging tip: using transaction hashes (#786)
okdas pushed a commit that referenced this pull request Nov 14, 2024
## Summary

This PR introduces the initial implementation of `Supplier` slashing:
- Adjusts proof parameters to align with the probabilistic proofs paper.
- Adds comments on potential follow-up improvements and adjustments.
- Implements `Supplier` slashing for invalid or missing proofs.
- Unstakes a `Supplier` whose stake falls below the minimum due to
slashing.
- Move the `ComputeUnitsToTokenMultiplier` to the `shared` module.

_~1300LOC is auto-generated code_

## Issue

To enable permissionless demand, the protocol must discourage the
submission of false or invalid claims.

As part of this, `Suppliers` should be penalized for failing to comply
with the C&P life cycle rules.

- #758 

## Type of change

Select one or more from the following:

- [x] New feature, functionality or library
- [x] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [ ] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [x] **Unit Tests**: `make go_develop_and_test`
- [x] **LocalNet E2E Tests**: `make test_e2e`
- [x] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## Sanity Checklist

- [x] I have tested my changes using the available tooling
- [x] I have commented my code
- [x] I have performed a self-review of my own code; both comments &
source code
- [ ] I create and reference any new tickets, if applicable
- [x] I have left TODOs throughout the codebase, if applicable

---------

Co-authored-by: Daniel Olshansky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus-breaking IMPORTANT! If the PR with this tag is merged, next release WILL HAVE TO BE an upgrade. devnet devnet-test-e2e on-chain On-chain business logic push-image CI related - pushes images to ghcr.io supplier Changes related to the Supplier actor tokenomics Token Economics - what else do you need?
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants