Skip to content

Conversation

@batconjurer
Copy link
Collaborator

@batconjurer batconjurer commented Apr 4, 2024

Describe your changes

This PR refactors shielded sync to make the following improvements

  • Allow fetching new masp txs and trial-decrypting notes to happen asynchronously
  • Parallelize the trial-decryptions
  • Modularize the logic so that we can mock parts of the algorithm for tests and to enable migration over to using a specila masp indexer
  • Added test coverage
  • Decouple nullifying notes and updating spent notes from the trial-decryption process
  • Refactor the masp.rs module in the sdk into several smaller files and submodules

The new architecture of the algorithm can be found here

N.B. The integration tests seem flaky on the CI, although I can't reproduce any of it locally. It's unclear to me if it is due to the changes in this PR.

Indicate on which release or other PRs this topic is based on

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@batconjurer batconjurer marked this pull request as draft April 4, 2024 15:42
@codecov
Copy link

codecov bot commented Apr 10, 2024

Codecov Report

Attention: Patch coverage is 46.65658% with 1763 lines in your changes missing coverage. Please review.

Project coverage is 54.73%. Comparing base (883bd0f) to head (bdb61c3).

Files Patch % Lines
crates/sdk/src/masp/shielded_ctx.rs 51.29% 844 Missing ⚠️
crates/sdk/src/masp/mod.rs 29.79% 436 Missing ⚠️
crates/sdk/src/masp/utils.rs 51.28% 208 Missing ⚠️
crates/apps_lib/src/client/masp.rs 0.00% 186 Missing ⚠️
crates/sdk/src/masp/types.rs 72.61% 43 Missing ⚠️
crates/node/src/bench_utils.rs 0.00% 27 Missing ⚠️
crates/sdk/src/masp/test_utils.rs 89.79% 15 Missing ⚠️
crates/apps_lib/src/cli/client.rs 0.00% 2 Missing ⚠️
crates/node/src/lib.rs 0.00% 1 Missing ⚠️
crates/sdk/src/tx.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3006      +/-   ##
==========================================
+ Coverage   54.05%   54.73%   +0.67%     
==========================================
  Files         315      319       +4     
  Lines      106296   107702    +1406     
==========================================
+ Hits        57461    58952    +1491     
+ Misses      48835    48750      -85     

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

@batconjurer batconjurer force-pushed the bat/fix/improve-ss-fetching branch 2 times, most recently from 33f5189 to 08a3b7f Compare April 30, 2024 08:45
@batconjurer batconjurer marked this pull request as ready for review May 2, 2024 11:59
@batconjurer batconjurer requested review from grarco and murisi May 2, 2024 11:59
@batconjurer batconjurer changed the title Made fetching and decrypting maps notes operate in parallel. Fetching… Refactor shielded sync May 2, 2024
@batconjurer batconjurer force-pushed the bat/fix/improve-ss-fetching branch from 174c5d2 to 3bcaee8 Compare May 2, 2024 16:04
grarco
grarco previously approved these changes May 3, 2024
Copy link
Collaborator

@grarco grarco left a comment

Choose a reason for hiding this comment

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

This looks good to me, just need to investigate the failing tests

@brentstone
Copy link
Collaborator

What's the status of this? Can we get it on a 0.35.1 base? @batconjurer @grarco

@grarco
Copy link
Collaborator

grarco commented May 9, 2024

I believe we haven't figured out yet the issue with the tests in CI

@batconjurer batconjurer force-pushed the bat/fix/improve-ss-fetching branch from 3bcaee8 to 24481a0 Compare May 13, 2024 16:12
@batconjurer batconjurer force-pushed the bat/fix/improve-ss-fetching branch from 24481a0 to 103a249 Compare May 27, 2024 10:26
@sug0 sug0 force-pushed the bat/fix/improve-ss-fetching branch 6 times, most recently from 1ceece0 to ca44b7b Compare May 29, 2024 14:41
@brentstone brentstone mentioned this pull request May 31, 2024
@batconjurer batconjurer force-pushed the bat/fix/improve-ss-fetching branch from b03d473 to 5bc4268 Compare June 3, 2024 19:09
@brentstone brentstone mentioned this pull request Jun 6, 2024
@sug0
Copy link
Collaborator

sug0 commented Jul 9, 2024

Closed in favor of #3385

@sug0 sug0 closed this Jul 9, 2024
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.

5 participants