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(state-viewer): add apply_tx and apply_receipt commands #6459

Merged
merged 5 commits into from
Apr 1, 2022

Conversation

marcelo-gonzalez
Copy link
Contributor

This adds apply_tx and apply_receipt commands that search for the given tx or receipt, and run the equivalent of view_state apply or view_state apply_chunk, based on whether the tx or receipt has been applied in some block we've already saved in the chain.

This is a breaking change, since we're getting rid of the --txs and --receipts flags for the apply_chunk command, but it's not that likely that anybody depends on that new command at this point.

Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

How did you test this implementation?

@marcelo-gonzalez
Copy link
Contributor Author

How did you test this implementation?

For the case where the tx/receipt is included in some block, I just ran the command with hashes of some txs/receipts that should exist. Also of course trying it with hashes that don't exist to see that it does what you'd want

then for the case where it doesn't exist in any block on disk, ran a localnet w/ this change:

diff --git a/runtime/runtime/src/lib.rs b/runtime/runtime/src/lib.rs
index 753cf36ad..b5f4d5c2b 100644
--- a/runtime/runtime/src/lib.rs
+++ b/runtime/runtime/src/lib.rs
@@ -226,6 +226,11 @@ impl Runtime {
             tracing::debug_span!(target: "runtime", "Runtime::process_transaction").entered();
         metrics::TRANSACTION_PROCESSED_TOTAL.inc();
 
+        if let Some(Action::Transfer(a)) = signed_transaction.transaction.actions.first() {
+            if a.deposit == 4 {
+                panic!("ai!");
+            }
+        }
         match verify_and_charge_transaction(
             &apply_state.config,
             state_update,

with this pytest code:

#!/usr/bin/env python3

import sys, time, base58
import pathlib

sys.path.append(str(pathlib.Path(__file__).resolve().parents[2] / 'lib'))

from cluster import start_cluster
import transaction
import utils

nodes = start_cluster(3, 0, 4,
                      None,
                      {},
                      {
                          0: { "tracked_shards": [0, 1, 2, 3] },
                          1: { "tracked_shards": [0, 1, 2, 3] },
                          2: { "tracked_shards": [0, 1, 2, 3] },
                      }
)

amount = 1
nonce = 1

for height, hash in utils.poll_blocks(nodes[0],
                                       timeout=100,
                                       poll_interval=0.1):
    print(height)
    if height >= 12:
        break
    if height > 1 and amount < 40:
        for node_idx in range(len(nodes)):
            tx = transaction.sign_payment_tx(
                nodes[0].signer_key, 'test%s' % node_idx, amount,
                nonce,
                base58.b58decode(hash.encode('utf8')))
            nodes[0].send_tx(tx)
            nonce += 1
        amount += 1

so that it should crash when trying to apply the block sending 4 tokens, but we do have chunks containing our txs/receipts

Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

@marcelo-gonzalez could we add some integration tests that can be run in CI? This is to avoid accidentally breaking it in the future

@marcelo-gonzalez
Copy link
Contributor Author

@marcelo-gonzalez could we add some integration tests that can be run in CI? This is to avoid accidentally breaking it in the future

@bowenwang1996 what did you have in mind as far as why the integration test infra would be needed instead of just unit tests? As far as I can tell there's no state viewer integration tests right now, @nikurt do you know if there are any?

Would not be opposed to adding some but that feels like a bigger state viewer testing change than just this PR, since it would be new testing infra. how do you feel about figuring out how to hook up state viewer to integration tests/what tests would be appropriate in a different PR?

@bowenwang1996
Copy link
Collaborator

@bowenwang1996 what did you have in mind as far as why the integration test infra would be needed instead of just unit tests? As far as I can tell there's no state viewer integration tests right now, @nikurt do you know if there are any?

Sorry I meant unit test in state viewer

@marcelo-gonzalez
Copy link
Contributor Author

@bowenwang1996 what did you have in mind as far as why the integration test infra would be needed instead of just unit tests? As far as I can tell there's no state viewer integration tests right now, @nikurt do you know if there are any?

Sorry I meant unit test in state viewer

@bowenwang1996 ok added test. btw this last commit basically contains #6488, because it makes it a little bit easier to test (the test code doesn't have a NearConfig around), and feels more correct anyway

@marcelo-gonzalez marcelo-gonzalez merged commit 65f549d into near:master Apr 1, 2022
@marcelo-gonzalez marcelo-gonzalez deleted the apply-tx branch April 1, 2022 16:34
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.

3 participants