From a2d5a643e7a0e38e89327de6ba215e5b47872bf3 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 2 Oct 2024 12:45:46 +0300 Subject: [PATCH 01/13] Introduced an additional structure to represent signed and unsigned hotstuff proposal. Updated SafetyRules interface and implementation --- consensus/hotstuff/consumer.go | 4 +- consensus/hotstuff/event_handler.go | 2 +- .../hotstuff/eventhandler/event_handler.go | 8 ++-- .../eventhandler/event_handler_test.go | 16 ++++---- consensus/hotstuff/eventloop/event_loop.go | 4 +- .../hotstuff/eventloop/event_loop_test.go | 2 +- .../hotstuff/forks/block_builder_test.go | 8 ++-- consensus/hotstuff/forks/forks.go | 2 +- consensus/hotstuff/helper/block.go | 21 +++++----- .../hotstuff/integration/filters_test.go | 14 +++---- .../hotstuff/integration/instance_test.go | 10 ++--- consensus/hotstuff/mocks/consumer.go | 2 +- consensus/hotstuff/mocks/event_handler.go | 4 +- consensus/hotstuff/mocks/event_loop.go | 2 +- .../hotstuff/mocks/participant_consumer.go | 2 +- consensus/hotstuff/mocks/safety_rules.go | 16 ++++---- consensus/hotstuff/mocks/validator.go | 4 +- .../mocks/vote_aggregation_consumer.go | 2 +- .../vote_aggregation_violation_consumer.go | 2 +- consensus/hotstuff/mocks/vote_aggregator.go | 6 +-- consensus/hotstuff/mocks/vote_collector.go | 4 +- .../hotstuff/mocks/vote_processor_factory.go | 8 ++-- consensus/hotstuff/model/errors.go | 6 +-- consensus/hotstuff/model/proposal.go | 38 ++++++++++++++----- .../hotstuff/notifications/log_consumer.go | 4 +- .../hotstuff/notifications/noop_consumer.go | 5 ++- .../pubsub/participant_distributor.go | 2 +- .../vote_aggregation_violation_consumer.go | 2 +- .../slashing_violation_consumer.go | 2 +- consensus/hotstuff/notifications/telemetry.go | 2 +- consensus/hotstuff/pacemaker/pacemaker.go | 2 +- consensus/hotstuff/pacemaker/view_tracker.go | 2 +- consensus/hotstuff/safety_rules.go | 2 +- .../hotstuff/safetyrules/safety_rules.go | 30 ++++++++++++--- .../hotstuff/safetyrules/safety_rules_test.go | 2 +- consensus/hotstuff/validator.go | 2 +- .../hotstuff/validator/metrics_wrapper.go | 2 +- consensus/hotstuff/validator/validator.go | 2 +- .../hotstuff/validator/validator_test.go | 4 +- .../verification/combined_signer_v3.go | 23 ----------- .../hotstuff/verification/staking_signer.go | 23 ----------- consensus/hotstuff/vote_aggregator.go | 4 +- consensus/hotstuff/vote_collector.go | 4 +- .../voteaggregator/vote_aggregator.go | 8 ++-- consensus/hotstuff/votecollector/factory.go | 4 +- .../hotstuff/votecollector/statemachine.go | 6 +-- .../votecollector/statemachine_test.go | 6 +-- consensus/hotstuff/votecollector/testutil.go | 2 +- consensus/recovery/recover.go | 10 ++--- consensus/recovery/recover_test.go | 6 +-- engine/collection/compliance/core_test.go | 2 +- engine/consensus/compliance/core_test.go | 2 +- module/hotstuff.go | 2 +- module/mock/hot_stuff.go | 2 +- 54 files changed, 176 insertions(+), 180 deletions(-) diff --git a/consensus/hotstuff/consumer.go b/consensus/hotstuff/consumer.go index 1a5bfb175af..2fcf2e4703e 100644 --- a/consensus/hotstuff/consumer.go +++ b/consensus/hotstuff/consumer.go @@ -60,7 +60,7 @@ type VoteAggregationViolationConsumer interface { // Prerequisites: // Implementation must be concurrency safe; Non-blocking; // and must handle repetition of the same events (with some processing overhead). - OnVoteForInvalidBlockDetected(vote *model.Vote, invalidProposal *model.Proposal) + OnVoteForInvalidBlockDetected(vote *model.Vote, invalidProposal *model.SignedProposal) } // TimeoutAggregationViolationConsumer consumes outbound notifications about Active Pacemaker violations specifically @@ -138,7 +138,7 @@ type ParticipantConsumer interface { // Prerequisites: // Implementation must be concurrency safe; Non-blocking; // and must handle repetition of the same events (with some processing overhead). - OnReceiveProposal(currentView uint64, proposal *model.Proposal) + OnReceiveProposal(currentView uint64, proposal *model.SignedProposal) // OnReceiveQc notifications are produced by the EventHandler when it starts processing a // QuorumCertificate [QC] constructed by the node's internal vote aggregator. diff --git a/consensus/hotstuff/event_handler.go b/consensus/hotstuff/event_handler.go index a2134680389..6deda44eeca 100644 --- a/consensus/hotstuff/event_handler.go +++ b/consensus/hotstuff/event_handler.go @@ -39,7 +39,7 @@ type EventHandler interface { // consensus participant. // All inputs should be validated before feeding into this function. Assuming trusted data. // No errors are expected during normal operation. - OnReceiveProposal(proposal *model.Proposal) error + OnReceiveProposal(proposal *model.SignedProposal) error // OnLocalTimeout handles a local timeout event by creating a model.TimeoutObject and broadcasting it. // No errors are expected during normal operation. diff --git a/consensus/hotstuff/eventhandler/event_handler.go b/consensus/hotstuff/eventhandler/event_handler.go index 04fc873bcc0..7b4c3d2a6f4 100644 --- a/consensus/hotstuff/eventhandler/event_handler.go +++ b/consensus/hotstuff/eventhandler/event_handler.go @@ -164,7 +164,7 @@ func (e *EventHandler) OnReceiveTc(tc *flow.TimeoutCertificate) error { // consensus participant. // All inputs should be validated before feeding into this function. Assuming trusted data. // No errors are expected during normal operation. -func (e *EventHandler) OnReceiveProposal(proposal *model.Proposal) error { +func (e *EventHandler) OnReceiveProposal(proposal *model.SignedProposal) error { block := proposal.Block curView := e.paceMaker.CurView() log := e.log.With(). @@ -429,7 +429,7 @@ func (e *EventHandler) proposeForNewViewIfPrimary() error { lastViewTC = nil } - // Construct Own Proposal + // Construct Own SignedProposal // CAUTION, design constraints: // (i) We cannot process our own proposal within the `EventHandler` right away. // (ii) We cannot add our own proposal to Forks here right away. @@ -491,7 +491,7 @@ func (e *EventHandler) proposeForNewViewIfPrimary() error { // It is called AFTER the block has been stored or found in Forks // It checks whether to vote for this block. // No errors are expected during normal operation. -func (e *EventHandler) processBlockForCurrentView(proposal *model.Proposal) error { +func (e *EventHandler) processBlockForCurrentView(proposal *model.SignedProposal) error { // sanity check that block is really for the current view: curView := e.paceMaker.CurView() block := proposal.Block @@ -526,7 +526,7 @@ func (e *EventHandler) processBlockForCurrentView(proposal *model.Proposal) erro // ownVote generates and forwards the own vote, if we decide to vote. // Any errors are potential symptoms of uncovered edge cases or corrupted internal state (fatal). // No errors are expected during normal operation. -func (e *EventHandler) ownVote(proposal *model.Proposal, curView uint64, nextLeader flow.Identifier) error { +func (e *EventHandler) ownVote(proposal *model.SignedProposal, curView uint64, nextLeader flow.Identifier) error { block := proposal.Block log := e.log.With(). Uint64("block_view", block.View). diff --git a/consensus/hotstuff/eventhandler/event_handler_test.go b/consensus/hotstuff/eventhandler/event_handler_test.go index b0c43fc3a82..6341138c607 100644 --- a/consensus/hotstuff/eventhandler/event_handler_test.go +++ b/consensus/hotstuff/eventhandler/event_handler_test.go @@ -132,14 +132,14 @@ func NewSafetyRules(t *testing.T) *SafetyRules { // SafetyRules will not vote for any block, unless the blockID exists in votable map safetyRules.On("ProduceVote", mock.Anything, mock.Anything).Return( - func(block *model.Proposal, _ uint64) *model.Vote { + func(block *model.SignedProposal, _ uint64) *model.Vote { _, ok := safetyRules.votable[block.Block.BlockID] if !ok { return nil } return createVote(block.Block) }, - func(block *model.Proposal, _ uint64) error { + func(block *model.SignedProposal, _ uint64) error { _, ok := safetyRules.votable[block.Block.BlockID] if !ok { return model.NewNoVoteErrorf("block not found") @@ -179,7 +179,7 @@ func NewForks(t *testing.T, finalized uint64) *Forks { } f.On("AddValidatedBlock", mock.Anything).Return(func(proposal *model.Block) error { - log.Info().Msgf("forks.AddValidatedBlock received Proposal for view: %v, QC: %v\n", proposal.View, proposal.QC.View) + log.Info().Msgf("forks.AddValidatedBlock received SignedProposal for view: %v, QC: %v\n", proposal.View, proposal.QC.View) return f.addProposal(proposal) }).Maybe() @@ -228,7 +228,7 @@ type BlockProducer struct { } func (b *BlockProducer) MakeBlockProposal(view uint64, qc *flow.QuorumCertificate, lastViewTC *flow.TimeoutCertificate) (*flow.Header, error) { - return model.ProposalToFlow(&model.Proposal{ + return model.ProposalToFlow(&model.SignedProposal{ Block: helper.MakeBlock( helper.WithBlockView(view), helper.WithBlockQC(qc), @@ -258,8 +258,8 @@ type EventHandlerSuite struct { initView uint64 // the current view at the beginning of the test case endView uint64 // the expected current view at the end of the test case - parentProposal *model.Proposal - votingProposal *model.Proposal + parentProposal *model.SignedProposal + votingProposal *model.SignedProposal qc *flow.QuorumCertificate tc *flow.TimeoutCertificate newview *model.NewViewEvent @@ -1033,9 +1033,9 @@ func createVote(block *model.Block) *model.Vote { } } -func createProposal(view uint64, qcview uint64) *model.Proposal { +func createProposal(view uint64, qcview uint64) *model.SignedProposal { block := createBlockWithQC(view, qcview) - return &model.Proposal{ + return &model.SignedProposal{ Block: block, SigData: nil, } diff --git a/consensus/hotstuff/eventloop/event_loop.go b/consensus/hotstuff/eventloop/event_loop.go index d7bc478490b..8a91d214355 100644 --- a/consensus/hotstuff/eventloop/event_loop.go +++ b/consensus/hotstuff/eventloop/event_loop.go @@ -22,7 +22,7 @@ import ( // it contains an attached insertionTime that is used to measure how long we have waited between queening proposal and // actually processing by `EventHandler`. type queuedProposal struct { - proposal *model.Proposal + proposal *model.SignedProposal insertionTime time.Time } @@ -263,7 +263,7 @@ func (el *EventLoop) loop(ctx context.Context) error { } // SubmitProposal pushes the received block to the proposals channel -func (el *EventLoop) SubmitProposal(proposal *model.Proposal) { +func (el *EventLoop) SubmitProposal(proposal *model.SignedProposal) { queueItem := queuedProposal{ proposal: proposal, insertionTime: time.Now(), diff --git a/consensus/hotstuff/eventloop/event_loop_test.go b/consensus/hotstuff/eventloop/event_loop_test.go index 8b6eeed5b25..1f57050922b 100644 --- a/consensus/hotstuff/eventloop/event_loop_test.go +++ b/consensus/hotstuff/eventloop/event_loop_test.go @@ -258,7 +258,7 @@ func TestReadyDoneWithStartTime(t *testing.T) { require.NoError(t, err) done := make(chan struct{}) - eh.On("OnReceiveProposal", mock.AnythingOfType("*model.Proposal")).Run(func(args mock.Arguments) { + eh.On("OnReceiveProposal", mock.AnythingOfType("*model.SignedProposal")).Run(func(args mock.Arguments) { require.True(t, time.Now().After(startTime)) close(done) }).Return(nil).Once() diff --git a/consensus/hotstuff/forks/block_builder_test.go b/consensus/hotstuff/forks/block_builder_test.go index 03daec535c1..2c50808601b 100644 --- a/consensus/hotstuff/forks/block_builder_test.go +++ b/consensus/hotstuff/forks/block_builder_test.go @@ -77,8 +77,8 @@ func (bb *BlockBuilder) AddVersioned(qcView uint64, blockView uint64, qcVersion // Proposals returns a list of all proposals added to the BlockBuilder. // Returns an error if the blocks do not form a connected tree rooted at genesis. -func (bb *BlockBuilder) Proposals() ([]*model.Proposal, error) { - blocks := make([]*model.Proposal, 0, len(bb.blockViews)) +func (bb *BlockBuilder) Proposals() ([]*model.SignedProposal, error) { + blocks := make([]*model.SignedProposal, 0, len(bb.blockViews)) genesisBlock := makeGenesis() genesisBV := &BlockView{ @@ -99,7 +99,7 @@ func (bb *BlockBuilder) Proposals() ([]*model.Proposal, error) { if qc.View+1 != bv.View { lastViewTC = helper.MakeTC(helper.WithTCView(bv.View - 1)) } - proposal := &model.Proposal{ + proposal := &model.SignedProposal{ Block: &model.Block{ View: bv.View, QC: qc, @@ -177,7 +177,7 @@ func makeGenesis() *model.CertifiedBlock { } // toBlocks converts the given proposals to slice of blocks -func toBlocks(proposals []*model.Proposal) []*model.Block { +func toBlocks(proposals []*model.SignedProposal) []*model.Block { blocks := make([]*model.Block, 0, len(proposals)) for _, b := range proposals { blocks = append(blocks, b.Block) diff --git a/consensus/hotstuff/forks/forks.go b/consensus/hotstuff/forks/forks.go index aa4db7f9853..8bba25508b9 100644 --- a/consensus/hotstuff/forks/forks.go +++ b/consensus/hotstuff/forks/forks.go @@ -401,7 +401,7 @@ func (f *Forks) checkForAdvancingFinalization(certifiedBlock *model.CertifiedBlo parentBlock := parentVertex.(*BlockContainer).Block() // Note: we assume that all stored blocks pass Forks.EnsureBlockIsValidExtension(block); - // specifically, that Proposal's ViewNumber is strictly monotonically + // specifically, that SignedProposal's ViewNumber is strictly monotonically // increasing which is enforced by LevelledForest.VerifyVertex(...) // We denote: // * a DIRECT 1-chain as '<-' diff --git a/consensus/hotstuff/helper/block.go b/consensus/hotstuff/helper/block.go index a3fa6f6e2e7..398ef2d7941 100644 --- a/consensus/hotstuff/helper/block.go +++ b/consensus/hotstuff/helper/block.go @@ -56,9 +56,12 @@ func WithBlockQC(qc *flow.QuorumCertificate) func(*model.Block) { } } -func MakeProposal(options ...func(*model.Proposal)) *model.Proposal { - proposal := &model.Proposal{ - Block: MakeBlock(), +func MakeProposal(options ...func(*model.SignedProposal)) *model.SignedProposal { + proposal := &model.SignedProposal{ + Proposal: model.Proposal{ + Block: MakeBlock(), + LastViewTC: nil, + }, SigData: unittest.SignatureFixture(), } for _, option := range options { @@ -67,20 +70,20 @@ func MakeProposal(options ...func(*model.Proposal)) *model.Proposal { return proposal } -func WithBlock(block *model.Block) func(*model.Proposal) { - return func(proposal *model.Proposal) { +func WithBlock(block *model.Block) func(*model.SignedProposal) { + return func(proposal *model.SignedProposal) { proposal.Block = block } } -func WithSigData(sigData []byte) func(*model.Proposal) { - return func(proposal *model.Proposal) { +func WithSigData(sigData []byte) func(*model.SignedProposal) { + return func(proposal *model.SignedProposal) { proposal.SigData = sigData } } -func WithLastViewTC(lastViewTC *flow.TimeoutCertificate) func(*model.Proposal) { - return func(proposal *model.Proposal) { +func WithLastViewTC(lastViewTC *flow.TimeoutCertificate) func(*model.SignedProposal) { + return func(proposal *model.SignedProposal) { proposal.LastViewTC = lastViewTC } } diff --git a/consensus/hotstuff/integration/filters_test.go b/consensus/hotstuff/integration/filters_test.go index 8d6ac067f48..77b41128292 100644 --- a/consensus/hotstuff/integration/filters_test.go +++ b/consensus/hotstuff/integration/filters_test.go @@ -34,28 +34,28 @@ func BlockVotesBy(voterID flow.Identifier) VoteFilter { } // ProposalFilter is a filter function for dropping Proposals. -// Return value `true` implies that the the given Proposal should be -// dropped, while `false` indicates that the Proposal should be received. -type ProposalFilter func(*model.Proposal) bool +// Return value `true` implies that the the given SignedProposal should be +// dropped, while `false` indicates that the SignedProposal should be received. +type ProposalFilter func(*model.SignedProposal) bool -func BlockNoProposals(*model.Proposal) bool { +func BlockNoProposals(*model.SignedProposal) bool { return false } -func BlockAllProposals(*model.Proposal) bool { +func BlockAllProposals(*model.SignedProposal) bool { return true } // BlockProposalRandomly drops proposals randomly with a probability of `dropProbability` ∈ [0,1] func BlockProposalRandomly(dropProbability float64) ProposalFilter { - return func(*model.Proposal) bool { + return func(*model.SignedProposal) bool { return rand.Float64() < dropProbability } } // BlockProposalsBy drops all proposals originating from the specified `proposerID` func BlockProposalsBy(proposerID flow.Identifier) ProposalFilter { - return func(proposal *model.Proposal) bool { + return func(proposal *model.SignedProposal) bool { return proposal.Block.ProposerID == proposerID } } diff --git a/consensus/hotstuff/integration/instance_test.go b/consensus/hotstuff/integration/instance_test.go index 069c6ede950..4f5b706ac51 100644 --- a/consensus/hotstuff/integration/instance_test.go +++ b/consensus/hotstuff/integration/instance_test.go @@ -59,7 +59,7 @@ type Instance struct { queue chan interface{} updatingBlocks sync.RWMutex headers map[flow.Identifier]*flow.Header - pendings map[flow.Identifier]*model.Proposal // indexed by parent ID + pendings map[flow.Identifier]*model.SignedProposal // indexed by parent ID // mocked dependencies committee *mocks.DynamicCommittee @@ -151,7 +151,7 @@ func NewInstance(t *testing.T, options ...Option) *Instance { stop: cfg.StopCondition, // instance data - pendings: make(map[flow.Identifier]*model.Proposal), + pendings: make(map[flow.Identifier]*model.SignedProposal), headers: make(map[flow.Identifier]*flow.Header), queue: make(chan interface{}, 1024), @@ -403,7 +403,7 @@ func NewInstance(t *testing.T, options ...Option) *Instance { minRequiredWeight := committees.WeightThresholdToBuildQC(uint64(len(in.participants)) * weight) voteProcessorFactory := mocks.NewVoteProcessorFactory(t) voteProcessorFactory.On("Create", mock.Anything, mock.Anything).Return( - func(log zerolog.Logger, proposal *model.Proposal) hotstuff.VerifyingVoteProcessor { + func(log zerolog.Logger, proposal *model.SignedProposal) hotstuff.VerifyingVoteProcessor { stakingSigAggtor := helper.MakeWeightedSignatureAggregator(weight) stakingSigAggtor.On("Verify", mock.Anything, mock.Anything).Return(nil).Maybe() @@ -597,7 +597,7 @@ func (in *Instance) Run() error { } case msg := <-in.queue: switch m := msg.(type) { - case *model.Proposal: + case *model.SignedProposal: // add block to aggregator in.voteAggregator.AddBlock(m) // then pass to event handler @@ -629,7 +629,7 @@ func (in *Instance) Run() error { } } -func (in *Instance) ProcessBlock(proposal *model.Proposal) { +func (in *Instance) ProcessBlock(proposal *model.SignedProposal) { in.updatingBlocks.Lock() defer in.updatingBlocks.Unlock() _, parentExists := in.headers[proposal.Block.QC.BlockID] diff --git a/consensus/hotstuff/mocks/consumer.go b/consensus/hotstuff/mocks/consumer.go index cd2363a1b93..311ae5a8a29 100644 --- a/consensus/hotstuff/mocks/consumer.go +++ b/consensus/hotstuff/mocks/consumer.go @@ -79,7 +79,7 @@ func (_m *Consumer) OnQcTriggeredViewChange(oldView uint64, newView uint64, qc * } // OnReceiveProposal provides a mock function with given fields: currentView, proposal -func (_m *Consumer) OnReceiveProposal(currentView uint64, proposal *model.Proposal) { +func (_m *Consumer) OnReceiveProposal(currentView uint64, proposal *model.SignedProposal) { _m.Called(currentView, proposal) } diff --git a/consensus/hotstuff/mocks/event_handler.go b/consensus/hotstuff/mocks/event_handler.go index a74897fd303..7fac8c02d1e 100644 --- a/consensus/hotstuff/mocks/event_handler.go +++ b/consensus/hotstuff/mocks/event_handler.go @@ -57,7 +57,7 @@ func (_m *EventHandler) OnPartialTcCreated(partialTC *hotstuff.PartialTcCreated) } // OnReceiveProposal provides a mock function with given fields: proposal -func (_m *EventHandler) OnReceiveProposal(proposal *model.Proposal) error { +func (_m *EventHandler) OnReceiveProposal(proposal *model.SignedProposal) error { ret := _m.Called(proposal) if len(ret) == 0 { @@ -65,7 +65,7 @@ func (_m *EventHandler) OnReceiveProposal(proposal *model.Proposal) error { } var r0 error - if rf, ok := ret.Get(0).(func(*model.Proposal) error); ok { + if rf, ok := ret.Get(0).(func(*model.SignedProposal) error); ok { r0 = rf(proposal) } else { r0 = ret.Error(0) diff --git a/consensus/hotstuff/mocks/event_loop.go b/consensus/hotstuff/mocks/event_loop.go index 1d3943eb3c4..c27549181b2 100644 --- a/consensus/hotstuff/mocks/event_loop.go +++ b/consensus/hotstuff/mocks/event_loop.go @@ -98,7 +98,7 @@ func (_m *EventLoop) Start(_a0 irrecoverable.SignalerContext) { } // SubmitProposal provides a mock function with given fields: proposal -func (_m *EventLoop) SubmitProposal(proposal *model.Proposal) { +func (_m *EventLoop) SubmitProposal(proposal *model.SignedProposal) { _m.Called(proposal) } diff --git a/consensus/hotstuff/mocks/participant_consumer.go b/consensus/hotstuff/mocks/participant_consumer.go index 883380232b4..b6970bba788 100644 --- a/consensus/hotstuff/mocks/participant_consumer.go +++ b/consensus/hotstuff/mocks/participant_consumer.go @@ -42,7 +42,7 @@ func (_m *ParticipantConsumer) OnQcTriggeredViewChange(oldView uint64, newView u } // OnReceiveProposal provides a mock function with given fields: currentView, proposal -func (_m *ParticipantConsumer) OnReceiveProposal(currentView uint64, proposal *model.Proposal) { +func (_m *ParticipantConsumer) OnReceiveProposal(currentView uint64, proposal *model.SignedProposal) { _m.Called(currentView, proposal) } diff --git a/consensus/hotstuff/mocks/safety_rules.go b/consensus/hotstuff/mocks/safety_rules.go index 54c36caf9fa..e1d87474521 100644 --- a/consensus/hotstuff/mocks/safety_rules.go +++ b/consensus/hotstuff/mocks/safety_rules.go @@ -46,7 +46,7 @@ func (_m *SafetyRules) ProduceTimeout(curView uint64, newestQC *flow.QuorumCerti } // ProduceVote provides a mock function with given fields: proposal, curView -func (_m *SafetyRules) ProduceVote(proposal *model.Proposal, curView uint64) (*model.Vote, error) { +func (_m *SafetyRules) ProduceVote(proposal *model.SignedProposal, curView uint64) (*model.Vote, error) { ret := _m.Called(proposal, curView) if len(ret) == 0 { @@ -55,10 +55,10 @@ func (_m *SafetyRules) ProduceVote(proposal *model.Proposal, curView uint64) (*m var r0 *model.Vote var r1 error - if rf, ok := ret.Get(0).(func(*model.Proposal, uint64) (*model.Vote, error)); ok { + if rf, ok := ret.Get(0).(func(*model.SignedProposal, uint64) (*model.Vote, error)); ok { return rf(proposal, curView) } - if rf, ok := ret.Get(0).(func(*model.Proposal, uint64) *model.Vote); ok { + if rf, ok := ret.Get(0).(func(*model.SignedProposal, uint64) *model.Vote); ok { r0 = rf(proposal, curView) } else { if ret.Get(0) != nil { @@ -66,7 +66,7 @@ func (_m *SafetyRules) ProduceVote(proposal *model.Proposal, curView uint64) (*m } } - if rf, ok := ret.Get(1).(func(*model.Proposal, uint64) error); ok { + if rf, ok := ret.Get(1).(func(*model.SignedProposal, uint64) error); ok { r1 = rf(proposal, curView) } else { r1 = ret.Error(1) @@ -76,7 +76,7 @@ func (_m *SafetyRules) ProduceVote(proposal *model.Proposal, curView uint64) (*m } // SignOwnProposal provides a mock function with given fields: unsignedProposal -func (_m *SafetyRules) SignOwnProposal(unsignedProposal *model.Proposal) (*model.Vote, error) { +func (_m *SafetyRules) SignOwnProposal(unsignedProposal *model.SignedProposal) (*model.Vote, error) { ret := _m.Called(unsignedProposal) if len(ret) == 0 { @@ -85,10 +85,10 @@ func (_m *SafetyRules) SignOwnProposal(unsignedProposal *model.Proposal) (*model var r0 *model.Vote var r1 error - if rf, ok := ret.Get(0).(func(*model.Proposal) (*model.Vote, error)); ok { + if rf, ok := ret.Get(0).(func(*model.SignedProposal) (*model.Vote, error)); ok { return rf(unsignedProposal) } - if rf, ok := ret.Get(0).(func(*model.Proposal) *model.Vote); ok { + if rf, ok := ret.Get(0).(func(*model.SignedProposal) *model.Vote); ok { r0 = rf(unsignedProposal) } else { if ret.Get(0) != nil { @@ -96,7 +96,7 @@ func (_m *SafetyRules) SignOwnProposal(unsignedProposal *model.Proposal) (*model } } - if rf, ok := ret.Get(1).(func(*model.Proposal) error); ok { + if rf, ok := ret.Get(1).(func(*model.SignedProposal) error); ok { r1 = rf(unsignedProposal) } else { r1 = ret.Error(1) diff --git a/consensus/hotstuff/mocks/validator.go b/consensus/hotstuff/mocks/validator.go index 9b97d23d762..728795ef209 100644 --- a/consensus/hotstuff/mocks/validator.go +++ b/consensus/hotstuff/mocks/validator.go @@ -16,7 +16,7 @@ type Validator struct { } // ValidateProposal provides a mock function with given fields: proposal -func (_m *Validator) ValidateProposal(proposal *model.Proposal) error { +func (_m *Validator) ValidateProposal(proposal *model.SignedProposal) error { ret := _m.Called(proposal) if len(ret) == 0 { @@ -24,7 +24,7 @@ func (_m *Validator) ValidateProposal(proposal *model.Proposal) error { } var r0 error - if rf, ok := ret.Get(0).(func(*model.Proposal) error); ok { + if rf, ok := ret.Get(0).(func(*model.SignedProposal) error); ok { r0 = rf(proposal) } else { r0 = ret.Error(0) diff --git a/consensus/hotstuff/mocks/vote_aggregation_consumer.go b/consensus/hotstuff/mocks/vote_aggregation_consumer.go index 23ceb89ee6a..52819fcb1f9 100644 --- a/consensus/hotstuff/mocks/vote_aggregation_consumer.go +++ b/consensus/hotstuff/mocks/vote_aggregation_consumer.go @@ -31,7 +31,7 @@ func (_m *VoteAggregationConsumer) OnQcConstructedFromVotes(_a0 *flow.QuorumCert } // OnVoteForInvalidBlockDetected provides a mock function with given fields: vote, invalidProposal -func (_m *VoteAggregationConsumer) OnVoteForInvalidBlockDetected(vote *model.Vote, invalidProposal *model.Proposal) { +func (_m *VoteAggregationConsumer) OnVoteForInvalidBlockDetected(vote *model.Vote, invalidProposal *model.SignedProposal) { _m.Called(vote, invalidProposal) } diff --git a/consensus/hotstuff/mocks/vote_aggregation_violation_consumer.go b/consensus/hotstuff/mocks/vote_aggregation_violation_consumer.go index acac87cdce9..dcb718532a2 100644 --- a/consensus/hotstuff/mocks/vote_aggregation_violation_consumer.go +++ b/consensus/hotstuff/mocks/vote_aggregation_violation_consumer.go @@ -23,7 +23,7 @@ func (_m *VoteAggregationViolationConsumer) OnInvalidVoteDetected(err model.Inva } // OnVoteForInvalidBlockDetected provides a mock function with given fields: vote, invalidProposal -func (_m *VoteAggregationViolationConsumer) OnVoteForInvalidBlockDetected(vote *model.Vote, invalidProposal *model.Proposal) { +func (_m *VoteAggregationViolationConsumer) OnVoteForInvalidBlockDetected(vote *model.Vote, invalidProposal *model.SignedProposal) { _m.Called(vote, invalidProposal) } diff --git a/consensus/hotstuff/mocks/vote_aggregator.go b/consensus/hotstuff/mocks/vote_aggregator.go index 545990086ea..9bf372ecb2c 100644 --- a/consensus/hotstuff/mocks/vote_aggregator.go +++ b/consensus/hotstuff/mocks/vote_aggregator.go @@ -15,7 +15,7 @@ type VoteAggregator struct { } // AddBlock provides a mock function with given fields: block -func (_m *VoteAggregator) AddBlock(block *model.Proposal) { +func (_m *VoteAggregator) AddBlock(block *model.SignedProposal) { _m.Called(block) } @@ -45,7 +45,7 @@ func (_m *VoteAggregator) Done() <-chan struct{} { } // InvalidBlock provides a mock function with given fields: block -func (_m *VoteAggregator) InvalidBlock(block *model.Proposal) error { +func (_m *VoteAggregator) InvalidBlock(block *model.SignedProposal) error { ret := _m.Called(block) if len(ret) == 0 { @@ -53,7 +53,7 @@ func (_m *VoteAggregator) InvalidBlock(block *model.Proposal) error { } var r0 error - if rf, ok := ret.Get(0).(func(*model.Proposal) error); ok { + if rf, ok := ret.Get(0).(func(*model.SignedProposal) error); ok { r0 = rf(block) } else { r0 = ret.Error(0) diff --git a/consensus/hotstuff/mocks/vote_collector.go b/consensus/hotstuff/mocks/vote_collector.go index 5165640a838..27db379c9a8 100644 --- a/consensus/hotstuff/mocks/vote_collector.go +++ b/consensus/hotstuff/mocks/vote_collector.go @@ -33,7 +33,7 @@ func (_m *VoteCollector) AddVote(vote *model.Vote) error { } // ProcessBlock provides a mock function with given fields: block -func (_m *VoteCollector) ProcessBlock(block *model.Proposal) error { +func (_m *VoteCollector) ProcessBlock(block *model.SignedProposal) error { ret := _m.Called(block) if len(ret) == 0 { @@ -41,7 +41,7 @@ func (_m *VoteCollector) ProcessBlock(block *model.Proposal) error { } var r0 error - if rf, ok := ret.Get(0).(func(*model.Proposal) error); ok { + if rf, ok := ret.Get(0).(func(*model.SignedProposal) error); ok { r0 = rf(block) } else { r0 = ret.Error(0) diff --git a/consensus/hotstuff/mocks/vote_processor_factory.go b/consensus/hotstuff/mocks/vote_processor_factory.go index cb87a8568c3..5c4925cce4d 100644 --- a/consensus/hotstuff/mocks/vote_processor_factory.go +++ b/consensus/hotstuff/mocks/vote_processor_factory.go @@ -17,7 +17,7 @@ type VoteProcessorFactory struct { } // Create provides a mock function with given fields: log, proposal -func (_m *VoteProcessorFactory) Create(log zerolog.Logger, proposal *model.Proposal) (hotstuff.VerifyingVoteProcessor, error) { +func (_m *VoteProcessorFactory) Create(log zerolog.Logger, proposal *model.SignedProposal) (hotstuff.VerifyingVoteProcessor, error) { ret := _m.Called(log, proposal) if len(ret) == 0 { @@ -26,10 +26,10 @@ func (_m *VoteProcessorFactory) Create(log zerolog.Logger, proposal *model.Propo var r0 hotstuff.VerifyingVoteProcessor var r1 error - if rf, ok := ret.Get(0).(func(zerolog.Logger, *model.Proposal) (hotstuff.VerifyingVoteProcessor, error)); ok { + if rf, ok := ret.Get(0).(func(zerolog.Logger, *model.SignedProposal) (hotstuff.VerifyingVoteProcessor, error)); ok { return rf(log, proposal) } - if rf, ok := ret.Get(0).(func(zerolog.Logger, *model.Proposal) hotstuff.VerifyingVoteProcessor); ok { + if rf, ok := ret.Get(0).(func(zerolog.Logger, *model.SignedProposal) hotstuff.VerifyingVoteProcessor); ok { r0 = rf(log, proposal) } else { if ret.Get(0) != nil { @@ -37,7 +37,7 @@ func (_m *VoteProcessorFactory) Create(log zerolog.Logger, proposal *model.Propo } } - if rf, ok := ret.Get(1).(func(zerolog.Logger, *model.Proposal) error); ok { + if rf, ok := ret.Get(1).(func(zerolog.Logger, *model.SignedProposal) error); ok { r1 = rf(log, proposal) } else { r1 = ret.Error(1) diff --git a/consensus/hotstuff/model/errors.go b/consensus/hotstuff/model/errors.go index 4244d0ac531..034aec7e0fd 100644 --- a/consensus/hotstuff/model/errors.go +++ b/consensus/hotstuff/model/errors.go @@ -113,7 +113,7 @@ type MissingBlockError struct { } func (e MissingBlockError) Error() string { - return fmt.Sprintf("missing Proposal at view %d with ID %v", e.View, e.BlockID) + return fmt.Sprintf("missing SignedProposal at view %d with ID %v", e.View, e.BlockID) } // IsMissingBlockError returns whether an error is MissingBlockError @@ -165,11 +165,11 @@ func (e InvalidTCError) Unwrap() error { // InvalidProposalError indicates that the proposal is invalid type InvalidProposalError struct { - InvalidProposal *Proposal + InvalidProposal *SignedProposal Err error } -func NewInvalidProposalErrorf(proposal *Proposal, msg string, args ...interface{}) error { +func NewInvalidProposalErrorf(proposal *SignedProposal, msg string, args ...interface{}) error { return InvalidProposalError{ InvalidProposal: proposal, Err: fmt.Errorf(msg, args...), diff --git a/consensus/hotstuff/model/proposal.go b/consensus/hotstuff/model/proposal.go index 0cd19f85177..b4cdab34f7a 100644 --- a/consensus/hotstuff/model/proposal.go +++ b/consensus/hotstuff/model/proposal.go @@ -4,16 +4,32 @@ import ( "github.com/onflow/flow-go/model/flow" ) -// Proposal represent a new proposed block within HotStuff (and thus -// a header in the bigger picture), signed by the proposer. +// Proposal represents a block proposal under construction. +// In order to decide whether a proposal is safe to sign, HotStuff's Safety Rules require +// proof that the leader entered the respective view in a protocol-compliant manner. Specifically, +// we require the QC (mandatory part of every block) and optionally a TC (of the QC in the block +// is _not_ for the immediately previous view). Note that the proposer's signature for its own +// block is _not_ required. +// By explicitly differentiating the Proposal from the SignedProposal (extending Proposal by +// adding the proposer's signature), we can unify the algorithmic path of signing block proposals. +// This codifies the important aspect that a proposer's signature for their own block +// is conceptually also just a vote (we explicitly use that for aggregating votes, including the +// proposer's own vote to a QC). In order to express this conceptual equivalence in code, the +// voting logic in Safety Rules must also operate on an unsigned Proposal. type Proposal struct { Block *Block - SigData []byte LastViewTC *flow.TimeoutCertificate } +// SignedProposal represent a new proposed block within HotStuff (and thus +// a header in the bigger picture), signed by the proposer. +type SignedProposal struct { + Proposal + SigData []byte +} + // ProposerVote extracts the proposer vote from the proposal -func (p *Proposal) ProposerVote() *Vote { +func (p *SignedProposal) ProposerVote() *Vote { vote := Vote{ View: p.Block.View, BlockID: p.Block.BlockID, @@ -24,17 +40,19 @@ func (p *Proposal) ProposerVote() *Vote { } // ProposalFromFlow turns a flow header into a hotstuff block type. -func ProposalFromFlow(header *flow.Header) *Proposal { - proposal := Proposal{ - Block: BlockFromFlow(header), - SigData: header.ProposerSigData, - LastViewTC: header.LastViewTC, +func ProposalFromFlow(header *flow.Header) *SignedProposal { + proposal := SignedProposal{ + Proposal: Proposal{ + Block: BlockFromFlow(header), + LastViewTC: header.LastViewTC, + }, + SigData: header.ProposerSigData, } return &proposal } // ProposalToFlow turns a block proposal into a flow header. -func ProposalToFlow(proposal *Proposal) *flow.Header { +func ProposalToFlow(proposal *SignedProposal) *flow.Header { block := proposal.Block header := &flow.Header{ diff --git a/consensus/hotstuff/notifications/log_consumer.go b/consensus/hotstuff/notifications/log_consumer.go index f8baea639dc..3e454b61272 100644 --- a/consensus/hotstuff/notifications/log_consumer.go +++ b/consensus/hotstuff/notifications/log_consumer.go @@ -69,7 +69,7 @@ func (lc *LogConsumer) OnDoubleProposeDetected(block *model.Block, alt *model.Bl Msg("double proposal detected") } -func (lc *LogConsumer) OnReceiveProposal(currentView uint64, proposal *model.Proposal) { +func (lc *LogConsumer) OnReceiveProposal(currentView uint64, proposal *model.SignedProposal) { logger := lc.logBasicBlockData(lc.log.Debug(), proposal.Block). Uint64("cur_view", currentView) lastViewTC := proposal.LastViewTC @@ -197,7 +197,7 @@ func (lc *LogConsumer) OnInvalidVoteDetected(err model.InvalidVoteError) { Msgf("invalid vote detected: %s", err.Error()) } -func (lc *LogConsumer) OnVoteForInvalidBlockDetected(vote *model.Vote, proposal *model.Proposal) { +func (lc *LogConsumer) OnVoteForInvalidBlockDetected(vote *model.Vote, proposal *model.SignedProposal) { lc.log.Warn(). Str(logging.KeySuspicious, "true"). Uint64("vote_view", vote.View). diff --git a/consensus/hotstuff/notifications/noop_consumer.go b/consensus/hotstuff/notifications/noop_consumer.go index d8ad3e66e4f..38433c72b1a 100644 --- a/consensus/hotstuff/notifications/noop_consumer.go +++ b/consensus/hotstuff/notifications/noop_consumer.go @@ -32,7 +32,7 @@ func (*NoopParticipantConsumer) OnEventProcessed() {} func (*NoopParticipantConsumer) OnStart(uint64) {} -func (*NoopParticipantConsumer) OnReceiveProposal(uint64, *model.Proposal) {} +func (*NoopParticipantConsumer) OnReceiveProposal(uint64, *model.SignedProposal) {} func (*NoopParticipantConsumer) OnReceiveQc(uint64, *flow.QuorumCertificate) {} @@ -116,7 +116,8 @@ func (*NoopProposalViolationConsumer) OnDoubleVotingDetected(*model.Vote, *model func (*NoopProposalViolationConsumer) OnInvalidVoteDetected(model.InvalidVoteError) {} -func (*NoopProposalViolationConsumer) OnVoteForInvalidBlockDetected(*model.Vote, *model.Proposal) {} +func (*NoopProposalViolationConsumer) OnVoteForInvalidBlockDetected(*model.Vote, *model.SignedProposal) { +} func (*NoopProposalViolationConsumer) OnDoubleTimeoutDetected(*model.TimeoutObject, *model.TimeoutObject) { } diff --git a/consensus/hotstuff/notifications/pubsub/participant_distributor.go b/consensus/hotstuff/notifications/pubsub/participant_distributor.go index f5047cd7a53..4285a96b258 100644 --- a/consensus/hotstuff/notifications/pubsub/participant_distributor.go +++ b/consensus/hotstuff/notifications/pubsub/participant_distributor.go @@ -46,7 +46,7 @@ func (d *ParticipantDistributor) OnStart(currentView uint64) { } } -func (d *ParticipantDistributor) OnReceiveProposal(currentView uint64, proposal *model.Proposal) { +func (d *ParticipantDistributor) OnReceiveProposal(currentView uint64, proposal *model.SignedProposal) { d.lock.RLock() defer d.lock.RUnlock() for _, subscriber := range d.consumers { diff --git a/consensus/hotstuff/notifications/pubsub/vote_aggregation_violation_consumer.go b/consensus/hotstuff/notifications/pubsub/vote_aggregation_violation_consumer.go index d9d1e9baa26..7b75bd933e1 100644 --- a/consensus/hotstuff/notifications/pubsub/vote_aggregation_violation_consumer.go +++ b/consensus/hotstuff/notifications/pubsub/vote_aggregation_violation_consumer.go @@ -43,7 +43,7 @@ func (d *VoteAggregationViolationDistributor) OnInvalidVoteDetected(err model.In } } -func (d *VoteAggregationViolationDistributor) OnVoteForInvalidBlockDetected(vote *model.Vote, invalidProposal *model.Proposal) { +func (d *VoteAggregationViolationDistributor) OnVoteForInvalidBlockDetected(vote *model.Vote, invalidProposal *model.SignedProposal) { d.lock.RLock() defer d.lock.RUnlock() for _, subscriber := range d.consumers { diff --git a/consensus/hotstuff/notifications/slashing_violation_consumer.go b/consensus/hotstuff/notifications/slashing_violation_consumer.go index c03347ece6f..940c8270198 100644 --- a/consensus/hotstuff/notifications/slashing_violation_consumer.go +++ b/consensus/hotstuff/notifications/slashing_violation_consumer.go @@ -78,7 +78,7 @@ func (c *SlashingViolationsConsumer) OnInvalidTimeoutDetected(err model.InvalidT Msg("OnInvalidTimeoutDetected") } -func (c *SlashingViolationsConsumer) OnVoteForInvalidBlockDetected(vote *model.Vote, proposal *model.Proposal) { +func (c *SlashingViolationsConsumer) OnVoteForInvalidBlockDetected(vote *model.Vote, proposal *model.SignedProposal) { c.log.Warn(). Uint64("vote_view", vote.View). Hex("voted_block_id", vote.BlockID[:]). diff --git a/consensus/hotstuff/notifications/telemetry.go b/consensus/hotstuff/notifications/telemetry.go index d6cc3852179..a636bac3789 100644 --- a/consensus/hotstuff/notifications/telemetry.go +++ b/consensus/hotstuff/notifications/telemetry.go @@ -60,7 +60,7 @@ func (t *TelemetryConsumer) OnStart(currentView uint64) { t.pathHandler.NextStep().Msg("OnStart") } -func (t *TelemetryConsumer) OnReceiveProposal(currentView uint64, proposal *model.Proposal) { +func (t *TelemetryConsumer) OnReceiveProposal(currentView uint64, proposal *model.SignedProposal) { block := proposal.Block t.pathHandler.StartNextPath(currentView) step := t.pathHandler.NextStep(). diff --git a/consensus/hotstuff/pacemaker/pacemaker.go b/consensus/hotstuff/pacemaker/pacemaker.go index ae62aee0ea2..ab9475311cc 100644 --- a/consensus/hotstuff/pacemaker/pacemaker.go +++ b/consensus/hotstuff/pacemaker/pacemaker.go @@ -120,7 +120,7 @@ func (p *ActivePaceMaker) ProcessQC(qc *flow.QuorumCertificate) (*model.NewViewE // ProcessTC notifies the Pacemaker of a new timeout certificate, which may allow // Pacemaker to fast-forward its current view. -// A nil TC is an expected valid input, so that callers may pass in e.g. `Proposal.LastViewTC`, +// A nil TC is an expected valid input, so that callers may pass in e.g. `SignedProposal.LastViewTC`, // which may or may not have a value. // No errors are expected, any error should be treated as exception func (p *ActivePaceMaker) ProcessTC(tc *flow.TimeoutCertificate) (*model.NewViewEvent, error) { diff --git a/consensus/hotstuff/pacemaker/view_tracker.go b/consensus/hotstuff/pacemaker/view_tracker.go index b52822d0d5a..fc4dd74d7f8 100644 --- a/consensus/hotstuff/pacemaker/view_tracker.go +++ b/consensus/hotstuff/pacemaker/view_tracker.go @@ -86,7 +86,7 @@ func (vt *viewTracker) ProcessQC(qc *flow.QuorumCertificate) (uint64, error) { } // ProcessTC ingests a TC, which might advance the current view. A nil TC is accepted as -// input, so that callers may pass in e.g. `Proposal.LastViewTC`, which may or may not have +// input, so that callers may pass in e.g. `SignedProposal.LastViewTC`, which may or may not have // a value. It returns the resulting view after processing the TC and embedded QC. // No errors are expected, any error should be treated as exception func (vt *viewTracker) ProcessTC(tc *flow.TimeoutCertificate) (uint64, error) { diff --git a/consensus/hotstuff/safety_rules.go b/consensus/hotstuff/safety_rules.go index f32e8873c4f..00ac9a63f51 100644 --- a/consensus/hotstuff/safety_rules.go +++ b/consensus/hotstuff/safety_rules.go @@ -40,7 +40,7 @@ type SafetyRules interface { // * (nil, model.NoVoteError): If the safety module decides that it is not safe to vote for the given block. // This is a sentinel error and _expected_ during normal operation. // All other errors are unexpected and potential symptoms of uncovered edge cases or corrupted internal state (fatal). - ProduceVote(proposal *model.Proposal, curView uint64) (*model.Vote, error) + ProduceVote(proposal *model.SignedProposal, curView uint64) (*model.Vote, error) // ProduceTimeout takes current view, highest locally known QC and TC (optional, must be nil if and // only if QC is for previous view) and decides whether to produce timeout for current view. // Returns: diff --git a/consensus/hotstuff/safetyrules/safety_rules.go b/consensus/hotstuff/safetyrules/safety_rules.go index 1d352742929..57d9c26232e 100644 --- a/consensus/hotstuff/safetyrules/safety_rules.go +++ b/consensus/hotstuff/safetyrules/safety_rules.go @@ -73,14 +73,33 @@ func New( // This is a sentinel error and _expected_ during normal operation. // // All other errors are unexpected and potential symptoms of uncovered edge cases or corrupted internal state (fatal). -func (r *SafetyRules) ProduceVote(proposal *model.Proposal, curView uint64) (*model.Vote, error) { +func (r *SafetyRules) ProduceVote(signedProposal *model.SignedProposal, curView uint64) (*model.Vote, error) { + return r.produceVote(&signedProposal.Proposal, curView) +} + +// produceVote implements the core Safety Rules to validate whether it is safe to vote. +// This method is to be used for voting for other leaders' blocks as well as this node's own proposals +// under construction. We explicitly codify the important aspect that a proposer's signature for their +// own block is conceptually also just a vote (we explicitly use that property when aggregating votes and +// including the proposer's own vote into a QC). In order to express this conceptual equivalence in code, the +// voting logic in Safety Rules must also operate on an unsigned Proposal. +// +// The curView is taken as input to ensure SafetyRules will only vote for proposals at current view and prevent double voting. +// Returns: +// - (vote, nil): On the _first_ block for the current view that is safe to vote for. +// Subsequently, voter does _not_ vote for any other block with the same (or lower) view. +// - (nil, model.NoVoteError): If the voter decides that it does not want to vote for the given block. +// This is a sentinel error and _expected_ during normal operation. +// +// All other errors are unexpected and potential symptoms of uncovered edge cases or corrupted internal state (fatal). +func (r *SafetyRules) produceVote(proposal *model.Proposal, curView uint64) (*model.Vote, error) { block := proposal.Block // sanity checks: if curView != block.View { return nil, fmt.Errorf("expecting block for current view %d, but block's view is %d", curView, block.View) } - err := r.IsSafeToVote(proposal) + err := r.isSafeToVote(proposal) if err != nil { return nil, fmt.Errorf("not safe to vote for proposal %x: %w", proposal.Block.BlockID, err) } @@ -144,6 +163,7 @@ func (r *SafetyRules) ProduceVote(proposal *model.Proposal, curView uint64) (*mo } return vote, nil + } // ProduceTimeout takes current view, highest locally known QC and TC (optional, must be nil if and @@ -221,13 +241,13 @@ func (r *SafetyRules) SignOwnProposal(unsignedProposal *model.Proposal) (*model. return nil, fmt.Errorf("can't sign proposal for someone else's block") } - return r.ProduceVote(unsignedProposal, unsignedProposal.Block.View) + return r.produceVote(unsignedProposal, unsignedProposal.Block.View) } -// IsSafeToVote checks if this proposal is valid in terms of voting rules, if voting for this proposal won't break safety rules. +// isSafeToVote checks if this proposal is valid in terms of voting rules, if voting for this proposal won't break safety rules. // Expected errors during normal operations: // - NoVoteError if replica already acted during this view (either voted or generated timeout) -func (r *SafetyRules) IsSafeToVote(proposal *model.Proposal) error { +func (r *SafetyRules) isSafeToVote(proposal *model.Proposal) error { blockView := proposal.Block.View err := r.validateEvidenceForEnteringView(blockView, proposal.Block.QC, proposal.LastViewTC) diff --git a/consensus/hotstuff/safetyrules/safety_rules_test.go b/consensus/hotstuff/safetyrules/safety_rules_test.go index bfdfe782f45..c12b7c839a6 100644 --- a/consensus/hotstuff/safetyrules/safety_rules_test.go +++ b/consensus/hotstuff/safetyrules/safety_rules_test.go @@ -31,7 +31,7 @@ type SafetyRulesTestSuite struct { suite.Suite bootstrapBlock *model.Block - proposal *model.Proposal + proposal *model.SignedProposal proposerIdentity *flow.Identity ourIdentity *flow.Identity signer *mocks.Signer diff --git a/consensus/hotstuff/validator.go b/consensus/hotstuff/validator.go index be3313e9f26..ff40c550a2a 100644 --- a/consensus/hotstuff/validator.go +++ b/consensus/hotstuff/validator.go @@ -24,7 +24,7 @@ type Validator interface { // During normal operations, the following error returns are expected: // * model.InvalidProposalError if the block is invalid // * model.ErrViewForUnknownEpoch if the proposal refers unknown epoch - ValidateProposal(proposal *model.Proposal) error + ValidateProposal(proposal *model.SignedProposal) error // ValidateVote checks the validity of a vote. // Returns the full entity for the voter. During normal operations, diff --git a/consensus/hotstuff/validator/metrics_wrapper.go b/consensus/hotstuff/validator/metrics_wrapper.go index 8876acef248..5bd2aad9bec 100644 --- a/consensus/hotstuff/validator/metrics_wrapper.go +++ b/consensus/hotstuff/validator/metrics_wrapper.go @@ -40,7 +40,7 @@ func (w ValidatorMetricsWrapper) ValidateTC(tc *flow.TimeoutCertificate) error { return err } -func (w ValidatorMetricsWrapper) ValidateProposal(proposal *model.Proposal) error { +func (w ValidatorMetricsWrapper) ValidateProposal(proposal *model.SignedProposal) error { processStart := time.Now() err := w.validator.ValidateProposal(proposal) w.metrics.ValidatorProcessingDuration(time.Since(processStart)) diff --git a/consensus/hotstuff/validator/validator.go b/consensus/hotstuff/validator/validator.go index 933c3751619..597f0b5360f 100644 --- a/consensus/hotstuff/validator/validator.go +++ b/consensus/hotstuff/validator/validator.go @@ -201,7 +201,7 @@ func (v *Validator) ValidateQC(qc *flow.QuorumCertificate) error { // - model.ErrViewForUnknownEpoch if the proposal refers unknown epoch // // Any other error should be treated as exception -func (v *Validator) ValidateProposal(proposal *model.Proposal) error { +func (v *Validator) ValidateProposal(proposal *model.SignedProposal) error { qc := proposal.Block.QC block := proposal.Block diff --git a/consensus/hotstuff/validator/validator_test.go b/consensus/hotstuff/validator/validator_test.go index 7683d7cbe0b..a5a47cb1af2 100644 --- a/consensus/hotstuff/validator/validator_test.go +++ b/consensus/hotstuff/validator/validator_test.go @@ -35,7 +35,7 @@ type ProposalSuite struct { parent *model.Block block *model.Block voters flow.IdentitySkeletonList - proposal *model.Proposal + proposal *model.SignedProposal vote *model.Vote voter *flow.IdentitySkeleton committee *mocks.Replicas @@ -70,7 +70,7 @@ func (ps *ProposalSuite) SetupTest() { require.NoError(ps.T(), err) ps.voters = ps.participants.Filter(filter.HasNodeID[flow.Identity](voterIDs...)).ToSkeleton() - ps.proposal = &model.Proposal{Block: ps.block} + ps.proposal = &model.SignedProposal{Block: ps.block} ps.vote = ps.proposal.ProposerVote() ps.voter = ps.leader diff --git a/consensus/hotstuff/verification/combined_signer_v3.go b/consensus/hotstuff/verification/combined_signer_v3.go index 09651fc4925..a496af91387 100644 --- a/consensus/hotstuff/verification/combined_signer_v3.go +++ b/consensus/hotstuff/verification/combined_signer_v3.go @@ -53,29 +53,6 @@ func NewCombinedSignerV3( return sc } -// CreateProposal will create a proposal with a combined signature for the given block. -func (c *CombinedSignerV3) CreateProposal(block *model.Block) (*model.Proposal, error) { - - // check that the block is created by us - if block.ProposerID != c.staking.NodeID() { - return nil, fmt.Errorf("can't create proposal for someone else's block") - } - - // create the signature data - sigData, err := c.genSigData(block) - if err != nil { - return nil, fmt.Errorf("signing my proposal failed: %w", err) - } - - // create the proposal - proposal := &model.Proposal{ - Block: block, - SigData: sigData, - } - - return proposal, nil -} - // CreateVote will create a vote with a combined signature for the given block. func (c *CombinedSignerV3) CreateVote(block *model.Block) (*model.Vote, error) { diff --git a/consensus/hotstuff/verification/staking_signer.go b/consensus/hotstuff/verification/staking_signer.go index bbc590d2e07..cc1b9ca1291 100644 --- a/consensus/hotstuff/verification/staking_signer.go +++ b/consensus/hotstuff/verification/staking_signer.go @@ -40,29 +40,6 @@ func NewStakingSigner( return sc } -// CreateProposal will create a proposal with a staking signature for the given block. -func (c *StakingSigner) CreateProposal(block *model.Block) (*model.Proposal, error) { - - // check that the block is created by us - if block.ProposerID != c.signerID { - return nil, fmt.Errorf("can't create proposal for someone else's block") - } - - // create the signature data - sigData, err := c.genSigData(block) - if err != nil { - return nil, fmt.Errorf("signing my proposal failed: %w", err) - } - - // create the proposal - proposal := &model.Proposal{ - Block: block, - SigData: sigData, - } - - return proposal, nil -} - // CreateVote will create a vote with a staking signature for the given block. func (c *StakingSigner) CreateVote(block *model.Block) (*model.Vote, error) { diff --git a/consensus/hotstuff/vote_aggregator.go b/consensus/hotstuff/vote_aggregator.go index 14dc4f7dc2f..7c9bbcaad01 100644 --- a/consensus/hotstuff/vote_aggregator.go +++ b/consensus/hotstuff/vote_aggregator.go @@ -25,12 +25,12 @@ type VoteAggregator interface { // CAUTION: we expect that the input block's validity has been confirmed prior to calling AddBlock, // including the proposer's signature. Otherwise, VoteAggregator might crash or exhibit undefined // behaviour. - AddBlock(block *model.Proposal) + AddBlock(block *model.SignedProposal) // InvalidBlock notifies the VoteAggregator about an invalid proposal, so that it // can process votes for the invalid block and slash the voters. // No errors are expected during normal operations - InvalidBlock(block *model.Proposal) error + InvalidBlock(block *model.SignedProposal) error // PruneUpToView deletes all votes _below_ to the given view, as well as // related indices. We only retain and process whose view is equal or larger diff --git a/consensus/hotstuff/vote_collector.go b/consensus/hotstuff/vote_collector.go index 157ef5338a7..3a259808dc4 100644 --- a/consensus/hotstuff/vote_collector.go +++ b/consensus/hotstuff/vote_collector.go @@ -61,7 +61,7 @@ type VoteCollector interface { // It returns nil if the block is valid. // It returns model.InvalidProposalError if block is invalid. // It returns other error if there is exception processing the block. - ProcessBlock(block *model.Proposal) error + ProcessBlock(block *model.SignedProposal) error // AddVote adds a vote to the collector // When enough votes have been added to produce a QC, the QC will be created asynchronously, and @@ -116,5 +116,5 @@ type VoteProcessorFactory interface { // Caller can be sure that proposal vote was successfully verified and processed. // Expected error returns during normal operations: // * model.InvalidProposalError - proposal has invalid proposer vote - Create(log zerolog.Logger, proposal *model.Proposal) (VerifyingVoteProcessor, error) + Create(log zerolog.Logger, proposal *model.SignedProposal) (VerifyingVoteProcessor, error) } diff --git a/consensus/hotstuff/voteaggregator/vote_aggregator.go b/consensus/hotstuff/voteaggregator/vote_aggregator.go index 6471cc6ada6..efb2e476bfc 100644 --- a/consensus/hotstuff/voteaggregator/vote_aggregator.go +++ b/consensus/hotstuff/voteaggregator/vote_aggregator.go @@ -156,7 +156,7 @@ func (va *VoteAggregator) processQueuedMessages(ctx context.Context) error { msg, ok := va.queuedBlocks.Pop() if ok { - block := msg.(*model.Proposal) + block := msg.(*model.SignedProposal) err := va.processQueuedBlock(block) if err != nil { return fmt.Errorf("could not process pending block %v: %w", block.Block.BlockID, err) @@ -224,7 +224,7 @@ func (va *VoteAggregator) processQueuedVote(vote *model.Vote) error { // including the proposer's signature. Otherwise, VoteAggregator might crash or exhibit undefined // behaviour. // No errors are expected during normal operation. -func (va *VoteAggregator) processQueuedBlock(block *model.Proposal) error { +func (va *VoteAggregator) processQueuedBlock(block *model.SignedProposal) error { // check if the block is for a view that has already been pruned (and is thus stale) if block.Block.View < va.lowestRetainedView.Value() { return nil @@ -293,7 +293,7 @@ func (va *VoteAggregator) AddVote(vote *model.Vote) { // CAUTION: we expect that the input block's validity has been confirmed prior to calling AddBlock, // including the proposer's signature. Otherwise, VoteAggregator might crash or exhibit undefined // behaviour. -func (va *VoteAggregator) AddBlock(block *model.Proposal) { +func (va *VoteAggregator) AddBlock(block *model.SignedProposal) { // It's ok to silently drop blocks in case our processing pipeline is full. // It means that we are probably catching up. if ok := va.queuedBlocks.Push(block); ok { @@ -306,7 +306,7 @@ func (va *VoteAggregator) AddBlock(block *model.Proposal) { // InvalidBlock notifies the VoteAggregator about an invalid proposal, so that it // can process votes for the invalid block and slash the voters. // No errors are expected during normal operations -func (va *VoteAggregator) InvalidBlock(proposal *model.Proposal) error { +func (va *VoteAggregator) InvalidBlock(proposal *model.SignedProposal) error { slashingVoteConsumer := func(vote *model.Vote) { if proposal.Block.BlockID == vote.BlockID { va.notifier.OnVoteForInvalidBlockDetected(vote, proposal) diff --git a/consensus/hotstuff/votecollector/factory.go b/consensus/hotstuff/votecollector/factory.go index 2c515fc052c..b444bc35ca7 100644 --- a/consensus/hotstuff/votecollector/factory.go +++ b/consensus/hotstuff/votecollector/factory.go @@ -16,7 +16,7 @@ import ( // CAUTION: the baseFactory creates the VerifyingVoteProcessor for the given block. It // does _not_ check the proposer's vote for its own block. The API reflects this by // expecting a `model.Block` as input (which does _not_ contain the proposer vote) as -// opposed to `model.Proposal` (combines block with proposer's vote). +// opposed to `model.SignedProposal` (combines block with proposer's vote). // Therefore, baseFactory does _not_ implement `hotstuff.VoteProcessorFactory` by itself. // The VoteProcessorFactory adds the missing logic to verify the proposer's vote, by // wrapping the baseFactory (decorator pattern). @@ -40,7 +40,7 @@ var _ hotstuff.VoteProcessorFactory = (*VoteProcessorFactory)(nil) // A VerifyingVoteProcessor are only created for proposals with valid proposer votes. // Expected error returns during normal operations: // * model.InvalidProposalError - proposal has invalid proposer vote -func (f *VoteProcessorFactory) Create(log zerolog.Logger, proposal *model.Proposal) (hotstuff.VerifyingVoteProcessor, error) { +func (f *VoteProcessorFactory) Create(log zerolog.Logger, proposal *model.SignedProposal) (hotstuff.VerifyingVoteProcessor, error) { processor, err := f.baseFactory(log, proposal.Block) if err != nil { return nil, fmt.Errorf("instantiating vote processor for block %v failed: %w", proposal.Block.BlockID, err) diff --git a/consensus/hotstuff/votecollector/statemachine.go b/consensus/hotstuff/votecollector/statemachine.go index d62159ea9ef..60558cf2aaf 100644 --- a/consensus/hotstuff/votecollector/statemachine.go +++ b/consensus/hotstuff/votecollector/statemachine.go @@ -18,7 +18,7 @@ var ( ) // VerifyingVoteProcessorFactory generates hotstuff.VerifyingVoteCollector instances -type VerifyingVoteProcessorFactory = func(log zerolog.Logger, proposal *model.Proposal) (hotstuff.VerifyingVoteProcessor, error) +type VerifyingVoteProcessorFactory = func(log zerolog.Logger, proposal *model.SignedProposal) (hotstuff.VerifyingVoteProcessor, error) // VoteCollector implements a state machine for transition between different states of vote collector type VoteCollector struct { @@ -175,7 +175,7 @@ func (m *VoteCollector) View() uint64 { // CachingVotes -> VerifyingVotes // CachingVotes -> Invalid // VerifyingVotes -> Invalid -func (m *VoteCollector) ProcessBlock(proposal *model.Proposal) error { +func (m *VoteCollector) ProcessBlock(proposal *model.SignedProposal) error { if proposal.Block.View != m.View() { return fmt.Errorf("this VoteCollector requires a proposal for view %d but received block %v with view %d", @@ -243,7 +243,7 @@ func (m *VoteCollector) RegisterVoteConsumer(consumer hotstuff.VoteConsumer) { // Error returns: // * ErrDifferentCollectorState if the VoteCollector's state is _not_ `CachingVotes` // * all other errors are unexpected and potential symptoms of internal bugs or state corruption (fatal) -func (m *VoteCollector) caching2Verifying(proposal *model.Proposal) error { +func (m *VoteCollector) caching2Verifying(proposal *model.SignedProposal) error { blockID := proposal.Block.BlockID newProc, err := m.createVerifyingProcessor(m.log, proposal) if err != nil { diff --git a/consensus/hotstuff/votecollector/statemachine_test.go b/consensus/hotstuff/votecollector/statemachine_test.go index 007dcce1fe2..b2aaa313c51 100644 --- a/consensus/hotstuff/votecollector/statemachine_test.go +++ b/consensus/hotstuff/votecollector/statemachine_test.go @@ -51,7 +51,7 @@ func (s *StateMachineTestSuite) SetupTest() { s.mockedProcessors = make(map[flow.Identifier]*mocks.VerifyingVoteProcessor) s.notifier = mocks.NewVoteAggregationConsumer(s.T()) - s.factoryMethod = func(log zerolog.Logger, block *model.Proposal) (hotstuff.VerifyingVoteProcessor, error) { + s.factoryMethod = func(log zerolog.Logger, block *model.SignedProposal) (hotstuff.VerifyingVoteProcessor, error) { if processor, found := s.mockedProcessors[block.Block.BlockID]; found { return processor, nil } @@ -64,7 +64,7 @@ func (s *StateMachineTestSuite) SetupTest() { // prepareMockedProcessor prepares a mocked processor and stores it in map, later it will be used // to mock behavior of verifying vote processor. -func (s *StateMachineTestSuite) prepareMockedProcessor(proposal *model.Proposal) *mocks.VerifyingVoteProcessor { +func (s *StateMachineTestSuite) prepareMockedProcessor(proposal *model.SignedProposal) *mocks.VerifyingVoteProcessor { processor := &mocks.VerifyingVoteProcessor{} processor.On("Block").Return(func() *model.Block { return proposal.Block @@ -101,7 +101,7 @@ func (s *StateMachineTestSuite) TestStatus_StateTransitions() { // factory are handed through (potentially wrapped), but are not replaced. func (s *StateMachineTestSuite) Test_FactoryErrorPropagation() { factoryError := errors.New("factory error") - factory := func(log zerolog.Logger, block *model.Proposal) (hotstuff.VerifyingVoteProcessor, error) { + factory := func(log zerolog.Logger, block *model.SignedProposal) (hotstuff.VerifyingVoteProcessor, error) { return nil, factoryError } s.collector.createVerifyingProcessor = factory diff --git a/consensus/hotstuff/votecollector/testutil.go b/consensus/hotstuff/votecollector/testutil.go index e36aca23170..2e305d96b66 100644 --- a/consensus/hotstuff/votecollector/testutil.go +++ b/consensus/hotstuff/votecollector/testutil.go @@ -22,7 +22,7 @@ type VoteProcessorTestSuiteBase struct { stakingAggregator *mockhotstuff.WeightedSignatureAggregator minRequiredWeight uint64 - proposal *model.Proposal + proposal *model.SignedProposal } func (s *VoteProcessorTestSuiteBase) SetupTest() { diff --git a/consensus/recovery/recover.go b/consensus/recovery/recover.go index a470aedc3ce..0d7d7616d06 100644 --- a/consensus/recovery/recover.go +++ b/consensus/recovery/recover.go @@ -12,7 +12,7 @@ import ( // BlockScanner describes a function for ingesting pending blocks. // Any returned errors are considered fatal. -type BlockScanner func(proposal *model.Proposal) error +type BlockScanner func(proposal *model.SignedProposal) error // Recover is a utility method for recovering the HotStuff state after a restart. // It receives the list `pending` containing _all_ blocks that @@ -48,7 +48,7 @@ func Recover(log zerolog.Logger, pending []*flow.Header, scanners ...BlockScanne // finalized block. Caution, input blocks must be valid and in parent-first order // (unless parent is the latest finalized block). func ForksState(forks hotstuff.Forks) BlockScanner { - return func(proposal *model.Proposal) error { + return func(proposal *model.SignedProposal) error { err := forks.AddValidatedBlock(proposal.Block) if err != nil { return fmt.Errorf("could not add block %v to forks: %w", proposal.Block.BlockID, err) @@ -63,7 +63,7 @@ func ForksState(forks hotstuff.Forks) BlockScanner { // // Caution: input blocks must be valid. func VoteAggregatorState(voteAggregator hotstuff.VoteAggregator) BlockScanner { - return func(proposal *model.Proposal) error { + return func(proposal *model.SignedProposal) error { voteAggregator.AddBlock(proposal) return nil } @@ -72,7 +72,7 @@ func VoteAggregatorState(voteAggregator hotstuff.VoteAggregator) BlockScanner { // CollectParentQCs collects all parent QCs included in the blocks descending from the // latest finalized block. Caution, input blocks must be valid. func CollectParentQCs(collector Collector[*flow.QuorumCertificate]) BlockScanner { - return func(proposal *model.Proposal) error { + return func(proposal *model.SignedProposal) error { qc := proposal.Block.QC if qc != nil { collector.Append(qc) @@ -84,7 +84,7 @@ func CollectParentQCs(collector Collector[*flow.QuorumCertificate]) BlockScanner // CollectTCs collect all TCs included in the blocks descending from the // latest finalized block. Caution, input blocks must be valid. func CollectTCs(collector Collector[*flow.TimeoutCertificate]) BlockScanner { - return func(proposal *model.Proposal) error { + return func(proposal *model.SignedProposal) error { tc := proposal.LastViewTC if tc != nil { collector.Append(tc) diff --git a/consensus/recovery/recover_test.go b/consensus/recovery/recover_test.go index ac0fb0c3d4f..836685fc7ad 100644 --- a/consensus/recovery/recover_test.go +++ b/consensus/recovery/recover_test.go @@ -19,8 +19,8 @@ func TestRecover(t *testing.T) { } // Recover with `pending` blocks and record what blocks are forwarded to `onProposal` - recovered := make([]*model.Proposal, 0) - scanner := func(block *model.Proposal) error { + recovered := make([]*model.SignedProposal, 0) + scanner := func(block *model.SignedProposal) error { recovered = append(recovered, block) return nil } @@ -35,7 +35,7 @@ func TestRecover(t *testing.T) { } func TestRecoverEmptyInput(t *testing.T) { - scanner := func(block *model.Proposal) error { + scanner := func(block *model.SignedProposal) error { require.Fail(t, "no proposal expected") return nil } diff --git a/engine/collection/compliance/core_test.go b/engine/collection/compliance/core_test.go index 2ed1292ee32..555574b99e8 100644 --- a/engine/collection/compliance/core_test.go +++ b/engine/collection/compliance/core_test.go @@ -552,7 +552,7 @@ func (cs *CoreSuite) TestProposalBufferingOrder() { } cs.hotstuff.On("SubmitProposal", mock.Anything).Times(4).Run( func(args mock.Arguments) { - header := args.Get(0).(*model.Proposal).Block + header := args.Get(0).(*model.SignedProposal).Block assert.Equal(cs.T(), order[index], header.BlockID, "should submit correct header to hotstuff") index++ cs.headerDB[header.BlockID] = proposalsLookup[header.BlockID] diff --git a/engine/consensus/compliance/core_test.go b/engine/consensus/compliance/core_test.go index 494d1d0e91d..a85ffb15764 100644 --- a/engine/consensus/compliance/core_test.go +++ b/engine/consensus/compliance/core_test.go @@ -618,7 +618,7 @@ func (cs *CoreSuite) TestProposalBufferingOrder() { } cs.hotstuff.On("SubmitProposal", mock.Anything).Times(4).Run( func(args mock.Arguments) { - proposal := args.Get(0).(*model.Proposal) + proposal := args.Get(0).(*model.SignedProposal) header := proposal.Block if calls == 0 { // first header processed must be the common parent diff --git a/module/hotstuff.go b/module/hotstuff.go index 8610ce0bce1..785aeed9988 100644 --- a/module/hotstuff.go +++ b/module/hotstuff.go @@ -22,7 +22,7 @@ type HotStuff interface { // // Block proposals must be submitted in order and only if they extend a // block already known to HotStuff core. - SubmitProposal(proposal *model.Proposal) + SubmitProposal(proposal *model.SignedProposal) } // HotStuffFollower is run by non-consensus nodes to observe the block chain diff --git a/module/mock/hot_stuff.go b/module/mock/hot_stuff.go index 4801da856c7..7c1ba755027 100644 --- a/module/mock/hot_stuff.go +++ b/module/mock/hot_stuff.go @@ -60,7 +60,7 @@ func (_m *HotStuff) Start(_a0 irrecoverable.SignalerContext) { } // SubmitProposal provides a mock function with given fields: proposal -func (_m *HotStuff) SubmitProposal(proposal *model.Proposal) { +func (_m *HotStuff) SubmitProposal(proposal *model.SignedProposal) { _m.Called(proposal) } From 61c4856e63e2940961eaa3d961ca764e707480ce Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 2 Oct 2024 12:47:37 +0300 Subject: [PATCH 02/13] Updated mocks --- consensus/hotstuff/mocks/safety_rules.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/consensus/hotstuff/mocks/safety_rules.go b/consensus/hotstuff/mocks/safety_rules.go index e1d87474521..83f33269542 100644 --- a/consensus/hotstuff/mocks/safety_rules.go +++ b/consensus/hotstuff/mocks/safety_rules.go @@ -76,7 +76,7 @@ func (_m *SafetyRules) ProduceVote(proposal *model.SignedProposal, curView uint6 } // SignOwnProposal provides a mock function with given fields: unsignedProposal -func (_m *SafetyRules) SignOwnProposal(unsignedProposal *model.SignedProposal) (*model.Vote, error) { +func (_m *SafetyRules) SignOwnProposal(unsignedProposal *model.Proposal) (*model.Vote, error) { ret := _m.Called(unsignedProposal) if len(ret) == 0 { @@ -85,10 +85,10 @@ func (_m *SafetyRules) SignOwnProposal(unsignedProposal *model.SignedProposal) ( var r0 *model.Vote var r1 error - if rf, ok := ret.Get(0).(func(*model.SignedProposal) (*model.Vote, error)); ok { + if rf, ok := ret.Get(0).(func(*model.Proposal) (*model.Vote, error)); ok { return rf(unsignedProposal) } - if rf, ok := ret.Get(0).(func(*model.SignedProposal) *model.Vote); ok { + if rf, ok := ret.Get(0).(func(*model.Proposal) *model.Vote); ok { r0 = rf(unsignedProposal) } else { if ret.Get(0) != nil { @@ -96,7 +96,7 @@ func (_m *SafetyRules) SignOwnProposal(unsignedProposal *model.SignedProposal) ( } } - if rf, ok := ret.Get(1).(func(*model.SignedProposal) error); ok { + if rf, ok := ret.Get(1).(func(*model.Proposal) error); ok { r1 = rf(unsignedProposal) } else { r1 = ret.Error(1) From 93b71a420f2070f604ee366731a9d4636d04299a Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 2 Oct 2024 12:48:44 +0300 Subject: [PATCH 03/13] Updated naming of utility functions --- .../hotstuff/blockproducer/safety_rules_wrapper.go | 2 +- .../hotstuff/eventhandler/event_handler_test.go | 4 ++-- consensus/hotstuff/eventloop/event_loop_test.go | 2 +- consensus/hotstuff/integration/connect_test.go | 2 +- consensus/hotstuff/integration/instance_test.go | 4 ++-- consensus/hotstuff/model/proposal.go | 8 ++++---- .../verification/combined_signer_v2_test.go | 4 ++-- consensus/recovery/recover.go | 2 +- consensus/recovery/recover_test.go | 2 +- engine/collection/compliance/core.go | 4 ++-- engine/collection/compliance/core_test.go | 10 +++++----- engine/collection/compliance/engine_test.go | 4 ++-- engine/collection/message_hub/message_hub.go | 2 +- engine/collection/message_hub/message_hub_test.go | 4 ++-- engine/common/follower/compliance_core.go | 2 +- engine/common/follower/compliance_core_test.go | 12 ++++++------ engine/consensus/compliance/core.go | 4 ++-- engine/consensus/compliance/core_test.go | 14 +++++++------- engine/consensus/compliance/engine_test.go | 4 ++-- engine/consensus/message_hub/message_hub.go | 2 +- engine/consensus/message_hub/message_hub_test.go | 4 ++-- 21 files changed, 48 insertions(+), 48 deletions(-) diff --git a/consensus/hotstuff/blockproducer/safety_rules_wrapper.go b/consensus/hotstuff/blockproducer/safety_rules_wrapper.go index 6d960eb836f..c004e27a33f 100644 --- a/consensus/hotstuff/blockproducer/safety_rules_wrapper.go +++ b/consensus/hotstuff/blockproducer/safety_rules_wrapper.go @@ -67,7 +67,7 @@ func (w *safetyRulesConcurrencyWrapper) Sign(unsignedHeader *flow.Header) error } // signer is now in state 1, and this thread is the only one every going to execute the following logic // signature for own block is structurally a vote - vote, err := w.safetyRules.SignOwnProposal(model.ProposalFromFlow(unsignedHeader)) + vote, err := w.safetyRules.SignOwnProposal(model.SignedProposalFromFlow(unsignedHeader)) if err != nil { return fmt.Errorf("could not sign block proposal: %w", err) } diff --git a/consensus/hotstuff/eventhandler/event_handler_test.go b/consensus/hotstuff/eventhandler/event_handler_test.go index 6341138c607..77080ff7583 100644 --- a/consensus/hotstuff/eventhandler/event_handler_test.go +++ b/consensus/hotstuff/eventhandler/event_handler_test.go @@ -228,7 +228,7 @@ type BlockProducer struct { } func (b *BlockProducer) MakeBlockProposal(view uint64, qc *flow.QuorumCertificate, lastViewTC *flow.TimeoutCertificate) (*flow.Header, error) { - return model.ProposalToFlow(&model.SignedProposal{ + return model.SignedProposalToFlow(&model.SignedProposal{ Block: helper.MakeBlock( helper.WithBlockView(view), helper.WithBlockQC(qc), @@ -670,7 +670,7 @@ func (es *EventHandlerSuite) TestOnReceiveTc_NextLeaderProposes() { // proposed block should contain valid newest QC and lastViewTC expectedNewestQC := es.paceMaker.NewestQC() - proposal := model.ProposalFromFlow(header) + proposal := model.SignedProposalFromFlow(header) require.Equal(es.T(), expectedNewestQC, proposal.Block.QC) require.Equal(es.T(), es.paceMaker.LastViewTC(), proposal.LastViewTC) }).Once() diff --git a/consensus/hotstuff/eventloop/event_loop_test.go b/consensus/hotstuff/eventloop/event_loop_test.go index 1f57050922b..6cd262b8c63 100644 --- a/consensus/hotstuff/eventloop/event_loop_test.go +++ b/consensus/hotstuff/eventloop/event_loop_test.go @@ -271,7 +271,7 @@ func TestReadyDoneWithStartTime(t *testing.T) { parentBlock := unittest.BlockHeaderFixture() block := unittest.BlockHeaderWithParentFixture(parentBlock) - eventLoop.SubmitProposal(model.ProposalFromFlow(block)) + eventLoop.SubmitProposal(model.SignedProposalFromFlow(block)) unittest.RequireCloseBefore(t, done, startTimeDuration+100*time.Millisecond, "proposal wasn't received") cancel() diff --git a/consensus/hotstuff/integration/connect_test.go b/consensus/hotstuff/integration/connect_test.go index a254e0f9f3c..177a8d0244b 100644 --- a/consensus/hotstuff/integration/connect_test.go +++ b/consensus/hotstuff/integration/connect_test.go @@ -37,7 +37,7 @@ func Connect(t *testing.T, instances []*Instance) { } // convert into proposal immediately - proposal := model.ProposalFromFlow(header) + proposal := model.SignedProposalFromFlow(header) // store locally and loop back to engine for processing sender.ProcessBlock(proposal) diff --git a/consensus/hotstuff/integration/instance_test.go b/consensus/hotstuff/integration/instance_test.go index 4f5b706ac51..8e777bf53e5 100644 --- a/consensus/hotstuff/integration/instance_test.go +++ b/consensus/hotstuff/integration/instance_test.go @@ -294,7 +294,7 @@ func NewInstance(t *testing.T, options ...Option) *Instance { } // convert into proposal immediately - proposal := model.ProposalFromFlow(header) + proposal := model.SignedProposalFromFlow(header) // store locally and loop back to engine for processing in.ProcessBlock(proposal) @@ -637,7 +637,7 @@ func (in *Instance) ProcessBlock(proposal *model.SignedProposal) { if parentExists { next := proposal for next != nil { - in.headers[next.Block.BlockID] = model.ProposalToFlow(next) + in.headers[next.Block.BlockID] = model.SignedProposalToFlow(next) in.queue <- next // keep processing the pending blocks diff --git a/consensus/hotstuff/model/proposal.go b/consensus/hotstuff/model/proposal.go index b4cdab34f7a..e4b68508756 100644 --- a/consensus/hotstuff/model/proposal.go +++ b/consensus/hotstuff/model/proposal.go @@ -39,8 +39,8 @@ func (p *SignedProposal) ProposerVote() *Vote { return &vote } -// ProposalFromFlow turns a flow header into a hotstuff block type. -func ProposalFromFlow(header *flow.Header) *SignedProposal { +// SignedProposalFromFlow turns a flow header into a hotstuff block type. +func SignedProposalFromFlow(header *flow.Header) *SignedProposal { proposal := SignedProposal{ Proposal: Proposal{ Block: BlockFromFlow(header), @@ -51,8 +51,8 @@ func ProposalFromFlow(header *flow.Header) *SignedProposal { return &proposal } -// ProposalToFlow turns a block proposal into a flow header. -func ProposalToFlow(proposal *SignedProposal) *flow.Header { +// SignedProposalToFlow turns a block proposal into a flow header. +func SignedProposalToFlow(proposal *SignedProposal) *flow.Header { block := proposal.Block header := &flow.Header{ diff --git a/consensus/hotstuff/verification/combined_signer_v2_test.go b/consensus/hotstuff/verification/combined_signer_v2_test.go index 1ff02a29fdd..d20cdffeca3 100644 --- a/consensus/hotstuff/verification/combined_signer_v2_test.go +++ b/consensus/hotstuff/verification/combined_signer_v2_test.go @@ -38,7 +38,7 @@ func TestCombinedSignWithBeaconKey(t *testing.T) { fblock.Header.View = proposerView fblock.Header.ParentView = proposerView - 1 fblock.Header.LastViewTC = nil - proposal := model.ProposalFromFlow(fblock.Header) + proposal := model.SignedProposalFromFlow(fblock.Header) signerID := fblock.Header.ProposerID beaconKeyStore := modulemock.NewRandomBeaconKeyStore(t) @@ -146,7 +146,7 @@ func TestCombinedSignWithNoBeaconKey(t *testing.T) { fblock.Header.View = proposerView fblock.Header.ParentView = proposerView - 1 fblock.Header.LastViewTC = nil - proposal := model.ProposalFromFlow(fblock.Header) + proposal := model.SignedProposalFromFlow(fblock.Header) signerID := fblock.Header.ProposerID beaconKeyStore := modulemock.NewRandomBeaconKeyStore(t) diff --git a/consensus/recovery/recover.go b/consensus/recovery/recover.go index 0d7d7616d06..1d85eeab65e 100644 --- a/consensus/recovery/recover.go +++ b/consensus/recovery/recover.go @@ -27,7 +27,7 @@ func Recover(log zerolog.Logger, pending []*flow.Header, scanners ...BlockScanne // add all pending blocks to forks for _, header := range pending { - proposal := model.ProposalFromFlow(header) // convert the header into a proposal + proposal := model.SignedProposalFromFlow(header) // convert the header into a proposal for _, s := range scanners { err := s(proposal) if err != nil { diff --git a/consensus/recovery/recover_test.go b/consensus/recovery/recover_test.go index 836685fc7ad..f3db1a6c42b 100644 --- a/consensus/recovery/recover_test.go +++ b/consensus/recovery/recover_test.go @@ -30,7 +30,7 @@ func TestRecover(t *testing.T) { // should forward blocks in exact order, just converting flow.Header to pending block require.Len(t, recovered, len(pending)) for i, r := range recovered { - require.Equal(t, model.ProposalFromFlow(pending[i]), r) + require.Equal(t, model.SignedProposalFromFlow(pending[i]), r) } } diff --git a/engine/collection/compliance/core.go b/engine/collection/compliance/core.go index c341d7cb146..dc5432d2925 100644 --- a/engine/collection/compliance/core.go +++ b/engine/collection/compliance/core.go @@ -260,7 +260,7 @@ func (c *Core) processBlockAndDescendants(proposal flow.Slashable[*cluster.Block }) // notify VoteAggregator about the invalid block - err = c.voteAggregator.InvalidBlock(model.ProposalFromFlow(header)) + err = c.voteAggregator.InvalidBlock(model.SignedProposalFromFlow(header)) if err != nil { if mempool.IsBelowPrunedThresholdError(err) { log.Warn().Msg("received invalid block, but is below pruned threshold") @@ -317,7 +317,7 @@ func (c *Core) processBlockProposal(proposal *cluster.Block) error { Logger() log.Info().Msg("processing block proposal") - hotstuffProposal := model.ProposalFromFlow(header) + hotstuffProposal := model.SignedProposalFromFlow(header) err := c.validator.ValidateProposal(hotstuffProposal) if err != nil { if model.IsInvalidProposalError(err) { diff --git a/engine/collection/compliance/core_test.go b/engine/collection/compliance/core_test.go index 555574b99e8..6fd27bc3963 100644 --- a/engine/collection/compliance/core_test.go +++ b/engine/collection/compliance/core_test.go @@ -206,7 +206,7 @@ func (cs *CoreSuite) TestOnBlockProposalValidParent() { // store the data for retrieval cs.headerDB[block.Header.ParentID] = cs.head - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) cs.validator.On("ValidateProposal", hotstuffProposal).Return(nil) cs.voteAggregator.On("AddBlock", hotstuffProposal).Once() cs.hotstuff.On("SubmitProposal", hotstuffProposal) @@ -232,7 +232,7 @@ func (cs *CoreSuite) TestOnBlockProposalValidAncestor() { cs.headerDB[parent.ID()] = &parent cs.headerDB[ancestor.ID()] = &ancestor - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) cs.validator.On("ValidateProposal", hotstuffProposal).Return(nil) cs.voteAggregator.On("AddBlock", hotstuffProposal).Once() cs.hotstuff.On("SubmitProposal", hotstuffProposal).Once() @@ -280,7 +280,7 @@ func (cs *CoreSuite) TestOnBlockProposal_FailsHotStuffValidation() { parent := unittest.ClusterBlockWithParent(&ancestor) block := unittest.ClusterBlockWithParent(&parent) proposal := messages.NewClusterBlockProposal(&block) - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) // store the data for retrieval cs.headerDB[parent.ID()] = &parent @@ -363,7 +363,7 @@ func (cs *CoreSuite) TestOnBlockProposal_FailsProtocolStateValidation() { parent := unittest.ClusterBlockWithParent(&ancestor) block := unittest.ClusterBlockWithParent(&parent) proposal := messages.NewClusterBlockProposal(&block) - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) // store the data for retrieval cs.headerDB[parent.ID()] = &parent @@ -476,7 +476,7 @@ func (cs *CoreSuite) TestProcessBlockAndDescendants() { cs.childrenDB[parentID] = append(cs.childrenDB[parentID], pending3) for _, block := range []cluster.Block{parent, block1, block2, block3} { - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) cs.validator.On("ValidateProposal", hotstuffProposal).Return(nil) cs.voteAggregator.On("AddBlock", hotstuffProposal).Once() cs.hotstuff.On("SubmitProposal", hotstuffProposal).Once() diff --git a/engine/collection/compliance/engine_test.go b/engine/collection/compliance/engine_test.go index 5ad01b19566..5c3a2c9cd99 100644 --- a/engine/collection/compliance/engine_test.go +++ b/engine/collection/compliance/engine_test.go @@ -165,7 +165,7 @@ func (cs *EngineSuite) TestSubmittingMultipleEntries() { for i := 0; i < blockCount; i++ { block := unittest.ClusterBlockWithParent(cs.head) proposal := messages.NewClusterBlockProposal(&block) - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) cs.hotstuff.On("SubmitProposal", hotstuffProposal).Return().Once() cs.voteAggregator.On("AddBlock", hotstuffProposal).Once() cs.validator.On("ValidateProposal", hotstuffProposal).Return(nil).Once() @@ -183,7 +183,7 @@ func (cs *EngineSuite) TestSubmittingMultipleEntries() { block := unittest.ClusterBlockWithParent(cs.head) proposal := messages.NewClusterBlockProposal(&block) - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) cs.hotstuff.On("SubmitProposal", hotstuffProposal).Once() cs.voteAggregator.On("AddBlock", hotstuffProposal).Once() cs.validator.On("ValidateProposal", hotstuffProposal).Return(nil).Once() diff --git a/engine/collection/message_hub/message_hub.go b/engine/collection/message_hub/message_hub.go index f2241dffb73..896d5ce0b5a 100644 --- a/engine/collection/message_hub/message_hub.go +++ b/engine/collection/message_hub/message_hub.go @@ -398,7 +398,7 @@ func (h *MessageHub) OnOwnProposal(proposal *flow.Header, targetPublicationTime return } - hotstuffProposal := model.ProposalFromFlow(proposal) + hotstuffProposal := model.SignedProposalFromFlow(proposal) // notify vote aggregator that new block proposal is available, in case we are next leader h.voteAggregator.AddBlock(hotstuffProposal) // non-blocking diff --git a/engine/collection/message_hub/message_hub_test.go b/engine/collection/message_hub/message_hub_test.go index d6032fa8e6f..d1f66a1af90 100644 --- a/engine/collection/message_hub/message_hub_test.go +++ b/engine/collection/message_hub/message_hub_test.go @@ -273,7 +273,7 @@ func (s *MessageHubSuite) TestOnOwnProposal() { expectedBroadcastMsg := messages.NewClusterBlockProposal(&block) submitted := make(chan struct{}) // closed when proposal is submitted to hotstuff - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) s.voteAggregator.On("AddBlock", hotstuffProposal).Once() s.hotstuff.On("SubmitProposal", hotstuffProposal). Run(func(args mock.Arguments) { close(submitted) }). @@ -334,7 +334,7 @@ func (s *MessageHubSuite) TestProcessMultipleMessagesHappyPath() { s.payloads.On("ByBlockID", proposal.Header.ID()).Return(proposal.Payload, nil) // unset chain and height to make sure they are correctly reconstructed - hotstuffProposal := model.ProposalFromFlow(proposal.Header) + hotstuffProposal := model.SignedProposalFromFlow(proposal.Header) s.voteAggregator.On("AddBlock", hotstuffProposal) s.hotstuff.On("SubmitProposal", hotstuffProposal) expectedBroadcastMsg := messages.NewClusterBlockProposal(&proposal) diff --git a/engine/common/follower/compliance_core.go b/engine/common/follower/compliance_core.go index fa297b13902..2a65a2f3df2 100644 --- a/engine/common/follower/compliance_core.go +++ b/engine/common/follower/compliance_core.go @@ -111,7 +111,7 @@ func (c *ComplianceCore) OnBlockRange(originID flow.Identifier, batch []*flow.Bl firstBlock := batch[0].Header lastBlock := batch[len(batch)-1].Header - hotstuffProposal := model.ProposalFromFlow(lastBlock) + hotstuffProposal := model.SignedProposalFromFlow(lastBlock) log := c.log.With(). Hex("origin_id", originID[:]). Str("chain_id", lastBlock.ChainID.String()). diff --git a/engine/common/follower/compliance_core_test.go b/engine/common/follower/compliance_core_test.go index 522fc26160e..8930d2e19ce 100644 --- a/engine/common/follower/compliance_core_test.go +++ b/engine/common/follower/compliance_core_test.go @@ -97,7 +97,7 @@ func (s *CoreSuite) TestProcessingSingleBlock() { block := unittest.BlockWithParentFixture(s.finalizedBlock) // incoming block has to be validated - s.validator.On("ValidateProposal", model.ProposalFromFlow(block.Header)).Return(nil).Once() + s.validator.On("ValidateProposal", model.SignedProposalFromFlow(block.Header)).Return(nil).Once() err := s.core.OnBlockRange(s.originID, []*flow.Block{block}) require.NoError(s.T(), err) @@ -114,7 +114,7 @@ func (s *CoreSuite) TestAddFinalizedBlock() { block.Header.View = s.finalizedBlock.View - 1 // block is below finalized view // incoming block has to be validated - s.validator.On("ValidateProposal", model.ProposalFromFlow(block.Header)).Return(nil).Once() + s.validator.On("ValidateProposal", model.SignedProposalFromFlow(block.Header)).Return(nil).Once() err := s.core.OnBlockRange(s.originID, []*flow.Block{&block}) require.NoError(s.T(), err) @@ -140,7 +140,7 @@ func (s *CoreSuite) TestProcessingRangeHappyPath() { wg.Done() }).Return().Once() } - s.validator.On("ValidateProposal", model.ProposalFromFlow(blocks[len(blocks)-1].Header)).Return(nil).Once() + s.validator.On("ValidateProposal", model.SignedProposalFromFlow(blocks[len(blocks)-1].Header)).Return(nil).Once() err := s.core.OnBlockRange(s.originID, blocks) require.NoError(s.T(), err) @@ -154,7 +154,7 @@ func (s *CoreSuite) TestProcessingNotOrderedBatch() { blocks := unittest.ChainFixtureFrom(10, s.finalizedBlock) blocks[2], blocks[3] = blocks[3], blocks[2] - s.validator.On("ValidateProposal", model.ProposalFromFlow(blocks[len(blocks)-1].Header)).Return(nil).Once() + s.validator.On("ValidateProposal", model.SignedProposalFromFlow(blocks[len(blocks)-1].Header)).Return(nil).Once() err := s.core.OnBlockRange(s.originID, blocks) require.ErrorIs(s.T(), err, cache.ErrDisconnectedBatch) @@ -164,7 +164,7 @@ func (s *CoreSuite) TestProcessingNotOrderedBatch() { func (s *CoreSuite) TestProcessingInvalidBlock() { blocks := unittest.ChainFixtureFrom(10, s.finalizedBlock) - invalidProposal := model.ProposalFromFlow(blocks[len(blocks)-1].Header) + invalidProposal := model.SignedProposalFromFlow(blocks[len(blocks)-1].Header) sentinelError := model.NewInvalidProposalErrorf(invalidProposal, "") s.validator.On("ValidateProposal", invalidProposal).Return(sentinelError).Once() s.followerConsumer.On("OnInvalidBlockDetected", flow.Slashable[model.InvalidProposalError]{ @@ -189,7 +189,7 @@ func (s *CoreSuite) TestProcessingBlocksAfterShutdown() { // to the protocol state blocks := unittest.ChainFixtureFrom(10, s.finalizedBlock) - s.validator.On("ValidateProposal", model.ProposalFromFlow(blocks[len(blocks)-1].Header)).Return(nil).Once() + s.validator.On("ValidateProposal", model.SignedProposalFromFlow(blocks[len(blocks)-1].Header)).Return(nil).Once() err := s.core.OnBlockRange(s.originID, blocks) require.NoError(s.T(), err) diff --git a/engine/consensus/compliance/core.go b/engine/consensus/compliance/core.go index 8ed733c8fe9..5ab6d26c269 100644 --- a/engine/consensus/compliance/core.go +++ b/engine/consensus/compliance/core.go @@ -271,7 +271,7 @@ func (c *Core) processBlockAndDescendants(proposal flow.Slashable[*flow.Block]) }) // notify VoteAggregator about the invalid block - err = c.voteAggregator.InvalidBlock(model.ProposalFromFlow(header)) + err = c.voteAggregator.InvalidBlock(model.SignedProposalFromFlow(header)) if err != nil { if mempool.IsBelowPrunedThresholdError(err) { log.Warn().Msg("received invalid block, but is below pruned threshold") @@ -327,7 +327,7 @@ func (c *Core) processBlockProposal(proposal *flow.Block) error { ) defer span.End() - hotstuffProposal := model.ProposalFromFlow(header) + hotstuffProposal := model.SignedProposalFromFlow(header) err := c.validator.ValidateProposal(hotstuffProposal) if err != nil { if model.IsInvalidProposalError(err) { diff --git a/engine/consensus/compliance/core_test.go b/engine/consensus/compliance/core_test.go index a85ffb15764..307dd5ad678 100644 --- a/engine/consensus/compliance/core_test.go +++ b/engine/consensus/compliance/core_test.go @@ -285,7 +285,7 @@ func (cs *CoreSuite) TestOnBlockProposalValidParent() { // store the data for retrieval cs.headerDB[block.Header.ParentID] = cs.head - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) cs.validator.On("ValidateProposal", hotstuffProposal).Return(nil) cs.voteAggregator.On("AddBlock", hotstuffProposal).Once() cs.hotstuff.On("SubmitProposal", hotstuffProposal) @@ -314,7 +314,7 @@ func (cs *CoreSuite) TestOnBlockProposalValidAncestor() { cs.headerDB[parent.ID()] = parent.Header cs.headerDB[ancestor.ID()] = ancestor.Header - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) cs.validator.On("ValidateProposal", hotstuffProposal).Return(nil) cs.voteAggregator.On("AddBlock", hotstuffProposal).Once() cs.hotstuff.On("SubmitProposal", hotstuffProposal) @@ -363,7 +363,7 @@ func (cs *CoreSuite) TestOnBlockProposal_FailsHotStuffValidation() { parent := unittest.BlockWithParentFixture(ancestor.Header) block := unittest.BlockWithParentFixture(parent.Header) proposal := unittest.ProposalFromBlock(block) - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) // store the data for retrieval cs.headerDB[parent.ID()] = parent.Header @@ -445,7 +445,7 @@ func (cs *CoreSuite) TestOnBlockProposal_FailsProtocolStateValidation() { parent := unittest.BlockWithParentFixture(ancestor.Header) block := unittest.BlockWithParentFixture(parent.Header) proposal := unittest.ProposalFromBlock(block) - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) // store the data for retrieval cs.headerDB[parent.ID()] = parent.Header @@ -551,7 +551,7 @@ func (cs *CoreSuite) TestProcessBlockAndDescendants() { cs.childrenDB[parentID] = append(cs.childrenDB[parentID], pending3) for _, block := range []*flow.Block{parent, block1, block2, block3} { - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) cs.validator.On("ValidateProposal", hotstuffProposal).Return(nil) cs.voteAggregator.On("AddBlock", hotstuffProposal).Once() cs.hotstuff.On("SubmitProposal", hotstuffProposal).Once() @@ -601,7 +601,7 @@ func (cs *CoreSuite) TestProposalBufferingOrder() { require.NoError(cs.T(), err, "proposal buffering should pass") // make sure no block is forwarded to hotstuff - cs.hotstuff.AssertNotCalled(cs.T(), "SubmitProposal", model.ProposalFromFlow(&proposal.Block.Header)) + cs.hotstuff.AssertNotCalled(cs.T(), "SubmitProposal", model.SignedProposalFromFlow(&proposal.Block.Header)) } // check that we submit each proposal in a valid order @@ -626,7 +626,7 @@ func (cs *CoreSuite) TestProposalBufferingOrder() { } // mark the proposal as processed delete(unprocessed, header.BlockID) - cs.headerDB[header.BlockID] = model.ProposalToFlow(proposal) + cs.headerDB[header.BlockID] = model.SignedProposalToFlow(proposal) calls++ }, ) diff --git a/engine/consensus/compliance/engine_test.go b/engine/consensus/compliance/engine_test.go index a82ccc558c7..e5afb20ad23 100644 --- a/engine/consensus/compliance/engine_test.go +++ b/engine/consensus/compliance/engine_test.go @@ -70,7 +70,7 @@ func (cs *EngineSuite) TestSubmittingMultipleEntries() { for i := 0; i < blockCount; i++ { block := unittest.BlockWithParentFixture(cs.head) proposal := messages.NewBlockProposal(block) - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) cs.hotstuff.On("SubmitProposal", hotstuffProposal).Return().Once() cs.voteAggregator.On("AddBlock", hotstuffProposal).Once() cs.validator.On("ValidateProposal", hotstuffProposal).Return(nil).Once() @@ -88,7 +88,7 @@ func (cs *EngineSuite) TestSubmittingMultipleEntries() { block := unittest.BlockWithParentFixture(cs.head) proposal := unittest.ProposalFromBlock(block) - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) cs.hotstuff.On("SubmitProposal", hotstuffProposal).Return().Once() cs.voteAggregator.On("AddBlock", hotstuffProposal).Once() cs.validator.On("ValidateProposal", hotstuffProposal).Return(nil).Once() diff --git a/engine/consensus/message_hub/message_hub.go b/engine/consensus/message_hub/message_hub.go index 07fe8c3a387..38cc34a95ca 100644 --- a/engine/consensus/message_hub/message_hub.go +++ b/engine/consensus/message_hub/message_hub.go @@ -436,7 +436,7 @@ func (h *MessageHub) OnOwnProposal(proposal *flow.Header, targetPublicationTime return } - hotstuffProposal := model.ProposalFromFlow(proposal) + hotstuffProposal := model.SignedProposalFromFlow(proposal) // notify vote aggregator that new block proposal is available, in case we are next leader h.voteAggregator.AddBlock(hotstuffProposal) // non-blocking diff --git a/engine/consensus/message_hub/message_hub_test.go b/engine/consensus/message_hub/message_hub_test.go index 68bd1adc59a..e5cd47ca1c1 100644 --- a/engine/consensus/message_hub/message_hub_test.go +++ b/engine/consensus/message_hub/message_hub_test.go @@ -250,7 +250,7 @@ func (s *MessageHubSuite) TestOnOwnProposal() { expectedBroadcastMsg := messages.NewBlockProposal(block) submitted := make(chan struct{}) // closed when proposal is submitted to hotstuff - hotstuffProposal := model.ProposalFromFlow(block.Header) + hotstuffProposal := model.SignedProposalFromFlow(block.Header) s.voteAggregator.On("AddBlock", hotstuffProposal).Once() s.hotstuff.On("SubmitProposal", hotstuffProposal). Run(func(args mock.Arguments) { close(submitted) }). @@ -315,7 +315,7 @@ func (s *MessageHubSuite) TestProcessMultipleMessagesHappyPath() { s.payloads.On("ByBlockID", proposal.Header.ID()).Return(proposal.Payload, nil) // unset chain and height to make sure they are correctly reconstructed - hotstuffProposal := model.ProposalFromFlow(proposal.Header) + hotstuffProposal := model.SignedProposalFromFlow(proposal.Header) s.voteAggregator.On("AddBlock", hotstuffProposal).Once() s.hotstuff.On("SubmitProposal", hotstuffProposal) expectedBroadcastMsg := messages.NewBlockProposal(&proposal) From 349c3508329c343b154771cacd3bf108a7c215a0 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 2 Oct 2024 14:43:20 +0300 Subject: [PATCH 04/13] Fixed consensus tests --- .../blockproducer/safety_rules_wrapper.go | 2 +- .../eventhandler/event_handler_test.go | 15 ++---- .../hotstuff/eventloop/event_loop_test.go | 4 +- .../hotstuff/forks/block_builder_test.go | 9 ++-- consensus/hotstuff/helper/block.go | 34 ++++++++---- consensus/hotstuff/model/proposal.go | 9 ++++ .../hotstuff/safetyrules/safety_rules_test.go | 50 ++++++++--------- .../timeout_processor_test.go | 3 +- .../hotstuff/validator/validator_test.go | 42 +++++++-------- .../verification/combined_signer_v2_test.go | 11 ++-- .../verification/combined_signer_v3_test.go | 16 +++--- .../verification/staking_signer_test.go | 53 +------------------ .../voteaggregator/vote_aggregator_test.go | 4 +- .../combined_vote_processor_v2_test.go | 6 +-- .../combined_vote_processor_v3_test.go | 8 +-- .../hotstuff/votecollector/factory_test.go | 8 +-- .../staking_vote_processor_test.go | 4 +- .../votecollector/statemachine_test.go | 34 +++++++----- consensus/hotstuff/votecollector/testutil.go | 2 +- 19 files changed, 143 insertions(+), 171 deletions(-) diff --git a/consensus/hotstuff/blockproducer/safety_rules_wrapper.go b/consensus/hotstuff/blockproducer/safety_rules_wrapper.go index c004e27a33f..6d960eb836f 100644 --- a/consensus/hotstuff/blockproducer/safety_rules_wrapper.go +++ b/consensus/hotstuff/blockproducer/safety_rules_wrapper.go @@ -67,7 +67,7 @@ func (w *safetyRulesConcurrencyWrapper) Sign(unsignedHeader *flow.Header) error } // signer is now in state 1, and this thread is the only one every going to execute the following logic // signature for own block is structurally a vote - vote, err := w.safetyRules.SignOwnProposal(model.SignedProposalFromFlow(unsignedHeader)) + vote, err := w.safetyRules.SignOwnProposal(model.ProposalFromFlow(unsignedHeader)) if err != nil { return fmt.Errorf("could not sign block proposal: %w", err) } diff --git a/consensus/hotstuff/eventhandler/event_handler_test.go b/consensus/hotstuff/eventhandler/event_handler_test.go index 77080ff7583..72cdb5ae1b6 100644 --- a/consensus/hotstuff/eventhandler/event_handler_test.go +++ b/consensus/hotstuff/eventhandler/event_handler_test.go @@ -228,14 +228,12 @@ type BlockProducer struct { } func (b *BlockProducer) MakeBlockProposal(view uint64, qc *flow.QuorumCertificate, lastViewTC *flow.TimeoutCertificate) (*flow.Header, error) { - return model.SignedProposalToFlow(&model.SignedProposal{ - Block: helper.MakeBlock( + return model.SignedProposalToFlow(helper.MakeSignedProposal(helper.WithProposal( + helper.MakeProposal(helper.WithBlock(helper.MakeBlock( helper.WithBlockView(view), helper.WithBlockQC(qc), - helper.WithBlockProposer(b.proposerID), - ), - LastViewTC: lastViewTC, - }), nil + helper.WithBlockProposer(b.proposerID))), + helper.WithLastViewTC(lastViewTC))))), nil } func TestEventHandler(t *testing.T) { @@ -1035,8 +1033,5 @@ func createVote(block *model.Block) *model.Vote { func createProposal(view uint64, qcview uint64) *model.SignedProposal { block := createBlockWithQC(view, qcview) - return &model.SignedProposal{ - Block: block, - SigData: nil, - } + return helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal(helper.WithBlock(block)))) } diff --git a/consensus/hotstuff/eventloop/event_loop_test.go b/consensus/hotstuff/eventloop/event_loop_test.go index 6cd262b8c63..2c40cecf5c8 100644 --- a/consensus/hotstuff/eventloop/event_loop_test.go +++ b/consensus/hotstuff/eventloop/event_loop_test.go @@ -75,7 +75,7 @@ func (s *EventLoopTestSuite) TestReadyDone() { // Test_SubmitQC tests that submitted proposal is eventually sent to event handler for processing func (s *EventLoopTestSuite) Test_SubmitProposal() { - proposal := helper.MakeProposal() + proposal := helper.MakeSignedProposal() processed := atomic.NewBool(false) s.eh.On("OnReceiveProposal", proposal).Run(func(args mock.Arguments) { processed.Store(true) @@ -229,7 +229,7 @@ func TestEventLoop_Timeout(t *testing.T) { go func() { defer wg.Done() for !processed.Load() { - eventLoop.SubmitProposal(helper.MakeProposal()) + eventLoop.SubmitProposal(helper.MakeSignedProposal()) } }() diff --git a/consensus/hotstuff/forks/block_builder_test.go b/consensus/hotstuff/forks/block_builder_test.go index 2c50808601b..62b4bf8bce5 100644 --- a/consensus/hotstuff/forks/block_builder_test.go +++ b/consensus/hotstuff/forks/block_builder_test.go @@ -77,8 +77,8 @@ func (bb *BlockBuilder) AddVersioned(qcView uint64, blockView uint64, qcVersion // Proposals returns a list of all proposals added to the BlockBuilder. // Returns an error if the blocks do not form a connected tree rooted at genesis. -func (bb *BlockBuilder) Proposals() ([]*model.SignedProposal, error) { - blocks := make([]*model.SignedProposal, 0, len(bb.blockViews)) +func (bb *BlockBuilder) Proposals() ([]*model.Proposal, error) { + blocks := make([]*model.Proposal, 0, len(bb.blockViews)) genesisBlock := makeGenesis() genesisBV := &BlockView{ @@ -99,14 +99,13 @@ func (bb *BlockBuilder) Proposals() ([]*model.SignedProposal, error) { if qc.View+1 != bv.View { lastViewTC = helper.MakeTC(helper.WithTCView(bv.View - 1)) } - proposal := &model.SignedProposal{ + proposal := &model.Proposal{ Block: &model.Block{ View: bv.View, QC: qc, PayloadHash: payloadHash, }, LastViewTC: lastViewTC, - SigData: nil, } proposal.Block.BlockID = makeBlockID(proposal.Block) @@ -177,7 +176,7 @@ func makeGenesis() *model.CertifiedBlock { } // toBlocks converts the given proposals to slice of blocks -func toBlocks(proposals []*model.SignedProposal) []*model.Block { +func toBlocks(proposals []*model.Proposal) []*model.Block { blocks := make([]*model.Block, 0, len(proposals)) for _, b := range proposals { blocks = append(blocks, b.Block) diff --git a/consensus/hotstuff/helper/block.go b/consensus/hotstuff/helper/block.go index 398ef2d7941..0c55147dcbd 100644 --- a/consensus/hotstuff/helper/block.go +++ b/consensus/hotstuff/helper/block.go @@ -56,13 +56,10 @@ func WithBlockQC(qc *flow.QuorumCertificate) func(*model.Block) { } } -func MakeProposal(options ...func(*model.SignedProposal)) *model.SignedProposal { +func MakeSignedProposal(options ...func(*model.SignedProposal)) *model.SignedProposal { proposal := &model.SignedProposal{ - Proposal: model.Proposal{ - Block: MakeBlock(), - LastViewTC: nil, - }, - SigData: unittest.SignatureFixture(), + Proposal: *MakeProposal(), + SigData: unittest.SignatureFixture(), } for _, option := range options { option(proposal) @@ -70,8 +67,25 @@ func MakeProposal(options ...func(*model.SignedProposal)) *model.SignedProposal return proposal } -func WithBlock(block *model.Block) func(*model.SignedProposal) { - return func(proposal *model.SignedProposal) { +func MakeProposal(options ...func(*model.Proposal)) *model.Proposal { + proposal := &model.Proposal{ + Block: MakeBlock(), + LastViewTC: nil, + } + for _, option := range options { + option(proposal) + } + return proposal +} + +func WithProposal(proposal *model.Proposal) func(*model.SignedProposal) { + return func(signedProposal *model.SignedProposal) { + signedProposal.Proposal = *proposal + } +} + +func WithBlock(block *model.Block) func(*model.Proposal) { + return func(proposal *model.Proposal) { proposal.Block = block } } @@ -82,8 +96,8 @@ func WithSigData(sigData []byte) func(*model.SignedProposal) { } } -func WithLastViewTC(lastViewTC *flow.TimeoutCertificate) func(*model.SignedProposal) { - return func(proposal *model.SignedProposal) { +func WithLastViewTC(lastViewTC *flow.TimeoutCertificate) func(*model.Proposal) { + return func(proposal *model.Proposal) { proposal.LastViewTC = lastViewTC } } diff --git a/consensus/hotstuff/model/proposal.go b/consensus/hotstuff/model/proposal.go index e4b68508756..50676ac7d5c 100644 --- a/consensus/hotstuff/model/proposal.go +++ b/consensus/hotstuff/model/proposal.go @@ -51,6 +51,15 @@ func SignedProposalFromFlow(header *flow.Header) *SignedProposal { return &proposal } +// ProposalFromFlow turns an unsigned flow header into a unsigned hotstuff block type. +func ProposalFromFlow(header *flow.Header) *Proposal { + proposal := Proposal{ + Block: BlockFromFlow(header), + LastViewTC: header.LastViewTC, + } + return &proposal +} + // SignedProposalToFlow turns a block proposal into a flow header. func SignedProposalToFlow(proposal *SignedProposal) *flow.Header { diff --git a/consensus/hotstuff/safetyrules/safety_rules_test.go b/consensus/hotstuff/safetyrules/safety_rules_test.go index c12b7c839a6..a9ef6738966 100644 --- a/consensus/hotstuff/safetyrules/safety_rules_test.go +++ b/consensus/hotstuff/safetyrules/safety_rules_test.go @@ -50,13 +50,13 @@ func (s *SafetyRulesTestSuite) SetupTest() { // bootstrap at random bootstrapBlock s.bootstrapBlock = helper.MakeBlock(helper.WithBlockView(100)) - s.proposal = helper.MakeProposal( + s.proposal = helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock( helper.MakeBlock( helper.WithParentBlock(s.bootstrapBlock), helper.WithBlockView(s.bootstrapBlock.View+1), helper.WithBlockProposer(s.proposerIdentity.NodeID)), - )) + )))) s.committee.On("Self").Return(s.ourIdentity.NodeID).Maybe() s.committee.On("LeaderForView", mock.Anything).Return(s.proposerIdentity.NodeID, nil).Maybe() @@ -104,13 +104,13 @@ func (s *SafetyRulesTestSuite) TestProduceVote_ShouldVote() { helper.WithTCNewestQC(s.proposal.Block.QC)) // voting on proposal where last view ended with TC - proposalWithTC := helper.MakeProposal( + proposalWithTC := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock( helper.MakeBlock( helper.WithParentBlock(s.bootstrapBlock), helper.WithBlockView(s.proposal.Block.View+2), helper.WithBlockProposer(s.proposerIdentity.NodeID))), - helper.WithLastViewTC(lastViewTC)) + helper.WithLastViewTC(lastViewTC)))) expectedSafetyData = &hotstuff.SafetyData{ LockedOneChainView: s.proposal.Block.QC.View, @@ -139,13 +139,13 @@ func (s *SafetyRulesTestSuite) TestProduceVote_IncludedQCHigherThanTCsQC() { helper.WithTCNewestQC(s.proposal.Block.QC)) // voting on proposal where last view ended with TC - proposalWithTC := helper.MakeProposal( + proposalWithTC := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock( helper.MakeBlock( helper.WithParentBlock(s.proposal.Block), helper.WithBlockView(s.proposal.Block.View+2), helper.WithBlockProposer(s.proposerIdentity.NodeID))), - helper.WithLastViewTC(lastViewTC)) + helper.WithLastViewTC(lastViewTC)))) expectedSafetyData := &hotstuff.SafetyData{ LockedOneChainView: proposalWithTC.Block.QC.View, @@ -210,13 +210,13 @@ func (s *SafetyRulesTestSuite) TestProduceVote_InvalidCurrentView() { }) s.Run("view-not-monotonously-increasing", func() { // create block with view < HighestAcknowledgedView - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock( helper.MakeBlock( func(block *model.Block) { block.QC = helper.MakeQC(helper.WithQCView(s.safetyData.HighestAcknowledgedView - 2)) }, - helper.WithBlockView(s.safetyData.HighestAcknowledgedView-1)))) + helper.WithBlockView(s.safetyData.HighestAcknowledgedView-1)))))) vote, err := s.safety.ProduceVote(proposal, proposal.Block.View) require.Nil(s.T(), vote) require.Error(s.T(), err) @@ -345,12 +345,12 @@ func (s *SafetyRulesTestSuite) TestProduceVote_VotingOnInvalidProposals() { // a proposal which includes a QC for the previous round should not contain a TC s.Run("proposal-includes-last-view-qc-and-tc", func() { - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock( helper.MakeBlock( helper.WithParentBlock(s.bootstrapBlock), helper.WithBlockView(s.bootstrapBlock.View+1))), - helper.WithLastViewTC(helper.MakeTC())) + helper.WithLastViewTC(helper.MakeTC())))) s.committee.On("IdentityByBlock", proposal.Block.BlockID, proposal.Block.ProposerID).Return(s.proposerIdentity, nil).Maybe() vote, err := s.safety.ProduceVote(proposal, proposal.Block.View) require.Error(s.T(), err) @@ -359,11 +359,11 @@ func (s *SafetyRulesTestSuite) TestProduceVote_VotingOnInvalidProposals() { }) s.Run("no-last-view-tc", func() { // create block where Block.View != Block.QC.View+1 and LastViewTC = nil - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock( helper.MakeBlock( helper.WithParentBlock(s.bootstrapBlock), - helper.WithBlockView(s.bootstrapBlock.View+2)))) + helper.WithBlockView(s.bootstrapBlock.View+2)))))) vote, err := s.safety.ProduceVote(proposal, proposal.Block.View) require.Error(s.T(), err) require.False(s.T(), model.IsNoVoteError(err)) @@ -372,14 +372,14 @@ func (s *SafetyRulesTestSuite) TestProduceVote_VotingOnInvalidProposals() { s.Run("last-view-tc-invalid-view", func() { // create block where Block.View != Block.QC.View+1 and // Block.View != LastViewTC.View+1 - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock( helper.MakeBlock( helper.WithParentBlock(s.bootstrapBlock), helper.WithBlockView(s.bootstrapBlock.View+2))), helper.WithLastViewTC( helper.MakeTC( - helper.WithTCView(s.bootstrapBlock.View)))) + helper.WithTCView(s.bootstrapBlock.View)))))) vote, err := s.safety.ProduceVote(proposal, proposal.Block.View) require.Error(s.T(), err) require.False(s.T(), model.IsNoVoteError(err)) @@ -389,7 +389,7 @@ func (s *SafetyRulesTestSuite) TestProduceVote_VotingOnInvalidProposals() { // create block where Block.View != Block.QC.View+1 and // Block.View == LastViewTC.View+1 and Block.QC.View >= Block.View // in this case block is not safe to extend since proposal includes QC which is newer than the proposal itself. - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock( helper.MakeBlock( helper.WithParentBlock(s.bootstrapBlock), @@ -399,7 +399,7 @@ func (s *SafetyRulesTestSuite) TestProduceVote_VotingOnInvalidProposals() { })), helper.WithLastViewTC( helper.MakeTC( - helper.WithTCView(s.bootstrapBlock.View+1)))) + helper.WithTCView(s.bootstrapBlock.View+1)))))) vote, err := s.safety.ProduceVote(proposal, proposal.Block.View) require.Error(s.T(), err) require.False(s.T(), model.IsNoVoteError(err)) @@ -411,7 +411,7 @@ func (s *SafetyRulesTestSuite) TestProduceVote_VotingOnInvalidProposals() { // in this case block is not safe to extend since proposal is built on top of QC, which is lower // than QC presented in LastViewTC. TONewestQC := helper.MakeQC(helper.WithQCView(s.bootstrapBlock.View + 1)) - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock( helper.MakeBlock( helper.WithParentBlock(s.bootstrapBlock), @@ -419,7 +419,7 @@ func (s *SafetyRulesTestSuite) TestProduceVote_VotingOnInvalidProposals() { helper.WithLastViewTC( helper.MakeTC( helper.WithTCView(s.bootstrapBlock.View+1), - helper.WithTCNewestQC(TONewestQC)))) + helper.WithTCNewestQC(TONewestQC)))))) vote, err := s.safety.ProduceVote(proposal, proposal.Block.View) require.Error(s.T(), err) require.False(s.T(), model.IsNoVoteError(err)) @@ -447,13 +447,13 @@ func (s *SafetyRulesTestSuite) TestProduceVote_VoteEquivocation() { require.NotNil(s.T(), vote) require.Equal(s.T(), expectedVote, vote) - equivocatingProposal := helper.MakeProposal( + equivocatingProposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock( helper.MakeBlock( helper.WithParentBlock(s.bootstrapBlock), helper.WithBlockView(s.bootstrapBlock.View+1), helper.WithBlockProposer(s.proposerIdentity.NodeID)), - )) + )))) // voting at same view(event different proposal) should result in NoVoteError vote, err = s.safety.ProduceVote(equivocatingProposal, s.proposal.Block.View) @@ -739,7 +739,7 @@ func (s *SafetyRulesTestSuite) TestSignOwnProposal() { s.committee.On("LeaderForView", s.proposal.Block.View).Return(s.ourIdentity.NodeID, nil).Once() s.signer.On("CreateVote", s.proposal.Block).Return(expectedVote, nil).Once() s.persister.On("PutSafetyData", expectedSafetyData).Return(nil).Once() - vote, err := s.safety.SignOwnProposal(s.proposal) + vote, err := s.safety.SignOwnProposal(&s.proposal.Proposal) require.NoError(s.T(), err) require.Equal(s.T(), vote, expectedVote) } @@ -747,7 +747,7 @@ func (s *SafetyRulesTestSuite) TestSignOwnProposal() { // TestSignOwnProposal_ProposalNotSelf tests that we cannot sign a proposal that is not ours. We // verify that SafetyRules returns and exception and does not the benign sentinel error NoVoteError. func (s *SafetyRulesTestSuite) TestSignOwnProposal_ProposalNotSelf() { - vote, err := s.safety.SignOwnProposal(s.proposal) + vote, err := s.safety.SignOwnProposal(&s.proposal.Proposal) require.Error(s.T(), err) require.False(s.T(), model.IsNoVoteError(err)) require.Nil(s.T(), vote) @@ -760,7 +760,7 @@ func (s *SafetyRulesTestSuite) TestSignOwnProposal_SelfInvalidLeader() { exception := errors.New("invalid-signer-identity") s.committee.On("LeaderForView").Unset() s.committee.On("LeaderForView", s.proposal.Block.View).Return(flow.Identifier{}, exception).Once() - vote, err := s.safety.SignOwnProposal(s.proposal) + vote, err := s.safety.SignOwnProposal(&s.proposal.Proposal) require.ErrorIs(s.T(), err, exception) require.False(s.T(), model.IsNoVoteError(err)) require.Nil(s.T(), vote) @@ -785,12 +785,12 @@ func (s *SafetyRulesTestSuite) TestSignOwnProposal_ProposalEquivocation() { s.signer.On("CreateVote", s.proposal.Block).Return(expectedVote, nil).Once() s.persister.On("PutSafetyData", expectedSafetyData).Return(nil).Once() - vote, err := s.safety.SignOwnProposal(s.proposal) + vote, err := s.safety.SignOwnProposal(&s.proposal.Proposal) require.NoError(s.T(), err) require.Equal(s.T(), expectedVote, vote) // signing same proposal again should return an error since we have already created a proposal for this view - vote, err = s.safety.SignOwnProposal(s.proposal) + vote, err = s.safety.SignOwnProposal(&s.proposal.Proposal) require.Error(s.T(), err) require.True(s.T(), model.IsNoVoteError(err)) require.Nil(s.T(), vote) diff --git a/consensus/hotstuff/timeoutcollector/timeout_processor_test.go b/consensus/hotstuff/timeoutcollector/timeout_processor_test.go index e5f443d6898..309d1351b98 100644 --- a/consensus/hotstuff/timeoutcollector/timeout_processor_test.go +++ b/consensus/hotstuff/timeoutcollector/timeout_processor_test.go @@ -575,8 +575,9 @@ func createRealQC( block *model.Block, ) *flow.QuorumCertificate { leader := signers[0] - proposal, err := signerObjects[leader.NodeID].CreateProposal(block) + leaderVote, err := signerObjects[leader.NodeID].CreateVote(block) require.NoError(t, err) + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal(helper.WithBlock(block))), helper.WithSigData(leaderVote.SigData)) var createdQC *flow.QuorumCertificate onQCCreated := func(qc *flow.QuorumCertificate) { diff --git a/consensus/hotstuff/validator/validator_test.go b/consensus/hotstuff/validator/validator_test.go index a5a47cb1af2..6c7e91ad0fa 100644 --- a/consensus/hotstuff/validator/validator_test.go +++ b/consensus/hotstuff/validator/validator_test.go @@ -70,7 +70,7 @@ func (ps *ProposalSuite) SetupTest() { require.NoError(ps.T(), err) ps.voters = ps.participants.Filter(filter.HasNodeID[flow.Identity](voterIDs...)).ToSkeleton() - ps.proposal = &model.SignedProposal{Block: ps.block} + ps.proposal = helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal(helper.WithBlock(ps.block)))) ps.vote = ps.proposal.ProposerVote() ps.voter = ps.leader @@ -256,7 +256,7 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { ps.committee.On("LeaderForView", mock.Anything).Return(ps.leader.NodeID, nil) ps.Run("happy-path", func() { - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock(helper.MakeBlock( helper.WithBlockView(ps.block.View+2), helper.WithBlockProposer(ps.leader.NodeID), @@ -267,14 +267,14 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { helper.WithTCSigners(ps.indices), helper.WithTCView(ps.block.View+1), helper.WithTCNewestQC(ps.block.QC))), - ) + ))) ps.verifier.On("VerifyTC", ps.voters, []byte(proposal.LastViewTC.SigData), proposal.LastViewTC.View, proposal.LastViewTC.NewestQCViews).Return(nil).Once() err := ps.validator.ValidateProposal(proposal) require.NoError(ps.T(), err) }) ps.Run("no-tc", func() { - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock(helper.MakeBlock( helper.WithBlockView(ps.block.View+2), helper.WithBlockProposer(ps.leader.NodeID), @@ -282,14 +282,14 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { helper.WithBlockQC(ps.block.QC)), ), // in this case proposal without LastViewTC is considered invalid - ) + ))) err := ps.validator.ValidateProposal(proposal) require.True(ps.T(), model.IsInvalidProposalError(err)) ps.verifier.AssertNotCalled(ps.T(), "VerifyQC") ps.verifier.AssertNotCalled(ps.T(), "VerifyTC") }) ps.Run("tc-for-wrong-view", func() { - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock(helper.MakeBlock( helper.WithBlockView(ps.block.View+2), helper.WithBlockProposer(ps.leader.NodeID), @@ -300,14 +300,14 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { helper.WithTCSigners(ps.indices), helper.WithTCView(ps.block.View+10), // LastViewTC.View must be equal to Block.View-1 helper.WithTCNewestQC(ps.block.QC))), - ) + ))) err := ps.validator.ValidateProposal(proposal) require.True(ps.T(), model.IsInvalidProposalError(err)) ps.verifier.AssertNotCalled(ps.T(), "VerifyQC") ps.verifier.AssertNotCalled(ps.T(), "VerifyTC") }) ps.Run("proposal-not-safe-to-extend", func() { - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock(helper.MakeBlock( helper.WithBlockView(ps.block.View+2), helper.WithBlockProposer(ps.leader.NodeID), @@ -319,14 +319,14 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { helper.WithTCView(ps.block.View+1), // proposal is not safe to extend because included QC.View is higher that Block.QC.View helper.WithTCNewestQC(helper.MakeQC(helper.WithQCView(ps.block.View+1))))), - ) + ))) err := ps.validator.ValidateProposal(proposal) require.True(ps.T(), model.IsInvalidProposalError(err)) ps.verifier.AssertNotCalled(ps.T(), "VerifyQC") ps.verifier.AssertNotCalled(ps.T(), "VerifyTC") }) ps.Run("included-tc-highest-qc-not-highest", func() { - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock(helper.MakeBlock( helper.WithBlockView(ps.block.View+2), helper.WithBlockProposer(ps.leader.NodeID), @@ -338,7 +338,7 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { helper.WithTCView(ps.block.View+1), helper.WithTCNewestQC(ps.block.QC), )), - ) + ))) ps.verifier.On("VerifyTC", ps.voters, []byte(proposal.LastViewTC.SigData), proposal.LastViewTC.View, mock.Anything).Return(nil).Once() @@ -352,7 +352,7 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { // TC is signed by only one signer - insufficient to reach weight threshold insufficientSignerIndices, err := signature.EncodeSignersToIndices(ps.participants.NodeIDs(), ps.participants.NodeIDs()[:1]) require.NoError(ps.T(), err) - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock(helper.MakeBlock( helper.WithBlockView(ps.block.View+2), helper.WithBlockProposer(ps.leader.NodeID), @@ -364,7 +364,7 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { helper.WithTCView(ps.block.View+1), helper.WithTCNewestQC(ps.block.QC), )), - ) + ))) err = ps.validator.ValidateProposal(proposal) require.True(ps.T(), model.IsInvalidProposalError(err) && model.IsInvalidTCError(err)) ps.verifier.AssertNotCalled(ps.T(), "VerifyTC") @@ -375,7 +375,7 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { helper.WithQCView(ps.block.QC.View-1), helper.WithQCSigners(ps.indices)) - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock(helper.MakeBlock( helper.WithBlockView(ps.block.View+2), helper.WithBlockProposer(ps.leader.NodeID), @@ -386,7 +386,7 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { helper.WithTCSigners(ps.indices), helper.WithTCView(ps.block.View+1), helper.WithTCNewestQC(qc))), - ) + ))) ps.verifier.On("VerifyTC", ps.voters, []byte(proposal.LastViewTC.SigData), proposal.LastViewTC.View, proposal.LastViewTC.NewestQCViews).Return(nil).Once() ps.verifier.On("VerifyQC", ps.voters, qc.SigData, @@ -399,7 +399,7 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { helper.WithQCView(ps.block.QC.View-2), helper.WithQCSigners(ps.indices)) - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock(helper.MakeBlock( helper.WithBlockView(ps.block.View+2), helper.WithBlockProposer(ps.leader.NodeID), @@ -410,7 +410,7 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { helper.WithTCSigners(ps.indices), helper.WithTCView(ps.block.View+1), helper.WithTCNewestQC(newestQC))), - ) + ))) ps.verifier.On("VerifyTC", ps.voters, []byte(proposal.LastViewTC.SigData), proposal.LastViewTC.View, proposal.LastViewTC.NewestQCViews).Return(nil).Once() // Validating QC included in TC returns ErrViewForUnknownEpoch @@ -423,7 +423,7 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { require.NotErrorIs(ps.T(), err, model.ErrViewForUnknownEpoch) }) ps.Run("included-tc-invalid-sig", func() { - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock(helper.MakeBlock( helper.WithBlockView(ps.block.View+2), helper.WithBlockProposer(ps.leader.NodeID), @@ -434,7 +434,7 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { helper.WithTCSigners(ps.indices), helper.WithTCView(ps.block.View+1), helper.WithTCNewestQC(ps.block.QC))), - ) + ))) ps.verifier.On("VerifyTC", ps.voters, []byte(proposal.LastViewTC.SigData), proposal.LastViewTC.View, proposal.LastViewTC.NewestQCViews).Return(model.ErrInvalidSignature).Once() err := ps.validator.ValidateProposal(proposal) @@ -443,7 +443,7 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { proposal.LastViewTC.View, proposal.LastViewTC.NewestQCViews) }) ps.Run("last-view-successful-but-includes-tc", func() { - proposal := helper.MakeProposal( + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock(helper.MakeBlock( helper.WithBlockView(ps.finalized+1), helper.WithBlockProposer(ps.leader.NodeID), @@ -451,7 +451,7 @@ func (ps *ProposalSuite) TestProposalWithLastViewTC() { helper.WithParentBlock(ps.parent)), ), helper.WithLastViewTC(helper.MakeTC()), - ) + ))) err := ps.validator.ValidateProposal(proposal) require.True(ps.T(), model.IsInvalidProposalError(err)) ps.verifier.AssertNotCalled(ps.T(), "VerifyTC") diff --git a/consensus/hotstuff/verification/combined_signer_v2_test.go b/consensus/hotstuff/verification/combined_signer_v2_test.go index d20cdffeca3..5e2149456a3 100644 --- a/consensus/hotstuff/verification/combined_signer_v2_test.go +++ b/consensus/hotstuff/verification/combined_signer_v2_test.go @@ -38,7 +38,7 @@ func TestCombinedSignWithBeaconKey(t *testing.T) { fblock.Header.View = proposerView fblock.Header.ParentView = proposerView - 1 fblock.Header.LastViewTC = nil - proposal := model.SignedProposalFromFlow(fblock.Header) + proposal := model.ProposalFromFlow(fblock.Header) signerID := fblock.Header.ProposerID beaconKeyStore := modulemock.NewRandomBeaconKeyStore(t) @@ -78,7 +78,7 @@ func TestCombinedSignWithBeaconKey(t *testing.T) { // check that a created proposal can be verified by a verifier vote, err := safetyRules.SignOwnProposal(proposal) require.NoError(t, err) - proposal.SigData = vote.SigData + err = verifier.VerifyVote(&ourIdentity.IdentitySkeleton, vote.SigData, proposal.Block.View, proposal.Block.BlockID) require.NoError(t, err) @@ -92,7 +92,7 @@ func TestCombinedSignWithBeaconKey(t *testing.T) { require.NoError(t, err) expectedSig := msig.EncodeDoubleSig(stakingSig, beaconSig) - require.Equal(t, expectedSig, proposal.SigData) + require.Equal(t, expectedSig, vote.SigData) // vote should be valid vote, err = signer.CreateVote(block) @@ -146,7 +146,7 @@ func TestCombinedSignWithNoBeaconKey(t *testing.T) { fblock.Header.View = proposerView fblock.Header.ParentView = proposerView - 1 fblock.Header.LastViewTC = nil - proposal := model.SignedProposalFromFlow(fblock.Header) + proposal := model.ProposalFromFlow(fblock.Header) signerID := fblock.Header.ProposerID beaconKeyStore := modulemock.NewRandomBeaconKeyStore(t) @@ -190,7 +190,6 @@ func TestCombinedSignWithNoBeaconKey(t *testing.T) { // check that a created proposal can be verified by a verifier vote, err := safetyRules.SignOwnProposal(proposal) require.NoError(t, err) - proposal.SigData = vote.SigData err = verifier.VerifyVote(&ourIdentity.IdentitySkeleton, vote.SigData, proposal.Block.View, proposal.Block.BlockID) require.NoError(t, err) @@ -202,7 +201,7 @@ func TestCombinedSignWithNoBeaconKey(t *testing.T) { msig.NewBLSHasher(msig.ConsensusVoteTag), ) require.NoError(t, err) - require.Equal(t, expectedStakingSig, crypto.Signature(proposal.SigData)) + require.Equal(t, expectedStakingSig, crypto.Signature(vote.SigData)) } // Test_VerifyQC_EmptySigners checks that Verifier returns an `model.InsufficientSignaturesError` diff --git a/consensus/hotstuff/verification/combined_signer_v3_test.go b/consensus/hotstuff/verification/combined_signer_v3_test.go index e655612dcc2..b8abd8f821f 100644 --- a/consensus/hotstuff/verification/combined_signer_v3_test.go +++ b/consensus/hotstuff/verification/combined_signer_v3_test.go @@ -58,11 +58,10 @@ func TestCombinedSignWithBeaconKeyV3(t *testing.T) { verifier := NewCombinedVerifierV3(committee, packer) // check that a created proposal can be verified by a verifier - proposal, err := signer.CreateProposal(block) + vote, err := signer.CreateVote(block) require.NoError(t, err) - vote := proposal.ProposerVote() - err = verifier.VerifyVote(nodeID, vote.SigData, proposal.Block.View, proposal.Block.BlockID) + err = verifier.VerifyVote(nodeID, vote.SigData, block.View, block.BlockID) require.NoError(t, err) // check that a created proposal's signature is a combined staking sig and random beacon sig @@ -72,14 +71,14 @@ func TestCombinedSignWithBeaconKeyV3(t *testing.T) { require.NoError(t, err) expectedSig := msig.EncodeSingleSig(encoding.SigTypeRandomBeacon, beaconSig) - require.Equal(t, expectedSig, proposal.SigData) + require.Equal(t, expectedSig, vote.SigData) // Vote from a node that is _not_ part of the Random Beacon committee should be rejected. // Specifically, we expect that the verifier recognizes the `protocol.IdentityNotFoundError` // as a sign of an invalid vote and wraps it into a `model.InvalidSignerError`. *dkg = mocks.DKG{} // overwrite DKG mock with a new one dkg.On("KeyShare", signerID).Return(nil, protocol.IdentityNotFoundError{NodeID: signerID}) - err = verifier.VerifyVote(nodeID, vote.SigData, proposal.Block.View, proposal.Block.BlockID) + err = verifier.VerifyVote(nodeID, vote.SigData, block.View, block.BlockID) require.True(t, model.IsInvalidSignerError(err)) } @@ -121,11 +120,10 @@ func TestCombinedSignWithNoBeaconKeyV3(t *testing.T) { packer := signature.NewConsensusSigDataPacker(committee) verifier := NewCombinedVerifierV3(committee, packer) - proposal, err := signer.CreateProposal(block) + vote, err := signer.CreateVote(block) require.NoError(t, err) - vote := proposal.ProposerVote() - err = verifier.VerifyVote(nodeID, vote.SigData, proposal.Block.View, proposal.Block.BlockID) + err = verifier.VerifyVote(nodeID, vote.SigData, block.View, block.BlockID) require.NoError(t, err) // check that a created proposal's signature is a combined staking sig and random beacon sig @@ -136,7 +134,7 @@ func TestCombinedSignWithNoBeaconKeyV3(t *testing.T) { expectedSig := msig.EncodeSingleSig(encoding.SigTypeStaking, stakingSig) // check the signature only has staking sig - require.Equal(t, expectedSig, proposal.SigData) + require.Equal(t, expectedSig, vote.SigData) } // Test_VerifyQC checks that a QC where either signer list is empty is rejected as invalid diff --git a/consensus/hotstuff/verification/staking_signer_test.go b/consensus/hotstuff/verification/staking_signer_test.go index 69f31bdfed3..6fc4d14fdc5 100644 --- a/consensus/hotstuff/verification/staking_signer_test.go +++ b/consensus/hotstuff/verification/staking_signer_test.go @@ -15,57 +15,6 @@ import ( "github.com/onflow/flow-go/utils/unittest" ) -// TestStakingSigner_CreateProposal verifies that StakingSigner can produce correctly signed proposal -// that can be verified later using StakingVerifier. -// Additionally, we check cases where errors during signing are happening. -func TestStakingSigner_CreateProposal(t *testing.T) { - stakingPriv := unittest.StakingPrivKeyFixture() - signer := unittest.IdentityFixture() - signerID := signer.NodeID - signer.StakingPubKey = stakingPriv.PublicKey() - - t.Run("invalid-signer-id", func(t *testing.T) { - me := &modulemock.Local{} - me.On("NodeID").Return(signerID) - signer := NewStakingSigner(me) - - block := helper.MakeBlock() - proposal, err := signer.CreateProposal(block) - require.Error(t, err) - require.Nil(t, proposal) - }) - t.Run("could-not-sign", func(t *testing.T) { - signException := errors.New("sign-exception") - me := &modulemock.Local{} - me.On("NodeID").Return(signerID) - me.On("Sign", mock.Anything, mock.Anything).Return(nil, signException).Once() - signer := NewStakingSigner(me) - - block := helper.MakeBlock() - proposal, err := signer.CreateProposal(block) - require.ErrorAs(t, err, &signException) - require.Nil(t, proposal) - }) - t.Run("created-proposal", func(t *testing.T) { - me, err := local.New(signer.IdentitySkeleton, stakingPriv) - require.NoError(t, err) - - signerIdentity := &unittest.IdentityFixture(unittest.WithNodeID(signerID), - unittest.WithStakingPubKey(stakingPriv.PublicKey())).IdentitySkeleton - - signer := NewStakingSigner(me) - - block := helper.MakeBlock(helper.WithBlockProposer(signerID)) - proposal, err := signer.CreateProposal(block) - require.NoError(t, err) - require.NotNil(t, proposal) - - verifier := NewStakingVerifier() - err = verifier.VerifyVote(signerIdentity, proposal.SigData, proposal.Block.View, proposal.Block.BlockID) - require.NoError(t, err) - }) -} - // TestStakingSigner_CreateVote verifies that StakingSigner can produce correctly signed vote // that can be verified later using StakingVerifier. // Additionally, we check cases where errors during signing are happening. @@ -83,7 +32,7 @@ func TestStakingSigner_CreateVote(t *testing.T) { signer := NewStakingSigner(me) block := helper.MakeBlock() - proposal, err := signer.CreateProposal(block) + proposal, err := signer.CreateVote(block) require.ErrorAs(t, err, &signException) require.Nil(t, proposal) }) diff --git a/consensus/hotstuff/voteaggregator/vote_aggregator_test.go b/consensus/hotstuff/voteaggregator/vote_aggregator_test.go index 006ab52b744..acc88729eb1 100644 --- a/consensus/hotstuff/voteaggregator/vote_aggregator_test.go +++ b/consensus/hotstuff/voteaggregator/vote_aggregator_test.go @@ -84,13 +84,13 @@ func (s *VoteAggregatorTestSuite) TestOnFinalizedBlock() { // an input to AddBlock (only expects _valid_ blocks per API contract). // The exception should be propagated to the VoteAggregator's internal `ComponentManager`. func (s *VoteAggregatorTestSuite) TestProcessInvalidBlock() { - block := helper.MakeProposal( + block := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal( helper.WithBlock( helper.MakeBlock( helper.WithBlockView(100), ), ), - ) + ))) processed := make(chan struct{}) collector := mocks.NewVoteCollector(s.T()) collector.On("ProcessBlock", block).Run(func(_ mock.Arguments) { diff --git a/consensus/hotstuff/votecollector/combined_vote_processor_v2_test.go b/consensus/hotstuff/votecollector/combined_vote_processor_v2_test.go index 5b4abf1691c..74c07af04bc 100644 --- a/consensus/hotstuff/votecollector/combined_vote_processor_v2_test.go +++ b/consensus/hotstuff/votecollector/combined_vote_processor_v2_test.go @@ -57,7 +57,7 @@ func (s *CombinedVoteProcessorV2TestSuite) SetupTest() { s.reconstructor = &mockhotstuff.RandomBeaconReconstructor{} s.packer = &mockhotstuff.Packer{} - s.proposal = helper.MakeProposal() + s.proposal = helper.MakeSignedProposal() s.minRequiredShares = 9 // we require 9 RB shares to reconstruct signature s.rbSharesTotal = 0 @@ -894,7 +894,7 @@ func TestCombinedVoteProcessorV2_BuildVerifyQC(t *testing.T) { require.NoError(t, err) vote, err := safetyRules.SignOwnProposal(proposal) require.NoError(t, err) - proposal.SigData = vote.SigData + signedProposal := helper.MakeSignedProposal(helper.WithProposal(proposal), helper.WithSigData(vote.SigData)) qcCreated := false onQCCreated := func(qc *flow.QuorumCertificate) { @@ -912,7 +912,7 @@ func TestCombinedVoteProcessorV2_BuildVerifyQC(t *testing.T) { } voteProcessorFactory := NewCombinedVoteProcessorFactory(committee, onQCCreated) - voteProcessor, err := voteProcessorFactory.Create(unittest.Logger(), proposal) + voteProcessor, err := voteProcessorFactory.Create(unittest.Logger(), signedProposal) require.NoError(t, err) // process votes by new leader, this will result in producing new QC diff --git a/consensus/hotstuff/votecollector/combined_vote_processor_v3_test.go b/consensus/hotstuff/votecollector/combined_vote_processor_v3_test.go index f4e7e6c385c..c07a7c25787 100644 --- a/consensus/hotstuff/votecollector/combined_vote_processor_v3_test.go +++ b/consensus/hotstuff/votecollector/combined_vote_processor_v3_test.go @@ -58,7 +58,7 @@ func (s *CombinedVoteProcessorV3TestSuite) SetupTest() { s.rbSigAggregator = &mockhotstuff.WeightedSignatureAggregator{} s.reconstructor = &mockhotstuff.RandomBeaconReconstructor{} s.packer = &mockhotstuff.Packer{} - s.proposal = helper.MakeProposal() + s.proposal = helper.MakeSignedProposal() s.minRequiredShares = 9 // we require 9 RB shares to reconstruct signature s.thresholdTotalWeight, s.rbSharesTotal = atomic.Uint64{}, atomic.Uint64{} @@ -982,8 +982,7 @@ func TestCombinedVoteProcessorV3_BuildVerifyQC(t *testing.T) { leader := stakingSigners[0] - block := helper.MakeBlock(helper.WithBlockView(proposerView), - helper.WithBlockProposer(leader.NodeID)) + block := helper.MakeBlock(helper.WithBlockView(proposerView), helper.WithBlockProposer(leader.NodeID)) inmemDKG, err := inmem.DKGFromEncodable(inmem.EncodableDKG{ GroupKey: encodable.RandomBeaconPubKey{ @@ -1010,8 +1009,9 @@ func TestCombinedVoteProcessorV3_BuildVerifyQC(t *testing.T) { } // create and sign proposal - proposal, err := signers[leader.NodeID].CreateProposal(block) + leaderVote, err := signers[leader.NodeID].CreateVote(block) require.NoError(t, err) + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal(helper.WithBlock(block))), helper.WithSigData(leaderVote.SigData)) qcCreated := false onQCCreated := func(qc *flow.QuorumCertificate) { diff --git a/consensus/hotstuff/votecollector/factory_test.go b/consensus/hotstuff/votecollector/factory_test.go index 9adeaef98f8..40207150e86 100644 --- a/consensus/hotstuff/votecollector/factory_test.go +++ b/consensus/hotstuff/votecollector/factory_test.go @@ -19,7 +19,7 @@ import ( func TestVoteProcessorFactory_CreateWithValidProposal(t *testing.T) { mockedFactory := mockhotstuff.VoteProcessorFactory{} - proposal := helper.MakeProposal() + proposal := helper.MakeSignedProposal() mockedProcessor := &mockhotstuff.VerifyingVoteProcessor{} mockedProcessor.On("Process", proposal.ProposerVote()).Return(nil).Once() mockedFactory.On("Create", unittest.Logger(), proposal).Return(mockedProcessor, nil).Once() @@ -44,7 +44,7 @@ func TestVoteProcessorFactory_CreateWithInvalidVote(t *testing.T) { mockedFactory := mockhotstuff.VoteProcessorFactory{} t.Run("invalid-vote", func(t *testing.T) { - proposal := helper.MakeProposal() + proposal := helper.MakeSignedProposal() mockedProcessor := &mockhotstuff.VerifyingVoteProcessor{} mockedProcessor.On("Process", proposal.ProposerVote()).Return(model.NewInvalidVoteErrorf(proposal.ProposerVote(), "")).Once() mockedFactory.On("Create", unittest.Logger(), proposal).Return(mockedProcessor, nil).Once() @@ -63,7 +63,7 @@ func TestVoteProcessorFactory_CreateWithInvalidVote(t *testing.T) { mockedProcessor.AssertExpectations(t) }) t.Run("process-vote-exception", func(t *testing.T) { - proposal := helper.MakeProposal() + proposal := helper.MakeSignedProposal() mockedProcessor := &mockhotstuff.VerifyingVoteProcessor{} exception := errors.New("process-exception") mockedProcessor.On("Process", proposal.ProposerVote()).Return(exception).Once() @@ -93,7 +93,7 @@ func TestVoteProcessorFactory_CreateWithInvalidVote(t *testing.T) { func TestVoteProcessorFactory_CreateProcessException(t *testing.T) { mockedFactory := mockhotstuff.VoteProcessorFactory{} - proposal := helper.MakeProposal() + proposal := helper.MakeSignedProposal() exception := errors.New("create-exception") mockedFactory.On("Create", unittest.Logger(), proposal).Return(nil, exception).Once() diff --git a/consensus/hotstuff/votecollector/staking_vote_processor_test.go b/consensus/hotstuff/votecollector/staking_vote_processor_test.go index b536a8ab1da..1b096419c4d 100644 --- a/consensus/hotstuff/votecollector/staking_vote_processor_test.go +++ b/consensus/hotstuff/votecollector/staking_vote_processor_test.go @@ -285,8 +285,10 @@ func TestStakingVoteProcessorV2_BuildVerifyQC(t *testing.T) { } // create and sign proposal - proposal, err := signers[leader.NodeID].CreateProposal(block) + leaderVote, err := signers[leader.NodeID].CreateVote(block) require.NoError(t, err) + proposal := helper.MakeSignedProposal(helper.WithProposal( + helper.MakeProposal(helper.WithBlock(block))), helper.WithSigData(leaderVote.SigData)) qcCreated := false onQCCreated := func(qc *flow.QuorumCertificate) { diff --git a/consensus/hotstuff/votecollector/statemachine_test.go b/consensus/hotstuff/votecollector/statemachine_test.go index b2aaa313c51..ae15d68dfe1 100644 --- a/consensus/hotstuff/votecollector/statemachine_test.go +++ b/consensus/hotstuff/votecollector/statemachine_test.go @@ -78,7 +78,7 @@ func (s *StateMachineTestSuite) prepareMockedProcessor(proposal *model.SignedPro // when proposal processing can possibly change state of collector func (s *StateMachineTestSuite) TestStatus_StateTransitions() { block := helper.MakeBlock(helper.WithBlockView(s.view)) - proposal := helper.MakeProposal(helper.WithBlock(block)) + proposal := helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal(helper.WithBlock(block)))) s.prepareMockedProcessor(proposal) // by default, we should create in caching status @@ -90,9 +90,9 @@ func (s *StateMachineTestSuite) TestStatus_StateTransitions() { require.Equal(s.T(), hotstuff.VoteCollectorStatusVerifying, s.collector.Status()) // after submitting double proposal we should transfer into invalid state - err = s.collector.ProcessBlock(helper.MakeProposal( - helper.WithBlock( - helper.MakeBlock(helper.WithBlockView(s.view))))) + err = s.collector.ProcessBlock(helper.MakeSignedProposal(helper.WithProposal( + helper.MakeProposal(helper.WithBlock( + helper.MakeBlock(helper.WithBlockView(s.view))))))) require.NoError(s.T(), err) require.Equal(s.T(), hotstuff.VoteCollectorStatusInvalid, s.collector.Status()) } @@ -107,7 +107,8 @@ func (s *StateMachineTestSuite) Test_FactoryErrorPropagation() { s.collector.createVerifyingProcessor = factory // failing to create collector has to result in error and won't change state - err := s.collector.ProcessBlock(helper.MakeProposal(helper.WithBlock(helper.MakeBlock(helper.WithBlockView(s.view))))) + proposal := makeSignedProposalWithView(s.view) + err := s.collector.ProcessBlock(proposal) require.ErrorIs(s.T(), err, factoryError) require.Equal(s.T(), hotstuff.VoteCollectorStatusCaching, s.collector.Status()) } @@ -115,8 +116,8 @@ func (s *StateMachineTestSuite) Test_FactoryErrorPropagation() { // TestAddVote_VerifyingState tests that AddVote correctly process valid and invalid votes as well // as repeated, invalid and double votes in verifying state func (s *StateMachineTestSuite) TestAddVote_VerifyingState() { - block := helper.MakeBlock(helper.WithBlockView(s.view)) - proposal := helper.MakeProposal(helper.WithBlock(block)) + proposal := makeSignedProposalWithView(s.view) + block := proposal.Block processor := s.prepareMockedProcessor(proposal) err := s.collector.ProcessBlock(proposal) require.NoError(s.T(), err) @@ -203,8 +204,8 @@ func (s *StateMachineTestSuite) TestAddVote_VerifyingState() { // are sent to vote processor func (s *StateMachineTestSuite) TestProcessBlock_ProcessingOfCachedVotes() { votes := 10 - block := helper.MakeBlock(helper.WithBlockView(s.view)) - proposal := helper.MakeProposal(helper.WithBlock(block)) + proposal := makeSignedProposalWithView(s.view) + block := proposal.Block processor := s.prepareMockedProcessor(proposal) for i := 0; i < votes; i++ { vote := unittest.VoteForBlockFixture(block) @@ -226,11 +227,12 @@ func (s *StateMachineTestSuite) TestProcessBlock_ProcessingOfCachedVotes() { // Test_VoteProcessorErrorPropagation verifies that unexpected errors from the `VoteProcessor` // are propagated up the call stack (potentially wrapped), but are not replaced. func (s *StateMachineTestSuite) Test_VoteProcessorErrorPropagation() { - block := helper.MakeBlock(helper.WithBlockView(s.view)) - proposal := helper.MakeProposal(helper.WithBlock(block)) + proposal := makeSignedProposalWithView(s.view) + block := proposal.Block processor := s.prepareMockedProcessor(proposal) - err := s.collector.ProcessBlock(helper.MakeProposal(helper.WithBlock(block))) + err := s.collector.ProcessBlock(helper.MakeSignedProposal( + helper.WithProposal(helper.MakeProposal(helper.WithBlock(block))))) require.NoError(s.T(), err) unexpectedError := errors.New("some unexpected error") @@ -244,8 +246,8 @@ func (s *StateMachineTestSuite) Test_VoteProcessorErrorPropagation() { // in strict ordering of arrival. func (s *StateMachineTestSuite) RegisterVoteConsumer() { votes := 10 - block := helper.MakeBlock(helper.WithBlockView(s.view)) - proposal := helper.MakeProposal(helper.WithBlock(block)) + proposal := makeSignedProposalWithView(s.view) + block := proposal.Block processor := s.prepareMockedProcessor(proposal) expectedVotes := make([]*model.Vote, 0) for i := 0; i < votes; i++ { @@ -273,3 +275,7 @@ func (s *StateMachineTestSuite) RegisterVoteConsumer() { require.Equal(s.T(), expectedVotes, actualVotes) } + +func makeSignedProposalWithView(view uint64) *model.SignedProposal { + return helper.MakeSignedProposal(helper.WithProposal(helper.MakeProposal(helper.WithBlock(helper.MakeBlock(helper.WithBlockView(view)))))) +} diff --git a/consensus/hotstuff/votecollector/testutil.go b/consensus/hotstuff/votecollector/testutil.go index 2e305d96b66..4c9f2d288e2 100644 --- a/consensus/hotstuff/votecollector/testutil.go +++ b/consensus/hotstuff/votecollector/testutil.go @@ -27,7 +27,7 @@ type VoteProcessorTestSuiteBase struct { func (s *VoteProcessorTestSuiteBase) SetupTest() { s.stakingAggregator = &mockhotstuff.WeightedSignatureAggregator{} - s.proposal = helper.MakeProposal() + s.proposal = helper.MakeSignedProposal() // let's assume we have 19 nodes each with weight 100 s.sigWeight = 100 From 125a77ae16fd1b208d4ab560bda980e63110e32d Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 7 Oct 2024 14:25:04 +0300 Subject: [PATCH 05/13] Apply suggestions from code review Co-authored-by: Jordan Schalm --- consensus/hotstuff/forks/forks.go | 2 +- consensus/hotstuff/model/errors.go | 2 +- consensus/hotstuff/pacemaker/pacemaker.go | 2 +- consensus/hotstuff/pacemaker/view_tracker.go | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/consensus/hotstuff/forks/forks.go b/consensus/hotstuff/forks/forks.go index 8bba25508b9..bf7ee881b6c 100644 --- a/consensus/hotstuff/forks/forks.go +++ b/consensus/hotstuff/forks/forks.go @@ -401,7 +401,7 @@ func (f *Forks) checkForAdvancingFinalization(certifiedBlock *model.CertifiedBlo parentBlock := parentVertex.(*BlockContainer).Block() // Note: we assume that all stored blocks pass Forks.EnsureBlockIsValidExtension(block); - // specifically, that SignedProposal's ViewNumber is strictly monotonically + // specifically, that block's ViewNumber is strictly monotonically // increasing which is enforced by LevelledForest.VerifyVertex(...) // We denote: // * a DIRECT 1-chain as '<-' diff --git a/consensus/hotstuff/model/errors.go b/consensus/hotstuff/model/errors.go index 034aec7e0fd..75f79de92fd 100644 --- a/consensus/hotstuff/model/errors.go +++ b/consensus/hotstuff/model/errors.go @@ -113,7 +113,7 @@ type MissingBlockError struct { } func (e MissingBlockError) Error() string { - return fmt.Sprintf("missing SignedProposal at view %d with ID %v", e.View, e.BlockID) + return fmt.Sprintf("missing block at view %d with ID %v", e.View, e.BlockID) } // IsMissingBlockError returns whether an error is MissingBlockError diff --git a/consensus/hotstuff/pacemaker/pacemaker.go b/consensus/hotstuff/pacemaker/pacemaker.go index ab9475311cc..ae62aee0ea2 100644 --- a/consensus/hotstuff/pacemaker/pacemaker.go +++ b/consensus/hotstuff/pacemaker/pacemaker.go @@ -120,7 +120,7 @@ func (p *ActivePaceMaker) ProcessQC(qc *flow.QuorumCertificate) (*model.NewViewE // ProcessTC notifies the Pacemaker of a new timeout certificate, which may allow // Pacemaker to fast-forward its current view. -// A nil TC is an expected valid input, so that callers may pass in e.g. `SignedProposal.LastViewTC`, +// A nil TC is an expected valid input, so that callers may pass in e.g. `Proposal.LastViewTC`, // which may or may not have a value. // No errors are expected, any error should be treated as exception func (p *ActivePaceMaker) ProcessTC(tc *flow.TimeoutCertificate) (*model.NewViewEvent, error) { diff --git a/consensus/hotstuff/pacemaker/view_tracker.go b/consensus/hotstuff/pacemaker/view_tracker.go index fc4dd74d7f8..b52822d0d5a 100644 --- a/consensus/hotstuff/pacemaker/view_tracker.go +++ b/consensus/hotstuff/pacemaker/view_tracker.go @@ -86,7 +86,7 @@ func (vt *viewTracker) ProcessQC(qc *flow.QuorumCertificate) (uint64, error) { } // ProcessTC ingests a TC, which might advance the current view. A nil TC is accepted as -// input, so that callers may pass in e.g. `SignedProposal.LastViewTC`, which may or may not have +// input, so that callers may pass in e.g. `Proposal.LastViewTC`, which may or may not have // a value. It returns the resulting view after processing the TC and embedded QC. // No errors are expected, any error should be treated as exception func (vt *viewTracker) ProcessTC(tc *flow.TimeoutCertificate) (uint64, error) { From 6125525d6818d45525dfe12bf257151e90f8bf59 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Tue, 22 Oct 2024 19:05:37 +0300 Subject: [PATCH 06/13] Update consensus/hotstuff/model/proposal.go Co-authored-by: Alexander Hentschel --- consensus/hotstuff/model/proposal.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/consensus/hotstuff/model/proposal.go b/consensus/hotstuff/model/proposal.go index 50676ac7d5c..b433608a2f9 100644 --- a/consensus/hotstuff/model/proposal.go +++ b/consensus/hotstuff/model/proposal.go @@ -23,6 +23,12 @@ type Proposal struct { // SignedProposal represent a new proposed block within HotStuff (and thus // a header in the bigger picture), signed by the proposer. +// +// CAUTION: the signature only covers the pair (Block.View, Block.BlockID). Therefore, only +// the data that is hashed into the BlockID is cryptographically secured by the proposer's +// signature. +// Specifically, the proposer's signature cannot be covered by the Block.BlockID, as the +// proposer _signs_ the Block.BlockID (otherwise we have a cyclic dependency). type SignedProposal struct { Proposal SigData []byte From fa3498ffa3e483f2311e073e92b5997a8ce11448 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Tue, 22 Oct 2024 19:08:03 +0300 Subject: [PATCH 07/13] Apply suggestions from code review Co-authored-by: Alexander Hentschel --- consensus/hotstuff/eventhandler/event_handler_test.go | 2 +- consensus/hotstuff/integration/filters_test.go | 2 +- consensus/hotstuff/safety_rules.go | 1 + consensus/hotstuff/safetyrules/safety_rules.go | 3 +-- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/consensus/hotstuff/eventhandler/event_handler_test.go b/consensus/hotstuff/eventhandler/event_handler_test.go index 72cdb5ae1b6..0747697ea3b 100644 --- a/consensus/hotstuff/eventhandler/event_handler_test.go +++ b/consensus/hotstuff/eventhandler/event_handler_test.go @@ -179,7 +179,7 @@ func NewForks(t *testing.T, finalized uint64) *Forks { } f.On("AddValidatedBlock", mock.Anything).Return(func(proposal *model.Block) error { - log.Info().Msgf("forks.AddValidatedBlock received SignedProposal for view: %v, QC: %v\n", proposal.View, proposal.QC.View) + log.Info().Msgf("forks.AddValidatedBlock received Block proposal for view: %v, QC: %v\n", proposal.View, proposal.QC.View) return f.addProposal(proposal) }).Maybe() diff --git a/consensus/hotstuff/integration/filters_test.go b/consensus/hotstuff/integration/filters_test.go index 77b41128292..323f3ca21a6 100644 --- a/consensus/hotstuff/integration/filters_test.go +++ b/consensus/hotstuff/integration/filters_test.go @@ -34,7 +34,7 @@ func BlockVotesBy(voterID flow.Identifier) VoteFilter { } // ProposalFilter is a filter function for dropping Proposals. -// Return value `true` implies that the the given SignedProposal should be +// Return value `true` implies that the given SignedProposal should be // dropped, while `false` indicates that the SignedProposal should be received. type ProposalFilter func(*model.SignedProposal) bool diff --git a/consensus/hotstuff/safety_rules.go b/consensus/hotstuff/safety_rules.go index 00ac9a63f51..df9cbc09057 100644 --- a/consensus/hotstuff/safety_rules.go +++ b/consensus/hotstuff/safety_rules.go @@ -41,6 +41,7 @@ type SafetyRules interface { // This is a sentinel error and _expected_ during normal operation. // All other errors are unexpected and potential symptoms of uncovered edge cases or corrupted internal state (fatal). ProduceVote(proposal *model.SignedProposal, curView uint64) (*model.Vote, error) + // ProduceTimeout takes current view, highest locally known QC and TC (optional, must be nil if and // only if QC is for previous view) and decides whether to produce timeout for current view. // Returns: diff --git a/consensus/hotstuff/safetyrules/safety_rules.go b/consensus/hotstuff/safetyrules/safety_rules.go index a4dccab090d..85778736a26 100644 --- a/consensus/hotstuff/safetyrules/safety_rules.go +++ b/consensus/hotstuff/safetyrules/safety_rules.go @@ -78,7 +78,7 @@ func (r *SafetyRules) ProduceVote(signedProposal *model.SignedProposal, curView } // produceVote implements the core Safety Rules to validate whether it is safe to vote. -// This method is to be used for voting for other leaders' blocks as well as this node's own proposals +// This method is to be used to vote for other leaders' blocks as well as this node's own proposals // under construction. We explicitly codify the important aspect that a proposer's signature for their // own block is conceptually also just a vote (we explicitly use that property when aggregating votes and // including the proposer's own vote into a QC). In order to express this conceptual equivalence in code, the @@ -172,7 +172,6 @@ func (r *SafetyRules) produceVote(proposal *model.Proposal, curView uint64) (*mo } return vote, nil - } // ProduceTimeout takes current view, highest locally known QC and TC (optional, must be nil if and From e05af5a4f2c57289c44b3b9b4da02f31519e8104 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Tue, 22 Oct 2024 19:13:07 +0300 Subject: [PATCH 08/13] Fixed the the typo --- consensus/hotstuff/forks/forks_test.go | 6 +++--- consensus/hotstuff/integration/filters_test.go | 4 ++-- engine/common/provider/engine.go | 2 +- integration/localnet/conf/tempo-local.yaml | 4 ++-- ledger/complete/mtrie/node/node.go | 6 +++--- ledger/complete/wal/checkpointer.go | 4 ++-- module/jobqueue/README.md | 2 +- module/signature/signing_tags.go | 2 +- network/message/Makefile | 2 +- state/protocol/prg/prg.go | 2 +- 10 files changed, 17 insertions(+), 17 deletions(-) diff --git a/consensus/hotstuff/forks/forks_test.go b/consensus/hotstuff/forks/forks_test.go index 9662533dd0d..7b9c7af9b50 100644 --- a/consensus/hotstuff/forks/forks_test.go +++ b/consensus/hotstuff/forks/forks_test.go @@ -261,7 +261,7 @@ func TestFinalize_Multiple2Chains(t *testing.T) { } // TestFinalize_OrphanedFork tests that we can finalize a block which causes a conflicting fork to be orphaned. -// We ingest the the following block tree: +// We ingest the following block tree: // // [◄(1) 2] [◄(2) 3] // [◄(2) 4] [◄(4) 5] [◄(5) 6] @@ -389,7 +389,7 @@ func TestIgnoreBlocksBelowFinalizedView(t *testing.T) { } // TestDoubleProposal tests that the DoubleProposal notification is emitted when two different -// blocks for the same view are added. We ingest the the following block tree: +// blocks for the same view are added. We ingest the following block tree: // // / [◄(1) 2] // [1] @@ -460,7 +460,7 @@ func TestConflictingQCs(t *testing.T) { } // TestConflictingFinalizedForks checks that finalizing 2 conflicting forks should return model.ByzantineThresholdExceededError -// We ingest the the following block tree: +// We ingest the following block tree: // // [◄(1) 2] [◄(2) 3] [◄(3) 4] [◄(4) 5] // [◄(2) 6] [◄(6) 7] [◄(7) 8] diff --git a/consensus/hotstuff/integration/filters_test.go b/consensus/hotstuff/integration/filters_test.go index 323f3ca21a6..8bf12fc6d3b 100644 --- a/consensus/hotstuff/integration/filters_test.go +++ b/consensus/hotstuff/integration/filters_test.go @@ -8,7 +8,7 @@ import ( ) // VoteFilter is a filter function for dropping Votes. -// Return value `true` implies that the the given Vote should be +// Return value `true` implies that the given Vote should be // dropped, while `false` indicates that the Vote should be received. type VoteFilter func(*model.Vote) bool @@ -61,7 +61,7 @@ func BlockProposalsBy(proposerID flow.Identifier) ProposalFilter { } // TimeoutObjectFilter is a filter function for dropping TimeoutObjects. -// Return value `true` implies that the the given TimeoutObject should be +// Return value `true` implies that the given TimeoutObject should be // dropped, while `false` indicates that the TimeoutObject should be received. type TimeoutObjectFilter func(*model.TimeoutObject) bool diff --git a/engine/common/provider/engine.go b/engine/common/provider/engine.go index 12593b614ef..745ed7dd585 100644 --- a/engine/common/provider/engine.go +++ b/engine/common/provider/engine.go @@ -266,7 +266,7 @@ func (e *Engine) onEntityRequest(request *internal.EntityRequest) error { e.log.Info(). Str("origin_id", request.OriginId.String()). Strs("entity_ids", flow.IdentifierList(entityIDs).Strings()). - Uint64("nonce", request.Nonce). // to match with the the entity request received log + Uint64("nonce", request.Nonce). // to match with the entity request received log Msg("entity response sent") return nil diff --git a/integration/localnet/conf/tempo-local.yaml b/integration/localnet/conf/tempo-local.yaml index d2f4089bbf8..fd453459942 100644 --- a/integration/localnet/conf/tempo-local.yaml +++ b/integration/localnet/conf/tempo-local.yaml @@ -41,7 +41,7 @@ storage: index_downsample_bytes: 1000 # number of bytes per index record encoding: zstd # block encoding/compression. options: none, gzip, lz4-64k, lz4-256k, lz4-1M, lz4, snappy, zstd, s2 wal: - path: /tmp/tempo/wal # where to store the the wal locally + path: /tmp/tempo/wal # where to store the wal locally encoding: snappy # wal encoding/compression. options: none, gzip, lz4-64k, lz4-256k, lz4-1M, lz4, snappy, zstd, s2 local: path: /tmp/tempo/blocks @@ -50,4 +50,4 @@ storage: queue_depth: 10000 overrides: - metrics_generator_processors: [service-graphs, span-metrics] \ No newline at end of file + metrics_generator_processors: [service-graphs, span-metrics] diff --git a/ledger/complete/mtrie/node/node.go b/ledger/complete/mtrie/node/node.go index 8446b9e2919..bd1d6b08140 100644 --- a/ledger/complete/mtrie/node/node.go +++ b/ledger/complete/mtrie/node/node.go @@ -211,18 +211,18 @@ func (n *Node) Path() *ledger.Path { return nil } -// Payload returns the the Node's payload. +// Payload returns the Node's payload. // Do NOT MODIFY returned slices! func (n *Node) Payload() *ledger.Payload { return n.payload } -// LeftChild returns the the Node's left child. +// LeftChild returns the Node's left child. // Only INTERIM nodes have children. // Do NOT MODIFY returned Node! func (n *Node) LeftChild() *Node { return n.lChild } -// RightChild returns the the Node's right child. +// RightChild returns the Node's right child. // Only INTERIM nodes have children. // Do NOT MODIFY returned Node! func (n *Node) RightChild() *Node { return n.rChild } diff --git a/ledger/complete/wal/checkpointer.go b/ledger/complete/wal/checkpointer.go index c9081134439..b67f2385440 100644 --- a/ledger/complete/wal/checkpointer.go +++ b/ledger/complete/wal/checkpointer.go @@ -1047,7 +1047,7 @@ func CopyCheckpointFile(filename string, from string, to string) ( []string, error, ) { - // It's possible that the trie dir does not yet exist. If not this will create the the required path + // It's possible that the trie dir does not yet exist. If not this will create the required path err := os.MkdirAll(to, 0700) if err != nil { return nil, err @@ -1091,7 +1091,7 @@ func CopyCheckpointFile(filename string, from string, to string) ( // the `to` directory func SoftlinkCheckpointFile(filename string, from string, to string) ([]string, error) { - // It's possible that the trie dir does not yet exist. If not this will create the the required path + // It's possible that the trie dir does not yet exist. If not this will create the required path err := os.MkdirAll(to, 0700) if err != nil { return nil, err diff --git a/module/jobqueue/README.md b/module/jobqueue/README.md index e36bc060144..9c9dbf8f239 100644 --- a/module/jobqueue/README.md +++ b/module/jobqueue/README.md @@ -77,7 +77,7 @@ The jobqueue architecture is optimized for "pull" style processes, where the job Some use cases might require "push" style jobs where there is a job producer that create new jobs, and a consumer that processes work from the producer. This is possible with the jobqueue, but requires the producer persist the jobs to a database, then implement the `Head` and `AtIndex` methods that allow accessing jobs by sequential `uint64` indexes. ### TODOs -1. Jobs at different index are processed in parallel, it's possible that there is a job takes a long time to work on, and causing too many completed jobs cached in memory before being used to update the the last processed job index. +1. Jobs at different index are processed in parallel, it's possible that there is a job takes a long time to work on, and causing too many completed jobs cached in memory before being used to update the last processed job index. `maxSearchAhead` will allow the job consumer to stop consume more blocks if too many jobs are completed, but the job at index lastProcesssed + 1 has not been unprocessed yet. The difference between `maxSearchAhead` and `maxProcessing` is that: `maxProcessing` allows at most `maxProcessing` number of works to process jobs. However, even if there is worker available, it might not be assigned to a job, because the job at index lastProcesssed +1 has not been done, it won't work on an job with index higher than `lastProcesssed + maxSearchAhead`. 2. accept callback to get notified when the consecutive job index is finished. diff --git a/module/signature/signing_tags.go b/module/signature/signing_tags.go index f2d142b4253..00d7e06903c 100644 --- a/module/signature/signing_tags.go +++ b/module/signature/signing_tags.go @@ -61,7 +61,7 @@ var ( // NewBLSHasher returns a hasher to be used for BLS signing and verifying // in the protocol and abstracts the hasher details from the protocol logic. // -// The hasher returned is the the expand-message step in the BLS hash-to-curve. +// The hasher returned is the expand-message step in the BLS hash-to-curve. // It uses a xof (extendable output function) based on KMAC128. It therefore has // 128-bytes outputs. func NewBLSHasher(tag string) hash.Hasher { diff --git a/network/message/Makefile b/network/message/Makefile index 9c83ad1c9dd..a9612fc3564 100644 --- a/network/message/Makefile +++ b/network/message/Makefile @@ -1,4 +1,4 @@ -# To re-generate the the protobuf go code, install tools first: +# To re-generate the protobuf go code, install tools first: # ``` # cd flow-go # make install-tools diff --git a/state/protocol/prg/prg.go b/state/protocol/prg/prg.go index 36b3b77751d..d17fd1a9ac0 100644 --- a/state/protocol/prg/prg.go +++ b/state/protocol/prg/prg.go @@ -17,7 +17,7 @@ const RandomSourceLength = crypto.SignatureLenBLSBLS12381 // The diversifier is used to further diversify the PRGs beyond the customizer. A diversifier // can be a slice of any length. If no diversification is needed, `diversifier` can be `nil`. // -// The function uses an extendable-output function (xof) to extract and expand the the input source, +// The function uses an extendable-output function (xof) to extract and expand the input source, // so that any source with enough entropy (at least 128 bits) can be used (no need to pre-hash). // Current implementation generates a ChaCha20-based CSPRG. // From b8f17f3d348444e09ee4fc272e4ce42ee1546ca6 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Tue, 22 Oct 2024 19:16:55 +0300 Subject: [PATCH 09/13] Update consensus/hotstuff/votecollector/combined_vote_processor_v3_test.go Co-authored-by: Alexander Hentschel --- .../hotstuff/votecollector/combined_vote_processor_v3_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/consensus/hotstuff/votecollector/combined_vote_processor_v3_test.go b/consensus/hotstuff/votecollector/combined_vote_processor_v3_test.go index c07a7c25787..f68f5dedaff 100644 --- a/consensus/hotstuff/votecollector/combined_vote_processor_v3_test.go +++ b/consensus/hotstuff/votecollector/combined_vote_processor_v3_test.go @@ -981,7 +981,6 @@ func TestCombinedVoteProcessorV3_BuildVerifyQC(t *testing.T) { } leader := stakingSigners[0] - block := helper.MakeBlock(helper.WithBlockView(proposerView), helper.WithBlockProposer(leader.NodeID)) inmemDKG, err := inmem.DKGFromEncodable(inmem.EncodableDKG{ From 7d4d6f4171ccde2446a0267fa3324cefbee217d4 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Tue, 22 Oct 2024 19:20:34 +0300 Subject: [PATCH 10/13] Apply suggestions from PR review --- .../eventhandler/event_handler_test.go | 2 +- consensus/hotstuff/helper/block.go | 26 +++++++++++++++++++ consensus/hotstuff/model/proposal.go | 20 -------------- .../verification/combined_signer_v2_test.go | 4 +-- .../verification/combined_signer_v3_test.go | 3 ++- .../votecollector/statemachine_test.go | 4 +-- 6 files changed, 32 insertions(+), 27 deletions(-) diff --git a/consensus/hotstuff/eventhandler/event_handler_test.go b/consensus/hotstuff/eventhandler/event_handler_test.go index 0747697ea3b..f01a7760e40 100644 --- a/consensus/hotstuff/eventhandler/event_handler_test.go +++ b/consensus/hotstuff/eventhandler/event_handler_test.go @@ -228,7 +228,7 @@ type BlockProducer struct { } func (b *BlockProducer) MakeBlockProposal(view uint64, qc *flow.QuorumCertificate, lastViewTC *flow.TimeoutCertificate) (*flow.Header, error) { - return model.SignedProposalToFlow(helper.MakeSignedProposal(helper.WithProposal( + return helper.SignedProposalToFlow(helper.MakeSignedProposal(helper.WithProposal( helper.MakeProposal(helper.WithBlock(helper.MakeBlock( helper.WithBlockView(view), helper.WithBlockQC(qc), diff --git a/consensus/hotstuff/helper/block.go b/consensus/hotstuff/helper/block.go index 0c55147dcbd..ce341f61476 100644 --- a/consensus/hotstuff/helper/block.go +++ b/consensus/hotstuff/helper/block.go @@ -101,3 +101,29 @@ func WithLastViewTC(lastViewTC *flow.TimeoutCertificate) func(*model.Proposal) { proposal.LastViewTC = lastViewTC } } + +// SignedProposalToFlow turns a block proposal into a flow header. +// +// CAUTION: This function is only suitable for TESTING purposes ONLY. +// In the conversion from `flow.Header` to HoStuff's `model.Block` we loose information +// (e.g. `ChainID` and `Height` are not included in `model.Block`) and hence the conversion +// is *not reversible*. This is on purpose, because we wanted to only expose data to +// HotStuff that HotStuff really needs. +func SignedProposalToFlow(proposal *model.SignedProposal) *flow.Header { + + block := proposal.Block + header := &flow.Header{ + ParentID: block.QC.BlockID, + PayloadHash: block.PayloadHash, + Timestamp: block.Timestamp, + View: block.View, + ParentView: block.QC.View, + ParentVoterIndices: block.QC.SignerIndices, + ParentVoterSigData: block.QC.SigData, + ProposerID: block.ProposerID, + ProposerSigData: proposal.SigData, + LastViewTC: proposal.LastViewTC, + } + + return header +} diff --git a/consensus/hotstuff/model/proposal.go b/consensus/hotstuff/model/proposal.go index b433608a2f9..fe28f1a64ba 100644 --- a/consensus/hotstuff/model/proposal.go +++ b/consensus/hotstuff/model/proposal.go @@ -65,23 +65,3 @@ func ProposalFromFlow(header *flow.Header) *Proposal { } return &proposal } - -// SignedProposalToFlow turns a block proposal into a flow header. -func SignedProposalToFlow(proposal *SignedProposal) *flow.Header { - - block := proposal.Block - header := &flow.Header{ - ParentID: block.QC.BlockID, - PayloadHash: block.PayloadHash, - Timestamp: block.Timestamp, - View: block.View, - ParentView: block.QC.View, - ParentVoterIndices: block.QC.SignerIndices, - ParentVoterSigData: block.QC.SigData, - ProposerID: block.ProposerID, - ProposerSigData: proposal.SigData, - LastViewTC: proposal.LastViewTC, - } - - return header -} diff --git a/consensus/hotstuff/verification/combined_signer_v2_test.go b/consensus/hotstuff/verification/combined_signer_v2_test.go index 5e2149456a3..4f31f730e72 100644 --- a/consensus/hotstuff/verification/combined_signer_v2_test.go +++ b/consensus/hotstuff/verification/combined_signer_v2_test.go @@ -75,7 +75,7 @@ func TestCombinedSignWithBeaconKey(t *testing.T) { safetyRules, err := safetyrules.New(signer, persist, committee) require.NoError(t, err) - // check that a created proposal can be verified by a verifier + // check that the proposer's vote for their own block (i.e. the proposer signature in the header) passes verification vote, err := safetyRules.SignOwnProposal(proposal) require.NoError(t, err) @@ -187,7 +187,7 @@ func TestCombinedSignWithNoBeaconKey(t *testing.T) { safetyRules, err := safetyrules.New(signer, persist, committee) require.NoError(t, err) - // check that a created proposal can be verified by a verifier + // check that the proposer's vote for their own block (i.e. the proposer signature in the header) passes verification vote, err := safetyRules.SignOwnProposal(proposal) require.NoError(t, err) diff --git a/consensus/hotstuff/verification/combined_signer_v3_test.go b/consensus/hotstuff/verification/combined_signer_v3_test.go index b8abd8f821f..9317268ce59 100644 --- a/consensus/hotstuff/verification/combined_signer_v3_test.go +++ b/consensus/hotstuff/verification/combined_signer_v3_test.go @@ -57,7 +57,7 @@ func TestCombinedSignWithBeaconKeyV3(t *testing.T) { packer := signature.NewConsensusSigDataPacker(committee) verifier := NewCombinedVerifierV3(committee, packer) - // check that a created proposal can be verified by a verifier + // check that the proposer's vote for their own block (i.e. the proposer signature in the header) passes verification vote, err := signer.CreateVote(block) require.NoError(t, err) @@ -120,6 +120,7 @@ func TestCombinedSignWithNoBeaconKeyV3(t *testing.T) { packer := signature.NewConsensusSigDataPacker(committee) verifier := NewCombinedVerifierV3(committee, packer) + // check that the proposer's vote for their own block (i.e. the proposer signature in the header) passes verification vote, err := signer.CreateVote(block) require.NoError(t, err) diff --git a/consensus/hotstuff/votecollector/statemachine_test.go b/consensus/hotstuff/votecollector/statemachine_test.go index ae15d68dfe1..1f6409c3136 100644 --- a/consensus/hotstuff/votecollector/statemachine_test.go +++ b/consensus/hotstuff/votecollector/statemachine_test.go @@ -90,9 +90,7 @@ func (s *StateMachineTestSuite) TestStatus_StateTransitions() { require.Equal(s.T(), hotstuff.VoteCollectorStatusVerifying, s.collector.Status()) // after submitting double proposal we should transfer into invalid state - err = s.collector.ProcessBlock(helper.MakeSignedProposal(helper.WithProposal( - helper.MakeProposal(helper.WithBlock( - helper.MakeBlock(helper.WithBlockView(s.view))))))) + err = s.collector.ProcessBlock(makeSignedProposalWithView(s.view)) require.NoError(s.T(), err) require.Equal(s.T(), hotstuff.VoteCollectorStatusInvalid, s.collector.Status()) } From 79782f2606babbf3b339689930e49abab3847ad6 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Tue, 22 Oct 2024 19:22:34 +0300 Subject: [PATCH 11/13] Linted --- consensus/hotstuff/integration/instance_test.go | 2 +- engine/consensus/compliance/core_test.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/consensus/hotstuff/integration/instance_test.go b/consensus/hotstuff/integration/instance_test.go index 8e777bf53e5..49b8e6e68bd 100644 --- a/consensus/hotstuff/integration/instance_test.go +++ b/consensus/hotstuff/integration/instance_test.go @@ -637,7 +637,7 @@ func (in *Instance) ProcessBlock(proposal *model.SignedProposal) { if parentExists { next := proposal for next != nil { - in.headers[next.Block.BlockID] = model.SignedProposalToFlow(next) + in.headers[next.Block.BlockID] = helper.SignedProposalToFlow(next) in.queue <- next // keep processing the pending blocks diff --git a/engine/consensus/compliance/core_test.go b/engine/consensus/compliance/core_test.go index 307dd5ad678..41a57a5ffc7 100644 --- a/engine/consensus/compliance/core_test.go +++ b/engine/consensus/compliance/core_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" + "github.com/onflow/flow-go/consensus/hotstuff/helper" hotstuff "github.com/onflow/flow-go/consensus/hotstuff/mocks" "github.com/onflow/flow-go/consensus/hotstuff/model" consensus "github.com/onflow/flow-go/engine/consensus/mock" @@ -626,7 +627,7 @@ func (cs *CoreSuite) TestProposalBufferingOrder() { } // mark the proposal as processed delete(unprocessed, header.BlockID) - cs.headerDB[header.BlockID] = model.SignedProposalToFlow(proposal) + cs.headerDB[header.BlockID] = helper.SignedProposalToFlow(proposal) calls++ }, ) From 497654848c0e13748ed547db62986f752b29f5e5 Mon Sep 17 00:00:00 2001 From: Alexander Hentschel Date: Fri, 25 Oct 2024 16:56:54 -0700 Subject: [PATCH 12/13] minor revision of godoc --- consensus/hotstuff/model/proposal.go | 16 +++++++++++++--- consensus/hotstuff/safety_rules.go | 2 +- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/consensus/hotstuff/model/proposal.go b/consensus/hotstuff/model/proposal.go index fe28f1a64ba..b5b25ccdaf2 100644 --- a/consensus/hotstuff/model/proposal.go +++ b/consensus/hotstuff/model/proposal.go @@ -7,15 +7,25 @@ import ( // Proposal represents a block proposal under construction. // In order to decide whether a proposal is safe to sign, HotStuff's Safety Rules require // proof that the leader entered the respective view in a protocol-compliant manner. Specifically, -// we require the QC (mandatory part of every block) and optionally a TC (of the QC in the block -// is _not_ for the immediately previous view). Note that the proposer's signature for its own -// block is _not_ required. +// we require a TimeoutCertificate [TC] if and only if the QC in the block is _not_ for the +// immediately preceding view. Thereby we protect the consensus process from malicious leaders +// attempting to skip views that haven't concluded yet (a form of front-running attack). +// However, LastViewTC is only relevant until a QC is known that certifies the correctness of +// the block. Thereafter, the QC attests that honest consensus participants have confirmed the +// validity of the fork up to the latest certified block (including protocol-compliant view transitions). +// // By explicitly differentiating the Proposal from the SignedProposal (extending Proposal by // adding the proposer's signature), we can unify the algorithmic path of signing block proposals. // This codifies the important aspect that a proposer's signature for their own block // is conceptually also just a vote (we explicitly use that for aggregating votes, including the // proposer's own vote to a QC). In order to express this conceptual equivalence in code, the // voting logic in Safety Rules must also operate on an unsigned Proposal. +// +// TODO: atm, the flow.Header embeds the LastViewTC. However, for HotStuff we have `model.Block` +// and `model.Proposal`, where the latter was introduced when we added the PaceMaker to +// vanilla HotStuff. It would be more consistent, if we added `LastViewTC` to `model.Block`, +// or even better, introduce an interface for HotStuff's notion of a block (exposing +// the fields in `model.Block` plus LastViewTC) type Proposal struct { Block *Block LastViewTC *flow.TimeoutCertificate diff --git a/consensus/hotstuff/safety_rules.go b/consensus/hotstuff/safety_rules.go index df9cbc09057..7e6e29b392f 100644 --- a/consensus/hotstuff/safety_rules.go +++ b/consensus/hotstuff/safety_rules.go @@ -31,7 +31,7 @@ type SafetyData struct { // Implementations are generally *not* concurrency safe. type SafetyRules interface { // ProduceVote takes a block proposal and current view, and decides whether to vote for the block. - // Voting is deterministic meaning voting for same proposal will always result in the same vote. + // Voting is deterministic, i.e. voting for same proposal will always result in the same vote. // Returns: // * (vote, nil): On the _first_ block for the current view that is safe to vote for. // Subsequently, voter does _not_ vote for any _other_ block with the same (or lower) view. From 78dd10d625981f22cc78469ab69b92ea23e9ea7e Mon Sep 17 00:00:00 2001 From: Alexander Hentschel Date: Fri, 25 Oct 2024 19:23:18 -0700 Subject: [PATCH 13/13] linted code --- consensus/hotstuff/model/proposal.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/consensus/hotstuff/model/proposal.go b/consensus/hotstuff/model/proposal.go index b5b25ccdaf2..6dcca721583 100644 --- a/consensus/hotstuff/model/proposal.go +++ b/consensus/hotstuff/model/proposal.go @@ -22,10 +22,10 @@ import ( // voting logic in Safety Rules must also operate on an unsigned Proposal. // // TODO: atm, the flow.Header embeds the LastViewTC. However, for HotStuff we have `model.Block` -// and `model.Proposal`, where the latter was introduced when we added the PaceMaker to -// vanilla HotStuff. It would be more consistent, if we added `LastViewTC` to `model.Block`, -// or even better, introduce an interface for HotStuff's notion of a block (exposing -// the fields in `model.Block` plus LastViewTC) +// and `model.Proposal`, where the latter was introduced when we added the PaceMaker to +// vanilla HotStuff. It would be more consistent, if we added `LastViewTC` to `model.Block`, +// or even better, introduce an interface for HotStuff's notion of a block (exposing +// the fields in `model.Block` plus LastViewTC) type Proposal struct { Block *Block LastViewTC *flow.TimeoutCertificate