From 2a1b38ce7a6f4c6316463cf48dadf6ae5ac1e379 Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Wed, 24 Nov 2021 19:34:59 +0200 Subject: [PATCH] Fix mergeset limit violation bug (#1855) * Added a test which reproduces a bug where virtual's mergeset exceeds the mergeset limit -- this test currently fails * Added a simple condition to fix the mergeset limit bug * Format issue --- .../block_header_in_context_test.go | 62 ++++++++++++++++++- .../pick_virtual_parents.go | 3 +- 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/domain/consensus/processes/blockvalidator/block_header_in_context_test.go b/domain/consensus/processes/blockvalidator/block_header_in_context_test.go index fc2455f8c..d55bd313b 100644 --- a/domain/consensus/processes/blockvalidator/block_header_in_context_test.go +++ b/domain/consensus/processes/blockvalidator/block_header_in_context_test.go @@ -165,14 +165,15 @@ func TestCheckMergeSizeLimit(t *testing.T) { testutils.ForAllNets(t, true, func(t *testing.T, consensusConfig *consensus.Config) { consensusConfig.MergeSetSizeLimit = 2 * uint64(consensusConfig.K) factory := consensus.NewFactory() - tc, teardown, err := factory.NewTestConsensus(consensusConfig, "TestCheckParentsIncest") + tc, teardown, err := factory.NewTestConsensus(consensusConfig, "TestCheckMergeSizeLimit") if err != nil { t.Fatalf("Error setting up consensus: %+v", err) } defer teardown(false) chain1TipHash := consensusConfig.GenesisHash - for i := uint64(0); i < consensusConfig.MergeSetSizeLimit+2; i++ { + // We add a chain larger by one than chain2 below, to make this one the selected chain + for i := uint64(0); i < consensusConfig.MergeSetSizeLimit+1; i++ { chain1TipHash, _, err = tc.AddBlock([]*externalapi.DomainHash{chain1TipHash}, nil, nil) if err != nil { t.Fatalf("AddBlock: %+v", err) @@ -180,7 +181,9 @@ func TestCheckMergeSizeLimit(t *testing.T) { } chain2TipHash := consensusConfig.GenesisHash - for i := uint64(0); i < consensusConfig.MergeSetSizeLimit+1; i++ { + // We add a merge set of size exactly MergeSetSizeLimit (to violate the limit), + // since selected parent is also counted + for i := uint64(0); i < consensusConfig.MergeSetSizeLimit; i++ { chain2TipHash, _, err = tc.AddBlock([]*externalapi.DomainHash{chain2TipHash}, nil, nil) if err != nil { t.Fatalf("AddBlock: %+v", err) @@ -193,3 +196,56 @@ func TestCheckMergeSizeLimit(t *testing.T) { } }) } + +func TestVirtualSelectionViolatingMergeSizeLimit(t *testing.T) { + testutils.ForAllNets(t, true, func(t *testing.T, consensusConfig *consensus.Config) { + consensusConfig.MergeSetSizeLimit = 2 * uint64(consensusConfig.K) + factory := consensus.NewFactory() + tc, teardown, err := factory.NewTestConsensus(consensusConfig, "TestVirtualSelectionViolatingMergeSizeLimit") + if err != nil { + t.Fatalf("Error setting up consensus: %+v", err) + } + defer teardown(false) + + chain1TipHash := consensusConfig.GenesisHash + // We add a chain larger than chain2 below, to make this one the selected chain + for i := uint64(0); i < consensusConfig.MergeSetSizeLimit; i++ { + chain1TipHash, _, err = tc.AddBlock([]*externalapi.DomainHash{chain1TipHash}, nil, nil) + if err != nil { + t.Fatalf("AddBlock: %+v", err) + } + } + + chain2TipHash := consensusConfig.GenesisHash + // We add a merge set of size exactly MergeSetSizeLimit-1 (to still not violate the limit) + for i := uint64(0); i < consensusConfig.MergeSetSizeLimit-1; i++ { + chain2TipHash, _, err = tc.AddBlock([]*externalapi.DomainHash{chain2TipHash}, nil, nil) + if err != nil { + t.Fatalf("AddBlock: %+v", err) + } + } + + // We now add a single block over genesis which is expected to exceed the limit + _, _, err = tc.AddBlock([]*externalapi.DomainHash{consensusConfig.GenesisHash}, nil, nil) + if err != nil { + t.Fatalf("AddBlock: %+v", err) + } + + stagingArea := model.NewStagingArea() + virtualSelectedParent, err := tc.GetVirtualSelectedParent() + if err != nil { + t.Fatalf("GetVirtualSelectedParent: %+v", err) + } + selectedParentAnticone, err := tc.DAGTraversalManager().AnticoneFromVirtualPOV(stagingArea, virtualSelectedParent) + if err != nil { + t.Fatalf("AnticoneFromVirtualPOV: %+v", err) + } + + // Test if Virtual's mergeset is too large + // Note: the selected parent itself is also counted in the mergeset limit + if len(selectedParentAnticone)+1 > (int)(consensusConfig.MergeSetSizeLimit) { + t.Fatalf("Virtual's mergset size (%d) exeeds merge set limit (%d)", + len(selectedParentAnticone)+1, consensusConfig.MergeSetSizeLimit) + } + }) +} diff --git a/domain/consensus/processes/consensusstatemanager/pick_virtual_parents.go b/domain/consensus/processes/consensusstatemanager/pick_virtual_parents.go index 3ea993d59..a232a88d6 100644 --- a/domain/consensus/processes/consensusstatemanager/pick_virtual_parents.go +++ b/domain/consensus/processes/consensusstatemanager/pick_virtual_parents.go @@ -59,7 +59,8 @@ func (csm *consensusStateManager) pickVirtualParents(stagingArea *model.StagingA selectedVirtualParents := []*externalapi.DomainHash{virtualSelectedParent} mergeSetSize := uint64(1) // starts counting from 1 because selectedParent is already in the mergeSet - for len(candidates) > 0 && uint64(len(selectedVirtualParents)) < uint64(csm.maxBlockParents) { + // First condition implies that no point in searching since limit was already reached + for mergeSetSize < csm.mergeSetSizeLimit && len(candidates) > 0 && uint64(len(selectedVirtualParents)) < uint64(csm.maxBlockParents) { candidate := candidates[0] candidates = candidates[1:]