Skip to content

feat(staking,state): IIP-59 commissionRate fields + feature flag (PR 1/5)#4859

Closed
envestcc wants to merge 1 commit into
iotexproject:masterfrom
envestcc:iip-59/pr1-fields
Closed

feat(staking,state): IIP-59 commissionRate fields + feature flag (PR 1/5)#4859
envestcc wants to merge 1 commit into
iotexproject:masterfrom
envestcc:iip-59/pr1-fields

Conversation

@envestcc

Copy link
Copy Markdown
Member

Summary

PR 1 of the IIP-59 series (protocol-native voter reward distribution; replaces the centralized Hermes service). This PR adds the schema fields and feature gate; no runtime behavior changes — fields are populated as zero on existing chain data and default behavior matches today exactly.

Series

# PR Status
0 iotex-proto: SetCommissionRate + Candidate.commissionRate iotexproject/iotex-proto#174 (draft, prerequisite)
1 This PR — fields + feature flag here
2 per-voter weight view (incremental, in-memory) TODO
3 SetCommissionRate action + handler TODO
4 distributeVoterReward + PutPollResult snapshot TODO
5 multi-node stress test + e2e harness TODO

Changes

Schema:

  • stakingpb.Candidate gains commissionRate = 11 + commissionRateLastEpoch = 12; Go staking.Candidate mirrors them. Equal/Clone/toProto/fromProto all updated (the original PoC at feat(iip-59): protocol-native voter reward distribution #4811 missed Equal — flagged in review Batch update 2019-04-27 6PM PDT #2 of that PR).
  • state.Candidate gains CommissionRate. The latest user-set value lives on staking.Candidate (mutable); state.Candidate.CommissionRate is the per-epoch frozen snapshot consumed by GrantEpochReward (set by PutPollResult in PR 4).
  • iotextypes.Candidate.commissionRate is set/read in candidateToPb / pbToCandidate so the new field travels through poll snapshots and over the wire.

Feature flag:

  • FeatureCtx.EnableVoterRewardDistribution, bound to g.IsToBeEnabled(height) per AGENTS.md convention for WIP features. The gate will be swapped for a real hardfork height at release time.

Toolchain:

  • stakingpb regenerated with protoc-gen-go v1.26.0 to match the version recorded in the existing staking.pb.go header (minimizes diff noise).

Local dev (transient):

  • go.mod replace pointing at ../iotex-proto so the build resolves the new iotextypes fields prior to iotex-proto PR add validation to minicluster integrate test #174 being merged/tagged. Remove the replace before final merge once iotex-proto cuts a release containing the SetCommissionRate + Candidate.commissionRate additions.

Why these fields, not more

The original PoC (#4811) used three rate fields on staking.Candidate: commissionRate + pendingCommissionRate + commissionRateLastEpoch, with explicit pending→active promotion logic. This PR instead uses only two fields on staking.Candidate (latest value + cooldown metadata) and lets the existing poll Cur/Nxt snapshot machinery handle the current-vs-next epoch separation naturally — current epoch's rate lives on the state.Candidate snapshot in CurCandidateKey, the latest user-set value lives on staking.Candidate. No new promotion code paths.

Test plan

  • go build ./... passes with the new go.mod replace pointing at the local iotex-proto branch
  • go test ./action/protocol/... ./state/... — 935 tests pass
  • New TestCandidateCommissionRate in state/candidate_test.go verifies Equal/Clone/Proto roundtrip for state.Candidate.CommissionRate
  • New TestCandidateCommissionRate in action/protocol/staking/candidate_test.go verifies Equal/Clone/toProto/fromProto for staking.Candidate.CommissionRate + CommissionRateLastEpoch, including the explicit assertion that Equal reflects both new fields (regression test for the PoC bug)
  • Existing TestClone, TestCloneWithDeactivation, and TestCandidate* continue to pass — no regression in the existing schema

Behavior gating

🤖 Generated with Claude Code

// 0 = legacy (no auto-distribution).
uint64 commissionRate = 11;
// IIP-59: epoch number of the last SetCommissionRate (cooldown enforcement).
uint64 commissionRateLastEpoch = 12;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

why commissionRateLastEpoch is needed, and what it will be used for?

Comment thread action/protocol/context.go Outdated
// distribution. When true, GrantEpochReward auto-splits each delegate's
// epoch reward between the delegate's commission and a proportional
// distribution to voters, and SetCommissionRate actions are accepted.
EnableVoterRewardDistribution bool

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is best for feature flags to default to indicating the activation status of a new feature. Since the default value of bool is false, it is advisable to rename the flag to something like NoVoterRewardDistribution and document this rule in the comments, so that AI will avoid making the same mistake in the future.

@envestcc
envestcc force-pushed the iip-59/pr1-fields branch from 64e1895 to 57b3e7e Compare June 25, 2026 03:05
@envestcc

Copy link
Copy Markdown
Member Author

Good catch — IIP-59 doesn't actually prescribe a per-rate-change cooldown. Re-reading the spec:

Why Epoch-Boundary Rate Changes: Commission rate changes take effect at the next epoch boundary, not immediately. This prevents a delegate from setting 0% commission to attract voters, switching to 100% right before epoch reward distribution, switching back to 0%. Epoch-boundary enforcement gives voters time to react to rate changes.

The IIP's only protection against rapid manipulation is epoch-boundary activation. In our design, the existing poll Cur/Nxt snapshot machinery already gives us that for free: a SetCommissionRate call at any moment only affects rewards in the epoch after the next PutPollResult snapshot (~1.5 epoch reaction window). No commissionRateLastEpoch needed.

The 168-epoch cooldown in the PoC (#4811 commit fbe52e7) was production hardening the author added beyond the spec, but it's a policy decision that can land later as an additive proto change if the protocol decides it wants stronger limits.

I've dropped the field from both PRs and force-pushed:

Stacked PR #4860 (VoterWeightView) is rebased on the updated PR 1 head.

Adds the proto-level field and feature gate that the rest of the IIP-59
protocol-native voter reward distribution PRs will hang behavior off of.
No runtime behavior changes in this PR — the field is populated as zero
on existing chain data, default behavior matches today exactly.

Schema:
- stakingpb.Candidate gains commissionRate=11; Go Candidate struct mirrors
  it. Equal/Clone/toProto/fromProto updated (the original PoC missed Equal
  — flagged in iotexproject#4811 review #2).
- state.Candidate gains CommissionRate, snapshotted per epoch from the
  staking candidate state by PutPollResult (added in PR 4). The latest
  user-set value lives on staking.Candidate; state.Candidate holds the
  per-epoch frozen value consumed by GrantEpochReward.
- iotextypes.Candidate.commissionRate is set/read in candidateToPb /
  pbToCandidate so the new field travels through poll snapshots and over
  the wire.

Feature flag:
- FeatureCtx.NoVoterRewardDistribution, bound to !g.IsToBeEnabled(height)
  per AGENTS.md convention for WIP features (the gate will be swapped for
  a real hardfork height at release time).
- Named so that the bool zero value (false) corresponds to the post-fork
  activated behavior, matching the existing NoCandidateExitQueue /
  NotSlashUnproductiveDelegates convention. A docstring records this rule
  next to the field for future readers.

Why no separate commissionRateLastEpoch field:
- IIP-59 doesn't prescribe a per-rate-change cooldown. Its protection
  against rapid manipulation is epoch-boundary activation, which our
  design already provides for free: a SetCommissionRate at any moment
  only affects rewards in the epoch *after* the next PutPollResult
  snapshot, giving voters ~1.5 epochs of reaction time.
- If the protocol later decides cooldown is needed, adding the field is
  an additive proto change with no migration cost.

Toolchain:
- stakingpb regenerated with protoc-gen-go v1.26.0 to match the version
  recorded in the existing staking.pb.go header.

Local dev:
- go.mod replace pointing at ../iotex-proto so the build resolves the new
  iotex-proto fields prior to the proto PR being tagged. Remove the
  replace once iotex-proto cuts a release containing the
  SetCommissionRate + Candidate.commissionRate additions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@envestcc
envestcc force-pushed the iip-59/pr1-fields branch from 57b3e7e to f52ee59 Compare June 25, 2026 03:42
@sonarqubecloud

Copy link
Copy Markdown

@envestcc

Copy link
Copy Markdown
Member Author

Good convention — adopted. Looking at the existing flags in context.go (NoCandidateExitQueue, NotSlashUnproductiveDelegates, NotUseMinSelfStakeToBeActive, CandidateBLSPublicKeyNotCopied, etc.), they all follow the rule you're describing: name the field so the bool zero value (false) is the post-fork activated behavior, and let the explicit !g.IsXxx(height) initializer clamp it to true only during the pre-fork window. My Enable* form was inconsistent with that.

Renamed EnableVoterRewardDistributionNoVoterRewardDistribution, flipped the initializer to !g.IsToBeEnabled(height), flipped the read sites in PR #4860 to if !featureCtx.NoVoterRewardDistribution { ... }, and added a docstring on the field explaining the convention so this is captured for future AI/human readers:

// NoVoterRewardDistribution gates IIP-59: protocol-native voter reward
// distribution. When false (the post-fork / activated state, which is also
// the bool zero value), GrantEpochReward auto-splits each delegate's
// epoch reward between the delegate's commission and a proportional
// distribution to voters, and SetCommissionRate actions are accepted.
// When true (the pre-fork legacy state), the protocol keeps today's
// behavior of granting the full epoch reward to the delegate.
//
// Naming convention: feature flags should be named such that the bool
// zero value (false) corresponds to the post-fork activated behavior.
// This matches NoCandidateExitQueue / NotSlashUnproductiveDelegates and
// makes a missing / partially-initialized FeatureCtx default to the
// long-term mainnet behavior rather than the temporary pre-fork state.
NoVoterRewardDistribution bool

Both branches force-pushed:

@envestcc

envestcc commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

Closing as part of IIP-59 PR reorganization. The proposal was amended (iotexproject/iips#73) to cover both block reward + epoch reward and to base voter distribution on a per-epoch snapshot. The new PR split is:

The incremental voter weight view (old #4860 + #4863) is dropped in favor of a per-PutPollResult full scan; ~40k buckets sorted+encoded once per epoch fits well inside the mint budget and eliminates the 13-handler-hook consensus-safety surface. Old #4864 (voter reward distribution) is replaced by new PR 3 which reads the snapshot instead of the live view.

Superseded by new PRs — will link once opened.

@envestcc envestcc closed this Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant