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

fix: prevent spamming blockchain.info with requests #3969

Merged
merged 2 commits into from
Dec 21, 2023

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Dec 18, 2023

In some circumstances, possibly triggered by a bug or unrelated failure it seems possible
for us to keep calling blockstream.info (or other electrs/esplora backend) from the autocommit loop.

I identified two orthogonal improvements:

  • watch needs to be called only once, so calling it again on retries is a waste of time
  • watch & get are two separate operations so should be treated as such

Each of them separately is effectively fixing the problem we witnessed, but both make sense, so I guess makes sense to land both.

@dpc dpc requested a review from a team as a code owner December 18, 2023 19:28
Copy link

codecov bot commented Dec 18, 2023

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (78db6f6) 57.02% compared to head (6044193) 57.01%.

Files Patch % Lines
fedimint-bitcoind/src/lib.rs 0.00% 5 Missing ⚠️
fedimint-bitcoind/src/bitcoincore.rs 0.00% 4 Missing ⚠️
fedimint-bitcoind/src/electrum.rs 0.00% 4 Missing ⚠️
fedimint-bitcoind/src/esplora.rs 0.00% 4 Missing ⚠️
modules/fedimint-wallet-client/src/deposit.rs 72.72% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3969      +/-   ##
==========================================
- Coverage   57.02%   57.01%   -0.01%     
==========================================
  Files         193      193              
  Lines       43143    43166      +23     
==========================================
+ Hits        24603    24612       +9     
- Misses      18540    18554      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

The second commit is an improvement, the first idk, a bit of complexity for not a whole lot of gain imo.

modules/fedimint-wallet-client/src/lib.rs Outdated Show resolved Hide resolved
@dpc
Copy link
Contributor Author

dpc commented Dec 18, 2023

I think what we've observed was actually caused be accumulating multiple deposit addresses. Created a separate issue in: #3970

@dpc dpc force-pushed the 23-12-18-spamming-blockstream-on-deposits branch 3 times, most recently from fecd5fa to 7d1ac83 Compare December 18, 2023 20:27
@dpc dpc force-pushed the 23-12-18-spamming-blockstream-on-deposits branch from 7d1ac83 to 3694237 Compare December 19, 2023 18:28
On some (most) backends "watch" is a no-op, so we
can just call nothing.
@dpc dpc force-pushed the 23-12-18-spamming-blockstream-on-deposits branch from 3694237 to 6044193 Compare December 20, 2023 00:31
@dpc dpc enabled auto-merge December 20, 2023 03:55
}

unreachable!()
Copy link
Contributor

Choose a reason for hiding this comment

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

Will be hit after about 100k years, sgtm.

@dpc dpc added this pull request to the merge queue Dec 21, 2023
Merged via the queue into fedimint:master with commit 05a8497 Dec 21, 2023
20 checks passed
@dpc dpc deleted the 23-12-18-spamming-blockstream-on-deposits branch December 21, 2023 13:56
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.

None yet

3 participants