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

Murisi/client anchor patch optimized #4442

Merged
merged 12 commits into from
Mar 13, 2025
Merged

Conversation

murisi
Copy link
Collaborator

@murisi murisi commented Mar 3, 2025

Describe your changes

Implemented a patch on the MASP client to search for the Transaction ordering used by the protocol. Specifically, for a given block, the client goes through different permutations of its Transactions and checks if applying them would result in a note commitment tree anchor in agreement with the protocol. When this condition is met, the client then moves onto the next block doing the same thing until the client is fully synchronized.

Improvements from original patch in https://github.com/anoma/namada/tree/murisi/check-acknowledgement :

  • Restricted the set of permutations tried to only those that can actually happen
  • Reduced the amount of commitment anchor existence queries by only doing so on block boundaries
  • Deduplicated calls to update_witness_map by not destroying ShieldedContext state after finding correct ordering
  • Now respect last_witnessed_tx so that previously applied Transactions are no longer reapplied
  • Now respect the needs_witness_map_update capability by not updating witness map if it is false
  • Replaced error-prone index arithmetic with iterator usage and factored out hack into separate function
  • Adjusted the client to not reorder (to block ordering) the Transactions it obtains from events or the indexer

Checklist before merging

  • If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
    • This will require 2 reviewers to approve the changes
  • If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
    • Relevant PR if applies:
  • If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
    • Relevant PR if applies:

@murisi murisi force-pushed the murisi/client-anchor-patch-optimized branch from 9d02510 to 3950f53 Compare March 3, 2025 21:57
@github-actions github-actions bot added the breaking:api public API breaking change label Mar 3, 2025
@murisi murisi force-pushed the murisi/client-anchor-patch-optimized branch from 3da173f to 1058fa2 Compare March 3, 2025 23:38
Copy link

codecov bot commented Mar 4, 2025

Codecov Report

Attention: Patch coverage is 79.62085% with 43 lines in your changes missing coverage. Please review.

Project coverage is 74.49%. Comparing base (7b4a880) to head (81907dc).
Report is 46 commits behind head on main.

Files with missing lines Patch % Lines
crates/sdk/src/masp/utilities.rs 34.78% 30 Missing ⚠️
...hielded_token/src/masp/shielded_sync/dispatcher.rs 92.30% 9 Missing ⚠️
crates/tx/src/types.rs 75.00% 3 Missing ⚠️
crates/shielded_token/src/masp/shielded_wallet.rs 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4442      +/-   ##
==========================================
+ Coverage   74.48%   74.49%   +0.01%     
==========================================
  Files         339      339              
  Lines      110643   110786     +143     
==========================================
+ Hits        82415    82535     +120     
- Misses      28228    28251      +23     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@murisi murisi force-pushed the murisi/client-anchor-patch-optimized branch from 98ab885 to 72fc7c8 Compare March 4, 2025 17:23
@murisi murisi force-pushed the murisi/client-anchor-patch-optimized branch from 72fc7c8 to 6879bff Compare March 5, 2025 09:18
@murisi murisi requested review from sug0 and batconjurer March 5, 2025 11:33
@murisi murisi added the do-not-merge Do not merge for now label Mar 8, 2025
@murisi murisi force-pushed the murisi/client-anchor-patch-optimized branch from 8aa7f7f to b9bcbc2 Compare March 8, 2025 07:39
@murisi
Copy link
Collaborator Author

murisi commented Mar 8, 2025

@sug0 @batconjurer Fixed the incorrect masp_index in ShieldedWallet::note_index bug we came across yesterday. This is done in 523af8b . Maybe this is also relevant to the indexer code?

@murisi murisi removed the do-not-merge Do not merge for now label Mar 8, 2025
@murisi murisi force-pushed the murisi/client-anchor-patch-optimized branch from b9bcbc2 to 2355406 Compare March 8, 2025 08:51
@murisi murisi force-pushed the murisi/client-anchor-patch-optimized branch from 2355406 to 523af8b Compare March 8, 2025 09:20
tzemanovic added a commit that referenced this pull request Mar 11, 2025
…t' (#4442)

* murisi/client-anchor-patch-optimized-libs-v0.48-backport:
  Make sure to reset masp_index counter to zero whenever new block is encountered.
  Correct the masp_index once it has been found.
  Reduce cloning of state by updating only the Merkle tree when searching for correct ordering.
  Update commitment tree anchors in benchmarks.
  Order transactions in the cache according to the MASP index instead of the block index.
  Assume that Transactions from the indexer are already ordered.
  Compute the transaction order even when needs_witness_map_update capability is disabled.
  Added a changelog entry.
  Factor out the Transaction order search into a separate function.
  Optimize transaction order search.
  Make client try all Transaction permutations.
  Instrumented code with commitment anchor existence checks.
@tzemanovic
Copy link
Member

this has been released in libs-v0.48.0 and apps v1.1.3

@tzemanovic tzemanovic added the merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass label Mar 13, 2025
mergify bot added a commit that referenced this pull request Mar 13, 2025
@mergify mergify bot merged commit f2da62a into main Mar 13, 2025
28 checks passed
@mergify mergify bot deleted the murisi/client-anchor-patch-optimized branch March 13, 2025 13:41
@grarco grarco mentioned this pull request Mar 18, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking:api public API breaking change client MASP merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants