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

move TrieNodesCount to near-vm-errors #9208

Merged
merged 2 commits into from
Jun 16, 2023
Merged

move TrieNodesCount to near-vm-errors #9208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0eeaff8
move TrieNodesCount to near-vm-errors
Ekleog-NEAR Jun 15, 2023
5ceb2a4
Merge branch 'master' into move-trie-nodes-count
Ekleog-NEAR Jun 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 1 addition & 19 deletions core/primitives/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use borsh::{BorshDeserialize, BorshSerialize};
use near_crypto::PublicKey;
/// Reexport primitive types
pub use near_primitives_core::types::*;
pub use near_vm_errors::TrieNodesCount;
use once_cell::sync::Lazy;
use serde_with::base64::Base64;
use serde_with::serde_as;
Expand Down Expand Up @@ -971,25 +972,6 @@ pub enum TrieCacheMode {
CachingChunk,
}

/// Counts trie nodes reads during tx/receipt execution for proper storage costs charging.
#[derive(Debug, PartialEq)]
pub struct TrieNodesCount {
/// Potentially expensive trie node reads which are served from disk in the worst case.
pub db_reads: u64,
/// Cheap trie node reads which are guaranteed to be served from RAM.
pub mem_reads: u64,
}

impl TrieNodesCount {
/// Used to determine the number of trie nodes charged during some operation.
pub fn checked_sub(self, other: &Self) -> Option<Self> {
Some(Self {
db_reads: self.db_reads.checked_sub(other.db_reads)?,
mem_reads: self.mem_reads.checked_sub(other.mem_reads)?,
})
}
}

/// State changes for a range of blocks.
/// Expects that a block is present at most once in the list.
#[derive(borsh::BorshDeserialize, borsh::BorshSerialize)]
Expand Down
21 changes: 21 additions & 0 deletions runtime/near-vm-errors/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use std::any::Any;
use std::fmt::{self, Error, Formatter};
use std::io;

// ----------8<----------
Copy link
Member

Choose a reason for hiding this comment

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

What is it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A marker for the delimitation of the section of stuff that should be moved to near-vm-runner (ascii art for a line with scissors mark)

// TODO: this does not belong in near-vm-errors but near-vm-runner.
// See #9176 and #9180 for more context.
#[derive(Debug, Clone, PartialEq, BorshDeserialize, BorshSerialize)]
Expand All @@ -30,6 +31,26 @@ impl fmt::Debug for dyn CompiledContractCache {
}
}

/// Counts trie nodes reads during tx/receipt execution for proper storage costs charging.
#[derive(Debug, PartialEq)]
pub struct TrieNodesCount {
Copy link
Member

Choose a reason for hiding this comment

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

Sanity check: this introduces dependency of near-store -> near-primitives -> near-vm-errors. But if we merge near-vm-errors with near-vm-runner, does it introduce dependency of near-store on near-vm-runner?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The end-goal of this is to make near-vm-logic, near-vm-errors, near-vm-runner and runtime into a single crate that will be depended on by the rest of nearcore. So yes, near-store will probably need to depend on near-transaction-runtime (or any other name we decide to give this crate), if only to implement the Store traits that will probably need to be defined in there.

Disclaimer: I’m not at all clear on what near-store exactly is, it’s currently outside my scope of perception; but the goal of all the current PRs is to make near-vm-runner depend on basically no other crate from nearcore. If the near-store -> near-vm-runner dependency is unwanted, we can introduce an intermediate crate that’d depend on both and make the translation from the structs of one to the other one.

(Also, I think near-primitives as a whole should completely cease to exist, and that might solve this specific issue if the dependency would not be by design. But that’s too much work to fit inside the resources allocated to limited replayability unfortunately, so I’m only dealing with the parts of near-primitives that we need to move in order to implement limited replayability)

Copy link
Collaborator Author

@Ekleog-NEAR Ekleog-NEAR Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Just in case, the overall plan is currently described in #8197 (comment) though we’re still discussing alternative approaches for the later steps. But anyway, the first step is to make the codebase that we want to implement limited replayability for be completely scoped and independent from nearcore, because otherwise we’re sure we can’t do anything to improve on it (without such improvements being very wobbly and subject to breakage)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding in comments now I’ve chatted with @jakmeier about it:

  • Today’s situation is both runtime and store depend on primitives(-core)
  • The ideal situation would be runtime and store are both completely isolated crates, and there is a third crate that depends on both to "tie the knot"
  • This PR (and subsequent PRs merging crates) have as a goal to first make (contract) runtime be completely isolated. This naturally means that the shared dependency on primitives must become a dependency from store to runtime in order to keep things working
  • A next step would bring us to the perfect world, by duplicating the TrieNodesCount into store and tying the knot from a crate that depends on both store and runtime
  • (If we wanted to make store isolated today, we would move TrieNodesCount to near-store and make a dependency from runtime to store in the same way, but AFAICT we currently have no direct need of making store isolated, so it can wait until the "ideal situation" listed above)

Copy link
Member

Choose a reason for hiding this comment

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

I see. Yeah I agree that near-store and near-vm-runner should be independent.

The ideal situation would be runtime and store are both completely isolated crates, and there is a third crate that depends on both to "tie the knot"

Question from a guy who never published a crate: why the third crate should go over first two, not under them, like near-primitives currently do?

Anyway there is no plan to make near-store independent so far. Still I would appreciate if all store&vm-related entities were in the same file, at least, to simplify lives of future engineers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Question from a guy who never published a crate: why the third crate should go over first two, not under them, like near-primitives currently do?

The reason is that if we want to publish runtime, then we also need to publish all its dependencies. Which would mean publishing primitives, and thus committing to a stable API for near-primitives.

Seeing how that’s probably not going to happen, going the other way around is much simpler: with a crate over the first two, the third crate doesn’t need to be published :)

Still I would appreciate if all store&vm-related entities were in the same file, at least, to simplify lives of future engineers.

Am I correct to assume that here you’re calling the runtime VM?
To sum up my plan for the "intermediate" architecture, it is currently "near-store is an implementation of the store abstraction exposed by the runtime." This would lead to all types and functions shared between store and runtime being in runtime for now, until we make store independent too and refactor things out

Copy link
Member

Choose a reason for hiding this comment

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

No, I meant only VM, because TrieNodeCount is shared with VM. My understanding of what VM means is that it is the logic of contract execution and TrieNodeCount is used there to compute execution costs.

So if there is something else related to both store and VM, it seems nice to keep all that in the same place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok so what you’re talking about here is near-vm-runner, aka contract runtime. It’s actually not part of the wasm VM, but more a part of near-vm-logic; contract runtime is basically wasm VM + near-vm-logic (that implements host functions).

Anyway, I’m with you that anything else that’d relate both to store and VM should belong close together, probably in a single file if there’s not too much of it :)

/// Potentially expensive trie node reads which are served from disk in the worst case.
pub db_reads: u64,
/// Cheap trie node reads which are guaranteed to be served from RAM.
pub mem_reads: u64,
}

impl TrieNodesCount {
/// Used to determine the number of trie nodes charged during some operation.
pub fn checked_sub(self, other: &Self) -> Option<Self> {
Some(Self {
db_reads: self.db_reads.checked_sub(other.db_reads)?,
mem_reads: self.mem_reads.checked_sub(other.mem_reads)?,
})
}
}
// ---------->8----------

/// For bugs in the runtime itself, crash and die is the usual response.
///
/// See the doc comment on `VMResult` for an explanation what the difference
Expand Down
4 changes: 2 additions & 2 deletions runtime/near-vm-logic/src/dependencies.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
//! External dependencies of the near-vm-logic.

use near_primitives::hash::CryptoHash;
use near_primitives::types::TrieNodesCount;
use near_primitives_core::hash::CryptoHash;
use near_primitives_core::types::{AccountId, Balance};
use near_vm_errors::TrieNodesCount;
use near_vm_errors::VMLogicError;

use std::borrow::Cow;
Expand Down
2 changes: 1 addition & 1 deletion runtime/near-vm-logic/src/gas_counter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use crate::{HostError, VMLogicError};
use near_primitives::types::TrieNodesCount;
use near_primitives_core::config::ExtCosts::read_cached_trie_node;
use near_primitives_core::config::ExtCosts::touching_trie_node;
use near_primitives_core::{
config::{ActionCosts, ExtCosts, ExtCostsConfig},
profile::ProfileDataV3,
types::Gas,
};
use near_vm_errors::TrieNodesCount;
use std::collections::HashMap;

#[inline]
Expand Down
4 changes: 2 additions & 2 deletions runtime/near-vm-logic/src/mocks/mock_external.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::{External, StorageGetMode, ValuePtr};
use near_primitives::hash::{hash, CryptoHash};
use near_primitives::types::TrieNodesCount;
use near_primitives_core::hash::{hash, CryptoHash};
use near_primitives_core::types::{AccountId, Balance};
use near_vm_errors::TrieNodesCount;
use std::collections::HashMap;

#[derive(Default, Clone)]
Expand Down