feat: add block hash API to near_vm_logic #4256

joshuajbouw · 2021-04-27T10:52:15Z

The PR is relatively straight forward, it just exposes two new methods block_hash and prev_block_hash to the VM logic, which may be useful.

I do need clarification that they work as intended, returning the latest block hash however, and not n-1.

frol · 2021-04-27T12:35:18Z

core/primitives-core/src/types.rs

@@ -18,6 +18,9 @@ pub type Nonce = u64;
 pub type BlockHeight = u64;
 /// Height of the epoch.
 pub type EpochHeight = u64;
+/// Block hash.
+#[cfg(feature = "protocol_feature_vm_hash")]
+pub type BlockHash = [u8; 32];


Why is it a new type instead of a CryptoHash?

Right, CryptoHash is a Newtype, and not just a normal type. I was thinking about the returns of the vm logic and how all of them are quite generalized types for primitives, and not Newtypes. So was following that.

Otherwise, I would've used CryptoHash

I think, it is mostly a coincidence that most of the types returned by vm logic are generalized. I would use CryptoHash here, if possible, to avoid introducing new aliases.

type in rust introduce alias, pub type BlockHash = CryptoHash plus type CryptoHash = [u8;32] will have equivalent semantic result as type BlockHash=[u8;32] but more robust if some day we change CryptoHash's definition

willemneal · 2021-04-27T13:41:10Z

core/primitives-core/src/types.rs

@@ -18,6 +18,9 @@ pub type Nonce = u64;
 pub type BlockHeight = u64;
 /// Height of the epoch.
 pub type EpochHeight = u64;
+/// Block hash.
+#[cfg(feature = "protocol_feature_vm_hash")]


Does a type alias need a feature check? It seems that it adds more complexity than its worth.

Fair point, as it shouldn't complain that it isn't being used.

willemneal · 2021-04-27T13:44:48Z

runtime/near-vm-logic/src/logic.rs

+    ///
+    /// `base`
+    #[cfg(feature = "protocol_feature_vm_hash")]
+    pub fn block_hash(&mut self) -> Result<BlockHash> {


I'm a fan of full type path here so that you don't need the feature check at the top of the file. But again it's a personal preference.

If we want to have block hashes - let's surface all the available block hashes based on the block index - e.g. 2 last epochs.

@ilblackdragon You are right, I do need to add in the method to get a block from an index. Ach.

Please sync with @bowenwang1996 to make sure we are not attempting to surface more block hashes than what is available with GC.

I also don't think surfacing more than let's say 10 block hashes is safe. We need to be super conservative with what we are exposing to contracts, because it cannot be undone. If we surface 2 epochs worth of hashes, then we cannot ever change the length of the epoch after that because some contracts may start breaking. And we did change the length of the epoch in past. We also do not want to surface number which is too large, because who knows how aggressive we would want our GC to be in the future. But surfacing something like 10 hashes seems arbitrary. Therefore I suggest we do not surface more than one or two hashes.

@nearmax I understand the concern. However, if we can't get at the last 256 hashes (needed by the EVM's BLOCKHASH instruction), then we will not be able to go with the scheme you outlined in #3456 (comment), but will have to compute the EVM's block hashes based on the NEAR block number instead of the NEAR block hash. Sounds like that'll be the safer thing to do, despite the temporary fork concern you raised there.

I think exposing last 256 blocks should not be a problem, although it requires more effort in the implementation to make sure that it is not expensive. @artob is there any alternative solution to exposing the last 256 block hashes?

EgorKulikov · 2021-04-27T14:25:29Z

Cargo.toml

@@ -117,6 +117,11 @@ protocol_feature_access_key_nonce_range = ["neard/protocol_feature_access_key_no
 protocol_feature_tx_size_limit = ["near-primitives/protocol_feature_tx_size_limit", "node-runtime/protocol_feature_tx_size_limit", "neard/protocol_feature_tx_size_limit"]
 protocol_feature_allow_create_account_on_delete = ["testlib/protocol_feature_allow_create_account_on_delete", "near-primitives/protocol_feature_allow_create_account_on_delete", "node-runtime/protocol_feature_allow_create_account_on_delete", "neard/protocol_feature_allow_create_account_on_delete"]
 protocol_feature_add_account_versions = ["near-primitives/protocol_feature_add_account_versions"]
+protocol_feature_vm_hash = [
+    "node-runtime/protocol_feature_vm_hash",


Seems like duplication

I had realised that, but just entered them all anyways.

joshuajbouw · 2021-04-27T14:56:57Z

I'll put this on WIP for now. We are working out on an alternative solution for EVM, and there is a change that needs to be made in order to call any block hash from a given number.

maxzaver · 2021-04-27T14:56:40Z

core/primitives-core/src/types.rs

@@ -18,6 +18,9 @@ pub type Nonce = u64;
 pub type BlockHeight = u64;
 /// Height of the epoch.
 pub type EpochHeight = u64;
+/// Block hash.
+#[cfg(feature = "protocol_feature_vm_hash")]
+pub type BlockHash = [u8; 32];


I think, it is mostly a coincidence that most of the types returned by vm logic are generalized. I would use CryptoHash here, if possible, to avoid introducing new aliases.

maxzaver · 2021-04-27T15:01:33Z

runtime/near-vm-logic/src/logic.rs

+    ///
+    /// `base`
+    #[cfg(feature = "protocol_feature_vm_hash")]
+    pub fn block_hash(&mut self) -> Result<BlockHash> {


Please sync with @bowenwang1996 to make sure we are not attempting to surface more block hashes than what is available with GC.

I also don't think surfacing more than let's say 10 block hashes is safe. We need to be super conservative with what we are exposing to contracts, because it cannot be undone. If we surface 2 epochs worth of hashes, then we cannot ever change the length of the epoch after that because some contracts may start breaking. And we did change the length of the epoch in past. We also do not want to surface number which is too large, because who knows how aggressive we would want our GC to be in the future. But surfacing something like 10 hashes seems arbitrary. Therefore I suggest we do not surface more than one or two hashes.

maxzaver

Also, we need to add it to imports:

nearcore/runtime/near-vm-runner/src/imports.rs

Line 189 in 8d04bcb

wrapped_imports! {

Then please also add it to runtime-params estimator (if you want in a separate PR to unblock you, since it is urgent). Please check with @olonho

artob · 2021-04-27T21:21:02Z

After calls with @nearmax, @bowenwang1996, @birchmd, and @djsatok today, the scope of this somewhat expanded. I'm reassigning to @birchmd given his previous Chain experience, and will write up an NEP detailing the current plan.

matklad · 2021-04-28T13:33:10Z

runtime/near-vm-logic/src/logic.rs

+    ///
+    /// `base`
+    #[cfg(feature = "protocol_feature_vm_hash")]
+    pub fn prev_block_hash(&mut self) -> Result<BlockHash> {


During discussion with nearmax, we realized that returning a BlockHash directly might not just work. All our other APIs only return primitive types (u64 integers), that's the first case where we want to return something bigger. Looks like we need either to use an out-parameter here, or multiple return values, the latter approach being more promising imo.

The pattern used in other examples, such as sha256, is to use the register API, so I think we should probably stick to that for consistency. Though wasm handling multiple return values now is really cool!

Ah, I somehow miss the fact that we already do use out parameters. Clearly, we should use the same design here as well.

birchmd · 2021-05-04T19:41:19Z

Closing this in favour of #4273

feat: add block hash API to near_vm_logic

22b83b5

joshuajbouw added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-EVM Area: Native EVM implementation and support labels Apr 27, 2021

joshuajbouw requested review from artob, willemneal, abacabadabacaba, bowenwang1996 and maxzaver April 27, 2021 10:52

joshuajbouw requested review from ailisp, EgorKulikov, evgenykuzyakov, frankbraun, frol, matklad, olonho and SkidanovAlex as code owners April 27, 2021 10:52

joshuajbouw changed the title ~~feat: add block hash API to near_vm_logic~~ Feat: Add block hash API to near_vm_logic Apr 27, 2021

joshuajbouw changed the title ~~Feat: Add block hash API to near_vm_logic~~ feat: add block hash API to near_vm_logic Apr 27, 2021

fix BlockHash import

c9a3829

joshuajbouw mentioned this pull request Apr 27, 2021

Support blockhash API #3456

Closed

put sha256 import behind feature

2eb007a

artob assigned joshuajbouw Apr 27, 2021

frol reviewed Apr 27, 2021

View reviewed changes

type cleanup

cc37f9b

willemneal reviewed Apr 27, 2021

View reviewed changes

joshuajbouw added 2 commits April 27, 2021 20:58

remove feature from type

1c571f6

fix tests

9e683a5

EgorKulikov approved these changes Apr 27, 2021

View reviewed changes

Merge branch 'master' into feat/vm-block_hash

f2c8f98

joshuajbouw marked this pull request as draft April 27, 2021 14:57

maxzaver suggested changes Apr 27, 2021

View reviewed changes

artob assigned birchmd and unassigned joshuajbouw Apr 27, 2021

matklad reviewed Apr 28, 2021

View reviewed changes

birchmd mentioned this pull request May 4, 2021

Feat(runtime): block_hash host function #4273

Closed

birchmd closed this May 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add block hash API to near_vm_logic #4256

feat: add block hash API to near_vm_logic #4256

joshuajbouw commented Apr 27, 2021 •

edited

Loading

frol Apr 27, 2021

joshuajbouw Apr 27, 2021 •

edited

Loading

maxzaver Apr 27, 2021

ailisp Apr 27, 2021

willemneal Apr 27, 2021

joshuajbouw Apr 27, 2021 •

edited

Loading

willemneal Apr 27, 2021

ilblackdragon Apr 27, 2021 •

edited

Loading

joshuajbouw Apr 27, 2021

joshuajbouw Apr 27, 2021

maxzaver Apr 27, 2021

artob Apr 27, 2021

bowenwang1996 Apr 27, 2021

EgorKulikov Apr 27, 2021

joshuajbouw Apr 27, 2021

joshuajbouw commented Apr 27, 2021

maxzaver Apr 27, 2021

maxzaver Apr 27, 2021

maxzaver left a comment

artob commented Apr 27, 2021

matklad Apr 28, 2021

birchmd Apr 28, 2021

matklad Apr 29, 2021

birchmd commented May 4, 2021

feat: add block hash API to near_vm_logic #4256

feat: add block hash API to near_vm_logic #4256

Conversation

joshuajbouw commented Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

joshuajbouw Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuajbouw Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilblackdragon Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuajbouw commented Apr 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxzaver left a comment

Choose a reason for hiding this comment

artob commented Apr 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

birchmd commented May 4, 2021

joshuajbouw commented Apr 27, 2021 •

edited

Loading

joshuajbouw Apr 27, 2021 •

edited

Loading

joshuajbouw Apr 27, 2021 •

edited

Loading

ilblackdragon Apr 27, 2021 •

edited

Loading