Skip to content

Avoid updating merkle paths of spent notes#4018

Merged
mergify[bot] merged 2 commits intomainfrom
tiago/update-witness-map-on-spend
Nov 19, 2024
Merged

Avoid updating merkle paths of spent notes#4018
mergify[bot] merged 2 commits intomainfrom
tiago/update-witness-map-on-spend

Conversation

@sug0
Copy link
Copy Markdown
Collaborator

@sug0 sug0 commented Nov 13, 2024

Describe your changes

Avoid updating merkle paths of spent notes. This should optimize the synchronous path of the shielded sync on the ledger client.

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:

@sug0 sug0 added enhancement New feature or request UX MASP SDK breaking:SDK SDK breaking change labels Nov 13, 2024
sug0 added a commit that referenced this pull request Nov 13, 2024
@sug0 sug0 force-pushed the tiago/update-witness-map-on-spend branch from 3325b92 to 550e03a Compare November 13, 2024 13:51
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.92%. Comparing base (b2332bf) to head (550e03a).
Report is 44 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4018      +/-   ##
==========================================
- Coverage   73.92%   73.92%   -0.01%     
==========================================
  Files         341      341              
  Lines      106510   106520      +10     
==========================================
+ Hits        78740    78741       +1     
- Misses      27770    27779       +9     

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


🚨 Try these New Features:

@sug0
Copy link
Copy Markdown
Collaborator Author

sug0 commented Nov 14, 2024

I think this code might be subtly wrong... The nullifier map (nf_map) only gets updated after calling save_decrypted_shielded_outputs, but save_shielded_spends depends on the nf_mapto add data to the set of spent notes (spents).

The point of reordering the call to save_shielded_spends in this PR was to remove entries from the witness map before new witnesses were appended during a call to update_witness_map. Might be best to remove the outdated witnesses inside of update_witness_map instead.

After some reflection with @grarco, we've concluded that this code is probably doing the right thing. For the time being, I'll leave the PR as a draft though.

@sug0 sug0 marked this pull request as draft November 14, 2024 15:08
@brentstone brentstone added this to the v0.46.0 milestone Nov 14, 2024
Copy link
Copy Markdown
Collaborator

@murisi murisi left a comment

Choose a reason for hiding this comment

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

This code looks okay to me if there is no special interaction between this optimization and the speculation code (which I've not looked into before). save_shielded_spends being called earlier should be okay since the new notes saved in save_decrypted_shielded_outputs can only be nullified by a Transaction coming after the one being processed in the current loop iteration.

@sug0 sug0 added merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass backport-45 labels Nov 19, 2024
@sug0 sug0 marked this pull request as ready for review November 19, 2024 16:33
mergify bot added a commit that referenced this pull request Nov 19, 2024
@mergify mergify bot merged commit 19b29ee into main Nov 19, 2024
@mergify mergify bot deleted the tiago/update-witness-map-on-spend branch November 19, 2024 17:48
mergify bot pushed a commit that referenced this pull request Nov 19, 2024
(cherry picked from commit 550e03a)
mergify bot added a commit that referenced this pull request Nov 21, 2024
Avoid updating merkle paths of spent notes (backport #4018)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking:SDK SDK breaking change enhancement New feature or request MASP merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass SDK UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants