From d1bec63e10470b8f0f9e4368da244ba137f538d8 Mon Sep 17 00:00:00 2001 From: "bors[bot]" <26634292+bors[bot]@users.noreply.github.com> Date: Tue, 17 Jan 2023 20:23:46 +0000 Subject: [PATCH 01/16] Merge #3805 3805: [Access] Add BlockStatus to GetBlock REST API r=lolpuddle a=lolpuddle Closes https://github.com/onflow/flow-go/issues/3622 Co-authored-by: lolpuddle --- engine/access/rest/blocks.go | 34 +++++++++++------------- engine/access/rest/blocks_test.go | 33 +++++++++++++---------- engine/access/rest/models/block.go | 2 ++ engine/access/rest/models/model_block.go | 1 + 4 files changed, 38 insertions(+), 32 deletions(-) diff --git a/engine/access/rest/blocks.go b/engine/access/rest/blocks.go index 11c9ca906b0..e729f67a9bd 100644 --- a/engine/access/rest/blocks.go +++ b/engine/access/rest/blocks.go @@ -5,15 +5,13 @@ import ( "fmt" "net/http" - "github.com/onflow/flow-go/engine/access/rest/models" - "github.com/onflow/flow-go/engine/access/rest/request" - "google.golang.org/grpc/codes" "google.golang.org/grpc/status" - "github.com/onflow/flow-go/model/flow" - "github.com/onflow/flow-go/access" + "github.com/onflow/flow-go/engine/access/rest/models" + "github.com/onflow/flow-go/engine/access/rest/request" + "github.com/onflow/flow-go/model/flow" ) // GetBlocksByIDs gets blocks by provided ID or list of IDs. @@ -99,7 +97,7 @@ func GetBlockPayloadByID(r *request.Request, backend access.API, _ models.LinkGe } blkProvider := NewBlockProvider(backend, forID(&req.ID)) - blk, statusErr := blkProvider.getBlock(r.Context()) + blk, _, statusErr := blkProvider.getBlock(r.Context()) if statusErr != nil { return nil, statusErr } @@ -116,7 +114,7 @@ func GetBlockPayloadByID(r *request.Request, backend access.API, _ models.LinkGe func getBlock(option blockProviderOption, req *request.Request, backend access.API, link models.LinkGenerator) (*models.Block, error) { // lookup block blkProvider := NewBlockProvider(backend, option) - blk, err := blkProvider.getBlock(req.Context()) + blk, blockStatus, err := blkProvider.getBlock(req.Context()) if err != nil { return nil, err } @@ -129,7 +127,7 @@ func getBlock(option blockProviderOption, req *request.Request, backend access.A // handle case where execution result is not yet available if se, ok := status.FromError(err); ok { if se.Code() == codes.NotFound { - err := block.Build(blk, nil, link, req.ExpandFields) + err := block.Build(blk, nil, link, blockStatus, req.ExpandFields) if err != nil { return nil, err } @@ -139,7 +137,7 @@ func getBlock(option blockProviderOption, req *request.Request, backend access.A return nil, err } - err = block.Build(blk, executionResult, link, req.ExpandFields) + err = block.Build(blk, executionResult, link, blockStatus, req.ExpandFields) if err != nil { return nil, err } @@ -192,31 +190,31 @@ func NewBlockProvider(backend access.API, options ...blockProviderOption) *block return blkProvider } -func (blkProvider *blockProvider) getBlock(ctx context.Context) (*flow.Block, error) { +func (blkProvider *blockProvider) getBlock(ctx context.Context) (*flow.Block, flow.BlockStatus, error) { if blkProvider.id != nil { blk, _, err := blkProvider.backend.GetBlockByID(ctx, *blkProvider.id) if err != nil { // unfortunately backend returns internal error status if not found - return nil, NewNotFoundError( + return nil, flow.BlockStatusUnknown, NewNotFoundError( fmt.Sprintf("error looking up block with ID %s", blkProvider.id.String()), err, ) } - return blk, nil + return blk, flow.BlockStatusUnknown, nil } if blkProvider.latest { - blk, _, err := blkProvider.backend.GetLatestBlock(ctx, blkProvider.sealed) + blk, status, err := blkProvider.backend.GetLatestBlock(ctx, blkProvider.sealed) if err != nil { // cannot be a 'not found' error since final and sealed block should always be found - return nil, NewRestError(http.StatusInternalServerError, "block lookup failed", err) + return nil, flow.BlockStatusUnknown, NewRestError(http.StatusInternalServerError, "block lookup failed", err) } - return blk, nil + return blk, status, nil } - blk, _, err := blkProvider.backend.GetBlockByHeight(ctx, blkProvider.height) + blk, status, err := blkProvider.backend.GetBlockByHeight(ctx, blkProvider.height) if err != nil { // unfortunately backend returns internal error status if not found - return nil, NewNotFoundError( + return nil, flow.BlockStatusUnknown, NewNotFoundError( fmt.Sprintf("error looking up block at height %d", blkProvider.height), err, ) } - return blk, nil + return blk, status, nil } diff --git a/engine/access/rest/blocks_test.go b/engine/access/rest/blocks_test.go index be27512ced0..7f977b06d69 100644 --- a/engine/access/rest/blocks_test.go +++ b/engine/access/rest/blocks_test.go @@ -38,11 +38,13 @@ func TestGetBlocks(t *testing.T) { blkCnt := 10 blockIDs, heights, blocks, executionResults := generateMocks(backend, blkCnt) - singleBlockExpandedResponse := expectedBlockResponsesExpanded(blocks[:1], executionResults[:1], true) - multipleBlockExpandedResponse := expectedBlockResponsesExpanded(blocks, executionResults, true) + singleBlockExpandedResponse := expectedBlockResponsesExpanded(blocks[:1], executionResults[:1], true, flow.BlockStatusUnknown) + singleSealedBlockExpandedResponse := expectedBlockResponsesExpanded(blocks[:1], executionResults[:1], true, flow.BlockStatusSealed) + multipleBlockExpandedResponse := expectedBlockResponsesExpanded(blocks, executionResults, true, flow.BlockStatusUnknown) + multipleSealedBlockExpandedResponse := expectedBlockResponsesExpanded(blocks, executionResults, true, flow.BlockStatusSealed) - singleBlockCondensedResponse := expectedBlockResponsesExpanded(blocks[:1], executionResults[:1], false) - multipleBlockCondensedResponse := expectedBlockResponsesExpanded(blocks, executionResults, false) + singleBlockCondensedResponse := expectedBlockResponsesExpanded(blocks[:1], executionResults[:1], false, flow.BlockStatusUnknown) + multipleBlockCondensedResponse := expectedBlockResponsesExpanded(blocks, executionResults, false, flow.BlockStatusUnknown) invalidID := unittest.IdentifierFixture().String() invalidHeight := fmt.Sprintf("%d", blkCnt+1) @@ -78,19 +80,19 @@ func TestGetBlocks(t *testing.T) { description: "Get single expanded block by height", request: getByHeightsExpandedURL(t, heights[:1]...), expectedStatus: http.StatusOK, - expectedResponse: singleBlockExpandedResponse, + expectedResponse: singleSealedBlockExpandedResponse, }, { description: "Get multiple expanded blocks by heights", request: getByHeightsExpandedURL(t, heights...), expectedStatus: http.StatusOK, - expectedResponse: multipleBlockExpandedResponse, + expectedResponse: multipleSealedBlockExpandedResponse, }, { description: "Get multiple expanded blocks by start and end height", request: getByStartEndHeightExpandedURL(t, heights[0], heights[len(heights)-1]), expectedStatus: http.StatusOK, - expectedResponse: multipleBlockExpandedResponse, + expectedResponse: multipleSealedBlockExpandedResponse, }, { description: "Get block by ID not found", @@ -223,20 +225,21 @@ func generateMocks(backend *mock.API, count int) ([]string, []string, []*flow.Bl return blockIDs, heights, blocks, executionResults } -func expectedBlockResponsesExpanded(blocks []*flow.Block, execResult []*flow.ExecutionResult, expanded bool) string { +func expectedBlockResponsesExpanded(blocks []*flow.Block, execResult []*flow.ExecutionResult, expanded bool, status flow.BlockStatus) string { blockResponses := make([]string, len(blocks)) for i, b := range blocks { - blockResponses[i] = expectedBlockResponse(b, execResult[i], expanded) + blockResponses[i] = expectedBlockResponse(b, execResult[i], expanded, status) } return fmt.Sprintf("[%s]", strings.Join(blockResponses, ",")) } -func expectedBlockResponse(block *flow.Block, execResult *flow.ExecutionResult, expanded bool) string { +func expectedBlockResponse(block *flow.Block, execResult *flow.ExecutionResult, expanded bool, status flow.BlockStatus) string { id := block.ID().String() execResultID := execResult.ID().String() execLink := fmt.Sprintf("/v1/execution_results/%s", execResultID) blockLink := fmt.Sprintf("/v1/blocks/%s", id) payloadLink := fmt.Sprintf("/v1/blocks/%s/payload", id) + blockStatus := status.String() timestamp := block.Header.Timestamp.Format(time.RFC3339Nano) @@ -258,9 +261,10 @@ func expectedBlockResponse(block *flow.Block, execResult *flow.ExecutionResult, "_expandable": {}, "_links": { "_self": "%s" - } + }, + "block_status": "%s" }`, id, block.Header.ParentID.String(), block.Header.Height, timestamp, - util.ToBase64(block.Header.ParentVoterSigData), executionResultExpectedStr(execResult), blockLink) + util.ToBase64(block.Header.ParentVoterSigData), executionResultExpectedStr(execResult), blockLink, blockStatus) } return fmt.Sprintf(` @@ -278,7 +282,8 @@ func expectedBlockResponse(block *flow.Block, execResult *flow.ExecutionResult, }, "_links": { "_self": "%s" - } + }, + "block_status": "%s" }`, id, block.Header.ParentID.String(), block.Header.Height, timestamp, - util.ToBase64(block.Header.ParentVoterSigData), payloadLink, execLink, blockLink) + util.ToBase64(block.Header.ParentVoterSigData), payloadLink, execLink, blockLink, blockStatus) } diff --git a/engine/access/rest/models/block.go b/engine/access/rest/models/block.go index d42c0ecf5d1..3d5ccc64bf9 100644 --- a/engine/access/rest/models/block.go +++ b/engine/access/rest/models/block.go @@ -9,6 +9,7 @@ func (b *Block) Build( block *flow.Block, execResult *flow.ExecutionResult, link LinkGenerator, + blockStatus flow.BlockStatus, expand map[string]bool, ) error { self, err := SelfLink(block.ID(), link.BlockLink) @@ -61,6 +62,7 @@ func (b *Block) Build( } b.Links = self + b.BlockStatus = blockStatus.String() return nil } diff --git a/engine/access/rest/models/model_block.go b/engine/access/rest/models/model_block.go index 6febe81cde1..ded5008bcdc 100644 --- a/engine/access/rest/models/model_block.go +++ b/engine/access/rest/models/model_block.go @@ -14,4 +14,5 @@ type Block struct { ExecutionResult *ExecutionResult `json:"execution_result,omitempty"` Expandable *BlockExpandable `json:"_expandable"` Links *Links `json:"_links,omitempty"` + BlockStatus string `json:"block_status"` } From 73e557c271f2a732258df706714a70a449241a61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Tue, 28 Feb 2023 13:54:57 -0800 Subject: [PATCH 02/16] update to Cadence v0.31.5-account-link-pragma (v0.31.5 + account linking pragma) --- go.mod | 2 +- go.sum | 4 ++-- insecure/go.mod | 2 +- insecure/go.sum | 4 ++-- integration/go.mod | 2 +- integration/go.sum | 4 ++-- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 4c26c43ffb0..9ed6534426f 100644 --- a/go.mod +++ b/go.mod @@ -57,7 +57,7 @@ require ( github.com/multiformats/go-multiaddr-dns v0.3.1 github.com/multiformats/go-multihash v0.2.1 github.com/onflow/atree v0.4.0 - github.com/onflow/cadence v0.31.5 + github.com/onflow/cadence v0.31.5-account-link-pragma github.com/onflow/flow v0.3.2 github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 diff --git a/go.sum b/go.sum index e7da2d34c71..d6c64a937f7 100644 --- a/go.sum +++ b/go.sum @@ -1223,8 +1223,8 @@ github.com/olekukonko/tablewriter v0.0.1/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXW github.com/olekukonko/tablewriter v0.0.2-0.20190409134802-7e037d187b0c/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo= github.com/onflow/atree v0.4.0 h1:+TbNisavAkukAKhgQ4plWnvR9o5+SkwPIsi3jaeAqKs= github.com/onflow/atree v0.4.0/go.mod h1:7Qe1xaW0YewvouLXrugzMFUYXNoRQ8MT/UsVAWx1Ndo= -github.com/onflow/cadence v0.31.5 h1:XzenJKuSMp4qHID1z04/ROf5anZtl2v8yP03kNHem0g= -github.com/onflow/cadence v0.31.5/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= +github.com/onflow/cadence v0.31.5-account-link-pragma h1:e4ldX3uNdDJ1o5XNJI996vPYxoP2WtDbDaA2F1nzbXk= +github.com/onflow/cadence v0.31.5-account-link-pragma/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= github.com/onflow/flow v0.3.2 h1:z3IuKOjM9Tvf0pXfloTbrLxM5nTunI47cklsDd+wxBE= github.com/onflow/flow v0.3.2/go.mod h1:lzyAYmbu1HfkZ9cfnL5/sjrrsnJiUU8fRL26CqLP7+c= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 h1:fH5e7L9xFXNOd3WLvMaPNkP1m7BngRTDP751zMNndws= diff --git a/insecure/go.mod b/insecure/go.mod index a2d22045c30..4565e6ea55b 100644 --- a/insecure/go.mod +++ b/insecure/go.mod @@ -176,7 +176,7 @@ require ( github.com/multiformats/go-varint v0.0.6 // indirect github.com/nxadm/tail v1.4.8 // indirect github.com/onflow/atree v0.4.0 // indirect - github.com/onflow/cadence v0.31.5 // indirect + github.com/onflow/cadence v0.31.5-account-link-pragma // indirect github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 // indirect github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 // indirect github.com/onflow/flow-ft/lib/go/contracts v0.5.0 // indirect diff --git a/insecure/go.sum b/insecure/go.sum index c085fe7d3b4..7d9cd681d9c 100644 --- a/insecure/go.sum +++ b/insecure/go.sum @@ -1182,8 +1182,8 @@ github.com/olekukonko/tablewriter v0.0.1/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXW github.com/olekukonko/tablewriter v0.0.2-0.20190409134802-7e037d187b0c/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo= github.com/onflow/atree v0.4.0 h1:+TbNisavAkukAKhgQ4plWnvR9o5+SkwPIsi3jaeAqKs= github.com/onflow/atree v0.4.0/go.mod h1:7Qe1xaW0YewvouLXrugzMFUYXNoRQ8MT/UsVAWx1Ndo= -github.com/onflow/cadence v0.31.5 h1:XzenJKuSMp4qHID1z04/ROf5anZtl2v8yP03kNHem0g= -github.com/onflow/cadence v0.31.5/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= +github.com/onflow/cadence v0.31.5-account-link-pragma h1:e4ldX3uNdDJ1o5XNJI996vPYxoP2WtDbDaA2F1nzbXk= +github.com/onflow/cadence v0.31.5-account-link-pragma/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 h1:fH5e7L9xFXNOd3WLvMaPNkP1m7BngRTDP751zMNndws= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722/go.mod h1:9nrgjIF/noY2jJ7LP8bKLHTpcdHOa2yO0ryCKTQpxvs= github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 h1:vgNS6I+MM/74pciIoKb7ZBs8XGF1ONsSdkAec36B9iU= diff --git a/integration/go.mod b/integration/go.mod index c5f7a7e45be..5beb78bc5e3 100644 --- a/integration/go.mod +++ b/integration/go.mod @@ -16,7 +16,7 @@ require ( github.com/ipfs/go-datastore v0.6.0 github.com/ipfs/go-ds-badger2 v0.1.3 github.com/ipfs/go-ipfs-blockstore v1.2.0 - github.com/onflow/cadence v0.31.5 + github.com/onflow/cadence v0.31.5-account-link-pragma github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 github.com/onflow/flow-emulator v0.38.1 diff --git a/integration/go.sum b/integration/go.sum index b05d5f0c4f2..70a32f4bc0a 100644 --- a/integration/go.sum +++ b/integration/go.sum @@ -1304,8 +1304,8 @@ github.com/olekukonko/tablewriter v0.0.1/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXW github.com/olekukonko/tablewriter v0.0.2-0.20190409134802-7e037d187b0c/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo= github.com/onflow/atree v0.4.0 h1:+TbNisavAkukAKhgQ4plWnvR9o5+SkwPIsi3jaeAqKs= github.com/onflow/atree v0.4.0/go.mod h1:7Qe1xaW0YewvouLXrugzMFUYXNoRQ8MT/UsVAWx1Ndo= -github.com/onflow/cadence v0.31.5 h1:XzenJKuSMp4qHID1z04/ROf5anZtl2v8yP03kNHem0g= -github.com/onflow/cadence v0.31.5/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= +github.com/onflow/cadence v0.31.5-account-link-pragma h1:e4ldX3uNdDJ1o5XNJI996vPYxoP2WtDbDaA2F1nzbXk= +github.com/onflow/cadence v0.31.5-account-link-pragma/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 h1:fH5e7L9xFXNOd3WLvMaPNkP1m7BngRTDP751zMNndws= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722/go.mod h1:9nrgjIF/noY2jJ7LP8bKLHTpcdHOa2yO0ryCKTQpxvs= github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 h1:vgNS6I+MM/74pciIoKb7ZBs8XGF1ONsSdkAec36B9iU= From 4f2e55e8e1f51d6efed7aab4068453a498841bb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Tue, 28 Feb 2023 15:58:54 -0800 Subject: [PATCH 03/16] enable account linking, but require pragma allowing usage --- engine/execution/computation/manager.go | 7 ++--- fvm/fvm_test.go | 39 ++++++++++++++++--------- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/engine/execution/computation/manager.go b/engine/execution/computation/manager.go index 3ba10926a1c..f510922e546 100644 --- a/engine/execution/computation/manager.go +++ b/engine/execution/computation/manager.go @@ -117,16 +117,13 @@ func New( vm = fvm.NewVirtualMachine() } - chainID := vmCtx.Chain.ChainID() - options := []fvm.Option{ fvm.WithReusableCadenceRuntimePool( reusableRuntime.NewReusableCadenceRuntimePool( ReusableCadenceRuntimePoolSize, runtime.Config{ - TracingEnabled: params.CadenceTracing, - // AccountLinking is enabled everywhere except on mainnet - AccountLinkingEnabled: chainID != flow.Mainnet, + TracingEnabled: params.CadenceTracing, + AccountLinkingEnabled: true, }, ), ), diff --git a/fvm/fvm_test.go b/fvm/fvm_test.go index 97275cba89a..aeb53113a76 100644 --- a/fvm/fvm_test.go +++ b/fvm/fvm_test.go @@ -2221,7 +2221,7 @@ func TestScriptIterationShouldNotHitsParseRestrictions(t *testing.T) { } func TestAuthAccountCapabilities(t *testing.T) { - test := func(t *testing.T, linkingEnabled bool) { + test := func(t *testing.T, allowAccountLinking bool) { newVMTest(). withBootstrapProcedureOptions(). withContextOptions( @@ -2229,7 +2229,7 @@ func TestAuthAccountCapabilities(t *testing.T) { reusableRuntime.NewReusableCadenceRuntimePool( 1, runtime.Config{ - AccountLinkingEnabled: linkingEnabled, + AccountLinkingEnabled: true, }, ), ), @@ -2253,13 +2253,26 @@ func TestAuthAccountCapabilities(t *testing.T) { require.NoError(t, err) account := accounts[0] - txBody := flow.NewTransactionBody().SetScript([]byte(` - transaction { - prepare(acct: AuthAccount) { - acct.linkAccount(/public/foo) - } - } - `)). + var pragma string + if allowAccountLinking { + pragma = "#allowAccountLinking" + } + + code := fmt.Sprintf( + ` + %s + + transaction { + prepare(acct: AuthAccount) { + acct.linkAccount(/public/foo) + } + } + `, + pragma, + ) + + txBody := flow.NewTransactionBody(). + SetScript([]byte(code)). AddAuthorizer(account). SetPayer(chain.ServiceAddress()). SetProposalKey(chain.ServiceAddress(), 0, 0) @@ -2270,7 +2283,7 @@ func TestAuthAccountCapabilities(t *testing.T) { err = vm.Run(ctx, tx, view) require.NoError(t, err) - if linkingEnabled { + if allowAccountLinking { require.NoError(t, tx.Err) } else { require.Error(t, tx.Err) @@ -2279,11 +2292,11 @@ func TestAuthAccountCapabilities(t *testing.T) { )(t) } - t.Run("linking enabled", func(t *testing.T) { + t.Run("account linking allowed", func(t *testing.T) { test(t, true) }) - t.Run("linking disabled", func(t *testing.T) { + + t.Run("account linking disallowed", func(t *testing.T) { test(t, false) }) - } From e2e3d698f831f73901117d051ffc35cd9d565cd1 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Mon, 12 Dec 2022 16:33:17 +0100 Subject: [PATCH 04/16] smart cache invalidation step 1. --- fvm/derived/derived_block_data.go | 19 ++++++++++++------- fvm/derived/invalidator.go | 3 +-- fvm/environment/derived_data_invalidator.go | 3 +-- fvm/environment/programs.go | 11 +++++++---- 4 files changed, 21 insertions(+), 15 deletions(-) diff --git a/fvm/derived/derived_block_data.go b/fvm/derived/derived_block_data.go index b3ec50c4f49..3bf31f30af3 100644 --- a/fvm/derived/derived_block_data.go +++ b/fvm/derived/derived_block_data.go @@ -9,10 +9,15 @@ import ( "github.com/onflow/flow-go/fvm/state" ) +type Program struct { + *interpreter.Program + dependencies map[common.AddressLocation]struct{} +} + // DerivedBlockData is a simple fork-aware OCC database for "caching" derived // data for a particular block. type DerivedBlockData struct { - programs *DerivedDataTable[common.AddressLocation, *interpreter.Program] + programs *DerivedDataTable[common.AddressLocation, *Program] meterParamOverrides *DerivedDataTable[struct{}, MeterParamOverrides] } @@ -22,7 +27,7 @@ type DerivedBlockData struct { type DerivedTransactionData struct { programs *TableTransaction[ common.AddressLocation, - *interpreter.Program, + *Program, ] // There's only a single entry in this table. For simplicity, we'll use @@ -34,7 +39,7 @@ func NewEmptyDerivedBlockData() *DerivedBlockData { return &DerivedBlockData{ programs: NewEmptyTable[ common.AddressLocation, - *interpreter.Program, + *Program, ](), meterParamOverrides: NewEmptyTable[ struct{}, @@ -49,7 +54,7 @@ func NewEmptyDerivedBlockDataWithTransactionOffset(offset uint32) *DerivedBlockD return &DerivedBlockData{ programs: NewEmptyTableWithOffset[ common.AddressLocation, - *interpreter.Program, + *Program, ](offset), meterParamOverrides: NewEmptyTableWithOffset[ struct{}, @@ -126,14 +131,14 @@ func (block *DerivedBlockData) NextTxIndexForTestingOnly() uint32 { func (block *DerivedBlockData) GetProgramForTestingOnly( addressLocation common.AddressLocation, -) *invalidatableEntry[*interpreter.Program] { +) *invalidatableEntry[*Program] { return block.programs.GetForTestingOnly(addressLocation) } func (transaction *DerivedTransactionData) GetProgram( addressLocation common.AddressLocation, ) ( - *interpreter.Program, + *Program, *state.State, bool, ) { @@ -142,7 +147,7 @@ func (transaction *DerivedTransactionData) GetProgram( func (transaction *DerivedTransactionData) SetProgram( addressLocation common.AddressLocation, - program *interpreter.Program, + program *Program, state *state.State, ) { transaction.programs.Set(addressLocation, program, state) diff --git a/fvm/derived/invalidator.go b/fvm/derived/invalidator.go index fb7cc41d20d..63e3ee290d5 100644 --- a/fvm/derived/invalidator.go +++ b/fvm/derived/invalidator.go @@ -2,7 +2,6 @@ package derived import ( "github.com/onflow/cadence/runtime/common" - "github.com/onflow/cadence/runtime/interpreter" "github.com/onflow/flow-go/fvm/meter" ) @@ -15,7 +14,7 @@ type MeterParamOverrides struct { type ProgramInvalidator TableInvalidator[ common.AddressLocation, - *interpreter.Program, + *Program, ] type MeterParamOverridesInvalidator TableInvalidator[ diff --git a/fvm/environment/derived_data_invalidator.go b/fvm/environment/derived_data_invalidator.go index 31e030f7bab..5c546a3887f 100644 --- a/fvm/environment/derived_data_invalidator.go +++ b/fvm/environment/derived_data_invalidator.go @@ -2,7 +2,6 @@ package environment import ( "github.com/onflow/cadence/runtime/common" - "github.com/onflow/cadence/runtime/interpreter" "github.com/onflow/flow-go/fvm/derived" "github.com/onflow/flow-go/fvm/state" @@ -91,7 +90,7 @@ func (invalidator ProgramInvalidator) ShouldInvalidateEntries() bool { func (invalidator ProgramInvalidator) ShouldInvalidateEntry( location common.AddressLocation, - program *interpreter.Program, + program *derived.Program, state *state.State, ) bool { // TODO(rbtz): switch to fine grain invalidation. diff --git a/fvm/environment/programs.go b/fvm/environment/programs.go index 7c9651055e0..3e507b0a297 100644 --- a/fvm/environment/programs.go +++ b/fvm/environment/programs.go @@ -8,6 +8,7 @@ import ( "github.com/onflow/cadence/runtime/common" "github.com/onflow/cadence/runtime/interpreter" + "github.com/onflow/flow-go/fvm/derived" "github.com/onflow/flow-go/fvm/errors" "github.com/onflow/flow-go/fvm/state" "github.com/onflow/flow-go/model/flow" @@ -17,8 +18,8 @@ import ( // TODO(patrick): remove and switch to *programs.DerivedTransactionData once // https://github.com/onflow/flow-emulator/pull/229 is integrated. type DerivedTransactionData interface { - GetProgram(loc common.AddressLocation) (*interpreter.Program, *state.State, bool) - SetProgram(loc common.AddressLocation, prog *interpreter.Program, state *state.State) + GetProgram(loc common.AddressLocation) (*derived.Program, *state.State, bool) + SetProgram(loc common.AddressLocation, prog *derived.Program, state *state.State) } // Programs manages operations around cadence program parsing. @@ -98,7 +99,9 @@ func (programs *Programs) set( return nil } - programs.derivedTxnData.SetProgram(address, program, state) + programs.derivedTxnData.SetProgram(address, &derived.Program{ + Program: program, + }, state) return nil } @@ -128,7 +131,7 @@ func (programs *Programs) get( err)) } - return program, true + return program.Program, true } // Address location program is reusable across transactions. Create From b0c87508ec8876dce5fa63af5f2e4dd9e53623f0 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Mon, 12 Dec 2022 17:38:20 +0100 Subject: [PATCH 05/16] cache invalidation step 2. --- fvm/derived/derived_block_data.go | 2 +- fvm/environment/derived_data_invalidator.go | 21 ++++++++++++-- fvm/environment/programs.go | 31 ++++++++++++++++++++- 3 files changed, 50 insertions(+), 4 deletions(-) diff --git a/fvm/derived/derived_block_data.go b/fvm/derived/derived_block_data.go index 3bf31f30af3..0c82a1f4f4c 100644 --- a/fvm/derived/derived_block_data.go +++ b/fvm/derived/derived_block_data.go @@ -11,7 +11,7 @@ import ( type Program struct { *interpreter.Program - dependencies map[common.AddressLocation]struct{} + Dependencies map[common.AddressLocation]struct{} } // DerivedBlockData is a simple fork-aware OCC database for "caching" derived diff --git a/fvm/environment/derived_data_invalidator.go b/fvm/environment/derived_data_invalidator.go index 5c546a3887f..29b1a23709e 100644 --- a/fvm/environment/derived_data_invalidator.go +++ b/fvm/environment/derived_data_invalidator.go @@ -93,8 +93,25 @@ func (invalidator ProgramInvalidator) ShouldInvalidateEntry( program *derived.Program, state *state.State, ) bool { - // TODO(rbtz): switch to fine grain invalidation. - return invalidator.ShouldInvalidateEntries() + if invalidator.MeterParamOverridesUpdated { + return true + } + + if len(invalidator.FrozenAccounts) > 0 { + // TODO: switch to fine grain invalidation. + return true + } + + for _, key := range invalidator.ContractUpdateKeys { + _, ok := program.Dependencies[common.AddressLocation{ + Address: common.MustBytesToAddress(key.Address.Bytes()), + Name: key.Name, + }] + if ok { + return true + } + } + return false } type MeterParamOverridesInvalidator struct { diff --git a/fvm/environment/programs.go b/fvm/environment/programs.go index 3e507b0a297..2e2919be6c3 100644 --- a/fvm/environment/programs.go +++ b/fvm/environment/programs.go @@ -40,6 +40,7 @@ type Programs struct { // NOTE: non-address programs are not reusable across transactions, hence // they are kept out of the derived data database. nonAddressPrograms map[common.Location]*interpreter.Program + dependencies map[common.AddressLocation]map[common.AddressLocation]struct{} } // NewPrograms construts a new ProgramHandler @@ -57,6 +58,7 @@ func NewPrograms( accounts: accounts, derivedTxnData: derivedTxnData, nonAddressPrograms: make(map[common.Location]*interpreter.Program), + dependencies: make(map[common.AddressLocation]map[common.AddressLocation]struct{}), } } @@ -99,8 +101,17 @@ func (programs *Programs) set( return nil } + // stop tracking dependencies + dependencies, ok := programs.dependencies[address] + if !ok { + dependencies = make(map[common.AddressLocation]struct{}) + } else { + delete(programs.dependencies, address) + } + programs.derivedTxnData.SetProgram(address, &derived.Program{ - Program: program, + Program: program, + Dependencies: dependencies, }, state) return nil } @@ -124,6 +135,16 @@ func (programs *Programs) get( program, state, has := programs.derivedTxnData.GetProgram(address) if has { + + // add the dependencies to the currents program being loaded + // if one is being loaded + for _, dependencies := range programs.dependencies { + dependencies[address] = struct{}{} + for newDep := range program.Dependencies { + dependencies[newDep] = struct{}{} + } + } + err := programs.txnState.AttachAndCommit(state) if err != nil { panic(fmt.Sprintf( @@ -143,6 +164,14 @@ func (programs *Programs) get( panic(err) } + // add the dependencies to the current programs being loaded + for _, dependencies := range programs.dependencies { + dependencies[address] = struct{}{} + } + + // start tracking dependencies for this program + programs.dependencies[address] = make(map[common.AddressLocation]struct{}) + return nil, false } From 40be658b59c0d8ef81812ba4ce4d5e5916bbf91b Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Mon, 12 Dec 2022 22:12:51 +0100 Subject: [PATCH 06/16] add dependency tracking and invalidation --- fvm/derived/derived_block_data.go | 18 +- fvm/derived/derived_chain_data_test.go | 8 +- fvm/environment/contract_updater.go | 9 +- fvm/environment/contract_updater_test.go | 8 +- fvm/environment/derived_data_invalidator.go | 21 +- .../derived_data_invalidator_test.go | 200 ++++++++++++++---- .../mock/derived_transaction_data.go | 13 +- fvm/environment/programs.go | 108 +++++++--- fvm/environment/programs_test.go | 35 ++- fvm/programs/programs.go | 7 +- 10 files changed, 322 insertions(+), 105 deletions(-) diff --git a/fvm/derived/derived_block_data.go b/fvm/derived/derived_block_data.go index 0c82a1f4f4c..78ddafc5fbc 100644 --- a/fvm/derived/derived_block_data.go +++ b/fvm/derived/derived_block_data.go @@ -9,9 +9,25 @@ import ( "github.com/onflow/flow-go/fvm/state" ) +// ProgramDependencies are the locations of the programs this program depends on. +type ProgramDependencies map[common.Address]struct{} + +// AddDependency adds the address as a dependency. +func (d ProgramDependencies) AddDependency(address common.Address) { + d[address] = struct{}{} +} + +// Merge merges current dependencies with other dependencies. +func (d ProgramDependencies) Merge(other ProgramDependencies) { + for address := range other { + d[address] = struct{}{} + } +} + type Program struct { *interpreter.Program - Dependencies map[common.AddressLocation]struct{} + + Dependencies ProgramDependencies } // DerivedBlockData is a simple fork-aware OCC database for "caching" derived diff --git a/fvm/derived/derived_chain_data_test.go b/fvm/derived/derived_chain_data_test.go index ee8b2b2a780..71eb1cd37ef 100644 --- a/fvm/derived/derived_chain_data_test.go +++ b/fvm/derived/derived_chain_data_test.go @@ -37,7 +37,9 @@ func TestDerivedChainData(t *testing.T) { require.NotNil(t, block1) loc1 := testLocation("0a") - prog1 := &interpreter.Program{} + prog1 := &Program{ + Program: &interpreter.Program{}, + } txn, err := block1.NewDerivedTransactionData(0, 0) require.NoError(t, err) @@ -61,7 +63,9 @@ func TestDerivedChainData(t *testing.T) { require.NotSame(t, block1, block2) loc2 := testLocation("0b") - prog2 := &interpreter.Program{} + prog2 := &Program{ + Program: &interpreter.Program{}, + } txn, err = block2.NewDerivedTransactionData(0, 0) require.NoError(t, err) diff --git a/fvm/environment/contract_updater.go b/fvm/environment/contract_updater.go index 59dcfba7b30..b3a1630674d 100644 --- a/fvm/environment/contract_updater.go +++ b/fvm/environment/contract_updater.go @@ -439,7 +439,7 @@ func (updater *ContractUpdaterImpl) SetContract( } contractUpdateKey := ContractUpdateKey{ - Address: flowAddress, + Address: address, Name: name, } @@ -465,8 +465,7 @@ func (updater *ContractUpdaterImpl) RemoveContract( "accounts")) } - add := flow.Address(address) - uk := ContractUpdateKey{Address: add, Name: name} + uk := ContractUpdateKey{Address: address, Name: name} u := ContractUpdate{ContractUpdateKey: uk} updater.draftUpdates[uk] = u @@ -480,12 +479,12 @@ func (updater *ContractUpdaterImpl) Commit() ([]ContractUpdateKey, error) { var err error for _, v := range updateList { if len(v.Code) > 0 { - err = updater.accounts.SetContract(v.Name, v.Address, v.Code) + err = updater.accounts.SetContract(v.Name, flow.BytesToAddress(v.Address.Bytes()), v.Code) if err != nil { return nil, err } } else { - err = updater.accounts.DeleteContract(v.Name, v.Address) + err = updater.accounts.DeleteContract(v.Name, flow.BytesToAddress(v.Address.Bytes())) if err != nil { return nil, err } diff --git a/fvm/environment/contract_updater_test.go b/fvm/environment/contract_updater_test.go index 9abdbe031a3..bb528e345fe 100644 --- a/fvm/environment/contract_updater_test.go +++ b/fvm/environment/contract_updater_test.go @@ -314,6 +314,7 @@ func TestContract_ContractUpdate(t *testing.T) { accounts := environment.NewAccounts(txnState) flowAddress := flow.HexToAddress("01") + flowCommonAddress := common.MustBytesToAddress(flowAddress.Bytes()) runtimeAddress := runtime.Address(flowAddress) err := accounts.Create(nil, flowAddress) require.NoError(t, err) @@ -353,7 +354,7 @@ func TestContract_ContractUpdate(t *testing.T) { t, []environment.ContractUpdateKey{ { - Address: flowAddress, + Address: flowCommonAddress, Name: "TestContract", }, }, @@ -412,6 +413,7 @@ func TestContract_ContractRemoval(t *testing.T) { accounts := environment.NewAccounts(txnState) flowAddress := flow.HexToAddress("01") + flowCommonAddress := common.MustBytesToAddress(flowAddress.Bytes()) runtimeAddress := runtime.Address(flowAddress) err := accounts.Create(nil, flowAddress) require.NoError(t, err) @@ -451,7 +453,7 @@ func TestContract_ContractRemoval(t *testing.T) { t, []environment.ContractUpdateKey{ { - Address: flowAddress, + Address: flowCommonAddress, Name: "TestContract", }, }, @@ -515,7 +517,7 @@ func TestContract_ContractRemoval(t *testing.T) { t, []environment.ContractUpdateKey{ { - Address: flowAddress, + Address: flowCommonAddress, Name: "TestContract", }, }, diff --git a/fvm/environment/derived_data_invalidator.go b/fvm/environment/derived_data_invalidator.go index 29b1a23709e..d061a1ac7b5 100644 --- a/fvm/environment/derived_data_invalidator.go +++ b/fvm/environment/derived_data_invalidator.go @@ -5,11 +5,10 @@ import ( "github.com/onflow/flow-go/fvm/derived" "github.com/onflow/flow-go/fvm/state" - "github.com/onflow/flow-go/model/flow" ) type ContractUpdateKey struct { - Address flow.Address + Address common.Address Name string } @@ -94,19 +93,23 @@ func (invalidator ProgramInvalidator) ShouldInvalidateEntry( state *state.State, ) bool { if invalidator.MeterParamOverridesUpdated { + // if meter parameters changed we need to invalidate all programs return true } - if len(invalidator.FrozenAccounts) > 0 { - // TODO: switch to fine grain invalidation. - return true + // if an account was (un)frozen we need to invalidate all + // programs that depend on any contract on that address. + for _, frozenAccount := range invalidator.FrozenAccounts { + _, ok := program.Dependencies[frozenAccount] + if ok { + return true + } } + // invalidate all programs depending on any of the contracts that were updated + // A program has itself listed as a dependency, so that this simpler. for _, key := range invalidator.ContractUpdateKeys { - _, ok := program.Dependencies[common.AddressLocation{ - Address: common.MustBytesToAddress(key.Address.Bytes()), - Name: key.Name, - }] + _, ok := program.Dependencies[key.Address] if ok { return true } diff --git a/fvm/environment/derived_data_invalidator_test.go b/fvm/environment/derived_data_invalidator_test.go index 57e54dfe12c..8f2154b60b1 100644 --- a/fvm/environment/derived_data_invalidator_test.go +++ b/fvm/environment/derived_data_invalidator_test.go @@ -17,53 +17,173 @@ import ( ) func TestDerivedDataProgramInvalidator(t *testing.T) { - invalidator := environment.DerivedDataInvalidator{}.ProgramInvalidator() - require.False(t, invalidator.ShouldInvalidateEntries()) - require.False(t, invalidator.ShouldInvalidateEntry( - common.AddressLocation{}, - nil, - nil)) - - invalidator = environment.DerivedDataInvalidator{ - ContractUpdateKeys: []environment.ContractUpdateKey{ - {}, // For now, the entry's value does not matter. + // create the following dependency graph + // ```mermaid + // graph TD + // C-->D + // C-->B + // B-->A + // ``` + + addressA := flow.HexToAddress("0xa") + cAddressA := common.MustBytesToAddress(addressA.Bytes()) + programALoc := common.AddressLocation{Address: cAddressA, Name: "A"} + programA := &derived.Program{ + Program: nil, + Dependencies: map[common.Address]struct{}{ + cAddressA: {}, }, - FrozenAccounts: nil, - MeterParamOverridesUpdated: false, - }.ProgramInvalidator() - - require.True(t, invalidator.ShouldInvalidateEntries()) - require.True(t, invalidator.ShouldInvalidateEntry( - common.AddressLocation{}, - nil, - nil)) + } - invalidator = environment.DerivedDataInvalidator{ - ContractUpdateKeys: nil, - FrozenAccounts: []common.Address{ - {}, // For now, the entry's value does not matter + addressB := flow.HexToAddress("0xb") + cAddressB := common.MustBytesToAddress(addressB.Bytes()) + programBLoc := common.AddressLocation{Address: cAddressB, Name: "B"} + programB := &derived.Program{ + Program: nil, + Dependencies: map[common.Address]struct{}{ + cAddressA: {}, + cAddressB: {}, }, - MeterParamOverridesUpdated: false, - }.ProgramInvalidator() + } - require.True(t, invalidator.ShouldInvalidateEntries()) - require.True(t, invalidator.ShouldInvalidateEntry( - common.AddressLocation{}, - nil, - nil)) + addressD := flow.HexToAddress("0xd") + cAddressD := common.MustBytesToAddress(addressD.Bytes()) + programDLoc := common.AddressLocation{Address: cAddressD, Name: "D"} + programD := &derived.Program{ + Program: nil, + Dependencies: map[common.Address]struct{}{ + cAddressD: {}, + }, + } - invalidator = environment.DerivedDataInvalidator{ - ContractUpdateKeys: nil, - FrozenAccounts: nil, - MeterParamOverridesUpdated: true, - }.ProgramInvalidator() + addressC := flow.HexToAddress("0xc") + cAddressC := common.MustBytesToAddress(addressC.Bytes()) + programCLoc := common.AddressLocation{Address: cAddressC, Name: "C"} + programC := &derived.Program{ + Program: nil, + Dependencies: map[common.Address]struct{}{ + // C indirectly depends on A trough B + cAddressA: {}, + cAddressB: {}, + cAddressC: {}, + cAddressD: {}, + }, + } - require.True(t, invalidator.ShouldInvalidateEntries()) - require.True(t, invalidator.ShouldInvalidateEntry( - common.AddressLocation{}, - nil, - nil)) + t.Run("empty invalidator does not invalidate entries", func(t *testing.T) { + invalidator := environment.DerivedDataInvalidator{}.ProgramInvalidator() + + require.False(t, invalidator.ShouldInvalidateEntries()) + require.False(t, invalidator.ShouldInvalidateEntry(programALoc, programA, nil)) + require.False(t, invalidator.ShouldInvalidateEntry(programBLoc, programB, nil)) + require.False(t, invalidator.ShouldInvalidateEntry(programCLoc, programC, nil)) + require.False(t, invalidator.ShouldInvalidateEntry(programDLoc, programD, nil)) + }) + t.Run("meter parameters invalidator invalidates all entries", func(t *testing.T) { + invalidator := environment.DerivedDataInvalidator{ + MeterParamOverridesUpdated: true, + }.ProgramInvalidator() + + require.True(t, invalidator.ShouldInvalidateEntries()) + require.True(t, invalidator.ShouldInvalidateEntry(programALoc, programA, nil)) + require.True(t, invalidator.ShouldInvalidateEntry(programBLoc, programB, nil)) + require.True(t, invalidator.ShouldInvalidateEntry(programCLoc, programC, nil)) + require.True(t, invalidator.ShouldInvalidateEntry(programDLoc, programD, nil)) + }) + + t.Run("address invalidator A invalidates all but D", func(t *testing.T) { + invalidator := environment.DerivedDataInvalidator{ + FrozenAccounts: []common.Address{ + cAddressA, + }, + }.ProgramInvalidator() + + require.True(t, invalidator.ShouldInvalidateEntries()) + require.True(t, invalidator.ShouldInvalidateEntry(programALoc, programA, nil)) + require.True(t, invalidator.ShouldInvalidateEntry(programBLoc, programB, nil)) + require.True(t, invalidator.ShouldInvalidateEntry(programCLoc, programC, nil)) + require.False(t, invalidator.ShouldInvalidateEntry(programDLoc, programD, nil)) + }) + + t.Run("address invalidator D invalidates D, C", func(t *testing.T) { + invalidator := environment.DerivedDataInvalidator{ + FrozenAccounts: []common.Address{ + cAddressD, + }, + }.ProgramInvalidator() + + require.True(t, invalidator.ShouldInvalidateEntries()) + require.False(t, invalidator.ShouldInvalidateEntry(programALoc, programA, nil)) + require.False(t, invalidator.ShouldInvalidateEntry(programBLoc, programB, nil)) + require.True(t, invalidator.ShouldInvalidateEntry(programCLoc, programC, nil)) + require.True(t, invalidator.ShouldInvalidateEntry(programDLoc, programD, nil)) + }) + + t.Run("address invalidator B invalidates B, C", func(t *testing.T) { + invalidator := environment.DerivedDataInvalidator{ + FrozenAccounts: []common.Address{ + cAddressB, + }, + }.ProgramInvalidator() + + require.True(t, invalidator.ShouldInvalidateEntries()) + require.False(t, invalidator.ShouldInvalidateEntry(programALoc, programA, nil)) + require.True(t, invalidator.ShouldInvalidateEntry(programBLoc, programB, nil)) + require.True(t, invalidator.ShouldInvalidateEntry(programCLoc, programC, nil)) + require.False(t, invalidator.ShouldInvalidateEntry(programDLoc, programD, nil)) + }) + + t.Run("contract invalidator A invalidates all but D", func(t *testing.T) { + invalidator := environment.DerivedDataInvalidator{ + ContractUpdateKeys: []environment.ContractUpdateKey{ + { + Address: cAddressA, + Name: "A", + }, + }, + }.ProgramInvalidator() + + require.True(t, invalidator.ShouldInvalidateEntries()) + require.True(t, invalidator.ShouldInvalidateEntry(programALoc, programA, nil)) + require.True(t, invalidator.ShouldInvalidateEntry(programBLoc, programB, nil)) + require.True(t, invalidator.ShouldInvalidateEntry(programCLoc, programC, nil)) + require.False(t, invalidator.ShouldInvalidateEntry(programDLoc, programD, nil)) + }) + + t.Run("contract invalidator C invalidates C", func(t *testing.T) { + invalidator := environment.DerivedDataInvalidator{ + ContractUpdateKeys: []environment.ContractUpdateKey{ + { + Address: cAddressC, + Name: "C", + }, + }, + }.ProgramInvalidator() + + require.True(t, invalidator.ShouldInvalidateEntries()) + require.False(t, invalidator.ShouldInvalidateEntry(programALoc, programA, nil)) + require.False(t, invalidator.ShouldInvalidateEntry(programBLoc, programB, nil)) + require.True(t, invalidator.ShouldInvalidateEntry(programCLoc, programC, nil)) + require.False(t, invalidator.ShouldInvalidateEntry(programDLoc, programD, nil)) + }) + + t.Run("contract invalidator D invalidates C, D", func(t *testing.T) { + invalidator := environment.DerivedDataInvalidator{ + ContractUpdateKeys: []environment.ContractUpdateKey{ + { + Address: cAddressD, + Name: "D", + }, + }, + }.ProgramInvalidator() + + require.True(t, invalidator.ShouldInvalidateEntries()) + require.False(t, invalidator.ShouldInvalidateEntry(programALoc, programA, nil)) + require.False(t, invalidator.ShouldInvalidateEntry(programBLoc, programB, nil)) + require.True(t, invalidator.ShouldInvalidateEntry(programCLoc, programC, nil)) + require.True(t, invalidator.ShouldInvalidateEntry(programDLoc, programD, nil)) + }) } func TestMeterParamOverridesInvalidator(t *testing.T) { diff --git a/fvm/environment/mock/derived_transaction_data.go b/fvm/environment/mock/derived_transaction_data.go index e39d8307e90..a5f86d06220 100644 --- a/fvm/environment/mock/derived_transaction_data.go +++ b/fvm/environment/mock/derived_transaction_data.go @@ -4,8 +4,7 @@ package mock import ( common "github.com/onflow/cadence/runtime/common" - - interpreter "github.com/onflow/cadence/runtime/interpreter" + derived "github.com/onflow/flow-go/fvm/derived" mock "github.com/stretchr/testify/mock" @@ -18,15 +17,15 @@ type DerivedTransactionData struct { } // GetProgram provides a mock function with given fields: loc -func (_m *DerivedTransactionData) GetProgram(loc common.AddressLocation) (*interpreter.Program, *state.State, bool) { +func (_m *DerivedTransactionData) GetProgram(loc common.AddressLocation) (*derived.Program, *state.State, bool) { ret := _m.Called(loc) - var r0 *interpreter.Program - if rf, ok := ret.Get(0).(func(common.AddressLocation) *interpreter.Program); ok { + var r0 *derived.Program + if rf, ok := ret.Get(0).(func(common.AddressLocation) *derived.Program); ok { r0 = rf(loc) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(*interpreter.Program) + r0 = ret.Get(0).(*derived.Program) } } @@ -50,7 +49,7 @@ func (_m *DerivedTransactionData) GetProgram(loc common.AddressLocation) (*inter } // SetProgram provides a mock function with given fields: loc, prog, _a2 -func (_m *DerivedTransactionData) SetProgram(loc common.AddressLocation, prog *interpreter.Program, _a2 *state.State) { +func (_m *DerivedTransactionData) SetProgram(loc common.AddressLocation, prog *derived.Program, _a2 *state.State) { _m.Called(loc, prog, _a2) } diff --git a/fvm/environment/programs.go b/fvm/environment/programs.go index 2e2919be6c3..023959960f5 100644 --- a/fvm/environment/programs.go +++ b/fvm/environment/programs.go @@ -40,7 +40,9 @@ type Programs struct { // NOTE: non-address programs are not reusable across transactions, hence // they are kept out of the derived data database. nonAddressPrograms map[common.Location]*interpreter.Program - dependencies map[common.AddressLocation]map[common.AddressLocation]struct{} + + // dependencyStack tracks programs currently being loaded and their dependencies. + dependencyStack *dependencyStack } // NewPrograms construts a new ProgramHandler @@ -58,7 +60,7 @@ func NewPrograms( accounts: accounts, derivedTxnData: derivedTxnData, nonAddressPrograms: make(map[common.Location]*interpreter.Program), - dependencies: make(map[common.AddressLocation]map[common.AddressLocation]struct{}), + dependencyStack: newDependencyStack(), } } @@ -92,6 +94,15 @@ func (programs *Programs) set( return err } + location, dependencies, err := programs.dependencyStack.pop() + if err != nil { + return err + } + if location != address { + return fmt.Errorf("cannot set program. Popped dependencies are for an unexpeced address"+ + " (expected %s, got %s)", address, location) + } + // if the program is nil cadence is signaling that the program was not loaded, // but the loading process is complete. // Do not set the program in the cache in this case, @@ -101,14 +112,6 @@ func (programs *Programs) set( return nil } - // stop tracking dependencies - dependencies, ok := programs.dependencies[address] - if !ok { - dependencies = make(map[common.AddressLocation]struct{}) - } else { - delete(programs.dependencies, address) - } - programs.derivedTxnData.SetProgram(address, &derived.Program{ Program: program, Dependencies: dependencies, @@ -135,16 +138,7 @@ func (programs *Programs) get( program, state, has := programs.derivedTxnData.GetProgram(address) if has { - - // add the dependencies to the currents program being loaded - // if one is being loaded - for _, dependencies := range programs.dependencies { - dependencies[address] = struct{}{} - for newDep := range program.Dependencies { - dependencies[newDep] = struct{}{} - } - } - + programs.dependencyStack.addDependencies(program.Dependencies) err := programs.txnState.AttachAndCommit(state) if err != nil { panic(fmt.Sprintf( @@ -155,6 +149,8 @@ func (programs *Programs) get( return program.Program, true } + programs.dependencyStack.push(address) + // Address location program is reusable across transactions. Create // a nested transaction here in order to capture the states read to // parse the program. @@ -164,14 +160,6 @@ func (programs *Programs) get( panic(err) } - // add the dependencies to the current programs being loaded - for _, dependencies := range programs.dependencies { - dependencies[address] = struct{}{} - } - - // start tracking dependencies for this program - programs.dependencies[address] = make(map[common.AddressLocation]struct{}) - return nil, false } @@ -244,3 +232,67 @@ func (programs *Programs) DecodeArgument( return v, err } + +// dependencyTracker tracks dependencies for a location +type dependencyTracker struct { + location common.AddressLocation + dependencies derived.ProgramDependencies +} + +// dependencyStack is a stack of dependencyTracker +// It is used during loading a program to create a dependency list for each program +type dependencyStack struct { + trackers []dependencyTracker +} + +func newDependencyStack() *dependencyStack { + return &dependencyStack{ + trackers: make([]dependencyTracker, 0), + } +} + +// push a new location to track dependencies for. +func (s *dependencyStack) push(loc common.AddressLocation) { + dependencies := make(derived.ProgramDependencies, 1) + + // A program is listed as its own dependency. + dependencies.AddDependency(loc.Address) + + s.trackers = append(s.trackers, dependencyTracker{ + location: loc, + dependencies: dependencies, + }) +} + +// addDependencies adds dependencies to the current dependency tracker +func (s *dependencyStack) addDependencies(dependencies derived.ProgramDependencies) { + l := len(s.trackers) + if l == 0 { + // stack is empty. + // This is expected if loading a program that is already cached. + return + } + + s.trackers[l-1].dependencies.Merge(dependencies) +} + +// pop the last dependencies on the stack and return them. +func (s *dependencyStack) pop() (common.AddressLocation, derived.ProgramDependencies, error) { + if len(s.trackers) == 0 { + return common.AddressLocation{}, + nil, + fmt.Errorf("cannot pop the programs dependency stack, because it is empty") + } + + // pop the last tracker + tracker := s.trackers[len(s.trackers)-1] + s.trackers = s.trackers[:len(s.trackers)-1] + + // there are more trackers in the stack. + // add the dependencies of the popped tracker to the parent tracker + if len(s.trackers) > 0 { + s.trackers[len(s.trackers)-1].dependencies.Merge(tracker.dependencies) + } + + return tracker.location, tracker.dependencies, nil +} diff --git a/fvm/environment/programs_test.go b/fvm/environment/programs_test.go index f091341eb94..ae3e44e413d 100644 --- a/fvm/environment/programs_test.go +++ b/fvm/environment/programs_test.go @@ -212,6 +212,10 @@ func Test_Programs(t *testing.T) { entry := derivedBlockData.GetProgramForTestingOnly(contractALocation) require.NotNil(t, entry) + // assert dependencies are correct + require.Len(t, entry.Value.Dependencies, 1) + require.NotNil(t, entry.Value.Dependencies[common.MustBytesToAddress(addressA.Bytes())]) + // type assertion for further inspections require.IsType(t, entry.State.View(), &delta.View{}) @@ -252,7 +256,7 @@ func Test_Programs(t *testing.T) { require.NoError(t, err) }) - t.Run("deploying another contract cleans programs storage", func(t *testing.T) { + t.Run("deploying another contract invalidates dependant programs", func(t *testing.T) { // deploy contract B procContractB := fvm.Transaction( @@ -261,11 +265,15 @@ func Test_Programs(t *testing.T) { err := vm.Run(context, procContractB, mainView) require.NoError(t, err) - entryA := derivedBlockData.GetProgramForTestingOnly(contractALocation) + // b and c are invalid entryB := derivedBlockData.GetProgramForTestingOnly(contractBLocation) + entryC := derivedBlockData.GetProgramForTestingOnly(contractCLocation) + // a is still valid + entryA := derivedBlockData.GetProgramForTestingOnly(contractALocation) - require.Nil(t, entryA) require.Nil(t, entryB) + require.Nil(t, entryC) + require.NotNil(t, entryA) }) var viewExecB *delta.View @@ -298,6 +306,11 @@ func Test_Programs(t *testing.T) { entryB := derivedBlockData.GetProgramForTestingOnly(contractBLocation) require.NotNil(t, entryB) + // assert dependencies are correct + require.Len(t, entryB.Value.Dependencies, 2) + require.NotNil(t, entryB.Value.Dependencies[common.MustBytesToAddress(addressA.Bytes())]) + require.NotNil(t, entryB.Value.Dependencies[common.MustBytesToAddress(addressB.Bytes())]) + // program B should contain all the registers used by program A, as it depends on it require.IsType(t, entryB.State.View(), &delta.View{}) deltaB := entryB.State.View().(*delta.View) @@ -378,7 +391,7 @@ func Test_Programs(t *testing.T) { require.NoError(t, err) }) - t.Run("deploying contract C cleans programs", func(t *testing.T) { + t.Run("deploying contract C invalidates C", func(t *testing.T) { require.NotNil(t, contractBView) // deploy contract C @@ -392,8 +405,8 @@ func Test_Programs(t *testing.T) { entryB := derivedBlockData.GetProgramForTestingOnly(contractBLocation) entryC := derivedBlockData.GetProgramForTestingOnly(contractCLocation) - require.Nil(t, entryA) - require.Nil(t, entryB) + require.NotNil(t, entryA) + require.NotNil(t, entryB) require.Nil(t, entryC) }) @@ -425,6 +438,16 @@ func Test_Programs(t *testing.T) { require.IsType(t, entryB.State.View(), &delta.View{}) deltaB := entryB.State.View().(*delta.View) compareViews(t, contractBView, deltaB) + + // program C assertions + entryC := derivedBlockData.GetProgramForTestingOnly(contractCLocation) + require.NotNil(t, entryC) + + // assert dependencies are correct + require.Len(t, entryC.Value.Dependencies, 3) + require.NotNil(t, entryC.Value.Dependencies[common.MustBytesToAddress(addressA.Bytes())]) + require.NotNil(t, entryC.Value.Dependencies[common.MustBytesToAddress(addressB.Bytes())]) + require.NotNil(t, entryC.Value.Dependencies[common.MustBytesToAddress(addressC.Bytes())]) }) } diff --git a/fvm/programs/programs.go b/fvm/programs/programs.go index daf9967d6db..dd3eda696dd 100644 --- a/fvm/programs/programs.go +++ b/fvm/programs/programs.go @@ -4,7 +4,6 @@ import ( "sync" "github.com/onflow/cadence/runtime/common" - "github.com/onflow/cadence/runtime/interpreter" "github.com/onflow/flow-go/fvm/derived" "github.com/onflow/flow-go/fvm/state" @@ -56,18 +55,18 @@ func (p *Programs) NextTxIndexForTestingOnly() uint32 { return p.block.NextTxIndexForTestingOnly() } -func (p *Programs) GetForTestingOnly(location common.AddressLocation) (*interpreter.Program, *state.State, bool) { +func (p *Programs) GetForTestingOnly(location common.AddressLocation) (*derived.Program, *state.State, bool) { return p.GetProgram(location) } -func (p *Programs) GetProgram(location common.AddressLocation) (*interpreter.Program, *state.State, bool) { +func (p *Programs) GetProgram(location common.AddressLocation) (*derived.Program, *state.State, bool) { p.lock.RLock() defer p.lock.RUnlock() return p.currentTxn.GetProgram(location) } -func (p *Programs) SetProgram(location common.AddressLocation, program *interpreter.Program, state *state.State) { +func (p *Programs) SetProgram(location common.AddressLocation, program *derived.Program, state *state.State) { p.lock.RLock() defer p.lock.RUnlock() From 36e1a5051a8dd10bf649bed336a89d60b2e6e57e Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Fri, 6 Jan 2023 12:32:53 +0100 Subject: [PATCH 07/16] Add clarifying comments to program dependency stack --- fvm/environment/programs.go | 38 +++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/fvm/environment/programs.go b/fvm/environment/programs.go index 023959960f5..7d3f2494400 100644 --- a/fvm/environment/programs.go +++ b/fvm/environment/programs.go @@ -94,13 +94,24 @@ func (programs *Programs) set( return err } - location, dependencies, err := programs.dependencyStack.pop() + // Get collected dependencies of the loaded program. + stackLocation, dependencies, err := programs.dependencyStack.pop() if err != nil { return err } - if location != address { - return fmt.Errorf("cannot set program. Popped dependencies are for an unexpeced address"+ - " (expected %s, got %s)", address, location) + if stackLocation != address { + // This should never happen, and indicates an implementation error. + // GetProgram and SetProgram should be always called in pair, this check depends on this assumption. + // Get pushes the stack and set pops the stack. + // Example: if loading B that depends on A (and none of them are in cache yet), + // - get(A): pushes A + // - get(B): pushes B + // - set(B): pops B + // - set(A): pops A + // Note: technically this check is redundant as `CommitParseRestricted` also has a similar check. + return fmt.Errorf( + "cannot set program. Popped dependencies are for an unexpeced address"+ + " (expected %s, got %s)", address, stackLocation) } // if the program is nil cadence is signaling that the program was not loaded, @@ -149,6 +160,11 @@ func (programs *Programs) get( return program.Program, true } + // this program is not in cache, so we need to load it into the cache. + // tho have proper invalidation, we need to track the dependencies of the program. + // If this program depends on another program, + // that program will be loaded before this one finishes loading (calls set). + // That is why this is a stack. programs.dependencyStack.push(address) // Address location program is reusable across transactions. Create @@ -234,6 +250,16 @@ func (programs *Programs) DecodeArgument( } // dependencyTracker tracks dependencies for a location +// Or in other words it builds up a list of dependencies for the program being loaded. +// If a program imports another program (A imports B), then B is a dependency of A. +// Assuming that if A imports B which imports C (already in cache), the loading process looks like this: +// - get(A): not in cache, so push A to tracker to start tracking dependencies for A. +// We can be assured that set(A) will eventually be called. +// - get(B): not in cache, push B +// - get(C): in cache, do no push C, just add C's dependencies to the tracker (C's dependencies are also in the cache) +// - set(B): pop B, getting all the collected dependencies for B, and add B's dependencies to the tracker +// (because A also depends on everything B depends on) +// - set(A): pop A, getting all the collected dependencies for A type dependencyTracker struct { location common.AddressLocation dependencies derived.ProgramDependencies @@ -252,6 +278,7 @@ func newDependencyStack() *dependencyStack { } // push a new location to track dependencies for. +// it is assumed that the dependencies will be loaded before the program is set and pop is called. func (s *dependencyStack) push(loc common.AddressLocation) { dependencies := make(derived.ProgramDependencies, 1) @@ -290,6 +317,9 @@ func (s *dependencyStack) pop() (common.AddressLocation, derived.ProgramDependen // there are more trackers in the stack. // add the dependencies of the popped tracker to the parent tracker + // This is an optimisation to avoid having to iterate through the entire stack + // everytime a dependency is pushed or added, instead we add the popped dependencies to the new top of the stack. + // (because if C depends on B which depends on A, A's dependencies include C). if len(s.trackers) > 0 { s.trackers[len(s.trackers)-1].dependencies.Merge(tracker.dependencies) } From 9cc62617f059e71d56618fe4b9a387088637a099 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Wed, 1 Feb 2023 18:27:01 +0100 Subject: [PATCH 08/16] Add cached programs metrics --- .../computation/computer/computer.go | 4 +- .../computation/computer/computer_test.go | 53 +++++++++++++++++-- fvm/derived/derived_block_data.go | 7 +++ fvm/environment/programs_test.go | 11 +++- module/metrics.go | 3 ++ module/metrics/execution.go | 13 +++++ module/metrics/noop.go | 1 + module/mock/execution_metrics.go | 5 ++ 8 files changed, 91 insertions(+), 6 deletions(-) diff --git a/engine/execution/computation/computer/computer.go b/engine/execution/computation/computer/computer.go index 24666dc94eb..83fcb44c392 100644 --- a/engine/execution/computation/computer/computer.go +++ b/engine/execution/computation/computer/computer.go @@ -187,8 +187,6 @@ func (e *blockComputer) ExecuteBlock( return nil, fmt.Errorf("failed to execute transactions: %w", err) } - // TODO: compute block fees & reward payments - return results, nil } @@ -315,6 +313,8 @@ func (e *blockComputer) executeBlock( Hex("block_id", logging.Entity(block)). Msg("all views committed") + e.metrics.ExecutionBlockCachedPrograms(derivedBlockData.CachedPrograms()) + executionDataID, err := e.executionDataProvider.Provide( ctx, block.Height(), diff --git a/engine/execution/computation/computer/computer_test.go b/engine/execution/computation/computer/computer_test.go index 0bca3fbf65b..b615c799731 100644 --- a/engine/execution/computation/computer/computer_test.go +++ b/engine/execution/computation/computer/computer_test.go @@ -60,16 +60,20 @@ func TestBlockExecutor_ExecuteBlock(t *testing.T) { t.Run("single collection", func(t *testing.T) { - execCtx := fvm.NewContext() + execCtx := fvm.NewContext( + fvm.WithDerivedBlockData(derived.NewEmptyDerivedBlockData()), + ) vm := new(computermock.VirtualMachine) vm.On("Run", mock.Anything, mock.Anything, mock.Anything). Return(nil). Run(func(args mock.Arguments) { - // ctx := args[0].(fvm.Context) + ctx := args[0].(fvm.Context) tx := args[1].(*fvm.TransactionProcedure) tx.Events = generateEvents(1, tx.TxIndex) + + getSetAProgram(t, ctx.DerivedBlockData) }). Times(2 + 1) // 2 txs in collection + system chunk @@ -96,6 +100,13 @@ func TestBlockExecutor_ExecuteBlock(t *testing.T) { Return(nil). Times(2 + 1) // 2 txs in collection + system chunk tx + expectedProgramsInCache := 1 // we set one program in the cache + exemetrics.On( + "ExecutionBlockCachedPrograms", + expectedProgramsInCache). + Return(nil). + Times(1) // 1 block + bservice := requesterunit.MockBlobService(blockstore.NewBlockstore(dssync.MutexWrap(datastore.NewMapDatastore()))) trackerStorage := new(mocktracker.Storage) trackerStorage.On("Update", mock.Anything).Return(func(fn tracker.UpdateFn) error { @@ -948,9 +959,12 @@ func Test_ExecutingSystemCollection(t *testing.T) { noopCollector := metrics.NewNoopCollector() - metrics := new(modulemock.ExecutionMetrics) expectedNumberOfEvents := 2 expectedEventSize := 911 + // bootstrapping does not cache programs + expectedCachedPrograms := 0 + + metrics := new(modulemock.ExecutionMetrics) metrics.On("ExecutionCollectionExecuted", mock.Anything, // duration mock.Anything). // stats @@ -968,6 +982,12 @@ func Test_ExecutingSystemCollection(t *testing.T) { Return(nil). Times(1) // system chunk tx + metrics.On( + "ExecutionBlockCachedPrograms", + expectedCachedPrograms). + Return(nil). + Times(1) // block + bservice := requesterunit.MockBlobService(blockstore.NewBlockstore(dssync.MutexWrap(datastore.NewMapDatastore()))) trackerStorage := new(mocktracker.Storage) trackerStorage.On("Update", mock.Anything).Return(func(fn tracker.UpdateFn) error { @@ -1100,3 +1120,30 @@ func generateEvents(eventCount int, txIndex uint32) []flow.Event { } return events } + +func getSetAProgram(t *testing.T, derivedBlockData *derived.DerivedBlockData) { + + derivedTxnData, err := derivedBlockData.NewDerivedTransactionData( + 0, + 0) + require.NoError(t, err) + + loc := common.AddressLocation{ + Name: "SomeContract", + Address: common.MustBytesToAddress([]byte{0x1}), + } + _, _, got := derivedTxnData.GetProgram( + loc, + ) + if got { + return + } + + derivedTxnData.SetProgram( + loc, + &derived.Program{}, + &state.State{}, + ) + err = derivedTxnData.Commit() + require.NoError(t, err) +} diff --git a/fvm/derived/derived_block_data.go b/fvm/derived/derived_block_data.go index 78ddafc5fbc..b972d155505 100644 --- a/fvm/derived/derived_block_data.go +++ b/fvm/derived/derived_block_data.go @@ -151,6 +151,13 @@ func (block *DerivedBlockData) GetProgramForTestingOnly( return block.programs.GetForTestingOnly(addressLocation) } +// CachedPrograms returns the number of programs cached. +// Note: this should only be called after calling commit, otherwise +// the count will contain invalidated entries. +func (block *DerivedBlockData) CachedPrograms() int { + return len(block.programs.items) +} + func (transaction *DerivedTransactionData) GetProgram( addressLocation common.AddressLocation, ) ( diff --git a/fvm/environment/programs_test.go b/fvm/environment/programs_test.go index ae3e44e413d..384bccde886 100644 --- a/fvm/environment/programs_test.go +++ b/fvm/environment/programs_test.go @@ -211,6 +211,8 @@ func Test_Programs(t *testing.T) { entry := derivedBlockData.GetProgramForTestingOnly(contractALocation) require.NotNil(t, entry) + cached := derivedBlockData.CachedPrograms() + require.Equal(t, 1, cached) // assert dependencies are correct require.Len(t, entry.Value.Dependencies, 1) @@ -257,7 +259,6 @@ func Test_Programs(t *testing.T) { }) t.Run("deploying another contract invalidates dependant programs", func(t *testing.T) { - // deploy contract B procContractB := fvm.Transaction( contractDeployTx("B", contractBCode, addressB), @@ -274,6 +275,9 @@ func Test_Programs(t *testing.T) { require.Nil(t, entryB) require.Nil(t, entryC) require.NotNil(t, entryA) + + cached := derivedBlockData.CachedPrograms() + require.Equal(t, 1, cached) }) var viewExecB *delta.View @@ -409,6 +413,8 @@ func Test_Programs(t *testing.T) { require.NotNil(t, entryB) require.Nil(t, entryC) + cached := derivedBlockData.CachedPrograms() + require.Equal(t, 2, cached) }) t.Run("importing C should chain-import B and A", func(t *testing.T) { @@ -448,6 +454,9 @@ func Test_Programs(t *testing.T) { require.NotNil(t, entryC.Value.Dependencies[common.MustBytesToAddress(addressA.Bytes())]) require.NotNil(t, entryC.Value.Dependencies[common.MustBytesToAddress(addressB.Bytes())]) require.NotNil(t, entryC.Value.Dependencies[common.MustBytesToAddress(addressC.Bytes())]) + + cached := derivedBlockData.CachedPrograms() + require.Equal(t, 3, cached) }) } diff --git a/module/metrics.go b/module/metrics.go index a976b1adbc9..4b549dffdf2 100644 --- a/module/metrics.go +++ b/module/metrics.go @@ -557,6 +557,9 @@ type ExecutionMetrics interface { // ExecutionBlockExecutionEffortVectorComponent reports the unweighted effort of given ComputationKind at block level ExecutionBlockExecutionEffortVectorComponent(string, uint) + // ExecutionBlockCachedPrograms reports the number of cached programs at the end of a block + ExecutionBlockCachedPrograms(programs int) + // ExecutionCollectionExecuted reports the total time and computation spent on executing a collection ExecutionCollectionExecuted(dur time.Duration, stats ExecutionResultStats) diff --git a/module/metrics/execution.go b/module/metrics/execution.go index 3ed3bfe6e19..03a962509ef 100644 --- a/module/metrics/execution.go +++ b/module/metrics/execution.go @@ -39,6 +39,7 @@ type ExecutionCollector struct { readDurationPerValue prometheus.Histogram blockComputationUsed prometheus.Histogram blockComputationVector *prometheus.GaugeVec + blockCachedPrograms prometheus.Gauge blockMemoryUsed prometheus.Histogram blockEventCounts prometheus.Histogram blockEventSize prometheus.Histogram @@ -254,6 +255,13 @@ func NewExecutionCollector(tracer module.Tracer) *ExecutionCollector { Help: "execution effort vector of the last executed block by computation kind", }, []string{LabelComputationKind}) + blockCachedPrograms := promauto.NewGauge(prometheus.GaugeOpts{ + Namespace: namespaceExecution, + Subsystem: subsystemRuntime, + Name: "block_execution_cached_programs", + Help: "Number of cached programs at the end of block execution", + }) + blockTransactionCounts := promauto.NewHistogram(prometheus.HistogramOpts{ Namespace: namespaceExecution, Subsystem: subsystemRuntime, @@ -543,6 +551,7 @@ func NewExecutionCollector(tracer module.Tracer) *ExecutionCollector { blockExecutionTime: blockExecutionTime, blockComputationUsed: blockComputationUsed, blockComputationVector: blockComputationVector, + blockCachedPrograms: blockCachedPrograms, blockMemoryUsed: blockMemoryUsed, blockEventCounts: blockEventCounts, blockEventSize: blockEventSize, @@ -706,6 +715,10 @@ func (ec *ExecutionCollector) ExecutionBlockExecutionEffortVectorComponent(compK ec.blockComputationVector.With(prometheus.Labels{LabelComputationKind: compKind}).Set(float64(value)) } +func (ec *ExecutionCollector) ExecutionBlockCachedPrograms(programs int) { + ec.blockCachedPrograms.Set(float64(programs)) +} + // TransactionExecuted reports stats for executing a transaction func (ec *ExecutionCollector) ExecutionTransactionExecuted( dur time.Duration, diff --git a/module/metrics/noop.go b/module/metrics/noop.go index 51dd9869b59..fbd10a28532 100644 --- a/module/metrics/noop.go +++ b/module/metrics/noop.go @@ -152,6 +152,7 @@ func (nc *NoopCollector) ExecutionBlockExecuted(_ time.Duration, _ module.Execut func (nc *NoopCollector) ExecutionCollectionExecuted(_ time.Duration, _ module.ExecutionResultStats) { } func (nc *NoopCollector) ExecutionBlockExecutionEffortVectorComponent(_ string, _ uint) {} +func (ec *NoopCollector) ExecutionBlockCachedPrograms(programs int) {} func (nc *NoopCollector) ExecutionTransactionExecuted(_ time.Duration, _, _, _ uint64, _, _ int, _ bool) { } func (nc *NoopCollector) ExecutionChunkDataPackGenerated(_, _ int) {} diff --git a/module/mock/execution_metrics.go b/module/mock/execution_metrics.go index 657f97f16d9..b9fcb7d34e0 100644 --- a/module/mock/execution_metrics.go +++ b/module/mock/execution_metrics.go @@ -21,6 +21,11 @@ func (_m *ExecutionMetrics) ChunkDataPackRequestProcessed() { _m.Called() } +// ExecutionBlockCachedPrograms provides a mock function with given fields: programs +func (_m *ExecutionMetrics) ExecutionBlockCachedPrograms(programs int) { + _m.Called(programs) +} + // ExecutionBlockDataUploadFinished provides a mock function with given fields: dur func (_m *ExecutionMetrics) ExecutionBlockDataUploadFinished(dur time.Duration) { _m.Called(dur) From 52434ed474dd8b943f392d357032279d7528a82d Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Thu, 2 Feb 2023 16:29:56 +0100 Subject: [PATCH 09/16] Add programs chache hit/miss metrics --- fvm/environment/facade_env.go | 1 + fvm/environment/mock/metrics_reporter.go | 10 ++++++++++ fvm/environment/program_logger.go | 8 ++++++++ fvm/environment/programs.go | 18 ++++++++++++++++-- module/metrics.go | 14 +++++++++++--- module/metrics/execution.go | 24 ++++++++++++++++++++++++ module/metrics/noop.go | 2 ++ module/mock/execution_metrics.go | 10 ++++++++++ module/mock/runtime_metrics.go | 10 ++++++++++ 9 files changed, 92 insertions(+), 5 deletions(-) diff --git a/fvm/environment/facade_env.go b/fvm/environment/facade_env.go index f839698bea4..379efe900eb 100644 --- a/fvm/environment/facade_env.go +++ b/fvm/environment/facade_env.go @@ -127,6 +127,7 @@ func newFacadeEnvironment( Programs: NewPrograms( tracer, meter, + params.MetricsReporter, txnState, accounts, derivedTxnData), diff --git a/fvm/environment/mock/metrics_reporter.go b/fvm/environment/mock/metrics_reporter.go index 0b899927049..bacd2d86ad0 100644 --- a/fvm/environment/mock/metrics_reporter.go +++ b/fvm/environment/mock/metrics_reporter.go @@ -33,6 +33,16 @@ func (_m *MetricsReporter) RuntimeTransactionParsed(_a0 time.Duration) { _m.Called(_a0) } +// RuntimeTransactionProgramsCacheHit provides a mock function with given fields: +func (_m *MetricsReporter) RuntimeTransactionProgramsCacheHit() { + _m.Called() +} + +// RuntimeTransactionProgramsCacheMiss provides a mock function with given fields: +func (_m *MetricsReporter) RuntimeTransactionProgramsCacheMiss() { + _m.Called() +} + type mockConstructorTestingTNewMetricsReporter interface { mock.TestingT Cleanup(func()) diff --git a/fvm/environment/program_logger.go b/fvm/environment/program_logger.go index d4d1f78a344..c95ad80f46d 100644 --- a/fvm/environment/program_logger.go +++ b/fvm/environment/program_logger.go @@ -17,6 +17,8 @@ type MetricsReporter interface { RuntimeTransactionChecked(time.Duration) RuntimeTransactionInterpreted(time.Duration) RuntimeSetNumberOfAccounts(count uint64) + RuntimeTransactionProgramsCacheMiss() + RuntimeTransactionProgramsCacheHit() } // NoopMetricsReporter is a MetricReporter that does nothing. @@ -34,6 +36,12 @@ func (NoopMetricsReporter) RuntimeTransactionInterpreted(time.Duration) {} // RuntimeSetNumberOfAccounts is a noop func (NoopMetricsReporter) RuntimeSetNumberOfAccounts(count uint64) {} +// RuntimeTransactionProgramsCacheMiss is a noop +func (NoopMetricsReporter) RuntimeTransactionProgramsCacheMiss() {} + +// RuntimeTransactionProgramsCacheHit is a noop +func (NoopMetricsReporter) RuntimeTransactionProgramsCacheHit() {} + type ProgramLoggerParams struct { zerolog.Logger diff --git a/fvm/environment/programs.go b/fvm/environment/programs.go index 7d3f2494400..02d3b3161d8 100644 --- a/fvm/environment/programs.go +++ b/fvm/environment/programs.go @@ -29,8 +29,9 @@ type DerivedTransactionData interface { // these nested transactions on Set calls in order to capture the states // needed for parsing the programs. type Programs struct { - tracer *Tracer - meter Meter + tracer *Tracer + meter Meter + metrics MetricsReporter txnState *state.TransactionState accounts Accounts @@ -49,6 +50,7 @@ type Programs struct { func NewPrograms( tracer *Tracer, meter Meter, + metrics MetricsReporter, txnState *state.TransactionState, accounts Accounts, derivedTxnData DerivedTransactionData, @@ -56,6 +58,7 @@ func NewPrograms( return &Programs{ tracer: tracer, meter: meter, + metrics: metrics, txnState: txnState, accounts: accounts, derivedTxnData: derivedTxnData, @@ -149,6 +152,8 @@ func (programs *Programs) get( program, state, has := programs.derivedTxnData.GetProgram(address) if has { + programs.cacheHit() + programs.dependencyStack.addDependencies(program.Dependencies) err := programs.txnState.AttachAndCommit(state) if err != nil { @@ -159,6 +164,7 @@ func (programs *Programs) get( return program.Program, true } + programs.cacheMiss() // this program is not in cache, so we need to load it into the cache. // tho have proper invalidation, we need to track the dependencies of the program. @@ -249,6 +255,14 @@ func (programs *Programs) DecodeArgument( return v, err } +func (programs *Programs) cacheHit() { + programs.metrics.RuntimeTransactionProgramsCacheHit() +} + +func (programs *Programs) cacheMiss() { + programs.metrics.RuntimeTransactionProgramsCacheMiss() +} + // dependencyTracker tracks dependencies for a location // Or in other words it builds up a list of dependencies for the program being loaded. // If a program imports another program (A imports B), then B is a dependency of A. diff --git a/module/metrics.go b/module/metrics.go index 4b549dffdf2..292ecce0947 100644 --- a/module/metrics.go +++ b/module/metrics.go @@ -448,17 +448,25 @@ type ExecutionDataRequesterMetrics interface { } type RuntimeMetrics interface { - // TransactionParsed reports the time spent parsing a single transaction + // RuntimeTransactionParsed reports the time spent parsing a single transaction RuntimeTransactionParsed(dur time.Duration) - // TransactionChecked reports the time spent checking a single transaction + // RuntimeTransactionChecked reports the time spent checking a single transaction RuntimeTransactionChecked(dur time.Duration) - // TransactionInterpreted reports the time spent interpreting a single transaction + // RuntimeTransactionInterpreted reports the time spent interpreting a single transaction RuntimeTransactionInterpreted(dur time.Duration) // RuntimeSetNumberOfAccounts Sets the total number of accounts on the network RuntimeSetNumberOfAccounts(count uint64) + + // RuntimeTransactionProgramsCacheMiss reports a programs cache miss + // during transaction execution + RuntimeTransactionProgramsCacheMiss() + + // RuntimeTransactionProgramsCacheHit reports a programs cache hit + // during transaction execution + RuntimeTransactionProgramsCacheHit() } type ProviderMetrics interface { diff --git a/module/metrics/execution.go b/module/metrics/execution.go index 03a962509ef..2912e842472 100644 --- a/module/metrics/execution.go +++ b/module/metrics/execution.go @@ -72,6 +72,8 @@ type ExecutionCollector struct { scriptMemoryEstimate prometheus.Histogram scriptMemoryDifference prometheus.Histogram numberOfAccounts prometheus.Gauge + programsCacheMiss prometheus.Counter + programsCacheHit prometheus.Counter chunkDataPackRequestProcessedTotal prometheus.Counter chunkDataPackProofSize prometheus.Histogram chunkDataPackCollectionSize prometheus.Histogram @@ -659,6 +661,20 @@ func NewExecutionCollector(tracer module.Tracer) *ExecutionCollector { Help: "the number of existing accounts on the network", }), + programsCacheMiss: promauto.NewCounter(prometheus.CounterOpts{ + Namespace: namespaceExecution, + Subsystem: subsystemRuntime, + Name: "programs_cache_miss", + Help: "the number of times a program was not found in the cache and had to be loaded", + }), + + programsCacheHit: promauto.NewCounter(prometheus.CounterOpts{ + Namespace: namespaceExecution, + Subsystem: subsystemRuntime, + Name: "programs_cache_hit", + Help: "the number of times a program was found in the cache", + }), + maxCollectionHeight: prometheus.NewGauge(prometheus.GaugeOpts{ Name: "max_collection_height", Namespace: namespaceExecution, @@ -905,6 +921,14 @@ func (ec *ExecutionCollector) RuntimeSetNumberOfAccounts(count uint64) { ec.numberOfAccounts.Set(float64(count)) } +func (ec *ExecutionCollector) RuntimeTransactionProgramsCacheMiss() { + ec.programsCacheMiss.Inc() +} + +func (ec *ExecutionCollector) RuntimeTransactionProgramsCacheHit() { + ec.programsCacheHit.Inc() +} + func (ec *ExecutionCollector) UpdateCollectionMaxHeight(height uint64) { ec.maxCollectionHeight.Set(float64(height)) } diff --git a/module/metrics/noop.go b/module/metrics/noop.go index fbd10a28532..ac8e75436e1 100644 --- a/module/metrics/noop.go +++ b/module/metrics/noop.go @@ -180,6 +180,8 @@ func (nc *NoopCollector) RuntimeTransactionParsed(dur time.Duration) func (nc *NoopCollector) RuntimeTransactionChecked(dur time.Duration) {} func (nc *NoopCollector) RuntimeTransactionInterpreted(dur time.Duration) {} func (nc *NoopCollector) RuntimeSetNumberOfAccounts(count uint64) {} +func (nc *NoopCollector) RuntimeTransactionProgramsCacheMiss() {} +func (nc *NoopCollector) RuntimeTransactionProgramsCacheHit() {} func (nc *NoopCollector) ScriptExecuted(dur time.Duration, size int) {} func (nc *NoopCollector) TransactionResultFetched(dur time.Duration, size int) {} func (nc *NoopCollector) TransactionReceived(txID flow.Identifier, when time.Time) {} diff --git a/module/mock/execution_metrics.go b/module/mock/execution_metrics.go index b9fcb7d34e0..f9731b7fa93 100644 --- a/module/mock/execution_metrics.go +++ b/module/mock/execution_metrics.go @@ -186,6 +186,16 @@ func (_m *ExecutionMetrics) RuntimeTransactionParsed(dur time.Duration) { _m.Called(dur) } +// RuntimeTransactionProgramsCacheHit provides a mock function with given fields: +func (_m *ExecutionMetrics) RuntimeTransactionProgramsCacheHit() { + _m.Called() +} + +// RuntimeTransactionProgramsCacheMiss provides a mock function with given fields: +func (_m *ExecutionMetrics) RuntimeTransactionProgramsCacheMiss() { + _m.Called() +} + // StartBlockReceivedToExecuted provides a mock function with given fields: blockID func (_m *ExecutionMetrics) StartBlockReceivedToExecuted(blockID flow.Identifier) { _m.Called(blockID) diff --git a/module/mock/runtime_metrics.go b/module/mock/runtime_metrics.go index 84aee22d34c..5168f446845 100644 --- a/module/mock/runtime_metrics.go +++ b/module/mock/runtime_metrics.go @@ -33,6 +33,16 @@ func (_m *RuntimeMetrics) RuntimeTransactionParsed(dur time.Duration) { _m.Called(dur) } +// RuntimeTransactionProgramsCacheHit provides a mock function with given fields: +func (_m *RuntimeMetrics) RuntimeTransactionProgramsCacheHit() { + _m.Called() +} + +// RuntimeTransactionProgramsCacheMiss provides a mock function with given fields: +func (_m *RuntimeMetrics) RuntimeTransactionProgramsCacheMiss() { + _m.Called() +} + type mockConstructorTestingTNewRuntimeMetrics interface { mock.TestingT Cleanup(func()) From be559765d5cac174793e4656318f0b9533b749a8 Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Thu, 2 Mar 2023 19:21:43 +0100 Subject: [PATCH 10/16] disable freezing --- fvm/environment/accounts_status.go | 4 +++- fvm/environment/accounts_status_test.go | 8 +++----- fvm/transactionVerifier_test.go | 3 +++ fvm/transaction_test.go | 3 ++- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/fvm/environment/accounts_status.go b/fvm/environment/accounts_status.go index ab161eabec1..e2d0dc1172e 100644 --- a/fvm/environment/accounts_status.go +++ b/fvm/environment/accounts_status.go @@ -71,7 +71,9 @@ func AccountStatusFromBytes(inp []byte) (*AccountStatus, error) { // IsAccountFrozen returns true if account's frozen flag is set func (a *AccountStatus) IsAccountFrozen() bool { - return a[flagIndex]&maskFrozen > 0 + // accounts are never frozen + // TODO: remove the freezing feature entirely + return false } // SetFrozenFlag sets the frozen flag diff --git a/fvm/environment/accounts_status_test.go b/fvm/environment/accounts_status_test.go index 7d81ad0a3f4..dd21ff29527 100644 --- a/fvm/environment/accounts_status_test.go +++ b/fvm/environment/accounts_status_test.go @@ -16,6 +16,9 @@ func TestAccountStatus(t *testing.T) { require.False(t, s.IsAccountFrozen()) t.Run("test frozen flag set/reset", func(t *testing.T) { + // TODO: remove freezing feature + t.Skip("Skip as we are removing the freezing feature.") + s.SetFrozenFlag(true) require.True(t, s.IsAccountFrozen()) @@ -24,9 +27,6 @@ func TestAccountStatus(t *testing.T) { }) t.Run("test setting values", func(t *testing.T) { - // set some values for side effect checks - s.SetFrozenFlag(true) - index := atree.StorageIndex{1, 2, 3, 4, 5, 6, 7, 8} s.SetStorageIndex(index) s.SetPublicKeyCount(34) @@ -37,8 +37,6 @@ func TestAccountStatus(t *testing.T) { require.True(t, bytes.Equal(index[:], returnedIndex[:])) require.Equal(t, uint64(34), s.PublicKeyCount()) - // check no side effect on flags - require.True(t, s.IsAccountFrozen()) }) t.Run("test serialization", func(t *testing.T) { diff --git a/fvm/transactionVerifier_test.go b/fvm/transactionVerifier_test.go index b3983865bcd..07556af440b 100644 --- a/fvm/transactionVerifier_test.go +++ b/fvm/transactionVerifier_test.go @@ -207,6 +207,9 @@ func TestTransactionVerification(t *testing.T) { }) t.Run("frozen account is rejected", func(t *testing.T) { + // TODO: remove freezing feature + t.Skip("Skip as we are removing the freezing feature.") + ctx := fvm.NewContext( fvm.WithAuthorizationChecksEnabled(true), fvm.WithAccountKeyWeightThreshold(-1), diff --git a/fvm/transaction_test.go b/fvm/transaction_test.go index c201da90fc9..8a2fa0fb7a0 100644 --- a/fvm/transaction_test.go +++ b/fvm/transaction_test.go @@ -50,12 +50,13 @@ func makeTwoAccounts( } func TestAccountFreezing(t *testing.T) { + // TODO: remove freezing feature + t.Skip("Skip as we are removing the freezing feature.") chain := flow.Mainnet.Chain() serviceAddress := chain.ServiceAddress() t.Run("setFrozenAccount can be enabled", func(t *testing.T) { - address, _, st := makeTwoAccounts(t, nil, nil) accounts := environment.NewAccounts(st) derivedBlockData := derived.NewEmptyDerivedBlockData() From ad73680e3dc4615c18b47dc2846699b9f8f57ba6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Mon, 6 Mar 2023 08:09:51 -0800 Subject: [PATCH 11/16] update Cadence to v0.31.5-account-link-improvements --- go.mod | 2 +- go.sum | 4 ++-- insecure/go.mod | 2 +- insecure/go.sum | 4 ++-- integration/go.mod | 2 +- integration/go.sum | 4 ++-- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 9ed6534426f..6592908e843 100644 --- a/go.mod +++ b/go.mod @@ -57,7 +57,7 @@ require ( github.com/multiformats/go-multiaddr-dns v0.3.1 github.com/multiformats/go-multihash v0.2.1 github.com/onflow/atree v0.4.0 - github.com/onflow/cadence v0.31.5-account-link-pragma + github.com/onflow/cadence v0.31.5-account-link-improvements github.com/onflow/flow v0.3.2 github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 diff --git a/go.sum b/go.sum index d6c64a937f7..312cb7df3fc 100644 --- a/go.sum +++ b/go.sum @@ -1223,8 +1223,8 @@ github.com/olekukonko/tablewriter v0.0.1/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXW github.com/olekukonko/tablewriter v0.0.2-0.20190409134802-7e037d187b0c/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo= github.com/onflow/atree v0.4.0 h1:+TbNisavAkukAKhgQ4plWnvR9o5+SkwPIsi3jaeAqKs= github.com/onflow/atree v0.4.0/go.mod h1:7Qe1xaW0YewvouLXrugzMFUYXNoRQ8MT/UsVAWx1Ndo= -github.com/onflow/cadence v0.31.5-account-link-pragma h1:e4ldX3uNdDJ1o5XNJI996vPYxoP2WtDbDaA2F1nzbXk= -github.com/onflow/cadence v0.31.5-account-link-pragma/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= +github.com/onflow/cadence v0.31.5-account-link-improvements h1:41H9DwfKV7e6n76FhKl+6YF0IY6P6KprM+5CDE58Stc= +github.com/onflow/cadence v0.31.5-account-link-improvements/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= github.com/onflow/flow v0.3.2 h1:z3IuKOjM9Tvf0pXfloTbrLxM5nTunI47cklsDd+wxBE= github.com/onflow/flow v0.3.2/go.mod h1:lzyAYmbu1HfkZ9cfnL5/sjrrsnJiUU8fRL26CqLP7+c= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 h1:fH5e7L9xFXNOd3WLvMaPNkP1m7BngRTDP751zMNndws= diff --git a/insecure/go.mod b/insecure/go.mod index 4565e6ea55b..ee0599d02da 100644 --- a/insecure/go.mod +++ b/insecure/go.mod @@ -176,7 +176,7 @@ require ( github.com/multiformats/go-varint v0.0.6 // indirect github.com/nxadm/tail v1.4.8 // indirect github.com/onflow/atree v0.4.0 // indirect - github.com/onflow/cadence v0.31.5-account-link-pragma // indirect + github.com/onflow/cadence v0.31.5-account-link-improvements // indirect github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 // indirect github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 // indirect github.com/onflow/flow-ft/lib/go/contracts v0.5.0 // indirect diff --git a/insecure/go.sum b/insecure/go.sum index 7d9cd681d9c..fdbbfb5e1d2 100644 --- a/insecure/go.sum +++ b/insecure/go.sum @@ -1182,8 +1182,8 @@ github.com/olekukonko/tablewriter v0.0.1/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXW github.com/olekukonko/tablewriter v0.0.2-0.20190409134802-7e037d187b0c/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo= github.com/onflow/atree v0.4.0 h1:+TbNisavAkukAKhgQ4plWnvR9o5+SkwPIsi3jaeAqKs= github.com/onflow/atree v0.4.0/go.mod h1:7Qe1xaW0YewvouLXrugzMFUYXNoRQ8MT/UsVAWx1Ndo= -github.com/onflow/cadence v0.31.5-account-link-pragma h1:e4ldX3uNdDJ1o5XNJI996vPYxoP2WtDbDaA2F1nzbXk= -github.com/onflow/cadence v0.31.5-account-link-pragma/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= +github.com/onflow/cadence v0.31.5-account-link-improvements h1:41H9DwfKV7e6n76FhKl+6YF0IY6P6KprM+5CDE58Stc= +github.com/onflow/cadence v0.31.5-account-link-improvements/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 h1:fH5e7L9xFXNOd3WLvMaPNkP1m7BngRTDP751zMNndws= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722/go.mod h1:9nrgjIF/noY2jJ7LP8bKLHTpcdHOa2yO0ryCKTQpxvs= github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 h1:vgNS6I+MM/74pciIoKb7ZBs8XGF1ONsSdkAec36B9iU= diff --git a/integration/go.mod b/integration/go.mod index 5beb78bc5e3..0878432ab99 100644 --- a/integration/go.mod +++ b/integration/go.mod @@ -16,7 +16,7 @@ require ( github.com/ipfs/go-datastore v0.6.0 github.com/ipfs/go-ds-badger2 v0.1.3 github.com/ipfs/go-ipfs-blockstore v1.2.0 - github.com/onflow/cadence v0.31.5-account-link-pragma + github.com/onflow/cadence v0.31.5-account-link-improvements github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 github.com/onflow/flow-emulator v0.38.1 diff --git a/integration/go.sum b/integration/go.sum index 70a32f4bc0a..97a7455461a 100644 --- a/integration/go.sum +++ b/integration/go.sum @@ -1304,8 +1304,8 @@ github.com/olekukonko/tablewriter v0.0.1/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXW github.com/olekukonko/tablewriter v0.0.2-0.20190409134802-7e037d187b0c/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo= github.com/onflow/atree v0.4.0 h1:+TbNisavAkukAKhgQ4plWnvR9o5+SkwPIsi3jaeAqKs= github.com/onflow/atree v0.4.0/go.mod h1:7Qe1xaW0YewvouLXrugzMFUYXNoRQ8MT/UsVAWx1Ndo= -github.com/onflow/cadence v0.31.5-account-link-pragma h1:e4ldX3uNdDJ1o5XNJI996vPYxoP2WtDbDaA2F1nzbXk= -github.com/onflow/cadence v0.31.5-account-link-pragma/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= +github.com/onflow/cadence v0.31.5-account-link-improvements h1:41H9DwfKV7e6n76FhKl+6YF0IY6P6KprM+5CDE58Stc= +github.com/onflow/cadence v0.31.5-account-link-improvements/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 h1:fH5e7L9xFXNOd3WLvMaPNkP1m7BngRTDP751zMNndws= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722/go.mod h1:9nrgjIF/noY2jJ7LP8bKLHTpcdHOa2yO0ryCKTQpxvs= github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 h1:vgNS6I+MM/74pciIoKb7ZBs8XGF1ONsSdkAec36B9iU= From 61f52134d86c289d873090ec3aeff9dc95cf558d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Mon, 6 Mar 2023 08:11:57 -0800 Subject: [PATCH 12/16] account links must be private now --- fvm/fvm_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fvm/fvm_test.go b/fvm/fvm_test.go index aeb53113a76..03b3ec699b3 100644 --- a/fvm/fvm_test.go +++ b/fvm/fvm_test.go @@ -2264,7 +2264,7 @@ func TestAuthAccountCapabilities(t *testing.T) { transaction { prepare(acct: AuthAccount) { - acct.linkAccount(/public/foo) + acct.linkAccount(/private/foo) } } `, From 725f30d06af1d1d35558f4d7ab8ac2f6b7cdf5d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Mon, 13 Mar 2023 16:17:46 -0700 Subject: [PATCH 13/16] update to Cadence v0.31.5-account-linking-improved-pragma --- go.mod | 2 +- go.sum | 4 ++-- insecure/go.mod | 2 +- insecure/go.sum | 4 ++-- integration/go.mod | 2 +- integration/go.sum | 4 ++-- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 6592908e843..41ed49b6627 100644 --- a/go.mod +++ b/go.mod @@ -57,7 +57,7 @@ require ( github.com/multiformats/go-multiaddr-dns v0.3.1 github.com/multiformats/go-multihash v0.2.1 github.com/onflow/atree v0.4.0 - github.com/onflow/cadence v0.31.5-account-link-improvements + github.com/onflow/cadence v0.31.5-account-linking-improved-pragma github.com/onflow/flow v0.3.2 github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 diff --git a/go.sum b/go.sum index 312cb7df3fc..5e015795bfd 100644 --- a/go.sum +++ b/go.sum @@ -1223,8 +1223,8 @@ github.com/olekukonko/tablewriter v0.0.1/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXW github.com/olekukonko/tablewriter v0.0.2-0.20190409134802-7e037d187b0c/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo= github.com/onflow/atree v0.4.0 h1:+TbNisavAkukAKhgQ4plWnvR9o5+SkwPIsi3jaeAqKs= github.com/onflow/atree v0.4.0/go.mod h1:7Qe1xaW0YewvouLXrugzMFUYXNoRQ8MT/UsVAWx1Ndo= -github.com/onflow/cadence v0.31.5-account-link-improvements h1:41H9DwfKV7e6n76FhKl+6YF0IY6P6KprM+5CDE58Stc= -github.com/onflow/cadence v0.31.5-account-link-improvements/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= +github.com/onflow/cadence v0.31.5-account-linking-improved-pragma h1:XuOLqgHUy3HSNeRjxWsRbooYo4+EobStXhEm6HZ0uKg= +github.com/onflow/cadence v0.31.5-account-linking-improved-pragma/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= github.com/onflow/flow v0.3.2 h1:z3IuKOjM9Tvf0pXfloTbrLxM5nTunI47cklsDd+wxBE= github.com/onflow/flow v0.3.2/go.mod h1:lzyAYmbu1HfkZ9cfnL5/sjrrsnJiUU8fRL26CqLP7+c= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 h1:fH5e7L9xFXNOd3WLvMaPNkP1m7BngRTDP751zMNndws= diff --git a/insecure/go.mod b/insecure/go.mod index ee0599d02da..4e645d6b056 100644 --- a/insecure/go.mod +++ b/insecure/go.mod @@ -176,7 +176,7 @@ require ( github.com/multiformats/go-varint v0.0.6 // indirect github.com/nxadm/tail v1.4.8 // indirect github.com/onflow/atree v0.4.0 // indirect - github.com/onflow/cadence v0.31.5-account-link-improvements // indirect + github.com/onflow/cadence v0.31.5-account-linking-improved-pragma // indirect github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 // indirect github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 // indirect github.com/onflow/flow-ft/lib/go/contracts v0.5.0 // indirect diff --git a/insecure/go.sum b/insecure/go.sum index fdbbfb5e1d2..84f05d8f9d4 100644 --- a/insecure/go.sum +++ b/insecure/go.sum @@ -1182,8 +1182,8 @@ github.com/olekukonko/tablewriter v0.0.1/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXW github.com/olekukonko/tablewriter v0.0.2-0.20190409134802-7e037d187b0c/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo= github.com/onflow/atree v0.4.0 h1:+TbNisavAkukAKhgQ4plWnvR9o5+SkwPIsi3jaeAqKs= github.com/onflow/atree v0.4.0/go.mod h1:7Qe1xaW0YewvouLXrugzMFUYXNoRQ8MT/UsVAWx1Ndo= -github.com/onflow/cadence v0.31.5-account-link-improvements h1:41H9DwfKV7e6n76FhKl+6YF0IY6P6KprM+5CDE58Stc= -github.com/onflow/cadence v0.31.5-account-link-improvements/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= +github.com/onflow/cadence v0.31.5-account-linking-improved-pragma h1:XuOLqgHUy3HSNeRjxWsRbooYo4+EobStXhEm6HZ0uKg= +github.com/onflow/cadence v0.31.5-account-linking-improved-pragma/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 h1:fH5e7L9xFXNOd3WLvMaPNkP1m7BngRTDP751zMNndws= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722/go.mod h1:9nrgjIF/noY2jJ7LP8bKLHTpcdHOa2yO0ryCKTQpxvs= github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 h1:vgNS6I+MM/74pciIoKb7ZBs8XGF1ONsSdkAec36B9iU= diff --git a/integration/go.mod b/integration/go.mod index 0878432ab99..9fd1c98977c 100644 --- a/integration/go.mod +++ b/integration/go.mod @@ -16,7 +16,7 @@ require ( github.com/ipfs/go-datastore v0.6.0 github.com/ipfs/go-ds-badger2 v0.1.3 github.com/ipfs/go-ipfs-blockstore v1.2.0 - github.com/onflow/cadence v0.31.5-account-link-improvements + github.com/onflow/cadence v0.31.5-account-linking-improved-pragma github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 github.com/onflow/flow-emulator v0.38.1 diff --git a/integration/go.sum b/integration/go.sum index 97a7455461a..384a5207607 100644 --- a/integration/go.sum +++ b/integration/go.sum @@ -1304,8 +1304,8 @@ github.com/olekukonko/tablewriter v0.0.1/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXW github.com/olekukonko/tablewriter v0.0.2-0.20190409134802-7e037d187b0c/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo= github.com/onflow/atree v0.4.0 h1:+TbNisavAkukAKhgQ4plWnvR9o5+SkwPIsi3jaeAqKs= github.com/onflow/atree v0.4.0/go.mod h1:7Qe1xaW0YewvouLXrugzMFUYXNoRQ8MT/UsVAWx1Ndo= -github.com/onflow/cadence v0.31.5-account-link-improvements h1:41H9DwfKV7e6n76FhKl+6YF0IY6P6KprM+5CDE58Stc= -github.com/onflow/cadence v0.31.5-account-link-improvements/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= +github.com/onflow/cadence v0.31.5-account-linking-improved-pragma h1:XuOLqgHUy3HSNeRjxWsRbooYo4+EobStXhEm6HZ0uKg= +github.com/onflow/cadence v0.31.5-account-linking-improved-pragma/go.mod h1:oRgWkvau1RH15m3NuDlZCPHFQzwvC72jEstCGu8OJ98= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722 h1:fH5e7L9xFXNOd3WLvMaPNkP1m7BngRTDP751zMNndws= github.com/onflow/flow-core-contracts/lib/go/contracts v0.11.2-0.20221216161720-c1b31d5a4722/go.mod h1:9nrgjIF/noY2jJ7LP8bKLHTpcdHOa2yO0ryCKTQpxvs= github.com/onflow/flow-core-contracts/lib/go/templates v0.11.2-0.20221216161720-c1b31d5a4722 h1:vgNS6I+MM/74pciIoKb7ZBs8XGF1ONsSdkAec36B9iU= From cf99dd3c20ba73050bd51ed286e8fdfa443c6512 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Mon, 13 Mar 2023 16:35:24 -0700 Subject: [PATCH 14/16] add test for account linking in contract --- fvm/fvm_test.go | 264 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 198 insertions(+), 66 deletions(-) diff --git a/fvm/fvm_test.go b/fvm/fvm_test.go index 03b3ec699b3..7ac6f933b63 100644 --- a/fvm/fvm_test.go +++ b/fvm/fvm_test.go @@ -2221,82 +2221,214 @@ func TestScriptIterationShouldNotHitsParseRestrictions(t *testing.T) { } func TestAuthAccountCapabilities(t *testing.T) { - test := func(t *testing.T, allowAccountLinking bool) { - newVMTest(). - withBootstrapProcedureOptions(). - withContextOptions( - fvm.WithReusableCadenceRuntimePool( - reusableRuntime.NewReusableCadenceRuntimePool( - 1, - runtime.Config{ - AccountLinkingEnabled: true, - }, + + t.Parallel() + + t.Run("transaction", func(t *testing.T) { + + t.Parallel() + + test := func(t *testing.T, allowAccountLinking bool) { + newVMTest(). + withBootstrapProcedureOptions(). + withContextOptions( + fvm.WithReusableCadenceRuntimePool( + reusableRuntime.NewReusableCadenceRuntimePool( + 1, + runtime.Config{ + AccountLinkingEnabled: true, + }, + ), ), - ), - ). - run( - func( - t *testing.T, - vm *fvm.VirtualMachine, - chain flow.Chain, - ctx fvm.Context, - view state.View, - derivedBlockData *derived.DerivedBlockData, - ) { - // Create an account private key. - privateKeys, err := testutil.GenerateAccountPrivateKeys(1) - privateKey := privateKeys[0] - require.NoError(t, err) + ). + run( + func( + t *testing.T, + vm *fvm.VirtualMachine, + chain flow.Chain, + ctx fvm.Context, + view state.View, + derivedBlockData *derived.DerivedBlockData, + ) { + // Create an account private key. + privateKeys, err := testutil.GenerateAccountPrivateKeys(1) + privateKey := privateKeys[0] + require.NoError(t, err) + + // Bootstrap a ledger, creating accounts with the provided private keys and the root account. + accounts, err := testutil.CreateAccounts(vm, view, derivedBlockData, privateKeys, chain) + require.NoError(t, err) + account := accounts[0] + + var pragma string + if allowAccountLinking { + pragma = "#allowAccountLinking" + } - // Bootstrap a ledger, creating accounts with the provided private keys and the root account. - accounts, err := testutil.CreateAccounts(vm, view, derivedBlockData, privateKeys, chain) - require.NoError(t, err) - account := accounts[0] + code := fmt.Sprintf( + ` + %s + + transaction { + prepare(acct: AuthAccount) { + acct.linkAccount(/private/foo) + } + } + `, + pragma, + ) - var pragma string - if allowAccountLinking { - pragma = "#allowAccountLinking" - } + txBody := flow.NewTransactionBody(). + SetScript([]byte(code)). + AddAuthorizer(account). + SetPayer(chain.ServiceAddress()). + SetProposalKey(chain.ServiceAddress(), 0, 0) + + _ = testutil.SignPayload(txBody, account, privateKey) + _ = testutil.SignEnvelope(txBody, chain.ServiceAddress(), unittest.ServiceAccountPrivateKey) + tx := fvm.Transaction(txBody, derivedBlockData.NextTxIndexForTestingOnly()) + + err = vm.Run(ctx, tx, view) + require.NoError(t, err) + if allowAccountLinking { + require.NoError(t, tx.Err) + } else { + require.Error(t, tx.Err) + } + }, + )(t) + } - code := fmt.Sprintf( - ` - %s + t.Run("account linking allowed", func(t *testing.T) { + test(t, true) + }) - transaction { - prepare(acct: AuthAccount) { - acct.linkAccount(/private/foo) - } - } - `, - pragma, - ) + t.Run("account linking disallowed", func(t *testing.T) { + test(t, false) + }) + }) - txBody := flow.NewTransactionBody(). - SetScript([]byte(code)). - AddAuthorizer(account). - SetPayer(chain.ServiceAddress()). - SetProposalKey(chain.ServiceAddress(), 0, 0) + t.Run("contract", func(t *testing.T) { + + t.Parallel() + + test := func(t *testing.T, allowAccountLinking bool) { + newVMTest(). + withBootstrapProcedureOptions(). + withContextOptions( + fvm.WithReusableCadenceRuntimePool( + reusableRuntime.NewReusableCadenceRuntimePool( + 1, + runtime.Config{ + AccountLinkingEnabled: true, + }, + ), + ), + fvm.WithContractDeploymentRestricted(false), + ). + run( + func( + t *testing.T, + vm *fvm.VirtualMachine, + chain flow.Chain, + ctx fvm.Context, + view state.View, + derivedBlockData *derived.DerivedBlockData, + ) { + // Create two private keys + privateKeys, err := testutil.GenerateAccountPrivateKeys(2) + require.NoError(t, err) + + // Bootstrap a ledger, creating accounts with the provided private keys and the root account. + accounts, err := testutil.CreateAccounts(vm, view, derivedBlockData, privateKeys, chain) + require.NoError(t, err) + + // Deploy contract + contractCode := ` + pub contract AccountLinker { + pub fun link(_ account: AuthAccount) { + account.linkAccount(/private/acct) + } + } + ` - _ = testutil.SignPayload(txBody, account, privateKey) - _ = testutil.SignEnvelope(txBody, chain.ServiceAddress(), unittest.ServiceAccountPrivateKey) - tx := fvm.Transaction(txBody, derivedBlockData.NextTxIndexForTestingOnly()) + deployingContractScriptTemplate := ` + transaction { + prepare(signer: AuthAccount) { + signer.contracts.add( + name: "AccountLinker", + code: "%s".decodeHex() + ) + } + } + ` - err = vm.Run(ctx, tx, view) - require.NoError(t, err) - if allowAccountLinking { + txBody := flow.NewTransactionBody(). + SetScript([]byte(fmt.Sprintf( + deployingContractScriptTemplate, + hex.EncodeToString([]byte(contractCode)), + ))). + SetPayer(chain.ServiceAddress()). + SetProposalKey(chain.ServiceAddress(), 0, 0). + AddAuthorizer(accounts[0]) + _ = testutil.SignPayload(txBody, accounts[0], privateKeys[0]) + _ = testutil.SignEnvelope(txBody, chain.ServiceAddress(), unittest.ServiceAccountPrivateKey) + + tx := fvm.Transaction(txBody, derivedBlockData.NextTxIndexForTestingOnly()) + err = vm.Run(ctx, tx, view) + require.NoError(t, err) require.NoError(t, tx.Err) - } else { - require.Error(t, tx.Err) - } - }, - )(t) - } - t.Run("account linking allowed", func(t *testing.T) { - test(t, true) - }) + // Use contract + + var pragma string + if allowAccountLinking { + pragma = "#allowAccountLinking" + } + + code := fmt.Sprintf( + ` + %s - t.Run("account linking disallowed", func(t *testing.T) { - test(t, false) + import AccountLinker from %s + + transaction { + prepare(acct: AuthAccount) { + AccountLinker.link(acct) + } + } + `, + pragma, + accounts[0].HexWithPrefix(), + ) + + txBody = flow.NewTransactionBody(). + SetScript([]byte(code)). + AddAuthorizer(accounts[1]). + SetPayer(chain.ServiceAddress()). + SetProposalKey(chain.ServiceAddress(), 0, 1) + + _ = testutil.SignPayload(txBody, accounts[1], privateKeys[1]) + _ = testutil.SignEnvelope(txBody, chain.ServiceAddress(), unittest.ServiceAccountPrivateKey) + tx = fvm.Transaction(txBody, derivedBlockData.NextTxIndexForTestingOnly()) + + err = vm.Run(ctx, tx, view) + require.NoError(t, err) + if allowAccountLinking { + require.NoError(t, tx.Err) + } else { + require.Error(t, tx.Err) + } + }, + )(t) + } + + t.Run("account linking allowed", func(t *testing.T) { + test(t, true) + }) + + t.Run("account linking disallowed", func(t *testing.T) { + test(t, false) + }) }) } From 1e48f723b0a668048195d225847d7439c81fb842 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Tue, 14 Mar 2023 13:47:52 -0700 Subject: [PATCH 15/16] quarantine TestEpochJoinAndLeaveAN --- integration/tests/epochs/epoch_join_and_leave_an_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/integration/tests/epochs/epoch_join_and_leave_an_test.go b/integration/tests/epochs/epoch_join_and_leave_an_test.go index 25b96bf425a..2ad29e46523 100644 --- a/integration/tests/epochs/epoch_join_and_leave_an_test.go +++ b/integration/tests/epochs/epoch_join_and_leave_an_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/suite" "github.com/onflow/flow-go/model/flow" + "github.com/onflow/flow-go/utils/unittest" ) func TestEpochJoinAndLeaveAN(t *testing.T) { @@ -19,5 +20,6 @@ type EpochJoinAndLeaveANSuite struct { // TestEpochJoinAndLeaveAN should update access nodes and assert healthy network conditions // after the epoch transition completes. See health check function for details. func (s *EpochJoinAndLeaveANSuite) TestEpochJoinAndLeaveAN() { + unittest.SkipUnless(s.T(), unittest.TEST_FLAKY, "fails on CI regularly") s.runTestEpochJoinAndLeave(flow.RoleAccess, s.assertNetworkHealthyAfterANChange) } From 5b217fd6df80af87101f2c6cf107dba818be555b Mon Sep 17 00:00:00 2001 From: Janez Podhostnik Date: Fri, 17 Mar 2023 12:19:57 +0100 Subject: [PATCH 16/16] Revert "disable freezing" This reverts commit be559765d5cac174793e4656318f0b9533b749a8. --- fvm/environment/accounts_status.go | 4 +--- fvm/environment/accounts_status_test.go | 8 +++++--- fvm/transactionVerifier_test.go | 3 --- fvm/transaction_test.go | 3 +-- 4 files changed, 7 insertions(+), 11 deletions(-) diff --git a/fvm/environment/accounts_status.go b/fvm/environment/accounts_status.go index e2d0dc1172e..ab161eabec1 100644 --- a/fvm/environment/accounts_status.go +++ b/fvm/environment/accounts_status.go @@ -71,9 +71,7 @@ func AccountStatusFromBytes(inp []byte) (*AccountStatus, error) { // IsAccountFrozen returns true if account's frozen flag is set func (a *AccountStatus) IsAccountFrozen() bool { - // accounts are never frozen - // TODO: remove the freezing feature entirely - return false + return a[flagIndex]&maskFrozen > 0 } // SetFrozenFlag sets the frozen flag diff --git a/fvm/environment/accounts_status_test.go b/fvm/environment/accounts_status_test.go index dd21ff29527..7d81ad0a3f4 100644 --- a/fvm/environment/accounts_status_test.go +++ b/fvm/environment/accounts_status_test.go @@ -16,9 +16,6 @@ func TestAccountStatus(t *testing.T) { require.False(t, s.IsAccountFrozen()) t.Run("test frozen flag set/reset", func(t *testing.T) { - // TODO: remove freezing feature - t.Skip("Skip as we are removing the freezing feature.") - s.SetFrozenFlag(true) require.True(t, s.IsAccountFrozen()) @@ -27,6 +24,9 @@ func TestAccountStatus(t *testing.T) { }) t.Run("test setting values", func(t *testing.T) { + // set some values for side effect checks + s.SetFrozenFlag(true) + index := atree.StorageIndex{1, 2, 3, 4, 5, 6, 7, 8} s.SetStorageIndex(index) s.SetPublicKeyCount(34) @@ -37,6 +37,8 @@ func TestAccountStatus(t *testing.T) { require.True(t, bytes.Equal(index[:], returnedIndex[:])) require.Equal(t, uint64(34), s.PublicKeyCount()) + // check no side effect on flags + require.True(t, s.IsAccountFrozen()) }) t.Run("test serialization", func(t *testing.T) { diff --git a/fvm/transactionVerifier_test.go b/fvm/transactionVerifier_test.go index 07556af440b..b3983865bcd 100644 --- a/fvm/transactionVerifier_test.go +++ b/fvm/transactionVerifier_test.go @@ -207,9 +207,6 @@ func TestTransactionVerification(t *testing.T) { }) t.Run("frozen account is rejected", func(t *testing.T) { - // TODO: remove freezing feature - t.Skip("Skip as we are removing the freezing feature.") - ctx := fvm.NewContext( fvm.WithAuthorizationChecksEnabled(true), fvm.WithAccountKeyWeightThreshold(-1), diff --git a/fvm/transaction_test.go b/fvm/transaction_test.go index 8a2fa0fb7a0..c201da90fc9 100644 --- a/fvm/transaction_test.go +++ b/fvm/transaction_test.go @@ -50,13 +50,12 @@ func makeTwoAccounts( } func TestAccountFreezing(t *testing.T) { - // TODO: remove freezing feature - t.Skip("Skip as we are removing the freezing feature.") chain := flow.Mainnet.Chain() serviceAddress := chain.ServiceAddress() t.Run("setFrozenAccount can be enabled", func(t *testing.T) { + address, _, st := makeTwoAccounts(t, nil, nil) accounts := environment.NewAccounts(st) derivedBlockData := derived.NewEmptyDerivedBlockData()