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 wallet / address group endpoints #82

Merged
merged 10 commits into from
Oct 18, 2024
Merged

Add wallet / address group endpoints #82

merged 10 commits into from
Oct 18, 2024

Conversation

mononaut
Copy link
Contributor

@mononaut mononaut commented Mar 28, 2024

No re-index required.

This draft PR adds some new endpoints at /addresses/txs, /scripthashes/txs, /addresses/txs/summary, and /scripthashes/txs/summary, which act as multi-address versions of the corresponding single address/scripthash APIs.

The general idea is to allow paging through the combined history of multiple addresses while keeping the cost per request low and without needing to spam separate requests for each address.

This is mostly a placeholder implementation to support developing/testing mempool/mempool#4831, and probably wants to be heavily refactored and optimized before it's deployed anywhere.

(e.g. the ReverseScanGroupIterator custom multi-iterator should maybe be replaced by something from itertools, and it would be nice to avoid fetching every unconfirmed txid for every address in paginated /txs calls)

@junderw
Copy link
Member

junderw commented Mar 28, 2024

I would be curious to see a comparison between this and just hitting the other endpoints consecutively... maybe write a simple bash script to try out both against this PR.

I feel like it might be significantly slower, although when accounting for latency in actual prod environments the ability to combine them into a single request might make things a bit faster.

Just looking over the code real quick, it seems pretty sane... but yeah, some testing needs to be done imo.

@mononaut
Copy link
Contributor Author

I think the main benefit of doing it this way is that you can share the pagination budget across several addresses.

e.g. currently you'd get rate limited or banned trying to fetch transaction history for 50 addresses with one transaction each in separate requests, even though the actual processing cost is similar to a single request for an address with many transactions.

@wiz
Copy link
Member

wiz commented Sep 27, 2024

does this require a re-index?

@junderw
Copy link
Member

junderw commented Sep 27, 2024

No re-index required

@wiz
Copy link
Member

wiz commented Sep 28, 2024

ok then please rebase

@wiz wiz requested a review from junderw September 28, 2024 03:27
Copy link
Member

@junderw junderw left a comment

Choose a reason for hiding this comment

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

LGTM

A little bit worried about prod performance though.

Local testing seems ok, but it's hard to simulate tons of users wanting to "check their wallets" and a bunch of people hit this endpoint all at once.

@junderw
Copy link
Member

junderw commented Sep 28, 2024

Also @mononaut if there's another reason for this to still be a Draft let me know. I'll let you remove the Draft status once you get a chance.

src/rest.rs Outdated Show resolved Hide resolved
@mononaut
Copy link
Contributor Author

mononaut commented Oct 6, 2024

I'm planning to proceed with this, since it's the best way to support mempool/mempool#5562, and I think it's a generally useful feature regardless.

But I do want to address the performance concerns before taking back out of draft.

@junderw junderw force-pushed the mononaut/wallet-apis branch from 2f7b9d2 to 12599e7 Compare October 7, 2024 06:32
@mononaut
Copy link
Contributor Author

mononaut commented Oct 8, 2024

it took a while to understand the new parallel iterator stuff, but it seems like a big improvement.

I'll take this out of draft now, since between that and the after_txid optimization I'm very happy with the performance now.

@mononaut mononaut marked this pull request as ready for review October 8, 2024 16:34
@junderw
Copy link
Member

junderw commented Oct 9, 2024

  1. It feels like a little bit of repetition can be removed from the TxidLocation stuff.
  2. We should probably include the after_txid/TxidLocation stuff into the single address / summary endpoints as well.

Copy link
Member

@junderw junderw left a comment

Choose a reason for hiding this comment

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

LGTM again.

Copy link
Contributor Author

@mononaut mononaut left a comment

Choose a reason for hiding this comment

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

Tested ACK @ [b7d9b3a]

@wiz wiz merged commit 80758ea into mempool Oct 18, 2024
7 checks passed
@wiz wiz deleted the mononaut/wallet-apis branch October 18, 2024 02:15
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