From 95cca6f29c299f0583211f6a104298bd477423a6 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Sun, 19 Apr 2020 13:48:36 +0100 Subject: [PATCH 01/24] build: Reduce go test timeout to 30 seconds The timeout was previously the go test default, 10 minutes --- scripts/build_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/build_test.sh b/scripts/build_test.sh index f627613..d12bbf9 100755 --- a/scripts/build_test.sh +++ b/scripts/build_test.sh @@ -5,4 +5,4 @@ SRC_DIR="$(dirname "${BASH_SOURCE[0]}")" source "$SRC_DIR/env.sh" -go test -race -coverprofile=coverage.out -covermode=atomic ./... +go test -race -timeout="30s" -coverprofile="coverage.out" -covermode="atomic" ./... From f89bcdc40cf84ea564332c866039d5914ad746f0 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Sun, 19 Apr 2020 13:49:53 +0100 Subject: [PATCH 02/24] vms: Use GenesisVM to initialise avm.Service tests --- vms/avm/service_test.go | 26 +------------------------- 1 file changed, 1 insertion(+), 25 deletions(-) diff --git a/vms/avm/service_test.go b/vms/avm/service_test.go index dff4c3e..b42f8e5 100644 --- a/vms/avm/service_test.go +++ b/vms/avm/service_test.go @@ -9,36 +9,12 @@ import ( "github.com/ava-labs/gecko/snow/choices" - "github.com/ava-labs/gecko/database/memdb" "github.com/ava-labs/gecko/ids" - "github.com/ava-labs/gecko/snow/engine/common" "github.com/ava-labs/gecko/utils/formatting" - "github.com/ava-labs/gecko/vms/secp256k1fx" ) func setup(t *testing.T) ([]byte, *VM, *Service) { - genesisBytes := BuildGenesisTest(t) - - ctx.Lock.Lock() - - // This VM initilialzation is very similar to that done by GenesisVM(). - // However replacing the body of this function, with a call to GenesisVM - // causes a timeout while executing the test suite. - // https://github.com/ava-labs/gecko/pull/59#pullrequestreview-392478636 - vm := &VM{} - err := vm.Initialize( - ctx, - memdb.New(), - genesisBytes, - make(chan common.Message, 1), - []*common.Fx{&common.Fx{ - ID: ids.Empty, - Fx: &secp256k1fx.Fx{}, - }}, - ) - if err != nil { - t.Fatal(err) - } + genesisBytes, _, vm := GenesisVM(t) s := &Service{vm: vm} return genesisBytes, vm, s } From de3fd2915bccca1f1fa5c44fc407b3c55bb3a5e8 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Sun, 19 Apr 2020 13:54:56 +0100 Subject: [PATCH 03/24] vms: Fix deadlock when stopping timers during avm.VM.Shutdown() refs #66 --- vms/avm/vm.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/vms/avm/vm.go b/vms/avm/vm.go index c8b33f1..fd68e44 100644 --- a/vms/avm/vm.go +++ b/vms/avm/vm.go @@ -202,7 +202,12 @@ func (vm *VM) Shutdown() { return } + // There is a potential deadlock if the timer is about to execute a timeout. + // So, the lock must be released before stopping the timer. + vm.ctx.Lock.Unlock() vm.timer.Stop() + vm.ctx.Lock.Lock() + if err := vm.baseDB.Close(); err != nil { vm.ctx.Log.Error("Closing the database failed with %s", err) } From 2eb8add469da2cf0f0354cf36d52f296f6d7b889 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Sun, 19 Apr 2020 14:21:28 +0100 Subject: [PATCH 04/24] vms: Ensure all avm.VM instances in tests get shutdown I suspect these could be simplified/made more uniform, but I don't think I fully understand the locking semantics. --- vms/avm/base_tx_test.go | 11 +++++++++++ vms/avm/export_tx_test.go | 2 ++ vms/avm/import_tx_test.go | 2 ++ vms/avm/prefixed_state_test.go | 6 ++++++ vms/avm/state_test.go | 4 ++++ vms/avm/vm_test.go | 7 +++++++ 6 files changed, 32 insertions(+) diff --git a/vms/avm/base_tx_test.go b/vms/avm/base_tx_test.go index e236230..78ebfa6 100644 --- a/vms/avm/base_tx_test.go +++ b/vms/avm/base_tx_test.go @@ -624,6 +624,7 @@ func TestBaseTxSyntacticVerifyUninitialized(t *testing.T) { func TestBaseTxSemanticVerify(t *testing.T) { genesisBytes, _, vm := GenesisVM(t) defer ctx.Lock.Unlock() + defer vm.Shutdown() genesisTx := GetFirstTxFromGenesisTest(genesisBytes, t) @@ -688,6 +689,7 @@ func TestBaseTxSemanticVerify(t *testing.T) { func TestBaseTxSemanticVerifyUnknownFx(t *testing.T) { genesisBytes, _, vm := GenesisVM(t) defer ctx.Lock.Unlock() + defer vm.Shutdown() vm.codec.RegisterType(&ava.TestVerifiable{}) @@ -737,6 +739,7 @@ func TestBaseTxSemanticVerifyUnknownFx(t *testing.T) { func TestBaseTxSemanticVerifyWrongAssetID(t *testing.T) { genesisBytes, _, vm := GenesisVM(t) defer ctx.Lock.Unlock() + defer vm.Shutdown() vm.codec.RegisterType(&ava.TestVerifiable{}) @@ -809,6 +812,7 @@ func TestBaseTxSemanticVerifyUnauthorizedFx(t *testing.T) { defer ctx.Lock.Unlock() vm := &VM{} + defer vm.Shutdown() err := vm.Initialize( ctx, memdb.New(), @@ -894,6 +898,7 @@ func TestBaseTxSemanticVerifyUnauthorizedFx(t *testing.T) { func TestBaseTxSemanticVerifyInvalidSignature(t *testing.T) { genesisBytes, _, vm := GenesisVM(t) defer ctx.Lock.Unlock() + defer vm.Shutdown() genesisTx := GetFirstTxFromGenesisTest(genesisBytes, t) @@ -945,6 +950,7 @@ func TestBaseTxSemanticVerifyInvalidSignature(t *testing.T) { func TestBaseTxSemanticVerifyMissingUTXO(t *testing.T) { genesisBytes, _, vm := GenesisVM(t) defer ctx.Lock.Unlock() + defer vm.Shutdown() genesisTx := GetFirstTxFromGenesisTest(genesisBytes, t) @@ -1009,6 +1015,7 @@ func TestBaseTxSemanticVerifyMissingUTXO(t *testing.T) { func TestBaseTxSemanticVerifyInvalidUTXO(t *testing.T) { genesisBytes, _, vm := GenesisVM(t) defer ctx.Lock.Unlock() + defer vm.Shutdown() genesisTx := GetFirstTxFromGenesisTest(genesisBytes, t) @@ -1141,6 +1148,7 @@ func TestBaseTxSemanticVerifyPendingInvalidUTXO(t *testing.T) { ctx.Lock.Lock() defer ctx.Lock.Unlock() + defer vm.Shutdown() vm.PendingTxs() @@ -1272,6 +1280,7 @@ func TestBaseTxSemanticVerifyPendingWrongAssetID(t *testing.T) { ctx.Lock.Lock() defer ctx.Lock.Unlock() + defer vm.Shutdown() vm.PendingTxs() @@ -1437,6 +1446,7 @@ func TestBaseTxSemanticVerifyPendingUnauthorizedFx(t *testing.T) { ctx.Lock.Lock() defer ctx.Lock.Unlock() + defer vm.Shutdown() vm.PendingTxs() @@ -1586,6 +1596,7 @@ func TestBaseTxSemanticVerifyPendingInvalidSignature(t *testing.T) { ctx.Lock.Lock() defer ctx.Lock.Unlock() + defer vm.Shutdown() vm.PendingTxs() diff --git a/vms/avm/export_tx_test.go b/vms/avm/export_tx_test.go index 98df9b0..f3cf508 100644 --- a/vms/avm/export_tx_test.go +++ b/vms/avm/export_tx_test.go @@ -217,6 +217,7 @@ func TestIssueExportTx(t *testing.T) { ctx.Lock.Lock() defer ctx.Lock.Unlock() + defer vm.Shutdown() txs := vm.PendingTxs() if len(txs) != 1 { @@ -350,6 +351,7 @@ func TestClearForceAcceptedExportTx(t *testing.T) { ctx.Lock.Lock() defer ctx.Lock.Unlock() + defer vm.Shutdown() txs := vm.PendingTxs() if len(txs) != 1 { diff --git a/vms/avm/import_tx_test.go b/vms/avm/import_tx_test.go index e0f5605..4f7ac43 100644 --- a/vms/avm/import_tx_test.go +++ b/vms/avm/import_tx_test.go @@ -222,6 +222,7 @@ func TestIssueImportTx(t *testing.T) { } ctx.Lock.Unlock() + defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() msg := <-issuer if msg != common.PendingTxs { @@ -265,6 +266,7 @@ func TestForceAcceptImportTx(t *testing.T) { defer ctx.Lock.Unlock() vm := &VM{platform: platformID} + defer vm.Shutdown() err := vm.Initialize( ctx, memdb.New(), diff --git a/vms/avm/prefixed_state_test.go b/vms/avm/prefixed_state_test.go index 1d790d5..f97757e 100644 --- a/vms/avm/prefixed_state_test.go +++ b/vms/avm/prefixed_state_test.go @@ -18,6 +18,8 @@ import ( func TestPrefixedSetsAndGets(t *testing.T) { _, _, vm := GenesisVM(t) ctx.Lock.Unlock() + defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() + state := vm.state vm.codec.RegisterType(&ava.TestVerifiable{}) @@ -113,6 +115,8 @@ func TestPrefixedSetsAndGets(t *testing.T) { func TestPrefixedFundingNoAddresses(t *testing.T) { _, _, vm := GenesisVM(t) ctx.Lock.Unlock() + defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() + state := vm.state vm.codec.RegisterType(&ava.TestVerifiable{}) @@ -137,6 +141,8 @@ func TestPrefixedFundingNoAddresses(t *testing.T) { func TestPrefixedFundingAddresses(t *testing.T) { _, _, vm := GenesisVM(t) ctx.Lock.Unlock() + defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() + state := vm.state vm.codec.RegisterType(&testAddressable{}) diff --git a/vms/avm/state_test.go b/vms/avm/state_test.go index aac77c1..4b8aa05 100644 --- a/vms/avm/state_test.go +++ b/vms/avm/state_test.go @@ -16,6 +16,7 @@ import ( func TestStateIDs(t *testing.T) { _, _, vm := GenesisVM(t) + defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() ctx.Lock.Unlock() state := vm.state.state @@ -126,6 +127,7 @@ func TestStateIDs(t *testing.T) { func TestStateStatuses(t *testing.T) { _, _, vm := GenesisVM(t) + defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() ctx.Lock.Unlock() state := vm.state.state @@ -175,6 +177,7 @@ func TestStateStatuses(t *testing.T) { func TestStateUTXOs(t *testing.T) { _, _, vm := GenesisVM(t) + defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() ctx.Lock.Unlock() state := vm.state.state @@ -246,6 +249,7 @@ func TestStateUTXOs(t *testing.T) { func TestStateTXs(t *testing.T) { _, _, vm := GenesisVM(t) + defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() ctx.Lock.Unlock() state := vm.state.state diff --git a/vms/avm/vm_test.go b/vms/avm/vm_test.go index cfd09e8..03f692f 100644 --- a/vms/avm/vm_test.go +++ b/vms/avm/vm_test.go @@ -396,6 +396,7 @@ func TestInvalidGenesis(t *testing.T) { defer ctx.Lock.Unlock() vm := &VM{} + defer vm.Shutdown() err := vm.Initialize( /*context=*/ ctx, /*db=*/ memdb.New(), @@ -415,6 +416,7 @@ func TestInvalidFx(t *testing.T) { defer ctx.Lock.Unlock() vm := &VM{} + defer vm.Shutdown() err := vm.Initialize( /*context=*/ ctx, /*db=*/ memdb.New(), @@ -436,6 +438,7 @@ func TestFxInitializationFailure(t *testing.T) { defer ctx.Lock.Unlock() vm := &VM{} + defer vm.Shutdown() err := vm.Initialize( /*context=*/ ctx, /*db=*/ memdb.New(), @@ -457,6 +460,7 @@ func (tx *testTxBytes) UnsignedBytes() []byte { return tx.unsignedBytes } func TestIssueTx(t *testing.T) { genesisBytes, issuer, vm := GenesisVM(t) + defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() newTx := NewTx(t, genesisBytes, vm) @@ -503,6 +507,7 @@ func TestGenesisGetUTXOs(t *testing.T) { // transaction should be issued successfully. func TestIssueDependentTx(t *testing.T) { genesisBytes, issuer, vm := GenesisVM(t) + defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() genesisTx := GetFirstTxFromGenesisTest(genesisBytes, t) @@ -638,6 +643,7 @@ func TestIssueNFT(t *testing.T) { defer ctx.Lock.Unlock() vm := &VM{} + defer vm.Shutdown() err := vm.Initialize( ctx, memdb.New(), @@ -796,6 +802,7 @@ func TestIssueProperty(t *testing.T) { defer ctx.Lock.Unlock() vm := &VM{} + defer vm.Shutdown() err := vm.Initialize( ctx, memdb.New(), From 826a1cc06d9e16f6a2c5f6cc111be61cbbb9c812 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Sun, 19 Apr 2020 14:40:30 +0100 Subject: [PATCH 05/24] vms: Note potential FIXMEs in avm.VM tests --- vms/avm/import_tx_test.go | 1 + vms/avm/prefixed_state_test.go | 8 ++++++++ vms/avm/state_test.go | 12 ++++++++++++ vms/avm/vm_test.go | 2 ++ 4 files changed, 23 insertions(+) diff --git a/vms/avm/import_tx_test.go b/vms/avm/import_tx_test.go index 4f7ac43..2228977 100644 --- a/vms/avm/import_tx_test.go +++ b/vms/avm/import_tx_test.go @@ -229,6 +229,7 @@ func TestIssueImportTx(t *testing.T) { t.Fatalf("Wrong message") } + // FIXME?: Is it safe to call vm.PendingTXs() called without the lock? txs := vm.PendingTxs() if len(txs) != 1 { t.Fatalf("Should have returned %d tx(s)", 1) diff --git a/vms/avm/prefixed_state_test.go b/vms/avm/prefixed_state_test.go index f97757e..ad160e9 100644 --- a/vms/avm/prefixed_state_test.go +++ b/vms/avm/prefixed_state_test.go @@ -20,8 +20,10 @@ func TestPrefixedSetsAndGets(t *testing.T) { ctx.Lock.Unlock() defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() + // FIXME? is it safe to access vm.state.state without the lock? state := vm.state + // FIXME? is it safe to call vm.codec.RegisterType() without the lock? vm.codec.RegisterType(&ava.TestVerifiable{}) utxo := &ava.UTXO{ @@ -53,6 +55,7 @@ func TestPrefixedSetsAndGets(t *testing.T) { }}, }} + // FIXME? Is it safe to call vm.codec.Marshal() without the lock? unsignedBytes, err := vm.codec.Marshal(tx.UnsignedTx) if err != nil { t.Fatal(err) @@ -72,6 +75,7 @@ func TestPrefixedSetsAndGets(t *testing.T) { }, }) + // FIXME? Is it safe to call vm.codec.Marshal() without the lock? b, err := vm.codec.Marshal(tx) if err != nil { t.Fatal(err) @@ -117,8 +121,10 @@ func TestPrefixedFundingNoAddresses(t *testing.T) { ctx.Lock.Unlock() defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() + // FIXME? is it safe to access vm.state.state without the lock? state := vm.state + // FIXME? is it safe to call vm.codec.RegisterType() without the lock? vm.codec.RegisterType(&ava.TestVerifiable{}) utxo := &ava.UTXO{ @@ -143,8 +149,10 @@ func TestPrefixedFundingAddresses(t *testing.T) { ctx.Lock.Unlock() defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() + // FIXME? is it safe to access vm.state.state without the lock? state := vm.state + // FIXME? is it safe to call vm.codec.RegisterType() without the lock? vm.codec.RegisterType(&testAddressable{}) utxo := &ava.UTXO{ diff --git a/vms/avm/state_test.go b/vms/avm/state_test.go index 4b8aa05..335292c 100644 --- a/vms/avm/state_test.go +++ b/vms/avm/state_test.go @@ -18,6 +18,8 @@ func TestStateIDs(t *testing.T) { _, _, vm := GenesisVM(t) defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() ctx.Lock.Unlock() + + // FIXME? is it safe to access vm.state.state without the lock? state := vm.state.state id0 := ids.NewID([32]byte{0xff, 0}) @@ -129,6 +131,8 @@ func TestStateStatuses(t *testing.T) { _, _, vm := GenesisVM(t) defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() ctx.Lock.Unlock() + + // FIXME? is it safe to access vm.state.state without the lock? state := vm.state.state if _, err := state.Status(ids.Empty); err == nil { @@ -179,8 +183,11 @@ func TestStateUTXOs(t *testing.T) { _, _, vm := GenesisVM(t) defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() ctx.Lock.Unlock() + + // FIXME? is it safe to access vm.state.state without the lock? state := vm.state.state + // FIXME? Is it safe to call vm.codec.RegisterType() without the lock? vm.codec.RegisterType(&ava.TestVerifiable{}) if _, err := state.UTXO(ids.Empty); err == nil { @@ -251,8 +258,11 @@ func TestStateTXs(t *testing.T) { _, _, vm := GenesisVM(t) defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() ctx.Lock.Unlock() + + // FIXME? is it safe to access vm.state.state without the lock? state := vm.state.state + // FIXME? Is it safe to call vm.codec.RegisterType() without the lock? vm.codec.RegisterType(&ava.TestTransferable{}) if _, err := state.Tx(ids.Empty); err == nil { @@ -279,6 +289,7 @@ func TestStateTXs(t *testing.T) { }}, }} + // FIXME? Is it safe to call vm.codec.Marshal() without the lock? unsignedBytes, err := vm.codec.Marshal(tx.UnsignedTx) if err != nil { t.Fatal(err) @@ -298,6 +309,7 @@ func TestStateTXs(t *testing.T) { }, }) + // FIXME? Is it safe to call vm.codec.Marshal() without the lock? b, err := vm.codec.Marshal(tx) if err != nil { t.Fatal(err) diff --git a/vms/avm/vm_test.go b/vms/avm/vm_test.go index 03f692f..36e0aa0 100644 --- a/vms/avm/vm_test.go +++ b/vms/avm/vm_test.go @@ -478,6 +478,7 @@ func TestIssueTx(t *testing.T) { t.Fatalf("Wrong message") } + // FIXME? vm.PendingTxs called after lock released. if txs := vm.PendingTxs(); len(txs) != 1 { t.Fatalf("Should have returned %d tx(s)", 1) } @@ -628,6 +629,7 @@ func TestIssueDependentTx(t *testing.T) { t.Fatalf("Wrong message") } + // FIXME? vm.PendingTxs called after lock released. if txs := vm.PendingTxs(); len(txs) != 2 { t.Fatalf("Should have returned %d tx(s)", 2) } From e3844c1d6c87482781f2ad5431aa2262bd2ecff4 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 21 Apr 2020 01:26:37 +0100 Subject: [PATCH 06/24] vms: Fix deadlock when stopping timers during platformvm.VM.Shutdown() refs #66 --- vms/platformvm/vm.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/vms/platformvm/vm.go b/vms/platformvm/vm.go index 3e4bf78..1a0e68b 100644 --- a/vms/platformvm/vm.go +++ b/vms/platformvm/vm.go @@ -386,7 +386,12 @@ func (vm *VM) Shutdown() { return } + // There is a potential deadlock if the timer is about to execute a timeout. + // So, the lock must be released before stopping the timer. + vm.Ctx.Lock.Unlock() vm.timer.Stop() + vm.Ctx.Lock.Lock() + if err := vm.DB.Close(); err != nil { vm.Ctx.Log.Error("Closing the database failed with %s", err) } From fb38cc8f2585a8667941423d352c30e2848aa273 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 21 Apr 2020 01:29:41 +0100 Subject: [PATCH 07/24] vms: Ensure all platform.VM instances in tests get shutdown --- .../add_default_subnet_delegator_tx_test.go | 2 ++ .../add_default_subnet_validator_tx_test.go | 2 ++ .../add_nondefault_subnet_validator_tx_test.go | 3 +++ vms/platformvm/advance_time_tx_test.go | 7 +++++++ vms/platformvm/create_chain_tx_test.go | 6 ++++++ vms/platformvm/event_heap_test.go | 8 ++++++++ vms/platformvm/reward_validator_tx_test.go | 5 +++++ vms/platformvm/vm_test.go | 18 ++++++++++++++++++ 8 files changed, 51 insertions(+) diff --git a/vms/platformvm/add_default_subnet_delegator_tx_test.go b/vms/platformvm/add_default_subnet_delegator_tx_test.go index 9d6d5cf..eda9049 100644 --- a/vms/platformvm/add_default_subnet_delegator_tx_test.go +++ b/vms/platformvm/add_default_subnet_delegator_tx_test.go @@ -13,6 +13,7 @@ import ( func TestAddDefaultSubnetDelegatorTxSyntacticVerify(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Case 1: tx is nil var tx *addDefaultSubnetDelegatorTx @@ -153,6 +154,7 @@ func TestAddDefaultSubnetDelegatorTxSyntacticVerify(t *testing.T) { func TestAddDefaultSubnetDelegatorTxSemanticVerify(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Case 1: Proposed validator currently validating default subnet // but stops validating non-default subnet after stops validating default subnet diff --git a/vms/platformvm/add_default_subnet_validator_tx_test.go b/vms/platformvm/add_default_subnet_validator_tx_test.go index bca9188..ee863e8 100644 --- a/vms/platformvm/add_default_subnet_validator_tx_test.go +++ b/vms/platformvm/add_default_subnet_validator_tx_test.go @@ -12,6 +12,7 @@ import ( func TestAddDefaultSubnetValidatorTxSyntacticVerify(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Case 1: tx is nil var tx *addDefaultSubnetValidatorTx @@ -216,6 +217,7 @@ func TestAddDefaultSubnetValidatorTxSyntacticVerify(t *testing.T) { // Test AddDefaultSubnetValidatorTx.SemanticVerify func TestAddDefaultSubnetValidatorTxSemanticVerify(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Case 1: Validator's start time too early tx, err := vm.newAddDefaultSubnetValidatorTx( diff --git a/vms/platformvm/add_nondefault_subnet_validator_tx_test.go b/vms/platformvm/add_nondefault_subnet_validator_tx_test.go index c29faf1..43069cf 100644 --- a/vms/platformvm/add_nondefault_subnet_validator_tx_test.go +++ b/vms/platformvm/add_nondefault_subnet_validator_tx_test.go @@ -14,6 +14,7 @@ import ( func TestAddNonDefaultSubnetValidatorTxSyntacticVerify(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Case 1: tx is nil var tx *addNonDefaultSubnetValidatorTx @@ -202,6 +203,7 @@ func TestAddNonDefaultSubnetValidatorTxSyntacticVerify(t *testing.T) { func TestAddNonDefaultSubnetValidatorTxSemanticVerify(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Case 1: Proposed validator currently validating default subnet // but stops validating non-default subnet after stops validating default subnet @@ -596,6 +598,7 @@ func TestAddNonDefaultSubnetValidatorTxSemanticVerify(t *testing.T) { // Test that marshalling/unmarshalling works func TestAddNonDefaultSubnetValidatorMarshal(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // valid tx tx, err := vm.newAddNonDefaultSubnetValidatorTx( diff --git a/vms/platformvm/advance_time_tx_test.go b/vms/platformvm/advance_time_tx_test.go index 8765600..d051869 100644 --- a/vms/platformvm/advance_time_tx_test.go +++ b/vms/platformvm/advance_time_tx_test.go @@ -17,6 +17,8 @@ func TestAdvanceTimeTxSyntacticVerify(t *testing.T) { // Case 2: Timestamp is ahead of synchrony bound vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + tx = &advanceTimeTx{ Time: uint64(defaultGenesisTime.Add(Delta).Add(1 * time.Second).Unix()), vm: vm, @@ -38,6 +40,7 @@ func TestAdvanceTimeTxSyntacticVerify(t *testing.T) { // Ensure semantic verification fails when proposed timestamp is at or before current timestamp func TestAdvanceTimeTxTimestampTooEarly(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() tx := &advanceTimeTx{ Time: uint64(defaultGenesisTime.Unix()), @@ -52,6 +55,7 @@ func TestAdvanceTimeTxTimestampTooEarly(t *testing.T) { // Ensure semantic verification fails when proposed timestamp is after next validator set change time func TestAdvanceTimeTxTimestampTooLate(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Case 1: Timestamp is after next validator start time // Add a pending validator @@ -117,6 +121,7 @@ func TestAdvanceTimeTxTimestampTooLate(t *testing.T) { // Ensure semantic verification updates the current and pending validator sets correctly func TestAdvanceTimeTxUpdateValidators(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Case 1: Timestamp is after next validator start time // Add a pending validator @@ -196,6 +201,7 @@ func TestAdvanceTimeTxUpdateValidators(t *testing.T) { // Test method InitiallyPrefersCommit func TestAdvanceTimeTxInitiallyPrefersCommit(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Proposed advancing timestamp to 1 second after current timestamp tx, err := vm.newAdvanceTimeTx(defaultGenesisTime.Add(1 * time.Second)) @@ -217,6 +223,7 @@ func TestAdvanceTimeTxInitiallyPrefersCommit(t *testing.T) { // Ensure marshaling/unmarshaling works func TestAdvanceTimeTxUnmarshal(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() tx, err := vm.newAdvanceTimeTx(defaultGenesisTime) if err != nil { diff --git a/vms/platformvm/create_chain_tx_test.go b/vms/platformvm/create_chain_tx_test.go index f9b8476..ae8694d 100644 --- a/vms/platformvm/create_chain_tx_test.go +++ b/vms/platformvm/create_chain_tx_test.go @@ -14,6 +14,7 @@ import ( // test method SyntacticVerify func TestCreateChainTxSyntacticVerify(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Case 1: tx is nil var tx *CreateChainTx @@ -142,6 +143,7 @@ func TestCreateChainTxSyntacticVerify(t *testing.T) { // Ensure SemanticVerify fails when there are not enough control sigs func TestCreateChainTxInsufficientControlSigs(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Case 1: No control sigs (2 are needed) tx, err := vm.newCreateChainTx( @@ -189,6 +191,7 @@ func TestCreateChainTxInsufficientControlSigs(t *testing.T) { // Ensure SemanticVerify fails when an incorrect control signature is given func TestCreateChainTxWrongControlSig(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Generate new, random key to sign tx with factory := crypto.FactorySECP256K1R{} @@ -222,6 +225,7 @@ func TestCreateChainTxWrongControlSig(t *testing.T) { // its validator set doesn't exist func TestCreateChainTxNoSuchSubnet(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() tx, err := vm.newCreateChainTx( defaultNonce+1, @@ -245,6 +249,7 @@ func TestCreateChainTxNoSuchSubnet(t *testing.T) { func TestCreateChainTxAlreadyExists(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // create a tx tx, err := vm.newCreateChainTx( @@ -276,6 +281,7 @@ func TestCreateChainTxAlreadyExists(t *testing.T) { // Ensure valid tx passes semanticVerify func TestCreateChainTxValid(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // create a valid tx tx, err := vm.newCreateChainTx( diff --git a/vms/platformvm/event_heap_test.go b/vms/platformvm/event_heap_test.go index 01dbc44..91570bd 100644 --- a/vms/platformvm/event_heap_test.go +++ b/vms/platformvm/event_heap_test.go @@ -11,6 +11,8 @@ import ( func TestTxHeapStart(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + txHeap := EventHeap{SortByStartTime: true} validator0, err := vm.newAddDefaultSubnetValidatorTx( @@ -78,6 +80,8 @@ func TestTxHeapStart(t *testing.T) { func TestTxHeapStop(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + txHeap := EventHeap{} validator0, err := vm.newAddDefaultSubnetValidatorTx( @@ -145,6 +149,8 @@ func TestTxHeapStop(t *testing.T) { func TestTxHeapStartValidatorVsDelegatorOrdering(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + txHeap := EventHeap{SortByStartTime: true} validator, err := vm.newAddDefaultSubnetValidatorTx( @@ -186,6 +192,8 @@ func TestTxHeapStartValidatorVsDelegatorOrdering(t *testing.T) { func TestTxHeapStopValidatorVsDelegatorOrdering(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + txHeap := EventHeap{} validator, err := vm.newAddDefaultSubnetValidatorTx( diff --git a/vms/platformvm/reward_validator_tx_test.go b/vms/platformvm/reward_validator_tx_test.go index b3563a2..3de1989 100644 --- a/vms/platformvm/reward_validator_tx_test.go +++ b/vms/platformvm/reward_validator_tx_test.go @@ -18,6 +18,8 @@ func TestRewardValidatorTxSyntacticVerify(t *testing.T) { } vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + txID := ids.NewID([32]byte{1, 2, 3, 4, 5, 6, 7}) tests := []test{ @@ -54,6 +56,8 @@ func TestRewardValidatorTxSyntacticVerify(t *testing.T) { func TestRewardValidatorTxSemanticVerify(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + var nextToRemove *addDefaultSubnetValidatorTx currentValidators, err := vm.getCurrentValidators(vm.DB, DefaultSubnetID) if err != nil { @@ -130,6 +134,7 @@ func TestRewardValidatorTxSemanticVerify(t *testing.T) { func TestRewardDelegatorTxSemanticVerify(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() keyIntf1, err := vm.factory.NewPrivateKey() if err != nil { diff --git a/vms/platformvm/vm_test.go b/vms/platformvm/vm_test.go index e0af19f..f53bd99 100644 --- a/vms/platformvm/vm_test.go +++ b/vms/platformvm/vm_test.go @@ -130,6 +130,8 @@ func defaultVM() *VM { db := memdb.New() msgChan := make(chan common.Message, 1) ctx := defaultContext() + ctx.Lock.Lock() + defer ctx.Lock.Unlock() if err := vm.Initialize(ctx, db, genesisBytes, msgChan, nil); err != nil { panic(err) } @@ -221,6 +223,7 @@ func GenesisCurrentValidators() *EventHeap { // Ensure genesis state is parsed from bytes and stored correctly func TestGenesis(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Ensure the genesis block has been accepted and stored genesisBlockID := vm.LastAccepted() // lastAccepted should be ID of genesis block @@ -290,6 +293,8 @@ func TestGenesis(t *testing.T) { // accept proposal to add validator to default subnet func TestAddDefaultSubnetValidatorCommit(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + startTime := defaultGenesisTime.Add(Delta).Add(1 * time.Second) endTime := startTime.Add(MinimumStakingDuration) key, _ := vm.factory.NewPrivateKey() @@ -357,6 +362,8 @@ func TestAddDefaultSubnetValidatorCommit(t *testing.T) { // Reject proposal to add validator to default subnet func TestAddDefaultSubnetValidatorReject(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + startTime := defaultGenesisTime.Add(Delta).Add(1 * time.Second) endTime := startTime.Add(MinimumStakingDuration) key, _ := vm.factory.NewPrivateKey() @@ -428,6 +435,8 @@ func TestAddDefaultSubnetValidatorReject(t *testing.T) { // Accept proposal to add validator to non-default subnet func TestAddNonDefaultSubnetValidatorAccept(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + startTime := defaultValidateStartTime.Add(Delta).Add(1 * time.Second) endTime := startTime.Add(MinimumStakingDuration) @@ -499,6 +508,8 @@ func TestAddNonDefaultSubnetValidatorAccept(t *testing.T) { // Reject proposal to add validator to non-default subnet func TestAddNonDefaultSubnetValidatorReject(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + startTime := defaultValidateStartTime.Add(Delta).Add(1 * time.Second) endTime := startTime.Add(MinimumStakingDuration) key, _ := vm.factory.NewPrivateKey() @@ -572,6 +583,7 @@ func TestAddNonDefaultSubnetValidatorReject(t *testing.T) { // Test case where default subnet validator rewarded func TestRewardValidatorAccept(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Fast forward clock to time for genesis validators to leave vm.clock.Set(defaultValidateEndTime) @@ -664,6 +676,7 @@ func TestRewardValidatorAccept(t *testing.T) { // Test case where default subnet validator not rewarded func TestRewardValidatorReject(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Fast forward clock to time for genesis validators to leave vm.clock.Set(defaultValidateEndTime) @@ -756,6 +769,7 @@ func TestRewardValidatorReject(t *testing.T) { // Ensure BuildBlock errors when there is no block to build func TestUnneededBuildBlock(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() if _, err := vm.BuildBlock(); err == nil { t.Fatalf("Should have errored on BuildBlock") @@ -765,6 +779,7 @@ func TestUnneededBuildBlock(t *testing.T) { // test acceptance of proposal to create a new chain func TestCreateChain(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() tx, err := vm.newCreateChainTx( defaultNonce+1, @@ -827,6 +842,7 @@ func TestCreateChain(t *testing.T) { // 4) Advance timestamp to validator's end time (removing validator from current) func TestCreateSubnet(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() createSubnetTx, err := vm.newCreateSubnetTx( testNetworkID, @@ -1072,6 +1088,7 @@ func TestCreateSubnet(t *testing.T) { // test asset import func TestAtomicImport(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() avmID := ids.Empty.Prefix(0) utxoID := ava.UTXOID{ @@ -1163,6 +1180,7 @@ func TestAtomicImport(t *testing.T) { // test optimistic asset import func TestOptimisticAtomicImport(t *testing.T) { vm := defaultVM() + defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() avmID := ids.Empty.Prefix(0) utxoID := ava.UTXOID{ From 243811f1e88bf1a0a9990bd4bdbab90048a47b2d Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 21 Apr 2020 01:30:31 +0100 Subject: [PATCH 08/24] vms: Note 2 potential FIXMEs in avm.VM tests This is not an exhaustive list. --- vms/platformvm/vm_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/vms/platformvm/vm_test.go b/vms/platformvm/vm_test.go index f53bd99..0ec0524 100644 --- a/vms/platformvm/vm_test.go +++ b/vms/platformvm/vm_test.go @@ -226,6 +226,7 @@ func TestGenesis(t *testing.T) { defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() // Ensure the genesis block has been accepted and stored + // FIXME? Calling vm.LastAccepted() without the lock genesisBlockID := vm.LastAccepted() // lastAccepted should be ID of genesis block genesisBlock, err := vm.getBlock(genesisBlockID) if err != nil { @@ -771,6 +772,7 @@ func TestUnneededBuildBlock(t *testing.T) { vm := defaultVM() defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + // FIXME? Calling vm.BuildBlock without the lock if _, err := vm.BuildBlock(); err == nil { t.Fatalf("Should have errored on BuildBlock") } From a769c2017ecd23d83059888821de6e723399ed2d Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 21 Apr 2020 01:31:33 +0100 Subject: [PATCH 09/24] vms: Fix deadlock when stopping timers during spchainvm.VM.Shutdown() refs #66 --- vms/spchainvm/vm.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/vms/spchainvm/vm.go b/vms/spchainvm/vm.go index 8956097..8ed6309 100644 --- a/vms/spchainvm/vm.go +++ b/vms/spchainvm/vm.go @@ -122,7 +122,11 @@ func (vm *VM) Shutdown() { return } + // There is a potential deadlock if the timer is about to execute a timeout. + // So, the lock must be released before stopping the timer. + vm.ctx.Lock.Unlock() vm.timer.Stop() + vm.ctx.Lock.Lock() if err := vm.baseDB.Close(); err != nil { vm.ctx.Log.Error("Closing the database failed with %s", err) } From 996ea99fb1f4aa6516bbf68086554a6817599797 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 21 Apr 2020 01:34:22 +0100 Subject: [PATCH 10/24] vms: Ensure all spchainvm.VM instances in tests get shutdown --- vms/spchainvm/consensus_benchmark_test.go | 2 ++ vms/spchainvm/vm_benchmark_test.go | 7 +++++++ vms/spchainvm/vm_test.go | 1 + 3 files changed, 10 insertions(+) diff --git a/vms/spchainvm/consensus_benchmark_test.go b/vms/spchainvm/consensus_benchmark_test.go index aa80e6d..8b6352d 100644 --- a/vms/spchainvm/consensus_benchmark_test.go +++ b/vms/spchainvm/consensus_benchmark_test.go @@ -66,6 +66,7 @@ func ConsensusLeader(numBlocks, numTxsPerBlock int, b *testing.B) { // Initialize the VM vm := &VM{} + defer func() { ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() ctx.Lock.Lock() if err := vm.Initialize(ctx, vmDB, genesisData, msgChan, nil); err != nil { b.Fatal(err) @@ -198,6 +199,7 @@ func ConsensusFollower(numBlocks, numTxsPerBlock int, b *testing.B) { vm := &VM{ onAccept: func(ids.ID) { wg.Done() }, } + defer func() { ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() ctx.Lock.Lock() if err := vm.Initialize(ctx, vmDB, genesisData, msgChan, nil); err != nil { b.Fatal(err) diff --git a/vms/spchainvm/vm_benchmark_test.go b/vms/spchainvm/vm_benchmark_test.go index ac4ca13..ba450b1 100644 --- a/vms/spchainvm/vm_benchmark_test.go +++ b/vms/spchainvm/vm_benchmark_test.go @@ -73,6 +73,7 @@ func BenchmarkParseBlock(b *testing.B) { /*testing=*/ b, ) vm := &VM{} + defer func() { ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() vm.Initialize( /*ctx=*/ ctx, /*db=*/ memdb.New(), @@ -106,6 +107,7 @@ func BenchmarkParseAndVerify(b *testing.B) { for n := 0; n < b.N; n++ { vm := &VM{} + defer func() { ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() vm.Initialize( /*ctx=*/ snow.DefaultContextTest(), /*db=*/ memdb.New(), @@ -141,6 +143,8 @@ func BenchmarkAccept(b *testing.B) { for n := 0; n < b.N; n++ { vm := &VM{} + defer func() { ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() + vm.Initialize( /*ctx=*/ snow.DefaultContextTest(), /*db=*/ memdb.New(), @@ -178,6 +182,7 @@ func ParseAndVerifyAndAccept(numBlocks, numTxsPerBlock int, b *testing.B) { for n := 0; n < b.N; n++ { vm := &VM{} + defer func() { ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() vm.Initialize( /*ctx=*/ snow.DefaultContextTest(), /*db=*/ memdb.New(), @@ -232,6 +237,7 @@ func ParseThenVerifyThenAccept(numBlocks, numTxsPerBlock int, b *testing.B) { for n := 0; n < b.N; n++ { vm := &VM{} + defer func() { ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() vm.Initialize( /*ctx=*/ snow.DefaultContextTest(), /*db=*/ memdb.New(), @@ -292,6 +298,7 @@ func IssueAndVerifyAndAccept(numBlocks, numTxsPerBlock int, b *testing.B) { for n := 0; n < b.N; n++ { vm := &VM{} + defer func() { ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() vm.Initialize( /*ctx=*/ snow.DefaultContextTest(), /*db=*/ memdb.New(), diff --git a/vms/spchainvm/vm_test.go b/vms/spchainvm/vm_test.go index 4432d1e..fb54941 100644 --- a/vms/spchainvm/vm_test.go +++ b/vms/spchainvm/vm_test.go @@ -67,6 +67,7 @@ func TestPayments(t *testing.T) { blocker, _ := queue.New(bootstrappingDB) vm := &VM{} + defer func() { ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() vm.Initialize(ctx, db, genesisData, msgChan, nil) sender := &common.SenderTest{} From 2e02f6863e20a79d3a1e6f96b88113cb3dd9d37b Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 21 Apr 2020 01:40:46 +0100 Subject: [PATCH 11/24] vms: Fix deadlock when stopping timers during spdagvm.VM.Shutdown() refs #66 --- vms/spdagvm/vm.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/vms/spdagvm/vm.go b/vms/spdagvm/vm.go index f23b2ca..5539446 100644 --- a/vms/spdagvm/vm.go +++ b/vms/spdagvm/vm.go @@ -134,7 +134,11 @@ func (vm *VM) Shutdown() { return } + // There is a potential deadlock if the timer is about to execute a timeout. + // So, the lock must be released before stopping the timer. + vm.ctx.Lock.Unlock() vm.timer.Stop() + vm.ctx.Lock.Lock() if err := vm.baseDB.Close(); err != nil { vm.ctx.Log.Error("Closing the database failed with %s", err) } From 9f1aa5bbd230341c0d37f2ec2c769e80f5790fbb Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Tue, 21 Apr 2020 01:41:12 +0100 Subject: [PATCH 12/24] vms: Ensure all spdagvm.VM instances in tests get shutdown --- vms/spdagvm/vm_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/vms/spdagvm/vm_test.go b/vms/spdagvm/vm_test.go index e12b15e..c2820dc 100644 --- a/vms/spdagvm/vm_test.go +++ b/vms/spdagvm/vm_test.go @@ -91,6 +91,7 @@ func TestAva(t *testing.T) { msgChan := make(chan common.Message, 1) vm := &VM{} + defer func() { vm.ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() vm.Initialize(ctx, vmDB, genesisTx.Bytes(), msgChan, nil) vm.batchTimeout = 0 @@ -172,6 +173,7 @@ func TestInvalidSpentTx(t *testing.T) { msgChan := make(chan common.Message, 1) vm := &VM{} + defer func() { vm.ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() ctx.Lock.Lock() vm.Initialize(ctx, vmDB, genesisTx.Bytes(), msgChan, nil) @@ -258,6 +260,7 @@ func TestInvalidTxVerification(t *testing.T) { msgChan := make(chan common.Message, 1) vm := &VM{} + defer func() { vm.ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() ctx.Lock.Lock() vm.Initialize(ctx, vmDB, genesisTx.Bytes(), msgChan, nil) @@ -319,6 +322,7 @@ func TestRPCAPI(t *testing.T) { vmDB := memdb.New() msgChan := make(chan common.Message, 1) vm := &VM{} + defer func() { vm.ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() vm.Initialize(ctx, vmDB, genesisTx.Bytes(), msgChan, nil) vm.batchTimeout = 0 @@ -526,6 +530,7 @@ func TestMultipleSend(t *testing.T) { vmDB := memdb.New() msgChan := make(chan common.Message, 1) vm := &VM{} + defer func() { vm.ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() vm.Initialize(ctx, vmDB, genesisTx.Bytes(), msgChan, nil) // Initialize these data structures @@ -635,6 +640,7 @@ func TestIssuePendingDependency(t *testing.T) { ctx.Lock.Lock() vm := &VM{} + defer func() { vm.ctx.Lock.Lock(); vm.Shutdown(); vm.ctx.Lock.Unlock() }() vm.Initialize(ctx, vmDB, genesisTx.Bytes(), msgChan, nil) vm.batchTimeout = 0 From f104b5f1150ab40b72d0eb0ed8dd21882303c43c Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Sun, 3 May 2020 18:16:25 -0400 Subject: [PATCH 13/24] add sanity checks to platform API method arguments --- vms/platformvm/service.go | 77 ++++++++++++++++++++++++++++++++++----- 1 file changed, 67 insertions(+), 10 deletions(-) diff --git a/vms/platformvm/service.go b/vms/platformvm/service.go index e2a9ffe..6e3c4e2 100644 --- a/vms/platformvm/service.go +++ b/vms/platformvm/service.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "net/http" + "time" "github.com/ava-labs/gecko/database" "github.com/ava-labs/gecko/ids" @@ -278,7 +279,7 @@ type GetAccountReply struct { func (service *Service) GetAccount(_ *http.Request, args *GetAccountArgs, reply *GetAccountReply) error { account, err := service.vm.getAccount(service.vm.DB, args.Address) if err != nil && err != database.ErrNotFound { - return errGetAccount + return fmt.Errorf("couldn't get account: %v", err) } else if err == database.ErrNotFound { account = newAccount(args.Address, 0, 0) } @@ -308,7 +309,7 @@ func (service *Service) ListAccounts(_ *http.Request, args *ListAccountsArgs, re // db holds the user's info that pertains to the Platform Chain userDB, err := service.vm.Ctx.Keystore.GetDatabase(args.Username, args.Password) if err != nil { - return errGetUser + return fmt.Errorf("couldn't get user: %v", err) } // The user @@ -319,7 +320,7 @@ func (service *Service) ListAccounts(_ *http.Request, args *ListAccountsArgs, re // IDs of accounts controlled by this user accountIDs, err := user.getAccountIDs() if err != nil { - return errGetAccounts + return fmt.Errorf("couldn't get accounts held by user: %v", err) } reply.Accounts = []APIAccount{} @@ -370,7 +371,7 @@ func (service *Service) CreateAccount(_ *http.Request, args *CreateAccountArgs, // userDB holds the user's info that pertains to the Platform Chain userDB, err := service.vm.Ctx.Keystore.GetDatabase(args.Username, args.Password) if err != nil { - return errGetUser + return fmt.Errorf("couldn't get user: %v", err) } // The user creating a new account @@ -428,7 +429,7 @@ type CreateTxResponse struct { type AddDefaultSubnetValidatorArgs struct { APIDefaultSubnetValidator - // Next unused nonce of the account the staked $AVA and tx fee are paid from + // Next nonce of the sender PayerNonce json.Uint64 `json:"payerNonce"` } @@ -437,8 +438,13 @@ type AddDefaultSubnetValidatorArgs struct { func (service *Service) AddDefaultSubnetValidator(_ *http.Request, args *AddDefaultSubnetValidatorArgs, reply *CreateTxResponse) error { service.vm.Ctx.Log.Debug("AddDefaultSubnetValidator called") - if args.ID.IsZero() { // If ID unspecified, use this node's ID as validator ID + switch { + case args.ID.IsZero(): // If ID unspecified, use this node's ID as validator ID args.ID = service.vm.Ctx.NodeID + case args.PayerNonce == 0: + return fmt.Errorf("sender's next nonce not specified") + case int64(args.StartTime) < time.Now().Unix(): + return fmt.Errorf("start time must be in the future") } // Create the transaction @@ -482,8 +488,13 @@ type AddDefaultSubnetDelegatorArgs struct { func (service *Service) AddDefaultSubnetDelegator(_ *http.Request, args *AddDefaultSubnetDelegatorArgs, reply *CreateTxResponse) error { service.vm.Ctx.Log.Debug("AddDefaultSubnetDelegator called") - if args.ID.IsZero() { // If ID unspecified, use this node's ID as validator ID + switch { + case args.ID.IsZero(): // If ID unspecified, use this node's ID as validator ID args.ID = service.vm.Ctx.NodeID + case args.PayerNonce == 0: + return fmt.Errorf("sender's next unused nonce not specified") + case int64(args.StartTime) < time.Now().Unix(): + return fmt.Errorf("start time must be in the future") } // Create the transaction @@ -571,6 +582,11 @@ type CreateSubnetArgs struct { func (service *Service) CreateSubnet(_ *http.Request, args *CreateSubnetArgs, response *CreateTxResponse) error { service.vm.Ctx.Log.Debug("platform.createSubnet called") + switch { + case args.PayerNonce == 0: + return fmt.Errorf("sender's next nonce not specified") + } + // Create the transaction tx := CreateSubnetTx{ UnsignedCreateSubnetTx: UnsignedCreateSubnetTx{ @@ -612,6 +628,13 @@ type ExportAVAArgs struct { func (service *Service) ExportAVA(_ *http.Request, args *ExportAVAArgs, response *CreateTxResponse) error { service.vm.Ctx.Log.Debug("platform.ExportAVA called") + switch { + case args.PayerNonce == 0: + return fmt.Errorf("sender's next nonce not specified") + case uint64(args.Amount) == 0: + return fmt.Errorf("amount must be >0") + } + // Create the transaction tx := ExportTx{UnsignedExportTx: UnsignedExportTx{ NetworkID: service.vm.Ctx.NetworkID, @@ -667,6 +690,11 @@ type SignResponse struct { func (service *Service) Sign(_ *http.Request, args *SignArgs, reply *SignResponse) error { service.vm.Ctx.Log.Debug("sign called") + switch { + case args.Signer.Equals(ids.ShortEmpty): + return fmt.Errorf("signer not specified") + } + // Get the key of the Signer db, err := service.vm.Ctx.Keystore.GetDatabase(args.Username, args.Password) if err != nil { @@ -861,7 +889,7 @@ type ImportAVAArgs struct { // ID of the account that will receive the imported funds, and pay the transaction fee To ids.ShortID `json:"to"` - // Next unused nonce of the account + // Next nonce of the sender PayerNonce json.Uint64 `json:"payerNonce"` // User that controls the account @@ -875,10 +903,15 @@ type ImportAVAArgs struct { func (service *Service) ImportAVA(_ *http.Request, args *ImportAVAArgs, response *SignResponse) error { service.vm.Ctx.Log.Debug("platform.ImportAVA called") + switch { + case args.PayerNonce == 0: + return fmt.Errorf("sender's next nonce not specified") + } + // Get the key of the Signer db, err := service.vm.Ctx.Keystore.GetDatabase(args.Username, args.Password) if err != nil { - return fmt.Errorf("couldn't get data for user '%s'. Does user exist?", args.Username) + return fmt.Errorf("couldn't get user: %v", err) } user := user{db: db} @@ -1099,7 +1132,7 @@ type CreateBlockchainArgs struct { // Human-readable name for the new blockchain, not necessarily unique Name string `json:"name"` - // Next unused nonce of the account paying the transaction fee + // Next nonce of the sender PayerNonce json.Uint64 `json:"payerNonce"` // Genesis state of the blockchain being created @@ -1111,6 +1144,15 @@ type CreateBlockchainArgs struct { func (service *Service) CreateBlockchain(_ *http.Request, args *CreateBlockchainArgs, response *CreateTxResponse) error { service.vm.Ctx.Log.Debug("createBlockchain called") + switch { + case args.PayerNonce == 0: + return errors.New("sender's next nonce not specified") + case args.VMID == "": + return errors.New("VM not specified") + case args.SubnetID.Equals(ids.Empty): + return errors.New("subnet not specified") + } + vmID, err := service.vm.chainManager.LookupVM(args.VMID) if err != nil { return fmt.Errorf("no VM with ID '%s' found", args.VMID) @@ -1180,6 +1222,11 @@ type GetBlockchainStatusReply struct { func (service *Service) GetBlockchainStatus(_ *http.Request, args *GetBlockchainStatusArgs, reply *GetBlockchainStatusReply) error { service.vm.Ctx.Log.Debug("getBlockchainStatus called") + switch { + case args.BlockchainID == "": + return errors.New("'blockchainID' not given") + } + _, err := service.vm.chainManager.Lookup(args.BlockchainID) if err == nil { reply.Status = Validating @@ -1255,6 +1302,11 @@ type ValidatedByResponse struct { func (service *Service) ValidatedBy(_ *http.Request, args *ValidatedByArgs, response *ValidatedByResponse) error { service.vm.Ctx.Log.Debug("validatedBy called") + switch { + case args.BlockchainID.Equals(ids.Empty): + return errors.New("'blockchainID' not specified") + } + chain, err := service.vm.getChain(service.vm.DB, args.BlockchainID) if err != nil { return err @@ -1277,6 +1329,11 @@ type ValidatesResponse struct { func (service *Service) Validates(_ *http.Request, args *ValidatesArgs, response *ValidatesResponse) error { service.vm.Ctx.Log.Debug("validates called") + switch { + case args.SubnetID.Equals(ids.Empty): + return errors.New("'subnetID' not specified") + } + // Verify that the Subnet exists if _, err := service.vm.getSubnet(service.vm.DB, args.SubnetID); err != nil { return err From 4a989dc62132bc5d744520fbdf0ddddcf0a0e4a0 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 4 May 2020 02:32:10 -0400 Subject: [PATCH 14/24] Added uniform periodic gossiping --- chains/manager.go | 3 +- networking/voting_handlers.go | 78 ++++++++++++------- snow/engine/avalanche/bootstrapper_test.go | 3 +- snow/engine/avalanche/transitive.go | 20 +++++ snow/engine/common/engine.go | 3 + snow/engine/common/sender.go | 8 ++ snow/engine/common/test_engine.go | 13 +++- snow/engine/common/test_sender.go | 16 +++- snow/engine/snowman/bootstrapper_test.go | 3 +- snow/engine/snowman/transitive.go | 13 ++++ snow/networking/handler/handler.go | 5 ++ snow/networking/handler/message.go | 3 + snow/networking/router/router.go | 4 +- snow/networking/router/subnet_router.go | 20 ++++- snow/networking/sender/external_sender.go | 2 + snow/networking/sender/sender.go | 6 ++ snow/networking/sender/sender_test.go | 2 +- .../networking/sender/test_external_sender.go | 18 ++++- 18 files changed, 183 insertions(+), 37 deletions(-) diff --git a/chains/manager.go b/chains/manager.go index 3f7d48d..3ce5fd6 100644 --- a/chains/manager.go +++ b/chains/manager.go @@ -40,6 +40,7 @@ import ( const ( defaultChannelSize = 1000 requestTimeout = 2 * time.Second + gossipFrequency = 10 * time.Second ) // Manager manages the chains running on this node. @@ -146,7 +147,7 @@ func New( timeoutManager.Initialize(requestTimeout) go log.RecoverAndPanic(timeoutManager.Dispatch) - router.Initialize(log, &timeoutManager) + router.Initialize(log, &timeoutManager, gossipFrequency) m := &manager{ stakingEnabled: stakingEnabled, diff --git a/networking/voting_handlers.go b/networking/voting_handlers.go index a5746d4..f6a43e3 100644 --- a/networking/voting_handlers.go +++ b/networking/voting_handlers.go @@ -18,6 +18,7 @@ import "C" import ( "errors" "fmt" + "math" "unsafe" "github.com/prometheus/client_golang/prometheus" @@ -29,9 +30,15 @@ import ( "github.com/ava-labs/gecko/snow/validators" "github.com/ava-labs/gecko/utils/formatting" "github.com/ava-labs/gecko/utils/logging" + "github.com/ava-labs/gecko/utils/random" "github.com/ava-labs/gecko/utils/timer" ) +// GossipSize is the maximum number of peers to gossip a container to +const ( + GossipSize = 50 +) + var ( // VotingNet implements the SenderExternal interface. VotingNet = Voting{} @@ -89,34 +96,7 @@ func (s *Voting) Shutdown() { s.executor.Stop() } // Accept is called after every consensus decision func (s *Voting) Accept(chainID, containerID ids.ID, container []byte) error { - peers := []salticidae.PeerID(nil) - - allPeers, allIDs, _ := s.conns.Conns() - for i, id := range allIDs { - if !s.vdrs.Contains(id) { - peers = append(peers, allPeers[i]) - } - } - - build := Builder{} - msg, err := build.Put(chainID, 0, containerID, container) - if err != nil { - return fmt.Errorf("Attempted to pack too large of a Put message.\nContainer length: %d: %w", len(container), err) - } - - s.log.Verbo("Sending a Put message to non-validators."+ - "\nNumber of Non-Validators: %d"+ - "\nChain: %s"+ - "\nContainer ID: %s"+ - "\nContainer:\n%s", - len(peers), - chainID, - containerID, - formatting.DumpBytes{Bytes: container}, - ) - s.send(msg, peers...) - s.numPutSent.Add(float64(len(peers))) - return nil + return s.gossip(chainID, containerID, container) } // GetAcceptedFrontier implements the Sender interface. @@ -412,6 +392,13 @@ func (s *Voting) Chits(validatorID ids.ShortID, chainID ids.ID, requestID uint32 s.numChitsSent.Inc() } +// Gossip attempts to gossip the container to the network +func (s *Voting) Gossip(chainID, containerID ids.ID, container []byte) { + if err := s.gossip(chainID, containerID, container); err != nil { + s.log.Error("Error gossiping container %s to %s\n%s", containerID, chainID, err) + } +} + func (s *Voting) send(msg Msg, peers ...salticidae.PeerID) { ds := msg.DataStream() defer ds.Free() @@ -429,6 +416,41 @@ func (s *Voting) send(msg Msg, peers ...salticidae.PeerID) { } } +func (s *Voting) gossip(chainID, containerID ids.ID, container []byte) error { + allPeers := s.conns.PeerIDs() + + numToGossip := GossipSize + if numToGossip > len(allPeers) { + numToGossip = len(allPeers) + } + peers := make([]salticidae.PeerID, numToGossip) + + sampler := random.Uniform{N: len(allPeers)} + for i := range peers { + peers[i] = allPeers[sampler.Sample()] + } + + build := Builder{} + msg, err := build.Put(chainID, math.MaxUint32, containerID, container) + if err != nil { + return fmt.Errorf("Attempted to pack too large of a Put message.\nContainer length: %d: %w", len(container), err) + } + + s.log.Verbo("Sending a Put message to non-validators."+ + "\nNumber of Non-Validators: %d"+ + "\nChain: %s"+ + "\nContainer ID: %s"+ + "\nContainer:\n%s", + len(peers), + chainID, + containerID, + formatting.DumpBytes{Bytes: container}, + ) + s.send(msg, peers...) + s.numPutSent.Add(float64(len(peers))) + return nil +} + // getAcceptedFrontier handles the recept of a getAcceptedFrontier container // message for a chain //export getAcceptedFrontier diff --git a/snow/engine/avalanche/bootstrapper_test.go b/snow/engine/avalanche/bootstrapper_test.go index cc63b68..3fa2cbe 100644 --- a/snow/engine/avalanche/bootstrapper_test.go +++ b/snow/engine/avalanche/bootstrapper_test.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "testing" + "time" "github.com/prometheus/client_golang/prometheus" @@ -60,7 +61,7 @@ func newConfig(t *testing.T) (BootstrapConfig, ids.ShortID, *common.SenderTest, handler.Initialize(engine, make(chan common.Message), 1) timeouts.Initialize(0) - router.Initialize(ctx.Log, timeouts) + router.Initialize(ctx.Log, timeouts, time.Hour) vtxBlocker, _ := queue.New(prefixdb.New([]byte("vtx"), db)) txBlocker, _ := queue.New(prefixdb.New([]byte("tx"), db)) diff --git a/snow/engine/avalanche/transitive.go b/snow/engine/avalanche/transitive.go index fc55250..4b33007 100644 --- a/snow/engine/avalanche/transitive.go +++ b/snow/engine/avalanche/transitive.go @@ -62,6 +62,26 @@ func (t *Transitive) finishBootstrapping() { t.bootstrapped = true } +// Gossip implements the Engine interface +func (t *Transitive) Gossip() { + edge := t.Config.State.Edge() + if len(edge) == 0 { + t.Config.Context.Log.Debug("Dropping gossip request as no vertices have been accepted") + return + } + + sampler := random.Uniform{N: len(edge)} + vtxID := edge[sampler.Sample()] + vtx, err := t.Config.State.GetVertex(vtxID) + if err != nil { + t.Config.Context.Log.Warn("Dropping gossip request as %s couldn't be loaded due to %s", vtxID, err) + return + } + + t.Config.Context.Log.Info("Gossiping %s as accepted to the network", vtxID) + t.Config.Sender.Gossip(vtxID, vtx.Bytes()) +} + // Shutdown implements the Engine interface func (t *Transitive) Shutdown() { t.Config.Context.Log.Info("Shutting down Avalanche consensus") diff --git a/snow/engine/common/engine.go b/snow/engine/common/engine.go index c4e431b..06761de 100644 --- a/snow/engine/common/engine.go +++ b/snow/engine/common/engine.go @@ -112,6 +112,9 @@ type InternalHandler interface { // Startup this engine. Startup() + // Gossip to the network a container on the accepted frontier + Gossip() + // Shutdown this engine. Shutdown() diff --git a/snow/engine/common/sender.go b/snow/engine/common/sender.go index d808b3a..a72375e 100644 --- a/snow/engine/common/sender.go +++ b/snow/engine/common/sender.go @@ -14,6 +14,7 @@ type Sender interface { AcceptedSender FetchSender QuerySender + Gossiper } // FrontierSender defines how a consensus engine sends frontier messages to @@ -70,3 +71,10 @@ type QuerySender interface { // Chits sends chits to the specified validator Chits(validatorID ids.ShortID, requestID uint32, votes ids.Set) } + +// Gossiper defines how a consensus engine gossips a container on the accepted +// frontier to other validators +type Gossiper interface { + // Gossip gossips the provided container throughout the network + Gossip(containerID ids.ID, container []byte) +} diff --git a/snow/engine/common/test_engine.go b/snow/engine/common/test_engine.go index d708008..27a07f5 100644 --- a/snow/engine/common/test_engine.go +++ b/snow/engine/common/test_engine.go @@ -15,6 +15,7 @@ type EngineTest struct { T *testing.T CantStartup, + CantGossip, CantShutdown, CantContext, @@ -38,7 +39,7 @@ type EngineTest struct { CantQueryFailed, CantChits bool - StartupF, ShutdownF func() + StartupF, GossipF, ShutdownF func() ContextF func() *snow.Context NotifyF func(Message) GetF, GetFailedF, PullQueryF func(validatorID ids.ShortID, requestID uint32, containerID ids.ID) @@ -50,6 +51,7 @@ type EngineTest struct { // Default ... func (e *EngineTest) Default(cant bool) { e.CantStartup = cant + e.CantGossip = cant e.CantShutdown = cant e.CantContext = cant @@ -83,6 +85,15 @@ func (e *EngineTest) Startup() { } } +// Gossip ... +func (e *EngineTest) Gossip() { + if e.GossipF != nil { + e.GossipF() + } else if e.CantGossip && e.T != nil { + e.T.Fatalf("Unexpectedly called Gossip") + } +} + // Shutdown ... func (e *EngineTest) Shutdown() { if e.ShutdownF != nil { diff --git a/snow/engine/common/test_sender.go b/snow/engine/common/test_sender.go index 10dd9c2..435f38b 100644 --- a/snow/engine/common/test_sender.go +++ b/snow/engine/common/test_sender.go @@ -16,7 +16,8 @@ type SenderTest struct { CantGetAcceptedFrontier, CantAcceptedFrontier, CantGetAccepted, CantAccepted, CantGet, CantPut, - CantPullQuery, CantPushQuery, CantChits bool + CantPullQuery, CantPushQuery, CantChits, + CantGossip bool GetAcceptedFrontierF func(ids.ShortSet, uint32) AcceptedFrontierF func(ids.ShortID, uint32, ids.Set) @@ -27,6 +28,7 @@ type SenderTest struct { PushQueryF func(ids.ShortSet, uint32, ids.ID, []byte) PullQueryF func(ids.ShortSet, uint32, ids.ID) ChitsF func(ids.ShortID, uint32, ids.Set) + GossipF func(ids.ID, []byte) } // Default set the default callable value to [cant] @@ -40,6 +42,7 @@ func (s *SenderTest) Default(cant bool) { s.CantPullQuery = cant s.CantPushQuery = cant s.CantChits = cant + s.CantGossip = cant } // GetAcceptedFrontier calls GetAcceptedFrontierF if it was initialized. If it @@ -140,3 +143,14 @@ func (s *SenderTest) Chits(vdr ids.ShortID, requestID uint32, votes ids.Set) { s.T.Fatalf("Unexpectedly called Chits") } } + +// Gossip calls GossipF if it was initialized. If it wasn't initialized and this +// function shouldn't be called and testing was initialized, then testing will +// fail. +func (s *SenderTest) Gossip(containerID ids.ID, container []byte) { + if s.GossipF != nil { + s.GossipF(containerID, container) + } else if s.CantGossip && s.T != nil { + s.T.Fatalf("Unexpectedly called Gossip") + } +} diff --git a/snow/engine/snowman/bootstrapper_test.go b/snow/engine/snowman/bootstrapper_test.go index 6168df2..19fa608 100644 --- a/snow/engine/snowman/bootstrapper_test.go +++ b/snow/engine/snowman/bootstrapper_test.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "testing" + "time" "github.com/prometheus/client_golang/prometheus" @@ -54,7 +55,7 @@ func newConfig(t *testing.T) (BootstrapConfig, ids.ShortID, *common.SenderTest, handler.Initialize(engine, make(chan common.Message), 1) timeouts.Initialize(0) - router.Initialize(ctx.Log, timeouts) + router.Initialize(ctx.Log, timeouts, time.Hour) blocker, _ := queue.New(db) diff --git a/snow/engine/snowman/transitive.go b/snow/engine/snowman/transitive.go index 9a827a6..f4778ac 100644 --- a/snow/engine/snowman/transitive.go +++ b/snow/engine/snowman/transitive.go @@ -65,6 +65,19 @@ func (t *Transitive) finishBootstrapping() { } } +// Gossip implements the Engine interface +func (t *Transitive) Gossip() { + blkID := t.Config.VM.LastAccepted() + blk, err := t.Config.VM.GetBlock(blkID) + if err != nil { + t.Config.Context.Log.Warn("Dropping gossip request as %s couldn't be loaded due to %s", blkID, err) + return + } + + t.Config.Context.Log.Info("Gossiping %s as accepted to the network", blkID) + t.Config.Sender.Gossip(blkID, blk.Bytes()) +} + // Shutdown implements the Engine interface func (t *Transitive) Shutdown() { t.Config.Context.Log.Info("Shutting down Snowman consensus") diff --git a/snow/networking/handler/handler.go b/snow/networking/handler/handler.go index dec10b7..12c65be 100644 --- a/snow/networking/handler/handler.go +++ b/snow/networking/handler/handler.go @@ -91,6 +91,8 @@ func (h *Handler) dispatchMsg(msg message) bool { h.engine.Chits(msg.validatorID, msg.requestID, msg.containerIDs) case notifyMsg: h.engine.Notify(msg.notification) + case gossipMsg: + h.engine.Gossip() case shutdownMsg: h.engine.Shutdown() return false @@ -232,6 +234,9 @@ func (h *Handler) QueryFailed(validatorID ids.ShortID, requestID uint32) { } } +// Gossip passes a gossip request to the consensus engine +func (h *Handler) Gossip() { h.msgs <- message{messageType: gossipMsg} } + // Shutdown shuts down the dispatcher func (h *Handler) Shutdown() { h.msgs <- message{messageType: shutdownMsg}; h.wg.Wait() } diff --git a/snow/networking/handler/message.go b/snow/networking/handler/message.go index 27d852d..f5394f0 100644 --- a/snow/networking/handler/message.go +++ b/snow/networking/handler/message.go @@ -29,6 +29,7 @@ const ( chitsMsg queryFailedMsg notifyMsg + gossipMsg shutdownMsg ) @@ -87,6 +88,8 @@ func (t msgType) String() string { return "Query Failed Message" case notifyMsg: return "Notify Message" + case gossipMsg: + return "Gossip Message" case shutdownMsg: return "Shutdown Message" default: diff --git a/snow/networking/router/router.go b/snow/networking/router/router.go index 4ed00f8..fca6983 100644 --- a/snow/networking/router/router.go +++ b/snow/networking/router/router.go @@ -4,6 +4,8 @@ package router import ( + "time" + "github.com/ava-labs/gecko/ids" "github.com/ava-labs/gecko/snow/networking/handler" "github.com/ava-labs/gecko/snow/networking/timeout" @@ -19,7 +21,7 @@ type Router interface { AddChain(chain *handler.Handler) RemoveChain(chainID ids.ID) Shutdown() - Initialize(log logging.Logger, timeouts *timeout.Manager) + Initialize(log logging.Logger, timeouts *timeout.Manager, gossipFrequency time.Duration) } // ExternalRouter routes messages from the network to the diff --git a/snow/networking/router/subnet_router.go b/snow/networking/router/subnet_router.go index ca1f6de..24b6efb 100644 --- a/snow/networking/router/subnet_router.go +++ b/snow/networking/router/subnet_router.go @@ -5,11 +5,13 @@ package router import ( "sync" + "time" "github.com/ava-labs/gecko/ids" "github.com/ava-labs/gecko/snow/networking/handler" "github.com/ava-labs/gecko/snow/networking/timeout" "github.com/ava-labs/gecko/utils/logging" + "github.com/ava-labs/gecko/utils/timer" ) // ChainRouter routes incoming messages from the validator network @@ -21,15 +23,17 @@ type ChainRouter struct { lock sync.RWMutex chains map[[32]byte]*handler.Handler timeouts *timeout.Manager + gossiper *timer.Repeater } // Initialize the router // When this router receives an incoming message, it cancels the timeout in [timeouts] // associated with the request that caused the incoming message, if applicable -func (sr *ChainRouter) Initialize(log logging.Logger, timeouts *timeout.Manager) { +func (sr *ChainRouter) Initialize(log logging.Logger, timeouts *timeout.Manager, gossipFrequency time.Duration) { sr.log = log sr.chains = make(map[[32]byte]*handler.Handler) sr.timeouts = timeouts + sr.gossiper = timer.NewRepeater(sr.Gossip, gossipFrequency) } // AddChain registers the specified chain so that incoming @@ -256,3 +260,17 @@ func (sr *ChainRouter) shutdown() { chain.Shutdown() } } + +// Gossip accepted containers +func (sr *ChainRouter) Gossip() { + sr.lock.RLock() + defer sr.lock.RUnlock() + + sr.gossip() +} + +func (sr *ChainRouter) gossip() { + for _, chain := range sr.chains { + chain.Gossip() + } +} diff --git a/snow/networking/sender/external_sender.go b/snow/networking/sender/external_sender.go index 6bb02db..618ddd0 100644 --- a/snow/networking/sender/external_sender.go +++ b/snow/networking/sender/external_sender.go @@ -20,4 +20,6 @@ type ExternalSender interface { PushQuery(validatorIDs ids.ShortSet, chainID ids.ID, requestID uint32, containerID ids.ID, container []byte) PullQuery(validatorIDs ids.ShortSet, chainID ids.ID, requestID uint32, containerID ids.ID) Chits(validatorID ids.ShortID, chainID ids.ID, requestID uint32, votes ids.Set) + + Gossip(chainID ids.ID, containerID ids.ID, container []byte) } diff --git a/snow/networking/sender/sender.go b/snow/networking/sender/sender.go index f72c842..ed500c3 100644 --- a/snow/networking/sender/sender.go +++ b/snow/networking/sender/sender.go @@ -163,3 +163,9 @@ func (s *Sender) Chits(validatorID ids.ShortID, requestID uint32, votes ids.Set) } s.sender.Chits(validatorID, s.ctx.ChainID, requestID, votes) } + +// Gossip the provided container +func (s *Sender) Gossip(containerID ids.ID, container []byte) { + s.ctx.Log.Verbo("Gossiping %s", containerID) + s.sender.Gossip(s.ctx.ChainID, containerID, container) +} diff --git a/snow/networking/sender/sender_test.go b/snow/networking/sender/sender_test.go index 3d6e3af..5a0ef83 100644 --- a/snow/networking/sender/sender_test.go +++ b/snow/networking/sender/sender_test.go @@ -38,7 +38,7 @@ func TestTimeout(t *testing.T) { go tm.Dispatch() router := router.ChainRouter{} - router.Initialize(logging.NoLog{}, &tm) + router.Initialize(logging.NoLog{}, &tm, time.Hour) sender := Sender{} sender.Initialize(snow.DefaultContextTest(), &ExternalSenderTest{}, &router, &tm) diff --git a/snow/networking/sender/test_external_sender.go b/snow/networking/sender/test_external_sender.go index aabe8cc..2bbfecf 100644 --- a/snow/networking/sender/test_external_sender.go +++ b/snow/networking/sender/test_external_sender.go @@ -17,7 +17,8 @@ type ExternalSenderTest struct { CantGetAcceptedFrontier, CantAcceptedFrontier, CantGetAccepted, CantAccepted, CantGet, CantPut, - CantPullQuery, CantPushQuery, CantChits bool + CantPullQuery, CantPushQuery, CantChits, + CantGossip bool GetAcceptedFrontierF func(validatorIDs ids.ShortSet, chainID ids.ID, requestID uint32) AcceptedFrontierF func(validatorID ids.ShortID, chainID ids.ID, requestID uint32, containerIDs ids.Set) @@ -28,6 +29,7 @@ type ExternalSenderTest struct { PushQueryF func(validatorIDs ids.ShortSet, chainID ids.ID, requestID uint32, containerID ids.ID, container []byte) PullQueryF func(validatorIDs ids.ShortSet, chainID ids.ID, requestID uint32, containerID ids.ID) ChitsF func(validatorID ids.ShortID, chainID ids.ID, requestID uint32, votes ids.Set) + GossipF func(chainID ids.ID, containerID ids.ID, container []byte) } // Default set the default callable value to [cant] @@ -41,6 +43,7 @@ func (s *ExternalSenderTest) Default(cant bool) { s.CantPullQuery = cant s.CantPushQuery = cant s.CantChits = cant + s.CantGossip = cant } // GetAcceptedFrontier calls GetAcceptedFrontierF if it was initialized. If it @@ -159,3 +162,16 @@ func (s *ExternalSenderTest) Chits(vdr ids.ShortID, chainID ids.ID, requestID ui s.B.Fatalf("Unexpectedly called Chits") } } + +// Gossip calls GossipF if it was initialized. If it wasn't initialized and this +// function shouldn't be called and testing was initialized, then testing will +// fail. +func (s *ExternalSenderTest) Gossip(chainID ids.ID, containerID ids.ID, container []byte) { + if s.GossipF != nil { + s.GossipF(chainID, containerID, container) + } else if s.CantGossip && s.T != nil { + s.T.Fatalf("Unexpectedly called Gossip") + } else if s.CantGossip && s.B != nil { + s.B.Fatalf("Unexpectedly called Gossip") + } +} From 13fbe14d44ca71917be4d141860fb920f6018c39 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 4 May 2020 02:44:56 -0400 Subject: [PATCH 15/24] Started gossiping thread --- snow/engine/avalanche/transitive.go | 2 +- snow/engine/snowman/transitive.go | 2 +- snow/networking/router/subnet_router.go | 3 +++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/snow/engine/avalanche/transitive.go b/snow/engine/avalanche/transitive.go index 4b33007..56f9a21 100644 --- a/snow/engine/avalanche/transitive.go +++ b/snow/engine/avalanche/transitive.go @@ -78,7 +78,7 @@ func (t *Transitive) Gossip() { return } - t.Config.Context.Log.Info("Gossiping %s as accepted to the network", vtxID) + t.Config.Context.Log.Debug("Gossiping %s as accepted to the network", vtxID) t.Config.Sender.Gossip(vtxID, vtx.Bytes()) } diff --git a/snow/engine/snowman/transitive.go b/snow/engine/snowman/transitive.go index f4778ac..4bea6c5 100644 --- a/snow/engine/snowman/transitive.go +++ b/snow/engine/snowman/transitive.go @@ -74,7 +74,7 @@ func (t *Transitive) Gossip() { return } - t.Config.Context.Log.Info("Gossiping %s as accepted to the network", blkID) + t.Config.Context.Log.Debug("Gossiping %s as accepted to the network", blkID) t.Config.Sender.Gossip(blkID, blk.Bytes()) } diff --git a/snow/networking/router/subnet_router.go b/snow/networking/router/subnet_router.go index 24b6efb..f1ac4e7 100644 --- a/snow/networking/router/subnet_router.go +++ b/snow/networking/router/subnet_router.go @@ -34,6 +34,8 @@ func (sr *ChainRouter) Initialize(log logging.Logger, timeouts *timeout.Manager, sr.chains = make(map[[32]byte]*handler.Handler) sr.timeouts = timeouts sr.gossiper = timer.NewRepeater(sr.Gossip, gossipFrequency) + + go log.RecoverAndPanic(sr.gossiper.Dispatch) } // AddChain registers the specified chain so that incoming @@ -259,6 +261,7 @@ func (sr *ChainRouter) shutdown() { for _, chain := range sr.chains { chain.Shutdown() } + sr.gossiper.Stop() } // Gossip accepted containers From 0ea445a2d18a9de05d4d43ba375e4c6cd1fe0aa6 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 4 May 2020 02:57:02 -0400 Subject: [PATCH 16/24] Added gossip tests --- snow/engine/avalanche/transitive_test.go | 51 ++++++++++++++++++++++++ snow/engine/snowman/transitive_test.go | 33 +++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/snow/engine/avalanche/transitive_test.go b/snow/engine/avalanche/transitive_test.go index 604afaf..4c95c89 100644 --- a/snow/engine/avalanche/transitive_test.go +++ b/snow/engine/avalanche/transitive_test.go @@ -2536,3 +2536,54 @@ func TestEnginePartiallyValidVertex(t *testing.T) { te.insert(vtx) } + +func TestEngineGossip(t *testing.T) { + config := DefaultConfig() + + sender := &common.SenderTest{} + sender.T = t + config.Sender = sender + + sender.Default(true) + + st := &stateTest{t: t} + config.State = st + + gVtx := &Vtx{ + id: GenerateID(), + status: choices.Accepted, + } + + te := &Transitive{} + te.Initialize(config) + te.finishBootstrapping() + + st.edge = func() []ids.ID { return []ids.ID{gVtx.ID()} } + st.getVertex = func(vtxID ids.ID) (avalanche.Vertex, error) { + switch { + case vtxID.Equals(gVtx.ID()): + return gVtx, nil + } + t.Fatal(errUnknownVertex) + return nil, errUnknownVertex + } + + called := new(bool) + sender.GossipF = func(vtxID ids.ID, vtxBytes []byte) { + *called = true + switch { + case !vtxID.Equals(gVtx.ID()): + t.Fatal(errUnknownVertex) + } + switch { + case !bytes.Equal(vtxBytes, gVtx.Bytes()): + t.Fatal(errUnknownVertex) + } + } + + te.Gossip() + + if !*called { + t.Fatalf("Should have gossiped the vertex") + } +} diff --git a/snow/engine/snowman/transitive_test.go b/snow/engine/snowman/transitive_test.go index a1875d8..047d689 100644 --- a/snow/engine/snowman/transitive_test.go +++ b/snow/engine/snowman/transitive_test.go @@ -1187,3 +1187,36 @@ func TestEngineUndeclaredDependencyDeadlock(t *testing.T) { t.Fatalf("Should have bubbled invalid votes to the valid parent") } } + +func TestEngineGossip(t *testing.T) { + _, _, sender, vm, te, gBlk := setup(t) + + vm.LastAcceptedF = func() ids.ID { return gBlk.ID() } + vm.GetBlockF = func(blkID ids.ID) (snowman.Block, error) { + switch { + case blkID.Equals(gBlk.ID()): + return gBlk, nil + } + t.Fatal(errUnknownBlock) + return nil, errUnknownBlock + } + + called := new(bool) + sender.GossipF = func(blkID ids.ID, blkBytes []byte) { + *called = true + switch { + case !blkID.Equals(gBlk.ID()): + t.Fatal(errUnknownBlock) + } + switch { + case !bytes.Equal(blkBytes, gBlk.Bytes()): + t.Fatal(errUnknownBytes) + } + } + + te.Gossip() + + if !*called { + t.Fatalf("Should have gossiped the block") + } +} From dfbb17aaedcd65f4eae28fc5a6b6b464e59f13b1 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 4 May 2020 13:59:25 -0400 Subject: [PATCH 17/24] Added gossip params to tests --- vms/platformvm/vm_test.go | 2 +- vms/spchainvm/consensus_benchmark_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/vms/platformvm/vm_test.go b/vms/platformvm/vm_test.go index 28be3fc..c32d8bd 100644 --- a/vms/platformvm/vm_test.go +++ b/vms/platformvm/vm_test.go @@ -1510,7 +1510,7 @@ func TestBootstrapPartiallyAccepted(t *testing.T) { go timeoutManager.Dispatch() router := &router.ChainRouter{} - router.Initialize(logging.NoLog{}, &timeoutManager) + router.Initialize(logging.NoLog{}, &timeoutManager, time.Hour) externalSender := &sender.ExternalSenderTest{T: t} externalSender.Default(true) diff --git a/vms/spchainvm/consensus_benchmark_test.go b/vms/spchainvm/consensus_benchmark_test.go index aa80e6d..1a33502 100644 --- a/vms/spchainvm/consensus_benchmark_test.go +++ b/vms/spchainvm/consensus_benchmark_test.go @@ -62,7 +62,7 @@ func ConsensusLeader(numBlocks, numTxsPerBlock int, b *testing.B) { go timeoutManager.Dispatch() router := &router.ChainRouter{} - router.Initialize(logging.NoLog{}, &timeoutManager) + router.Initialize(logging.NoLog{}, &timeoutManager, time.Hour) // Initialize the VM vm := &VM{} @@ -189,7 +189,7 @@ func ConsensusFollower(numBlocks, numTxsPerBlock int, b *testing.B) { go timeoutManager.Dispatch() router := &router.ChainRouter{} - router.Initialize(logging.NoLog{}, &timeoutManager) + router.Initialize(logging.NoLog{}, &timeoutManager, time.Hour) wg := sync.WaitGroup{} wg.Add(numBlocks) From 5d115a0b62a103231f593963968904ae1e192f9d Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Mon, 4 May 2020 17:16:00 -0400 Subject: [PATCH 18/24] %v --> %w --- vms/platformvm/service.go | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/vms/platformvm/service.go b/vms/platformvm/service.go index 6e3c4e2..bd6c4cb 100644 --- a/vms/platformvm/service.go +++ b/vms/platformvm/service.go @@ -77,7 +77,7 @@ type GetSubnetsResponse struct { func (service *Service) GetSubnets(_ *http.Request, args *GetSubnetsArgs, response *GetSubnetsResponse) error { subnets, err := service.vm.getSubnets(service.vm.DB) // all subnets if err != nil { - return fmt.Errorf("error getting subnets from database: %v", err) + return fmt.Errorf("error getting subnets from database: %w", err) } getAll := len(args.IDs) == 0 @@ -279,7 +279,7 @@ type GetAccountReply struct { func (service *Service) GetAccount(_ *http.Request, args *GetAccountArgs, reply *GetAccountReply) error { account, err := service.vm.getAccount(service.vm.DB, args.Address) if err != nil && err != database.ErrNotFound { - return fmt.Errorf("couldn't get account: %v", err) + return fmt.Errorf("couldn't get account: %w", err) } else if err == database.ErrNotFound { account = newAccount(args.Address, 0, 0) } @@ -309,7 +309,7 @@ func (service *Service) ListAccounts(_ *http.Request, args *ListAccountsArgs, re // db holds the user's info that pertains to the Platform Chain userDB, err := service.vm.Ctx.Keystore.GetDatabase(args.Username, args.Password) if err != nil { - return fmt.Errorf("couldn't get user: %v", err) + return fmt.Errorf("couldn't get user: %w", err) } // The user @@ -320,14 +320,14 @@ func (service *Service) ListAccounts(_ *http.Request, args *ListAccountsArgs, re // IDs of accounts controlled by this user accountIDs, err := user.getAccountIDs() if err != nil { - return fmt.Errorf("couldn't get accounts held by user: %v", err) + return fmt.Errorf("couldn't get accounts held by user: %w", err) } reply.Accounts = []APIAccount{} for _, accountID := range accountIDs { account, err := service.vm.getAccount(service.vm.DB, accountID) // Get account whose ID is [accountID] if err != nil && err != database.ErrNotFound { - service.vm.Ctx.Log.Error("couldn't get account from database: %v", err) + service.vm.Ctx.Log.Error("couldn't get account from database: %w", err) continue } else if err == database.ErrNotFound { account = newAccount(accountID, 0, 0) @@ -371,7 +371,7 @@ func (service *Service) CreateAccount(_ *http.Request, args *CreateAccountArgs, // userDB holds the user's info that pertains to the Platform Chain userDB, err := service.vm.Ctx.Keystore.GetDatabase(args.Username, args.Password) if err != nil { - return fmt.Errorf("couldn't get user: %v", err) + return fmt.Errorf("couldn't get user: %w", err) } // The user creating a new account @@ -747,7 +747,7 @@ func (service *Service) signAddDefaultSubnetValidatorTx(tx *addDefaultSubnetVali unsignedIntf := interface{}(&tx.UnsignedAddDefaultSubnetValidatorTx) unsignedTxBytes, err := Codec.Marshal(&unsignedIntf) if err != nil { - return nil, fmt.Errorf("error serializing unsigned tx: %v", err) + return nil, fmt.Errorf("error serializing unsigned tx: %w", err) } sig, err := key.Sign(unsignedTxBytes) @@ -770,7 +770,7 @@ func (service *Service) signAddDefaultSubnetDelegatorTx(tx *addDefaultSubnetDele unsignedIntf := interface{}(&tx.UnsignedAddDefaultSubnetDelegatorTx) unsignedTxBytes, err := Codec.Marshal(&unsignedIntf) if err != nil { - return nil, fmt.Errorf("error serializing unsigned tx: %v", err) + return nil, fmt.Errorf("error serializing unsigned tx: %w", err) } sig, err := key.Sign(unsignedTxBytes) @@ -793,7 +793,7 @@ func (service *Service) signCreateSubnetTx(tx *CreateSubnetTx, key *crypto.Priva unsignedIntf := interface{}(&tx.UnsignedCreateSubnetTx) unsignedTxBytes, err := Codec.Marshal(&unsignedIntf) if err != nil { - return nil, fmt.Errorf("error serializing unsigned tx: %v", err) + return nil, fmt.Errorf("error serializing unsigned tx: %w", err) } sig, err := key.Sign(unsignedTxBytes) @@ -816,7 +816,7 @@ func (service *Service) signExportTx(tx *ExportTx, key *crypto.PrivateKeySECP256 unsignedIntf := interface{}(&tx.UnsignedExportTx) unsignedTxBytes, err := Codec.Marshal(&unsignedIntf) if err != nil { - return nil, fmt.Errorf("error serializing unsigned tx: %v", err) + return nil, fmt.Errorf("error serializing unsigned tx: %w", err) } sig, err := key.Sign(unsignedTxBytes) @@ -844,7 +844,7 @@ func (service *Service) signAddNonDefaultSubnetValidatorTx(tx *addNonDefaultSubn unsignedIntf := interface{}(&tx.UnsignedAddNonDefaultSubnetValidatorTx) unsignedTxBytes, err := Codec.Marshal(&unsignedIntf) if err != nil { - return nil, fmt.Errorf("error serializing unsigned tx: %v", err) + return nil, fmt.Errorf("error serializing unsigned tx: %w", err) } sig, err := key.Sign(unsignedTxBytes) if err != nil { @@ -857,7 +857,7 @@ func (service *Service) signAddNonDefaultSubnetValidatorTx(tx *addNonDefaultSubn // Get information about the subnet subnet, err := service.vm.getSubnet(service.vm.DB, tx.SubnetID()) if err != nil { - return nil, fmt.Errorf("problem getting subnet information: %v", err) + return nil, fmt.Errorf("problem getting subnet information: %w", err) } // Find the location at which [key] should put its signature. @@ -911,7 +911,7 @@ func (service *Service) ImportAVA(_ *http.Request, args *ImportAVAArgs, response // Get the key of the Signer db, err := service.vm.Ctx.Keystore.GetDatabase(args.Username, args.Password) if err != nil { - return fmt.Errorf("couldn't get user: %v", err) + return fmt.Errorf("couldn't get user: %w", err) } user := user{db: db} @@ -1024,7 +1024,7 @@ func (service *Service) signCreateChainTx(tx *CreateChainTx, key *crypto.Private unsignedIntf := interface{}(&tx.UnsignedCreateChainTx) unsignedTxBytes, err := Codec.Marshal(&unsignedIntf) if err != nil { - return nil, fmt.Errorf("error serializing unsigned tx: %v", err) + return nil, fmt.Errorf("error serializing unsigned tx: %w", err) } sig, err := key.Sign(unsignedTxBytes) if err != nil { @@ -1037,7 +1037,7 @@ func (service *Service) signCreateChainTx(tx *CreateChainTx, key *crypto.Private // Get information about the subnet subnet, err := service.vm.getSubnet(service.vm.DB, tx.SubnetID) if err != nil { - return nil, fmt.Errorf("problem getting subnet information: %v", err) + return nil, fmt.Errorf("problem getting subnet information: %w", err) } // Find the location at which [key] should put its signature. @@ -1198,7 +1198,7 @@ func (service *Service) CreateBlockchain(_ *http.Request, args *CreateBlockchain txBytes, err := Codec.Marshal(genericTx{Tx: &tx}) if err != nil { - service.vm.Ctx.Log.Error("problem marshaling createChainTx: %v", err) + service.vm.Ctx.Log.Error("problem marshaling createChainTx: %w", err) return errCreatingTransaction } @@ -1379,7 +1379,7 @@ func (service *Service) GetBlockchains(_ *http.Request, args *struct{}, response chains, err := service.vm.getChains(service.vm.DB) if err != nil { - return fmt.Errorf("couldn't retrieve blockchains: %v", err) + return fmt.Errorf("couldn't retrieve blockchains: %w", err) } for _, chain := range chains { From c56870045ecdf22c2705c702090c3e735757103f Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 4 May 2020 18:12:34 -0400 Subject: [PATCH 19/24] fixed logging message --- networking/voting_handlers.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/networking/voting_handlers.go b/networking/voting_handlers.go index f6a43e3..2e475a6 100644 --- a/networking/voting_handlers.go +++ b/networking/voting_handlers.go @@ -436,8 +436,8 @@ func (s *Voting) gossip(chainID, containerID ids.ID, container []byte) error { return fmt.Errorf("Attempted to pack too large of a Put message.\nContainer length: %d: %w", len(container), err) } - s.log.Verbo("Sending a Put message to non-validators."+ - "\nNumber of Non-Validators: %d"+ + s.log.Verbo("Sending a Put message to peers."+ + "\nNumber of Peers: %d"+ "\nChain: %s"+ "\nContainer ID: %s"+ "\nContainer:\n%s", From 8f91f7294bc0f8516e756e3802c241cbee31a1b0 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 4 May 2020 23:32:54 -0400 Subject: [PATCH 20/24] Added gossip frequency docs --- snow/networking/router/subnet_router.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/snow/networking/router/subnet_router.go b/snow/networking/router/subnet_router.go index f1ac4e7..6482966 100644 --- a/snow/networking/router/subnet_router.go +++ b/snow/networking/router/subnet_router.go @@ -26,9 +26,14 @@ type ChainRouter struct { gossiper *timer.Repeater } -// Initialize the router -// When this router receives an incoming message, it cancels the timeout in [timeouts] -// associated with the request that caused the incoming message, if applicable +// Initialize the router. +// +// When this router receives an incoming message, it cancels the timeout in +// [timeouts] associated with the request that caused the incoming message, if +// applicable. +// +// This router also fires a gossip event every [gossipFrequency] to the engine, +// notifying the engine it should gossip it's accepted set. func (sr *ChainRouter) Initialize(log logging.Logger, timeouts *timeout.Manager, gossipFrequency time.Duration) { sr.log = log sr.chains = make(map[[32]byte]*handler.Handler) From 940f9a2fb8cd0471278842c5d730d79d2aa888fc Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Tue, 5 May 2020 17:45:23 -0400 Subject: [PATCH 21/24] make error message more clear when an argument can't be unmarshaled --- utils/json/codec.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/utils/json/codec.go b/utils/json/codec.go index 3058007..15e38fd 100644 --- a/utils/json/codec.go +++ b/utils/json/codec.go @@ -50,3 +50,10 @@ func (r *request) Method() (string, error) { uppercaseRune := string(unicode.ToUpper(firstRune)) return fmt.Sprintf("%s.%s%s", class, string(uppercaseRune), function[runeLen:]), nil } + +func (r *request) ReadRequest(args interface{}) error { + if err := r.CodecRequest.ReadRequest(args); err != nil { + return errors.New("couldn't unmarshal an argument. Ensure arguments are valid and properly formatted. See documentation for example calls") + } + return nil +} From ac27d66c02c6dc5199b71affc0a58fef5bf51af4 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Sun, 10 May 2020 15:26:41 -0400 Subject: [PATCH 22/24] Fixed platformvm locking --- vms/platformvm/vm_test.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/vms/platformvm/vm_test.go b/vms/platformvm/vm_test.go index c32d8bd..d45602c 100644 --- a/vms/platformvm/vm_test.go +++ b/vms/platformvm/vm_test.go @@ -1271,6 +1271,8 @@ func TestRestartPartiallyAccepted(t *testing.T) { firstVM.clock.Set(defaultGenesisTime) firstCtx := defaultContext() + firstCtx.Lock.Lock() + firstMsgChan := make(chan common.Message, 1) if err := firstVM.Initialize(firstCtx, db, genesisBytes, firstMsgChan, nil); err != nil { t.Fatal(err) @@ -1318,6 +1320,7 @@ func TestRestartPartiallyAccepted(t *testing.T) { } firstVM.Shutdown() + firstCtx.Lock.Unlock() secondVM := &VM{ SnowmanVM: &core.SnowmanVM{}, @@ -1330,6 +1333,9 @@ func TestRestartPartiallyAccepted(t *testing.T) { secondVM.clock.Set(defaultGenesisTime) secondCtx := defaultContext() + secondCtx.Lock.Lock() + defer secondCtx.Lock.Unlock() + secondMsgChan := make(chan common.Message, 1) if err := secondVM.Initialize(secondCtx, db, genesisBytes, secondMsgChan, nil); err != nil { t.Fatal(err) @@ -1371,6 +1377,8 @@ func TestRestartFullyAccepted(t *testing.T) { firstVM.clock.Set(defaultGenesisTime) firstCtx := defaultContext() + firstCtx.Lock.Lock() + firstMsgChan := make(chan common.Message, 1) if err := firstVM.Initialize(firstCtx, db, genesisBytes, firstMsgChan, nil); err != nil { t.Fatal(err) @@ -1418,6 +1426,7 @@ func TestRestartFullyAccepted(t *testing.T) { } firstVM.Shutdown() + firstCtx.Lock.Unlock() secondVM := &VM{ SnowmanVM: &core.SnowmanVM{}, @@ -1430,6 +1439,9 @@ func TestRestartFullyAccepted(t *testing.T) { secondVM.clock.Set(defaultGenesisTime) secondCtx := defaultContext() + secondCtx.Lock.Lock() + defer secondCtx.Lock.Unlock() + secondMsgChan := make(chan common.Message, 1) if err := secondVM.Initialize(secondCtx, db, genesisBytes, secondMsgChan, nil); err != nil { t.Fatal(err) @@ -1471,7 +1483,6 @@ func TestBootstrapPartiallyAccepted(t *testing.T) { SnowmanVM: &core.SnowmanVM{}, chainManager: chains.MockManager{}, } - defer vm.Shutdown() defaultSubnet := validators.NewSet() vm.validators = validators.NewManager() @@ -1479,9 +1490,9 @@ func TestBootstrapPartiallyAccepted(t *testing.T) { vm.clock.Set(defaultGenesisTime) ctx := defaultContext() - msgChan := make(chan common.Message, 1) - ctx.Lock.Lock() + + msgChan := make(chan common.Message, 1) if err := vm.Initialize(ctx, vmDB, genesisBytes, msgChan, nil); err != nil { t.Fatal(err) } From 8dd5f21847750740e508acd6fb185c5ec9b0bb8e Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Sun, 10 May 2020 17:03:12 -0400 Subject: [PATCH 23/24] clean up locking in the AVM / platformVM tests --- vms/avm/base_tx_test.go | 76 +++++++---- vms/avm/import_tx_test.go | 7 +- vms/avm/prefixed_state_test.go | 26 ++-- vms/avm/service_test.go | 48 ++++--- vms/avm/state_test.go | 32 ++--- vms/avm/vm_test.go | 59 +++++---- .../add_default_subnet_delegator_tx_test.go | 12 +- .../add_default_subnet_validator_tx_test.go | 18 ++- ...add_nondefault_subnet_validator_tx_test.go | 19 ++- vms/platformvm/advance_time_tx_test.go | 39 +++++- vms/platformvm/create_chain_tx_test.go | 36 +++++- vms/platformvm/event_heap_test.go | 24 +++- vms/platformvm/reward_validator_tx_test.go | 18 ++- vms/platformvm/vm_test.go | 122 +++++++++++------- 14 files changed, 356 insertions(+), 180 deletions(-) diff --git a/vms/avm/base_tx_test.go b/vms/avm/base_tx_test.go index 78ebfa6..197b7fe 100644 --- a/vms/avm/base_tx_test.go +++ b/vms/avm/base_tx_test.go @@ -623,8 +623,10 @@ func TestBaseTxSyntacticVerifyUninitialized(t *testing.T) { func TestBaseTxSemanticVerify(t *testing.T) { genesisBytes, _, vm := GenesisVM(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() genesisTx := GetFirstTxFromGenesisTest(genesisBytes, t) @@ -688,8 +690,10 @@ func TestBaseTxSemanticVerify(t *testing.T) { func TestBaseTxSemanticVerifyUnknownFx(t *testing.T) { genesisBytes, _, vm := GenesisVM(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() vm.codec.RegisterType(&ava.TestVerifiable{}) @@ -738,8 +742,10 @@ func TestBaseTxSemanticVerifyUnknownFx(t *testing.T) { func TestBaseTxSemanticVerifyWrongAssetID(t *testing.T) { genesisBytes, _, vm := GenesisVM(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() vm.codec.RegisterType(&ava.TestVerifiable{}) @@ -804,15 +810,15 @@ func TestBaseTxSemanticVerifyWrongAssetID(t *testing.T) { } func TestBaseTxSemanticVerifyUnauthorizedFx(t *testing.T) { - genesisBytes := BuildGenesisTest(t) - - issuer := make(chan common.Message, 1) - - ctx.Lock.Lock() - defer ctx.Lock.Unlock() - vm := &VM{} - defer vm.Shutdown() + ctx.Lock.Lock() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() + + genesisBytes := BuildGenesisTest(t) + issuer := make(chan common.Message, 1) err := vm.Initialize( ctx, memdb.New(), @@ -897,8 +903,10 @@ func TestBaseTxSemanticVerifyUnauthorizedFx(t *testing.T) { func TestBaseTxSemanticVerifyInvalidSignature(t *testing.T) { genesisBytes, _, vm := GenesisVM(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() genesisTx := GetFirstTxFromGenesisTest(genesisBytes, t) @@ -949,8 +957,10 @@ func TestBaseTxSemanticVerifyInvalidSignature(t *testing.T) { func TestBaseTxSemanticVerifyMissingUTXO(t *testing.T) { genesisBytes, _, vm := GenesisVM(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() genesisTx := GetFirstTxFromGenesisTest(genesisBytes, t) @@ -1014,8 +1024,10 @@ func TestBaseTxSemanticVerifyMissingUTXO(t *testing.T) { func TestBaseTxSemanticVerifyInvalidUTXO(t *testing.T) { genesisBytes, _, vm := GenesisVM(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() genesisTx := GetFirstTxFromGenesisTest(genesisBytes, t) @@ -1147,8 +1159,10 @@ func TestBaseTxSemanticVerifyPendingInvalidUTXO(t *testing.T) { <-issuer ctx.Lock.Lock() - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() vm.PendingTxs() @@ -1279,8 +1293,10 @@ func TestBaseTxSemanticVerifyPendingWrongAssetID(t *testing.T) { <-issuer ctx.Lock.Lock() - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() vm.PendingTxs() @@ -1445,8 +1461,10 @@ func TestBaseTxSemanticVerifyPendingUnauthorizedFx(t *testing.T) { <-issuer ctx.Lock.Lock() - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() vm.PendingTxs() @@ -1595,8 +1613,10 @@ func TestBaseTxSemanticVerifyPendingInvalidSignature(t *testing.T) { <-issuer ctx.Lock.Lock() - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() vm.PendingTxs() diff --git a/vms/avm/import_tx_test.go b/vms/avm/import_tx_test.go index 2228977..9cdecdb 100644 --- a/vms/avm/import_tx_test.go +++ b/vms/avm/import_tx_test.go @@ -220,16 +220,17 @@ func TestIssueImportTx(t *testing.T) { if _, err := vm.IssueTx(tx.Bytes(), nil); err != nil { t.Fatalf("should have issued the transaction correctly but errored: %s", err) } - ctx.Lock.Unlock() - defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() msg := <-issuer if msg != common.PendingTxs { t.Fatalf("Wrong message") } - // FIXME?: Is it safe to call vm.PendingTXs() called without the lock? + ctx.Lock.Lock() + defer ctx.Lock.Unlock() + defer vm.Shutdown() + txs := vm.PendingTxs() if len(txs) != 1 { t.Fatalf("Should have returned %d tx(s)", 1) diff --git a/vms/avm/prefixed_state_test.go b/vms/avm/prefixed_state_test.go index ad160e9..4eea404 100644 --- a/vms/avm/prefixed_state_test.go +++ b/vms/avm/prefixed_state_test.go @@ -17,13 +17,13 @@ import ( func TestPrefixedSetsAndGets(t *testing.T) { _, _, vm := GenesisVM(t) - ctx.Lock.Unlock() - defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() - // FIXME? is it safe to access vm.state.state without the lock? state := vm.state - // FIXME? is it safe to call vm.codec.RegisterType() without the lock? vm.codec.RegisterType(&ava.TestVerifiable{}) utxo := &ava.UTXO{ @@ -55,7 +55,6 @@ func TestPrefixedSetsAndGets(t *testing.T) { }}, }} - // FIXME? Is it safe to call vm.codec.Marshal() without the lock? unsignedBytes, err := vm.codec.Marshal(tx.UnsignedTx) if err != nil { t.Fatal(err) @@ -75,7 +74,6 @@ func TestPrefixedSetsAndGets(t *testing.T) { }, }) - // FIXME? Is it safe to call vm.codec.Marshal() without the lock? b, err := vm.codec.Marshal(tx) if err != nil { t.Fatal(err) @@ -118,13 +116,13 @@ func TestPrefixedSetsAndGets(t *testing.T) { func TestPrefixedFundingNoAddresses(t *testing.T) { _, _, vm := GenesisVM(t) - ctx.Lock.Unlock() - defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() - // FIXME? is it safe to access vm.state.state without the lock? state := vm.state - // FIXME? is it safe to call vm.codec.RegisterType() without the lock? vm.codec.RegisterType(&ava.TestVerifiable{}) utxo := &ava.UTXO{ @@ -146,13 +144,13 @@ func TestPrefixedFundingNoAddresses(t *testing.T) { func TestPrefixedFundingAddresses(t *testing.T) { _, _, vm := GenesisVM(t) - ctx.Lock.Unlock() - defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() - // FIXME? is it safe to access vm.state.state without the lock? state := vm.state - // FIXME? is it safe to call vm.codec.RegisterType() without the lock? vm.codec.RegisterType(&testAddressable{}) utxo := &ava.UTXO{ diff --git a/vms/avm/service_test.go b/vms/avm/service_test.go index b42f8e5..f36e962 100644 --- a/vms/avm/service_test.go +++ b/vms/avm/service_test.go @@ -21,8 +21,10 @@ func setup(t *testing.T) ([]byte, *VM, *Service) { func TestServiceIssueTx(t *testing.T) { genesisBytes, vm, s := setup(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() txArgs := &IssueTxArgs{} txReply := &IssueTxReply{} @@ -44,8 +46,10 @@ func TestServiceIssueTx(t *testing.T) { func TestServiceGetTxStatus(t *testing.T) { genesisBytes, vm, s := setup(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() statusArgs := &GetTxStatusArgs{} statusReply := &GetTxStatusReply{} @@ -85,8 +89,10 @@ func TestServiceGetTxStatus(t *testing.T) { func TestServiceGetUTXOsInvalidAddress(t *testing.T) { _, vm, s := setup(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() addr0 := keys[0].PublicKey().Address() tests := []struct { @@ -113,8 +119,10 @@ func TestServiceGetUTXOsInvalidAddress(t *testing.T) { func TestServiceGetUTXOs(t *testing.T) { _, vm, s := setup(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() addr0 := keys[0].PublicKey().Address() tests := []struct { @@ -163,8 +171,10 @@ func TestServiceGetUTXOs(t *testing.T) { func TestGetAssetDescription(t *testing.T) { genesisBytes, vm, s := setup(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() genesisTx := GetFirstTxFromGenesisTest(genesisBytes, t) @@ -188,8 +198,10 @@ func TestGetAssetDescription(t *testing.T) { func TestGetBalance(t *testing.T) { genesisBytes, vm, s := setup(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() genesisTx := GetFirstTxFromGenesisTest(genesisBytes, t) @@ -211,8 +223,10 @@ func TestGetBalance(t *testing.T) { func TestCreateFixedCapAsset(t *testing.T) { _, vm, s := setup(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() reply := CreateFixedCapAssetReply{} err := s.CreateFixedCapAsset(nil, &CreateFixedCapAssetArgs{ @@ -235,8 +249,10 @@ func TestCreateFixedCapAsset(t *testing.T) { func TestCreateVariableCapAsset(t *testing.T) { _, vm, s := setup(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() reply := CreateVariableCapAssetReply{} err := s.CreateVariableCapAsset(nil, &CreateVariableCapAssetArgs{ diff --git a/vms/avm/state_test.go b/vms/avm/state_test.go index 335292c..e0598cc 100644 --- a/vms/avm/state_test.go +++ b/vms/avm/state_test.go @@ -16,10 +16,11 @@ import ( func TestStateIDs(t *testing.T) { _, _, vm := GenesisVM(t) - defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() - ctx.Lock.Unlock() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() - // FIXME? is it safe to access vm.state.state without the lock? state := vm.state.state id0 := ids.NewID([32]byte{0xff, 0}) @@ -129,10 +130,11 @@ func TestStateIDs(t *testing.T) { func TestStateStatuses(t *testing.T) { _, _, vm := GenesisVM(t) - defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() - ctx.Lock.Unlock() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() - // FIXME? is it safe to access vm.state.state without the lock? state := vm.state.state if _, err := state.Status(ids.Empty); err == nil { @@ -181,13 +183,13 @@ func TestStateStatuses(t *testing.T) { func TestStateUTXOs(t *testing.T) { _, _, vm := GenesisVM(t) - defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() - ctx.Lock.Unlock() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() - // FIXME? is it safe to access vm.state.state without the lock? state := vm.state.state - // FIXME? Is it safe to call vm.codec.RegisterType() without the lock? vm.codec.RegisterType(&ava.TestVerifiable{}) if _, err := state.UTXO(ids.Empty); err == nil { @@ -256,13 +258,13 @@ func TestStateUTXOs(t *testing.T) { func TestStateTXs(t *testing.T) { _, _, vm := GenesisVM(t) - defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() - ctx.Lock.Unlock() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() - // FIXME? is it safe to access vm.state.state without the lock? state := vm.state.state - // FIXME? Is it safe to call vm.codec.RegisterType() without the lock? vm.codec.RegisterType(&ava.TestTransferable{}) if _, err := state.Tx(ids.Empty); err == nil { @@ -289,7 +291,6 @@ func TestStateTXs(t *testing.T) { }}, }} - // FIXME? Is it safe to call vm.codec.Marshal() without the lock? unsignedBytes, err := vm.codec.Marshal(tx.UnsignedTx) if err != nil { t.Fatal(err) @@ -309,7 +310,6 @@ func TestStateTXs(t *testing.T) { }, }) - // FIXME? Is it safe to call vm.codec.Marshal() without the lock? b, err := vm.codec.Marshal(tx) if err != nil { t.Fatal(err) diff --git a/vms/avm/vm_test.go b/vms/avm/vm_test.go index f495e87..c397f4e 100644 --- a/vms/avm/vm_test.go +++ b/vms/avm/vm_test.go @@ -460,7 +460,10 @@ func (tx *testTxBytes) UnsignedBytes() []byte { return tx.unsignedBytes } func TestIssueTx(t *testing.T) { genesisBytes, issuer, vm := GenesisVM(t) - defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() newTx := NewTx(t, genesisBytes, vm) @@ -477,8 +480,8 @@ func TestIssueTx(t *testing.T) { if msg != common.PendingTxs { t.Fatalf("Wrong message") } + ctx.Lock.Lock() - // FIXME? vm.PendingTxs called after lock released. if txs := vm.PendingTxs(); len(txs) != 1 { t.Fatalf("Should have returned %d tx(s)", 1) } @@ -508,7 +511,10 @@ func TestGenesisGetUTXOs(t *testing.T) { // transaction should be issued successfully. func TestIssueDependentTx(t *testing.T) { genesisBytes, issuer, vm := GenesisVM(t) - defer func() { ctx.Lock.Lock(); vm.Shutdown(); ctx.Lock.Unlock() }() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() genesisTx := GetFirstTxFromGenesisTest(genesisBytes, t) @@ -621,15 +627,14 @@ func TestIssueDependentTx(t *testing.T) { if err != nil { t.Fatal(err) } - ctx.Lock.Unlock() msg := <-issuer if msg != common.PendingTxs { t.Fatalf("Wrong message") } + ctx.Lock.Lock() - // FIXME? vm.PendingTxs called after lock released. if txs := vm.PendingTxs(); len(txs) != 2 { t.Fatalf("Should have returned %d tx(s)", 2) } @@ -637,15 +642,15 @@ func TestIssueDependentTx(t *testing.T) { // Test issuing a transaction that creates an NFT family func TestIssueNFT(t *testing.T) { - genesisBytes := BuildGenesisTest(t) - - issuer := make(chan common.Message, 1) - - ctx.Lock.Lock() - defer ctx.Lock.Unlock() - vm := &VM{} - defer vm.Shutdown() + ctx.Lock.Lock() + defer func() { + ctx.Lock.Unlock() + vm.Shutdown() + }() + + genesisBytes := BuildGenesisTest(t) + issuer := make(chan common.Message, 1) err := vm.Initialize( ctx, memdb.New(), @@ -796,15 +801,15 @@ func TestIssueNFT(t *testing.T) { // Test issuing a transaction that creates an Property family func TestIssueProperty(t *testing.T) { - genesisBytes := BuildGenesisTest(t) - - issuer := make(chan common.Message, 1) - - ctx.Lock.Lock() - defer ctx.Lock.Unlock() - vm := &VM{} - defer vm.Shutdown() + ctx.Lock.Lock() + defer func() { + ctx.Lock.Unlock() + vm.Shutdown() + }() + + genesisBytes := BuildGenesisTest(t) + issuer := make(chan common.Message, 1) err := vm.Initialize( ctx, memdb.New(), @@ -946,8 +951,10 @@ func TestIssueProperty(t *testing.T) { func TestVMFormat(t *testing.T) { _, _, vm := GenesisVM(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + ctx.Lock.Unlock() + vm.Shutdown() + }() tests := []struct { in string @@ -966,8 +973,10 @@ func TestVMFormat(t *testing.T) { func TestVMFormatAliased(t *testing.T) { _, _, vm := GenesisVM(t) - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + ctx.Lock.Unlock() + vm.Shutdown() + }() origAliases := ctx.BCLookup defer func() { ctx.BCLookup = origAliases }() diff --git a/vms/platformvm/add_default_subnet_delegator_tx_test.go b/vms/platformvm/add_default_subnet_delegator_tx_test.go index eda9049..65a0a71 100644 --- a/vms/platformvm/add_default_subnet_delegator_tx_test.go +++ b/vms/platformvm/add_default_subnet_delegator_tx_test.go @@ -13,7 +13,11 @@ import ( func TestAddDefaultSubnetDelegatorTxSyntacticVerify(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // Case 1: tx is nil var tx *addDefaultSubnetDelegatorTx @@ -154,7 +158,11 @@ func TestAddDefaultSubnetDelegatorTxSyntacticVerify(t *testing.T) { func TestAddDefaultSubnetDelegatorTxSemanticVerify(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // Case 1: Proposed validator currently validating default subnet // but stops validating non-default subnet after stops validating default subnet diff --git a/vms/platformvm/add_default_subnet_validator_tx_test.go b/vms/platformvm/add_default_subnet_validator_tx_test.go index ee863e8..34ff7ef 100644 --- a/vms/platformvm/add_default_subnet_validator_tx_test.go +++ b/vms/platformvm/add_default_subnet_validator_tx_test.go @@ -12,7 +12,11 @@ import ( func TestAddDefaultSubnetValidatorTxSyntacticVerify(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // Case 1: tx is nil var tx *addDefaultSubnetValidatorTx @@ -217,7 +221,11 @@ func TestAddDefaultSubnetValidatorTxSyntacticVerify(t *testing.T) { // Test AddDefaultSubnetValidatorTx.SemanticVerify func TestAddDefaultSubnetValidatorTxSemanticVerify(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // Case 1: Validator's start time too early tx, err := vm.newAddDefaultSubnetValidatorTx( @@ -283,9 +291,9 @@ func TestAddDefaultSubnetValidatorTxSemanticVerify(t *testing.T) { } startTime := defaultGenesisTime.Add(1 * time.Second) tx, err = vm.newAddDefaultSubnetValidatorTx( - defaultNonce+1, // nonce - defaultStakeAmount, // stake amount - uint64(startTime.Unix()), // start time + defaultNonce+1, // nonce + defaultStakeAmount, // stake amount + uint64(startTime.Unix()), // start time uint64(startTime.Add(MinimumStakingDuration).Unix()), // end time key.PublicKey().Address(), // node ID defaultKey.PublicKey().Address(), // destination diff --git a/vms/platformvm/add_nondefault_subnet_validator_tx_test.go b/vms/platformvm/add_nondefault_subnet_validator_tx_test.go index 43069cf..df012c7 100644 --- a/vms/platformvm/add_nondefault_subnet_validator_tx_test.go +++ b/vms/platformvm/add_nondefault_subnet_validator_tx_test.go @@ -14,7 +14,11 @@ import ( func TestAddNonDefaultSubnetValidatorTxSyntacticVerify(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // Case 1: tx is nil var tx *addNonDefaultSubnetValidatorTx @@ -203,7 +207,11 @@ func TestAddNonDefaultSubnetValidatorTxSyntacticVerify(t *testing.T) { func TestAddNonDefaultSubnetValidatorTxSemanticVerify(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // Case 1: Proposed validator currently validating default subnet // but stops validating non-default subnet after stops validating default subnet @@ -592,13 +600,16 @@ func TestAddNonDefaultSubnetValidatorTxSemanticVerify(t *testing.T) { if err == nil { t.Fatal("should have failed verification because validator already in pending validator set of the specified subnet") } - } // Test that marshalling/unmarshalling works func TestAddNonDefaultSubnetValidatorMarshal(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // valid tx tx, err := vm.newAddNonDefaultSubnetValidatorTx( diff --git a/vms/platformvm/advance_time_tx_test.go b/vms/platformvm/advance_time_tx_test.go index d051869..0874b09 100644 --- a/vms/platformvm/advance_time_tx_test.go +++ b/vms/platformvm/advance_time_tx_test.go @@ -17,7 +17,11 @@ func TestAdvanceTimeTxSyntacticVerify(t *testing.T) { // Case 2: Timestamp is ahead of synchrony bound vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() tx = &advanceTimeTx{ Time: uint64(defaultGenesisTime.Add(Delta).Add(1 * time.Second).Unix()), @@ -40,7 +44,11 @@ func TestAdvanceTimeTxSyntacticVerify(t *testing.T) { // Ensure semantic verification fails when proposed timestamp is at or before current timestamp func TestAdvanceTimeTxTimestampTooEarly(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() tx := &advanceTimeTx{ Time: uint64(defaultGenesisTime.Unix()), @@ -55,7 +63,7 @@ func TestAdvanceTimeTxTimestampTooEarly(t *testing.T) { // Ensure semantic verification fails when proposed timestamp is after next validator set change time func TestAdvanceTimeTxTimestampTooLate(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() // Case 1: Timestamp is after next validator start time // Add a pending validator @@ -98,9 +106,16 @@ func TestAdvanceTimeTxTimestampTooLate(t *testing.T) { if err == nil { t.Fatal("should've failed verification because proposed timestamp is after pending validator start time") } + vm.Shutdown() + vm.Ctx.Lock.Unlock() // Case 2: Timestamp is after next validator end time vm = defaultVM() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // fast forward clock to 10 seconds before genesis validators stop validating vm.clock.Set(defaultValidateEndTime.Add(-10 * time.Second)) @@ -121,7 +136,11 @@ func TestAdvanceTimeTxTimestampTooLate(t *testing.T) { // Ensure semantic verification updates the current and pending validator sets correctly func TestAdvanceTimeTxUpdateValidators(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // Case 1: Timestamp is after next validator start time // Add a pending validator @@ -201,7 +220,11 @@ func TestAdvanceTimeTxUpdateValidators(t *testing.T) { // Test method InitiallyPrefersCommit func TestAdvanceTimeTxInitiallyPrefersCommit(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // Proposed advancing timestamp to 1 second after current timestamp tx, err := vm.newAdvanceTimeTx(defaultGenesisTime.Add(1 * time.Second)) @@ -223,7 +246,11 @@ func TestAdvanceTimeTxInitiallyPrefersCommit(t *testing.T) { // Ensure marshaling/unmarshaling works func TestAdvanceTimeTxUnmarshal(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() tx, err := vm.newAdvanceTimeTx(defaultGenesisTime) if err != nil { diff --git a/vms/platformvm/create_chain_tx_test.go b/vms/platformvm/create_chain_tx_test.go index ae8694d..ef66bc2 100644 --- a/vms/platformvm/create_chain_tx_test.go +++ b/vms/platformvm/create_chain_tx_test.go @@ -14,7 +14,11 @@ import ( // test method SyntacticVerify func TestCreateChainTxSyntacticVerify(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // Case 1: tx is nil var tx *CreateChainTx @@ -143,7 +147,11 @@ func TestCreateChainTxSyntacticVerify(t *testing.T) { // Ensure SemanticVerify fails when there are not enough control sigs func TestCreateChainTxInsufficientControlSigs(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // Case 1: No control sigs (2 are needed) tx, err := vm.newCreateChainTx( @@ -191,7 +199,11 @@ func TestCreateChainTxInsufficientControlSigs(t *testing.T) { // Ensure SemanticVerify fails when an incorrect control signature is given func TestCreateChainTxWrongControlSig(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // Generate new, random key to sign tx with factory := crypto.FactorySECP256K1R{} @@ -225,7 +237,11 @@ func TestCreateChainTxWrongControlSig(t *testing.T) { // its validator set doesn't exist func TestCreateChainTxNoSuchSubnet(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() tx, err := vm.newCreateChainTx( defaultNonce+1, @@ -249,7 +265,11 @@ func TestCreateChainTxNoSuchSubnet(t *testing.T) { func TestCreateChainTxAlreadyExists(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // create a tx tx, err := vm.newCreateChainTx( @@ -281,7 +301,11 @@ func TestCreateChainTxAlreadyExists(t *testing.T) { // Ensure valid tx passes semanticVerify func TestCreateChainTxValid(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // create a valid tx tx, err := vm.newCreateChainTx( diff --git a/vms/platformvm/event_heap_test.go b/vms/platformvm/event_heap_test.go index 91570bd..a9d3775 100644 --- a/vms/platformvm/event_heap_test.go +++ b/vms/platformvm/event_heap_test.go @@ -11,7 +11,11 @@ import ( func TestTxHeapStart(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() txHeap := EventHeap{SortByStartTime: true} @@ -80,7 +84,11 @@ func TestTxHeapStart(t *testing.T) { func TestTxHeapStop(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() txHeap := EventHeap{} @@ -149,7 +157,11 @@ func TestTxHeapStop(t *testing.T) { func TestTxHeapStartValidatorVsDelegatorOrdering(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() txHeap := EventHeap{SortByStartTime: true} @@ -192,7 +204,11 @@ func TestTxHeapStartValidatorVsDelegatorOrdering(t *testing.T) { func TestTxHeapStopValidatorVsDelegatorOrdering(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() txHeap := EventHeap{} diff --git a/vms/platformvm/reward_validator_tx_test.go b/vms/platformvm/reward_validator_tx_test.go index 3de1989..c9cca73 100644 --- a/vms/platformvm/reward_validator_tx_test.go +++ b/vms/platformvm/reward_validator_tx_test.go @@ -18,7 +18,11 @@ func TestRewardValidatorTxSyntacticVerify(t *testing.T) { } vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() txID := ids.NewID([32]byte{1, 2, 3, 4, 5, 6, 7}) @@ -56,7 +60,11 @@ func TestRewardValidatorTxSyntacticVerify(t *testing.T) { func TestRewardValidatorTxSemanticVerify(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() var nextToRemove *addDefaultSubnetValidatorTx currentValidators, err := vm.getCurrentValidators(vm.DB, DefaultSubnetID) @@ -134,7 +142,11 @@ func TestRewardValidatorTxSemanticVerify(t *testing.T) { func TestRewardDelegatorTxSemanticVerify(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() keyIntf1, err := vm.factory.NewPrivateKey() if err != nil { diff --git a/vms/platformvm/vm_test.go b/vms/platformvm/vm_test.go index ad51e9e..70174e7 100644 --- a/vms/platformvm/vm_test.go +++ b/vms/platformvm/vm_test.go @@ -235,10 +235,13 @@ func GenesisCurrentValidators() *EventHeap { // Ensure genesis state is parsed from bytes and stored correctly func TestGenesis(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // Ensure the genesis block has been accepted and stored - // FIXME? Calling vm.LastAccepted() without the lock genesisBlockID := vm.LastAccepted() // lastAccepted should be ID of genesis block genesisBlock, err := vm.getBlock(genesisBlockID) if err != nil { @@ -306,7 +309,11 @@ func TestGenesis(t *testing.T) { // accept proposal to add validator to default subnet func TestAddDefaultSubnetValidatorCommit(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() startTime := defaultGenesisTime.Add(Delta).Add(1 * time.Second) endTime := startTime.Add(MinimumStakingDuration) @@ -331,12 +338,10 @@ func TestAddDefaultSubnetValidatorCommit(t *testing.T) { // trigger block creation vm.unissuedEvents.Add(tx) - vm.Ctx.Lock.Lock() blk, err := vm.BuildBlock() if err != nil { t.Fatal(err) } - vm.Ctx.Lock.Unlock() // Assert preferences are correct block := blk.(*ProposalBlock) @@ -375,7 +380,11 @@ func TestAddDefaultSubnetValidatorCommit(t *testing.T) { // Reject proposal to add validator to default subnet func TestAddDefaultSubnetValidatorReject(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() startTime := defaultGenesisTime.Add(Delta).Add(1 * time.Second) endTime := startTime.Add(MinimumStakingDuration) @@ -400,12 +409,10 @@ func TestAddDefaultSubnetValidatorReject(t *testing.T) { // trigger block creation vm.unissuedEvents.Add(tx) - vm.Ctx.Lock.Lock() blk, err := vm.BuildBlock() if err != nil { t.Fatal(err) } - vm.Ctx.Lock.Unlock() // Assert preferences are correct block := blk.(*ProposalBlock) @@ -448,7 +455,11 @@ func TestAddDefaultSubnetValidatorReject(t *testing.T) { // Accept proposal to add validator to non-default subnet func TestAddNonDefaultSubnetValidatorAccept(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() startTime := defaultValidateStartTime.Add(Delta).Add(1 * time.Second) endTime := startTime.Add(MinimumStakingDuration) @@ -473,12 +484,10 @@ func TestAddNonDefaultSubnetValidatorAccept(t *testing.T) { // trigger block creation vm.unissuedEvents.Add(tx) - vm.Ctx.Lock.Lock() blk, err := vm.BuildBlock() if err != nil { t.Fatal(err) } - vm.Ctx.Lock.Unlock() // Assert preferences are correct block := blk.(*ProposalBlock) @@ -521,7 +530,11 @@ func TestAddNonDefaultSubnetValidatorAccept(t *testing.T) { // Reject proposal to add validator to non-default subnet func TestAddNonDefaultSubnetValidatorReject(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() startTime := defaultValidateStartTime.Add(Delta).Add(1 * time.Second) endTime := startTime.Add(MinimumStakingDuration) @@ -548,12 +561,10 @@ func TestAddNonDefaultSubnetValidatorReject(t *testing.T) { // trigger block creation vm.unissuedEvents.Add(tx) - vm.Ctx.Lock.Lock() blk, err := vm.BuildBlock() if err != nil { t.Fatal(err) } - vm.Ctx.Lock.Unlock() // Assert preferences are correct block := blk.(*ProposalBlock) @@ -596,17 +607,19 @@ func TestAddNonDefaultSubnetValidatorReject(t *testing.T) { // Test case where default subnet validator rewarded func TestRewardValidatorAccept(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // Fast forward clock to time for genesis validators to leave vm.clock.Set(defaultValidateEndTime) - vm.Ctx.Lock.Lock() blk, err := vm.BuildBlock() // should contain proposal to advance time if err != nil { t.Fatal(err) } - vm.Ctx.Lock.Unlock() // Assert preferences are correct block := blk.(*ProposalBlock) @@ -643,12 +656,10 @@ func TestRewardValidatorAccept(t *testing.T) { t.Fatal("expected timestamp to have advanced") } - vm.Ctx.Lock.Lock() blk, err = vm.BuildBlock() // should contain proposal to reward genesis validator if err != nil { t.Fatal(err) } - vm.Ctx.Lock.Unlock() // Assert preferences are correct block = blk.(*ProposalBlock) @@ -689,17 +700,19 @@ func TestRewardValidatorAccept(t *testing.T) { // Test case where default subnet validator not rewarded func TestRewardValidatorReject(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() // Fast forward clock to time for genesis validators to leave vm.clock.Set(defaultValidateEndTime) - vm.Ctx.Lock.Lock() blk, err := vm.BuildBlock() // should contain proposal to advance time if err != nil { t.Fatal(err) } - vm.Ctx.Lock.Unlock() // Assert preferences are correct block := blk.(*ProposalBlock) @@ -736,12 +749,10 @@ func TestRewardValidatorReject(t *testing.T) { t.Fatal("expected timestamp to have advanced") } - vm.Ctx.Lock.Lock() blk, err = vm.BuildBlock() // should contain proposal to reward genesis validator if err != nil { t.Fatal(err) } - vm.Ctx.Lock.Unlock() // Assert preferences are correct block = blk.(*ProposalBlock) @@ -782,9 +793,12 @@ func TestRewardValidatorReject(t *testing.T) { // Ensure BuildBlock errors when there is no block to build func TestUnneededBuildBlock(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() - // FIXME? Calling vm.BuildBlock without the lock if _, err := vm.BuildBlock(); err == nil { t.Fatalf("Should have errored on BuildBlock") } @@ -793,7 +807,11 @@ func TestUnneededBuildBlock(t *testing.T) { // test acceptance of proposal to create a new chain func TestCreateChain(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() tx, err := vm.newCreateChainTx( defaultNonce+1, @@ -810,13 +828,11 @@ func TestCreateChain(t *testing.T) { t.Fatal(err) } - vm.Ctx.Lock.Lock() vm.unissuedDecisionTxs = append(vm.unissuedDecisionTxs, tx) blk, err := vm.BuildBlock() // should contain proposal to create chain if err != nil { t.Fatal(err) } - vm.Ctx.Lock.Unlock() if err := blk.Verify(); err != nil { t.Fatal(err) @@ -856,7 +872,11 @@ func TestCreateChain(t *testing.T) { // 4) Advance timestamp to validator's end time (removing validator from current) func TestCreateSubnet(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() createSubnetTx, err := vm.newCreateSubnetTx( testNetworkID, @@ -872,13 +892,11 @@ func TestCreateSubnet(t *testing.T) { t.Fatal(err) } - vm.Ctx.Lock.Lock() vm.unissuedDecisionTxs = append(vm.unissuedDecisionTxs, createSubnetTx) blk, err := vm.BuildBlock() // should contain proposal to create subnet if err != nil { t.Fatal(err) } - vm.Ctx.Lock.Unlock() if err := blk.Verify(); err != nil { t.Fatal(err) @@ -935,13 +953,11 @@ func TestCreateSubnet(t *testing.T) { t.Fatal(err) } - vm.Ctx.Lock.Lock() vm.unissuedEvents.Push(addValidatorTx) blk, err = vm.BuildBlock() // should add validator to the new subnet if err != nil { t.Fatal(err) } - vm.Ctx.Lock.Unlock() // Assert preferences are correct // and accept the proposal/commit @@ -989,12 +1005,10 @@ func TestCreateSubnet(t *testing.T) { // from pending to current validator set vm.clock.Set(startTime) - vm.Ctx.Lock.Lock() blk, err = vm.BuildBlock() // should be advance time tx if err != nil { t.Fatal(err) } - vm.Ctx.Lock.Unlock() // Assert preferences are correct // and accept the proposal/commit @@ -1049,12 +1063,10 @@ func TestCreateSubnet(t *testing.T) { // fast forward clock to time validator should stop validating vm.clock.Set(endTime) - vm.Ctx.Lock.Lock() blk, err = vm.BuildBlock() // should be advance time tx if err != nil { t.Fatal(err) } - vm.Ctx.Lock.Unlock() // Assert preferences are correct // and accept the proposal/commit @@ -1102,7 +1114,11 @@ func TestCreateSubnet(t *testing.T) { // test asset import func TestAtomicImport(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() avmID := ids.Empty.Prefix(0) utxoID := ava.UTXOID{ @@ -1136,9 +1152,6 @@ func TestAtomicImport(t *testing.T) { t.Fatal(err) } - vm.Ctx.Lock.Lock() - defer vm.Ctx.Lock.Unlock() - vm.ava = assetID vm.avm = avmID @@ -1194,7 +1207,11 @@ func TestAtomicImport(t *testing.T) { // test optimistic asset import func TestOptimisticAtomicImport(t *testing.T) { vm := defaultVM() - defer func() { vm.Ctx.Lock.Lock(); vm.Shutdown(); vm.Ctx.Lock.Unlock() }() + vm.Ctx.Lock.Lock() + defer func() { + vm.Shutdown() + vm.Ctx.Lock.Unlock() + }() avmID := ids.Empty.Prefix(0) utxoID := ava.UTXOID{ @@ -1228,9 +1245,6 @@ func TestOptimisticAtomicImport(t *testing.T) { t.Fatal(err) } - vm.Ctx.Lock.Lock() - defer vm.Ctx.Lock.Unlock() - vm.ava = assetID vm.avm = avmID @@ -1354,7 +1368,10 @@ func TestRestartPartiallyAccepted(t *testing.T) { secondVM.clock.Set(defaultGenesisTime) secondCtx := defaultContext() secondCtx.Lock.Lock() - defer secondCtx.Lock.Unlock() + defer func() { + secondVM.Shutdown() + secondCtx.Lock.Unlock() + }() secondMsgChan := make(chan common.Message, 1) if err := secondVM.Initialize(secondCtx, db, genesisBytes, secondMsgChan, nil); err != nil { @@ -1460,7 +1477,10 @@ func TestRestartFullyAccepted(t *testing.T) { secondVM.clock.Set(defaultGenesisTime) secondCtx := defaultContext() secondCtx.Lock.Lock() - defer secondCtx.Lock.Unlock() + defer func() { + secondVM.Shutdown() + secondCtx.Lock.Unlock() + }() secondMsgChan := make(chan common.Message, 1) if err := secondVM.Initialize(secondCtx, db, genesisBytes, secondMsgChan, nil); err != nil { @@ -1658,6 +1678,12 @@ func TestUnverifiedParent(t *testing.T) { vm.clock.Set(defaultGenesisTime) ctx := defaultContext() + ctx.Lock.Lock() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() + msgChan := make(chan common.Message, 1) if err := vm.Initialize(ctx, db, genesisBytes, msgChan, nil); err != nil { t.Fatal(err) From d082579393cf9af393b099c01be2f95780b4008f Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Sun, 10 May 2020 17:12:03 -0400 Subject: [PATCH 24/24] finished race cleanup in the avm tests --- vms/avm/export_tx_test.go | 12 ++++++---- vms/avm/import_tx_test.go | 16 ++++++++----- vms/avm/vm_test.go | 50 ++++++++++++++++++++++----------------- 3 files changed, 46 insertions(+), 32 deletions(-) diff --git a/vms/avm/export_tx_test.go b/vms/avm/export_tx_test.go index f3cf508..4c27d58 100644 --- a/vms/avm/export_tx_test.go +++ b/vms/avm/export_tx_test.go @@ -216,8 +216,10 @@ func TestIssueExportTx(t *testing.T) { } ctx.Lock.Lock() - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() txs := vm.PendingTxs() if len(txs) != 1 { @@ -350,8 +352,10 @@ func TestClearForceAcceptedExportTx(t *testing.T) { } ctx.Lock.Lock() - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() txs := vm.PendingTxs() if len(txs) != 1 { diff --git a/vms/avm/import_tx_test.go b/vms/avm/import_tx_test.go index 9cdecdb..f01be37 100644 --- a/vms/avm/import_tx_test.go +++ b/vms/avm/import_tx_test.go @@ -228,8 +228,10 @@ func TestIssueImportTx(t *testing.T) { } ctx.Lock.Lock() - defer ctx.Lock.Unlock() - defer vm.Shutdown() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() txs := vm.PendingTxs() if len(txs) != 1 { @@ -264,11 +266,13 @@ func TestForceAcceptImportTx(t *testing.T) { platformID := ids.Empty.Prefix(0) - ctx.Lock.Lock() - defer ctx.Lock.Unlock() - vm := &VM{platform: platformID} - defer vm.Shutdown() + ctx.Lock.Lock() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() + err := vm.Initialize( ctx, memdb.New(), diff --git a/vms/avm/vm_test.go b/vms/avm/vm_test.go index c397f4e..397d4eb 100644 --- a/vms/avm/vm_test.go +++ b/vms/avm/vm_test.go @@ -392,11 +392,13 @@ func TestTxSerialization(t *testing.T) { } func TestInvalidGenesis(t *testing.T) { - ctx.Lock.Lock() - defer ctx.Lock.Unlock() - vm := &VM{} - defer vm.Shutdown() + ctx.Lock.Lock() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() + err := vm.Initialize( /*context=*/ ctx, /*db=*/ memdb.New(), @@ -410,13 +412,14 @@ func TestInvalidGenesis(t *testing.T) { } func TestInvalidFx(t *testing.T) { - genesisBytes := BuildGenesisTest(t) - - ctx.Lock.Lock() - defer ctx.Lock.Unlock() - vm := &VM{} - defer vm.Shutdown() + ctx.Lock.Lock() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() + + genesisBytes := BuildGenesisTest(t) err := vm.Initialize( /*context=*/ ctx, /*db=*/ memdb.New(), @@ -432,13 +435,14 @@ func TestInvalidFx(t *testing.T) { } func TestFxInitializationFailure(t *testing.T) { - genesisBytes := BuildGenesisTest(t) - - ctx.Lock.Lock() - defer ctx.Lock.Unlock() - vm := &VM{} - defer vm.Shutdown() + ctx.Lock.Lock() + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() + + genesisBytes := BuildGenesisTest(t) err := vm.Initialize( /*context=*/ ctx, /*db=*/ memdb.New(), @@ -489,6 +493,10 @@ func TestIssueTx(t *testing.T) { func TestGenesisGetUTXOs(t *testing.T) { _, _, vm := GenesisVM(t) + defer func() { + vm.Shutdown() + ctx.Lock.Unlock() + }() shortAddr := keys[0].PublicKey().Address() addr := ids.NewID(hashing.ComputeHash256Array(shortAddr.Bytes())) @@ -499,8 +507,6 @@ func TestGenesisGetUTXOs(t *testing.T) { if err != nil { t.Fatal(err) } - vm.Shutdown() - ctx.Lock.Unlock() if len(utxos) != 7 { t.Fatalf("Wrong number of utxos. Expected (%d) returned (%d)", 7, len(utxos)) @@ -645,8 +651,8 @@ func TestIssueNFT(t *testing.T) { vm := &VM{} ctx.Lock.Lock() defer func() { - ctx.Lock.Unlock() vm.Shutdown() + ctx.Lock.Unlock() }() genesisBytes := BuildGenesisTest(t) @@ -804,8 +810,8 @@ func TestIssueProperty(t *testing.T) { vm := &VM{} ctx.Lock.Lock() defer func() { - ctx.Lock.Unlock() vm.Shutdown() + ctx.Lock.Unlock() }() genesisBytes := BuildGenesisTest(t) @@ -952,8 +958,8 @@ func TestIssueProperty(t *testing.T) { func TestVMFormat(t *testing.T) { _, _, vm := GenesisVM(t) defer func() { - ctx.Lock.Unlock() vm.Shutdown() + ctx.Lock.Unlock() }() tests := []struct { @@ -974,8 +980,8 @@ func TestVMFormat(t *testing.T) { func TestVMFormatAliased(t *testing.T) { _, _, vm := GenesisVM(t) defer func() { - ctx.Lock.Unlock() vm.Shutdown() + ctx.Lock.Unlock() }() origAliases := ctx.BCLookup