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

engine/snowman: clean up some comments in "bubbleVotes" unit tests #1444

Merged
merged 15 commits into from
May 18, 2023
Merged

engine/snowman: clean up some comments in "bubbleVotes" unit tests #1444

Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f02933f
engine/snowman: clean up some comments in "bubbleVotes" unit tests
gyuho Apr 28, 2023
b07e073
Merge branch 'dev' into bubble-vote-tests-doc-update
gyuho Apr 28, 2023
69d3ff5
Merge branch 'dev' into bubble-vote-tests-doc-update
gyuho May 1, 2023
70f6b6f
Update snow/engine/snowman/transitive_test.go
gyuho May 1, 2023
87dffd5
Update snow/engine/snowman/transitive_test.go
gyuho May 1, 2023
126ee97
bubble vote testing improvements based on Stephen's feedback
gyuho May 1, 2023
7ae146c
Merge branch 'dev' into bubble-vote-tests-doc-update
gyuho May 1, 2023
6d1c272
Update snow/engine/snowman/transitive_test.go
gyuho May 3, 2023
21c22c6
Update snow/engine/snowman/transitive_test.go
gyuho May 3, 2023
78851ed
clean up more based on Stephen's feedback
gyuho May 3, 2023
744d743
Merge branch 'dev' into bubble-vote-tests-doc-update
StephenButtolph May 12, 2023
b14a2b1
Update snow/engine/snowman/transitive_test.go
gyuho May 13, 2023
1657219
Update snow/engine/snowman/transitive_test.go
gyuho May 13, 2023
67d95e4
Merge branch 'dev' into bubble-vote-tests-doc-update
StephenButtolph May 18, 2023
ad99eb9
final nit
StephenButtolph May 18, 2023
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
204 changes: 72 additions & 132 deletions snow/engine/snowman/transitive_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2710,6 +2710,7 @@ func TestEngineNonPreferredAmplification(t *testing.T) {
// B
func TestEngineBubbleVotesThroughInvalidBlock(t *testing.T) {
vdr, _, sender, vm, te, gBlk := setupDefaultConfig(t)
require := require.New(t)

// [blk1] is a child of [gBlk] and currently passes verification
blk1 := &snowman.TestBlock{
Expand Down Expand Up @@ -2742,13 +2743,14 @@ func TestEngineBubbleVotesThroughInvalidBlock(t *testing.T) {
case bytes.Equal(b, blk2.Bytes()):
return blk2, nil
default:
t.Fatalf("Unknown block bytes")
require.FailNow("Unknown block bytes")
return nil, nil
}
}

// The VM should only be able to retrieve [gBlk] from storage
// TODO GetBlockF should be updated after blocks are verified/accepted
// for now, this VM should only be able to retrieve [gBlk] from storage
// this "GetBlockF" will be updated after blocks are verified/accepted
// in the following tests
vm.GetBlockF = func(_ context.Context, blkID ids.ID) (snowman.Block, error) {
switch blkID {
case gBlk.ID():
Expand All @@ -2762,54 +2764,39 @@ func TestEngineBubbleVotesThroughInvalidBlock(t *testing.T) {
reqID := new(uint32)
sender.SendGetF = func(_ context.Context, inVdr ids.NodeID, requestID uint32, blkID ids.ID) {
*reqID = requestID
if *asked {
t.Fatalf("Asked multiple times")
}
if blkID != blk1.ID() {
t.Fatalf("Expected engine to request blk1")
}
if inVdr != vdr {
t.Fatalf("Expected engine to request blk2 from vdr")
}
require.False(*asked, "Asked multiple times")
require.Equal(blkID, blk1.ID(), "Expected engine to request blk1")
require.Equal(inVdr, vdr, "Expected engine to request blk1 from vdr")
gyuho marked this conversation as resolved.
Show resolved Hide resolved
*asked = true
}
// Receive Gossip message for [blk2] first and expect the sender to issue a Get request for
// its ancestor: [blk1].
if err := te.Put(context.Background(), vdr, constants.GossipMsgRequestID, blk2.Bytes()); err != nil {
t.Fatal(err)
}

if !*asked {
t.Fatalf("Didn't ask for missing blk1")
}
// this engine receives Gossip message for [blk2] which is "unknown" in this engine
// thus sending a Get request for its ancestor [blk1]
// (see above for expected "Get" request)
gyuho marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(te.Put(context.Background(), vdr, constants.GossipMsgRequestID, blk2.Bytes()))
require.True(*asked, "Didn't ask for missing blk1")

// Prepare to PushQuery [blk1] after our Get request is fulfilled. We should not PushQuery
// [blk2] since it currently fails verification.
queried := new(bool)
queryRequestID := new(uint32)
sender.SendPushQueryF = func(_ context.Context, inVdrs set.Set[ids.NodeID], requestID uint32, blkBytes []byte) {
if *queried {
t.Fatalf("Asked multiple times")
}
require.False(*queried, "Asked multiple times")
*queried = true

*queryRequestID = requestID
vdrSet := set.Set[ids.NodeID]{}
vdrSet.Add(vdr)
if !inVdrs.Equals(vdrSet) {
t.Fatalf("Asking wrong validator for preference")
}
if !bytes.Equal(blk1.Bytes(), blkBytes) {
t.Fatalf("Asking for wrong block")
}
}

// Answer the request, this should allow [blk1] to be issued and cause [blk2] to
// fail verification.
if err := te.Put(context.Background(), vdr, *reqID, blk1.Bytes()); err != nil {
t.Fatal(err)
require.Equal(inVdrs, vdrSet, "Asking wrong validator for preference")
require.Equal(blk1.Bytes(), blkBytes, "Asking for wrong block")
}
// this engine now answers the "Get" request, and this should allow [blk1] to be issued
// and cause [blk2] to fail verification.
// (see above for expected "PushQuery" request)
gyuho marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(te.Put(context.Background(), vdr, *reqID, blk1.Bytes()))
require.True(*asked, "Didn't ask for preferences regarding blk1")
require.True(*queried, "Didn't query the newly issued blk1")

// now blk1 is verified, vm can return it
// now [blk1] is verified, vm can return it
vm.GetBlockF = func(_ context.Context, blkID ids.ID) (snowman.Block, error) {
switch blkID {
case gBlk.ID():
Expand All @@ -2821,52 +2808,46 @@ func TestEngineBubbleVotesThroughInvalidBlock(t *testing.T) {
}
}

if !*queried {
t.Fatalf("Didn't ask for preferences regarding blk1")
}

sendReqID := new(uint32)
reqVdr := new(ids.NodeID)
// Update GetF to produce a more detailed error message in the case that receiving a Chits
// message causes us to send another Get request.
sender.SendGetF = func(_ context.Context, inVdr ids.NodeID, requestID uint32, blkID ids.ID) {
switch blkID {
case blk1.ID():
t.Fatal("Unexpectedly sent a Get request for blk1")
require.FailNow("Unexpectedly sent a Get request for blk1")
case blk2.ID():
*sendReqID = requestID
*reqVdr = inVdr
return
default:
t.Fatal("Unexpectedly sent a Get request for unknown block")
require.FailNow("Unexpectedly sent a Get request for unknown block")
}
gyuho marked this conversation as resolved.
Show resolved Hide resolved
}

// this VM expects to receive "Chits" in response to the "PullQuery"
// not to send out a "PullQuery"
sender.SendPullQueryF = func(_ context.Context, _ set.Set[ids.NodeID], _ uint32, blkID ids.ID) {
switch blkID {
case blk1.ID():
t.Fatal("Unexpectedly sent a PullQuery request for blk1")
require.FailNow("Unexpectedly sent a PullQuery request for blk1")
case blk2.ID():
t.Fatal("Unexpectedly sent a PullQuery request for blk2")
require.FailNow("Unexpectedly sent a PullQuery request for blk2")
default:
t.Fatal("Unexpectedly sent a PullQuery request for unknown block")
require.FailNow("Unexpectedly sent a PullQuery request for unknown block")
}
}
gyuho marked this conversation as resolved.
Show resolved Hide resolved

// Now we are expecting a Chits message, and we receive it for blk2 instead of blk1
// The votes should be bubbled through blk2 despite the fact that it is failing verification.
if err := te.Chits(context.Background(), vdr, *queryRequestID, []ids.ID{blk2.ID()}, nil); err != nil {
t.Fatal(err)
}
// Now we are expecting a Chits message, and we receive it for [blk2] instead of [blk1].
// This will cause the node to again request [blk2].
require.NoError(te.Chits(context.Background(), vdr, *queryRequestID, []ids.ID{blk2.ID()}, nil))

if err := te.Put(context.Background(), *reqVdr, *sendReqID, blk2.Bytes()); err != nil {
t.Fatal(err)
}
// The votes should be bubbled through [blk2] despite the fact that it is failing verification.
require.NoError(te.Put(context.Background(), *reqVdr, *sendReqID, blk2.Bytes()))

// The vote should be bubbled through [blk2], such that [blk1] gets marked as Accepted.
if blk1.Status() != choices.Accepted {
t.Fatalf("Expected blk1 to be Accepted, but found status: %s", blk1.Status())
}
require.Equalf(blk1.Status(), choices.Accepted, "Expected blk1 to be Accepted, but found status: %s", blk1.Status())
require.Equal(blk2.Status(), choices.Processing)

// Now that [blk1] has been marked as Accepted, [blk2] can pass verification.
blk2.VerifyV = nil
Expand All @@ -2885,37 +2866,21 @@ func TestEngineBubbleVotesThroughInvalidBlock(t *testing.T) {
*queried = false
// Prepare to PushQuery [blk2] after receiving a Gossip message with [blk2].
sender.SendPushQueryF = func(_ context.Context, inVdrs set.Set[ids.NodeID], requestID uint32, blkBytes []byte) {
if *queried {
t.Fatalf("Asked multiple times")
}
require.False(*queried, "Asked multiple times")
*queried = true
*queryRequestID = requestID
vdrSet := set.Set[ids.NodeID]{}
vdrSet.Add(vdr)
if !inVdrs.Equals(vdrSet) {
t.Fatalf("Asking wrong validator for preference")
}
if !bytes.Equal(blk2.Bytes(), blkBytes) {
t.Fatalf("Asking for wrong block")
}
require.Equal(inVdrs, vdrSet, "Asking wrong validator for preference")
require.Equal(blk2.Bytes(), blkBytes, "Asking for wrong block")
}
// Expect that the Engine will send a PushQuery after receiving this Gossip message for [blk2].
if err := te.Put(context.Background(), vdr, constants.GossipMsgRequestID, blk2.Bytes()); err != nil {
t.Fatal(err)
}

if !*queried {
t.Fatalf("Didn't ask for preferences regarding blk2")
}
require.NoError(te.Put(context.Background(), vdr, constants.GossipMsgRequestID, blk2.Bytes()))
require.True(*queried, "Didn't ask for preferences regarding blk2")

// After a single vote for [blk2], it should be marked as accepted.
if err := te.Chits(context.Background(), vdr, *queryRequestID, []ids.ID{blk2.ID()}, nil); err != nil {
t.Fatal(err)
}

if blk2.Status() != choices.Accepted {
t.Fatalf("Expected blk2 to be Accepted, but found status: %s", blk2.Status())
}
require.NoError(te.Chits(context.Background(), vdr, *queryRequestID, []ids.ID{blk2.ID()}, nil))
require.Equalf(blk2.Status(), choices.Accepted, "Expected blk2 to be Accepted, but found status: %s", blk2.Status())
gyuho marked this conversation as resolved.
Show resolved Hide resolved
}

// Test that in the following scenario, if block B fails verification, votes
Expand All @@ -2933,6 +2898,7 @@ func TestEngineBubbleVotesThroughInvalidBlock(t *testing.T) {
// C
func TestEngineBubbleVotesThroughInvalidChain(t *testing.T) {
vdr, _, sender, vm, te, gBlk := setupDefaultConfig(t)
require := require.New(t)

// [blk1] is a child of [gBlk] and currently passes verification
blk1 := &snowman.TestBlock{
Expand Down Expand Up @@ -2978,7 +2944,7 @@ func TestEngineBubbleVotesThroughInvalidChain(t *testing.T) {
case bytes.Equal(b, blk3.Bytes()):
return blk3, nil
default:
t.Fatalf("Unknown block bytes")
require.FailNow("Unknown block bytes")
return nil, nil
}
}
Expand All @@ -2999,56 +2965,34 @@ func TestEngineBubbleVotesThroughInvalidChain(t *testing.T) {
reqID := new(uint32)
sender.SendGetF = func(_ context.Context, inVdr ids.NodeID, requestID uint32, blkID ids.ID) {
*reqID = requestID
if *asked {
t.Fatalf("Asked multiple times")
}
if blkID != blk2.ID() {
t.Fatalf("Expected engine to request blk2")
}
if inVdr != vdr {
t.Fatalf("Expected engine to request blk2 from vdr")
}
require.False(*asked, "Asked multiple times")
require.Equal(blkID, blk2.ID(), "Expected engine to request blk2")
require.Equal(inVdr, vdr, "Expected engine to request blk2 from vdr")
*asked = true
}
// Receive Gossip message for [blk3] first and expect the sender to issue a
// Get request for its ancestor: [blk2].
if err := te.Put(context.Background(), vdr, constants.GossipMsgRequestID, blk3.Bytes()); err != nil {
t.Fatal(err)
}

if !*asked {
t.Fatalf("Didn't ask for missing blk2")
}
require.NoError(te.Put(context.Background(), vdr, constants.GossipMsgRequestID, blk3.Bytes()))
require.True(*asked, "Didn't ask for preferences regarding blk2")

// Prepare to PushQuery [blk1] after our request for [blk2] is fulfilled.
// We should not PushQuery [blk2] since it currently fails verification.
// We should not PushQuery [blk3] because [blk2] wasn't issued.
queried := new(bool)
queryRequestID := new(uint32)
sender.SendPushQueryF = func(_ context.Context, inVdrs set.Set[ids.NodeID], requestID uint32, blkBytes []byte) {
if *queried {
t.Fatalf("Asked multiple times")
}
require.False(*queried, "Asked multiple times")
*queried = true
*queryRequestID = requestID
vdrSet := set.Set[ids.NodeID]{}
vdrSet.Add(vdr)
if !inVdrs.Equals(vdrSet) {
t.Fatalf("Asking wrong validator for preference")
}
if !bytes.Equal(blk1.Bytes(), blkBytes) {
t.Fatalf("Asking for wrong block")
}
require.Equal(inVdrs, vdrSet, "Asking wrong validator for preference")
require.Equal(blk1.Bytes(), blkBytes, "Asking for wrong block")
}

// Answer the request, this should result in [blk1] being issued as well.
if err := te.Put(context.Background(), vdr, *reqID, blk2.Bytes()); err != nil {
t.Fatal(err)
}

if !*queried {
t.Fatalf("Didn't ask for preferences regarding blk1")
}
require.NoError(te.Put(context.Background(), vdr, *reqID, blk2.Bytes()))
require.True(*queried, "Didn't ask for preferences regarding blk1")

sendReqID := new(uint32)
reqVdr := new(ids.NodeID)
Expand All @@ -3057,7 +3001,7 @@ func TestEngineBubbleVotesThroughInvalidChain(t *testing.T) {
sender.SendGetF = func(_ context.Context, inVdr ids.NodeID, requestID uint32, blkID ids.ID) {
switch blkID {
case blk1.ID():
t.Fatal("Unexpectedly sent a Get request for blk1")
require.FailNow("Unexpectedly sent a Get request for blk1")
case blk2.ID():
t.Logf("sending get for blk2 with %d", requestID)
*sendReqID = requestID
Expand All @@ -3069,41 +3013,37 @@ func TestEngineBubbleVotesThroughInvalidChain(t *testing.T) {
*reqVdr = inVdr
return
default:
t.Fatal("Unexpectedly sent a Get request for unknown block")
require.FailNow("Unexpectedly sent a Get request for unknown block")
}
}

// this VM expects to receive "Chits" in response to the "PullQuery"
// not to send out a "PullQuery"
sender.SendPullQueryF = func(_ context.Context, _ set.Set[ids.NodeID], _ uint32, blkID ids.ID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yikes this is jank - we just shouldn't set SendPullQueryF here right? if the var is nil then it will already error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the var is nil then it will already error

You mean if SendPullQueryF is nil, then it will panic for the wrong call, correct?

Yes, I think it's better just remove this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It will actually error, not panic: https://github.com/ava-labs/avalanchego/blob/master/snow/engine/common/test_sender.go#L259

switch blkID {
case blk1.ID():
t.Fatal("Unexpectedly sent a PullQuery request for blk1")
require.FailNow("Unexpectedly sent a PullQuery request for blk1")
case blk2.ID():
t.Fatal("Unexpectedly sent a PullQuery request for blk2")
require.FailNow("Unexpectedly sent a PullQuery request for blk2")
case blk3.ID():
t.Fatal("Unexpectedly sent a PullQuery request for blk3")
require.FailNow("Unexpectedly sent a PullQuery request for blk3")
default:
t.Fatal("Unexpectedly sent a PullQuery request for unknown block")
require.FailNow("Unexpectedly sent a PullQuery request for unknown block")
}
}

// Now we are expecting a Chits message, and we receive it for [blk3]
// instead of blk1. This will cause the node to again request [blk3].
if err := te.Chits(context.Background(), vdr, *queryRequestID, []ids.ID{blk3.ID()}, nil); err != nil {
t.Fatal(err)
}
// Now we are expecting a Chits message, and we receive it for [blk3] instead of [blk1].
// This will cause the node to again request [blk3].
require.NoError(te.Chits(context.Background(), vdr, *queryRequestID, []ids.ID{blk3.ID()}, nil))

// Drop the re-request for blk3 to cause the poll to termindate. The votes
// should be bubbled through blk3 despite the fact that it hasn't been
// Drop the re-request for [blk3] to cause the poll to termindate. The votes
// should be bubbled through [blk3] despite the fact that it hasn't been
// issued.
if err := te.GetFailed(context.Background(), *reqVdr, *sendReqID); err != nil {
t.Fatal(err)
}
require.NoError(te.GetFailed(context.Background(), *reqVdr, *sendReqID))

// The vote should be bubbled through [blk3] and [blk2] such that [blk1]
// gets marked as Accepted.
if blk1.Status() != choices.Accepted {
t.Fatalf("Expected blk1 to be Accepted, but found status: %s", blk1.Status())
}
require.Equalf(blk1.Status(), choices.Accepted, "Expected blk1 to be Accepted, but found status: %s", blk1.Status())
}

func TestMixedQueryNumPushSet(t *testing.T) {
Expand Down