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

add(test): test disabled lightwalletd mempool gRPCs via zebrad logs #5016

Merged
merged 25 commits into from
Sep 6, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Aug 31, 2022

Motivation

We want to test the lightwalletd mempool gRPC queries, but they are currently disabled.

Designs

Call the gRPC tests, but expect no results.
Check that lightwalletd is querying Zebra for the mempool transactions via the zebrad logs.

Solution

  • Test fetching sent mempool transactions using gRPC
  • Add extra zebrad log checks to the send transaction test
  • Check zebrad JSON-RPC logs instead of disabled lightwalletd gRPCs
  • Add a debug option that makes RPCs pretend the sync is finished

Extra logging:

  • Add debugging output for mempool transaction verification

Related fixes:

  • Only copy the inner state directory in the send transactions test
  • Add transactions to the send transactions list, rather than replacing the list with each new block
  • Preload Zcash parameters in some transaction verification tests
  • Wait for zebrad mempool activation before running gRPC tests
  • Add a block and transaction Hash method to convert from display order
  • Update test coverage docs

Closes #4350

Review

@oxarbitrage is probably the best person to review this test, it uses a bunch of his code from PR #4989.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

If we're going to make more changes to the send transaction test, we might want to:

@teor2345 teor2345 added P-Medium ⚡ A-diagnostics Area: Diagnosing issues or monitoring performance C-testing Category: These are tests A-rpc Area: Remote Procedure Call interfaces lightwalletd any work associated with lightwalletd labels Aug 31, 2022
@teor2345 teor2345 requested a review from oxarbitrage August 31, 2022 05:12
@teor2345 teor2345 self-assigned this Aug 31, 2022
@teor2345 teor2345 requested review from a team as code owners August 31, 2022 05:12
@teor2345
Copy link
Contributor Author

If you want to run this test locally, I'm using:

export TMPDIR=/home/dev/.cache/zebra-tmp
export ZEBRA_CACHED_STATE_DIR=/home/dev/.cache/zebra-custom
export LIGHTWALLETD_DATA_DIR=/home/dev/.cache/lightwalletd-custom
export RUST_LOG=info

cargo test --release \
      --test acceptance \
      --features lightwalletd-grpc-tests \
      -- \
      --nocapture --include-ignored \
      sending_transactions_using_lightwalletd

@teor2345
Copy link
Contributor Author

That's weird, it looks like all the commits didn't get pushed to this PR. It compiled locally, I'll try to find them.

@oxarbitrage
Copy link
Contributor

the command provided to run locally has 2 improvements that i was not using on my testing which are:

  • TMPDIR env var, i didnt knew this was possible so i had the tmp folder hardcoded in the code to juse a different hard drive with enough space to create the copy.
  • --release , this is great, i didnt realized you could do it for this tests, it speed up building time.

I think we should consider adding this to https://github.com/ZcashFoundation/zebra/blob/main/zebrad/tests/acceptance.rs#L96 but we can do that later.

@oxarbitrage
Copy link
Contributor

oxarbitrage commented Aug 31, 2022

That's weird, it looks like all the commits didn't get pushed to this PR. It compiled locally, I'll try to find them.

It build locally as it is, it seems the problem is in the clippy.

@oxarbitrage
Copy link
Contributor

I was running this test locally and it crashed after 2 hours, i think i might be too far away from the tip as it tried to sync and timed out:

2022-08-31T20:11:52.231739Z  INFO {zebrad="c1cb2d6" net="Main"}: zebrad::components::sync::progress: estimated progress to chain tip sync_percent=99.755% current_height=Height(1788271) network_upgrade=Nu5 remaining_sync_blocks=4401 time_since_last_state_block=0s
2022-08-31T20:12:50.216565Z  INFO {zebrad="c1cb2d6" net="Main"}:sync:try_to_sync: zebrad::components::sync: starting sync, obtaining new tips state_tip=Some(Height(1788271))
Error: 
   0: stdout of command did not log any matches for the given regex,
      within the FormattedDuration(3600s) command timeout

Location:
   /media/oxarbitrage/4eb53770-4e4f-4d80-a830-0914f3f5b89a/zebra/send-tx-grpc-mempool-tests/zebra/zebra-test/src/command.rs:856

Match Regex:
   [
       "activating mempool",
   ]

Command:
   "/media/oxarbitrage/4eb53770-4e4f-4d80-a830-0914f3f5b89a/zebra/send-tx-grpc-mempool-tests/zebra/target/release/zebrad" "-c" "/media/oxarbitrage/4eb53770-4e4f-4d80-a830-0914f3f5b89a/tmp/zebrad_testsqZ7F8s/zebrad.toml" "start"

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ SPANTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

   0: zebra_test::command::expect_line_matching_regexes with success_regexes=RegexSet(["activating mempool"]) stream_name="stdout"
      at zebra-test/src/command.rs:814
   1: zebra_test::command::expect_stdout_line_matches with success_regex="activating mempool"
      at zebra-test/src/command.rs:705
   2: acceptance::common::sync::sync_until with height=Height(2147483647) network=Mainnet stop_regex="finished initial sync to chain tip, using gossiped blocks .*sync_percent.*=.*100\\." timeout=3600s mempool_behavior=ShouldAutomaticallyActivate checkpoint_sync=true check_legacy_chain=false
      at zebrad/tests/common/sync.rs:181
   3: acceptance::common::sync::copy_state_and_perform_full_sync with network=Mainnet partial_sync_path="/media/oxarbitrage/4eb53770-4e4f-4d80-a830-0914f3f5b89a/chain/zebra"
      at zebrad/tests/common/sync.rs:302
   4: acceptance::common::lightwalletd::send_transaction_test::load_transactions_from_future_blocks with network=Mainnet zebrad_state_path="/media/oxarbitrage/4eb53770-4e4f-4d80-a830-0914f3f5b89a/chain/zebra"
      at zebrad/tests/common/lightwalletd/send_transaction_test.rs:248

Backtrace omitted. Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.
---- end of test output ----

@teor2345
Copy link
Contributor Author

the command provided to run locally has 2 improvements that i was not using on my testing which are:

TMPDIR env var, i didnt knew this was possible so i had the tmp folder hardcoded in the code to juse a different hard drive with enough space to create the copy.

I added this in this PR.

--release , this is great, i didnt realized you could do it for this tests, it speed up building time.

In ticket #4955 we're going to rename these tests, which would also be a good time to update those docs to use entrypoint.sh. That would automatically run with --release and any other improvements we add later.

I was running this test locally and it crashed after 2 hours, i think i might be too far away from the tip as it tried to sync and timed out

I made all the sync timeouts the same in this PR, so that should fix it.

@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #5016 (225e6ee) into main (ceff590) will decrease coverage by 0.17%.
The diff coverage is 53.70%.

@@            Coverage Diff             @@
##             main    #5016      +/-   ##
==========================================
- Coverage   79.27%   79.09%   -0.18%     
==========================================
  Files         310      310              
  Lines       38907    38990      +83     
==========================================
- Hits        30843    30841       -2     
- Misses       8064     8149      +85     

@oxarbitrage
Copy link
Contributor

I can't understand the CI error pointing to a file that does not exist: https://github.com/ZcashFoundation/zebra/runs/8173468346?check_suite_focus=true#step:13:654

I tried this PR locally and everything build and pass, new test passing.

oxarbitrage
oxarbitrage previously approved these changes Sep 4, 2022
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

This looks really good, thank you for wrapping all this up. It costed me a lot to figure out what was going on here.

zebrad/src/components/mempool.rs Outdated Show resolved Hide resolved
zebrad/tests/acceptance.rs Show resolved Hide resolved
zebrad/tests/common/launch.rs Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor Author

teor2345 commented Sep 5, 2022

I can't understand the CI error pointing to a file that does not exist: https://github.com/ZcashFoundation/zebra/runs/8173468346?check_suite_focus=true#step:13:654

I tried this PR locally and everything build and pass, new test passing.

It happened because two PRs changed the rpc config at the same time, so I needed to resolve the merge conflict in this PR. Until the merge conflict was resolved, GitHub was building the code from the original PR, or maybe building some kind of half-merged branch.

@teor2345 teor2345 requested a review from oxarbitrage September 5, 2022 00:21
@teor2345 teor2345 force-pushed the send-tx-grpc-mempool-tests branch from 8aff9f4 to 61ad23f Compare September 5, 2022 01:15
@teor2345
Copy link
Contributor Author

teor2345 commented Sep 5, 2022

This is #5069:

kex_exchange_identification: Connection closed by remote host

https://github.com/ZcashFoundation/zebra/runs/8181572127?check_suite_focus=true#step:6:105

@teor2345 teor2345 force-pushed the send-tx-grpc-mempool-tests branch from 61ad23f to d1f8b32 Compare September 5, 2022 03:54
@teor2345
Copy link
Contributor Author

teor2345 commented Sep 5, 2022

I fixed a test bug by adding transactions to the send transactions list, rather than replacing the whole list with each new block.

@teor2345 teor2345 force-pushed the send-tx-grpc-mempool-tests branch from d1f8b32 to 225e6ee Compare September 5, 2022 20:47
@teor2345
Copy link
Contributor Author

teor2345 commented Sep 5, 2022

Oops, I forgot to push the latest fixes here.

@teor2345
Copy link
Contributor Author

teor2345 commented Sep 6, 2022

The beta Rust test failed, I opened PR #5090 to disable it.

We can merge this PR anyway, that test isn't required.

mergify bot added a commit that referenced this pull request Sep 6, 2022
@mergify mergify bot merged commit ea34baa into main Sep 6, 2022
@mergify mergify bot deleted the send-tx-grpc-mempool-tests branch September 6, 2022 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Diagnosing issues or monitoring performance A-rpc Area: Remote Procedure Call interfaces C-testing Category: These are tests lightwalletd any work associated with lightwalletd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GetMempoolTx and GetMempoolStream gRPC tests
2 participants