From 95cca6f29c299f0583211f6a104298bd477423a6 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Sun, 19 Apr 2020 13:48:36 +0100 Subject: [PATCH 01/42] 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/42] 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/42] 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/42] 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/42] 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/42] 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/42] 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/42] 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/42] 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/42] 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/42] 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/42] 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/42] 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 5d115a0b62a103231f593963968904ae1e192f9d Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Mon, 4 May 2020 17:16:00 -0400 Subject: [PATCH 14/42] %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 940f9a2fb8cd0471278842c5d730d79d2aa888fc Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Tue, 5 May 2020 17:45:23 -0400 Subject: [PATCH 15/42] 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 87a175eaaf8fe18833d72f650ef112e49b41924a Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Wed, 6 May 2020 20:55:52 -0400 Subject: [PATCH 16/42] Changed signal handling to use go signal handlers --- node/node.go | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/node/node.go b/node/node.go index 3aafae6..0fc8e71 100644 --- a/node/node.go +++ b/node/node.go @@ -4,7 +4,7 @@ package node // #include "salticidae/network.h" -// void onTerm(int sig, void *); +// void onTerm(threadcall_handle_t *, void *); // void errorHandler(SalticidaeCError *, bool, int32_t, void *); import "C" @@ -14,6 +14,8 @@ import ( "errors" "fmt" "io/ioutil" + "os" + "os/signal" "path" "sync" "unsafe" @@ -92,6 +94,10 @@ type Node struct { // Event loop manager EC salticidae.EventContext + + // Caller to the event context + TCall salticidae.ThreadCall + // Network that manages validator peers PeerNet salticidae.PeerNetwork // Network that manages clients @@ -124,7 +130,7 @@ type Node struct { */ //export onTerm -func onTerm(C.int, unsafe.Pointer) { +func onTerm(*C.threadcall_handle_t, unsafe.Pointer) { MainNode.Log.Debug("Terminate signal received") MainNode.EC.Stop() } @@ -143,6 +149,17 @@ func errorHandler(_err *C.struct_SalticidaeCError, fatal C.bool, asyncID C.int32 func (n *Node) initNetlib() error { // Create main event context n.EC = salticidae.NewEventContext() + n.TCall = salticidae.NewThreadCall(n.EC) + + c := make(chan os.Signal, 1) + signal.Notify(c, os.Interrupt) + signal.Notify(c, os.Kill) + + go func() { + for range c { + n.TCall.AsyncCall(salticidae.ThreadCallCallback(C.onTerm), nil) + } + }() // Set up interrupt signal and terminate signal handlers evInt := salticidae.NewSigEvent(n.EC, salticidae.SigEventCallback(C.onTerm), nil) From a5a3c703f1738496f73fd8ba6229eb3567f42832 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Wed, 6 May 2020 20:57:45 -0400 Subject: [PATCH 17/42] Removed cgo signal handlers --- node/node.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/node/node.go b/node/node.go index 0fc8e71..da3d311 100644 --- a/node/node.go +++ b/node/node.go @@ -161,12 +161,6 @@ func (n *Node) initNetlib() error { } }() - // Set up interrupt signal and terminate signal handlers - evInt := salticidae.NewSigEvent(n.EC, salticidae.SigEventCallback(C.onTerm), nil) - evInt.Add(salticidae.SIGINT) - evTerm := salticidae.NewSigEvent(n.EC, salticidae.SigEventCallback(C.onTerm), nil) - evTerm.Add(salticidae.SIGTERM) - // Create peer network config, may have tls enabled peerConfig := salticidae.NewPeerNetworkConfig() peerConfig.ConnTimeout(60) From b096ee67718f017db513fc64673a1f40dd390f08 Mon Sep 17 00:00:00 2001 From: Anil Dasari Date: Thu, 7 May 2020 13:00:27 +0530 Subject: [PATCH 18/42] add DeleteUser rpc --- api/keystore/service.go | 46 +++++++++++++++++++++++++++++++++ api/keystore/service_test.go | 49 ++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+) diff --git a/api/keystore/service.go b/api/keystore/service.go index 7ca34b5..557fdc7 100644 --- a/api/keystore/service.go +++ b/api/keystore/service.go @@ -301,6 +301,52 @@ func (ks *Keystore) ImportUser(r *http.Request, args *ImportUserArgs, reply *Imp return nil } +// DeleteUserArgs are arguments for passing into CreateUser requests +type DeleteUserArgs struct { + Username string `json:"username"` + Password string `json:"password"` +} + +// DeleteUserReply is the response from calling CreateUser +type DeleteUserReply struct { + Success bool `json:"success"` +} + +// DeleteUser deletes user with the provided username and password. +func (ks *Keystore) DeleteUser(_ *http.Request, args *DeleteUserArgs, reply *DeleteUserReply) error { + ks.lock.Lock() + defer ks.lock.Unlock() + + ks.log.Verbo("DeleteUser called with %s", args.Username) + + if args.Username == "" { + return errEmptyUsername + } + + // check if user exists and valid user. + usr, err := ks.getUser(args.Username) + switch { + case err != nil || usr == nil: + return fmt.Errorf("user doesn't exists: %s", args.Username) + case !usr.CheckPassword(args.Password): + return fmt.Errorf("incorrect password for user %q", args.Username) + } + + // TODO: user is deleted from user db but returned an error due to some other problem. + // valid scenario ? discuss with team. + + // delete from user db. + if err := ks.userDB.Delete([]byte(args.Username)); err != nil { + return err + } + + // delete from users map. + delete(ks.users, args.Username) + + reply.Success = true + return nil +} + // NewBlockchainKeyStore ... func (ks *Keystore) NewBlockchainKeyStore(blockchainID ids.ID) *BlockchainKeystore { return &BlockchainKeystore{ diff --git a/api/keystore/service_test.go b/api/keystore/service_test.go index 0868a29..ca2f862 100644 --- a/api/keystore/service_test.go +++ b/api/keystore/service_test.go @@ -7,6 +7,7 @@ import ( "bytes" "fmt" "math/rand" + "reflect" "testing" "github.com/ava-labs/gecko/database/memdb" @@ -280,3 +281,51 @@ func TestServiceExportImport(t *testing.T) { } } } + +func TestServiceDeleteUser(t *testing.T) { + testUser := "testUser" + password := "passwTest@fake01ord" + tests := []struct { + desc string + request *DeleteUserArgs + want *DeleteUserReply + wantError bool + }{{ + desc: "empty user name case", + request: &DeleteUserArgs{}, + wantError: true, + }, { + desc: "user not exists case", + request: &DeleteUserArgs{Username: "dummy"}, + wantError: true, + }, { + desc: "user exists and invalid password case", + request: &DeleteUserArgs{Username: testUser, Password: "password"}, + wantError: true, + }, { + desc: "user exists and valid password case", + request: &DeleteUserArgs{Username: testUser, Password: password}, + want: &DeleteUserReply{Success: true}, + }} + + ks := Keystore{} + ks.Initialize(logging.NoLog{}, memdb.New()) + + if err := ks.CreateUser(nil, &CreateUserArgs{Username: "testUser", Password: "passwTest@fake01ord"}, &CreateUserReply{}); err != nil { + t.Fatalf("failed to create user: %v", err) + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + got := &DeleteUserReply{} + err := ks.DeleteUser(nil, tt.request, got) + if (err != nil) != tt.wantError { + t.Fatalf("DeleteUser() failed: error %v, wantError %v", err, tt.wantError) + } + + if !tt.wantError && !reflect.DeepEqual(tt.want, got) { + t.Fatalf("DeleteUser() failed: got %v, want %v", got, tt.want) + } + }) + } +} From c81521a4547093b105dec66511172b9907178ac5 Mon Sep 17 00:00:00 2001 From: Anil Dasari Date: Thu, 7 May 2020 13:08:42 +0530 Subject: [PATCH 19/42] comments typo correction --- api/keystore/service.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/keystore/service.go b/api/keystore/service.go index 557fdc7..33c1cad 100644 --- a/api/keystore/service.go +++ b/api/keystore/service.go @@ -301,13 +301,13 @@ func (ks *Keystore) ImportUser(r *http.Request, args *ImportUserArgs, reply *Imp return nil } -// DeleteUserArgs are arguments for passing into CreateUser requests +// DeleteUserArgs are arguments for passing into DeleteUser requests type DeleteUserArgs struct { Username string `json:"username"` Password string `json:"password"` } -// DeleteUserReply is the response from calling CreateUser +// DeleteUserReply is the response from calling DeleteUser type DeleteUserReply struct { Success bool `json:"success"` } From 41e3ccbded88b14d9c77ad08f3f8c6561746bbd0 Mon Sep 17 00:00:00 2001 From: bb-2 <43212522+bb-2@users.noreply.github.com> Date: Thu, 7 May 2020 08:21:45 -0400 Subject: [PATCH 20/42] prevent runtime error in user.getKey by checking for nil shortID's in request args --- vms/platformvm/service.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/vms/platformvm/service.go b/vms/platformvm/service.go index e2a9ffe..835ef36 100644 --- a/vms/platformvm/service.go +++ b/vms/platformvm/service.go @@ -34,6 +34,8 @@ var ( errGetStakeSource = errors.New("couldn't get account specified in 'stakeSource'") errNoBlockchainWithAlias = errors.New("there is no blockchain with the specified alias") errDSCantValidate = errors.New("new blockchain can't be validated by default Subnet") + errNilSigner = errors.New("nil ShortID 'signer' is not valid") + errNilTo = errors.New("nil ShortID 'to' is not valid") ) // Service defines the API calls that can be made to the platform chain @@ -674,6 +676,10 @@ func (service *Service) Sign(_ *http.Request, args *SignArgs, reply *SignRespons } user := user{db: db} + if args.Signer.IsZero() { + return errNilSigner + } + key, err := user.getKey(args.Signer) // Key of [args.Signer] if err != nil { return errDB @@ -882,6 +888,10 @@ func (service *Service) ImportAVA(_ *http.Request, args *ImportAVAArgs, response } user := user{db: db} + if args.To.IsZero() { + return errNilTo + } + kc := secp256k1fx.NewKeychain() key, err := user.getKey(args.To) if err != nil { From d8a8617e3b569a162cc7d3909c6b111be5d7ee58 Mon Sep 17 00:00:00 2001 From: bb-2 <43212522+bb-2@users.noreply.github.com> Date: Thu, 7 May 2020 11:21:26 -0400 Subject: [PATCH 21/42] move the argument validity checks to the top of the methods --- vms/platformvm/service.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/vms/platformvm/service.go b/vms/platformvm/service.go index 835ef36..973c6cc 100644 --- a/vms/platformvm/service.go +++ b/vms/platformvm/service.go @@ -669,6 +669,10 @@ type SignResponse struct { func (service *Service) Sign(_ *http.Request, args *SignArgs, reply *SignResponse) error { service.vm.Ctx.Log.Debug("sign called") + if args.Signer.IsZero() { + return errNilSigner + } + // Get the key of the Signer db, err := service.vm.Ctx.Keystore.GetDatabase(args.Username, args.Password) if err != nil { @@ -676,10 +680,6 @@ func (service *Service) Sign(_ *http.Request, args *SignArgs, reply *SignRespons } user := user{db: db} - if args.Signer.IsZero() { - return errNilSigner - } - key, err := user.getKey(args.Signer) // Key of [args.Signer] if err != nil { return errDB @@ -881,6 +881,10 @@ type ImportAVAArgs struct { func (service *Service) ImportAVA(_ *http.Request, args *ImportAVAArgs, response *SignResponse) error { service.vm.Ctx.Log.Debug("platform.ImportAVA called") + if args.To.IsZero() { + return errNilTo + } + // Get the key of the Signer db, err := service.vm.Ctx.Keystore.GetDatabase(args.Username, args.Password) if err != nil { @@ -888,10 +892,6 @@ func (service *Service) ImportAVA(_ *http.Request, args *ImportAVAArgs, response } user := user{db: db} - if args.To.IsZero() { - return errNilTo - } - kc := secp256k1fx.NewKeychain() key, err := user.getKey(args.To) if err != nil { From c43d9f5b2f73d45fd2174477bbedd2b9feec27d0 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Thu, 7 May 2020 12:04:04 -0400 Subject: [PATCH 22/42] Cahgned kill_playbook to send proper signal --- scripts/ansible/roles/ava-stop/tasks/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/ansible/roles/ava-stop/tasks/main.yml b/scripts/ansible/roles/ava-stop/tasks/main.yml index 1053cf0..e96e356 100644 --- a/scripts/ansible/roles/ava-stop/tasks/main.yml +++ b/scripts/ansible/roles/ava-stop/tasks/main.yml @@ -1,3 +1,3 @@ - name: Kill Node - command: killall ava + command: killall -SIGINT ava ignore_errors: true From 02f162db1a16c77e1d92a6721650e1ecf15ae71a Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Thu, 7 May 2020 12:22:09 -0400 Subject: [PATCH 23/42] remove key/cert generator script --- keys/genCA.sh | 5 --- keys/genStaker.sh | 13 -------- keys/rootCA.crt | 34 ------------------- keys/rootCA.key | 51 ----------------------------- keys/rootCA.srl | 1 - {keys => staking}/local/staker1.crt | 0 {keys => staking}/local/staker1.key | 0 {keys => staking}/local/staker2.crt | 0 {keys => staking}/local/staker2.key | 0 {keys => staking}/local/staker3.crt | 0 {keys => staking}/local/staker3.key | 0 {keys => staking}/local/staker4.crt | 0 {keys => staking}/local/staker4.key | 0 {keys => staking}/local/staker5.crt | 0 {keys => staking}/local/staker5.key | 0 15 files changed, 104 deletions(-) delete mode 100755 keys/genCA.sh delete mode 100755 keys/genStaker.sh delete mode 100644 keys/rootCA.crt delete mode 100644 keys/rootCA.key delete mode 100644 keys/rootCA.srl rename {keys => staking}/local/staker1.crt (100%) rename {keys => staking}/local/staker1.key (100%) rename {keys => staking}/local/staker2.crt (100%) rename {keys => staking}/local/staker2.key (100%) rename {keys => staking}/local/staker3.crt (100%) rename {keys => staking}/local/staker3.key (100%) rename {keys => staking}/local/staker4.crt (100%) rename {keys => staking}/local/staker4.key (100%) rename {keys => staking}/local/staker5.crt (100%) rename {keys => staking}/local/staker5.key (100%) diff --git a/keys/genCA.sh b/keys/genCA.sh deleted file mode 100755 index 14a0f4c..0000000 --- a/keys/genCA.sh +++ /dev/null @@ -1,5 +0,0 @@ -#!/bin/sh -set -ex - -openssl genrsa -out `dirname "$0"`/rootCA.key 4096 -openssl req -x509 -new -nodes -key `dirname "$0"`/rootCA.key -sha256 -days 365250 -out `dirname "$0"`/rootCA.crt diff --git a/keys/genStaker.sh b/keys/genStaker.sh deleted file mode 100755 index 34f6889..0000000 --- a/keys/genStaker.sh +++ /dev/null @@ -1,13 +0,0 @@ -#!/bin/sh -set -ex - -keypath=$GOPATH/src/github.com/ava-labs/gecko/keys - -if test -f "$keypath/staker.key" || test -f "$keypath/staker.crt"; then - echo "staker.key or staker.crt already exists. Not generating new key/certificiate." - exit 1 -fi - -openssl genrsa -out `dirname "$0"`/staker.key 4096 -openssl req -new -sha256 -key `dirname "$0"`/staker.key -subj "/C=US/ST=NY/O=Avalabs/CN=ava" -out `dirname "$0"`/staker.csr -openssl x509 -req -in `dirname "$0"`/staker.csr -CA `dirname "$0"`/rootCA.crt -CAkey `dirname "$0"`/rootCA.key -CAcreateserial -out `dirname "$0"`/staker.crt -days 365250 -sha256 diff --git a/keys/rootCA.crt b/keys/rootCA.crt deleted file mode 100644 index da6320a..0000000 --- a/keys/rootCA.crt +++ /dev/null @@ -1,34 +0,0 @@ ------BEGIN CERTIFICATE----- -MIIF1jCCA76gAwIBAgIJALI1DF9cpwfEMA0GCSqGSIb3DQEBCwUAMH8xCzAJBgNV -BAYTAlVTMQswCQYDVQQIDAJOWTEPMA0GA1UEBwwGSXRoYWNhMRAwDgYDVQQKDAdB -dmFsYWJzMQ4wDAYDVQQLDAVHZWNrbzEMMAoGA1UEAwwDYXZhMSIwIAYJKoZIhvcN -AQkBFhNzdGVwaGVuQGF2YWxhYnMub3JnMCAXDTE5MDIyODIwNTkyNFoYDzMwMTkw -MzA4MjA1OTI0WjB/MQswCQYDVQQGEwJVUzELMAkGA1UECAwCTlkxDzANBgNVBAcM -Bkl0aGFjYTEQMA4GA1UECgwHQXZhbGFiczEOMAwGA1UECwwFR2Vja28xDDAKBgNV -BAMMA2F2YTEiMCAGCSqGSIb3DQEJARYTc3RlcGhlbkBhdmFsYWJzLm9yZzCCAiIw -DQYJKoZIhvcNAQEBBQADggIPADCCAgoCggIBAJ45ScWV8tsCNO+NTIBuUYsPkhcg -jrp0HEyCHY3XEkxsLuDqtesNyv39YA0xQ3M3FP1e29tjFeHWJzyzV8O1H+6yco93 -QAtzh9xELYD301Yq+x55yZrSjZxNIC5Tmz1ewTfD315lNR04M6JmqjrStIuLsWFU -m6P4OgXs4daqnyq9au4PYSrejcbexW59rKxLryK6Acv+E9Ax04oS33g9KqPmlRx0 -lfu3x4nkIKIl+VaK1wC5CwJDYZ91KpEbC8Z2YvTeVDH+/hz/MvKl1CEaqK/4G5FB -KGEyd/bGRxMVQF41G7liJLaXzPLyZnKO2n21ZuJhkA9MZelt1U0LuQU505qU7IzW -cmKFEIb1MOrclaF19Is7HQlJWKyDo2/hfjSCZO8zH7eR9EGzKyQwZhwkYCycJD44 -RKEHq6s/Z2dHUlpLIgRJ7k171TNkL9+xLntu8v1lzTkhemSNeO9asqJ7VcvpnMHH -bQXpDxJpi8jTnV8In8EolSqaKeN6/nzwbbSJ7gHehgpDhC1DlXPRzTt/ktQKlNGW -T5bdNdvYFyYTd9fu78aJZSbJo8jS2fykWuBgOgnlV8VmwpDa7iHM3EECByhf5GKB -J1jBlXO1ZyfJ7sNTbuVM7Uc2JkB4ASKdm3GZ3sFv95HjSTJAUORjE4pQ1es4kfDU -KqzDHH+bEHaGIGJTAgMBAAGjUzBRMB0GA1UdDgQWBBQr2T0duSMkvGXe3bSdWcei -73QtwzAfBgNVHSMEGDAWgBQr2T0duSMkvGXe3bSdWcei73QtwzAPBgNVHRMBAf8E -BTADAQH/MA0GCSqGSIb3DQEBCwUAA4ICAQBpP18zCdzvnSdPigg9wx+a8Znr4aJj -FxZYwBY6/BmKb56ke9g+zKKCw2dYYkRYDcTOEfuBgBvNeCSJv4R5rmkukkL8RCIG -XV/WfSn2d3Mnz5KTgGQS6Q9s5qx+8ydkiGZthi+8a8ltXczyYrvWgd47U0NWTcOY -omjgF6XF+hVLWLgiwmA468pd7wyCsuJJkyxxeyDPXQ422I1AJW/7c5JQQa+lDNsv -Vna6420mZ/DiQd3stFkdjhRjmBZvGQ09g6l3zo6TgI1TWr5TMYPrempBVCWPNilC -XaMysU77+tPutI+7kMBuGvLuZtPrH/2uTYdXWPodyORm5i2ABF6In3VISPD9YNc0 -gWx3PYGi2BfdnZepCojsffUMlhT3SsiAKMYv5FhW8LQBNMRR4721U1Vf5f8fzNQn -3E55TthV5HXZQ6HcLhkmOvH8CMqtWGETTbBtYSA2AVMjoqs7QDGkfsCH9UuwGd1N -W12JOf53XyOQT2XwWerSQC2kv7elsTh6Bk7PhvrCi0OwCVSGny5IQY/aXM1n6Z6s -scJlZmq6P3AJZ3tRtBt9yDK7iIW7mzNLTb/kAjsNQh06oETJIJ0CIgL0Bn6CANYU -kNqB4oTxmAhdOPKNgqaIwdZAL1VDIVaQEcvGeZRduo7yZvA/MhuQD8IIKSeOBFaD -DB8IRfWqBx2nWw== ------END CERTIFICATE----- diff --git a/keys/rootCA.key b/keys/rootCA.key deleted file mode 100644 index fe23a96..0000000 --- a/keys/rootCA.key +++ /dev/null @@ -1,51 +0,0 @@ ------BEGIN RSA PRIVATE KEY----- -MIIJJwIBAAKCAgEAnjlJxZXy2wI0741MgG5Riw+SFyCOunQcTIIdjdcSTGwu4Oq1 -6w3K/f1gDTFDczcU/V7b22MV4dYnPLNXw7Uf7rJyj3dAC3OH3EQtgPfTVir7HnnJ -mtKNnE0gLlObPV7BN8PfXmU1HTgzomaqOtK0i4uxYVSbo/g6Bezh1qqfKr1q7g9h -Kt6Nxt7Fbn2srEuvIroBy/4T0DHTihLfeD0qo+aVHHSV+7fHieQgoiX5VorXALkL -AkNhn3UqkRsLxnZi9N5UMf7+HP8y8qXUIRqor/gbkUEoYTJ39sZHExVAXjUbuWIk -tpfM8vJmco7afbVm4mGQD0xl6W3VTQu5BTnTmpTsjNZyYoUQhvUw6tyVoXX0izsd -CUlYrIOjb+F+NIJk7zMft5H0QbMrJDBmHCRgLJwkPjhEoQerqz9nZ0dSWksiBEnu -TXvVM2Qv37Eue27y/WXNOSF6ZI1471qyontVy+mcwcdtBekPEmmLyNOdXwifwSiV -Kpop43r+fPBttInuAd6GCkOELUOVc9HNO3+S1AqU0ZZPlt0129gXJhN31+7vxoll -JsmjyNLZ/KRa4GA6CeVXxWbCkNruIczcQQIHKF/kYoEnWMGVc7VnJ8nuw1Nu5Uzt -RzYmQHgBIp2bcZnewW/3keNJMkBQ5GMTilDV6ziR8NQqrMMcf5sQdoYgYlMCAwEA -AQKCAgAhNota05AoEv2Dr5h4eS/azgjvm+D6GLd8A/AqPxRTQH5SrlJDpiCPUmmg -O1AaVlyslwX1toX4YxjXcBojNdkfJQxRO0oRXU4Oma0nnl4Zf2o5Sn1cZ4hcYAA6 -WUiECGjsyMwRp5MPsCV+mKhxMpu9kzRH5xfIwqmDZuc9RZGlyh8xG79c3VzLeyXc -fLsLa9O2qW8JICuOj3cFS9LnDYfu4c85Kuv06+4R7vY+s1P0q65YM3+xGO3cKB8o -WJIPNfityCHKYOl8ssFCGDdAP7VbQuyegBv20z5FafevdM2POPy53HUycwkNkn6Y -243Xx4VyTeKMo4/dATY+NxC+nRXiz4jLna5a7IIIzjAHl2kF6iJVasd3+X/xWHsM -Lx9iDRjERf+J+y58GaDxetXL1C0xm7Rv28yMYVPAzpucvS4i72Xj7X8JkO3az3Qv -/wqBnxj8ouh+5jvT0nqCJsFZwK0F7Dr3na2lSf34XBCTnd9//FfSIY7mDIddxuVF -2rKKOl2KkvbDUuSKVZwdJeAp1CccN6SfLnxKy+436Z5hYzBIeGyejpCMWivDJ2I3 -wjs4w4IPobT5dqaSdPYFTKJnoDv62vYbIN3o8pQ3QUXwmRPyKoPuxe7OZZyec43R -WUtajiW6AXjxUoEtPPPHAT/3pGKG2a0VGtDfjLjpp5OtQmteiQKCAQEAz62n9Lh1 -POdC4088GEqrGPhq5MUz2j2pOCjEZ7utmD+Lo8x95McCU+jf4UJ+rbaf96dvjPRC -T0Sc6X6IvvQycJubzQe6XX6eyZsr67qpuY2MGze+NvmO4LcCOfNHerRyLK2DoGLW -jQVxJNsBIFo0T7iSuUICbfxKdKxfH+27rPANEvpqS5BJalAfeGPEL0GgUTKQmswc -23Pnu5mkb7TWLKNVq7o/5PxvXyKmJQaFHCV98pqQr/HhXd79dMD12TPZRvsNgPGK -XOsmPtC5RHhbs/Wmdk3X3ihoVezou2VPeWCIrCANCuU0zZBK1igVC3FGeUK8u1Dl -jrTkRsNTLbBiPwKCAQEAwwngBBjbdRCVvUVWIBQBOk/t/6WyeAVH4O/fq32KklW+ -/SN5yeZhXjwMrFhSOqFUDipg/C4Imf5S3V+MlXO4lQsZzZa0d0euBIBt0SEcGE8P -rAkGcvwPfISBfYCnPem1ax01ixNJBxWDrgkfHZchywNPFgopiqpYR7X5ttACctCl -KLaDOXn667QmG1icsVgZV3L8gBxEdyjhmUGWFH/auS28oxqhUgiXrUQXbJKCesGD -E39r/SyOAGP5ZtTkWmNDp2+va8lSJwL1Ix+6qvexi/hIIGoFlSh5w+BwnBlxBL4C -cUanaXRtIqQ9rcO/xhZ7izmQzruNARLDPGIJ59MS7QKCAQBGR3wJAssZ2yD1j4DE -r7AK+TYjSODtP+SeDp24hPiQByEYQ0FvRDFzd+Ebd8cqvhyQUGcdiiNOc+et1JYu -GLFhDifBUJYuwYS2sP5B/Z8mHdKF+20xaW6CeSwVtFBCJAJnQCjFA+2bN3Y8hKhy -7FO7jriIXOA5nCEOLq7aPTc/pNSn0XpbK+7MPWUI9qoTW+AG2le5Ks2xLh4DjFDr -RIUeAgAh5xtsQEjoJu+WpAgzqDRg/xFrmS0s+SNIeWw5HqSuspK1SggKvcDpjPTF -SP2vfrfgXSNqGL6GJW/0yqoEZziZFxeS0lH2JphMtK+6eZDhxEXeFdg5XNnLYJor -Yf89AoIBAHbRLESys/c0HFTKybYPGdRhXzcvxXKynOBeoZ9Cgsm1LP3fv9EM5WJY -KMxRnf6Ty7Y5gQ4AKUNPGUI9dFKTxe4ebiC938EOzOd3Ke+OQSRZ/c0rTl98SR7t -Rkmjt77TAq93gufv3rxPEgJTEj6flHmt0V8236nXLqK5LKB/Rg6WJxePYJACTKeM -/u4H5KVxazbIGSUek2MYZ59KwlhIr4HCaDng/kgQbf6jDbYZ5x1LiEO3i50XqIZ6 -YTSRG3ApKsz1ECQU6FRVy+sS6FBBR0ti/OWqUS5WEyAOOewO37go3SoPBewLfnTt -I5oZN1pA1hCyCBK5VSRDPucpPqmY/90CggEAbFRUDyEkq9p7/Va/PYJLMe+1zGoy -+jCC1nm5LioxrUdpE+CV1t1cVutnlI3sRD+79oX/zwlwQ+pCx1XOMCmGs4uZUx5f -UtpGnsPamlyQKyQfPam3N4+4gaY9LLPiYCrI/XQh+vZQNlQTStuKLtb0R8+4wEER -KDTtC2cNN5fSnexEifpvq5yK3x6bH66pPyuRE27vVQ7diPar9A+VwkLs+zGbfnWW -MP/zYUbuiatC/LozcYLs/01m3Nu6oYi0OP/nFofepXNpQoZO8jKpnGRVVJ0EfgSe -f3qb9nkygj+gqGWT+PY6H39xKFz0h7dmmcP3Z7CrYXFEFfTCsUgbOKulAA== ------END RSA PRIVATE KEY----- diff --git a/keys/rootCA.srl b/keys/rootCA.srl deleted file mode 100644 index 617b916..0000000 --- a/keys/rootCA.srl +++ /dev/null @@ -1 +0,0 @@ -BAF3B5C5C6D0D166 diff --git a/keys/local/staker1.crt b/staking/local/staker1.crt similarity index 100% rename from keys/local/staker1.crt rename to staking/local/staker1.crt diff --git a/keys/local/staker1.key b/staking/local/staker1.key similarity index 100% rename from keys/local/staker1.key rename to staking/local/staker1.key diff --git a/keys/local/staker2.crt b/staking/local/staker2.crt similarity index 100% rename from keys/local/staker2.crt rename to staking/local/staker2.crt diff --git a/keys/local/staker2.key b/staking/local/staker2.key similarity index 100% rename from keys/local/staker2.key rename to staking/local/staker2.key diff --git a/keys/local/staker3.crt b/staking/local/staker3.crt similarity index 100% rename from keys/local/staker3.crt rename to staking/local/staker3.crt diff --git a/keys/local/staker3.key b/staking/local/staker3.key similarity index 100% rename from keys/local/staker3.key rename to staking/local/staker3.key diff --git a/keys/local/staker4.crt b/staking/local/staker4.crt similarity index 100% rename from keys/local/staker4.crt rename to staking/local/staker4.crt diff --git a/keys/local/staker4.key b/staking/local/staker4.key similarity index 100% rename from keys/local/staker4.key rename to staking/local/staker4.key diff --git a/keys/local/staker5.crt b/staking/local/staker5.crt similarity index 100% rename from keys/local/staker5.crt rename to staking/local/staker5.crt diff --git a/keys/local/staker5.key b/staking/local/staker5.key similarity index 100% rename from keys/local/staker5.key rename to staking/local/staker5.key From 522c94611513f0d321da900377ce138a73598e2b Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Thu, 7 May 2020 14:30:14 -0400 Subject: [PATCH 24/42] Added GetTx method to the AVM endpoint --- vms/avm/service.go | 30 ++++++++++++++++++++++++++++++ vms/avm/service_test.go | 40 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/vms/avm/service.go b/vms/avm/service.go index 10b9b3b..0f16a92 100644 --- a/vms/avm/service.go +++ b/vms/avm/service.go @@ -96,6 +96,36 @@ func (service *Service) GetTxStatus(r *http.Request, args *GetTxStatusArgs, repl return nil } +// GetTxArgs are arguments for passing into GetTx requests +type GetTxArgs struct { + TxID ids.ID `json:"txID"` +} + +// GetTxReply defines the GetTxStatus replies returned from the API +type GetTxReply struct { + Tx formatting.CB58 `json:"tx"` +} + +// GetTx returns the specified transaction +func (service *Service) GetTx(r *http.Request, args *GetTxArgs, reply *GetTxReply) error { + service.vm.ctx.Log.Verbo("GetTx called with %s", args.TxID) + + if args.TxID.IsZero() { + return errNilTxID + } + + tx := UniqueTx{ + vm: service.vm, + txID: args.TxID, + } + if status := tx.Status(); !status.Fetched() { + return errUnknownTx + } + + reply.Tx.Bytes = tx.Bytes() + return nil +} + // GetUTXOsArgs are arguments for passing into GetUTXOs requests type GetUTXOsArgs struct { Addresses []string `json:"addresses"` diff --git a/vms/avm/service_test.go b/vms/avm/service_test.go index dff4c3e..23b8a3e 100644 --- a/vms/avm/service_test.go +++ b/vms/avm/service_test.go @@ -7,10 +7,11 @@ import ( "fmt" "testing" - "github.com/ava-labs/gecko/snow/choices" + "github.com/stretchr/testify/assert" "github.com/ava-labs/gecko/database/memdb" "github.com/ava-labs/gecko/ids" + "github.com/ava-labs/gecko/snow/choices" "github.com/ava-labs/gecko/snow/engine/common" "github.com/ava-labs/gecko/utils/formatting" "github.com/ava-labs/gecko/vms/secp256k1fx" @@ -107,6 +108,43 @@ func TestServiceGetTxStatus(t *testing.T) { } } +func TestServiceGetTx(t *testing.T) { + genesisBytes, vm, s := setup(t) + defer ctx.Lock.Unlock() + defer vm.Shutdown() + + genesisTx := GetFirstTxFromGenesisTest(genesisBytes, t) + genesisTxBytes := genesisTx.Bytes() + txID := genesisTx.ID() + + reply := GetTxReply{} + err := s.GetTx(nil, &GetTxArgs{ + TxID: txID, + }, &reply) + assert.NoError(t, err) + assert.Equal(t, genesisTxBytes, reply.Tx.Bytes, "Wrong tx returned from service.GetTx") +} + +func TestServiceGetNilTx(t *testing.T) { + _, vm, s := setup(t) + defer ctx.Lock.Unlock() + defer vm.Shutdown() + + reply := GetTxReply{} + err := s.GetTx(nil, &GetTxArgs{}, &reply) + assert.Error(t, err, "Nil TxID should have returned an error") +} + +func TestServiceGetUnknownTx(t *testing.T) { + _, vm, s := setup(t) + defer ctx.Lock.Unlock() + defer vm.Shutdown() + + reply := GetTxReply{} + err := s.GetTx(nil, &GetTxArgs{TxID: ids.Empty}, &reply) + assert.Error(t, err, "Unknown TxID should have returned an error") +} + func TestServiceGetUTXOsInvalidAddress(t *testing.T) { _, vm, s := setup(t) defer ctx.Lock.Unlock() From 6bd84af8ecbbb5c08bc1c9379e3d19d92ed3de7b Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Thu, 7 May 2020 14:34:32 -0400 Subject: [PATCH 25/42] generate staking key at ~/.gecko/staking/staker.key if no key given. --- main/params.go | 109 ++++++++++++++++++++++++++++---------- staking/gen_staker_key.go | 74 ++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 27 deletions(-) create mode 100644 staking/gen_staker_key.go diff --git a/main/params.go b/main/params.go index a216720..f6b9daa 100644 --- a/main/params.go +++ b/main/params.go @@ -19,6 +19,7 @@ import ( "github.com/ava-labs/gecko/nat" "github.com/ava-labs/gecko/node" "github.com/ava-labs/gecko/snow/networking/router" + "github.com/ava-labs/gecko/staking" "github.com/ava-labs/gecko/utils" "github.com/ava-labs/gecko/utils/formatting" "github.com/ava-labs/gecko/utils/hashing" @@ -34,8 +35,14 @@ const ( // Results of parsing the CLI var ( - Config = node.Config{} - Err error + Config = node.Config{} + Err error + defaultStakingKeyPath = "~/.gecko/staking/staker.key" + defaultStakingCertPath = "~/.gecko/staking/staker.crt" +) + +var ( + errBootstrapMismatch = errors.New("more bootstrap IDs provided than bootstrap IPs") ) // GetIPs returns the default IPs for each network @@ -54,17 +61,15 @@ func GetIPs(networkID uint32) []string { } } -var ( - errBootstrapMismatch = errors.New("more bootstrap IDs provided than bootstrap IPs") -) - // Parse the CLI arguments func init() { errs := &wrappers.Errs{} defer func() { Err = errs.Err }() loggingConfig, err := logging.DefaultConfig() - errs.Add(err) + if errs.Add(err); errs.Errored() { + return + } fs := flag.NewFlagSet("gecko", flag.ContinueOnError) @@ -100,8 +105,8 @@ func init() { // Staking: consensusPort := fs.Uint("staking-port", 9651, "Port of the consensus server") fs.BoolVar(&Config.EnableStaking, "staking-tls-enabled", true, "Require TLS to authenticate staking connections") - fs.StringVar(&Config.StakingKeyFile, "staking-tls-key-file", "keys/staker.key", "TLS private key file for staking connections") - fs.StringVar(&Config.StakingCertFile, "staking-tls-cert-file", "keys/staker.crt", "TLS certificate file for staking connections") + fs.StringVar(&Config.StakingKeyFile, "staking-tls-key-file", defaultStakingKeyPath, "TLS private key for staking") + fs.StringVar(&Config.StakingCertFile, "staking-tls-cert-file", defaultStakingCertPath, "TLS certificate for staking") // Plugins: fs.StringVar(&Config.PluginDir, "plugin-dir", "./build/plugins", "Plugin directory for Ava VMs") @@ -142,22 +147,24 @@ func init() { } networkID, err := genesis.NetworkID(*networkName) - errs.Add(err) + if errs.Add(err); errs.Errored() { + return + } Config.NetworkID = networkID // DB: - if *db && err == nil { - // TODO: Add better params here - if *dbDir == defaultDbDir { - if *dbDir, err = homedir.Expand(defaultDbDir); err != nil { - errs.Add(fmt.Errorf("couldn't resolve default db path: %v", err)) - } + if *db { + *dbDir, err = homedir.Expand(*dbDir) + if errs.Add(fmt.Errorf("couldn't resolve db path: %w", err)); errs.Errored() { + return } dbPath := path.Join(*dbDir, genesis.NetworkName(Config.NetworkID), dbVersion) db, err := leveldb.New(dbPath, 0, 0, 0) + if errs.Add(fmt.Errorf("couldn't create db: %w", err)); errs.Errored() { + return + } Config.DB = db - errs.Add(err) } else { Config.DB = memdb.New() } @@ -169,7 +176,7 @@ func init() { if *consensusIP == "" { ip, err = Config.Nat.IP() if err != nil { - ip = net.IPv4zero + ip = net.IPv4zero // Couldn't get my IP...set to 0.0.0.0 } } else { ip = net.ParseIP(*consensusIP) @@ -177,7 +184,9 @@ func init() { if ip == nil { errs.Add(fmt.Errorf("Invalid IP Address %s", *consensusIP)) + return } + Config.StakingIP = utils.IPDesc{ IP: ip, Port: uint16(*consensusPort), @@ -190,7 +199,10 @@ func init() { for _, ip := range strings.Split(*bootstrapIPs, ",") { if ip != "" { addr, err := utils.ToIPDesc(ip) - errs.Add(err) + if err != nil { + errs.Add(fmt.Errorf("couldn't parse ip: %w", err)) + return + } Config.BootstrapPeers = append(Config.BootstrapPeers, &node.Peer{ IP: addr, }) @@ -209,20 +221,27 @@ func init() { cb58 := formatting.CB58{} for _, id := range strings.Split(*bootstrapIDs, ",") { if id != "" { - errs.Add(cb58.FromString(id)) - cert, err := ids.ToShortID(cb58.Bytes) - errs.Add(err) - + err = cb58.FromString(id) + if err != nil { + errs.Add(fmt.Errorf("couldn't parse bootstrap peer id to bytes: %w", err)) + return + } + peerID, err := ids.ToShortID(cb58.Bytes) + if err != nil { + errs.Add(fmt.Errorf("couldn't parse bootstrap peer id: %w", err)) + return + } if len(Config.BootstrapPeers) <= i { errs.Add(errBootstrapMismatch) - continue + return } - Config.BootstrapPeers[i].ID = cert + Config.BootstrapPeers[i].ID = peerID i++ } } if len(Config.BootstrapPeers) != i { errs.Add(fmt.Errorf("More bootstrap IPs, %d, provided than bootstrap IDs, %d", len(Config.BootstrapPeers), i)) + return } } else { for _, peer := range Config.BootstrapPeers { @@ -230,6 +249,38 @@ func init() { } } + // Staking + Config.StakingKeyFile, err = homedir.Expand(Config.StakingKeyFile) + if err != nil { + errs.Add(fmt.Errorf("couldn't resolve staking key path: %w", err)) + return + } + Config.StakingCertFile, err = homedir.Expand(Config.StakingCertFile) + if err != nil { + errs.Add(fmt.Errorf("couldn't resolve staking cert path: %v", err)) + return + } + defaultKeyPath, _ := homedir.Expand(defaultStakingKeyPath) + defaultStakingCertPath, _ := homedir.Expand(defaultStakingCertPath) + + switch { + // If staking key/cert locations are specified but not found, error + case Config.StakingKeyFile != defaultKeyPath || Config.StakingCertFile != defaultStakingCertPath: + if _, err := os.Stat(Config.StakingKeyFile); os.IsNotExist(err) { + errs.Add(fmt.Errorf("couldn't find staking key at %s", Config.StakingKeyFile)) + return + } else if _, err := os.Stat(Config.StakingCertFile); os.IsNotExist(err) { + errs.Add(fmt.Errorf("couldn't find staking certificate at %s", Config.StakingCertFile)) + return + } + default: + // Only creates staking key/cert if [stakingKeyPath] doesn't exist + if err := staking.GenerateStakingKeyCert(Config.StakingKeyFile, Config.StakingCertFile); err != nil { + errs.Add(fmt.Errorf("couldn't generate staking key/cert: %w", err)) + return + } + } + // HTTP: Config.HTTPPort = uint16(*httpPort) @@ -238,14 +289,18 @@ func init() { loggingConfig.Directory = *logsDir } logFileLevel, err := logging.ToLevel(*logLevel) - errs.Add(err) + if errs.Add(err); errs.Errored() { + return + } loggingConfig.LogLevel = logFileLevel if *logDisplayLevel == "" { *logDisplayLevel = *logLevel } displayLevel, err := logging.ToLevel(*logDisplayLevel) - errs.Add(err) + if errs.Add(err); errs.Errored() { + return + } loggingConfig.DisplayLevel = displayLevel Config.LoggingConfig = loggingConfig diff --git a/staking/gen_staker_key.go b/staking/gen_staker_key.go new file mode 100644 index 0000000..8969ea3 --- /dev/null +++ b/staking/gen_staker_key.go @@ -0,0 +1,74 @@ +package staking + +import ( + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "encoding/pem" + "fmt" + "math/big" + "os" + "path/filepath" + "time" +) + +// GenerateStakingKeyCert generates a self-signed TLS key/cert pair to use in staking +// The key and files will be placed at [keyPath] and [certPath], respectively +// If there is already a file at [keyPath], returns nil +func GenerateStakingKeyCert(keyPath, certPath string) error { + // If there is already a file at [keyPath], do nothing + if _, err := os.Stat(keyPath); !os.IsNotExist(err) { + return nil + } + + // Create key to sign cert with + key, err := rsa.GenerateKey(rand.Reader, 4096) + if err != nil { + return fmt.Errorf("couldn't generate rsa key: %w", err) + } + + // Create self-signed staking cert + certTemplate := &x509.Certificate{ + SerialNumber: big.NewInt(0), + NotBefore: time.Date(2000, time.January, 0, 0, 0, 0, 0, time.UTC), + NotAfter: time.Now().AddDate(100, 0, 0), + KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageDataEncipherment, + BasicConstraintsValid: true, + } + certBytes, err := x509.CreateCertificate(rand.Reader, certTemplate, certTemplate, &key.PublicKey, key) + if err != nil { + return fmt.Errorf("couldn't create certificate: %w", err) + } + + // Write cert to disk + if err := os.MkdirAll(filepath.Dir(certPath), 0755); err != nil { + return fmt.Errorf("couldn't create path for key/cert: %w", err) + } + certOut, err := os.Create(certPath) + if err != nil { + return fmt.Errorf("couldn't create cert file: %w", err) + } + if err := pem.Encode(certOut, &pem.Block{Type: "CERTIFICATE", Bytes: certBytes}); err != nil { + return fmt.Errorf("couldn't write cert file: %w", err) + } + if err := certOut.Close(); err != nil { + return fmt.Errorf("couldn't close cert file: %w", err) + } + + // Write key to disk + keyOut, err := os.Create(keyPath) + if err != nil { + return fmt.Errorf("couldn't create key file: %w", err) + } + privBytes, err := x509.MarshalPKCS8PrivateKey(key) + if err != nil { + return fmt.Errorf("couldn't marshal private key: %w", err) + } + if err := pem.Encode(keyOut, &pem.Block{Type: "PRIVATE KEY", Bytes: privBytes}); err != nil { + return fmt.Errorf("couldn't write private key: %w", err) + } + if err := keyOut.Close(); err != nil { + return fmt.Errorf("couldn't close key file: %w", err) + } + return nil +} From 03ddc6c03417a50afecebe027d241284350a3020 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Thu, 7 May 2020 15:17:13 -0400 Subject: [PATCH 26/42] fix bug --- main/params.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/main/params.go b/main/params.go index f6b9daa..1b9e622 100644 --- a/main/params.go +++ b/main/params.go @@ -156,12 +156,12 @@ func init() { // DB: if *db { *dbDir, err = homedir.Expand(*dbDir) - if errs.Add(fmt.Errorf("couldn't resolve db path: %w", err)); errs.Errored() { + if errs.Add(fmt.Errorf("couldn't resolve db path: %w", err)); err != nil { return } dbPath := path.Join(*dbDir, genesis.NetworkName(Config.NetworkID), dbVersion) db, err := leveldb.New(dbPath, 0, 0, 0) - if errs.Add(fmt.Errorf("couldn't create db: %w", err)); errs.Errored() { + if errs.Add(fmt.Errorf("couldn't create db: %w", err)); err != nil { return } Config.DB = db @@ -199,8 +199,7 @@ func init() { for _, ip := range strings.Split(*bootstrapIPs, ",") { if ip != "" { addr, err := utils.ToIPDesc(ip) - if err != nil { - errs.Add(fmt.Errorf("couldn't parse ip: %w", err)) + if errs.Add(fmt.Errorf("couldn't parse ip: %w", err)); err != nil { return } Config.BootstrapPeers = append(Config.BootstrapPeers, &node.Peer{ @@ -223,8 +222,9 @@ func init() { if id != "" { err = cb58.FromString(id) if err != nil { - errs.Add(fmt.Errorf("couldn't parse bootstrap peer id to bytes: %w", err)) - return + if errs.Add(fmt.Errorf("couldn't parse bootstrap peer id to bytes: %w", err)); err != nil { + return + } } peerID, err := ids.ToShortID(cb58.Bytes) if err != nil { From 966e28d928f0b7f4010f4bbc46fa119fae8e539c Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Thu, 7 May 2020 17:31:50 -0400 Subject: [PATCH 27/42] Added UTXOIDs to the getBalance call to enable atomic reads --- vms/avm/service.go | 26 +++++++++++++++----------- vms/avm/service_test.go | 22 ++++++++++++++++++++++ 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/vms/avm/service.go b/vms/avm/service.go index 10b9b3b..a292327 100644 --- a/vms/avm/service.go +++ b/vms/avm/service.go @@ -188,7 +188,8 @@ type GetBalanceArgs struct { // GetBalanceReply defines the GetBalance replies returned from the API type GetBalanceReply struct { - Balance json.Uint64 `json:"balance"` + Balance json.Uint64 `json:"balance"` + UTXOIDs []ava.UTXOID `json:"utxoIDs"` } // GetBalance returns the amount of an asset that an address at least partially owns @@ -217,18 +218,21 @@ func (service *Service) GetBalance(r *http.Request, args *GetBalanceArgs, reply } for _, utxo := range utxos { - if utxo.AssetID().Equals(assetID) { - transferable, ok := utxo.Out.(ava.Transferable) - if !ok { - continue - } - amt, err := safemath.Add64(transferable.Amount(), uint64(reply.Balance)) - if err != nil { - return err - } - reply.Balance = json.Uint64(amt) + if !utxo.AssetID().Equals(assetID) { + continue } + transferable, ok := utxo.Out.(ava.Transferable) + if !ok { + continue + } + amt, err := safemath.Add64(transferable.Amount(), uint64(reply.Balance)) + if err != nil { + return err + } + reply.Balance = json.Uint64(amt) + reply.UTXOIDs = append(reply.UTXOIDs, utxo.UTXOID) } + return nil } diff --git a/vms/avm/service_test.go b/vms/avm/service_test.go index dff4c3e..fa6ad41 100644 --- a/vms/avm/service_test.go +++ b/vms/avm/service_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/ava-labs/gecko/snow/choices" + "github.com/stretchr/testify/assert" "github.com/ava-labs/gecko/database/memdb" "github.com/ava-labs/gecko/ids" @@ -107,6 +108,27 @@ func TestServiceGetTxStatus(t *testing.T) { } } +func TestServiceGetBalance(t *testing.T) { + genesisBytes, vm, s := setup(t) + defer ctx.Lock.Unlock() + defer vm.Shutdown() + + genesisTx := GetFirstTxFromGenesisTest(genesisBytes, t) + assetID := genesisTx.ID() + addr := keys[0].PublicKey().Address() + + balanceArgs := &GetBalanceArgs{ + Address: fmt.Sprintf("%s-%s", vm.ctx.ChainID, addr), + AssetID: assetID.String(), + } + balanceReply := &GetBalanceReply{} + err := s.GetBalance(nil, balanceArgs, balanceReply) + assert.NoError(t, err) + assert.Equal(t, uint64(balanceReply.Balance), uint64(300000)) + + assert.Len(t, balanceReply.UTXOIDs, 4, "should have only returned four utxoIDs") +} + func TestServiceGetUTXOsInvalidAddress(t *testing.T) { _, vm, s := setup(t) defer ctx.Lock.Unlock() From 3130eb13bf6540dc6c243342d5f2a72e6fd71518 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Thu, 7 May 2020 17:36:27 -0400 Subject: [PATCH 28/42] Hid the Symbolic UTXO, from the json serialization, as it is an implementation detail --- vms/components/ava/utxo_id.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vms/components/ava/utxo_id.go b/vms/components/ava/utxo_id.go index b21000a..59b5d39 100644 --- a/vms/components/ava/utxo_id.go +++ b/vms/components/ava/utxo_id.go @@ -24,7 +24,7 @@ type UTXOID struct { OutputIndex uint32 `serialize:"true" json:"outputIndex"` // Symbol is false if the UTXO should be part of the DB - Symbol bool + Symbol bool `json:"-"` // id is the unique ID of a UTXO, it is calculated from TxID and OutputIndex id ids.ID } From cb3110f424c3ec978009f8a0531cee208b845c18 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Fri, 8 May 2020 18:17:54 -0400 Subject: [PATCH 29/42] fix error handling. Use env variable expansion. Change defaults to use /home/danlaine --- main/params.go | 47 ++++++++++++++++++----------------------------- 1 file changed, 18 insertions(+), 29 deletions(-) diff --git a/main/params.go b/main/params.go index 1b9e622..8f8dbff 100644 --- a/main/params.go +++ b/main/params.go @@ -10,6 +10,7 @@ import ( "net" "os" "path" + "path/filepath" "strings" "github.com/ava-labs/gecko/database/leveldb" @@ -25,20 +26,19 @@ import ( "github.com/ava-labs/gecko/utils/hashing" "github.com/ava-labs/gecko/utils/logging" "github.com/ava-labs/gecko/utils/wrappers" - "github.com/mitchellh/go-homedir" ) const ( - dbVersion = "v0.2.0" - defaultDbDir = "~/.gecko/db" + dbVersion = "v0.2.0" ) // Results of parsing the CLI var ( Config = node.Config{} Err error - defaultStakingKeyPath = "~/.gecko/staking/staker.key" - defaultStakingCertPath = "~/.gecko/staking/staker.crt" + defaultDbDir = os.ExpandEnv(filepath.Join("$HOME", ".gecko", "db")) + defaultStakingKeyPath = os.ExpandEnv(filepath.Join("$HOME", ".gecko", "staking", "staker.key")) + defaultStakingCertPath = os.ExpandEnv(filepath.Join("$HOME", ".gecko", "staking", "staker.crt")) ) var ( @@ -147,7 +147,7 @@ func init() { } networkID, err := genesis.NetworkID(*networkName) - if errs.Add(err); errs.Errored() { + if errs.Add(err); err != nil { return } @@ -155,13 +155,11 @@ func init() { // DB: if *db { - *dbDir, err = homedir.Expand(*dbDir) - if errs.Add(fmt.Errorf("couldn't resolve db path: %w", err)); err != nil { - return - } + *dbDir = os.ExpandEnv(*dbDir) // parse any env variables dbPath := path.Join(*dbDir, genesis.NetworkName(Config.NetworkID), dbVersion) db, err := leveldb.New(dbPath, 0, 0, 0) - if errs.Add(fmt.Errorf("couldn't create db: %w", err)); err != nil { + if err != nil { + errs.Add(fmt.Errorf("couldn't create db at %s: %w", dbPath, err)) return } Config.DB = db @@ -199,7 +197,8 @@ func init() { for _, ip := range strings.Split(*bootstrapIPs, ",") { if ip != "" { addr, err := utils.ToIPDesc(ip) - if errs.Add(fmt.Errorf("couldn't parse ip: %w", err)); err != nil { + if err != nil { + errs.Add(fmt.Errorf("couldn't parse ip: %w", err)) return } Config.BootstrapPeers = append(Config.BootstrapPeers, &node.Peer{ @@ -222,7 +221,8 @@ func init() { if id != "" { err = cb58.FromString(id) if err != nil { - if errs.Add(fmt.Errorf("couldn't parse bootstrap peer id to bytes: %w", err)); err != nil { + if err != nil { + errs.Add(fmt.Errorf("couldn't parse bootstrap peer id to bytes: %w", err)) return } } @@ -250,22 +250,11 @@ func init() { } // Staking - Config.StakingKeyFile, err = homedir.Expand(Config.StakingKeyFile) - if err != nil { - errs.Add(fmt.Errorf("couldn't resolve staking key path: %w", err)) - return - } - Config.StakingCertFile, err = homedir.Expand(Config.StakingCertFile) - if err != nil { - errs.Add(fmt.Errorf("couldn't resolve staking cert path: %v", err)) - return - } - defaultKeyPath, _ := homedir.Expand(defaultStakingKeyPath) - defaultStakingCertPath, _ := homedir.Expand(defaultStakingCertPath) - + Config.StakingCertFile = os.ExpandEnv(Config.StakingCertFile) // parse any env variable + Config.StakingKeyFile = os.ExpandEnv(Config.StakingKeyFile) switch { // If staking key/cert locations are specified but not found, error - case Config.StakingKeyFile != defaultKeyPath || Config.StakingCertFile != defaultStakingCertPath: + case Config.StakingKeyFile != defaultStakingKeyPath || Config.StakingCertFile != defaultStakingCertPath: if _, err := os.Stat(Config.StakingKeyFile); os.IsNotExist(err) { errs.Add(fmt.Errorf("couldn't find staking key at %s", Config.StakingKeyFile)) return @@ -289,7 +278,7 @@ func init() { loggingConfig.Directory = *logsDir } logFileLevel, err := logging.ToLevel(*logLevel) - if errs.Add(err); errs.Errored() { + if errs.Add(err); err != nil { return } loggingConfig.LogLevel = logFileLevel @@ -298,7 +287,7 @@ func init() { *logDisplayLevel = *logLevel } displayLevel, err := logging.ToLevel(*logDisplayLevel) - if errs.Add(err); errs.Errored() { + if errs.Add(err); err != nil { return } loggingConfig.DisplayLevel = displayLevel From ac27d66c02c6dc5199b71affc0a58fef5bf51af4 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Sun, 10 May 2020 15:26:41 -0400 Subject: [PATCH 30/42] 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 31/42] 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 32/42] 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 From fa230e6e905f933848fdd6026b11c7617c4e51be Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Mon, 11 May 2020 11:48:16 -0400 Subject: [PATCH 33/42] fix typo --- main/params.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/main/params.go b/main/params.go index 8f8dbff..c18a937 100644 --- a/main/params.go +++ b/main/params.go @@ -221,10 +221,8 @@ func init() { if id != "" { err = cb58.FromString(id) if err != nil { - if err != nil { - errs.Add(fmt.Errorf("couldn't parse bootstrap peer id to bytes: %w", err)) - return - } + errs.Add(fmt.Errorf("couldn't parse bootstrap peer id to bytes: %w", err)) + return } peerID, err := ids.ToShortID(cb58.Bytes) if err != nil { From 8b83cb517555db5e36fa9d4ce3124e9870d25e9a Mon Sep 17 00:00:00 2001 From: Anil Dasari Date: Tue, 12 May 2020 00:41:04 +0530 Subject: [PATCH 34/42] review comments: remove from all dbs --- api/keystore/service.go | 15 +++++++++++++++ api/keystore/service_test.go | 9 ++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/api/keystore/service.go b/api/keystore/service.go index 33c1cad..98aaac8 100644 --- a/api/keystore/service.go +++ b/api/keystore/service.go @@ -332,6 +332,21 @@ func (ks *Keystore) DeleteUser(_ *http.Request, args *DeleteUserArgs, reply *Del return fmt.Errorf("incorrect password for user %q", args.Username) } + userBatch := ks.userDB.NewBatch() + if err := userBatch.Delete([]byte(args.Username)); err != nil { + return err + } + + userDataDB := prefixdb.New([]byte(args.Username), ks.bcDB) + dataBatch := userDataDB.NewBatch() + if err := dataBatch.Delete([]byte(args.Username)); err != nil { + return err + } + + if err := atomic.WriteAll(dataBatch, userBatch); err != nil { + return err + } + // TODO: user is deleted from user db but returned an error due to some other problem. // valid scenario ? discuss with team. diff --git a/api/keystore/service_test.go b/api/keystore/service_test.go index ca2f862..6971fac 100644 --- a/api/keystore/service_test.go +++ b/api/keystore/service_test.go @@ -311,7 +311,7 @@ func TestServiceDeleteUser(t *testing.T) { ks := Keystore{} ks.Initialize(logging.NoLog{}, memdb.New()) - if err := ks.CreateUser(nil, &CreateUserArgs{Username: "testUser", Password: "passwTest@fake01ord"}, &CreateUserReply{}); err != nil { + if err := ks.CreateUser(nil, &CreateUserArgs{Username: testUser, Password: password}, &CreateUserReply{}); err != nil { t.Fatalf("failed to create user: %v", err) } @@ -326,6 +326,13 @@ func TestServiceDeleteUser(t *testing.T) { if !tt.wantError && !reflect.DeepEqual(tt.want, got) { t.Fatalf("DeleteUser() failed: got %v, want %v", got, tt.want) } + + // deleted user details should be available to create user again. + if err == nil { // delete is successful + if err = ks.CreateUser(nil, &CreateUserArgs{Username: testUser, Password: password}, &CreateUserReply{}); err != nil { + t.Fatalf("failed to create user: %v", err) + } + } }) } } From 8c2b71e00b94ccbcb9c0862b50071acff6393a04 Mon Sep 17 00:00:00 2001 From: Anil Dasari Date: Tue, 12 May 2020 00:46:26 +0530 Subject: [PATCH 35/42] refactoring and styling fix --- api/keystore/service.go | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/api/keystore/service.go b/api/keystore/service.go index 98aaac8..0e9394f 100644 --- a/api/keystore/service.go +++ b/api/keystore/service.go @@ -332,14 +332,15 @@ func (ks *Keystore) DeleteUser(_ *http.Request, args *DeleteUserArgs, reply *Del return fmt.Errorf("incorrect password for user %q", args.Username) } + userNameBytes := []byte(args.Username) userBatch := ks.userDB.NewBatch() - if err := userBatch.Delete([]byte(args.Username)); err != nil { + if err := userBatch.Delete(userNameBytes); err != nil { return err } - userDataDB := prefixdb.New([]byte(args.Username), ks.bcDB) + userDataDB := prefixdb.New(userNameBytes, ks.bcDB) dataBatch := userDataDB.NewBatch() - if err := dataBatch.Delete([]byte(args.Username)); err != nil { + if err := dataBatch.Delete(userNameBytes); err != nil { return err } @@ -347,14 +348,6 @@ func (ks *Keystore) DeleteUser(_ *http.Request, args *DeleteUserArgs, reply *Del return err } - // TODO: user is deleted from user db but returned an error due to some other problem. - // valid scenario ? discuss with team. - - // delete from user db. - if err := ks.userDB.Delete([]byte(args.Username)); err != nil { - return err - } - // delete from users map. delete(ks.users, args.Username) From 4ba3a9fd8c1b2d01058b2aa0ac6b067d6c52ae67 Mon Sep 17 00:00:00 2001 From: Anil Dasari Date: Tue, 12 May 2020 03:19:16 +0530 Subject: [PATCH 36/42] review comment: delete prefix db data --- api/keystore/service.go | 22 ++++++++++++++-- api/keystore/service_test.go | 51 +++++++++++++++++++++++++++++------- 2 files changed, 61 insertions(+), 12 deletions(-) diff --git a/api/keystore/service.go b/api/keystore/service.go index 0e9394f..8dcaf43 100644 --- a/api/keystore/service.go +++ b/api/keystore/service.go @@ -339,11 +339,29 @@ func (ks *Keystore) DeleteUser(_ *http.Request, args *DeleteUserArgs, reply *Del } userDataDB := prefixdb.New(userNameBytes, ks.bcDB) - dataBatch := userDataDB.NewBatch() - if err := dataBatch.Delete(userNameBytes); err != nil { + + var data []KeyValuePair + it := userDataDB.NewIterator() + defer it.Release() + + for it.Next() { + data = append(data, KeyValuePair{ + Key: it.Key(), + Value: it.Value(), + }) + } + + if err = it.Error(); err != nil { return err } + dataBatch := userDataDB.NewBatch() + for _, kvp := range data { + if err = dataBatch.Delete(kvp.Key); err != nil { + return err + } + } + if err := atomic.WriteAll(dataBatch, userBatch); err != nil { return err } diff --git a/api/keystore/service_test.go b/api/keystore/service_test.go index 6971fac..75b7166 100644 --- a/api/keystore/service_test.go +++ b/api/keystore/service_test.go @@ -287,6 +287,7 @@ func TestServiceDeleteUser(t *testing.T) { password := "passwTest@fake01ord" tests := []struct { desc string + setup func(ks *Keystore) error request *DeleteUserArgs want *DeleteUserReply wantError bool @@ -303,20 +304,46 @@ func TestServiceDeleteUser(t *testing.T) { request: &DeleteUserArgs{Username: testUser, Password: "password"}, wantError: true, }, { - desc: "user exists and valid password case", + desc: "user exists and valid password case", + setup: func(ks *Keystore) error { + return ks.CreateUser(nil, &CreateUserArgs{Username: testUser, Password: password}, &CreateUserReply{}) + }, + request: &DeleteUserArgs{Username: testUser, Password: password}, + want: &DeleteUserReply{Success: true}, + }, { + desc: "delete a user, imported from import api case", + setup: func(ks *Keystore) error { + + reply := CreateUserReply{} + if err := ks.CreateUser(nil, &CreateUserArgs{Username: testUser, Password: password}, &reply); err != nil { + return err + } + + // created data in bob db + db, err := ks.GetDatabase(ids.Empty, testUser, password) + if err != nil { + return err + } + if err := db.Put([]byte("hello"), []byte("world")); err != nil { + return err + } + + return nil + }, request: &DeleteUserArgs{Username: testUser, Password: password}, want: &DeleteUserReply{Success: true}, }} - ks := Keystore{} - ks.Initialize(logging.NoLog{}, memdb.New()) - - if err := ks.CreateUser(nil, &CreateUserArgs{Username: testUser, Password: password}, &CreateUserReply{}); err != nil { - t.Fatalf("failed to create user: %v", err) - } - for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { + ks := Keystore{} + ks.Initialize(logging.NoLog{}, memdb.New()) + + if tt.setup != nil { + if err := tt.setup(&ks); err != nil { + t.Fatalf("failed to create user setup in keystore: %v", err) + } + } got := &DeleteUserReply{} err := ks.DeleteUser(nil, tt.request, got) if (err != nil) != tt.wantError { @@ -327,8 +354,12 @@ func TestServiceDeleteUser(t *testing.T) { t.Fatalf("DeleteUser() failed: got %v, want %v", got, tt.want) } - // deleted user details should be available to create user again. - if err == nil { // delete is successful + if err == nil && got.Success { // delete is successful + if _, ok := ks.users[testUser]; ok { + t.Fatalf("DeleteUser() failed: expected the user %s should be delete from users map", testUser) + } + + // deleted user details should be available to create user again. if err = ks.CreateUser(nil, &CreateUserArgs{Username: testUser, Password: password}, &CreateUserReply{}); err != nil { t.Fatalf("failed to create user: %v", err) } From 2c644b2fff79b41b5e04b13b7359eb409ead425a Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 11 May 2020 21:58:10 -0400 Subject: [PATCH 37/42] Cleaned up signal handling from PR feedback --- node/node.go | 14 ++++---------- utils/signal.go | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 10 deletions(-) create mode 100644 utils/signal.go diff --git a/node/node.go b/node/node.go index da3d311..7cbb11d 100644 --- a/node/node.go +++ b/node/node.go @@ -15,7 +15,6 @@ import ( "fmt" "io/ioutil" "os" - "os/signal" "path" "sync" "unsafe" @@ -37,6 +36,7 @@ import ( "github.com/ava-labs/gecko/networking/xputtest" "github.com/ava-labs/gecko/snow/triggers" "github.com/ava-labs/gecko/snow/validators" + "github.com/ava-labs/gecko/utils" "github.com/ava-labs/gecko/utils/hashing" "github.com/ava-labs/gecko/utils/logging" "github.com/ava-labs/gecko/utils/wrappers" @@ -151,15 +151,9 @@ func (n *Node) initNetlib() error { n.EC = salticidae.NewEventContext() n.TCall = salticidae.NewThreadCall(n.EC) - c := make(chan os.Signal, 1) - signal.Notify(c, os.Interrupt) - signal.Notify(c, os.Kill) - - go func() { - for range c { - n.TCall.AsyncCall(salticidae.ThreadCallCallback(C.onTerm), nil) - } - }() + utils.HandleSignals(func(os.Signal) { + n.TCall.AsyncCall(salticidae.ThreadCallCallback(C.onTerm), nil) + }, os.Interrupt, os.Kill) // Create peer network config, may have tls enabled peerConfig := salticidae.NewPeerNetworkConfig() diff --git a/utils/signal.go b/utils/signal.go new file mode 100644 index 0000000..0c48999 --- /dev/null +++ b/utils/signal.go @@ -0,0 +1,46 @@ +// (c) 2019-2020, Ava Labs, Inc. All rights reserved. +// See the file LICENSE for licensing terms. + +package utils + +import ( + "os" + "os/signal" +) + +// HandleSignals calls f if the go runtime receives the any of the provided +// signals with the received signal. +// +// If f is nil or there are no provided signals, then nil will be returned. +// Otherwise, a signal channel will be returned that can be used to clear the +// signals registed by this function by valling ClearSignals. +func HandleSignals(f func(os.Signal), sigs ...os.Signal) chan<- os.Signal { + if f == nil || len(sigs) == 0 { + return nil + } + + // register signals + c := make(chan os.Signal, 1) + for _, sig := range sigs { + signal.Notify(c, sig) + } + + go func() { + for sig := range c { + f(sig) + } + }() + + return c +} + +// ClearSignals clears any signals that have been registered on the provided +// channel and closes the channel. +func ClearSignals(c chan<- os.Signal) { + if c == nil { + return + } + + signal.Stop(c) + close(c) +} From ed11d05dfd032de9f945d359cdb6be2ae21d1b02 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 11 May 2020 22:11:28 -0400 Subject: [PATCH 38/42] Close the signal channel on node shutdown --- node/node.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/node/node.go b/node/node.go index 7cbb11d..4cd9932 100644 --- a/node/node.go +++ b/node/node.go @@ -121,6 +121,9 @@ type Node struct { // This node's configuration Config *Config + + // channel for closing the node + nodeCloser chan<- os.Signal } /* @@ -151,7 +154,7 @@ func (n *Node) initNetlib() error { n.EC = salticidae.NewEventContext() n.TCall = salticidae.NewThreadCall(n.EC) - utils.HandleSignals(func(os.Signal) { + n.nodeCloser = utils.HandleSignals(func(os.Signal) { n.TCall.AsyncCall(salticidae.ThreadCallCallback(C.onTerm), nil) }, os.Interrupt, os.Kill) @@ -660,4 +663,5 @@ func (n *Node) Shutdown() { n.ValidatorAPI.Shutdown() n.ConsensusAPI.Shutdown() n.chainManager.Shutdown() + utils.ClearSignals(n.nodeCloser) } From 0c2354dbaee8eabe59a9047ec26bcf143c338df1 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 11 May 2020 22:24:28 -0400 Subject: [PATCH 39/42] Fixed race in platformvm repolling --- vms/platformvm/vm_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/vms/platformvm/vm_test.go b/vms/platformvm/vm_test.go index 70174e7..8c0a7f9 100644 --- a/vms/platformvm/vm_test.go +++ b/vms/platformvm/vm_test.go @@ -1632,6 +1632,7 @@ func TestBootstrapPartiallyAccepted(t *testing.T) { externalSender.GetF = nil externalSender.CantPushQuery = false + externalSender.CantPullQuery = false engine.Put(ctx.NodeID, *reqID, advanceTimeBlkID, advanceTimeBlkBytes) From b96ff7099ed9f5380551b031a8506cf0809cd8a2 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 11 May 2020 23:21:08 -0400 Subject: [PATCH 40/42] Fixed test-inventory key location --- scripts/ansible/restart_playbook.yml | 4 ++-- scripts/ansible/test-inventory.yml | 20 ++++++++++---------- scripts/ansible/update_playbook.yml | 4 ++-- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/scripts/ansible/restart_playbook.yml b/scripts/ansible/restart_playbook.yml index ee43d0e..e3c011f 100755 --- a/scripts/ansible/restart_playbook.yml +++ b/scripts/ansible/restart_playbook.yml @@ -7,8 +7,8 @@ vars: ava_binary: ~/go/src/github.com/ava-labs/gecko/build/ava repo_folder: ~/go/src/github.com/ava-labs/gecko - repo_name: ava-labs/gecko-internal - repo_branch: platformvm-proposal-accept + repo_name: ava-labs/gecko + repo_branch: master roles: - name: ava-stop - name: ava-build diff --git a/scripts/ansible/test-inventory.yml b/scripts/ansible/test-inventory.yml index 220a8f4..dccde9e 100755 --- a/scripts/ansible/test-inventory.yml +++ b/scripts/ansible/test-inventory.yml @@ -2,8 +2,8 @@ borealis_bootstrap: hosts: bootstrap1: ansible_host: 3.84.129.247 - staking_tls_key_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/keys/local/staker1.key" - staking_tls_cert_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/keys/local/staker1.crt" + staking_tls_key_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/staking/local/staker1.key" + staking_tls_cert_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/staking/local/staker1.crt" vars: ansible_connection: ssh ansible_user: ubuntu @@ -44,20 +44,20 @@ borealis_node: hosts: node1: ansible_host: 35.153.99.244 - staking_tls_key_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/keys/local/staker2.key" - staking_tls_cert_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/keys/local/staker2.crt" + staking_tls_key_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/staking/local/staker2.key" + staking_tls_cert_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/staking/local/staker2.crt" node2: ansible_host: 34.201.137.119 - staking_tls_key_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/keys/local/staker3.key" - staking_tls_cert_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/keys/local/staker3.crt" + staking_tls_key_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/staking/local/staker3.key" + staking_tls_cert_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/staking/local/staker3.crt" node3: ansible_host: 54.146.1.110 - staking_tls_key_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/keys/local/staker4.key" - staking_tls_cert_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/keys/local/staker4.crt" + staking_tls_key_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/staking/local/staker4.key" + staking_tls_cert_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/staking/local/staker4.crt" node4: ansible_host: 54.91.255.231 - staking_tls_key_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/keys/local/staker5.key" - staking_tls_cert_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/keys/local/staker5.crt" + staking_tls_key_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/staking/local/staker5.key" + staking_tls_cert_file: "/home/ubuntu/go/src/github.com/ava-labs/gecko/staking/local/staker5.crt" vars: ansible_connection: ssh ansible_user: ubuntu diff --git a/scripts/ansible/update_playbook.yml b/scripts/ansible/update_playbook.yml index b704eee..7386eaa 100755 --- a/scripts/ansible/update_playbook.yml +++ b/scripts/ansible/update_playbook.yml @@ -7,8 +7,8 @@ vars: ava_binary: ~/go/src/github.com/ava-labs/gecko/build/ava repo_folder: ~/go/src/github.com/ava-labs/gecko - repo_name: ava-labs/gecko-internal - repo_branch: platformvm-proposal-accept + repo_name: ava-labs/gecko + repo_branch: master roles: - name: ava-stop - name: ava-build From 59e38e369ca5190111d0e4d43709092621c3c6a8 Mon Sep 17 00:00:00 2001 From: Anil Dasari Date: Tue, 12 May 2020 10:10:46 +0530 Subject: [PATCH 41/42] error descrption fix --- api/keystore/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/keystore/service.go b/api/keystore/service.go index 8dcaf43..c167e07 100644 --- a/api/keystore/service.go +++ b/api/keystore/service.go @@ -327,7 +327,7 @@ func (ks *Keystore) DeleteUser(_ *http.Request, args *DeleteUserArgs, reply *Del usr, err := ks.getUser(args.Username) switch { case err != nil || usr == nil: - return fmt.Errorf("user doesn't exists: %s", args.Username) + return fmt.Errorf("user doesn't exist: %s", args.Username) case !usr.CheckPassword(args.Password): return fmt.Errorf("incorrect password for user %q", args.Username) } From deb71d519c0af9804e0de027dddb091a263ec003 Mon Sep 17 00:00:00 2001 From: Anil Dasari Date: Tue, 12 May 2020 10:15:33 +0530 Subject: [PATCH 42/42] remove keyvalue slice --- api/keystore/service.go | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/api/keystore/service.go b/api/keystore/service.go index c167e07..fe1b349 100644 --- a/api/keystore/service.go +++ b/api/keystore/service.go @@ -339,29 +339,21 @@ func (ks *Keystore) DeleteUser(_ *http.Request, args *DeleteUserArgs, reply *Del } userDataDB := prefixdb.New(userNameBytes, ks.bcDB) + dataBatch := userDataDB.NewBatch() - var data []KeyValuePair it := userDataDB.NewIterator() defer it.Release() for it.Next() { - data = append(data, KeyValuePair{ - Key: it.Key(), - Value: it.Value(), - }) + if err = dataBatch.Delete(it.Key()); err != nil { + return err + } } if err = it.Error(); err != nil { return err } - dataBatch := userDataDB.NewBatch() - for _, kvp := range data { - if err = dataBatch.Delete(kvp.Key); err != nil { - return err - } - } - if err := atomic.WriteAll(dataBatch, userBatch); err != nil { return err }