Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve block handling in remote store #489

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improve block handling in remote store #489

wants to merge 1 commit into from

Conversation

peterargue
Copy link
Contributor

@peterargue peterargue commented Oct 25, 2023 •
edited
Loading

Closes #???

Description

The forked mainnet functionality is helpful to testing full application integration. With the current implementation, there are some key differences between behavior of the real network and emulator, particularly with error messages and block handling.

This PR attempts to close the gap and adds testing to cover various cases to ensure block responses behave as expected.

For contributor use:

  • Targeted PR against master branch
  • Linked to GitHub issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the GitHub PR explorer
  • Added appropriate labels

@peterargue peterargue self-assigned this Oct 25, 2023
@peterargue peterargue added the Improvement Technical work without new features, refactoring, improving tests label Oct 25, 2023
peterargue
peterargue commented Oct 25, 2023
View reviewed changes
Makefile
@@ -15,7 +15,7 @@ GOPATH ?= $(HOME)/go
install-tools:
mkdir -p ${GOPATH}; \
cd ${GOPATH}; \
GO111MODULE=on go install github.com/golang/mock/mockgen@v1.6.0; \
GO111MODULE=on go install go.uber.org/mock/mockgen@v0.3.0; \
Copy link
Contributor Author

@peterargue peterargue Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

github.com/golang/mock is deprecated

peterargue
peterargue commented Oct 25, 2023
View reviewed changes
storage/remote/store.go
@@ -176,11 +224,6 @@ func (s *Store) LedgerByHeight(
ctx context.Context,
blockHeight uint64,
) (snapshot.StorageSnapshot, error) {
err := s.SetBlockHeight(blockHeight)
Copy link
Contributor Author

@peterargue peterargue Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting this here could have confusing results, since it may reset the latest height to an earlier block if ExecuteScript is called with a past height.

It's already called when committing a block, so I think we can skip it

flow-emulator/storage/store.go

Lines 213 to 216 in 0273f8f

if block.Header.Height >= latestBlockHeight {
return s.DataSetter.SetBytes(ctx, s.KeyGenerator.Storage(globalStoreName), s.KeyGenerator.LatestBlock(), mustEncodeUint64(block.Header.Height))
}
return nil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sideninja keep me honest

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, looking at this it doesn't need to be here, not sure what was my thinking at the time.

@bluesign
Copy link
Collaborator

@peterargue thanks a lot for this, btw do we have archive nodes still ? last time I tried to work on this I was failing at that.

Also if someone can fix, or at least explain to me, how flow-archive tags works, it would be great, there are 2 version stream one with v1.x another one is v0.x but I guess newer is v0.x but last time I tried archive node was using v1.x etc

@sideninja
Copy link
Member

@peterargue thanks a lot for this, btw do we have archive nodes still ? last time I tried to work on this I was failing at that.

Also if someone can fix, or at least explain to me, how flow-archive tags works, it would be great, there are 2 version stream one with v1.x another one is v0.x but I guess newer is v0.x but last time I tried archive node was using v1.x etc

We are working on merging archive node functionality into access nodes as we speak. So right now the issues you are experiencing are due to that. We should have something available soon.

sideninja
sideninja reviewed Oct 25, 2023
View reviewed changes
storage/remote/store.go
store.DataGetter = store
store.DataSetter = store
store.KeyGenerator = &storage.DefaultKeyGenerator{}

return store, nil
}

func (s *Store) initializeStartBlock(ctx context.Context) error {
if s.startBlockHeight > 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This misses a case where you don't provide the block height but you persisted it in the previous session.

sideninja
sideninja reviewed Oct 25, 2023
View reviewed changes
storage/remote/store_test.go
@@ -287,3 +292,224 @@ func Test_SimulatedMainnetTransactionWithChanges(t *testing.T) {
assert.Equal(t, txRes.Events[0].String(), "A.9799f28ff0453528.Ping.PingEmitted: 0x953f6f26d61710cb0e140bfde1022483b9ef410ddd181bac287d9968c84f4778")
assert.Equal(t, txRes.Events[0].Value.String(), `A.9799f28ff0453528.Ping.PingEmitted(sound: "pong pong pong")`)
}

func Test_SimulatedMainnetProgressesBlocks(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

bluesign
bluesign reviewed Oct 25, 2023
View reviewed changes
storage/remote/store.go
if err != nil {
return nil, err
}

return snapshot.NewReadFuncStorageSnapshot(func(id flowgo.RegisterID) (flowgo.RegisterValue, error) {
// first try to see if we have local stored ledger
value, err := s.DefaultStore.GetBytesAtVersion(
Copy link
Collaborator

@bluesign bluesign Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here needs a check, something like: if blockHeight <= s.CurrentHeight
or we should totally disallow higher block height requests than forked one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great point. I think this should just rewrite the height if it's greater than the fork height

turbolent
turbolent reviewed Oct 25, 2023
View reviewed changes
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

storage/remote/store.go
@@ -154,6 +198,10 @@ func (s *Store) BlockByHeight(ctx context.Context, height uint64) (*flowgo.Block
return block, nil
}

if latestBlockHeight, err := s.LatestBlockHeight(ctx); err == nil && height > latestBlockHeight {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if err != nil?

storage/remote/store.go Show resolved Hide resolved
@bluesign
Copy link
Collaborator

bluesign commented Nov 4, 2023

I think we need to fix also register requests part, I messed up there before because of flow-archive versioning problems. ( I can make a PR for it, or contribute to this PR )

@bluesign
Copy link
Collaborator

bluesign commented Nov 7, 2023

ok I updated #433 with required changes, which required a PR on flow-archive ( for conflicting go versions ) onflow/flow-archive#143

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turbolent turbolent turbolent left review comments

@bluesign bluesign bluesign left review comments

@sideninja sideninja sideninja approved these changes

@janezpodhostnik janezpodhostnik Awaiting requested review from janezpodhostnik janezpodhostnik is a code owner

Assignees

@peterargue peterargue

Labels
Improvement Technical work without new features, refactoring, improving tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@peterargue @bluesign @sideninja @turbolent