From bfe72ecf1c23471ee14aa6aa4554930d3b73a45e Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Fri, 10 Apr 2020 14:48:38 -0400 Subject: [PATCH 01/33] add ListAssets API method --- vms/avm/service.go | 77 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 72 insertions(+), 5 deletions(-) diff --git a/vms/avm/service.go b/vms/avm/service.go index a96200d..0d72e30 100644 --- a/vms/avm/service.go +++ b/vms/avm/service.go @@ -7,6 +7,7 @@ import ( "bytes" "errors" "fmt" + "math" "net/http" "github.com/ava-labs/gecko/ids" @@ -15,7 +16,7 @@ import ( "github.com/ava-labs/gecko/utils/formatting" "github.com/ava-labs/gecko/utils/hashing" "github.com/ava-labs/gecko/utils/json" - "github.com/ava-labs/gecko/utils/math" + safemath "github.com/ava-labs/gecko/utils/math" "github.com/ava-labs/gecko/vms/components/ava" "github.com/ava-labs/gecko/vms/components/verify" "github.com/ava-labs/gecko/vms/secp256k1fx" @@ -221,7 +222,7 @@ func (service *Service) GetBalance(r *http.Request, args *GetBalanceArgs, reply if !ok { continue } - amt, err := math.Add64(transferable.Amount(), uint64(reply.Balance)) + amt, err := safemath.Add64(transferable.Amount(), uint64(reply.Balance)) if err != nil { return err } @@ -231,6 +232,72 @@ func (service *Service) GetBalance(r *http.Request, args *GetBalanceArgs, reply return nil } +// ListAssetsArgs are arguments for calling into ListAssets +type ListAssetsArgs struct { + Address string `json:"address"` +} + +type listAssetsReplyElement struct { + AssetID string `json:"assetID"` + Balance json.Uint64 `json:"balance"` +} + +// ListAssetsReply is the response from a call to ListAssets +type ListAssetsReply struct { + Assets []listAssetsReplyElement `json:"assets"` +} + +// ListAssets returns a list of maps where each map is: +// Key: ID of an asset such that [args.Address] has a non-zero balance of the asset +// Value: The balance of the asset held by the address +// Returns null if the address holds no assets +// Note that balances include assets that the address only _partially_ owns +// (ie is one of several addresses specified in a multi-sig) +func (service *Service) ListAssets(r *http.Request, args *ListAssetsArgs, reply *ListAssetsReply) error { + service.vm.ctx.Log.Verbo("ListAssets called with address: %s", args.Address) + + address, err := service.vm.Parse(args.Address) + if err != nil { + return fmt.Errorf("couldn't parse given address: %s", err) + } + addrAsSet := ids.Set{} + addrAsSet.Add(ids.NewID(hashing.ComputeHash256Array(address))) + + utxos, err := service.vm.GetUTXOs(addrAsSet) + if err != nil { + return fmt.Errorf("couldn't get address's UTXOs: %s", err) + } + + assetIDs := ids.Set{} // IDs of assets the address has a non-zero balance of + balances := make(map[[32]byte]uint64, 0) // key: ID (as bytes). value: balance of that asset + for _, utxo := range utxos { + transferable, ok := utxo.Out.(ava.Transferable) + if !ok { + continue + } + assetID := utxo.AssetID() + assetIDs.Add(assetID) + if balance, ok := balances[assetID.Key()]; ok { + balance, err := safemath.Add64(transferable.Amount(), balance) + if err != nil { + balances[assetID.Key()] = math.MaxUint64 + } else { + balances[assetID.Key()] = balance + } + } else { + balances[assetID.Key()] = transferable.Amount() + } + } + + sortedAssetIDs := assetIDs.List() // sort so response is always in same order + ids.SortIDs(sortedAssetIDs) + for _, assetID := range sortedAssetIDs { + reply.Assets = append(reply.Assets, listAssetsReplyElement{assetID.String(), json.Uint64(balances[assetID.Key()])}) + } + + return nil +} + // CreateFixedCapAssetArgs are arguments for passing into CreateFixedCapAsset requests type CreateFixedCapAssetArgs struct { Username string `json:"username"` @@ -613,7 +680,7 @@ func (service *Service) Send(r *http.Request, args *SendArgs, reply *SendReply) if !ok { continue } - spent, err := math.Add64(amountSpent, input.Amount()) + spent, err := safemath.Add64(amountSpent, input.Amount()) if err != nil { return errSpendOverflow } @@ -1020,7 +1087,7 @@ func (service *Service) ImportAVA(_ *http.Request, args *ImportAVAArgs, reply *I if !ok { continue } - spent, err := math.Add64(amount, input.Amount()) + spent, err := safemath.Add64(amount, input.Amount()) if err != nil { return errSpendOverflow } @@ -1164,7 +1231,7 @@ func (service *Service) ExportAVA(_ *http.Request, args *ExportAVAArgs, reply *E if !ok { continue } - spent, err := math.Add64(amountSpent, input.Amount()) + spent, err := safemath.Add64(amountSpent, input.Amount()) if err != nil { return errSpendOverflow } From 53b29745b6cb79280d2de76ef503bfb3c99ddec0 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Fri, 10 Apr 2020 16:05:11 -0400 Subject: [PATCH 02/33] rename method to GetAllBalances; change return type to map --- vms/avm/service.go | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/vms/avm/service.go b/vms/avm/service.go index 0d72e30..b16d5e2 100644 --- a/vms/avm/service.go +++ b/vms/avm/service.go @@ -232,29 +232,23 @@ func (service *Service) GetBalance(r *http.Request, args *GetBalanceArgs, reply return nil } -// ListAssetsArgs are arguments for calling into ListAssets -type ListAssetsArgs struct { +// GetAllBalancesArgs are arguments for calling into GetAllBalances +type GetAllBalancesArgs struct { Address string `json:"address"` } -type listAssetsReplyElement struct { - AssetID string `json:"assetID"` - Balance json.Uint64 `json:"balance"` +// GetAllBalancesReply is the response from a call to GetAllBalances +type GetAllBalancesReply struct { + Balances map[string]json.Uint64 `json:"balances"` } -// ListAssetsReply is the response from a call to ListAssets -type ListAssetsReply struct { - Assets []listAssetsReplyElement `json:"assets"` -} - -// ListAssets returns a list of maps where each map is: +// GetAllBalances returns a map where: // Key: ID of an asset such that [args.Address] has a non-zero balance of the asset // Value: The balance of the asset held by the address -// Returns null if the address holds no assets // Note that balances include assets that the address only _partially_ owns // (ie is one of several addresses specified in a multi-sig) -func (service *Service) ListAssets(r *http.Request, args *ListAssetsArgs, reply *ListAssetsReply) error { - service.vm.ctx.Log.Verbo("ListAssets called with address: %s", args.Address) +func (service *Service) GetAllBalances(r *http.Request, args *GetAllBalancesArgs, reply *GetAllBalancesReply) error { + service.vm.ctx.Log.Verbo("GetAllBalances called with address: %s", args.Address) address, err := service.vm.Parse(args.Address) if err != nil { @@ -291,8 +285,9 @@ func (service *Service) ListAssets(r *http.Request, args *ListAssetsArgs, reply sortedAssetIDs := assetIDs.List() // sort so response is always in same order ids.SortIDs(sortedAssetIDs) + reply.Balances = make(map[string]json.Uint64, len(sortedAssetIDs)) for _, assetID := range sortedAssetIDs { - reply.Assets = append(reply.Assets, listAssetsReplyElement{assetID.String(), json.Uint64(balances[assetID.Key()])}) + reply.Balances[assetID.String()] = json.Uint64(balances[assetID.Key()]) } return nil From 95cca6f29c299f0583211f6a104298bd477423a6 Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Sun, 19 Apr 2020 13:48:36 +0100 Subject: [PATCH 03/33] 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 04/33] 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 05/33] 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 06/33] 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 07/33] 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 08/33] 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 09/33] 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 10/33] 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 11/33] 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 12/33] 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 13/33] 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 14/33] 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 10aa724d30f2d499d0424558550b24341b1d0a9e Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Fri, 1 May 2020 13:28:38 -0400 Subject: [PATCH 15/33] don't try to sort balances since maps aren't sorted by key. simplify balance calculation since zero value of uint64 is 0. --- vms/avm/service.go | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/vms/avm/service.go b/vms/avm/service.go index b16d5e2..8b90278 100644 --- a/vms/avm/service.go +++ b/vms/avm/service.go @@ -271,22 +271,17 @@ func (service *Service) GetAllBalances(r *http.Request, args *GetAllBalancesArgs } assetID := utxo.AssetID() assetIDs.Add(assetID) - if balance, ok := balances[assetID.Key()]; ok { - balance, err := safemath.Add64(transferable.Amount(), balance) - if err != nil { - balances[assetID.Key()] = math.MaxUint64 - } else { - balances[assetID.Key()] = balance - } + balance := balances[assetID.Key()] // 0 if key doesn't exist + balance, err := safemath.Add64(transferable.Amount(), balance) + if err != nil { + balances[assetID.Key()] = math.MaxUint64 } else { - balances[assetID.Key()] = transferable.Amount() + balances[assetID.Key()] = balance } } - sortedAssetIDs := assetIDs.List() // sort so response is always in same order - ids.SortIDs(sortedAssetIDs) - reply.Balances = make(map[string]json.Uint64, len(sortedAssetIDs)) - for _, assetID := range sortedAssetIDs { + reply.Balances = make(map[string]json.Uint64, assetIDs.Len()) + for _, assetID := range assetIDs.List() { reply.Balances[assetID.String()] = json.Uint64(balances[assetID.Key()]) } From acb96c8184969f320416453a928912f9469c60d7 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Sat, 2 May 2020 13:22:11 -0400 Subject: [PATCH 16/33] check for nil account IDs in get user --- vms/platformvm/user.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/vms/platformvm/user.go b/vms/platformvm/user.go index e8d9c7e..edb13c5 100644 --- a/vms/platformvm/user.go +++ b/vms/platformvm/user.go @@ -104,6 +104,9 @@ func (u *user) getKey(accountID ids.ShortID) (*crypto.PrivateKeySECP256K1R, erro if u.db == nil { return nil, errDbNil } + if accountID.IsZero() { + return nil, errEmptyAccountAddress + } factory := crypto.FactorySECP256K1R{} bytes, err := u.db.Get(accountID.Bytes()) From 4c3fce408e2cab49104e1ea6ac24488e138a7798 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Sat, 2 May 2020 14:05:57 -0400 Subject: [PATCH 17/33] Added user tests --- vms/platformvm/reward_validator_tx.go | 2 +- vms/platformvm/service.go | 5 +- vms/platformvm/user.go | 29 ++++--- vms/platformvm/user_test.go | 108 ++++++++++++++++++++++++++ 4 files changed, 129 insertions(+), 15 deletions(-) create mode 100644 vms/platformvm/user_test.go diff --git a/vms/platformvm/reward_validator_tx.go b/vms/platformvm/reward_validator_tx.go index fa1d309..2903e42 100644 --- a/vms/platformvm/reward_validator_tx.go +++ b/vms/platformvm/reward_validator_tx.go @@ -63,7 +63,7 @@ func (tx *rewardValidatorTx) SemanticVerify(db database.Database) (*versiondb.Da return nil, nil, nil, nil, err } if db == nil { - return nil, nil, nil, nil, errDbNil + return nil, nil, nil, nil, errDBNil } currentEvents, err := tx.vm.getCurrentValidators(db, DefaultSubnetID) diff --git a/vms/platformvm/service.go b/vms/platformvm/service.go index 86093b0..e2a9ffe 100644 --- a/vms/platformvm/service.go +++ b/vms/platformvm/service.go @@ -322,7 +322,7 @@ func (service *Service) ListAccounts(_ *http.Request, args *ListAccountsArgs, re return errGetAccounts } - var accounts []APIAccount + 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 { @@ -331,13 +331,12 @@ func (service *Service) ListAccounts(_ *http.Request, args *ListAccountsArgs, re } else if err == database.ErrNotFound { account = newAccount(accountID, 0, 0) } - accounts = append(accounts, APIAccount{ + reply.Accounts = append(reply.Accounts, APIAccount{ Address: accountID, Nonce: json.Uint64(account.Nonce), Balance: json.Uint64(account.Balance), }) } - reply.Accounts = accounts return nil } diff --git a/vms/platformvm/user.go b/vms/platformvm/user.go index edb13c5..45f7ce9 100644 --- a/vms/platformvm/user.go +++ b/vms/platformvm/user.go @@ -15,7 +15,10 @@ import ( // account IDs this user controls var accountIDsKey = ids.Empty.Bytes() -var errDbNil = errors.New("db uninitialized") +var ( + errDBNil = errors.New("db uninitialized") + errKeyNil = errors.New("key uninitialized") +) type user struct { // This user's database, acquired from the keystore @@ -25,7 +28,7 @@ type user struct { // Get the IDs of the accounts controlled by this user func (u *user) getAccountIDs() ([]ids.ShortID, error) { if u.db == nil { - return nil, errDbNil + return nil, errDBNil } // If user has no accounts, return empty list @@ -34,8 +37,9 @@ func (u *user) getAccountIDs() ([]ids.ShortID, error) { return nil, errDB } if !hasAccounts { - return make([]ids.ShortID, 0), nil + return nil, nil } + // User has accounts. Get them. bytes, err := u.db.Get(accountIDsKey) if err != nil { @@ -50,21 +54,24 @@ func (u *user) getAccountIDs() ([]ids.ShortID, error) { // controlsAccount returns true iff this user controls the account // with the specified ID -func (u *user) controlsAccount(ID ids.ShortID) (bool, error) { +func (u *user) controlsAccount(accountID ids.ShortID) (bool, error) { if u.db == nil { - return false, errDbNil + return false, errDBNil } - - if _, err := u.db.Get(ID.Bytes()); err == nil { - return true, nil + if accountID.IsZero() { + return false, errEmptyAccountAddress } - return false, nil + return u.db.Has(accountID.Bytes()) } // putAccount persists that this user controls the account whose ID is // [privKey].PublicKey().Address() func (u *user) putAccount(privKey *crypto.PrivateKeySECP256K1R) error { - newAccountID := privKey.PublicKey().Address() // Account thie privKey controls + if privKey == nil { + return errKeyNil + } + + newAccountID := privKey.PublicKey().Address() // Account the privKey controls controlsAccount, err := u.controlsAccount(newAccountID) if err != nil { return err @@ -102,7 +109,7 @@ func (u *user) putAccount(privKey *crypto.PrivateKeySECP256K1R) error { // Key returns the private key that controls the account with the specified ID func (u *user) getKey(accountID ids.ShortID) (*crypto.PrivateKeySECP256K1R, error) { if u.db == nil { - return nil, errDbNil + return nil, errDBNil } if accountID.IsZero() { return nil, errEmptyAccountAddress diff --git a/vms/platformvm/user_test.go b/vms/platformvm/user_test.go new file mode 100644 index 0000000..758be35 --- /dev/null +++ b/vms/platformvm/user_test.go @@ -0,0 +1,108 @@ +// (c) 2019-2020, Ava Labs, Inc. All rights reserved. +// See the file LICENSE for licensing terms. + +package platformvm + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/ava-labs/gecko/database/memdb" + "github.com/ava-labs/gecko/ids" + "github.com/ava-labs/gecko/utils/crypto" +) + +func TestUserNilDB(t *testing.T) { + u := user{} + + _, err := u.getAccountIDs() + assert.Error(t, err, "nil db should have caused an error") + + _, err = u.controlsAccount(ids.ShortEmpty) + assert.Error(t, err, "nil db should have caused an error") + + _, err = u.getKey(ids.ShortEmpty) + assert.Error(t, err, "nil db should have caused an error") + + factory := crypto.FactorySECP256K1R{} + sk, err := factory.NewPrivateKey() + assert.NoError(t, err) + + err = u.putAccount(sk.(*crypto.PrivateKeySECP256K1R)) + assert.Error(t, err, "nil db should have caused an error") +} + +func TestUserClosedDB(t *testing.T) { + db := memdb.New() + err := db.Close() + assert.NoError(t, err) + + u := user{db: db} + + _, err = u.getAccountIDs() + assert.Error(t, err, "closed db should have caused an error") + + _, err = u.controlsAccount(ids.ShortEmpty) + assert.Error(t, err, "closed db should have caused an error") + + _, err = u.getKey(ids.ShortEmpty) + assert.Error(t, err, "closed db should have caused an error") + + factory := crypto.FactorySECP256K1R{} + sk, err := factory.NewPrivateKey() + assert.NoError(t, err) + + err = u.putAccount(sk.(*crypto.PrivateKeySECP256K1R)) + assert.Error(t, err, "closed db should have caused an error") +} + +func TestUserNilSK(t *testing.T) { + u := user{db: memdb.New()} + + err := u.putAccount(nil) + assert.Error(t, err, "nil key should have caused an error") +} + +func TestUserNilAccount(t *testing.T) { + u := user{db: memdb.New()} + + _, err := u.controlsAccount(ids.ShortID{}) + assert.Error(t, err, "nil accountID should have caused an error") + + _, err = u.getKey(ids.ShortID{}) + assert.Error(t, err, "nil accountID should have caused an error") +} + +func TestUser(t *testing.T) { + u := user{db: memdb.New()} + + accountIDs, err := u.getAccountIDs() + assert.NoError(t, err) + assert.Empty(t, accountIDs, "new user shouldn't have accounts") + + factory := crypto.FactorySECP256K1R{} + sk, err := factory.NewPrivateKey() + assert.NoError(t, err) + + err = u.putAccount(sk.(*crypto.PrivateKeySECP256K1R)) + assert.NoError(t, err) + + addr := sk.PublicKey().Address() + + ok, err := u.controlsAccount(addr) + assert.NoError(t, err) + assert.True(t, ok, "added account should have been marked as controlled") + + savedSk, err := u.getKey(addr) + assert.NoError(t, err) + assert.Equal(t, sk.Bytes(), savedSk.Bytes(), "wrong key returned") + + accountIDs, err = u.getAccountIDs() + assert.NoError(t, err) + assert.Len(t, accountIDs, 1, "account should have been added") + + savedAddr := accountIDs[0] + equals := addr.Equals(savedAddr) + assert.True(t, equals, "saved address should match provided address") +} From 4a989dc62132bc5d744520fbdf0ddddcf0a0e4a0 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 4 May 2020 02:32:10 -0400 Subject: [PATCH 18/33] Added uniform periodic gossiping --- chains/manager.go | 3 +- networking/voting_handlers.go | 78 ++++++++++++------- snow/engine/avalanche/bootstrapper_test.go | 3 +- snow/engine/avalanche/transitive.go | 20 +++++ snow/engine/common/engine.go | 3 + snow/engine/common/sender.go | 8 ++ snow/engine/common/test_engine.go | 13 +++- snow/engine/common/test_sender.go | 16 +++- snow/engine/snowman/bootstrapper_test.go | 3 +- snow/engine/snowman/transitive.go | 13 ++++ snow/networking/handler/handler.go | 5 ++ snow/networking/handler/message.go | 3 + snow/networking/router/router.go | 4 +- snow/networking/router/subnet_router.go | 20 ++++- snow/networking/sender/external_sender.go | 2 + snow/networking/sender/sender.go | 6 ++ snow/networking/sender/sender_test.go | 2 +- .../networking/sender/test_external_sender.go | 18 ++++- 18 files changed, 183 insertions(+), 37 deletions(-) diff --git a/chains/manager.go b/chains/manager.go index 3f7d48d..3ce5fd6 100644 --- a/chains/manager.go +++ b/chains/manager.go @@ -40,6 +40,7 @@ import ( const ( defaultChannelSize = 1000 requestTimeout = 2 * time.Second + gossipFrequency = 10 * time.Second ) // Manager manages the chains running on this node. @@ -146,7 +147,7 @@ func New( timeoutManager.Initialize(requestTimeout) go log.RecoverAndPanic(timeoutManager.Dispatch) - router.Initialize(log, &timeoutManager) + router.Initialize(log, &timeoutManager, gossipFrequency) m := &manager{ stakingEnabled: stakingEnabled, diff --git a/networking/voting_handlers.go b/networking/voting_handlers.go index a5746d4..f6a43e3 100644 --- a/networking/voting_handlers.go +++ b/networking/voting_handlers.go @@ -18,6 +18,7 @@ import "C" import ( "errors" "fmt" + "math" "unsafe" "github.com/prometheus/client_golang/prometheus" @@ -29,9 +30,15 @@ import ( "github.com/ava-labs/gecko/snow/validators" "github.com/ava-labs/gecko/utils/formatting" "github.com/ava-labs/gecko/utils/logging" + "github.com/ava-labs/gecko/utils/random" "github.com/ava-labs/gecko/utils/timer" ) +// GossipSize is the maximum number of peers to gossip a container to +const ( + GossipSize = 50 +) + var ( // VotingNet implements the SenderExternal interface. VotingNet = Voting{} @@ -89,34 +96,7 @@ func (s *Voting) Shutdown() { s.executor.Stop() } // Accept is called after every consensus decision func (s *Voting) Accept(chainID, containerID ids.ID, container []byte) error { - peers := []salticidae.PeerID(nil) - - allPeers, allIDs, _ := s.conns.Conns() - for i, id := range allIDs { - if !s.vdrs.Contains(id) { - peers = append(peers, allPeers[i]) - } - } - - build := Builder{} - msg, err := build.Put(chainID, 0, containerID, container) - if err != nil { - return fmt.Errorf("Attempted to pack too large of a Put message.\nContainer length: %d: %w", len(container), err) - } - - s.log.Verbo("Sending a Put message to non-validators."+ - "\nNumber of Non-Validators: %d"+ - "\nChain: %s"+ - "\nContainer ID: %s"+ - "\nContainer:\n%s", - len(peers), - chainID, - containerID, - formatting.DumpBytes{Bytes: container}, - ) - s.send(msg, peers...) - s.numPutSent.Add(float64(len(peers))) - return nil + return s.gossip(chainID, containerID, container) } // GetAcceptedFrontier implements the Sender interface. @@ -412,6 +392,13 @@ func (s *Voting) Chits(validatorID ids.ShortID, chainID ids.ID, requestID uint32 s.numChitsSent.Inc() } +// Gossip attempts to gossip the container to the network +func (s *Voting) Gossip(chainID, containerID ids.ID, container []byte) { + if err := s.gossip(chainID, containerID, container); err != nil { + s.log.Error("Error gossiping container %s to %s\n%s", containerID, chainID, err) + } +} + func (s *Voting) send(msg Msg, peers ...salticidae.PeerID) { ds := msg.DataStream() defer ds.Free() @@ -429,6 +416,41 @@ func (s *Voting) send(msg Msg, peers ...salticidae.PeerID) { } } +func (s *Voting) gossip(chainID, containerID ids.ID, container []byte) error { + allPeers := s.conns.PeerIDs() + + numToGossip := GossipSize + if numToGossip > len(allPeers) { + numToGossip = len(allPeers) + } + peers := make([]salticidae.PeerID, numToGossip) + + sampler := random.Uniform{N: len(allPeers)} + for i := range peers { + peers[i] = allPeers[sampler.Sample()] + } + + build := Builder{} + msg, err := build.Put(chainID, math.MaxUint32, containerID, container) + if err != nil { + return fmt.Errorf("Attempted to pack too large of a Put message.\nContainer length: %d: %w", len(container), err) + } + + s.log.Verbo("Sending a Put message to non-validators."+ + "\nNumber of Non-Validators: %d"+ + "\nChain: %s"+ + "\nContainer ID: %s"+ + "\nContainer:\n%s", + len(peers), + chainID, + containerID, + formatting.DumpBytes{Bytes: container}, + ) + s.send(msg, peers...) + s.numPutSent.Add(float64(len(peers))) + return nil +} + // getAcceptedFrontier handles the recept of a getAcceptedFrontier container // message for a chain //export getAcceptedFrontier diff --git a/snow/engine/avalanche/bootstrapper_test.go b/snow/engine/avalanche/bootstrapper_test.go index cc63b68..3fa2cbe 100644 --- a/snow/engine/avalanche/bootstrapper_test.go +++ b/snow/engine/avalanche/bootstrapper_test.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "testing" + "time" "github.com/prometheus/client_golang/prometheus" @@ -60,7 +61,7 @@ func newConfig(t *testing.T) (BootstrapConfig, ids.ShortID, *common.SenderTest, handler.Initialize(engine, make(chan common.Message), 1) timeouts.Initialize(0) - router.Initialize(ctx.Log, timeouts) + router.Initialize(ctx.Log, timeouts, time.Hour) vtxBlocker, _ := queue.New(prefixdb.New([]byte("vtx"), db)) txBlocker, _ := queue.New(prefixdb.New([]byte("tx"), db)) diff --git a/snow/engine/avalanche/transitive.go b/snow/engine/avalanche/transitive.go index fc55250..4b33007 100644 --- a/snow/engine/avalanche/transitive.go +++ b/snow/engine/avalanche/transitive.go @@ -62,6 +62,26 @@ func (t *Transitive) finishBootstrapping() { t.bootstrapped = true } +// Gossip implements the Engine interface +func (t *Transitive) Gossip() { + edge := t.Config.State.Edge() + if len(edge) == 0 { + t.Config.Context.Log.Debug("Dropping gossip request as no vertices have been accepted") + return + } + + sampler := random.Uniform{N: len(edge)} + vtxID := edge[sampler.Sample()] + vtx, err := t.Config.State.GetVertex(vtxID) + if err != nil { + t.Config.Context.Log.Warn("Dropping gossip request as %s couldn't be loaded due to %s", vtxID, err) + return + } + + t.Config.Context.Log.Info("Gossiping %s as accepted to the network", vtxID) + t.Config.Sender.Gossip(vtxID, vtx.Bytes()) +} + // Shutdown implements the Engine interface func (t *Transitive) Shutdown() { t.Config.Context.Log.Info("Shutting down Avalanche consensus") diff --git a/snow/engine/common/engine.go b/snow/engine/common/engine.go index c4e431b..06761de 100644 --- a/snow/engine/common/engine.go +++ b/snow/engine/common/engine.go @@ -112,6 +112,9 @@ type InternalHandler interface { // Startup this engine. Startup() + // Gossip to the network a container on the accepted frontier + Gossip() + // Shutdown this engine. Shutdown() diff --git a/snow/engine/common/sender.go b/snow/engine/common/sender.go index d808b3a..a72375e 100644 --- a/snow/engine/common/sender.go +++ b/snow/engine/common/sender.go @@ -14,6 +14,7 @@ type Sender interface { AcceptedSender FetchSender QuerySender + Gossiper } // FrontierSender defines how a consensus engine sends frontier messages to @@ -70,3 +71,10 @@ type QuerySender interface { // Chits sends chits to the specified validator Chits(validatorID ids.ShortID, requestID uint32, votes ids.Set) } + +// Gossiper defines how a consensus engine gossips a container on the accepted +// frontier to other validators +type Gossiper interface { + // Gossip gossips the provided container throughout the network + Gossip(containerID ids.ID, container []byte) +} diff --git a/snow/engine/common/test_engine.go b/snow/engine/common/test_engine.go index d708008..27a07f5 100644 --- a/snow/engine/common/test_engine.go +++ b/snow/engine/common/test_engine.go @@ -15,6 +15,7 @@ type EngineTest struct { T *testing.T CantStartup, + CantGossip, CantShutdown, CantContext, @@ -38,7 +39,7 @@ type EngineTest struct { CantQueryFailed, CantChits bool - StartupF, ShutdownF func() + StartupF, GossipF, ShutdownF func() ContextF func() *snow.Context NotifyF func(Message) GetF, GetFailedF, PullQueryF func(validatorID ids.ShortID, requestID uint32, containerID ids.ID) @@ -50,6 +51,7 @@ type EngineTest struct { // Default ... func (e *EngineTest) Default(cant bool) { e.CantStartup = cant + e.CantGossip = cant e.CantShutdown = cant e.CantContext = cant @@ -83,6 +85,15 @@ func (e *EngineTest) Startup() { } } +// Gossip ... +func (e *EngineTest) Gossip() { + if e.GossipF != nil { + e.GossipF() + } else if e.CantGossip && e.T != nil { + e.T.Fatalf("Unexpectedly called Gossip") + } +} + // Shutdown ... func (e *EngineTest) Shutdown() { if e.ShutdownF != nil { diff --git a/snow/engine/common/test_sender.go b/snow/engine/common/test_sender.go index 10dd9c2..435f38b 100644 --- a/snow/engine/common/test_sender.go +++ b/snow/engine/common/test_sender.go @@ -16,7 +16,8 @@ type SenderTest struct { CantGetAcceptedFrontier, CantAcceptedFrontier, CantGetAccepted, CantAccepted, CantGet, CantPut, - CantPullQuery, CantPushQuery, CantChits bool + CantPullQuery, CantPushQuery, CantChits, + CantGossip bool GetAcceptedFrontierF func(ids.ShortSet, uint32) AcceptedFrontierF func(ids.ShortID, uint32, ids.Set) @@ -27,6 +28,7 @@ type SenderTest struct { PushQueryF func(ids.ShortSet, uint32, ids.ID, []byte) PullQueryF func(ids.ShortSet, uint32, ids.ID) ChitsF func(ids.ShortID, uint32, ids.Set) + GossipF func(ids.ID, []byte) } // Default set the default callable value to [cant] @@ -40,6 +42,7 @@ func (s *SenderTest) Default(cant bool) { s.CantPullQuery = cant s.CantPushQuery = cant s.CantChits = cant + s.CantGossip = cant } // GetAcceptedFrontier calls GetAcceptedFrontierF if it was initialized. If it @@ -140,3 +143,14 @@ func (s *SenderTest) Chits(vdr ids.ShortID, requestID uint32, votes ids.Set) { s.T.Fatalf("Unexpectedly called Chits") } } + +// Gossip calls GossipF if it was initialized. If it wasn't initialized and this +// function shouldn't be called and testing was initialized, then testing will +// fail. +func (s *SenderTest) Gossip(containerID ids.ID, container []byte) { + if s.GossipF != nil { + s.GossipF(containerID, container) + } else if s.CantGossip && s.T != nil { + s.T.Fatalf("Unexpectedly called Gossip") + } +} diff --git a/snow/engine/snowman/bootstrapper_test.go b/snow/engine/snowman/bootstrapper_test.go index 6168df2..19fa608 100644 --- a/snow/engine/snowman/bootstrapper_test.go +++ b/snow/engine/snowman/bootstrapper_test.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "testing" + "time" "github.com/prometheus/client_golang/prometheus" @@ -54,7 +55,7 @@ func newConfig(t *testing.T) (BootstrapConfig, ids.ShortID, *common.SenderTest, handler.Initialize(engine, make(chan common.Message), 1) timeouts.Initialize(0) - router.Initialize(ctx.Log, timeouts) + router.Initialize(ctx.Log, timeouts, time.Hour) blocker, _ := queue.New(db) diff --git a/snow/engine/snowman/transitive.go b/snow/engine/snowman/transitive.go index 9a827a6..f4778ac 100644 --- a/snow/engine/snowman/transitive.go +++ b/snow/engine/snowman/transitive.go @@ -65,6 +65,19 @@ func (t *Transitive) finishBootstrapping() { } } +// Gossip implements the Engine interface +func (t *Transitive) Gossip() { + blkID := t.Config.VM.LastAccepted() + blk, err := t.Config.VM.GetBlock(blkID) + if err != nil { + t.Config.Context.Log.Warn("Dropping gossip request as %s couldn't be loaded due to %s", blkID, err) + return + } + + t.Config.Context.Log.Info("Gossiping %s as accepted to the network", blkID) + t.Config.Sender.Gossip(blkID, blk.Bytes()) +} + // Shutdown implements the Engine interface func (t *Transitive) Shutdown() { t.Config.Context.Log.Info("Shutting down Snowman consensus") diff --git a/snow/networking/handler/handler.go b/snow/networking/handler/handler.go index dec10b7..12c65be 100644 --- a/snow/networking/handler/handler.go +++ b/snow/networking/handler/handler.go @@ -91,6 +91,8 @@ func (h *Handler) dispatchMsg(msg message) bool { h.engine.Chits(msg.validatorID, msg.requestID, msg.containerIDs) case notifyMsg: h.engine.Notify(msg.notification) + case gossipMsg: + h.engine.Gossip() case shutdownMsg: h.engine.Shutdown() return false @@ -232,6 +234,9 @@ func (h *Handler) QueryFailed(validatorID ids.ShortID, requestID uint32) { } } +// Gossip passes a gossip request to the consensus engine +func (h *Handler) Gossip() { h.msgs <- message{messageType: gossipMsg} } + // Shutdown shuts down the dispatcher func (h *Handler) Shutdown() { h.msgs <- message{messageType: shutdownMsg}; h.wg.Wait() } diff --git a/snow/networking/handler/message.go b/snow/networking/handler/message.go index 27d852d..f5394f0 100644 --- a/snow/networking/handler/message.go +++ b/snow/networking/handler/message.go @@ -29,6 +29,7 @@ const ( chitsMsg queryFailedMsg notifyMsg + gossipMsg shutdownMsg ) @@ -87,6 +88,8 @@ func (t msgType) String() string { return "Query Failed Message" case notifyMsg: return "Notify Message" + case gossipMsg: + return "Gossip Message" case shutdownMsg: return "Shutdown Message" default: diff --git a/snow/networking/router/router.go b/snow/networking/router/router.go index 4ed00f8..fca6983 100644 --- a/snow/networking/router/router.go +++ b/snow/networking/router/router.go @@ -4,6 +4,8 @@ package router import ( + "time" + "github.com/ava-labs/gecko/ids" "github.com/ava-labs/gecko/snow/networking/handler" "github.com/ava-labs/gecko/snow/networking/timeout" @@ -19,7 +21,7 @@ type Router interface { AddChain(chain *handler.Handler) RemoveChain(chainID ids.ID) Shutdown() - Initialize(log logging.Logger, timeouts *timeout.Manager) + Initialize(log logging.Logger, timeouts *timeout.Manager, gossipFrequency time.Duration) } // ExternalRouter routes messages from the network to the diff --git a/snow/networking/router/subnet_router.go b/snow/networking/router/subnet_router.go index ca1f6de..24b6efb 100644 --- a/snow/networking/router/subnet_router.go +++ b/snow/networking/router/subnet_router.go @@ -5,11 +5,13 @@ package router import ( "sync" + "time" "github.com/ava-labs/gecko/ids" "github.com/ava-labs/gecko/snow/networking/handler" "github.com/ava-labs/gecko/snow/networking/timeout" "github.com/ava-labs/gecko/utils/logging" + "github.com/ava-labs/gecko/utils/timer" ) // ChainRouter routes incoming messages from the validator network @@ -21,15 +23,17 @@ type ChainRouter struct { lock sync.RWMutex chains map[[32]byte]*handler.Handler timeouts *timeout.Manager + gossiper *timer.Repeater } // Initialize the router // When this router receives an incoming message, it cancels the timeout in [timeouts] // associated with the request that caused the incoming message, if applicable -func (sr *ChainRouter) Initialize(log logging.Logger, timeouts *timeout.Manager) { +func (sr *ChainRouter) Initialize(log logging.Logger, timeouts *timeout.Manager, gossipFrequency time.Duration) { sr.log = log sr.chains = make(map[[32]byte]*handler.Handler) sr.timeouts = timeouts + sr.gossiper = timer.NewRepeater(sr.Gossip, gossipFrequency) } // AddChain registers the specified chain so that incoming @@ -256,3 +260,17 @@ func (sr *ChainRouter) shutdown() { chain.Shutdown() } } + +// Gossip accepted containers +func (sr *ChainRouter) Gossip() { + sr.lock.RLock() + defer sr.lock.RUnlock() + + sr.gossip() +} + +func (sr *ChainRouter) gossip() { + for _, chain := range sr.chains { + chain.Gossip() + } +} diff --git a/snow/networking/sender/external_sender.go b/snow/networking/sender/external_sender.go index 6bb02db..618ddd0 100644 --- a/snow/networking/sender/external_sender.go +++ b/snow/networking/sender/external_sender.go @@ -20,4 +20,6 @@ type ExternalSender interface { PushQuery(validatorIDs ids.ShortSet, chainID ids.ID, requestID uint32, containerID ids.ID, container []byte) PullQuery(validatorIDs ids.ShortSet, chainID ids.ID, requestID uint32, containerID ids.ID) Chits(validatorID ids.ShortID, chainID ids.ID, requestID uint32, votes ids.Set) + + Gossip(chainID ids.ID, containerID ids.ID, container []byte) } diff --git a/snow/networking/sender/sender.go b/snow/networking/sender/sender.go index f72c842..ed500c3 100644 --- a/snow/networking/sender/sender.go +++ b/snow/networking/sender/sender.go @@ -163,3 +163,9 @@ func (s *Sender) Chits(validatorID ids.ShortID, requestID uint32, votes ids.Set) } s.sender.Chits(validatorID, s.ctx.ChainID, requestID, votes) } + +// Gossip the provided container +func (s *Sender) Gossip(containerID ids.ID, container []byte) { + s.ctx.Log.Verbo("Gossiping %s", containerID) + s.sender.Gossip(s.ctx.ChainID, containerID, container) +} diff --git a/snow/networking/sender/sender_test.go b/snow/networking/sender/sender_test.go index 3d6e3af..5a0ef83 100644 --- a/snow/networking/sender/sender_test.go +++ b/snow/networking/sender/sender_test.go @@ -38,7 +38,7 @@ func TestTimeout(t *testing.T) { go tm.Dispatch() router := router.ChainRouter{} - router.Initialize(logging.NoLog{}, &tm) + router.Initialize(logging.NoLog{}, &tm, time.Hour) sender := Sender{} sender.Initialize(snow.DefaultContextTest(), &ExternalSenderTest{}, &router, &tm) diff --git a/snow/networking/sender/test_external_sender.go b/snow/networking/sender/test_external_sender.go index aabe8cc..2bbfecf 100644 --- a/snow/networking/sender/test_external_sender.go +++ b/snow/networking/sender/test_external_sender.go @@ -17,7 +17,8 @@ type ExternalSenderTest struct { CantGetAcceptedFrontier, CantAcceptedFrontier, CantGetAccepted, CantAccepted, CantGet, CantPut, - CantPullQuery, CantPushQuery, CantChits bool + CantPullQuery, CantPushQuery, CantChits, + CantGossip bool GetAcceptedFrontierF func(validatorIDs ids.ShortSet, chainID ids.ID, requestID uint32) AcceptedFrontierF func(validatorID ids.ShortID, chainID ids.ID, requestID uint32, containerIDs ids.Set) @@ -28,6 +29,7 @@ type ExternalSenderTest struct { PushQueryF func(validatorIDs ids.ShortSet, chainID ids.ID, requestID uint32, containerID ids.ID, container []byte) PullQueryF func(validatorIDs ids.ShortSet, chainID ids.ID, requestID uint32, containerID ids.ID) ChitsF func(validatorID ids.ShortID, chainID ids.ID, requestID uint32, votes ids.Set) + GossipF func(chainID ids.ID, containerID ids.ID, container []byte) } // Default set the default callable value to [cant] @@ -41,6 +43,7 @@ func (s *ExternalSenderTest) Default(cant bool) { s.CantPullQuery = cant s.CantPushQuery = cant s.CantChits = cant + s.CantGossip = cant } // GetAcceptedFrontier calls GetAcceptedFrontierF if it was initialized. If it @@ -159,3 +162,16 @@ func (s *ExternalSenderTest) Chits(vdr ids.ShortID, chainID ids.ID, requestID ui s.B.Fatalf("Unexpectedly called Chits") } } + +// Gossip calls GossipF if it was initialized. If it wasn't initialized and this +// function shouldn't be called and testing was initialized, then testing will +// fail. +func (s *ExternalSenderTest) Gossip(chainID ids.ID, containerID ids.ID, container []byte) { + if s.GossipF != nil { + s.GossipF(chainID, containerID, container) + } else if s.CantGossip && s.T != nil { + s.T.Fatalf("Unexpectedly called Gossip") + } else if s.CantGossip && s.B != nil { + s.B.Fatalf("Unexpectedly called Gossip") + } +} From 13fbe14d44ca71917be4d141860fb920f6018c39 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 4 May 2020 02:44:56 -0400 Subject: [PATCH 19/33] Started gossiping thread --- snow/engine/avalanche/transitive.go | 2 +- snow/engine/snowman/transitive.go | 2 +- snow/networking/router/subnet_router.go | 3 +++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/snow/engine/avalanche/transitive.go b/snow/engine/avalanche/transitive.go index 4b33007..56f9a21 100644 --- a/snow/engine/avalanche/transitive.go +++ b/snow/engine/avalanche/transitive.go @@ -78,7 +78,7 @@ func (t *Transitive) Gossip() { return } - t.Config.Context.Log.Info("Gossiping %s as accepted to the network", vtxID) + t.Config.Context.Log.Debug("Gossiping %s as accepted to the network", vtxID) t.Config.Sender.Gossip(vtxID, vtx.Bytes()) } diff --git a/snow/engine/snowman/transitive.go b/snow/engine/snowman/transitive.go index f4778ac..4bea6c5 100644 --- a/snow/engine/snowman/transitive.go +++ b/snow/engine/snowman/transitive.go @@ -74,7 +74,7 @@ func (t *Transitive) Gossip() { return } - t.Config.Context.Log.Info("Gossiping %s as accepted to the network", blkID) + t.Config.Context.Log.Debug("Gossiping %s as accepted to the network", blkID) t.Config.Sender.Gossip(blkID, blk.Bytes()) } diff --git a/snow/networking/router/subnet_router.go b/snow/networking/router/subnet_router.go index 24b6efb..f1ac4e7 100644 --- a/snow/networking/router/subnet_router.go +++ b/snow/networking/router/subnet_router.go @@ -34,6 +34,8 @@ func (sr *ChainRouter) Initialize(log logging.Logger, timeouts *timeout.Manager, sr.chains = make(map[[32]byte]*handler.Handler) sr.timeouts = timeouts sr.gossiper = timer.NewRepeater(sr.Gossip, gossipFrequency) + + go log.RecoverAndPanic(sr.gossiper.Dispatch) } // AddChain registers the specified chain so that incoming @@ -259,6 +261,7 @@ func (sr *ChainRouter) shutdown() { for _, chain := range sr.chains { chain.Shutdown() } + sr.gossiper.Stop() } // Gossip accepted containers From 0ea445a2d18a9de05d4d43ba375e4c6cd1fe0aa6 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 4 May 2020 02:57:02 -0400 Subject: [PATCH 20/33] Added gossip tests --- snow/engine/avalanche/transitive_test.go | 51 ++++++++++++++++++++++++ snow/engine/snowman/transitive_test.go | 33 +++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/snow/engine/avalanche/transitive_test.go b/snow/engine/avalanche/transitive_test.go index 604afaf..4c95c89 100644 --- a/snow/engine/avalanche/transitive_test.go +++ b/snow/engine/avalanche/transitive_test.go @@ -2536,3 +2536,54 @@ func TestEnginePartiallyValidVertex(t *testing.T) { te.insert(vtx) } + +func TestEngineGossip(t *testing.T) { + config := DefaultConfig() + + sender := &common.SenderTest{} + sender.T = t + config.Sender = sender + + sender.Default(true) + + st := &stateTest{t: t} + config.State = st + + gVtx := &Vtx{ + id: GenerateID(), + status: choices.Accepted, + } + + te := &Transitive{} + te.Initialize(config) + te.finishBootstrapping() + + st.edge = func() []ids.ID { return []ids.ID{gVtx.ID()} } + st.getVertex = func(vtxID ids.ID) (avalanche.Vertex, error) { + switch { + case vtxID.Equals(gVtx.ID()): + return gVtx, nil + } + t.Fatal(errUnknownVertex) + return nil, errUnknownVertex + } + + called := new(bool) + sender.GossipF = func(vtxID ids.ID, vtxBytes []byte) { + *called = true + switch { + case !vtxID.Equals(gVtx.ID()): + t.Fatal(errUnknownVertex) + } + switch { + case !bytes.Equal(vtxBytes, gVtx.Bytes()): + t.Fatal(errUnknownVertex) + } + } + + te.Gossip() + + if !*called { + t.Fatalf("Should have gossiped the vertex") + } +} diff --git a/snow/engine/snowman/transitive_test.go b/snow/engine/snowman/transitive_test.go index a1875d8..047d689 100644 --- a/snow/engine/snowman/transitive_test.go +++ b/snow/engine/snowman/transitive_test.go @@ -1187,3 +1187,36 @@ func TestEngineUndeclaredDependencyDeadlock(t *testing.T) { t.Fatalf("Should have bubbled invalid votes to the valid parent") } } + +func TestEngineGossip(t *testing.T) { + _, _, sender, vm, te, gBlk := setup(t) + + vm.LastAcceptedF = func() ids.ID { return gBlk.ID() } + vm.GetBlockF = func(blkID ids.ID) (snowman.Block, error) { + switch { + case blkID.Equals(gBlk.ID()): + return gBlk, nil + } + t.Fatal(errUnknownBlock) + return nil, errUnknownBlock + } + + called := new(bool) + sender.GossipF = func(blkID ids.ID, blkBytes []byte) { + *called = true + switch { + case !blkID.Equals(gBlk.ID()): + t.Fatal(errUnknownBlock) + } + switch { + case !bytes.Equal(blkBytes, gBlk.Bytes()): + t.Fatal(errUnknownBytes) + } + } + + te.Gossip() + + if !*called { + t.Fatalf("Should have gossiped the block") + } +} From cbb20b2faaae224791a71d6bfb526fc38b0c8567 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Mon, 4 May 2020 13:59:10 -0400 Subject: [PATCH 21/33] return 'AVA' rather than its asset id --- vms/avm/service.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/vms/avm/service.go b/vms/avm/service.go index 8b90278..20b4578 100644 --- a/vms/avm/service.go +++ b/vms/avm/service.go @@ -280,9 +280,18 @@ func (service *Service) GetAllBalances(r *http.Request, args *GetAllBalancesArgs } } + avaAssetID, err := service.vm.Lookup("AVA") + if err != nil { + return errors.New("couldn't get asset ID of AVA") + } + reply.Balances = make(map[string]json.Uint64, assetIDs.Len()) for _, assetID := range assetIDs.List() { - reply.Balances[assetID.String()] = json.Uint64(balances[assetID.Key()]) + if assetID.Equals(avaAssetID) { + reply.Balances["AVA"] = json.Uint64(balances[assetID.Key()]) + } else { + reply.Balances[assetID.String()] = json.Uint64(balances[assetID.Key()]) + } } return nil From dfbb17aaedcd65f4eae28fc5a6b6b464e59f13b1 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 4 May 2020 13:59:25 -0400 Subject: [PATCH 22/33] Added gossip params to tests --- vms/platformvm/vm_test.go | 2 +- vms/spchainvm/consensus_benchmark_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/vms/platformvm/vm_test.go b/vms/platformvm/vm_test.go index 28be3fc..c32d8bd 100644 --- a/vms/platformvm/vm_test.go +++ b/vms/platformvm/vm_test.go @@ -1510,7 +1510,7 @@ func TestBootstrapPartiallyAccepted(t *testing.T) { go timeoutManager.Dispatch() router := &router.ChainRouter{} - router.Initialize(logging.NoLog{}, &timeoutManager) + router.Initialize(logging.NoLog{}, &timeoutManager, time.Hour) externalSender := &sender.ExternalSenderTest{T: t} externalSender.Default(true) diff --git a/vms/spchainvm/consensus_benchmark_test.go b/vms/spchainvm/consensus_benchmark_test.go index aa80e6d..1a33502 100644 --- a/vms/spchainvm/consensus_benchmark_test.go +++ b/vms/spchainvm/consensus_benchmark_test.go @@ -62,7 +62,7 @@ func ConsensusLeader(numBlocks, numTxsPerBlock int, b *testing.B) { go timeoutManager.Dispatch() router := &router.ChainRouter{} - router.Initialize(logging.NoLog{}, &timeoutManager) + router.Initialize(logging.NoLog{}, &timeoutManager, time.Hour) // Initialize the VM vm := &VM{} @@ -189,7 +189,7 @@ func ConsensusFollower(numBlocks, numTxsPerBlock int, b *testing.B) { go timeoutManager.Dispatch() router := &router.ChainRouter{} - router.Initialize(logging.NoLog{}, &timeoutManager) + router.Initialize(logging.NoLog{}, &timeoutManager, time.Hour) wg := sync.WaitGroup{} wg.Add(numBlocks) From d727166f4fb4835384cd522d0d6dcc1a0741fecb Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Mon, 4 May 2020 14:44:35 -0400 Subject: [PATCH 23/33] change response format for getAllBalances --- vms/avm/service.go | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/vms/avm/service.go b/vms/avm/service.go index 20b4578..b9f3aaa 100644 --- a/vms/avm/service.go +++ b/vms/avm/service.go @@ -232,6 +232,12 @@ func (service *Service) GetBalance(r *http.Request, args *GetBalanceArgs, reply return nil } +// Balance ... +type Balance struct { + AssetID string `json:"assetID"` + Balance json.Uint64 `json:"balance"` +} + // GetAllBalancesArgs are arguments for calling into GetAllBalances type GetAllBalancesArgs struct { Address string `json:"address"` @@ -239,7 +245,7 @@ type GetAllBalancesArgs struct { // GetAllBalancesReply is the response from a call to GetAllBalances type GetAllBalancesReply struct { - Balances map[string]json.Uint64 `json:"balances"` + Balances []Balance `json:"balances"` } // GetAllBalances returns a map where: @@ -285,13 +291,15 @@ func (service *Service) GetAllBalances(r *http.Request, args *GetAllBalancesArgs return errors.New("couldn't get asset ID of AVA") } - reply.Balances = make(map[string]json.Uint64, assetIDs.Len()) - for _, assetID := range assetIDs.List() { + reply.Balances = make([]Balance, assetIDs.Len()) + for i, assetID := range assetIDs.List() { + var b Balance if assetID.Equals(avaAssetID) { - reply.Balances["AVA"] = json.Uint64(balances[assetID.Key()]) + b = Balance{AssetID: "AVA", Balance: json.Uint64(balances[assetID.Key()])} } else { - reply.Balances[assetID.String()] = json.Uint64(balances[assetID.Key()]) + b = Balance{AssetID: assetID.String(), Balance: json.Uint64(balances[assetID.Key()])} } + reply.Balances[i] = b } return nil From a721c188a5c868f98a572135e624c5110f52bbae Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Mon, 4 May 2020 17:06:07 -0400 Subject: [PATCH 24/33] use asset alias in response --- vms/avm/service.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/vms/avm/service.go b/vms/avm/service.go index b9f3aaa..10b9b3b 100644 --- a/vms/avm/service.go +++ b/vms/avm/service.go @@ -234,7 +234,7 @@ func (service *Service) GetBalance(r *http.Request, args *GetBalanceArgs, reply // Balance ... type Balance struct { - AssetID string `json:"assetID"` + AssetID string `json:"asset"` Balance json.Uint64 `json:"balance"` } @@ -286,20 +286,19 @@ func (service *Service) GetAllBalances(r *http.Request, args *GetAllBalancesArgs } } - avaAssetID, err := service.vm.Lookup("AVA") - if err != nil { - return errors.New("couldn't get asset ID of AVA") - } - reply.Balances = make([]Balance, assetIDs.Len()) for i, assetID := range assetIDs.List() { - var b Balance - if assetID.Equals(avaAssetID) { - b = Balance{AssetID: "AVA", Balance: json.Uint64(balances[assetID.Key()])} + if alias, err := service.vm.PrimaryAlias(assetID); err == nil { + reply.Balances[i] = Balance{ + AssetID: alias, + Balance: json.Uint64(balances[assetID.Key()]), + } } else { - b = Balance{AssetID: assetID.String(), Balance: json.Uint64(balances[assetID.Key()])} + reply.Balances[i] = Balance{ + AssetID: assetID.String(), + Balance: json.Uint64(balances[assetID.Key()]), + } } - reply.Balances[i] = b } return nil From c56870045ecdf22c2705c702090c3e735757103f Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 4 May 2020 18:12:34 -0400 Subject: [PATCH 25/33] fixed logging message --- networking/voting_handlers.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/networking/voting_handlers.go b/networking/voting_handlers.go index f6a43e3..2e475a6 100644 --- a/networking/voting_handlers.go +++ b/networking/voting_handlers.go @@ -436,8 +436,8 @@ func (s *Voting) gossip(chainID, containerID ids.ID, container []byte) error { return fmt.Errorf("Attempted to pack too large of a Put message.\nContainer length: %d: %w", len(container), err) } - s.log.Verbo("Sending a Put message to non-validators."+ - "\nNumber of Non-Validators: %d"+ + s.log.Verbo("Sending a Put message to peers."+ + "\nNumber of Peers: %d"+ "\nChain: %s"+ "\nContainer ID: %s"+ "\nContainer:\n%s", From 282e89653c3c7bd40445c409c27024697a42d89a Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 4 May 2020 23:24:17 -0400 Subject: [PATCH 26/33] Make sure that Keys / Values are memory safe from levelDB --- database/leveldb/db.go | 19 ++++++++++++- database/test_database.go | 58 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/database/leveldb/db.go b/database/leveldb/db.go index a763829..33a51f1 100644 --- a/database/leveldb/db.go +++ b/database/leveldb/db.go @@ -208,7 +208,14 @@ func (r *replayer) Delete(key []byte) { type iter struct{ iterator.Iterator } -func (i *iter) Error() error { return updateError(i.Iterator.Error()) } +// Error implements the Iterator interface +func (it *iter) Error() error { return updateError(it.Iterator.Error()) } + +// Key implements the Iterator interface +func (it *iter) Key() []byte { return copyBytes(it.Iterator.Key()) } + +// Value implements the Iterator interface +func (it *iter) Value() []byte { return copyBytes(it.Iterator.Value()) } func updateError(err error) error { switch err { @@ -220,3 +227,13 @@ func updateError(err error) error { return err } } + +func copyBytes(bytes []byte) []byte { + if bytes == nil { + return nil + } + + copiedBytes := make([]byte, len(bytes)) + copy(copiedBytes, bytes) + return copiedBytes +} diff --git a/database/test_database.go b/database/test_database.go index 2e33b25..4a05651 100644 --- a/database/test_database.go +++ b/database/test_database.go @@ -24,6 +24,7 @@ var ( TestIteratorStart, TestIteratorPrefix, TestIteratorStartPrefix, + TestIteratorMemorySafety, TestIteratorClosed, TestStatNoPanic, TestCompactNoPanic, @@ -622,6 +623,63 @@ func TestIteratorStartPrefix(t *testing.T, db Database) { } } +// TestIteratorMemorySafety ... +func TestIteratorMemorySafety(t *testing.T, db Database) { + key1 := []byte("hello1") + value1 := []byte("world1") + + key2 := []byte("z") + value2 := []byte("world2") + + key3 := []byte("hello3") + value3 := []byte("world3") + + if err := db.Put(key1, value1); err != nil { + t.Fatalf("Unexpected error on batch.Put: %s", err) + } else if err := db.Put(key2, value2); err != nil { + t.Fatalf("Unexpected error on batch.Put: %s", err) + } else if err := db.Put(key3, value3); err != nil { + t.Fatalf("Unexpected error on batch.Put: %s", err) + } + + iterator := db.NewIterator() + if iterator == nil { + t.Fatalf("db.NewIteratorWithStartAndPrefix returned nil") + } + defer iterator.Release() + + keys := [][]byte{} + values := [][]byte{} + for iterator.Next() { + keys = append(keys, iterator.Key()) + values = append(values, iterator.Value()) + } + + expectedKeys := [][]byte{ + key1, + key3, + key2, + } + expectedValues := [][]byte{ + value1, + value3, + value2, + } + + for i, key := range keys { + value := values[i] + expectedKey := expectedKeys[i] + expectedValue := expectedValues[i] + + if !bytes.Equal(key, expectedKey) { + t.Fatalf("Wrong key") + } + if !bytes.Equal(value, expectedValue) { + t.Fatalf("Wrong key") + } + } +} + // TestIteratorClosed ... func TestIteratorClosed(t *testing.T, db Database) { key1 := []byte("hello1") From f837398403a508175b171a27a9c15ff884bc1b89 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 4 May 2020 23:28:36 -0400 Subject: [PATCH 27/33] updated DB spec --- database/iterator.go | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/database/iterator.go b/database/iterator.go index 283ed62..ed23ab5 100644 --- a/database/iterator.go +++ b/database/iterator.go @@ -17,46 +17,42 @@ package database // iterator until exhaustion. An iterator is not safe for concurrent use, but it // is safe to use multiple iterators concurrently. type Iterator interface { - // Next moves the iterator to the next key/value pair. It returns whether the - // iterator is exhausted. + // Next moves the iterator to the next key/value pair. It returns whether + // the iterator is exhausted. Next() bool // Error returns any accumulated error. Exhausting all the key/value pairs // is not considered to be an error. Error() error - // Key returns the key of the current key/value pair, or nil if done. The caller - // should not modify the contents of the returned slice, and its contents may - // change on the next call to Next. + // Key returns the key of the current key/value pair, or nil if done. Key() []byte - // Value returns the value of the current key/value pair, or nil if done. The - // caller should not modify the contents of the returned slice, and its contents - // may change on the next call to Next. + // Value returns the value of the current key/value pair, or nil if done. Value() []byte - // Release releases associated resources. Release should always succeed and can - // be called multiple times without causing error. + // Release releases associated resources. Release should always succeed and + // can be called multiple times without causing error. Release() } // Iteratee wraps the NewIterator methods of a backing data store. type Iteratee interface { - // NewIterator creates a binary-alphabetical iterator over the entire keyspace - // contained within the key-value database. + // NewIterator creates a binary-alphabetical iterator over the entire + // keyspace contained within the key-value database. NewIterator() Iterator - // NewIteratorWithStart creates a binary-alphabetical iterator over a subset of - // database content starting at a particular initial key (or after, if it does - // not exist). + // NewIteratorWithStart creates a binary-alphabetical iterator over a subset + // of database content starting at a particular initial key (or after, if it + // does not exist). NewIteratorWithStart(start []byte) Iterator - // NewIteratorWithPrefix creates a binary-alphabetical iterator over a subset - // of database content with a particular key prefix. + // NewIteratorWithPrefix creates a binary-alphabetical iterator over a + // subset of database content with a particular key prefix. NewIteratorWithPrefix(prefix []byte) Iterator - // NewIteratorWithStartAndPrefix creates a binary-alphabetical iterator over a - // subset of database content with a particular key prefix starting at a + // NewIteratorWithStartAndPrefix creates a binary-alphabetical iterator over + // a subset of database content with a particular key prefix starting at a // specified key. NewIteratorWithStartAndPrefix(start, prefix []byte) Iterator } From 8f91f7294bc0f8516e756e3802c241cbee31a1b0 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Mon, 4 May 2020 23:32:54 -0400 Subject: [PATCH 28/33] Added gossip frequency docs --- snow/networking/router/subnet_router.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/snow/networking/router/subnet_router.go b/snow/networking/router/subnet_router.go index f1ac4e7..6482966 100644 --- a/snow/networking/router/subnet_router.go +++ b/snow/networking/router/subnet_router.go @@ -26,9 +26,14 @@ type ChainRouter struct { gossiper *timer.Repeater } -// Initialize the router -// When this router receives an incoming message, it cancels the timeout in [timeouts] -// associated with the request that caused the incoming message, if applicable +// Initialize the router. +// +// When this router receives an incoming message, it cancels the timeout in +// [timeouts] associated with the request that caused the incoming message, if +// applicable. +// +// This router also fires a gossip event every [gossipFrequency] to the engine, +// notifying the engine it should gossip it's accepted set. func (sr *ChainRouter) Initialize(log logging.Logger, timeouts *timeout.Manager, gossipFrequency time.Duration) { sr.log = log sr.chains = make(map[[32]byte]*handler.Handler) From c3b3b14872a306f4927f1feccb9ee7ff823c4134 Mon Sep 17 00:00:00 2001 From: bb-2 <43212522+bb-2@users.noreply.github.com> Date: Tue, 5 May 2020 08:10:22 -0400 Subject: [PATCH 29/33] Moving the cache put statements in SetStatus and SetIDs to be after the error checking --- vms/spdagvm/state.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/vms/spdagvm/state.go b/vms/spdagvm/state.go index 74c12c9..0f02c03 100644 --- a/vms/spdagvm/state.go +++ b/vms/spdagvm/state.go @@ -130,8 +130,6 @@ func (s *state) SetStatus(id ids.ID, status choices.Status) error { return s.vm.db.Delete(id.Bytes()) } - s.c.Put(id, status) - p := wrappers.Packer{Bytes: make([]byte, 4)} p.PackInt(uint32(status)) @@ -143,6 +141,8 @@ func (s *state) SetStatus(id ids.ID, status choices.Status) error { if p.Errored() { return p.Err } + + s.c.Put(id, status) return s.vm.db.Put(id.Bytes(), p.Bytes) } @@ -186,8 +186,6 @@ func (s *state) SetIDs(id ids.ID, idSlice []ids.ID) error { return s.vm.db.Delete(id.Bytes()) } - s.c.Put(id, idSlice) - size := wrappers.IntLen + hashing.HashLen*len(idSlice) p := wrappers.Packer{Bytes: make([]byte, size)} @@ -203,5 +201,7 @@ func (s *state) SetIDs(id ids.ID, idSlice []ids.ID) error { if p.Errored() { return p.Err } + + s.c.Put(id, idSlice) return s.vm.db.Put(id.Bytes(), p.Bytes) } From 695290fb597ebfb8d51d5d3e119622515e108c85 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Tue, 5 May 2020 14:04:03 -0400 Subject: [PATCH 30/33] moved copyBytes to the utils package --- database/encdb/db.go | 11 +++-------- database/leveldb/db.go | 15 +++------------ database/memdb/db.go | 15 +++++---------- database/prefixdb/db.go | 11 +++-------- database/rpcdb/db_client.go | 11 +++-------- database/versiondb/db.go | 13 ++++--------- utils/bytes.go | 16 ++++++++++++++++ utils/bytes_test.go | 24 ++++++++++++++++++++++++ 8 files changed, 61 insertions(+), 55 deletions(-) create mode 100644 utils/bytes.go create mode 100644 utils/bytes_test.go diff --git a/database/encdb/db.go b/database/encdb/db.go index e93032e..eb06549 100644 --- a/database/encdb/db.go +++ b/database/encdb/db.go @@ -12,6 +12,7 @@ import ( "github.com/ava-labs/gecko/database" "github.com/ava-labs/gecko/database/nodb" + "github.com/ava-labs/gecko/utils" "github.com/ava-labs/gecko/utils/hashing" "github.com/ava-labs/gecko/vms/components/codec" ) @@ -174,7 +175,7 @@ type batch struct { } func (b *batch) Put(key, value []byte) error { - b.writes = append(b.writes, keyValue{copyBytes(key), copyBytes(value), false}) + b.writes = append(b.writes, keyValue{utils.CopyBytes(key), utils.CopyBytes(value), false}) encValue, err := b.db.encrypt(value) if err != nil { return err @@ -183,7 +184,7 @@ func (b *batch) Put(key, value []byte) error { } func (b *batch) Delete(key []byte) error { - b.writes = append(b.writes, keyValue{copyBytes(key), nil, true}) + b.writes = append(b.writes, keyValue{utils.CopyBytes(key), nil, true}) return b.Batch.Delete(key) } @@ -251,12 +252,6 @@ func (it *iterator) Error() error { func (it *iterator) Value() []byte { return it.val } -func copyBytes(bytes []byte) []byte { - copiedBytes := make([]byte, len(bytes)) - copy(copiedBytes, bytes) - return copiedBytes -} - type encryptedValue struct { Ciphertext []byte `serialize:"true"` Nonce []byte `serialize:"true"` diff --git a/database/leveldb/db.go b/database/leveldb/db.go index 33a51f1..edcb4be 100644 --- a/database/leveldb/db.go +++ b/database/leveldb/db.go @@ -7,6 +7,7 @@ import ( "bytes" "github.com/ava-labs/gecko/database" + "github.com/ava-labs/gecko/utils" "github.com/syndtr/goleveldb/leveldb" "github.com/syndtr/goleveldb/leveldb/errors" "github.com/syndtr/goleveldb/leveldb/filter" @@ -212,10 +213,10 @@ type iter struct{ iterator.Iterator } func (it *iter) Error() error { return updateError(it.Iterator.Error()) } // Key implements the Iterator interface -func (it *iter) Key() []byte { return copyBytes(it.Iterator.Key()) } +func (it *iter) Key() []byte { return utils.CopyBytes(it.Iterator.Key()) } // Value implements the Iterator interface -func (it *iter) Value() []byte { return copyBytes(it.Iterator.Value()) } +func (it *iter) Value() []byte { return utils.CopyBytes(it.Iterator.Value()) } func updateError(err error) error { switch err { @@ -227,13 +228,3 @@ func updateError(err error) error { return err } } - -func copyBytes(bytes []byte) []byte { - if bytes == nil { - return nil - } - - copiedBytes := make([]byte, len(bytes)) - copy(copiedBytes, bytes) - return copiedBytes -} diff --git a/database/memdb/db.go b/database/memdb/db.go index 24b5104..de0cae3 100644 --- a/database/memdb/db.go +++ b/database/memdb/db.go @@ -10,6 +10,7 @@ import ( "github.com/ava-labs/gecko/database" "github.com/ava-labs/gecko/database/nodb" + "github.com/ava-labs/gecko/utils" ) // DefaultSize is the default initial size of the memory database @@ -62,7 +63,7 @@ func (db *Database) Get(key []byte) ([]byte, error) { return nil, database.ErrClosed } if entry, ok := db.db[string(key)]; ok { - return copyBytes(entry), nil + return utils.CopyBytes(entry), nil } return nil, database.ErrNotFound } @@ -75,7 +76,7 @@ func (db *Database) Put(key []byte, value []byte) error { if db.db == nil { return database.ErrClosed } - db.db[string(key)] = copyBytes(value) + db.db[string(key)] = utils.CopyBytes(value) return nil } @@ -154,13 +155,13 @@ type batch struct { } func (b *batch) Put(key, value []byte) error { - b.writes = append(b.writes, keyValue{copyBytes(key), copyBytes(value), false}) + b.writes = append(b.writes, keyValue{utils.CopyBytes(key), utils.CopyBytes(value), false}) b.size += len(value) return nil } func (b *batch) Delete(key []byte) error { - b.writes = append(b.writes, keyValue{copyBytes(key), nil, true}) + b.writes = append(b.writes, keyValue{utils.CopyBytes(key), nil, true}) b.size++ return nil } @@ -253,9 +254,3 @@ func (it *iterator) Value() []byte { // Release implements the Iterator interface func (it *iterator) Release() { it.keys = nil; it.values = nil } - -func copyBytes(bytes []byte) []byte { - copiedBytes := make([]byte, len(bytes)) - copy(copiedBytes, bytes) - return copiedBytes -} diff --git a/database/prefixdb/db.go b/database/prefixdb/db.go index 5e88318..34bc50d 100644 --- a/database/prefixdb/db.go +++ b/database/prefixdb/db.go @@ -8,6 +8,7 @@ import ( "github.com/ava-labs/gecko/database" "github.com/ava-labs/gecko/database/nodb" + "github.com/ava-labs/gecko/utils" "github.com/ava-labs/gecko/utils/hashing" ) @@ -174,13 +175,13 @@ type batch struct { // Put implements the Batch interface func (b *batch) Put(key, value []byte) error { - b.writes = append(b.writes, keyValue{copyBytes(key), copyBytes(value), false}) + b.writes = append(b.writes, keyValue{utils.CopyBytes(key), utils.CopyBytes(value), false}) return b.Batch.Put(b.db.prefix(key), value) } // Delete implements the Batch interface func (b *batch) Delete(key []byte) error { - b.writes = append(b.writes, keyValue{copyBytes(key), nil, true}) + b.writes = append(b.writes, keyValue{utils.CopyBytes(key), nil, true}) return b.Batch.Delete(b.db.prefix(key)) } @@ -229,9 +230,3 @@ func (it *iterator) Key() []byte { } return key } - -func copyBytes(bytes []byte) []byte { - copiedBytes := make([]byte, len(bytes)) - copy(copiedBytes, bytes) - return copiedBytes -} diff --git a/database/rpcdb/db_client.go b/database/rpcdb/db_client.go index 136121b..67af7ef 100644 --- a/database/rpcdb/db_client.go +++ b/database/rpcdb/db_client.go @@ -11,6 +11,7 @@ import ( "github.com/ava-labs/gecko/database" "github.com/ava-labs/gecko/database/nodb" "github.com/ava-labs/gecko/database/rpcdb/rpcdbproto" + "github.com/ava-labs/gecko/utils" ) var ( @@ -137,13 +138,13 @@ type batch struct { } func (b *batch) Put(key, value []byte) error { - b.writes = append(b.writes, keyValue{copyBytes(key), copyBytes(value), false}) + b.writes = append(b.writes, keyValue{utils.CopyBytes(key), utils.CopyBytes(value), false}) b.size += len(value) return nil } func (b *batch) Delete(key []byte) error { - b.writes = append(b.writes, keyValue{copyBytes(key), nil, true}) + b.writes = append(b.writes, keyValue{utils.CopyBytes(key), nil, true}) b.size++ return nil } @@ -246,12 +247,6 @@ func (it *iterator) Release() { }) } -func copyBytes(bytes []byte) []byte { - copiedBytes := make([]byte, len(bytes)) - copy(copiedBytes, bytes) - return copiedBytes -} - func updateError(err error) error { if err == nil { return nil diff --git a/database/versiondb/db.go b/database/versiondb/db.go index 6f42131..fb692bf 100644 --- a/database/versiondb/db.go +++ b/database/versiondb/db.go @@ -11,6 +11,7 @@ import ( "github.com/ava-labs/gecko/database" "github.com/ava-labs/gecko/database/memdb" "github.com/ava-labs/gecko/database/nodb" + "github.com/ava-labs/gecko/utils" ) // Database implements the Database interface by living on top of another @@ -61,7 +62,7 @@ func (db *Database) Get(key []byte) ([]byte, error) { if val.delete { return nil, database.ErrNotFound } - return copyBytes(val.value), nil + return utils.CopyBytes(val.value), nil } return db.db.Get(key) } @@ -262,14 +263,14 @@ type batch struct { // Put implements the Database interface func (b *batch) Put(key, value []byte) error { - b.writes = append(b.writes, keyValue{copyBytes(key), copyBytes(value), false}) + b.writes = append(b.writes, keyValue{utils.CopyBytes(key), utils.CopyBytes(value), false}) b.size += len(value) return nil } // Delete implements the Database interface func (b *batch) Delete(key []byte) error { - b.writes = append(b.writes, keyValue{copyBytes(key), nil, true}) + b.writes = append(b.writes, keyValue{utils.CopyBytes(key), nil, true}) b.size++ return nil } @@ -414,9 +415,3 @@ func (it *iterator) Release() { it.values = nil it.Iterator.Release() } - -func copyBytes(bytes []byte) []byte { - copiedBytes := make([]byte, len(bytes)) - copy(copiedBytes, bytes) - return copiedBytes -} diff --git a/utils/bytes.go b/utils/bytes.go new file mode 100644 index 0000000..789b881 --- /dev/null +++ b/utils/bytes.go @@ -0,0 +1,16 @@ +// (c) 2019-2020, Ava Labs, Inc. All rights reserved. +// See the file LICENSE for licensing terms. + +package utils + +// CopyBytes returns a copy of the provided byte slice. If nil is provided, nil +// will be returned. +func CopyBytes(b []byte) []byte { + if b == nil { + return nil + } + + cb := make([]byte, len(b)) + copy(cb, b) + return cb +} diff --git a/utils/bytes_test.go b/utils/bytes_test.go new file mode 100644 index 0000000..10f40a7 --- /dev/null +++ b/utils/bytes_test.go @@ -0,0 +1,24 @@ +// (c) 2019-2020, Ava Labs, Inc. All rights reserved. +// See the file LICENSE for licensing terms. + +package utils + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCopyBytesNil(t *testing.T) { + result := CopyBytes(nil) + assert.Nil(t, result, "CopyBytes(nil) should have returned nil") +} + +func TestCopyBytes(t *testing.T) { + input := []byte{1} + result := CopyBytes(input) + assert.Equal(t, input, result, "CopyBytes should have returned equal bytes") + + input[0] = 0 + assert.NotEqual(t, input, result, "CopyBytes should have returned independent bytes") +} From 9b583ccc6e87c772879d10b33d1ab449cce69d36 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Tue, 5 May 2020 14:06:30 -0400 Subject: [PATCH 31/33] fixed iterator error message to report the correct function --- database/test_database.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/database/test_database.go b/database/test_database.go index 4a05651..df1e015 100644 --- a/database/test_database.go +++ b/database/test_database.go @@ -644,7 +644,7 @@ func TestIteratorMemorySafety(t *testing.T, db Database) { iterator := db.NewIterator() if iterator == nil { - t.Fatalf("db.NewIteratorWithStartAndPrefix returned nil") + t.Fatalf("db.NewIterator returned nil") } defer iterator.Release() From d35834c4e59934bec8b44744f595bbd80a4b6c96 Mon Sep 17 00:00:00 2001 From: bb-2 <43212522+bb-2@users.noreply.github.com> Date: Wed, 6 May 2020 08:49:44 -0400 Subject: [PATCH 32/33] move avm cache puts after error checking and catch err on status unmarshalling --- vms/components/ava/state.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/vms/components/ava/state.go b/vms/components/ava/state.go index a9c5424..627cc56 100644 --- a/vms/components/ava/state.go +++ b/vms/components/ava/state.go @@ -90,7 +90,9 @@ func (s *State) Status(id ids.ID) (choices.Status, error) { } var status choices.Status - s.Codec.Unmarshal(bytes, &status) + if err := s.Codec.Unmarshal(bytes, &status); err != nil { + return choices.Unknown, err + } s.Cache.Put(id, status) return status, nil @@ -103,12 +105,12 @@ func (s *State) SetStatus(id ids.ID, status choices.Status) error { return s.DB.Delete(id.Bytes()) } - s.Cache.Put(id, status) - bytes, err := s.Codec.Marshal(status) if err != nil { return err } + + s.Cache.Put(id, status) return s.DB.Put(id.Bytes(), bytes) } @@ -142,12 +144,11 @@ func (s *State) SetIDs(id ids.ID, idSlice []ids.ID) error { return s.DB.Delete(id.Bytes()) } - s.Cache.Put(id, idSlice) - bytes, err := s.Codec.Marshal(idSlice) if err != nil { return err } + s.Cache.Put(id, idSlice) return s.DB.Put(id.Bytes(), bytes) } From ac27d66c02c6dc5199b71affc0a58fef5bf51af4 Mon Sep 17 00:00:00 2001 From: StephenButtolph Date: Sun, 10 May 2020 15:26:41 -0400 Subject: [PATCH 33/33] 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) }