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: expose validator information as host functions #2504

Merged
merged 18 commits into from
May 21, 2020

Conversation

bowenwang1996
Copy link
Collaborator

@bowenwang1996 bowenwang1996 commented Apr 19, 2020

Expose validator information as host functions:

validator_stake(account_id_len: u64, account_id_ptr: u64, stake_ptr: u64) -> []
total_validator_stake(stake_ptr: u64) -> []

so that voting contract can calculate the weight of each vote. Resolves #2475. Implements the runtime change specified in near/NEPs#62

Test plan

Test with near-sdk-rs (TODO).

@gitpod-io
Copy link

gitpod-io bot commented Apr 19, 2020

Copy link
Collaborator

@evgenykuzyakov evgenykuzyakov 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 it has to go through NEP first.

While I agree, that we might want to know the current stake of a given validator, we also want to be able to go through the list of all validators.

@bowenwang1996
Copy link
Collaborator Author

NEP near/NEPs#62

@bowenwang1996 bowenwang1996 marked this pull request as ready for review May 12, 2020 18:36
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

I want to raise a concern that VMContext is getting bigger, and it merges various sorts of data from all over the places unconditionally (it locks epoch_manager to fetch this hashmap of validators for every single call even if it is not used inside the contract).

I understand the time constraints, so I am ok unblocking this PR if you think it is fine for now.

P.S. I am marking it as "request changes" since otherwise I will get pinged by GitHub bot for not providing a review.

neard/src/runtime.rs Outdated Show resolved Hide resolved
neard/src/runtime.rs Outdated Show resolved Hide resolved
Comment on lines 616 to 620
/// For not nul-terminated account id:
/// `base + read_memory_base + read_memory_byte * num_bytes + utf8_decoding_base + utf8_decoding_byte * num_bytes + memory_write_base + memory_write_size * 16`
///
/// For nul-terminated account id :
/// `base + (read_memory_base + read_memory_byte) * num_bytes + utf8_decoding_base + utf8_decoding_byte * num_bytes + memory_write_base + memory_write_size * 16`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? How do we end up in this weird state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is related to how get_utf8_string works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's just reference get_utf8_string for costs here. I am as a reader was confused.

runtime/near-vm-logic/src/logic.rs Show resolved Hide resolved
runtime/runtime-params-estimator/src/wasmer_estimator.rs Outdated Show resolved Hide resolved
@bowenwang1996
Copy link
Collaborator Author

I want to raise a concern that VMContext is getting bigger, and it merges various sorts of data from all over the places unconditionally

That is true, but unless we do some static analysis on the code invoked in a block (which I think is more expensive), I don's see a better solution.

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

That is true, but unless we do some static analysis on the code invoked in a block (which I think is more expensive), I don's see a better solution.

I can think of a solution of using static capabilities set on the contract level. Like on Web & Mobile, you ask for permission to get some information. In our case, we can benefit from using metadata enabling certain capabilities statically on compilation or deployment. /cc @evgenykuzyakov @nearmax @vgrichina @ilblackdragon @fckt

P.S. I am approving this PR due to the time constraints. We can design the capabilities for contracts later, when we address the metadata topic.

Comment on lines 616 to 620
/// For not nul-terminated account id:
/// `base + read_memory_base + read_memory_byte * num_bytes + utf8_decoding_base + utf8_decoding_byte * num_bytes + memory_write_base + memory_write_size * 16`
///
/// For nul-terminated account id :
/// `base + (read_memory_base + read_memory_byte) * num_bytes + utf8_decoding_base + utf8_decoding_byte * num_bytes + memory_write_base + memory_write_size * 16`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's just reference get_utf8_string for costs here. I am as a reader was confused.

neard/src/runtime.rs Outdated Show resolved Hide resolved
@@ -7,4 +7,4 @@ pub use genesis_config::{
};

/// Current latest version of the protocol
pub const PROTOCOL_VERSION: u32 = 10;
pub const PROTOCOL_VERSION: u32 = 11;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update after #2574

Copy link
Collaborator

@evgenykuzyakov evgenykuzyakov 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 we should expose this information through External trait instead of passing it to VMContext. Otherwise every simple function call now requires to reconstruct the map. Including view calls.
If RuntimeExt instead takes reference to EpochManager trait that exposes this information, then we don't need to construct the map. It will allow optimizations later.

It will matter, especially when there are sharding and there are 800+ seats.

@MaksymZavershynskyi
Copy link
Contributor

For the context: #2610

@MaksymZavershynskyi
Copy link
Contributor

@bowenwang1996 Should we close this PR in favor of the new design that we have discussed on Tue May 12?

@MaksymZavershynskyi
Copy link
Contributor

CC @mikhailOK

@bowenwang1996
Copy link
Collaborator Author

bowenwang1996 commented May 14, 2020

@nearmax it is different. This is for the voting contract.

@bowenwang1996
Copy link
Collaborator Author

@nearmax please let me know how I should price ext usage.

@MaksymZavershynskyi
Copy link
Contributor

As discussed during the middleware sync our infrastructure is not ready to estimate this specific host function correctly, not until we extract epoch manager and add it to the standalone runtime. Until then we will overshoot the cost of this function to avoid grinding. I suggest the price of this function to be as high as the price of deploying a 1MB contract. @bowenwang1996 , when adding this fee please sync with @olonho to also make a change in the price estimation tool to avoid breaking it.

neard/src/runtime.rs Outdated Show resolved Hide resolved
neard/src/runtime.rs Outdated Show resolved Hide resolved
core/primitives/src/errors.rs Outdated Show resolved Hide resolved
runtime/near-vm-logic/src/logic.rs Outdated Show resolved Hide resolved
runtime/near-vm-runner/src/imports.rs Outdated Show resolved Hide resolved
runtime/runtime/src/ext.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@MaksymZavershynskyi MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

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

There is a bug with order of application of fees. Also, I couldn't find the test for the new functionality. Also, please don't forget to upate nomicon and near-sdk-rs. CC @willemneal for near-sdk-as.

runtime/near-vm-logic/src/logic.rs Outdated Show resolved Hide resolved
runtime/near-vm-logic/src/logic.rs Show resolved Hide resolved
core/primitives/src/errors.rs Show resolved Hide resolved
@bowenwang1996
Copy link
Collaborator Author

@nearmax tests are in near-vm-runner/tests/test_rs_contract.rs:

def_test_ext!(
    ext_validator_stake_alice,
    b"ext_validator_stake",
    &(100u128).to_le_bytes(),
    b"alice"
);
def_test_ext!(ext_validator_stake_bob, b"ext_validator_stake", &(1u128).to_le_bytes(), b"bob");
def_test_ext!(ext_validator_stake_carol, b"ext_validator_stake", &(0u128).to_le_bytes(), b"carol");

def_test_ext!(
    ext_validator_total_stake,
    b"ext_validator_total_stake",
    &(100u128 + 1).to_le_bytes()
)

Copy link
Contributor

@MaksymZavershynskyi MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

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

LGTM assuming the internals of epoch manager are used correctly. Please see the comment about bumping versions.

@@ -1,6 +1,6 @@
[package]
name = "near-vm-logic"
version = "0.8.0"
version = "0.9.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update version of all crates in nearcore/runtime. We currently keep them in sync, until we decide to split them in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see

@bowenwang1996 bowenwang1996 merged commit 442998a into master May 21, 2020
@bowenwang1996 bowenwang1996 deleted the expose-epoch-info branch May 21, 2020 23:28
bowenwang1996 added a commit that referenced this pull request May 22, 2020
In #2504 I made a mistake of not making `validators` in `MockedExternal` public and this is problematic for testing on near-sdk-rs side (cannot change validator information). This PR fixes the issue.

Test plan
---------
Existing tests pass.
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.

Expose validators as host function
6 participants