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

feat: flat storage rollout on nightly #8697

Merged
merged 24 commits into from
Mar 20, 2023
Merged

Conversation

Longarithm
Copy link
Member

@Longarithm Longarithm commented Mar 8, 2023

Here we switch from assertions of flat storage vs. trie reads to reads from flat storage only. Part of #8501.

Also moving create_flat_storage_manager to FlatStorageManager::test - I need to reuse it in Runtime tests.

Testing

  • test_storage_read_write_costs_runtime - runtime-specific test to check costs for subsequent storage read and write. It emphasizes the difference in costs before and after flat storage. Note that without flat storage we still can call contracts, but results, especially costs, will likely diverge from results on chain.
  • test_flat_storage_upgrade - integration test for protocol upgrade correctness check
  • https://nayduck.near.org/#/run/2901 https://nayduck.near.org/#/run/2907

@Longarithm Longarithm changed the title draft: flat storage rollout draft: flat storage rollout on nightly Mar 9, 2023
@Longarithm Longarithm marked this pull request as ready for review March 9, 2023 18:52
@Longarithm Longarithm requested a review from a team as a code owner March 9, 2023 18:52
@Longarithm Longarithm changed the title draft: flat storage rollout on nightly feat: flat storage rollout on nightly Mar 9, 2023
.sign(&signer);
let tx_hash = signed_transaction.get_hash();
env.clients[0].process_tx(signed_transaction, false, false);
for i in 0..epoch_length / 3 {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to produce epoch_length / 3 blocks here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is subtle. Added comment and linking this to #8703 for future reference

I confirmed that this issue with protocol auto-upgrade is still the case. Protocol version updates to latest after I process 4 epochs. I learned from somewhere that first 2 epochs are fine.

@Longarithm Longarithm requested review from pugachAG and jakmeier March 14, 2023 17:05
@Longarithm
Copy link
Member Author

Longarithm commented Mar 14, 2023

@jakmeier small note on test_cost_sanity: it may look suspicious that only read_cached_trie_node cost changes. It is right though, one can see that we touch the same set of unique nodes in the test, so TTN doesn't change.

@jakmeier
Copy link
Contributor

As discussed, this will break neard view-state apply_range. For now only on nightly but that is already quite annoying.

Do we have a follow up on this comment already? https://near.zulipchat.com/#narrow/stream/297873-pagoda.2Fnode/topic/replayability.20and.20flat.20storage/near/342101682

@Longarithm
Copy link
Member Author

@jakmeier sure, filed #8741.

@@ -80,6 +80,7 @@ impl RuntimeUser {
apply_state: ApplyState,
prev_receipts: Vec<Receipt>,
transactions: Vec<SignedTransaction>,
use_flat_storage: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I don't understand this flag, why wouldn't we want to use flat state in all tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed to check fallback to Trie, to defend from accidental protocol changes - if at some point we decide to implement this fallback, the new test requires doing it properly.
And such fallback is needed for view clients anyway, so I think it is valuable to have it here.

Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

I think this is good to go into nightly.

@near-bulldozer near-bulldozer bot merged commit bc25a2d into near:master Mar 20, 2023
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