Mempool Streaming API #358
Conversation
|
cc: @LarryRuane @pacu @gmale |
|
Hi Aditya, this is great, thanks for contributing this. I tested it and it seems to work perfectly. Just after you mentioned this idea a month or two ago, I started implementing this myself (sorry for the lack of communication but I wasn't sure how far I'd get), but then I got distracted doing other things and put it aside. Seeing your PR, I looked again at those changes, and decided the best would be to combine our changes, which I did, please take a look at the latest commit on this branch. (I thought it would be simpler to make a commit instead of leaving lots of comments on this PR.) The main difference is that I avoided using goroutines, which are cool but somewhat complex to use and to verify the correctness of (in particular, that there are no race conditions). So the code in my branch is somewhat simpler but very similar. I tested this code but would encourage others to test too. A small change I made which we may not want is I added a new message type, It would also be nice to have unit tests; I can work on those if you'd like. If @gmale and @pacu haven't started using the inferior (non-streaming) mempool stuff that I pushed a while ago (the |
|
This is great! I looked through your branch and tested it against Zecwallet, and it looks good to me. We can just use that, and close this PR. The one request I have is to use If you're OK changing the API to return I'm also OK removing the old mempool API. |
|
@LarryRuane None of the other wallets we support or use our SDK use the mempool RPC |
Yes, I just restored that (forced-pushed 01812b2). Instead of closing this PR, it would be less friction to just cherry-pick my commit into this PR (I haven't made my own PR yet, it's just a branch at the moment). Is that okay? |
|
Sounds good. Will you take over this PR and add the tests as well? I'm not very familiar with Go testing. |
There was a problem hiding this comment.
@adityapk00, I removed the old mempool code and added a unit test, could you please review? When it looks okay to you, I'll merge it (I don't have any further changes).
|
This looks great! Thanks for getting this done! Looks good to me! |
This commit is based on adityapk00 streaming mempool interface but avoids using goroutines, which are difficult to reason about. Co-authored-by: Aditya Kulkarni <adityapk@gmail.com>
This RPC has never been used, and is now superceded by GetMempoolStream.
Instead of the Sleep function pointer being a standalong global variable, move it into a new Time struct, and add a Now function pointer, so that time.Now() can be mocked. Time.Now() isn't used yet. This will be cleaner if we need to mock more time-related functions in the future.
LarryRuane
force-pushed
the
mempool_stream
branch
from
863dce4
to
98585ae
Compare
|
Rebased onto the latest master branch, force-pushed. |
This was removed as part of PR zcash#358 (commit "remove gRPC GetMempoolTx") but should not have been, so it's being restored.
This was removed as part of PR #358 (commit "remove gRPC GetMempoolTx") but should not have been, so it's being restored.
New Mempool streaming API and implementation