From 9f9610df49988179bcc4c09fe2ee78a3468f4410 Mon Sep 17 00:00:00 2001 From: msutton Date: Thu, 14 Jul 2022 18:30:30 +0300 Subject: [PATCH] Fixed a bug in the reported virtual changeset, and modified the test to verify it --- domain/consensus/consensus.go | 16 +++---- .../consensus/model/testapi/test_consensus.go | 2 +- .../consensusstatemanager/resolve.go | 2 +- .../resolve_virtual_test.go | 45 ++++++++++++++----- domain/consensus/test_consensus.go | 2 +- 5 files changed, 45 insertions(+), 22 deletions(-) diff --git a/domain/consensus/consensus.go b/domain/consensus/consensus.go index d6c71ce38..610c737ef 100644 --- a/domain/consensus/consensus.go +++ b/domain/consensus/consensus.go @@ -916,7 +916,7 @@ func (s *consensus) ResolveVirtual(progressReportCallback func(uint64, uint64)) // release the lock each time we resolve 100 blocks. // Note: maxBlocksToResolve should be smaller than `params.FinalityDuration` in order to avoid a situation // where UpdatePruningPointByVirtual skips a pruning point. - isCompletelyResolved, err := s.resolveVirtualChunkWithLock(100) + _, isCompletelyResolved, err := s.resolveVirtualChunkWithLock(100) if err != nil { return err } @@ -928,37 +928,37 @@ func (s *consensus) ResolveVirtual(progressReportCallback func(uint64, uint64)) return nil } -func (s *consensus) resolveVirtualChunkWithLock(maxBlocksToResolve uint64) (bool, error) { +func (s *consensus) resolveVirtualChunkWithLock(maxBlocksToResolve uint64) (*externalapi.VirtualChangeSet, bool, error) { s.lock.Lock() defer s.lock.Unlock() return s.resolveVirtualNoLock(maxBlocksToResolve) } -func (s *consensus) resolveVirtualNoLock(maxBlocksToResolve uint64) (bool, error) { +func (s *consensus) resolveVirtualNoLock(maxBlocksToResolve uint64) (*externalapi.VirtualChangeSet, bool, error) { virtualChangeSet, isCompletelyResolved, err := s.consensusStateManager.ResolveVirtual(maxBlocksToResolve) if err != nil { - return false, err + return nil, false, err } s.virtualNotUpdated = !isCompletelyResolved stagingArea := model.NewStagingArea() err = s.pruningManager.UpdatePruningPointByVirtual(stagingArea) if err != nil { - return false, err + return nil, false, err } err = staging.CommitAllChanges(s.databaseContext, stagingArea) if err != nil { - return false, err + return nil, false, err } err = s.sendVirtualChangedEvent(virtualChangeSet, true) if err != nil { - return false, err + return nil, false, err } - return isCompletelyResolved, nil + return virtualChangeSet, isCompletelyResolved, nil } func (s *consensus) BuildPruningPointProof() (*externalapi.PruningPointProof, error) { diff --git a/domain/consensus/model/testapi/test_consensus.go b/domain/consensus/model/testapi/test_consensus.go index 828cabd1a..f7be59406 100644 --- a/domain/consensus/model/testapi/test_consensus.go +++ b/domain/consensus/model/testapi/test_consensus.go @@ -49,7 +49,7 @@ type TestConsensus interface { *externalapi.VirtualChangeSet, error) UpdatePruningPointByVirtual() error - ResolveVirtualWithMaxParam(maxBlocksToResolve uint64) (bool, error) + ResolveVirtualWithMaxParam(maxBlocksToResolve uint64) (*externalapi.VirtualChangeSet, bool, error) MineJSON(r io.Reader, blockType MineJSONBlockType) (tips []*externalapi.DomainHash, err error) ToJSON(w io.Writer) error diff --git a/domain/consensus/processes/consensusstatemanager/resolve.go b/domain/consensus/processes/consensusstatemanager/resolve.go index 4e151f7aa..737de40eb 100644 --- a/domain/consensus/processes/consensusstatemanager/resolve.go +++ b/domain/consensus/processes/consensusstatemanager/resolve.go @@ -203,7 +203,7 @@ func (csm *consensusStateManager) ResolveVirtual(maxBlocksToResolve uint64) (*ex } selectedParentChainChanges, err := csm.dagTraversalManager. - CalculateChainPath(updateVirtualStagingArea, previousVirtualSelectedParent, pendingTip) + CalculateChainPath(updateVirtualStagingArea, previousVirtualSelectedParent, processingPoint) if err != nil { return nil, false, err } diff --git a/domain/consensus/processes/consensusstatemanager/resolve_virtual_test.go b/domain/consensus/processes/consensusstatemanager/resolve_virtual_test.go index 5407843db..5257c45b8 100644 --- a/domain/consensus/processes/consensusstatemanager/resolve_virtual_test.go +++ b/domain/consensus/processes/consensusstatemanager/resolve_virtual_test.go @@ -56,7 +56,7 @@ func TestAddBlockBetweenResolveVirtualCalls(t *testing.T) { } // Resolve one step - _, err = tc.ResolveVirtualWithMaxParam(2) + _, _, err = tc.ResolveVirtualWithMaxParam(2) if err != nil { t.Fatalf("Error resolving virtual in re-org chain: %+v", err) } @@ -75,7 +75,7 @@ func TestAddBlockBetweenResolveVirtualCalls(t *testing.T) { } // Resolve one more step - isCompletelyResolved, err := tc.ResolveVirtualWithMaxParam(2) + _, isCompletelyResolved, err := tc.ResolveVirtualWithMaxParam(2) if err != nil { t.Fatalf("Error resolving virtual in re-org chain: %+v", err) } @@ -89,7 +89,7 @@ func TestAddBlockBetweenResolveVirtualCalls(t *testing.T) { // Complete resolving virtual for !isCompletelyResolved { - isCompletelyResolved, err = tc.ResolveVirtualWithMaxParam(2) + _, isCompletelyResolved, err = tc.ResolveVirtualWithMaxParam(2) if err != nil { t.Fatalf("Error resolving virtual in re-org chain: %+v", err) } @@ -142,7 +142,7 @@ func TestAddGenesisChildAfterOneResolveVirtualCall(t *testing.T) { } // Resolve one step - isCompletelyResolved, err := tc.ResolveVirtualWithMaxParam(2) + _, isCompletelyResolved, err := tc.ResolveVirtualWithMaxParam(2) if err != nil { t.Fatalf("Error resolving virtual in re-org chain: %+v", err) } @@ -154,7 +154,7 @@ func TestAddGenesisChildAfterOneResolveVirtualCall(t *testing.T) { // Complete resolving virtual for !isCompletelyResolved { - isCompletelyResolved, err = tc.ResolveVirtualWithMaxParam(2) + _, isCompletelyResolved, err = tc.ResolveVirtualWithMaxParam(2) if err != nil { t.Fatalf("Error resolving virtual in re-org chain: %+v", err) } @@ -207,13 +207,13 @@ func TestAddGenesisChildAfterTwoResolveVirtualCalls(t *testing.T) { } // Resolve one step - _, err = tc.ResolveVirtualWithMaxParam(2) + _, _, err = tc.ResolveVirtualWithMaxParam(2) if err != nil { t.Fatalf("Error resolving virtual in re-org chain: %+v", err) } // Resolve one more step - isCompletelyResolved, err := tc.ResolveVirtualWithMaxParam(2) + _, isCompletelyResolved, err := tc.ResolveVirtualWithMaxParam(2) if err != nil { t.Fatalf("Error resolving virtual in re-org chain: %+v", err) } @@ -225,7 +225,7 @@ func TestAddGenesisChildAfterTwoResolveVirtualCalls(t *testing.T) { // Complete resolving virtual for !isCompletelyResolved { - isCompletelyResolved, err = tc.ResolveVirtualWithMaxParam(2) + _, isCompletelyResolved, err = tc.ResolveVirtualWithMaxParam(2) if err != nil { t.Fatalf("Error resolving virtual in re-org chain: %+v", err) } @@ -296,22 +296,45 @@ func TestResolveVirtualBackAndForthReorgs(t *testing.T) { printUtxoDiffChildren(t, tc, hashes, blocks) verifyUtxoDiffPaths(t, tc, hashes) + previousVirtualSelectedParent, err := tc.GetVirtualSelectedParent() + if err != nil { + t.Fatal(err) + } + // Resolve one step - _, err = tc.ResolveVirtualWithMaxParam(3) + virtualChangeSet, _, err := tc.ResolveVirtualWithMaxParam(3) if err != nil { printUtxoDiffChildren(t, tc, hashes, blocks) t.Fatalf("Error resolving virtual in re-org chain: %+v", err) } + newVirtualSelectedParent, err := tc.GetVirtualSelectedParent() + if err != nil { + t.Fatal(err) + } + + // Make sure the reported change-set is compatible with actual changes. + // Checking this for one call should suffice to avoid possible bugs. + reportedPreviousVirtualSelectedParent := virtualChangeSet.VirtualSelectedParentChainChanges.Removed[0] + reportedNewVirtualSelectedParent := virtualChangeSet.VirtualSelectedParentChainChanges. + Added[len(virtualChangeSet.VirtualSelectedParentChainChanges.Added)-1] + + if !previousVirtualSelectedParent.Equal(reportedPreviousVirtualSelectedParent) { + t.Fatalf("The reported changeset is incompatible with actual changes") + } + if !newVirtualSelectedParent.Equal(reportedNewVirtualSelectedParent) { + t.Fatalf("The reported changeset is incompatible with actual changes") + } + // Resolve one more step - isCompletelyResolved, err := tc.ResolveVirtualWithMaxParam(3) + _, isCompletelyResolved, err := tc.ResolveVirtualWithMaxParam(3) if err != nil { t.Fatalf("Error resolving virtual in re-org chain: %+v", err) } // Complete resolving virtual for !isCompletelyResolved { - isCompletelyResolved, err = tc.ResolveVirtualWithMaxParam(3) + _, isCompletelyResolved, err = tc.ResolveVirtualWithMaxParam(3) if err != nil { t.Fatalf("Error resolving virtual in re-org chain: %+v", err) } diff --git a/domain/consensus/test_consensus.go b/domain/consensus/test_consensus.go index 0eda211c9..56b1fee97 100644 --- a/domain/consensus/test_consensus.go +++ b/domain/consensus/test_consensus.go @@ -112,7 +112,7 @@ func (tc *testConsensus) AddUTXOInvalidBlock(parentHashes []*externalapi.DomainH return consensushashing.BlockHash(block), virtualChangeSet, nil } -func (tc *testConsensus) ResolveVirtualWithMaxParam(maxBlocksToResolve uint64) (bool, error) { +func (tc *testConsensus) ResolveVirtualWithMaxParam(maxBlocksToResolve uint64) (*externalapi.VirtualChangeSet, bool, error) { tc.lock.Lock() defer tc.lock.Unlock()