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

test(grpc): GetMempoolTx and GetMempoolStream test #4537

Closed
wants to merge 5 commits into from

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented May 30, 2022

Motivation

We have this 2 calls missing from the grpc tests.

Close #4350

Depends-On: #4663
Depends-On: #4704
Depends-On: #4848

Solution

One of the calls seems to be not working.

Review

Reviewer Checklist

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

Follow Up Work

@oxarbitrage oxarbitrage requested a review from a team as a code owner May 30, 2022 21:14
@oxarbitrage oxarbitrage requested review from teor2345 and removed request for a team May 30, 2022 21:14
@codecov
Copy link

codecov bot commented May 30, 2022

Codecov Report

Merging #4537 (e3ba20d) into main (e9c9ea9) will decrease coverage by 0.04%.
The diff coverage is n/a.

❗ Current head e3ba20d differs from pull request most recent head 43f8f7f. Consider uploading reports for the commit 43f8f7f to get more accurate results

@@            Coverage Diff             @@
##             main    #4537      +/-   ##
==========================================
- Coverage   78.77%   78.72%   -0.05%     
==========================================
  Files         305      305              
  Lines       38742    37423    -1319     
==========================================
- Hits        30518    29461    -1057     
+ Misses       8224     7962     -262     

@conradoplg
Copy link
Collaborator

@oxarbitrage can you please sync this again now that the lightwalletd image issue was fixed?

@teor2345
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jun 15, 2022

update

✅ Branch has been successfully updated

@conradoplg conradoplg mentioned this pull request Jun 15, 2022
3 tasks
@conradoplg
Copy link
Collaborator

Same issue persists after the CI fix... I'm investigating in #4626

@teor2345
Copy link
Contributor

This PR depends on CI and test fixes in PRs:

It might also depend on the sync fixes in PR:

After those PRs merge, we should be able to make more progress on these tests.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I just noticed this typo.

Once PR #4848 is merged, let's update this branch with main and see if the tests pass.

.await?
.into_inner();

// Make sure transactions are returned from the mmepool
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Make sure transactions are returned from the mmepool
// Make sure transactions are returned from the mempool

@teor2345 teor2345 added C-testing Category: These are tests A-rpc Area: Remote Procedure Call interfaces lightwalletd any work associated with lightwalletd and removed S-blocked Status: Blocked on other tasks labels Jul 31, 2022
@teor2345
Copy link
Contributor

teor2345 commented Aug 1, 2022

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Aug 1, 2022

update

✅ Branch has been successfully updated

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

We need to find out how the GetMempoolTx gRPC is meant to work.

counter += 1;
}
// TODO: It seems to be a bug in `GetMempoolTx` as the grpc is not returning transactions
// but we know there are transactions in the mempool as they are returned by `GetMempoolStream` test above.
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried calling the RPCs in the opposite order?
It's possible that they only provide new mempool transactions.

while let Some(_txs) = response.message().await? {
counter += 1;
}
assert!(counter > 0);
Copy link
Contributor

@teor2345 teor2345 Aug 5, 2022

Choose a reason for hiding this comment

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

This assertion might be failing because lightwalletd is still syncing when the transactions are sent. When lightwalletd finishes syncing, it looks like it clears its internal mempool cache, so transactions are no longer available to clients.

(It should eventually re-check the mempool and get the transactions again. But the test will still be unreliable, because it doesn't wait.)

{"app":"lightwalletd","level":"info","msg":"Latest Block changed, clearing everything","time":"2022-08-01T05:30:30Z"}

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened ticket #4894 to fix this.

@teor2345
Copy link
Contributor

teor2345 commented Aug 5, 2022

It looks like we need to fix the full sync test, then this test should work.

I've opened ticket #4894 in the next sprint to fix that.

We can't make any progress on this PR until that's done, so I've moved this ticket to the next sprint as well.

Feel free to re-open this PR when we start work on it again.

@teor2345 teor2345 closed this Aug 5, 2022
@teor2345 teor2345 deleted the issue4350_2 branch September 5, 2022 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
5 participants