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

[base-node] Optimise pruned UTXO sync streaming protocol #2857

Merged

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Apr 18, 2021

Description

Streams a pruned UTXO set to a client. Each block boundary (given by the
header) is punctuated by a deleted MMR difference bitmap.

Replaces the previous sync_rpc method, which has been removed and so,
this PR is not backward-compatible.

Motivation and Context

Continues from changes in #2847.
There are a few other performance optimisations still to be made for horizon sync.

How Has This Been Tested?

Manually tested. Sync took 15m.

Checklist:

  • I'm merging against the development branch.
  • I have squashed my commits into a single commit.

@sdbondi sdbondi force-pushed the basenode-pruned-sync-perf branch 4 times, most recently from 8397456 to c40e532 Compare April 18, 2021 12:19
philipr-za
philipr-za previously approved these changes Apr 19, 2021
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@SWvheerden SWvheerden 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.
I think the fn synchronize_outputs( is a bit more complex.
I also don't like the use of the word block in the logs for the pruned mode sync. This is technically incorrect and I think confusing as we never download blocks. I flag some but not all uses

@sdbondi sdbondi force-pushed the basenode-pruned-sync-perf branch 3 times, most recently from 6d82210 to 9b7a5cd Compare April 19, 2021 08:23
Streams a pruned UTXO set to a client. Each block boundary (given by the
header) is punctuated by a deleted MMR difference bitmap.

Replaces the previous sync_rpc method, which has been removed and so,
this PR is not backward-compatible.
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

Happy with the approach, but I am concerned about the single DeletedBitmap and can't see how it maps to the MMR hash in the header in all cases.

@sdbondi
Copy link
Member Author

sdbondi commented Apr 20, 2021

@mikethetike The deleted difference bitmap is sent at the end fo each block boundary (as given by output mmr size).
so : UTXO_1 , UTXO_2, ... , deleted diff indexes (signals that we should check the MMR/end of the block)

The only difference to what is sent from before is that instead of many difference bitmaps Vec<Bitmap> I send one union bitmap containing all the newly spent UTXO indexes, these are equivalent in the data they contain. On the client side, we take the previous blocks bitmap and union the differences and check the MMR. So in effect, nothing has changed from before except it's a little smaller over the wire and only 1 bitmap needs to be serialized.

EDIT:
The way fetch_utxos_by_mmr_position works is very confusing, but it will always give you the difference in deleted MMR index differences between h - 1 and h where h is the header heights where start, end index overlap - even if start does not start at the beginning of a block. I don't think we really need this mapping now that we have the output_mmr_size in the headers - outputs should simply be indexed by the integer key mmr_position. Fetching the deleted difference could probably be another call, riffing but something like fetch_spent_txo_diff_bitmap_at(height: u64)

EDIT2:
I think that the biggest perf optimisation after this will be syncing kernels and UTXOs concurrently followed by building an accumulated commitment while syncing for use in the finialise step

Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

Ok, happy

@stringhandler stringhandler merged commit f1fe004 into tari-project:development Apr 20, 2021
@sdbondi sdbondi deleted the basenode-pruned-sync-perf branch April 20, 2021 11:41
stringhandler added a commit that referenced this pull request Apr 28, 2021
Changes since v0.8.8:

Base Node
---
- [#2870](#2870) [base-node] Fixes and tidies up prune mode cleanup
- [#2857](#2857) [base-node] Optimise pruned UTXO sync streaming protocol
- [#2868](#2868) [base-node] Test to reproduce target difficulty problem

Wallet
---
- [#2867](#2867) [wallet] Add option to write unblinded UTXOs to CSV file
- [#2862](#2862) [wallet] Add mined height to display of mined transactions in Console Wallet

Common
---
- [#2876](#2876) [common] Fix incorrect boolean condition
- [#2873](#2873) [tests] Fix case of cucumber
- [#2837](#2837) [docs] Clarify one sided payment
- [#2871](#2871) [tests] Update tests to use sha3
- [#2855](#2855) [tests] Fix re-org test
@stringhandler stringhandler mentioned this pull request Apr 28, 2021
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

4 participants