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

feature(chain): Exposed accounts touched within the block #2253

Merged
merged 7 commits into from
Mar 17, 2020

Conversation

frol
Copy link
Collaborator

@frol frol commented Mar 13, 2020

Resolves #2128

API:

$ http post http://localhost:3030 jsonrpc=2.0 id=dontcare \
    method=EXPERIMENTAL_changes_in_block \
    'params:={"block_id": "DYHRzRbxUR1ANPdCQcgQE9g5zyYQoDZ1k8BJEQ3hDSgW"}'
$ http post http://localhost:3030 jsonrpc=2.0 id=dontcare \
    method=EXPERIMENTAL_changes_in_block \
    'params:={"block_id": 100500}'
$ http post http://localhost:3030 jsonrpc=2.0 id=dontcare \
    method=EXPERIMENTAL_changes_in_block \
    'params:={"finality": "latest"}'

NOTE: The endpoint naming needs some bikeshedding.

The response:

{
    "block_hash": "DYHRzRbxUR1ANPdCQcgQE9g5zyYQoDZ1k8BJEQ3hDSgW",
    "changes": [
        {
            "type": "data_touched",
            "account_id": "frol.near"
        },
        {
            "type": "account_touched",
            "account_id": "frol.near"
        },
        {
            "type": "access_key_touched",
            "account_id": "user.near"
        },
    ]
}

@gitpod-io
Copy link

gitpod-io bot commented Mar 13, 2020

Copy link
Collaborator Author

@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 am not absolutely happy with the naming, so I am open to suggestions.

Comment on lines 828 to 835
/// Retrieve the kinds of state changes occurred in a given block.
///
/// We store different types of data, so we prefer to only expose minimal information about the
/// changes (i.e. a kind of the change and an account id).
fn get_key_value_changes_in_block(
&self,
block_hash: &CryptoHash,
) -> Result<StateChangesKinds, Error> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI, This is the core part of the PR.

core/primitives/src/utils.rs Show resolved Hide resolved
pytest/tests/sanity/rpc_key_value_changes.py Show resolved Hide resolved
core/primitives/src/types.rs Outdated Show resolved Hide resolved
core/primitives/src/utils.rs Show resolved Hide resolved
@evgenykuzyakov
Copy link
Collaborator

evgenykuzyakov commented Mar 14, 2020

@frol  It would also be ideal to not base API for any particular state implementation. E.g. our current serialization with `col` can change later, e.g. if we find it beneficial to build a merkle tree per account. Or some other implementation. 

What is not going to change is API's results, e.g. the format in which you return results already. So it would be good to use input filter to be similar based on `account_id` or suffix/prefix of a given `account_id` (if needed) instead of raw keys.

It's already not the case. Nevermind :)

…e stage to be compatible with the upcoming hashed account_id design
@frol frol requested a review from evgenykuzyakov March 14, 2020 05:16
@frol
Copy link
Collaborator Author

frol commented Mar 14, 2020

@evgenykuzyakov @nearmax I need your help.

This latest commit failed on a test, and this raised a question I had before. Do we want to expose the following types of changes via changes API?

    pub const POSTPONED_RECEIPT_ID: &[u8] = &[4];
    pub const PENDING_DATA_COUNT: &[u8] = &[5];
    pub const POSTPONED_RECEIPT: &[u8] = &[6];
    pub const DELAYED_RECEIPT_INDICES: &[u8] = &[7];
    pub const DELAYED_RECEIPT: &[u8] = &[8];

At least DELAYED_RECEIPT_INDICES gets saved into Trie (this is why the test fail, as I have not implemented key parsing for it, though, there is no info in the key, the whole key is just [7], which makes me believe that it is a "singleton")

@evgenykuzyakov
Copy link
Collaborator

At least DELAYED_RECEIPT_INDICES gets saved into Trie (this is why the test fail, as I have not implemented key parsing for it, though, there is no info in the key, the whole key is just [7], which makes me believe that it is a "singleton")

It's a singleton per shard. It keeps the indices of the delayed receipts queue:
https://github.com/nearprotocol/nearcore/blob/175f42076fb383ffcfc565942e43e53183f09c01/runtime/runtime/src/lib.rs#L1058-L1060

pub const RECEIVED_DATA: &[u8] = &[3]; - Data received for a key data_id. The required postponed receipt might be still not received or requires more pending input data.

pub const POSTPONED_RECEIPT_ID: &[u8] = &[4]; - receipt ID for a key data_id. The Data is not available and is needed for the postponed receipt to execute.

pub const PENDING_DATA_COUNT: &[u8] = &[5]; - the number of missing data inputs that are still not available for a key receipt_id.

pub const POSTPONED_RECEIPT: &[u8] = &[6]; - the receipt itself for a key receipt_id.

pub const DELAYED_RECEIPT_INDICES: &[u8] = &[7]; - the singleton described above

pub const DELAYED_RECEIPT: &[u8] = &[8]; - delayed receipt, because the shard is overwhelmed.

@frol
Copy link
Collaborator Author

frol commented Mar 16, 2020

@evgenykuzyakov Thank you for the information! I will put your comments into the code. It seems that that info is only relevant for internal operation, and given that we don't expose it via API yet, I can skip saving those changes. Though, reading all the above I realized that we are missing shard information in our APIs. It seems that we need to pass shard_id into WrappedTrieChanges and attach it to the stored value and expose it via RPC. Let me know if that does not make sense to you.

@frol frol mentioned this pull request Mar 16, 2020
@evgenykuzyakov
Copy link
Collaborator

Though, reading all the above I realized that we are missing shard information in our APIs. It seems that we need to pass shard_id into WrappedTrieChanges and attach it to the stored value and expose it via RPC.

I'm not sure you need to do this, because shard_id will be always extracted from a pair of AccountId and BlockHeight. So shard_id is more an internal concept. We should not expose it to the RPC if we have an AccountID.

@bowenwang1996
Copy link
Collaborator

Yeah we should not expose shard_id to rpc because it should not be external in any way (it can be subject to change internally).

@frol
Copy link
Collaborator Author

frol commented Mar 17, 2020

I believe this PR is ready to be merged.

@frol
Copy link
Collaborator Author

frol commented Mar 17, 2020

The follow-up documentation request is near/docs#267

@frol frol deleted the feature/2128-RPC-expose-changes-kinds-per-block branch March 20, 2020 14:51
evgenykuzyakov pushed a commit that referenced this pull request Mar 24, 2020
Introducing `TrieKey` trait to work with keys within `TrieUpdate` instead of using raw `Vec<u8>`.

This allows to have additional logic for keys. E.g. if we want to hash AccountID for keys (see #2215) and later return all touched accounts for a block (see #2253) we need a way to map `sha256(account_id)` from a raw key back to the `account_id`.

Then `TrieKey` may implement `fn get_account_id_with_hash() -> Option<(AccountId, CryptoHash)>` logic that provides Account ID and hash of this Account ID. With this info we can remember pairs of touched Accounts in memory of `TrieUpdate` and now we can map back hashes here: https://github.com/nearprotocol/nearcore/blob/62f00b211915790d06e822da866fb01ceb6cdc89/core/store/src/trie/mod.rs#L578

Removes iterators from VMLogic (cause they somewhat block changes from this PR).
Fixes #2276 

##Test Plan:
- unit tests and CI
@frol frol mentioned this pull request Mar 25, 2020
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.

Allow querying accounts touched within the block
3 participants