Optimize balance-related queries with a cache #2383

rafal-ch · 2024-10-22T15:02:46Z

Linked Issues/PRs

Closes #1965

Description

The client uses the off-chain database metadata to tell if it can use the optimized queries or fall-back to the legacy ones.
When client starts the metadata is either:
1. Kept intact in-case genesis exists (meaning - no optimized queries are available)
  - this stems from the fact that the pre-existing DB contains some coins already and we'll not be re-indexing them in order to create the lookup indexes
  - in this mode of operation the "old" way of getting balances is used as confirmed in logs:
    - worker_service: 607: Balances cache available: false
    - query::balance: 100: Querying balances without balances cache owner=53a9c6a...
2. Initialized with V2 if genesis is missing
  - this means that while re-syncing the DB the balance indexes are going to be created and could be used to satisfy the requests
    - worker_service: 607: Balances cache available: true
    - query::balance: 151: Querying balances using balances cache owner=53a9c6a...

Technical considerations:

Metadata is extended with V2 which now contains indexation_availability. This is a set that holds the available indexations (currently only Balances)
New databases are added: CoinBalances and MessageBalances that keep the coin and message balances respectively
In case balances indexation is available (see "Description" above):
- these databases are updated in the GrapQL API worker service when processing one of these events: MessageImported, MessageConsumed, CoinCreated, CoinConsumed
- balance() and balances() queries use these databases to satisfy the query

Checklist

New behavior is reflected in tests

Before requesting review

I have reviewed the code myself

rafal-ch · 2024-10-30T16:44:34Z

crates/fuel-core/src/graphql_api/worker_service.rs

+    match event {
+        Event::MessageImported(message) => message
+            .update_balances(block_st_transaction, |balance: Cow<u64>, amount| {
+                balance.saturating_add(amount)


I think I'm gonna change these operations to "checked". My understanding is that we'll not be producing events that could lead to overflow, but if something unexpected happen it's still worth to see an explicit error instead of the balance being calculated incorrectly.

Good catch. Hmm, it can happen if anyone decide to create their own token with huge balances. I think the Amount should be u128.

After I implemented this check, some of the integration tests started to fail, because they used some arbitrary transactions with arbitrary amounts and wallets.

I'll be fixing them to use the wallets available in the test environment. One such test is fixed here. @MitchTurner, can you have a look at this and see if this fix makes sense? If so, I'll apply this to other tests.

It turned out that the required changes to tests are too far reaching. I reverted the commits with modified test (474e207 de11071 895d9da) and added a note here (9848f64).

With balances type changed to u128 we are a bit more safe, but still this is an issue worth discussing.

I changed the asset balance to be u128, leaving the contract balance as is.

Also, the amount of each asset is still u64, only the total balance has been promoted to u128. If we want to also upgrade the amount I'll do it in a follow-up PR.

Follow-up issue created: #2428

xgreenx · 2024-10-30T17:44:55Z

crates/fuel-core/src/service/genesis/importer/off_chain.rs

@@ -110,7 +110,7 @@ impl ImportTable for Handler<OwnedMessageIds, Messages> {
        let events = group
            .into_iter()
            .map(|TableEntry { value, .. }| Cow::Owned(Event::MessageImported(value)));
-        worker_service::process_executor_events(events, tx)?;
+        worker_service::process_executor_events(events, tx, true)?;


[nit]: Would be nice to use some named variable or have a comment describing why it is true(the same for another true below)

Updated in 0b39319

xgreenx · 2024-10-30T17:47:21Z

crates/fuel-core/src/service/genesis.rs

+            &DatabaseMetadata::V2 {
+                version: <OffChain as DatabaseDescription>::version(),
+                height: Default::default(),
+                indexation_availability: [(IndexationKind::Balances)]


It would be nice to have a way to force us(via compiler) to handle new variant of the IndexationKind enum here.

Updated here: 8f3e817

xgreenx · 2024-10-30T17:51:21Z

crates/fuel-core/src/graphql_api/storage.rs

+    /// Coin balances per user and asset.
+    CoinBalances = 23,
+    /// Message balances per user.
+    MessageBalances = 24,


I think having 2 tables for balance it is too much=D I think w can have just one table with some:

enum Balance { V1 { coins: Amount, messages: Amount, } }

The suggestion would require the "asset id" to be a part of the key, even if it is technically not needed for messages. That's why I decided to structure it in two separate columns.

Also, current solution would allow us to add more types of "amounts" in the future, without requiring V2.

xgreenx · 2024-10-30T18:02:11Z

crates/fuel-core/src/graphql_api/worker_service.rs

+    match event {
+        Event::MessageImported(message) => message
+            .update_balances(block_st_transaction, |balance: Cow<u64>, amount| {
+                balance.saturating_add(amount)


Good catch. Hmm, it can happen if anyone decide to create their own token with huge balances. I think the Amount should be u128.

…s added

…checks" This reverts commit 474e207.

…checks" This reverts commit de11071.

…check was added" This reverts commit 895d9da.

…n flow (without indexation)

rafal-ch added 30 commits October 14, 2024 10:27

Add basic balances functionality

4a70f52

Add support for querying all balances for user

7d472fb

Adding balances_indexation_progress to DB metadata

4296fd4

Attempt at migrating the metadata to store the indexation progress

77d0f16

Merge remote-tracking branch 'upstream/master' into 1965_balances

525e6f9

Hack the replace_forced() and commit_changes_forced in

8eba5d7

Introduce ForcedCommitDatabase

c083f45

Update dependencies

6b76c37

DB metadata can track multiple indexation progresses

f246cd0

into_genesis() attemt

7472db7

Add some TODOs with ideas for the future

b4d2e0f

Use double_key! macro to define the balances key

d38fdb3

Add basic_storage_tests! for Balances

292acab

Merge remote-tracking branch 'upstream/master' into 1965_balances

651bcc8

Balances DB stores separate information for coins and messages

8342c8f

Fix the recursive call

9a9f120

Init indexation progresses with 0 upon metadata migration

6aa9325

Remove debug prints

b153db4

Store incoming balance in the new Balances DB

512a8a3

Read balance from the new Balances database

adf9e2a

Update coin balance, don't overwrite

344ca90

Use more detailed IndexationStatus, not just block height

f73dbed

Add processing of MessageImported

80da09b

Simplify processing of coins and message amounts

ad5216d

Extract increase_balance()

c784e44

Store coin and message balances separately

5762665

Clean up column naming

5595985

Add test for coin balances

38dd8d6

Support both coins and messages in the new balance system

530bcaf

Merge remote-tracking branch 'upstream/master' into 1965_balances

5604f30

rafal-ch added 3 commits October 29, 2024 15:54

Simplify trait bounds for HasIndexation trait

ddadd80

Further simplify trait bounds for HasIndexation trait

ccadf41

Split and rename the HasIndexation trait

91e8abc

rafal-ch commented Oct 30, 2024

View reviewed changes

xgreenx reviewed Oct 30, 2024

View reviewed changes

rafal-ch added 22 commits October 31, 2024 10:11

WIP - bail when unable to update balance due to overflow

ab7808f

Prefer core::fmt::Display over std::fmt::Display

90fa259

Use named const instead of plain true in genesis importer for clarity

0b39319

Remove reduntant write to DatabaseMetadata::V2

4d6e17a

Ensure all indexations are enabled at genesis

8f3e817

Fix produce_block__raises_gas_price after balance overflow check wa…

895d9da

…s added

Fix more integration tests to work with the balance overflow checks

de11071

Fix typo

71226d4

Fix BLOB integration tests to work with the balance overflow checks

474e207

Revert "Fix BLOB integration tests to work with the balance overflow …

ca6057d

…checks" This reverts commit 474e207.

Revert "Fix more integration tests to work with the balance overflow …

4d43038

…checks" This reverts commit de11071.

Revert "Fix produce_block__raises_gas_price after balance overflow …

e3d898d

…check was added" This reverts commit 895d9da.

Do not bail when balances cannot be updated, log the error instead

9848f64

Total balance of assets is now represented as u128

dae0af5

Infer types in process_balances_update()

facbd2e

Simplify trait bounds in update_balances()

9b8e491

Merge remote-tracking branch 'upstream/master' into 1965_balances_cache

9ea9c02

Fix formatting

54087fc

Satisfy Clippy

20081fa

Update tests

8b9f4e8

Asset balance queries now return U128 instead of U64.

e819e23

Log errors when balance cannot be calculated in the legacy calculatio…

de16099

…n flow (without indexation)

rafal-ch mentioned this pull request Nov 11, 2024

Handle the overflow in "balances" calculation. #2428

Open

rafal-ch added 2 commits November 11, 2024 12:47

Mention an balance overflow follow-up issue in the comments

4f307e3

Remove the TODO comment

66d5948

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize balance-related queries with a cache #2383

Optimize balance-related queries with a cache #2383

rafal-ch commented Oct 22, 2024 •

edited

Loading

rafal-ch Oct 30, 2024

xgreenx Oct 30, 2024

rafal-ch Oct 31, 2024 •

edited

Loading

rafal-ch Nov 2, 2024

rafal-ch Nov 7, 2024

rafal-ch Nov 8, 2024

rafal-ch Nov 11, 2024

xgreenx Oct 30, 2024

rafal-ch Oct 31, 2024

xgreenx Oct 30, 2024

rafal-ch Oct 31, 2024

xgreenx Oct 30, 2024

rafal-ch Nov 8, 2024

xgreenx Oct 30, 2024

Optimize balance-related queries with a cache #2383

Are you sure you want to change the base?

Optimize balance-related queries with a cache #2383

Conversation

rafal-ch commented Oct 22, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafal-ch Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafal-ch commented Oct 22, 2024 •

edited

Loading

rafal-ch Oct 31, 2024 •

edited

Loading