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

feat: await_block LRU cache (non-global) #4080

Merged

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Jan 19, 2024

On top of #4035 and #4042.

This is a version #4046 without global statics.

Wasm couldn't handle lazy_static, and globals are icky. The price is wrangling with an already overly complicated set of traits in this area.

@dpc dpc requested review from a team as code owners January 19, 2024 23:53
@@ -360,15 +385,6 @@ pub trait GlobalFederationApi {

async fn await_transaction(&self, txid: TransactionId) -> FederationResult<TransactionId>;

async fn await_output_outcome<R>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not object safe so had be kicked out (to impl DynGlobalApi).

@dpc dpc force-pushed the 24-01-19-24-01-15-recovery-caching-non-global branch from fa0419e to b68a820 Compare January 19, 2024 23:59
@dpc dpc mentioned this pull request Jan 19, 2024
Copy link

codecov bot commented Jan 20, 2024

Codecov Report

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

Comparison is base (6fb1c18) 58.27% compared to head (2db867d) 58.09%.
Report is 21 commits behind head on master.

Files Patch % Lines
fedimint-core/src/api.rs 75.00% 21 Missing ⚠️
fedimint-client/src/lib.rs 50.00% 2 Missing ⚠️
fedimint-load-test-tool/src/main.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4080      +/-   ##
==========================================
- Coverage   58.27%   58.09%   -0.19%     
==========================================
  Files         192      192              
  Lines       42682    42972     +290     
==========================================
+ Hits        24873    24964      +91     
- Misses      17809    18008     +199     

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

@dpc dpc force-pushed the 24-01-19-24-01-15-recovery-caching-non-global branch 3 times, most recently from 09d8f03 to 4634780 Compare January 20, 2024 01:56
@dpc dpc mentioned this pull request Jan 20, 2024
3 tasks
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.

Looks good, but blocked on two other PRs

fedimint-core/src/api.rs Outdated Show resolved Hide resolved
@dpc dpc force-pushed the 24-01-19-24-01-15-recovery-caching-non-global branch from 4634780 to 4c4e9da Compare January 24, 2024 18:23
@dpc dpc requested a review from elsirion January 24, 2024 18:23
@dpc
Copy link
Contributor Author

dpc commented Jan 26, 2024

@elsirion ✔️ ❓ 🙏 🤣

elsirion
elsirion previously approved these changes Jan 26, 2024
fedimint-core/src/api.rs Outdated Show resolved Hide resolved
maan2003
maan2003 previously approved these changes Jan 26, 2024

// but multiple request for the same `block_index` will serialize on the
// per-index lock, avoiding doing the same work
let mut block_lock = block_entry_arc.lock().await;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@maan2003 maan2003 Jan 26, 2024

Choose a reason for hiding this comment

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

i pushed 2db867d to reduce review cycles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting.

@dpc dpc added this pull request to the merge queue Jan 26, 2024
@maan2003 maan2003 removed this pull request from the merge queue due to a manual request Jan 26, 2024
@maan2003 maan2003 dismissed stale reviews from elsirion and themself via 4efb0f2 January 26, 2024 12:38
maan2003
maan2003 previously approved these changes Jan 26, 2024
@maan2003 maan2003 force-pushed the 24-01-19-24-01-15-recovery-caching-non-global branch from 4efb0f2 to 2db867d Compare January 26, 2024 12:42
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.

Good catch, makes the code a lot simpler!

@elsirion elsirion added this pull request to the merge queue Jan 26, 2024
Merged via the queue into fedimint:master with commit 04d32a7 Jan 26, 2024
21 of 22 checks passed
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