feat: allow defining validator power threshold in consensus params #1052
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update revises dependency installation in CI/CD (workflow and Makefile) and systematically updates how validator sets are created. Most changes add an extra parameter (often Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Application/Tests
participant VSFactory as ValidatorSet Factory
participant VS as ValidatorSet
Caller->>VSFactory: Call NewValidatorSet(validators, pubKey, llmqType, quorumHash, hasPublicKeys, additionalParam)
VSFactory-->>Caller: Returns ValidatorSet instance (with voting_power_threshold set)
Note over VS: Validates and assigns voting power threshold if provided.
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…ram-signature-threshold
…ram-signature-threshold
|
@coderabbitai review all |
|
@lklimek I'll review all the changes in the PR. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
Makefile (1)
20-22: Update GOGOPROTO_PATH to Use the Patched RepositoryThe change updates the
GOGOPROTO_PATHto point togithub.com/lklimek/gogoprotopinned to a specific commit (564fd924f58c5d076b0ad8e3f1c6fb54d065cbbe), replacing the old Cosmos version. This ensures that builds leverage the fixed version of the plugin for the gogoproto bug.Note: There is an extra space in
"go list"which may be harmless but could be tidied for consistency.types/validator_set.go (1)
160-181: Enforce the TODO check for alignment betweenvals.VotingPowerThresholdandValidatorParams.You have a TODO (line 160) to confirm
vals.VotingPowerThresholdmatches theValidatorParams. This helps detect any mismatch in threshold usage.Would you like guidance or a patch to perform that check automatically?
internal/consensus/versioned/selectproposer/height_proposer_test.go (1)
31-31: Consider setting an explicit voting power threshold.While passing
nilworks, consider setting an explicit voting power threshold to better reflect real-world validator set configurations.- }, bls12381.GenPrivKey().PubKey(), btcjson.LLMQType_5_60, crypto.RandQuorumHash(), true, nil) + }, bls12381.GenPrivKey().PubKey(), btcjson.LLMQType_5_60, crypto.RandQuorumHash(), true, types.DefaultVotingPowerThreshold)test/e2e/tests/validator_test.go (1)
189-189: Consider setting an explicit voting power threshold in test setup.While passing
nilworks for test setup, consider setting an explicit voting power threshold for better test coverage.- vs := types.NewValidatorSet(makeVals(valMap), thresholdPublicKey, quorumType, quorumHash, true, nil) + vs := types.NewValidatorSet(makeVals(valMap), thresholdPublicKey, quorumType, quorumHash, true, types.DefaultVotingPowerThreshold)internal/state/state.go (1)
384-389: Consider adding validation for validator parameters.The new
valParamsvariable is correctly initialized fromgenDoc.ConsensusParams, but it might be beneficial to add validation for the voting power threshold if it's present.Consider adding validation before using the validator parameters:
var valParams *types.ValidatorParams if genDoc.ConsensusParams != nil { valParams = &genDoc.ConsensusParams.Validator + if valParams.VotingPowerThreshold != nil { + // Ensure the threshold is within valid bounds + if *valParams.VotingPowerThreshold == 0 { + return State{}, fmt.Errorf("voting power threshold cannot be zero") + } + } }Also applies to: 390-392
internal/consensus/replayer.go (1)
354-357: Consider documenting the commented-out code.The commented-out code suggests a potential feature for updating the voting threshold of an existing validator set. Consider adding a TODO comment explaining why this code is commented out and when it might be enabled.
-// } else if valParams != nil && valParams.VotingPowerThreshold != nil { -// // we update the existing validator set with the new voting threshold -// state.Validators.VotingPowerThreshold = *valParams.VotingPowerThreshold +// TODO(#issue): Enable updating voting threshold of existing validator set +// } else if valParams != nil && valParams.VotingPowerThreshold != nil { +// // we update the existing validator set with the new voting threshold +// state.Validators.VotingPowerThreshold = *valParams.VotingPowerThresholdtypes/params.go (2)
72-73: Consider documenting the VotingPowerThreshold field.The new field would benefit from documentation explaining its purpose, valid values, and when it should be used.
type ValidatorParams struct { PubKeyTypes []string `json:"pub_key_types"` + // VotingPowerThreshold is an optional parameter that defines the minimum voting power + // required for validator set decisions. When nil, the default threshold is used. + // Must be greater than 0 when set. VotingPowerThreshold *uint64 `json:"threshold,omitempty"` }
244-280: Consider adding validation in ToProto and ValidatorParamsFromProto.The conversion methods should validate the voting power threshold to ensure it's greater than 0 when set.
func (val *ValidatorParams) ToProto() *tmproto.ValidatorParams { if val == nil { return nil } + + if val.VotingPowerThreshold != nil && *val.VotingPowerThreshold == 0 { + panic("voting power threshold cannot be zero") + } params := tmproto.ValidatorParams{ PubKeyTypes: val.PubKeyTypes, }func ValidatorParamsFromProto(pbValParams *tmproto.ValidatorParams) ValidatorParams { var params = ValidatorParams{ PubKeyTypes: []string{}, } if pbValParams != nil { if pbValParams.XVotingPowerThreshold != nil { val := pbValParams.GetVotingPowerThreshold() + if val == 0 { + panic("voting power threshold cannot be zero") + } params.VotingPowerThreshold = &val }proto/tendermint/types/params.proto (1)
61-79: Consider adding examples to validation rules and clarify nullable behavior.The validation rules are well-documented, but consider:
- Adding examples to illustrate the threshold calculation for different validator set sizes.
- Clarifying whether the field should be nullable since it's marked as optional but doesn't use the
gogoproto.nullableannotation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (23)
abci/types/types.pb.gois excluded by!**/*.pb.goproto/tendermint/blocksync/types.pb.gois excluded by!**/*.pb.goproto/tendermint/consensus/types.pb.gois excluded by!**/*.pb.goproto/tendermint/consensus/wal.pb.gois excluded by!**/*.pb.goproto/tendermint/crypto/keys.pb.gois excluded by!**/*.pb.goproto/tendermint/crypto/proof.pb.gois excluded by!**/*.pb.goproto/tendermint/libs/bits/types.pb.gois excluded by!**/*.pb.goproto/tendermint/mempool/types.pb.gois excluded by!**/*.pb.goproto/tendermint/p2p/conn.pb.gois excluded by!**/*.pb.goproto/tendermint/p2p/pex.pb.gois excluded by!**/*.pb.goproto/tendermint/p2p/types.pb.gois excluded by!**/*.pb.goproto/tendermint/privval/types.pb.gois excluded by!**/*.pb.goproto/tendermint/state/types.pb.gois excluded by!**/*.pb.goproto/tendermint/statesync/types.pb.gois excluded by!**/*.pb.goproto/tendermint/types/block.pb.gois excluded by!**/*.pb.goproto/tendermint/types/canonical.pb.gois excluded by!**/*.pb.goproto/tendermint/types/dash.pb.gois excluded by!**/*.pb.goproto/tendermint/types/events.pb.gois excluded by!**/*.pb.goproto/tendermint/types/evidence.pb.gois excluded by!**/*.pb.goproto/tendermint/types/params.pb.gois excluded by!**/*.pb.goproto/tendermint/types/types.pb.gois excluded by!**/*.pb.goproto/tendermint/types/validator.pb.gois excluded by!**/*.pb.goproto/tendermint/version/types.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (33)
.github/workflows/check-generated.yml(1 hunks)Makefile(2 hunks)internal/consensus/replay_test.go(1 hunks)internal/consensus/replayer.go(2 hunks)internal/consensus/replayer_test.go(1 hunks)internal/consensus/versioned/selectproposer/height_proposer_test.go(1 hunks)internal/evidence/pool_test.go(2 hunks)internal/evidence/reactor_test.go(2 hunks)internal/evidence/verify_test.go(1 hunks)internal/state/current_round_state.go(2 hunks)internal/state/state.go(2 hunks)internal/state/state_test.go(1 hunks)internal/test/factory/validator_set.go(1 hunks)light/helpers_test.go(1 hunks)light/provider/http/http.go(1 hunks)proto/tendermint/types/dash_test.go(3 hunks)proto/tendermint/types/params.proto(1 hunks)proto/tendermint/types/validator.proto(1 hunks)rpc/client/evidence_test.go(1 hunks)test/e2e/runner/evidence.go(1 hunks)test/e2e/tests/validator_test.go(2 hunks)types/evidence.go(3 hunks)types/evidence_test.go(1 hunks)types/generator.go(1 hunks)types/params.go(6 hunks)types/params_test.go(1 hunks)types/protobuf.go(3 hunks)types/protobuf_test.go(1 hunks)types/validation_test.go(1 hunks)types/validator_set.go(7 hunks)types/validator_set_test.go(5 hunks)types/vote_set_test.go(4 hunks)version/version.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
internal/consensus/versioned/selectproposer/height_proposer_test.go (1)
Learnt from: lklimek
PR: dashpay/tenderdash#959
File: internal/consensus/versioned/selectproposer/height_proposer.go:93-93
Timestamp: 2024-11-12T14:56:12.333Z
Learning: The fix in `height_proposer.go` eliminates the `validator set hash mismatch` error during quorum rotation, so no additional error handling is required in the `/validators` endpoint.
🪛 GitHub Check: golangci-lint
internal/evidence/reactor_test.go
[failure] 102-102:
unused-parameter: parameter 'chdesc' seems to be unused, consider removing or renaming it to match ^_ (revive)
types/validator_set.go
[failure] 429-429:
G115: integer overflow conversion uint64 -> int64 (gosec)
🪛 GitHub Actions: Golang Linter
types/validator_set.go
[error] 429-429: G115: integer overflow conversion uint64 -> int64 (gosec)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_apps
- GitHub Check: test_abci_cli
- GitHub Check: tests (03)
- GitHub Check: e2e-test (rotate)
- GitHub Check: e2e-test (dashcore)
- GitHub Check: Super linter
🔇 Additional comments (39)
.github/workflows/check-generated.yml (1)
78-86: Forked gogoproto Installation UpdateThe new commands clone a fork of the gogoproto repository (branch
fix/panic-marshalto-nil-m) and install theprotoc-gen-gogofasterfrom that source. This change addresses a panic issue with nil marshaling. The implementation correctly cleans up the temporary clone.Makefile (1)
118-121: Align Dependency Error Message for gogofaster PluginThe error message under the
check-proto-depstarget now correctly instructs users to install theprotoc-gen-gogofasterplugin from the new repository and commit. This ensures consistency with the updated dependency sources in both the workflow and the Makefile.types/validator_set.go (4)
83-91: Good integration of voting power threshold in constructor.The logic to set
vals.VotingPowerThresholdfromvalidatorParams.VotingPowerThresholdis clear. Just be sure all call sites are updated to provide (or omit) the parameter correctly.
107-116: Simplify usage by relying on existing constructor.Forwarding to
NewValidatorSetis a clean approach. This ensures consistent initialization in a single place.
120-120: No concerns with creating an empty validator set.Returning a validator set with no threshold is fine as a default approach, since no voting is expected in an empty set.
937-938: Properly populatingVotingPowerThresholdin proto structure.Serializing the threshold into the proto is correct, ensuring the value is transferred consistently.
version/version.go (1)
14-14: Version bump is appropriate.Updating
ABCISemVerfrom “1.2.0” to “1.3.0” indicates a semver increment consistent with breaking changes (new field inConsensusParams).proto/tendermint/types/dash_test.go (2)
7-8: New import statement is fine.No issues with referencing the
dashpay/tenderdash/proto/tendermint/typespackage.
60-62: Marshaling test logic confirmed.The test now checks marshalling of
Voteobjects without expecting a panic. This accurately reflects the revised code behavior.internal/test/factory/validator_set.go (1)
47-54: LGTM! Well-structured voting power threshold calculation.The threshold calculation is logical, multiplying the number of validators by the default voting power to determine the total threshold. This implementation aligns with the PR's objective of allowing configurable thresholds.
rpc/client/evidence_test.go (1)
42-42: LGTM! Appropriate use of nil for test context.The update correctly adapts to the new
NewValidatorSetsignature while usingnilfor the threshold parameter, which is suitable for this test context where specific voting power thresholds aren't critical.types/protobuf_test.go (1)
46-46: LGTM! Consistent with other test updates.The change appropriately adds the
nilthreshold parameter toNewValidatorSet, maintaining consistency with other test files while focusing on the core purpose of testing ABCI validator conversion.types/validation_test.go (1)
26-26: LGTM!The change correctly updates the
NewValidatorSetcall to include the new voting power threshold parameter, usingnilto maintain default behavior in tests.types/protobuf.go (2)
163-172: Good use of test-only restriction!The function is properly restricted to test usage with clear documentation and runtime checks.
193-193: Consistent with voting power threshold changes.The
nilparameter maintains consistency with the broader changes to support configurable voting power thresholds.test/e2e/runner/evidence.go (1)
71-74: LGTM!The change correctly updates the
NewValidatorSetcall to include the new voting power threshold parameter, maintaining default behavior in the evidence injection tests.internal/evidence/verify_test.go (1)
42-42: LGTM!The change correctly updates the
NewValidatorSetcall to include the new voting power threshold parameter, maintaining default behavior in the evidence verification tests.internal/consensus/replayer_test.go (1)
201-203: LGTM! Good synchronization of voting power threshold.The code correctly synchronizes the genesis validator set's voting power threshold with the one from the validator set, and the comment clearly explains the rationale.
test/e2e/tests/validator_test.go (1)
260-260: LGTM! Good use of consensus params validator settings.The code correctly uses the validator settings from consensus params when creating a new validator set.
types/evidence_test.go (1)
167-169: LGTM!The addition of the
nilparameter for the voting power threshold maintains the existing test behavior while supporting the new feature.light/provider/http/http.go (1)
241-241: LGTM!The addition of the
nilparameter for the voting power threshold maintains the existing light client behavior while supporting the new feature.internal/state/current_round_state.go (3)
331-333: LGTM!The validator set creation now correctly includes the validator parameters, allowing for custom voting power thresholds.
344-346: LGTM!The code properly handles the optional voting power threshold by checking for nil before assignment.
349-349: Enhanced error handling with ValidateBasic check.The function now validates the validator set before returning it, ensuring the voting power threshold and other parameters are valid.
types/params_test.go (1)
221-222: LGTM!The addition of the
VotingPowerThresholdfield with anildefault value maintains backward compatibility while supporting the new feature.internal/consensus/replayer.go (1)
345-349: LGTM! Validator parameters are correctly handled.The implementation correctly extracts validator parameters from the response and handles the case when they are not present.
types/evidence.go (1)
494-496: LGTM! Good safety check for test-only code.The runtime check ensures that the mock function is only used in tests, preventing accidental usage in production code.
internal/evidence/reactor_test.go (2)
102-102: Approve parameter renaming to indicate unused context.The renaming of
ctxto_ctxfollows Go's convention for indicating unused parameters.Also applies to: 109-109
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 102-102:
unused-parameter: parameter 'chdesc' seems to be unused, consider removing or renaming it to match ^_ (revive)
547-554: LGTM! Validator set initialization updated.The addition of the
nilparameter toNewValidatorSetaligns with the broader changes to support voting power threshold configuration.internal/evidence/pool_test.go (1)
133-133: LGTM! Validator set initialization updated.The addition of the
nilparameter toNewValidatorSetaligns with the broader changes to support voting power threshold configuration.types/vote_set_test.go (2)
516-516: LGTM! Validator set initialization updated.The addition of the
nilparameter toNewValidatorSetaligns with the broader changes to support voting power threshold configuration.
552-639: LGTM! Comprehensive test coverage for validator threshold.The new test function provides thorough coverage of voting threshold behavior across different network sizes, including edge cases for single-node and two-node networks.
internal/state/state_test.go (1)
357-359: LGTM! Improved test setup with proper validator initialization.The changes enhance test reliability by:
- Initializing state with a proper random validator set
- Using the threshold public key from the validator set for consistency
Also applies to: 364-364
proto/tendermint/types/validator.proto (1)
10-18: LGTM! The field addition maintains consistency with ValidatorParams.The new field and its relationship with
ValidatorParams.voting_power_thresholdare clearly documented.types/validator_set_test.go (4)
236-236: LGTM! Consistent with other test cases.The benchmark test correctly includes the new parameter.
281-282: LGTM! Empty validator set test is updated correctly.The test case for empty validator set properly includes the new parameter.
313-315: LGTM! Panic test cases are updated consistently.The test cases for validating panic conditions are properly updated with the new parameter.
Also applies to: 322-324, 334-336
27-27:✅ Verification successful
Verify the intended behavior of passing nil for voting_power_threshold.
The test is passing
nilfor the newvoting_power_thresholdparameter. Confirm that this is the intended behavior and that it correctly tests the default threshold calculation based on LLMQ quorum type.
🏁 Script executed:
#!/bin/bash # Search for other test files that might provide insights into the expected behavior rg -A 5 "NewValidatorSet.*nil.*btcjson.LLMQType" --type goLength of output: 893
Passing nil for voting_power_threshold appears to be the intended behavior.
- The test file consistently passes nil for the new voting_power_threshold parameter in all calls to
NewValidatorSet.- This usage suggests that the test is deliberately leveraging the default threshold calculation based on the LLMQ quorum type.
- No conflicting tests or usage patterns were found that indicate a need for a non-nil value.
internal/consensus/replay_test.go (1)
1279-1286: LGTM! Handshake test is updated correctly.The test case for validator set updates during handshake properly includes the new parameter.
| // Threshold power, must equal to ValidatorParams.voting_power_threshold | ||
| VotingPowerThreshold uint64 `json:"threshold_power"` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Introduce boundary checks for new VotingPowerThreshold.
While adding the VotingPowerThreshold field is beneficial, ensure that its range aligns with the rest of the system (which uses int64 for total power). Consider validating it against math.MaxInt64 to avoid inconsistencies or potential overflow down the line.
| vals.VotingPowerThreshold = vp.GetVotingPowerThreshold() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restoring VotingPowerThreshold from proto.
Deserializing the threshold back into the validator set is correct. Again, consider a boundary check to avoid overflow on assignment.
| @@ -83,7 +83,7 @@ func GenerateValidatorSet(valParams []ValSetParam, opts ...ValSetOptionFunc) (*V | |||
| valz = append(valz, NewValidator(qks.PubKey, opt.VotingPower, proTxHash, "")) | |||
| } | |||
| sort.Sort(PrivValidatorsByProTxHash(privValidators)) | |||
| return NewValidatorSet(valz, ld.ThresholdPubKey, crypto.SmallQuorumType(), quorumHash, true), privValidators | |||
| return NewValidatorSet(valz, ld.ThresholdPubKey, crypto.SmallQuorumType(), quorumHash, true, nil), privValidators | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider setting an explicit voting power threshold.
Given that this is a core generator function and the PR's objective is to support configurable voting thresholds, consider setting an explicit threshold instead of passing nil. This would ensure generated validator sets have well-defined voting power thresholds.
- return NewValidatorSet(valz, ld.ThresholdPubKey, crypto.SmallQuorumType(), quorumHash, true, nil), privValidators
+ votingPowerThreshold := uint64(len(valz)) * uint64(DefaultDashVotingPower)
+ return NewValidatorSet(valz, ld.ThresholdPubKey, crypto.SmallQuorumType(), quorumHash, true, &types.ValidatorParams{VotingPowerThreshold: &votingPowerThreshold}), privValidators📝 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.
| return NewValidatorSet(valz, ld.ThresholdPubKey, crypto.SmallQuorumType(), quorumHash, true, nil), privValidators | |
| votingPowerThreshold := uint64(len(valz)) * uint64(DefaultDashVotingPower) | |
| return NewValidatorSet(valz, ld.ThresholdPubKey, crypto.SmallQuorumType(), quorumHash, true, &types.ValidatorParams{VotingPowerThreshold: &votingPowerThreshold}), privValidators |
| @@ -51,6 +51,7 @@ func (pkz privKeys) ToValidators(thresholdPublicKey crypto.PubKey) *types.Valida | |||
| btcjson.LLMQType_5_60, | |||
| crypto.Checksum(thresholdPublicKey.Bytes()), | |||
| true, | |||
| nil, | |||
There was a problem hiding this comment.
Set an explicit voting power threshold for light client security.
For light client validation, it's important to set an explicit voting power threshold to ensure proper security validation.
- nil,
+ types.DefaultVotingPowerThreshold,📝 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.
| nil, | |
| types.DefaultVotingPowerThreshold, |
Issue being fixed or feature implemented
For single-node development environments, we need a way to change number of votes (voting threshold) in order to allow single-node networks to pass voting with 1 vote.
We also want this to be documented in the blockchain, as we want some accountability in case someone tries to change the threshold on a running network (like mainnet).
What was done?
How Has This Been Tested?
Breaking Changes
New field added to ConsensusParams, affecting ABCI protocol. ABCI protocol version bumped.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Chores