Conversation
CoderZhi
requested review from
a team,
Liuhaai,
dustinxie and
envestcc
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the consensus and blockchain modules to support multiple producers instead of a single producer by replacing singular fields (e.g. encodedAddr, priKey) with slices (encodedAddrs, priKeys) and updating the associated logic. Key changes include:
- Updating the rollDPoS context and related functions to handle multiple producer keys and addresses.
- Refactoring endorsement and block minting routines to work with multiple endorsements and key selections.
- Modifying tests and configuration functions across consensus, blockchain, and server components to align with the new multi-producer support.
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| consensus/scheme/rolldpos/rolldposctx.go | Replaced singular producer fields with slices and updated proposal and endorsement flows accordingly. |
| blockchain/config.go | Updated API calls to use MainProducerAddress/MainProducerPrivateKey in place of single producer functions. |
| endorsement/endorsement.go | Modified the Endorse function to return a slice of endorsements for multiple signers. |
| blockchain/blockchain.go | Revised the MintNewBlock signature and logic to support filtering on multiple producer keys. |
| consensus/scheme/rolldpos/roundctx.go | Updated ReadyToCommit and delegate checks to iterate over multiple producer addresses. |
| consensus/consensusfsm/* | Changed consensus event production routines to return/process slices of events rather than a single event. |
| Various tests and mocks | Adjusted tests, builders, and mocks to remove obsolete SetAddr calls and refer to multi-producer APIs. |
Comments suppressed due to low confidence (3)
consensus/scheme/rolldpos/rolldposctx.go:691
- The check for exactly one endorsement may be too strict now that the system supports multiple producers; consider if multiple valid endorsements should be accepted or if the logic needs updating to handle the new multi-producer scenario.
if len(ens) != 1 {
consensus/scheme/rolldpos/rolldpos.go:123
- The 'height' parameter is accepted but not used when calling bc.MintNewBlock; verify whether the intended block height should influence block minting or if the parameter should be removed.
func (cm *chainManager) MintNewBlock(height uint64, timestamp time.Time, fs ...blockchain.FilterFunc) (*block.Block, error) {
consensus/consensusfsm/mock_context_test.go:206
- Ensure that all occurrences and mocks referring to delegate status consistently use 'HasDelegate' instead of the deprecated 'IsDelegate' to avoid confusion in both production and test code.
func (m *MockContext) HasDelegate() bool {
huangzhiran
approved these changes
Mar 18, 2025
| // TODO: we should use keystore in the future | ||
| encodedAddr string | ||
| priKey crypto.PrivateKey | ||
| priKey []crypto.PrivateKey |
envestcc
reviewed
Mar 18, 2025
| // MintNewBlock creates a new block with given actions | ||
| func (cm *chainManager) MintNewBlock(timestamp time.Time) (*block.Block, error) { | ||
| return cm.bc.MintNewBlock(timestamp) | ||
| func (cm *chainManager) MintNewBlock(height uint64, timestamp time.Time, fs ...blockchain.FilterFunc) (*block.Block, error) { |
Member
There was a problem hiding this comment.
The height parameter has not been used.
envestcc
reviewed
Mar 18, 2025
blockchain/blockchain.go
Outdated
| // MintNewBlock creates a new block with given actions | ||
| // Note: the coinbase transfer will be added to the given transfers when minting a new block | ||
| MintNewBlock(timestamp time.Time) (*block.Block, error) | ||
| MintNewBlock(time.Time, ...FilterFunc) (*block.Block, error) |
Member
There was a problem hiding this comment.
only minter address param maybe enough
envestcc
approved these changes
Mar 18, 2025
Member
envestcc
left a comment
envestcc
left a comment
There was a problem hiding this comment.
mainly looks good to me, left two small comments
CoderZhi
force-pushed
the
multiple_producers
branch
from
6208d94 to
03c109e
Compare
|
envestcc
pushed a commit
that referenced
this pull request
Apr 21, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
as title
Fixes #(issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: