Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Merklize accounts internal state#4999

Closed
t-nelson wants to merge 2 commits intosolana-labs:masterfrom
t-nelson:merklize_accounts_state
Closed

Merklize accounts internal state#4999
t-nelson wants to merge 2 commits intosolana-labs:masterfrom
t-nelson:merklize_accounts_state

Conversation

@t-nelson
Copy link
Copy Markdown
Contributor

@t-nelson t-nelson commented Jul 9, 2019

Problem

We need a way to generate proofs of account state changes

Summary of Changes

Replace hash of concatenated accounts state hashes with a Merkle root

@t-nelson
Copy link
Copy Markdown
Contributor Author

t-nelson commented Jul 9, 2019

@mvines I think this is the only place where SPV and verifying accounts loaded from snapshots overlap. Should be good to go on cutting someone loose on the latter

@t-nelson t-nelson force-pushed the merklize_accounts_state branch from 3a5e8d5 to f9e7c6d Compare July 10, 2019 17:52
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 11, 2019

Codecov Report

Merging #4999 into master will decrease coverage by 4.3%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #4999     +/-   ##
========================================
- Coverage    76.9%   72.5%   -4.4%     
========================================
  Files         200     197      -3     
  Lines       37494   39023   +1529     
========================================
- Hits        28834   28312    -522     
- Misses       8660   10711   +2051

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 11, 2019

Codecov Report

Merging #4999 into master will decrease coverage by 16.8%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #4999      +/-   ##
=========================================
- Coverage    78.2%   61.3%   -16.9%     
=========================================
  Files         196     197       +1     
  Lines       35789   45635    +9846     
=========================================
+ Hits        27993   27995       +2     
- Misses       7796   17640    +9844

Comment thread runtime/src/accounts.rs Outdated
@t-nelson t-nelson force-pushed the merklize_accounts_state branch 2 times, most recently from 2a8ded9 to 977842b Compare July 11, 2019 22:18
@t-nelson t-nelson requested review from garious and sakridge July 12, 2019 16:43
@sakridge
Copy link
Copy Markdown
Contributor

Speed difference?

@t-nelson
Copy link
Copy Markdown
Contributor Author

@sakridge runtime/benches/accounts.rs is my friend for that info?

@sakridge
Copy link
Copy Markdown
Contributor

sakridge commented Jul 12, 2019

@t-nelson I don't think there exists a micro-bench for this at the moment.

@t-nelson
Copy link
Copy Markdown
Contributor Author

@sakridge Yeah saw that. Adding one now.

@sakridge
Copy link
Copy Markdown
Contributor

sakridge commented Jul 12, 2019

@t-nelson You might check this one out:
#5075
run with cd banking_bench; RUST_LOG=solana_banking_bench=info cargo run --bin solana-banking-bench --release

There should be a print of "average: xx us tx_average: xx us" average contains the time with the accounts hash_internal_state.

@t-nelson
Copy link
Copy Markdown
Contributor Author

@sakridge I used the "test" crate, as was already in place for some benches under runtime. t-nelson@22fc268#diff-7348e822a67e92dad03464ab694a595eR61. No problem to write up something more thorough if it's insufficient

It's takes about twice as long

Hash of concatenated account hashes (old):
test test_accounts_hash_internal_state ... bench: 285,918 ns/iter (+/- 28,519)

Merklized account hashes (new):
test test_accounts_hash_internal_state ... bench: 584,399 ns/iter (+/- 255,462)

@sakridge
Copy link
Copy Markdown
Contributor

@t-nelson Yea I ran it on the benchmark I referenced, it is about 15% reduction in performance just from this. That's pretty huge. Can we make this any faster?

@t-nelson
Copy link
Copy Markdown
Contributor Author

Assuming we don't need the actual account hash later (they appear to be discarded now), I think I might be able to tweak it to pass the serialization of the account state directly to the Merkle Tree ctor. This would eliminate N hashing operations on something that is doing ~3N now

@t-nelson
Copy link
Copy Markdown
Contributor Author

@sakridge is the above bench something we want in tree? I can PR it separately if so

I'll look into reducing hashing operations later today

@t-nelson
Copy link
Copy Markdown
Contributor Author

@sakridge ~20% improvement with the proposed reduction of hashes

Without proposal:
test test_accounts_hash_internal_state ... bench: 578,833 ns/iter (+/- 57,388)

With proposal:
test test_accounts_hash_internal_state ... bench: 471,129 ns/iter (+/- 78,149)

@sakridge
Copy link
Copy Markdown
Contributor

@t-nelson yea we should add the bench to the tree. separate PR is great.

@garious
Copy link
Copy Markdown
Contributor

garious commented Jul 22, 2019

@t-nelson, what's going on with this PR?

@t-nelson
Copy link
Copy Markdown
Contributor Author

@garious I need to reduce negative effects on perf

@t-nelson t-nelson force-pushed the merklize_accounts_state branch from 977842b to 7f5019b Compare July 23, 2019 16:24
@stale
Copy link
Copy Markdown

stale Bot commented Aug 22, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Aug 22, 2019
@aeyakovenko
Copy link
Copy Markdown
Member

@t-nelson @sakridge close this one?

@stale stale Bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Aug 29, 2019
@stale
Copy link
Copy Markdown

stale Bot commented Sep 28, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Sep 28, 2019
@t-nelson
Copy link
Copy Markdown
Contributor Author

Superseded by #5573

@t-nelson t-nelson closed this Sep 28, 2019
@t-nelson t-nelson deleted the merklize_accounts_state branch March 2, 2021 21:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

stale [bot only] Added to stale content; results in auto-close after a week.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants