feat: keep smt memory #6265
feat: keep smt memory #6265
Conversation
Test Results (CI) 3 files 120 suites 34m 48s ⏱️ Results for commit 94393b7. ♻️ This comment has been updated with latest results. |
ghpbot-tari-project
added
P-acks_required
SWvheerden
force-pushed
the
sw_smt_memory
branch
from
73d20db
to
4472b94
Compare
There was a problem hiding this comment.
I am really impressed by the effect of this PR. I ran system-level testing to sync a 927,181 UTXO blockchain from scratch and archival block sync took 30 min, whereas horizon sync took 2 hours and 7 minutes (still something off with our implementation there, but not due to this PR).
Calculating the SMT for this test blockchain took in excess of 1 minute.
I propose we add an SMT "caching/snapshot" function to the code to save the SMT to file when the base node exits together with the block hash for that height. Whenever the base node starts up it can read the SMT from file, verify that it is for the best block it has, and continue. Whenever it exits again it can create a new snapshot at the best block and delete the previous one. Whenever the SMT needs to be re-calculated during runtime, it could also make use of the snapshot as a starting point.
| if let Err(e) = insert_best_block(&mut txn, block.clone(), consensus, smt.clone()) { | ||
| let mut write_smt = smt.write().map_err(|e| { | ||
| error!( | ||
| target: LOG_TARGET, | ||
| "An attempt to get a write lock on the smt failed. {:?}", e | ||
| ); | ||
| ChainStorageError::AccessError("write lock on smt".into()) | ||
| })?; | ||
| *write_smt = backend.calculate_tip_smt()?; | ||
| return Err(e); | ||
| } |
There was a problem hiding this comment.
I count 3x places in the code where this pattern is employed, where the SMT needs to be recalculated if some underlying error happens, If this function (calculate_tip_smt) errors we cannot recover.
Maybe at this point the SMT should be wiped, and we should have a wrapper function that checks to see if the SMT is empty and try to calculate it whenever we want to use it.
There was a problem hiding this comment.
if calculate_tip_smt fails, you entire db is stuffed, and you need to sync from scratch. Aka wipe the node. So I don't see the need to add a helper function to wipe it and only let it fail again. This replaces the SMT in memory.
There was a problem hiding this comment.
It is better to have no SMT than to have one that is not in sync with the db. The calculate function could fail due to other reasons, not only due to bad lmdb data, for example the OS could be busy swapping memory to disk for a long time and impair other read operations.
There was a problem hiding this comment.
for that a restart of the node is better anyway
base_layer/core/src/validation/block_body/block_body_full_validator.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Hansie Odendaal <39146854+hansieodendaal@users.noreply.github.com>
|
If we want to save a min by caching the SMT, I created a ticket here: #6269 |
* development: fix!: avoid `Encryptable` domain collisions (tari-project#6275) ci(fix): docker image build fix and ci improvements (tari-project#6270) feat: keep smt memory (tari-project#6265) feat: show warning when GRPC method is disallowed (tari-project#6246) fix(chat): metadata panic (tari-project#6247) feat: add monerod detection as an option to the merge mining proxy (tari-project#6248) chore(deps): bump h2 from 0.3.24 to 0.3.26 (tari-project#6250) feat: improve lmdb dynamic growth (tari-project#6242) feat: allow wallet type from db to have preference (tari-project#6245) feat: prevent mempool panic (tari-project#6239) ci: bump nightly version (tari-project#6241) feat: add validation for zero confirmation block sync (tari-project#6237) feat: new template with coinbase call (tari-project#6226) feat: improve wallet sql queries (tari-project#6232) chore: remove ahash as dependancy (tari-project#6238) feat: add dynamic growth to lmdb (tari-project#6231) chore(deps): bump borsh from 0.10.3 to 1.0.0 in /applications/minotari_ledger_wallet (tari-project#6236)
* development: fix!: avoid `Encryptable` domain collisions (tari-project#6275) ci(fix): docker image build fix and ci improvements (tari-project#6270) feat: keep smt memory (tari-project#6265) feat: show warning when GRPC method is disallowed (tari-project#6246) fix(chat): metadata panic (tari-project#6247) feat: add monerod detection as an option to the merge mining proxy (tari-project#6248) chore(deps): bump h2 from 0.3.24 to 0.3.26 (tari-project#6250) feat: improve lmdb dynamic growth (tari-project#6242) feat: allow wallet type from db to have preference (tari-project#6245) feat: prevent mempool panic (tari-project#6239) ci: bump nightly version (tari-project#6241) feat: add validation for zero confirmation block sync (tari-project#6237) feat: new template with coinbase call (tari-project#6226) feat: improve wallet sql queries (tari-project#6232) chore: remove ahash as dependancy (tari-project#6238) feat: add dynamic growth to lmdb (tari-project#6231) chore(deps): bump borsh from 0.10.3 to 1.0.0 in /applications/minotari_ledger_wallet (tari-project#6236)
Description
This only keeps only a single copy of the smt in memory
Motivation and Context
Writing and load the smt from disc every time is inefficient and take long, this keeps in in memory.
Keeping a single copy in an
Arc<RwLock<>>alles the node to keep a single copy and use it.How Has This Been Tested?
unit tests and manual.