Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove rejection from consensus.Add #3084

Merged
merged 2 commits into from
Jun 6, 2024
Merged

Remove rejection from consensus.Add #3084

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf8ab41
Remove rejection from consensus.Add
StephenButtolph Jun 4, 2024
43772da
fix test
StephenButtolph Jun 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions snow/consensus/snowman/consensus.go
View file
Open in desktop
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
149 changes: 53 additions & 96 deletions snow/consensus/snowman/consensus_test.go
View file
Open in desktop
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())
Comment on lines -263 to -267
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the preference check removed? I assume the rejected check is removed because consensus now only returns an error, but does not mark the failed to add block as rejected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't have to since we don't add to consensus anymore. Previously we were adding and immediately deciding rejected, now we're just dropping it from consensus entirely and not making a decision at all.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, this was testing that the preference stayed the same as it was previously (ie. preference is still genesis after adding the new block fails) though since it dropped and rejected the block in the same place.

I agree we don't need the check in the test though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just didn't seem like a useful check anymore. I can revert that if we feel like it was important to test.

// 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 @@ -1411,7 +1368,7 @@ func ErrorOnAddDecidedBlockTest(t *testing.T, factory Factory) {
block := snowmantest.BuildChild(snowmantest.Genesis)
require.NoError(block.Accept(context.Background()))

err := sm.Add(context.Background(), block)
err := sm.Add(block)
require.ErrorIs(err, errDuplicateAdd)
}

Expand Down Expand Up @@ -1458,8 +1415,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 +1461,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
Open in desktop
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
Loading