Skip to content

Commit 7e2f656

Browse files
algonautshantalgonautshant
authored
Fixing a bug in eval and TestOnSwitchToUnSupportedProtocol (#2220)
estOnSwitchToUnSupportedProtocol had multiple segments. This test is separated into multiple tests. This PR fixes two issues: Fix for a bug in eval: in the event of an error and early termination of the eval, the loading of accounts in a separate go routine was not terminated. loadAccounts will be terminated when eval returns. TestOnSwitchToUnSupportedProtocol1 had a bug in setting the blocks for protocol switch. The first block was not getting the NextProtocolSwitchOn set to 1. Despite the bug, the test was passing most of the time, and failing some of the time. TestOnSwitchToUnSupportedProtocol3 had a bug in setting the blocks for protocol switch. AddBlocks starts adding from the next round of the passed block. NextProtocolSwitchOn was getting set one round late.
1 parent 3a4a37f commit 7e2f656

File tree

2 files changed

+103
-95
lines changed

2 files changed

+103
-95
lines changed

‎catchup/service_test.go‎

Lines changed: 94 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -421,98 +421,96 @@ func TestServiceFetchBlocksMalformed(t *testing.T) {
421421
//require.True(t, s.fetcherFactory.(*MockedFetcherFactory).fetcher.client.closed)
422422
}
423423

424-
func TestOnSwitchToUnSupportedProtocol(t *testing.T) {
425-
t.Skip("This test is flacky and need to be fixed.")
426-
427-
// Test the interruption in the initial loop
428-
// This cannot happen in practice, but is used to test the code.
429-
{
430-
lastRoundRemote := 5
431-
lastRoundLocal := 0
432-
roundWithSwitchOn := 0
433-
local, remote := helperTestOnSwitchToUnSupportedProtocol(t, lastRoundRemote, lastRoundLocal, roundWithSwitchOn, 0)
434-
435-
// Last supported round is 0, but is guaranteed
436-
// to stop after 2 rounds.
437-
438-
// SeedLookback is 2, which allows two parallel fetches.
439-
// i.e. rounds 1 and 2 may be simultaneously fetched.
440-
require.Less(t, int(local.LastRound()), 3)
441-
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
442-
remote.Ledger.Close()
443-
}
444-
445-
// Test the interruption in "the rest" loop
446-
{
447-
lastRoundRemote := 10
448-
lastRoundLocal := 7
449-
roundWithSwitchOn := 5
450-
local, remote := helperTestOnSwitchToUnSupportedProtocol(t, lastRoundRemote, lastRoundLocal, roundWithSwitchOn, 0)
451-
for r := 1; r <= lastRoundLocal; r++ {
452-
blk, err := local.Block(basics.Round(r))
453-
require.NoError(t, err)
454-
require.Equal(t, r, int(blk.Round()))
455-
}
456-
require.Equal(t, lastRoundLocal, int(local.LastRound()))
457-
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
458-
remote.Ledger.Close()
459-
}
460-
461-
// Test the interruption with short notice (less than
462-
// SeedLookback or the number of parallel fetches which in the
463-
// test is the same: 2)
464-
465-
// This can not happen in practice, because there will be
466-
// enough rounds for the protocol upgrade notice.
467-
{
468-
lastRoundRemote := 14
469-
lastRoundLocal := 7
470-
roundWithSwitchOn := 7
471-
local, remote := helperTestOnSwitchToUnSupportedProtocol(t, lastRoundRemote, lastRoundLocal, roundWithSwitchOn, 0)
472-
for r := 1; r <= lastRoundLocal; r = r + 1 {
473-
blk, err := local.Block(basics.Round(r))
474-
require.NoError(t, err)
475-
require.Equal(t, r, int(blk.Round()))
476-
}
477-
// Since round with switch on (7) can be fetched
478-
// Simultaneously with round 8, round 8 might also be
479-
// fetched.
480-
require.Less(t, int(local.LastRound()), lastRoundLocal+2)
481-
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
482-
remote.Ledger.Close()
483-
}
484-
485-
// Test the interruption with short notice (less than
486-
// SeedLookback or the number of parallel fetches which in the
487-
// test is the same: 2)
488-
489-
// This case is a variation of the previous case. This may
490-
// happen when the catchup service restart at the round when
491-
// an upgrade happens.
492-
{
493-
lastRoundRemote := 14
494-
lastRoundLocal := 7
495-
roundWithSwitchOn := 7
496-
roundsAlreadyInLocal := 8 // round 0 -> 7
497-
498-
local, remote := helperTestOnSwitchToUnSupportedProtocol(
499-
t,
500-
lastRoundRemote,
501-
lastRoundLocal,
502-
roundWithSwitchOn,
503-
roundsAlreadyInLocal)
504-
505-
for r := 1; r <= lastRoundLocal; r = r + 1 {
506-
blk, err := local.Block(basics.Round(r))
507-
require.NoError(t, err)
508-
require.Equal(t, r, int(blk.Round()))
509-
}
510-
// Since round with switch on (7) is already in the
511-
// ledger, round 8 will not be fetched.
512-
require.Equal(t, int(local.LastRound()), lastRoundLocal)
513-
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
514-
remote.Ledger.Close()
424+
// Test the interruption in the initial loop
425+
// This cannot happen in practice, but is used to test the code.
426+
func TestOnSwitchToUnSupportedProtocol1(t *testing.T) {
427+
428+
lastRoundRemote := 5
429+
lastRoundLocal := 0
430+
roundWithSwitchOn := 0
431+
local, remote := helperTestOnSwitchToUnSupportedProtocol(t, lastRoundRemote, lastRoundLocal, roundWithSwitchOn, 0)
432+
433+
// Last supported round is 0, but is guaranteed
434+
// to stop after 2 rounds.
435+
436+
// SeedLookback is 2, which allows two parallel fetches.
437+
// i.e. rounds 1 and 2 may be simultaneously fetched.
438+
require.Less(t, int(local.LastRound()), 3)
439+
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
440+
remote.Ledger.Close()
441+
}
442+
443+
// Test the interruption in "the rest" loop
444+
func TestOnSwitchToUnSupportedProtocol2(t *testing.T) {
445+
446+
lastRoundRemote := 10
447+
lastRoundLocal := 7
448+
roundWithSwitchOn := 5
449+
local, remote := helperTestOnSwitchToUnSupportedProtocol(t, lastRoundRemote, lastRoundLocal, roundWithSwitchOn, 0)
450+
for r := 1; r <= lastRoundLocal; r++ {
451+
blk, err := local.Block(basics.Round(r))
452+
require.NoError(t, err)
453+
require.Equal(t, r, int(blk.Round()))
454+
}
455+
require.Equal(t, lastRoundLocal, int(local.LastRound()))
456+
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
457+
remote.Ledger.Close()
458+
}
459+
460+
// Test the interruption with short notice (less than
461+
// SeedLookback or the number of parallel fetches which in the
462+
// test is the same: 2)
463+
// This can not happen in practice, because there will be
464+
// enough rounds for the protocol upgrade notice.
465+
func TestOnSwitchToUnSupportedProtocol3(t *testing.T) {
466+
467+
lastRoundRemote := 14
468+
lastRoundLocal := 7
469+
roundWithSwitchOn := 7
470+
local, remote := helperTestOnSwitchToUnSupportedProtocol(t, lastRoundRemote, lastRoundLocal, roundWithSwitchOn, 0)
471+
for r := 1; r <= lastRoundLocal; r = r + 1 {
472+
blk, err := local.Block(basics.Round(r))
473+
require.NoError(t, err)
474+
require.Equal(t, r, int(blk.Round()))
475+
}
476+
// Since round with switch on (7) can be fetched
477+
// Simultaneously with round 8, round 8 might also be
478+
// fetched.
479+
require.Less(t, int(local.LastRound()), lastRoundLocal+2)
480+
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
481+
remote.Ledger.Close()
482+
}
483+
484+
// Test the interruption with short notice (less than
485+
// SeedLookback or the number of parallel fetches which in the
486+
// test is the same: 2)
487+
// This case is a variation of the previous case. This may
488+
// happen when the catchup service restart at the round when
489+
// an upgrade happens.
490+
func TestOnSwitchToUnSupportedProtocol4(t *testing.T) {
491+
492+
lastRoundRemote := 14
493+
lastRoundLocal := 7
494+
roundWithSwitchOn := 7
495+
roundsAlreadyInLocal := 8 // round 0 -> 7
496+
497+
local, remote := helperTestOnSwitchToUnSupportedProtocol(
498+
t,
499+
lastRoundRemote,
500+
lastRoundLocal,
501+
roundWithSwitchOn,
502+
roundsAlreadyInLocal)
503+
504+
for r := 1; r <= lastRoundLocal; r = r + 1 {
505+
blk, err := local.Block(basics.Round(r))
506+
require.NoError(t, err)
507+
require.Equal(t, r, int(blk.Round()))
515508
}
509+
// Since round with switch on (7) is already in the
510+
// ledger, round 8 will not be fetched.
511+
require.Equal(t, int(local.LastRound()), lastRoundLocal)
512+
require.Equal(t, lastRoundRemote, int(remote.LastRound()))
513+
remote.Ledger.Close()
516514
}
517515

518516
func helperTestOnSwitchToUnSupportedProtocol(
@@ -535,14 +533,16 @@ func helperTestOnSwitchToUnSupportedProtocol(
535533
config := defaultConfig
536534
config.CatchupParallelBlocks = 2
537535

538-
remote, _, blk, err := buildTestLedger(t, bookkeeping.Block{}) //mRemote.blocks[0])
536+
block1 := mRemote.blocks[1]
537+
remote, _, blk, err := buildTestLedger(t, block1)
539538
if err != nil {
540539
t.Fatal(err)
541540
return local, remote
542541
}
543542
for i := 1; i < lastRoundRemote; i++ {
544-
blk.NextProtocolSwitchOn = mRemote.blocks[i].NextProtocolSwitchOn
545-
blk.NextProtocol = mRemote.blocks[i].NextProtocol
543+
blk.NextProtocolSwitchOn = mRemote.blocks[i+1].NextProtocolSwitchOn
544+
blk.NextProtocol = mRemote.blocks[i+1].NextProtocol
545+
// Adds blk.BlockHeader.Round + 1
546546
addBlocks(t, remote, blk, 1)
547547
blk.BlockHeader.Round++
548548
}

‎ledger/eval.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1174,7 +1174,15 @@ func eval(ctx context.Context, l ledgerForEvaluator, blk bookkeeping.Block, vali
11741174
return ledgercore.StateDelta{}, err
11751175
}
11761176

1177-
paysetgroupsCh := loadAccounts(ctx, l, blk.Round()-1, paysetgroups, blk.BlockHeader.FeeSink, blk.ConsensusProtocol())
1177+
accountLoadingCtx, accountLoadingCancel := context.WithCancel(ctx)
1178+
paysetgroupsCh := loadAccounts(accountLoadingCtx, l, blk.Round()-1, paysetgroups, blk.BlockHeader.FeeSink, blk.ConsensusProtocol())
1179+
// ensure that before we exit from this method, the account loading is no longer active.
1180+
defer func() {
1181+
accountLoadingCancel()
1182+
// wait for the paysetgroupsCh to get closed.
1183+
for range paysetgroupsCh {
1184+
}
1185+
}()
11781186

11791187
var txvalidator evalTxValidator
11801188
if validate {

0 commit comments

Comments
 (0)