Skip to content

Fixing a bug in eval and TestOnSwitchToUnSupportedProtocol #2220

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

Merged
tsachiherman merged 3 commits into from
Jun 3, 2021
Merged

Fixing a bug in eval and TestOnSwitchToUnSupportedProtocol #2220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
96e2ed6
Fixing a bug in block header setup for the test
algonautshant May 30, 2021
c5d619e
Terminate the loading of accounts in case of an error.
algonautshant May 30, 2021
d0bf55b
Separate test segments into separate tests. Fix one more bug
algonautshant May 30, 2021
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
188 changes: 94 additions & 94 deletions catchup/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,98 +421,96 @@ func TestServiceFetchBlocksMalformed(t *testing.T) {
//require.True(t, s.fetcherFactory.(*MockedFetcherFactory).fetcher.client.closed)
}

func TestOnSwitchToUnSupportedProtocol(t *testing.T) {
Copy link
Contributor Author

@algonautshant algonautshant May 30, 2021

Choose a reason for hiding this comment

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

No changes in TestOnSwitchToUnSupportedProtocol except for dividing it into TestOnSwitchToUnSupportedProtocol1, TestOnSwitchToUnSupportedProtocol2, TestOnSwitchToUnSupportedProtocol3 and TestOnSwitchToUnSupportedProtocol4.

t.Skip("This test is flacky and need to be fixed.")

// Test the interruption in the initial loop
// This cannot happen in practice, but is used to test the code.
{
lastRoundRemote := 5
lastRoundLocal := 0
roundWithSwitchOn := 0
local, remote := helperTestOnSwitchToUnSupportedProtocol(t, lastRoundRemote, lastRoundLocal, roundWithSwitchOn, 0)

// Last supported round is 0, but is guaranteed
// to stop after 2 rounds.

// SeedLookback is 2, which allows two parallel fetches.
// i.e. rounds 1 and 2 may be simultaneously fetched.
require.Less(t, int(local.LastRound()), 3)
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
remote.Ledger.Close()
}

// Test the interruption in "the rest" loop
{
lastRoundRemote := 10
lastRoundLocal := 7
roundWithSwitchOn := 5
local, remote := helperTestOnSwitchToUnSupportedProtocol(t, lastRoundRemote, lastRoundLocal, roundWithSwitchOn, 0)
for r := 1; r <= lastRoundLocal; r++ {
blk, err := local.Block(basics.Round(r))
require.NoError(t, err)
require.Equal(t, r, int(blk.Round()))
}
require.Equal(t, lastRoundLocal, int(local.LastRound()))
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
remote.Ledger.Close()
}

// Test the interruption with short notice (less than
// SeedLookback or the number of parallel fetches which in the
// test is the same: 2)

// This can not happen in practice, because there will be
// enough rounds for the protocol upgrade notice.
{
lastRoundRemote := 14
lastRoundLocal := 7
roundWithSwitchOn := 7
local, remote := helperTestOnSwitchToUnSupportedProtocol(t, lastRoundRemote, lastRoundLocal, roundWithSwitchOn, 0)
for r := 1; r <= lastRoundLocal; r = r + 1 {
blk, err := local.Block(basics.Round(r))
require.NoError(t, err)
require.Equal(t, r, int(blk.Round()))
}
// Since round with switch on (7) can be fetched
// Simultaneously with round 8, round 8 might also be
// fetched.
require.Less(t, int(local.LastRound()), lastRoundLocal+2)
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
remote.Ledger.Close()
}

// Test the interruption with short notice (less than
// SeedLookback or the number of parallel fetches which in the
// test is the same: 2)

// This case is a variation of the previous case. This may
// happen when the catchup service restart at the round when
// an upgrade happens.
{
lastRoundRemote := 14
lastRoundLocal := 7
roundWithSwitchOn := 7
roundsAlreadyInLocal := 8 // round 0 -> 7

local, remote := helperTestOnSwitchToUnSupportedProtocol(
t,
lastRoundRemote,
lastRoundLocal,
roundWithSwitchOn,
roundsAlreadyInLocal)

for r := 1; r <= lastRoundLocal; r = r + 1 {
blk, err := local.Block(basics.Round(r))
require.NoError(t, err)
require.Equal(t, r, int(blk.Round()))
}
// Since round with switch on (7) is already in the
// ledger, round 8 will not be fetched.
require.Equal(t, int(local.LastRound()), lastRoundLocal)
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
remote.Ledger.Close()
// Test the interruption in the initial loop
// This cannot happen in practice, but is used to test the code.
func TestOnSwitchToUnSupportedProtocol1(t *testing.T) {

lastRoundRemote := 5
lastRoundLocal := 0
roundWithSwitchOn := 0
local, remote := helperTestOnSwitchToUnSupportedProtocol(t, lastRoundRemote, lastRoundLocal, roundWithSwitchOn, 0)

// Last supported round is 0, but is guaranteed
// to stop after 2 rounds.

// SeedLookback is 2, which allows two parallel fetches.
// i.e. rounds 1 and 2 may be simultaneously fetched.
require.Less(t, int(local.LastRound()), 3)
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
remote.Ledger.Close()
}

// Test the interruption in "the rest" loop
func TestOnSwitchToUnSupportedProtocol2(t *testing.T) {

lastRoundRemote := 10
lastRoundLocal := 7
roundWithSwitchOn := 5
local, remote := helperTestOnSwitchToUnSupportedProtocol(t, lastRoundRemote, lastRoundLocal, roundWithSwitchOn, 0)
for r := 1; r <= lastRoundLocal; r++ {
blk, err := local.Block(basics.Round(r))
require.NoError(t, err)
require.Equal(t, r, int(blk.Round()))
}
require.Equal(t, lastRoundLocal, int(local.LastRound()))
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
remote.Ledger.Close()
}

// Test the interruption with short notice (less than
// SeedLookback or the number of parallel fetches which in the
// test is the same: 2)
// This can not happen in practice, because there will be
// enough rounds for the protocol upgrade notice.
func TestOnSwitchToUnSupportedProtocol3(t *testing.T) {

lastRoundRemote := 14
lastRoundLocal := 7
roundWithSwitchOn := 7
local, remote := helperTestOnSwitchToUnSupportedProtocol(t, lastRoundRemote, lastRoundLocal, roundWithSwitchOn, 0)
for r := 1; r <= lastRoundLocal; r = r + 1 {
blk, err := local.Block(basics.Round(r))
require.NoError(t, err)
require.Equal(t, r, int(blk.Round()))
}
// Since round with switch on (7) can be fetched
// Simultaneously with round 8, round 8 might also be
// fetched.
require.Less(t, int(local.LastRound()), lastRoundLocal+2)
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
remote.Ledger.Close()
}

// Test the interruption with short notice (less than
// SeedLookback or the number of parallel fetches which in the
// test is the same: 2)
// This case is a variation of the previous case. This may
// happen when the catchup service restart at the round when
// an upgrade happens.
func TestOnSwitchToUnSupportedProtocol4(t *testing.T) {

lastRoundRemote := 14
lastRoundLocal := 7
roundWithSwitchOn := 7
roundsAlreadyInLocal := 8 // round 0 -> 7

local, remote := helperTestOnSwitchToUnSupportedProtocol(
t,
lastRoundRemote,
lastRoundLocal,
roundWithSwitchOn,
roundsAlreadyInLocal)

for r := 1; r <= lastRoundLocal; r = r + 1 {
blk, err := local.Block(basics.Round(r))
require.NoError(t, err)
require.Equal(t, r, int(blk.Round()))
}
// Since round with switch on (7) is already in the
// ledger, round 8 will not be fetched.
require.Equal(t, int(local.LastRound()), lastRoundLocal)
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
remote.Ledger.Close()
}

func helperTestOnSwitchToUnSupportedProtocol(
Expand All @@ -535,14 +533,16 @@ func helperTestOnSwitchToUnSupportedProtocol(
config := defaultConfig
config.CatchupParallelBlocks = 2

remote, _, blk, err := buildTestLedger(t, bookkeeping.Block{}) //mRemote.blocks[0])
block1 := mRemote.blocks[1]
Copy link
Contributor

@brianolson brianolson Jun 1, 2021

Choose a reason for hiding this comment

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

I like this the previous way because in this way it could erroneously use block1 or mRemote.blocks[1] after buildTestLedger, but it's pass by value not pass by reference. I want to avoid trying to use the unmodified blank block and thinking we're getting something else.

Copy link
Contributor Author

@algonautshant algonautshant Jun 1, 2021

Choose a reason for hiding this comment

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

What do you mean by?

unmodified blank block

mRemote.blocks[1] will have the NextProtocolSwitchOn set according to the parameters passed to NextProtocolSwitchOn. That is why it is important to pick mRemote.blocks[1] instead of bookkeeping.Block{}.

Copy link
Contributor

@brianolson brianolson Jun 2, 2021

Choose a reason for hiding this comment

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

Your other comment said no change in behavior, just reorg, but this line is a change in behavior of what buildTestLedger() gets from Block{} to whatever testingenvWithUpgrade() may have put into mRemote.blocks[1]. Just checking that's intentional and there's no pass-by-value pass-by-reference mixup here.

Copy link
Contributor Author

@algonautshant algonautshant Jun 2, 2021

Choose a reason for hiding this comment

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

oh I see. sorry for the confusion I caused with that comment.
That comment applies exclusively to the TestOnSwitchToUnSupportedProtocol function.

remote, _, blk, err := buildTestLedger(t, block1)
if err != nil {
t.Fatal(err)
return local, remote
}
for i := 1; i < lastRoundRemote; i++ {
blk.NextProtocolSwitchOn = mRemote.blocks[i].NextProtocolSwitchOn
blk.NextProtocol = mRemote.blocks[i].NextProtocol
blk.NextProtocolSwitchOn = mRemote.blocks[i+1].NextProtocolSwitchOn
blk.NextProtocol = mRemote.blocks[i+1].NextProtocol
// Adds blk.BlockHeader.Round + 1
addBlocks(t, remote, blk, 1)
blk.BlockHeader.Round++
}
Expand Down
10 changes: 9 additions & 1 deletion ledger/eval.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1174,7 +1174,15 @@ func eval(ctx context.Context, l ledgerForEvaluator, blk bookkeeping.Block, vali
return ledgercore.StateDelta{}, err
}

paysetgroupsCh := loadAccounts(ctx, l, blk.Round()-1, paysetgroups, blk.BlockHeader.FeeSink, blk.ConsensusProtocol())
accountLoadingCtx, accountLoadingCancel := context.WithCancel(ctx)
paysetgroupsCh := loadAccounts(accountLoadingCtx, l, blk.Round()-1, paysetgroups, blk.BlockHeader.FeeSink, blk.ConsensusProtocol())
// ensure that before we exit from this method, the account loading is no longer active.
defer func() {
accountLoadingCancel()
// wait for the paysetgroupsCh to get closed.
Copy link
Contributor

@winder winder Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
// wait for the paysetgroupsCh to get closed.
// wait for the paysetgroupsCh to get closed (and remove any pending accounts).

Copy link
Contributor Author

@algonautshant algonautshant Jun 1, 2021

Choose a reason for hiding this comment

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

Will this stop waiting if the channel is emptied? I think it has to get closed.

Copy link
Contributor

@tsachiherman tsachiherman Jun 1, 2021

Choose a reason for hiding this comment

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

The channel being closed is an indication that the go-routine is done.. removing the accounts from the channel is not really interesting here, but rather a way to detect that the channel got closed ( which ensures that no database actions would be taken place ).

Copy link
Contributor

@winder winder Jun 2, 2021

Choose a reason for hiding this comment

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

You're right, "and/or" was wrong. I thought it was interesting to mention that if there were unprocessed accounts (i.e. if there was an error) they would be released.

for range paysetgroupsCh {
}
}()

var txvalidator evalTxValidator
if validate {
Expand Down