add(test): Integration test to send transactions using lightwalletd #4068

jvff · 2022-04-08T19:03:35Z

Motivation

Zebra's current lightwalletd integration test does not cover some RPCs. These RPCs should be covered by extra tests.

Solution

Add an integration test that sends some valid transactions to Zebra using lightwalletd. These transactions are obtained from a valid block in the Zcash chain that the Zebra instance doesn't have. In order to prevent the Zebra instance from downloading that block, it is started with an existing partially synchronized chain and does not have any network connections, so it can't extend that chain.

Closes #3512.

Review

This is still in a draft stage because

change(test): Refactor how extra arguments are handled when spawing lightwalled #4067 needs to be merged first
I'm not sure where all the helper functions should be placed, I left them in the main file, but it should probably be in a separate module
There are some zebrad execution timeouts that I hard-coded to 1 hour, but we might want to change them

Reviewer Checklist

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

Follow Up Work

This test should be added to CI, but it needs an existing cached synchronized chain state to run.

teor2345

This looks like a good design, and it seems to do what we want it to do.

I have some suggestions to make sure the tests are correct and easy to understand.
(The performance questions are not as important.)

zebrad/build.rs

zebrad/proto/compact_formats.proto

zebrad/tests/acceptance.rs

teor2345

I just checked this test against the existing lightwalletd_integration test.

I have a few more suggestions to increase test coverage and reliability.

zebrad/tests/acceptance.rs

zebrad/build.rs

teor2345 · 2022-04-10T23:21:48Z

I've added a bunch of different comments to this PR, let me know if you want to do a video catch up to work through any of them.

jvff · 2022-04-20T21:54:40Z

I had an idea about speeding the test up, let me know what you think:

The transactions that we send are going to be the same each time.

I was actually aiming to not have the same transactions every time 😅 My goal was to use real diverse transactions from the Zcash chain, and my assumption was that the cached state of the full synchronization test would be updated more frequently. Something like being updated every day or so 🤔

But I agree using test vectors would be simpler. I'm not sure how to build test vectors with a good coverage though :/ Or maybe we just want a few simple transactions?

teor2345 · 2022-04-21T22:34:31Z

I was actually aiming to not have the same transactions every time 😅 My goal was to use real diverse transactions from the Zcash chain, and my assumption was that the cached state of the full synchronization test would be updated more frequently. Something like being updated every day or so 🤔

I think this is fine, and we can use the cached tip state.

The cached states should be updated every time we merge to main.

teor2345

Looks great!

If we want to make the environmental variable names consistent, we should do it now, because we'll use them on the DevOps side.

Otherwise I'm happy to merge this, and we can do cleanups, fixes, and refactoring later. (I'd like to re-use some of the utility functions in the full sync test.)

zebrad/tests/acceptance.rs

jvff · 2022-04-22T00:51:31Z

I'd like to move the utility functions to a sub-module, but I don't know which 😅 My initial thought was to create a new module for all of the send transaction test functions (utility functions and the main test function), but I wonder if that might make the test hard to find 🤔

One option might be to create a zebrad/tests/common/lightwalletd/send_transaction_test.rs sub-module. Then I could have the main test function in acceptance.rs be simply a "jump function" (i.e., it would simply call the test function inside that new sub-module). What do you think?

teor2345 · 2022-04-22T01:28:44Z

One option might be to create a zebrad/tests/common/lightwalletd/send_transaction_test.rs sub-module. Then I could have the main test function in acceptance.rs be simply a "jump function" (i.e., it would simply call the test function inside that new sub-module). What do you think?

Based on our planned integration tests, I think it might be useful to have:

a module for gRPC wallet tests setup, for:
- Integration tests for lightwalletd RPCs that are only used when a wallet connects #3655
- Send transactions in lightwalletd integration tests #3512
a module for lightwalletd test setup, for:
- the above tests, and
- Add lightwalletd full sync integration test #3511
- Add lightwalletd quick tip sync test #4166
a module for fully synced Zebra test setup, for:
- the above tests, and
- RPC test with fully synced Zebra #4160

I don't mind what we do with the sendrawtransaction-specific code, as long as we keep it together. Either in acceptance.rs or its own module would be fine.

This isn't a merge blocker, we can always work it out later.

zebrad/tests/acceptance.rs

teor2345

Thanks for these changes, I think everything that's left over is optional.

Feel free to resolve all the comments and merge.
Or anyone should feel free to re-review after any changes.

zebrad/tests/common/lightwalletd/rpc.rs

teor2345

Looks good, just wondering if my build script suggestion worked?

jvff · 2022-04-26T21:10:31Z

Looks good, just wondering if my build script suggestion worked?

Sorry, I think I missed something. What build script suggestion? 😅

oxarbitrage · 2022-04-26T21:27:04Z

Ill probably move some code in send_transaction_test.rs into a generic file and maybe just keep the run() there as i am creating tests that use the same utility functions but do different rpc calls.

teor2345 · 2022-04-26T21:34:11Z

Looks good, just wondering if my build script suggestion worked?

Sorry, I think I missed something. What build script suggestion? 😅

This optional suggestion:
#4068 (comment)

teor2345 · 2022-04-26T23:13:32Z

It's ok to ignore the duplicate heck dependency by updating deny.toml.

teor2345 · 2022-04-26T23:22:48Z

@Mergifyio update

Speed up the initialization of the Zebrad instance used for lightwalletd to connect to.

Document how the environment variable can be used to speed up the test.

Enable checking for known process failure messages.

Document why it exists and how the choice of the value affects the test.

And use it for the Zebrad and the Lightwalletd instances used in the send transaction integration test.

Enable checking the lightwalletd process for known failure messages.

Use the latest version and fix CI failures because `rustfmt` isn't installed in the build environment.

Move the send transaction using lightwalletd test and its helper functions into a new module.

Place it in the parent `lightwalletd` module.

Make them more accessible so that they can be used by other tests.

Move the test utility functions related to using a cached Zebra state into the module.

Keep to closer to the synchronization utility functions.

Place it in the `cached_state` module.

Make it part of the set of tests ignored as a whole if no lightwalletd tests should be executed.

Place it in the `launch` sub-module.

Avoid any potential misunderstandings when the name is seen out of context.

At least until `structopt` is updated or `zebra-utils` is updated to use `clap` 3.

deny.toml

teor2345

I'm not sure what's going on, seems like cmake is missing now?

https://github.com/ZcashFoundation/zebra/runs/6186747677?check_suite_focus=true#step:9:1631

teor2345

Looks good!

prost is a dev dependency, socmake is only needed for the tests. (The locked release build is on a GitHub runner without protobuf, and it works without these changes.)

So we don't need to update the README to list these new requirements.

jvff added P-Medium ⚡ C-testing Category: These are tests A-rpc Area: Remote Procedure Call interfaces labels Apr 8, 2022

jvff self-assigned this Apr 8, 2022

teor2345 reviewed Apr 10, 2022

View reviewed changes

zebrad/build.rs Show resolved Hide resolved

teor2345 added A-dependencies Area: Dependency file updates lightwalletd any work associated with lightwalletd labels Apr 10, 2022

This comment was marked as outdated.

Sign in to view

jvff requested a review from teor2345 April 21, 2022 00:31

conradoplg mentioned this pull request Apr 21, 2022

Integration tests for lightwalletd RPCs that are only used when a wallet connects #3655

Closed

teor2345 reviewed Apr 21, 2022

View reviewed changes

teor2345 reviewed Apr 22, 2022

View reviewed changes

zebrad/tests/acceptance.rs Outdated Show resolved Hide resolved

teor2345 mentioned this pull request Apr 25, 2022

test(rpc): Add Rust tests for lightwalletd sync from Zebra #4177

Merged

3 tasks

teor2345 previously approved these changes Apr 26, 2022

View reviewed changes

zebrad/tests/common/lightwalletd/rpc.rs Outdated Show resolved Hide resolved

jvff dismissed teor2345’s stale review via 015ae07 April 26, 2022 14:50

teor2345 previously approved these changes Apr 26, 2022

View reviewed changes

jvff dismissed teor2345’s stale review via d5db500 April 26, 2022 23:06

jvff force-pushed the send-transaction-lightwalletd-test branch from 015ae07 to d5db500 Compare April 26, 2022 23:06

jvff added 17 commits April 27, 2022 02:46

Force mempool to be enabled in Zebrad instance

411d46c

Speed up the initialization of the Zebrad instance used for lightwalletd to connect to.

Refactor to create LIGHTWALLETD_DATA_DIR_VAR

5728225

Document how the environment variable can be used to speed up the test.

Check for process errors in spawned Zebra instance

600be79

Enable checking for known process failure messages.

Add FINISH_PARTIAL_SYNC_TIMEOUT constant

696e178

Document why it exists and how the choice of the value affects the test.

Add LIGHTWALLETD_TEST_TIMEOUT constant

5fd7a3e

And use it for the Zebrad and the Lightwalletd instances used in the send transaction integration test.

Check lightwalletd process for errors

5d09eec

Enable checking the lightwalletd process for known failure messages.

Update tonic and prost dependencies

179a6e6

Use the latest version and fix CI failures because `rustfmt` isn't installed in the build environment.

Create send_transaction_test module

5040cc8

Move the send transaction using lightwalletd test and its helper functions into a new module.

Move LIGHTWALLETD_TEST_TIMEOUT constant

ed26cc1

Place it in the parent `lightwalletd` module.

Move gRPC helper functions and types to rpc mod.

640879b

Make them more accessible so that they can be used by other tests.

Create a cached_state module

915e43d

Move the test utility functions related to using a cached Zebra state into the module.

Move perform_full_sync_starting_from to sync

29d8374

Keep to closer to the synchronization utility functions.

Move Zebra cached state path variable constant

a4e85b4

Place it in the `cached_state` module.

Skip test if ZEBRA_TEST_LIGHTWALLETD is not set

a3755c8

Make it part of the set of tests ignored as a whole if no lightwalletd tests should be executed.

Move spawn_zebrad_for_rpc_without_initial_peers

d9c8bdf

Place it in the `launch` sub-module.

Rename rpc module into wallet_grpc

e57628f

Avoid any potential misunderstandings when the name is seen out of context.

Allow duplicate heck dependency

6c34231

At least until `structopt` is updated or `zebra-utils` is updated to use `clap` 3.

jvff dismissed teor2345’s stale review via 6c34231 April 27, 2022 02:46

jvff force-pushed the send-transaction-lightwalletd-test branch from 432a1a1 to 6c34231 Compare April 27, 2022 02:46

teor2345 reviewed Apr 27, 2022

View reviewed changes

deny.toml Outdated Show resolved Hide resolved

Fix a deny.toml typo

95c07eb

teor2345 reviewed Apr 27, 2022

View reviewed changes

fix(build): CMake is required by prost crate

dfe4e8b

jvff requested a review from a team as a code owner April 27, 2022 18:10

teor2345 approved these changes Apr 27, 2022

View reviewed changes

mergify bot added a commit that referenced this pull request Apr 27, 2022

Merge of #4068

8b9a37c

mergify bot mentioned this pull request Apr 27, 2022

merge-queue: embarking main (06dc517) and #4068 together #4224

Closed

44 tasks

mergify bot merged commit 5a94a09 into main Apr 27, 2022

mergify bot deleted the send-transaction-lightwalletd-test branch April 27, 2022 23:06

teor2345 mentioned this pull request Apr 28, 2022

fix(ci): Install cmake to fix zcash-params Dockerfile build failure #4235

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add(test): Integration test to send transactions using lightwalletd #4068

add(test): Integration test to send transactions using lightwalletd #4068

jvff commented Apr 8, 2022 •

edited

Loading

teor2345 left a comment •

edited

Loading

teor2345 left a comment

teor2345 commented Apr 10, 2022

This comment was marked as outdated.

jvff commented Apr 20, 2022

teor2345 commented Apr 21, 2022

teor2345 left a comment

jvff commented Apr 22, 2022

teor2345 commented Apr 22, 2022 •

edited

Loading

teor2345 left a comment

teor2345 left a comment

jvff commented Apr 26, 2022

oxarbitrage commented Apr 26, 2022

teor2345 commented Apr 26, 2022

teor2345 commented Apr 26, 2022

teor2345 commented Apr 26, 2022

teor2345 left a comment •

edited

Loading

teor2345 left a comment

add(test): Integration test to send transactions using lightwalletd #4068

add(test): Integration test to send transactions using lightwalletd #4068

Conversation

jvff commented Apr 8, 2022 • edited Loading

Motivation

Solution

Review

Reviewer Checklist

Follow Up Work

teor2345 left a comment • edited Loading

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 commented Apr 10, 2022

This comment was marked as outdated.

jvff commented Apr 20, 2022

teor2345 commented Apr 21, 2022

teor2345 left a comment

Choose a reason for hiding this comment

jvff commented Apr 22, 2022

teor2345 commented Apr 22, 2022 • edited Loading

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

jvff commented Apr 26, 2022

oxarbitrage commented Apr 26, 2022

teor2345 commented Apr 26, 2022

teor2345 commented Apr 26, 2022

teor2345 commented Apr 26, 2022

teor2345 left a comment • edited Loading

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

jvff commented Apr 8, 2022 •

edited

Loading

teor2345 left a comment •

edited

Loading

teor2345 commented Apr 22, 2022 •

edited

Loading

teor2345 left a comment •

edited

Loading