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

Mark previewnet as transient #5448

Merged
merged 4 commits into from
Feb 27, 2024

Conversation

janezpodhostnik
Copy link
Contributor

@janezpodhostnik janezpodhostnik commented Feb 23, 2024

See discussion here: https://discord.com/channels/613813861610684416/1210016175258599515/1210558298055516240

  1. Previewnet is "Transient" in the sense that it gets bootstrapped by the default bootstrapping settings
  2. Bootstrap with EVM by default
  3. Move service account indexes by 1 because of the new EVM address that is created before them.

@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 56.39%. Comparing base (b9c9109) to head (05837b2).

Files Patch % Lines
model/flow/chain.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                   Coverage Diff                   @@
##           feature/stable-cadence    #5448   +/-   ##
=======================================================
  Coverage                   56.39%   56.39%           
=======================================================
  Files                        1032     1032           
  Lines                      100711   100711           
=======================================================
+ Hits                        56796    56798    +2     
- Misses                      39605    39607    +2     
+ Partials                     4310     4306    -4     
Flag Coverage Δ
unittests 56.39% <0.00%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@turbolent
Copy link
Member

CI fails for a few tests:

  • TestWriteMachineAccountFiles
  • TestEpochStaticTransition/TestStaticEpochTransition
  • TestEpochJoinAndLeaveSN/TestEpochJoinAndLeaveSN

@@ -173,7 +174,7 @@ func WriteMachineAccountFiles(chainID flow.ChainID, nodeInfos []bootstrap.NodeIn
//
// for the machine account key, we keep track of the address index to map
// the Flow address of the machine account to the key.
addressIndex := uint64(4)
addressIndex := uint64(systemcontracts.EVMStorageAccountIndex)
Copy link
Member

Choose a reason for hiding this comment

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

Summarizing the machine account issue here:

Taking SN2 as an example, which has the following machine account file:

  • Address: 0xb4bcb7e5ea8c4c64 [index 25]
  • Machine account PubKey: 0x584319d80567e7b19414c1952a52b5e5a588d6170993ae84b4b655690233482c8fe9e354cf8e308b3771c04e80322b5b1dbe33fe4eeec67d8de6ea9d9dab6178

On-chain we have:

  • Account 0xb4bcb7e5ea8c4c64 [index 25] has the wrong key/hashing algo
  • Account 0x50db0e38fb764cbb [index 24] has pubkey 584319d80567e7b19414c1952a52b5e5a588d6170993ae84b4b655690233482c8fe9e354cf8e308b3771c04e80322b5b1dbe33fe4eeec67d8de6ea9d9dab6178, matching the machine account file

This means that the key generation is using index 25 but the machine account was actually created in index 24. Meaning that our initial index is off by +1 (5 rather than 4). So I think reverting this change will resolve the problem (it also seems to indicate the EVMStorageAccountIndex constant is wrong.

Suggested change
addressIndex := uint64(systemcontracts.EVMStorageAccountIndex)
addressIndex := uint64(4)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it also seems to indicate the EVMStorageAccountIndex constant is wrong

That is concerning!

I will revert those changes from this PR and I will open another PR to deal with that.

@janezpodhostnik janezpodhostnik merged commit bf3cec4 into feature/stable-cadence Feb 27, 2024
50 of 51 checks passed
@janezpodhostnik janezpodhostnik deleted the janez/previewnet-is-transient branch February 27, 2024 13:31
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.

6 participants