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

fix(resharding): storage costs #12661

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

Conversation

Longarithm
Copy link
Member

Fixing old storage costs inaccuracy, which caused a failure during some forknet experiment https://near.zulipchat.com/#narrow/channel/407288-core.2Fresharding/topic/forknet/near/489699725

The fix is one-liner, charge_gas_for_trie_node_access: false,. Originally it wasn't the case because we had a protocol upgrade from trie to flat storage read costs, so we needed to compute costs differently. But since flat storage costs were enabled, and we started to use only the flat storage read costs. Moreover, later it became controlled by runtime Parameter::FlatStorageReads, but the original condition stayed.

And now, when we do flat storage resharding, flat storage indeed doesn't exist for a while, which triggered trie costs for some blocks during resharding again. However, on chunk validation, costs were correct, so chain couldn't validate any chunks since resharding start.

I test this by calling a contract which reads a key and then writes key-value pair back. For the old code, key read charges more cost than it should, which causes InvalidOutcomesProof error.

@Longarithm Longarithm requested a review from a team as a code owner December 20, 2024 16:13
Copy link
Contributor

@wacban wacban left a comment

Choose a reason for hiding this comment

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

LGTM, nice find!

@@ -698,7 +698,7 @@ impl Trie {
storage,
memtries,
root,
charge_gas_for_trie_node_access: flat_storage_chunk_view.is_none(),
charge_gas_for_trie_node_access: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically speaking this should be something more like the following. This one breaks replayability but I don't really mind to be honest given we actually only replay using tools and not this code path.
!ProtocolVersion::FlatStorage.enabled(protocol_version).

@@ -754,6 +841,15 @@ fn test_resharding_v3_base(params: TestReshardingParameters) {
}
trie_sanity_check.assert_state_sanity(&clients, expected_num_shards);
latest_block_height.set(tip.height);
let shard_layout =
client.epoch_manager.get_epoch_config(&tip.epoch_id).unwrap().shard_layout;
println!(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: tracing::info and use the structured logging by setting fields rather than putting them in string

tracing::info!(
  last_block_hash=?tip.last_block_hash,
  ...
  "block"
);

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.

2 participants