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.

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

Re-enable Epoch Integration Tests #3835

Merged
merged 16 commits into from
Jan 25, 2023

Conversation

jordanschalm
Copy link
Member

@jordanschalm jordanschalm commented Jan 19, 2023

This PR re-enables all epoch integration tests, reduces time taken by epoch integration tests by about 30%, and fixes a bug in the snapshot validator function.

  • Fix off-by-one error in snapshot validator function
  • Update documentation of validator, and require in Access Node as well
  • Remove expiry-escape mitigation in AN ingestion, which is now covered by the snapshot validator
  • Replace separate staking step transactions (create account, fund, set up staking collection, register node) with one custom transaction
  • This enables reducing the epoch length by 50%, speeding up the test significantly

Test Speed Notes

Local

Before improvements, was 650-720s.
After improvements, is 400-430s

CI

Before improvements, was 30-40m
After improvements, is ~23m

Previously failure to retrieve finalized block would log an error but
continue, causing a nil ptr exception.
@codecov-commenter
Copy link

codecov-commenter commented Jan 19, 2023

Codecov Report

Merging #3835 (2b7a1fd) into feature/active-pacemaker (3a52f2e) will decrease coverage by 8.98%.
The diff coverage is n/a.

@@                     Coverage Diff                      @@
##           feature/active-pacemaker    #3835      +/-   ##
============================================================
- Coverage                     53.51%   44.53%   -8.98%     
============================================================
  Files                           812      223     -589     
  Lines                         76585    25261   -51324     
============================================================
- Hits                          40984    11251   -29733     
+ Misses                        32307    12999   -19308     
+ Partials                       3294     1011    -2283     
Flag Coverage Δ
unittests 44.53% <ø> (-8.98%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
fvm/transactionVerifier.go 80.86% <0.00%> (-8.72%) ⬇️
fvm/transactionInvoker.go 77.09% <0.00%> (-7.21%) ⬇️
cmd/scaffold.go 15.63% <0.00%> (-5.27%) ⬇️
fvm/environment/programs.go 58.69% <0.00%> (-5.01%) ⬇️
ledger/complete/wal/checkpoint_v6_reader.go 59.01% <0.00%> (-1.23%) ⬇️
fvm/context.go 54.94% <0.00%> (-1.10%) ⬇️
fvm/environment/contract_updater.go 66.88% <0.00%> (-0.66%) ⬇️
engine/access/ingestion/engine.go
state/protocol/badger/validity.go
...s/hotstuff/signature/randombeacon_reconstructor.go
... and 586 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

(we should look at splitting these out into separate jobs, but this
will show whether test changes are working)
Previously ANs joining with non-spork-root snapshot would skip
ingesting first 600 block's collection, to ensure they could resolve
all reference blocks. That has been replaced by ensuring the initial
snapshot itself has enough history to resolve all reference
blocks for all ingested blocks after the root block
(which will create and setup staking account all in one shot)
* remove unused functions, add link to commit where they can be found
  again if needed
Copy link
Member

@durkmurder durkmurder left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Member

@AlexHentschel AlexHentschel left a comment

Choose a reason for hiding this comment

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

very nice.

sdktemplates "github.com/onflow/flow-go-sdk/templates"
"github.com/onflow/flow-go/model/flow"
)

//go:embed templates/create-and-setup-node.cdc
Copy link
Member

Choose a reason for hiding this comment

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

cool, didn't know about go:embed; very handy

@jordanschalm
Copy link
Member Author

bors merge

@bors bors bot merged commit 70cfc86 into feature/active-pacemaker Jan 25, 2023
@bors bors bot deleted the jordan/6442-epoch-sn-test branch January 25, 2023 20:29
bors bot added a commit that referenced this pull request Jan 30, 2023
3869: Sync `feature/active-pacemaker` with `v0.29` r=jordanschalm a=jordanschalm

Sync `feature/active-pacemaker` with `v0.29`

Adds 2 PRs:
- #3859
- #3835

Additional commit resolves merge conflicts:
- 0ba6366

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
bors bot added a commit that referenced this pull request Jan 30, 2023
3869: Sync `feature/active-pacemaker` with `v0.29` r=jordanschalm a=jordanschalm

Sync `feature/active-pacemaker` with `v0.29`

Adds 2 PRs:
- #3859
- #3835

Additional commit resolves merge conflicts:
- 0ba6366

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants