Skip to content

Commit

Permalink
Remove rejection from consensus.Add (#3084)
Browse files Browse the repository at this point in the history
  • Loading branch information
@StephenButtolph
StephenButtolph authored Jun 6, 2024
1 parent 08ed4ac commit 657fe0b
Show file tree
Hide file tree
Showing 7 changed files with 68 additions and 136 deletions.
8 changes: 6 additions & 2 deletions snow/consensus/snowman/consensus.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
154 changes: 54 additions & 100 deletions snow/consensus/snowman/consensus_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ var (
NumProcessingTest,
AddToTailTest,
AddToNonTailTest,
AddToUnknownTest,
AddOnUnknownParentTest,
StatusOrProcessingPreviouslyAcceptedTest,
StatusOrProcessingPreviouslyRejectedTest,
StatusOrProcessingUnissuedTest,
Expand All @@ -51,7 +51,6 @@ var (
MetricsProcessingErrorTest,
MetricsAcceptedErrorTest,
MetricsRejectedErrorTest,
ErrorOnInitialRejectionTest,
ErrorOnAcceptTest,
ErrorOnRejectSiblingTest,
ErrorOnTransitiveRejectionTest,
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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))

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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())

Expand Down Expand Up @@ -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())

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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")
Expand Down Expand Up @@ -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())

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion snow/consensus/snowman/network_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit 657fe0b

Please sign in to comment.