diff --git a/snow/consensus/snowman/consensus.go b/snow/consensus/snowman/consensus.go index 3f1006416366..1eaff4b0e2f4 100644 --- a/snow/consensus/snowman/consensus.go +++ b/snow/consensus/snowman/consensus.go @@ -31,9 +31,13 @@ type Consensus interface { // Returns the number of blocks processing NumProcessing() int - // Adds a new decision. Assumes the dependency has already been added. + // Add a new block. + // + // Add should not be called multiple times with the same block. + // The parent block should either be the last accepted block or processing. + // // Returns if a critical error has occurred. - Add(context.Context, Block) error + Add(Block) error // Decided returns true if the block has been decided. Decided(Block) bool diff --git a/snow/consensus/snowman/consensus_test.go b/snow/consensus/snowman/consensus_test.go index d52e76759512..b4cb5b03a494 100644 --- a/snow/consensus/snowman/consensus_test.go +++ b/snow/consensus/snowman/consensus_test.go @@ -32,7 +32,7 @@ var ( NumProcessingTest, AddToTailTest, AddToNonTailTest, - AddToUnknownTest, + AddOnUnknownParentTest, StatusOrProcessingPreviouslyAcceptedTest, StatusOrProcessingPreviouslyRejectedTest, StatusOrProcessingUnissuedTest, @@ -51,7 +51,6 @@ var ( MetricsProcessingErrorTest, MetricsAcceptedErrorTest, MetricsRejectedErrorTest, - ErrorOnInitialRejectionTest, ErrorOnAcceptTest, ErrorOnRejectSiblingTest, ErrorOnTransitiveRejectionTest, @@ -139,7 +138,7 @@ func NumProcessingTest(t *testing.T, factory Factory) { require.Zero(sm.NumProcessing()) // Adding to the previous preference will update the preference - require.NoError(sm.Add(context.Background(), block)) + require.NoError(sm.Add(block)) require.Equal(1, sm.NumProcessing()) votes := bag.Of(block.ID()) @@ -176,7 +175,7 @@ func AddToTailTest(t *testing.T, factory Factory) { block := snowmantest.BuildChild(snowmantest.Genesis) // Adding to the previous preference will update the preference - require.NoError(sm.Add(context.Background(), block)) + require.NoError(sm.Add(block)) require.Equal(block.ID(), sm.Preference()) require.True(sm.IsPreferred(block)) @@ -215,18 +214,18 @@ func AddToNonTailTest(t *testing.T, factory Factory) { secondBlock := snowmantest.BuildChild(snowmantest.Genesis) // Adding to the previous preference will update the preference - require.NoError(sm.Add(context.Background(), firstBlock)) + require.NoError(sm.Add(firstBlock)) require.Equal(firstBlock.IDV, sm.Preference()) // Adding to something other than the previous preference won't update the // preference - require.NoError(sm.Add(context.Background(), secondBlock)) + require.NoError(sm.Add(secondBlock)) require.Equal(firstBlock.IDV, sm.Preference()) } // Make sure that adding a block that is detached from the rest of the tree -// rejects the block -func AddToUnknownTest(t *testing.T, factory Factory) { +// returns an error +func AddOnUnknownParentTest(t *testing.T, factory Factory) { require := require.New(t) sm := factory.New() @@ -260,11 +259,9 @@ func AddToUnknownTest(t *testing.T, factory Factory) { HeightV: snowmantest.GenesisHeight + 2, } - // Adding a block with an unknown parent means the parent must have already - // been rejected. Therefore the block should be immediately rejected - require.NoError(sm.Add(context.Background(), block)) - require.Equal(snowmantest.GenesisID, sm.Preference()) - require.Equal(choices.Rejected, block.Status()) + // Adding a block with an unknown parent should error. + err := sm.Add(block) + require.ErrorIs(err, errUnknownParentBlock) } func StatusOrProcessingPreviouslyAcceptedTest(t *testing.T, factory Factory) { @@ -402,7 +399,7 @@ func StatusOrProcessingIssuedTest(t *testing.T, factory Factory) { block := snowmantest.BuildChild(snowmantest.Genesis) - require.NoError(sm.Add(context.Background(), block)) + require.NoError(sm.Add(block)) require.Equal(choices.Processing, block.Status()) require.True(sm.Processing(block.ID())) require.False(sm.Decided(block)) @@ -440,7 +437,7 @@ func RecordPollAcceptSingleBlockTest(t *testing.T, factory Factory) { block := snowmantest.BuildChild(snowmantest.Genesis) - require.NoError(sm.Add(context.Background(), block)) + require.NoError(sm.Add(block)) votes := bag.Of(block.ID()) require.NoError(sm.RecordPoll(context.Background(), votes)) @@ -482,8 +479,8 @@ func RecordPollAcceptAndRejectTest(t *testing.T, factory Factory) { firstBlock := snowmantest.BuildChild(snowmantest.Genesis) secondBlock := snowmantest.BuildChild(snowmantest.Genesis) - require.NoError(sm.Add(context.Background(), firstBlock)) - require.NoError(sm.Add(context.Background(), secondBlock)) + require.NoError(sm.Add(firstBlock)) + require.NoError(sm.Add(secondBlock)) votes := bag.Of(firstBlock.ID()) @@ -530,8 +527,8 @@ func RecordPollSplitVoteNoChangeTest(t *testing.T, factory Factory) { firstBlock := snowmantest.BuildChild(snowmantest.Genesis) secondBlock := snowmantest.BuildChild(snowmantest.Genesis) - require.NoError(sm.Add(context.Background(), firstBlock)) - require.NoError(sm.Add(context.Background(), secondBlock)) + require.NoError(sm.Add(firstBlock)) + require.NoError(sm.Add(secondBlock)) votes := bag.Of(firstBlock.ID(), secondBlock.ID()) @@ -614,9 +611,9 @@ func RecordPollRejectTransitivelyTest(t *testing.T, factory Factory) { block1 := snowmantest.BuildChild(snowmantest.Genesis) block2 := snowmantest.BuildChild(block1) - require.NoError(sm.Add(context.Background(), block0)) - require.NoError(sm.Add(context.Background(), block1)) - require.NoError(sm.Add(context.Background(), block2)) + require.NoError(sm.Add(block0)) + require.NoError(sm.Add(block1)) + require.NoError(sm.Add(block2)) // Current graph structure: // G @@ -670,10 +667,10 @@ func RecordPollTransitivelyResetConfidenceTest(t *testing.T, factory Factory) { block2 := snowmantest.BuildChild(block1) block3 := snowmantest.BuildChild(block1) - require.NoError(sm.Add(context.Background(), block0)) - require.NoError(sm.Add(context.Background(), block1)) - require.NoError(sm.Add(context.Background(), block2)) - require.NoError(sm.Add(context.Background(), block3)) + require.NoError(sm.Add(block0)) + require.NoError(sm.Add(block1)) + require.NoError(sm.Add(block2)) + require.NoError(sm.Add(block3)) // Current graph structure: // G @@ -738,7 +735,7 @@ func RecordPollInvalidVoteTest(t *testing.T, factory Factory) { block := snowmantest.BuildChild(snowmantest.Genesis) unknownBlockID := ids.GenerateTestID() - require.NoError(sm.Add(context.Background(), block)) + require.NoError(sm.Add(block)) validVotes := bag.Of(block.ID()) require.NoError(sm.RecordPoll(context.Background(), validVotes)) @@ -781,11 +778,11 @@ func RecordPollTransitiveVotingTest(t *testing.T, factory Factory) { block3 := snowmantest.BuildChild(block0) block4 := snowmantest.BuildChild(block3) - require.NoError(sm.Add(context.Background(), block0)) - require.NoError(sm.Add(context.Background(), block1)) - require.NoError(sm.Add(context.Background(), block2)) - require.NoError(sm.Add(context.Background(), block3)) - require.NoError(sm.Add(context.Background(), block4)) + require.NoError(sm.Add(block0)) + require.NoError(sm.Add(block1)) + require.NoError(sm.Add(block2)) + require.NoError(sm.Add(block3)) + require.NoError(sm.Add(block4)) // Current graph structure: // G @@ -882,8 +879,8 @@ func RecordPollDivergedVotingWithNoConflictingBitTest(t *testing.T, factory Fact } block3 := snowmantest.BuildChild(block2) - require.NoError(sm.Add(context.Background(), block0)) - require.NoError(sm.Add(context.Background(), block1)) + require.NoError(sm.Add(block0)) + require.NoError(sm.Add(block1)) // When voting for [block0], we end up finalizing the first bit as 0. The // second bit is contested as either 0 or 1. For when the second bit is 1, @@ -896,11 +893,11 @@ func RecordPollDivergedVotingWithNoConflictingBitTest(t *testing.T, factory Fact // instance has already decided it is rejected. Snowman doesn't actually // know that though, because that is an implementation detail of the // Snowball trie that is used. - require.NoError(sm.Add(context.Background(), block2)) + require.NoError(sm.Add(block2)) // Because [block2] is effectively rejected, [block3] is also effectively // rejected. - require.NoError(sm.Add(context.Background(), block3)) + require.NoError(sm.Add(block3)) require.Equal(block0.ID(), sm.Preference()) require.Equal(choices.Processing, block0.Status(), "should not be decided yet") @@ -964,10 +961,10 @@ func RecordPollChangePreferredChainTest(t *testing.T, factory Factory) { a2Block := snowmantest.BuildChild(a1Block) b2Block := snowmantest.BuildChild(b1Block) - require.NoError(sm.Add(context.Background(), a1Block)) - require.NoError(sm.Add(context.Background(), a2Block)) - require.NoError(sm.Add(context.Background(), b1Block)) - require.NoError(sm.Add(context.Background(), b2Block)) + require.NoError(sm.Add(a1Block)) + require.NoError(sm.Add(a2Block)) + require.NoError(sm.Add(b1Block)) + require.NoError(sm.Add(b2Block)) require.Equal(a2Block.ID(), sm.Preference()) @@ -1053,10 +1050,10 @@ func LastAcceptedTest(t *testing.T, factory Factory) { require.Equal(snowmantest.GenesisID, lastAcceptedID) require.Equal(snowmantest.GenesisHeight, lastAcceptedHeight) - require.NoError(sm.Add(context.Background(), block0)) - require.NoError(sm.Add(context.Background(), block1)) - require.NoError(sm.Add(context.Background(), block1Conflict)) - require.NoError(sm.Add(context.Background(), block2)) + require.NoError(sm.Add(block0)) + require.NoError(sm.Add(block1)) + require.NoError(sm.Add(block1Conflict)) + require.NoError(sm.Add(block2)) lastAcceptedID, lastAcceptedHeight = sm.LastAccepted() require.Equal(snowmantest.GenesisID, lastAcceptedID) @@ -1195,46 +1192,6 @@ func MetricsRejectedErrorTest(t *testing.T, factory Factory) { require.Error(err) //nolint:forbidigo // error is not exported https://github.com/prometheus/client_golang/blob/main/prometheus/registry.go#L315 } -func ErrorOnInitialRejectionTest(t *testing.T, factory Factory) { - require := require.New(t) - - sm := factory.New() - - snowCtx := snowtest.Context(t, snowtest.CChainID) - ctx := snowtest.ConsensusContext(snowCtx) - params := snowball.Parameters{ - K: 1, - AlphaPreference: 1, - AlphaConfidence: 1, - Beta: 1, - ConcurrentRepolls: 1, - OptimalProcessing: 1, - MaxOutstandingItems: 1, - MaxItemProcessingTime: 1, - } - - require.NoError(sm.Initialize( - ctx, - params, - snowmantest.GenesisID, - snowmantest.GenesisHeight, - snowmantest.GenesisTimestamp, - )) - - block := &snowmantest.Block{ - TestDecidable: choices.TestDecidable{ - IDV: ids.GenerateTestID(), - RejectV: errTest, - StatusV: choices.Processing, - }, - ParentV: ids.GenerateTestID(), - HeightV: snowmantest.GenesisHeight + 1, - } - - err := sm.Add(context.Background(), block) - require.ErrorIs(err, errTest) -} - func ErrorOnAcceptTest(t *testing.T, factory Factory) { require := require.New(t) @@ -1264,7 +1221,7 @@ func ErrorOnAcceptTest(t *testing.T, factory Factory) { block := snowmantest.BuildChild(snowmantest.Genesis) block.AcceptV = errTest - require.NoError(sm.Add(context.Background(), block)) + require.NoError(sm.Add(block)) votes := bag.Of(block.ID()) err := sm.RecordPoll(context.Background(), votes) @@ -1301,8 +1258,8 @@ func ErrorOnRejectSiblingTest(t *testing.T, factory Factory) { block1 := snowmantest.BuildChild(snowmantest.Genesis) block1.RejectV = errTest - require.NoError(sm.Add(context.Background(), block0)) - require.NoError(sm.Add(context.Background(), block1)) + require.NoError(sm.Add(block0)) + require.NoError(sm.Add(block1)) votes := bag.Of(block0.ID()) err := sm.RecordPoll(context.Background(), votes) @@ -1340,9 +1297,9 @@ func ErrorOnTransitiveRejectionTest(t *testing.T, factory Factory) { block2 := snowmantest.BuildChild(block1) block2.RejectV = errTest - require.NoError(sm.Add(context.Background(), block0)) - require.NoError(sm.Add(context.Background(), block1)) - require.NoError(sm.Add(context.Background(), block2)) + require.NoError(sm.Add(block0)) + require.NoError(sm.Add(block1)) + require.NoError(sm.Add(block2)) votes := bag.Of(block0.ID()) err := sm.RecordPoll(context.Background(), votes) @@ -1408,11 +1365,8 @@ func ErrorOnAddDecidedBlockTest(t *testing.T, factory Factory) { snowmantest.GenesisTimestamp, )) - block := snowmantest.BuildChild(snowmantest.Genesis) - require.NoError(block.Accept(context.Background())) - - err := sm.Add(context.Background(), block) - require.ErrorIs(err, errDuplicateAdd) + err := sm.Add(snowmantest.Genesis) + require.ErrorIs(err, errUnknownParentBlock) } func gatherCounterGauge(t *testing.T, reg *prometheus.Registry) map[string]float64 { @@ -1458,8 +1412,8 @@ func RecordPollWithDefaultParameters(t *testing.T, factory Factory) { blk1 := snowmantest.BuildChild(snowmantest.Genesis) blk2 := snowmantest.BuildChild(snowmantest.Genesis) - require.NoError(sm.Add(context.Background(), blk1)) - require.NoError(sm.Add(context.Background(), blk2)) + require.NoError(sm.Add(blk1)) + require.NoError(sm.Add(blk2)) votes := bag.Bag[ids.ID]{} votes.AddCount(blk1.ID(), params.AlphaConfidence) @@ -1504,9 +1458,9 @@ func RecordPollRegressionCalculateInDegreeIndegreeCalculation(t *testing.T, fact blk2 := snowmantest.BuildChild(blk1) blk3 := snowmantest.BuildChild(blk2) - require.NoError(sm.Add(context.Background(), blk1)) - require.NoError(sm.Add(context.Background(), blk2)) - require.NoError(sm.Add(context.Background(), blk3)) + require.NoError(sm.Add(blk1)) + require.NoError(sm.Add(blk2)) + require.NoError(sm.Add(blk3)) votes := bag.Bag[ids.ID]{} votes.AddCount(blk2.ID(), 1) diff --git a/snow/consensus/snowman/network_test.go b/snow/consensus/snowman/network_test.go index eec42f017953..8c302ed5c7c6 100644 --- a/snow/consensus/snowman/network_test.go +++ b/snow/consensus/snowman/network_test.go @@ -91,7 +91,7 @@ func (n *Network) AddNode(t testing.TB, sm Consensus) error { VerifyV: blk.Verify(context.Background()), BytesV: blk.Bytes(), } - if err := sm.Add(context.Background(), myBlock); err != nil { + if err := sm.Add(myBlock); err != nil { return err } deps[myBlock.ID()] = myDep diff --git a/snow/consensus/snowman/topological.go b/snow/consensus/snowman/topological.go index f2ef015654c7..6956d707a4e9 100644 --- a/snow/consensus/snowman/topological.go +++ b/snow/consensus/snowman/topological.go @@ -21,6 +21,7 @@ import ( var ( errDuplicateAdd = errors.New("duplicate block add") + errUnknownParentBlock = errors.New("unknown parent block") errTooManyProcessingBlocks = errors.New("too many processing blocks") errBlockProcessingTooLong = errors.New("block processing too long") @@ -137,7 +138,7 @@ func (ts *Topological) NumProcessing() int { return len(ts.blocks) - 1 } -func (ts *Topological) Add(ctx context.Context, blk Block) error { +func (ts *Topological) Add(blk Block) error { blkID := blk.ID() height := blk.Height() ts.ctx.Log.Verbo("adding block", @@ -145,12 +146,8 @@ func (ts *Topological) Add(ctx context.Context, blk Block) error { zap.Uint64("height", height), ) - // Make sure a block is not inserted twice. This enforces the invariant that - // blocks are always added in topological order. Essentially, a block that - // is being added should never have a child that was already added. - // Additionally, this prevents any edge cases that may occur due to adding - // different blocks with the same ID. - if ts.Decided(blk) || ts.Processing(blkID) { + // Make sure a block is not inserted twice. + if ts.Processing(blkID) { return errDuplicateAdd } @@ -160,20 +157,7 @@ func (ts *Topological) Add(ctx context.Context, blk Block) error { parentID := blk.Parent() parentNode, ok := ts.blocks[parentID] if !ok { - ts.ctx.Log.Verbo("block ancestor is missing, being rejected", - zap.Stringer("blkID", blkID), - zap.Uint64("height", height), - zap.Stringer("parentID", parentID), - ) - - // If the ancestor is missing, this means the ancestor must have already - // been pruned. Therefore, the dependent should be transitively - // rejected. - if err := blk.Reject(ctx); err != nil { - return err - } - ts.metrics.Rejected(blkID, ts.pollNumber, len(blk.Bytes())) - return nil + return errUnknownParentBlock } // add the block as a child of its parent, and add the block to the tree diff --git a/snow/consensus/snowman/traced_consensus.go b/snow/consensus/snowman/traced_consensus.go index 10f49229fe16..d2d5f197a65c 100644 --- a/snow/consensus/snowman/traced_consensus.go +++ b/snow/consensus/snowman/traced_consensus.go @@ -29,16 +29,6 @@ func Trace(consensus Consensus, tracer trace.Tracer) Consensus { } } -func (c *tracedConsensus) Add(ctx context.Context, blk Block) error { - ctx, span := c.tracer.Start(ctx, "tracedConsensus.Add", oteltrace.WithAttributes( - attribute.Stringer("blkID", blk.ID()), - attribute.Int64("height", int64(blk.Height())), - )) - defer span.End() - - return c.Consensus.Add(ctx, blk) -} - func (c *tracedConsensus) RecordPoll(ctx context.Context, votes bag.Bag[ids.ID]) error { ctx, span := c.tracer.Start(ctx, "tracedConsensus.RecordPoll", oteltrace.WithAttributes( attribute.Int("numVotes", votes.Len()), diff --git a/snow/engine/snowman/transitive.go b/snow/engine/snowman/transitive.go index 9e89fedd22b2..6bc93d1e8611 100644 --- a/snow/engine/snowman/transitive.go +++ b/snow/engine/snowman/transitive.go @@ -1116,7 +1116,7 @@ func (t *Transitive) addUnverifiedBlockToConsensus( zap.Stringer("blkID", blkID), zap.Uint64("height", blkHeight), ) - return true, t.Consensus.Add(ctx, &memoryBlock{ + return true, t.Consensus.Add(&memoryBlock{ Block: blk, metrics: t.metrics, tree: t.nonVerifieds, diff --git a/snow/engine/snowman/transitive_test.go b/snow/engine/snowman/transitive_test.go index 2961b018c8ce..8c912df64a4a 100644 --- a/snow/engine/snowman/transitive_test.go +++ b/snow/engine/snowman/transitive_test.go @@ -2878,7 +2878,7 @@ func TestGetProcessingAncestor(t *testing.T) { time.Now(), )) - require.NoError(t, c.Add(context.Background(), issuedBlock)) + require.NoError(t, c.Add(issuedBlock)) nonVerifiedAncestors := ancestor.NewTree() nonVerifiedAncestors.Add(unissuedBlock.ID(), unissuedBlock.Parent())