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.

Sign up for GitHub

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

Jump to bottom

feat: improve wallet sql queries #6232

Merged
merged 2 commits into from
Mar 27, 2024
Merged

feat: improve wallet sql queries #6232

merged 2 commits into from
Mar 27, 2024

Conversation

hansieodendaal
Copy link
Contributor

Description

  • Improved wallet sql queries in:
    • fn update_last_validation_timestamps
    • async fn select_utxos + pub fn fetch_unspent_outputs_for_spending
  • Added the balance_enquiry_cooldown_period config option back in that was removed by a previous PR to minimize balance query impacts for busy console wallets.

Motivation and Context

The console wallet could not efficiently submit transactions if it had many unspent outputs (> 80,000) in its database.

How Has This Been Tested?

System-level stress testing. Previously, each of these selections, fetch_unspent_outputs_for_spending up to final_selection, would take multiple seconds.

2024-03-21 06:44:12.344337500 [wallet::output_manager_service] TRACE select_utxos profile - fetch_unspent_outputs_for_spending: 4000 outputs, 577 ms (at 577)
2024-03-21 06:44:12.346397400 [wallet::output_manager_service] TRACE select_utxos profile - final_selection: 1 outputs from 4000, 2 ms (at 579)
2024-03-21 06:44:13.547512200 [wallet::output_manager_service] TRACE select_utxos profile - fetch_unspent_outputs_for_spending: 4000 outputs, 557 ms (at 557)
2024-03-21 06:44:13.549151900 [wallet::output_manager_service] TRACE select_utxos profile - final_selection: 1 outputs from 4000, 1 ms (at 559)
2024-03-21 06:44:15.137607600 [wallet::output_manager_service] TRACE select_utxos profile - fetch_unspent_outputs_for_spending: 4000 outputs, 552 ms (at 552)
2024-03-21 06:44:15.139724100 [wallet::output_manager_service] TRACE select_utxos profile - final_selection: 1 outputs from 4000, 2 ms (at 554)
2024-03-21 06:44:16.432081200 [wallet::output_manager_service] TRACE select_utxos profile - fetch_unspent_outputs_for_spending: 4000 outputs, 593 ms (at 593)
2024-03-21 06:44:16.433796800 [wallet::output_manager_service] TRACE select_utxos profile - final_selection: 1 outputs from 4000, 1 ms (at 594)
2024-03-21 06:44:17.691752400 [wallet::output_manager_service] TRACE select_utxos profile - fetch_unspent_outputs_for_spending: 4000 outputs, 583 ms (at 583)
2024-03-21 06:44:17.693400000 [wallet::output_manager_service] TRACE select_utxos profile - final_selection: 1 outputs from 4000, 1 ms (at 584)

What process can a PR reviewer use to test or verify this change?

Code review.
System-level stress test (optional).

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

@hansieodendaal hansieodendaal requested a review from a team as a code owner March 25, 2024 16:12
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 25, 2024 •
edited
Loading

Test Results (CI)

    3 files    120 suites   47m 35s ⏱️
1 269 tests 1 269 ✅ 0 💤 0 ❌
3 799 runs  3 799 ✅ 0 💤 0 ❌

Results for commit e727514.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Mar 25, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 25, 2024 •
edited
Loading

Test Results (Integration tests)

 2 files  11 suites   34m 56s ⏱️
32 tests 21 ✅ 0 💤 11 ❌
51 runs  26 ✅ 0 💤 25 ❌

For more details on these failures, see this check.

Results for commit e727514.

♻️ This comment has been updated with latest results.

SWvheerden
SWvheerden reviewed Mar 26, 2024
View reviewed changes
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.

This looks good, just add in a check before the insert

base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs Show resolved Hide resolved
@hansieodendaal
Improve wallet sql queries
06891fb 
- Improved wallet sql queries in:
  - `fn update_last_validation_timestamps`
  - `async fn select_utxos` + `pub fn fetch_unspent_outputs_for_spending`
- Added the `balance_enquiry_cooldown_period` config option back in that
  was removed by a previous PR to minimize balance query impacts for busy
  console wallets.
@hansieodendaal
review comments
e727514
@SWvheerden SWvheerden merged commit 0290204 into tari-project:development Mar 27, 2024
14 of 16 checks passed
@hansieodendaal hansieodendaal mentioned this pull request Mar 27, 2024
Closed
@hansieodendaal hansieodendaal deleted the ho_sql_query_improvements branch March 27, 2024 07:46
sdbondi added a commit to sdbondi/tari that referenced this pull request Apr 15, 2024
@sdbondi
Merge branch 'development' into update-feature-dan2
9afd2c8 
* 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)
sdbondi added a commit to sdbondi/tari that referenced this pull request Apr 15, 2024
@sdbondi
Merge branch 'development' into update-feature-dan2
00cdbb0 
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SWvheerden SWvheerden SWvheerden left review comments

Assignees
No one assigned
Labels
P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hansieodendaal @SWvheerden @ghpbot-tari-project