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

Optimize balance-related queries with a cache #2383

Open
wants to merge 94 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 70 commits
Commits
Show all changes
94 commits
Select commit Hold shift + click to select a range
4a70f52
Add basic balances functionality
rafal-ch Oct 14, 2024
7d472fb
Add support for querying all balances for user
rafal-ch Oct 14, 2024
4296fd4
Adding `balances_indexation_progress` to DB metadata
rafal-ch Oct 14, 2024
77d0f16
Attempt at migrating the metadata to store the indexation progress
rafal-ch Oct 15, 2024
525e6f9
Merge remote-tracking branch 'upstream/master' into 1965_balances
rafal-ch Oct 16, 2024
8eba5d7
Hack the `replace_forced()` and `commit_changes_forced` in
rafal-ch Oct 16, 2024
c083f45
Introduce `ForcedCommitDatabase`
rafal-ch Oct 16, 2024
6b76c37
Update dependencies
rafal-ch Oct 16, 2024
f246cd0
DB metadata can track multiple indexation progresses
rafal-ch Oct 17, 2024
7472db7
into_genesis() attemt
rafal-ch Oct 17, 2024
b4d2e0f
Add some TODOs with ideas for the future
rafal-ch Oct 17, 2024
d38fdb3
Use `double_key!` macro to define the balances key
rafal-ch Oct 17, 2024
292acab
Add `basic_storage_tests!` for Balances
rafal-ch Oct 17, 2024
651bcc8
Merge remote-tracking branch 'upstream/master' into 1965_balances
rafal-ch Oct 17, 2024
8342c8f
Balances DB stores separate information for coins and messages
rafal-ch Oct 17, 2024
9a9f120
Fix the recursive call
rafal-ch Oct 18, 2024
6aa9325
Init indexation progresses with 0 upon metadata migration
rafal-ch Oct 18, 2024
b153db4
Remove debug prints
rafal-ch Oct 18, 2024
512a8a3
Store incoming balance in the new Balances DB
rafal-ch Oct 18, 2024
adf9e2a
Read balance from the new Balances database
rafal-ch Oct 18, 2024
344ca90
Update coin balance, don't overwrite
rafal-ch Oct 18, 2024
f73dbed
Use more detailed `IndexationStatus`, not just block height
rafal-ch Oct 21, 2024
80da09b
Add processing of `MessageImported`
rafal-ch Oct 21, 2024
ad5216d
Simplify processing of coins and message amounts
rafal-ch Oct 21, 2024
c784e44
Extract `increase_balance()`
rafal-ch Oct 21, 2024
5762665
Store coin and message balances separately
rafal-ch Oct 21, 2024
5595985
Clean up column naming
rafal-ch Oct 21, 2024
38dd8d6
Add test for coin balances
rafal-ch Oct 21, 2024
530bcaf
Support both coins and messages in the new balance system
rafal-ch Oct 21, 2024
5604f30
Merge remote-tracking branch 'upstream/master' into 1965_balances
rafal-ch Oct 22, 2024
562c087
update comment
rafal-ch Oct 22, 2024
82f7a16
Remove the database migration hack
rafal-ch Oct 22, 2024
7d0b014
Remove code related to handling base asset id
rafal-ch Oct 22, 2024
1fe3bba
Clean-up of the balance update code
rafal-ch Oct 22, 2024
fe60c98
Remove unused tests
rafal-ch Oct 22, 2024
adef78e
Handle overflow in `balance` query
rafal-ch Oct 22, 2024
9409d52
Clippy and formatting
rafal-ch Oct 22, 2024
13b6db9
Update db tests after reverting changes to metadata
rafal-ch Oct 22, 2024
55f9025
Add comment
rafal-ch Oct 22, 2024
d215844
Add support for `balances()` (plural) query
rafal-ch Oct 22, 2024
53131a3
Handle error properly instead of using `expect()`
rafal-ch Oct 22, 2024
1e24aaa
Add support for `direction` in balances query
rafal-ch Oct 22, 2024
eeea199
Remove safety check from the `balance` query
rafal-ch Oct 22, 2024
e77f33b
Fix balance logging
rafal-ch Oct 22, 2024
c9b7e29
Include message balance in `balances()` query
rafal-ch Oct 22, 2024
076cb42
Revert "Fix balance logging"
rafal-ch Oct 22, 2024
f5e2a6d
Revert a couple of unintentional changes
rafal-ch Oct 22, 2024
d1c6fcc
Rename database `Balances` to `CoinBalances`
rafal-ch Oct 22, 2024
8cc0280
Update comments
rafal-ch Oct 22, 2024
ca32195
Merge remote-tracking branch 'upstream/master' into 1965_balances_cache
rafal-ch Oct 22, 2024
f082291
Fix formatting
rafal-ch Oct 23, 2024
f598395
Merge remote-tracking branch 'upstream/master' into 1965_balances_cache
rafal-ch Oct 24, 2024
8ce4550
Add comment about potential discount on the balances query cost
rafal-ch Oct 24, 2024
4e84ac3
Update DB metadata with balances info
rafal-ch Oct 28, 2024
9577f30
Support both 'new' and 'old' way of querying balances
rafal-ch Oct 28, 2024
a16ef36
Support both 'new' and 'old' way of querying balance
rafal-ch Oct 28, 2024
80fb852
Do not touch the onchain DB metadata
rafal-ch Oct 28, 2024
c94a484
Write balances cache information do off-chain db on genesis
rafal-ch Oct 28, 2024
ddf0571
Use balances cache in GraphQL api
rafal-ch Oct 28, 2024
9078b05
Clean-up error handling
rafal-ch Oct 28, 2024
3b6b8c6
Satisfy clippy and fmt
rafal-ch Oct 28, 2024
5162936
generic update function
rafal-ch Oct 28, 2024
f736ecb
Extract processing of balance updates
rafal-ch Oct 28, 2024
d880e0a
Merge remote-tracking branch 'upstream/master' into 1965_balances_cache
rafal-ch Oct 28, 2024
6927f03
Use instead of for balances key
rafal-ch Oct 29, 2024
66e26d9
Revert the supporsed-to-be temporary change that disabled warnings
rafal-ch Oct 29, 2024
a87698a
Remove unused lifetime from `HasIndexation` trait
rafal-ch Oct 29, 2024
ddadd80
Simplify trait bounds for `HasIndexation` trait
rafal-ch Oct 29, 2024
ccadf41
Further simplify trait bounds for `HasIndexation` trait
rafal-ch Oct 29, 2024
91e8abc
Split and rename the `HasIndexation` trait
rafal-ch Oct 30, 2024
ab7808f
WIP - bail when unable to update balance due to overflow
rafal-ch Oct 31, 2024
90fa259
Prefer `core::fmt::Display` over `std::fmt::Display`
rafal-ch Oct 31, 2024
0b39319
Use named const instead of plain `true` in genesis importer for clarity
rafal-ch Oct 31, 2024
4d6e17a
Remove reduntant write to `DatabaseMetadata::V2`
rafal-ch Oct 31, 2024
8f3e817
Ensure all indexations are enabled at genesis
rafal-ch Oct 31, 2024
895d9da
Fix `produce_block__raises_gas_price` after balance overflow check wa…
rafal-ch Oct 31, 2024
de11071
Fix more integration tests to work with the balance overflow checks
rafal-ch Nov 1, 2024
71226d4
Fix typo
rafal-ch Nov 1, 2024
474e207
Fix BLOB integration tests to work with the balance overflow checks
rafal-ch Nov 1, 2024
ca6057d
Revert "Fix BLOB integration tests to work with the balance overflow …
rafal-ch Nov 2, 2024
4d43038
Revert "Fix more integration tests to work with the balance overflow …
rafal-ch Nov 2, 2024
e3d898d
Revert "Fix `produce_block__raises_gas_price` after balance overflow …
rafal-ch Nov 2, 2024
9848f64
Do not bail when balances cannot be updated, log the error instead
rafal-ch Nov 2, 2024
dae0af5
Total balance of assets is now represented as `u128`
rafal-ch Nov 6, 2024
facbd2e
Infer types in `process_balances_update()`
rafal-ch Nov 6, 2024
9b8e491
Simplify trait bounds in `update_balances()`
rafal-ch Nov 6, 2024
9ea9c02
Merge remote-tracking branch 'upstream/master' into 1965_balances_cache
rafal-ch Nov 6, 2024
54087fc
Fix formatting
rafal-ch Nov 6, 2024
20081fa
Satisfy Clippy
rafal-ch Nov 6, 2024
8b9f4e8
Update tests
rafal-ch Nov 6, 2024
e819e23
Asset balance queries now return U128 instead of U64.
rafal-ch Nov 7, 2024
de16099
Log errors when balance cannot be calculated in the legacy calculatio…
rafal-ch Nov 8, 2024
4f307e3
Mention an balance overflow follow-up issue in the comments
rafal-ch Nov 11, 2024
66d5948
Remove the `TODO` comment
rafal-ch Nov 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/fuel-core/src/coins_query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -986,6 +986,7 @@ mod tests {
let on_chain = self.database.on_chain().clone();
let off_chain = self.database.off_chain().clone();
ServiceDatabase::new(100, 0u32.into(), on_chain, off_chain)
.expect("should create service database")
}
}

Expand Down
1 change: 0 additions & 1 deletion crates/fuel-core/src/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,6 @@ where

Ok(())
}

#[cfg(feature = "rocksdb")]
pub fn convert_to_rocksdb_direction(direction: IterDirection) -> rocksdb::Direction {
match direction {
Expand Down
34 changes: 32 additions & 2 deletions crates/fuel-core/src/database/database_description.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use fuel_core_types::{
blockchain::primitives::DaBlockHeight,
fuel_types::BlockHeight,
};
use std::collections::HashSet;

pub mod gas_price;
pub mod off_chain;
Expand Down Expand Up @@ -67,24 +68,53 @@ pub trait DatabaseDescription: 'static + Copy + Debug + Send + Sync {
fn prefix(column: &Self::Column) -> Option<usize>;
}

#[derive(
Copy, Clone, Debug, serde::Serialize, serde::Deserialize, Eq, PartialEq, Hash,
)]
pub enum IndexationKind {
Balances,
_CoinsToSpend,
}

/// The metadata of the database contains information about the version and its height.
#[derive(Copy, Clone, Debug, serde::Serialize, serde::Deserialize)]
#[derive(Clone, Debug, serde::Serialize, serde::Deserialize)]
pub enum DatabaseMetadata<Height> {
V1 { version: u32, height: Height },
V1 {
version: u32,
height: Height,
},
V2 {
version: u32,
height: Height,
indexation_availability: HashSet<IndexationKind>,
},
}

impl<Height> DatabaseMetadata<Height> {
/// Returns the version of the database.
pub fn version(&self) -> u32 {
match self {
Self::V1 { version, .. } => *version,
Self::V2 { version, .. } => *version,
}
}

/// Returns the height of the database.
pub fn height(&self) -> &Height {
match self {
Self::V1 { height, .. } => height,
Self::V2 { height, .. } => height,
}
}

/// Returns true if the given indexation kind is available.
pub fn indexation_available(&self, kind: IndexationKind) -> bool {
match self {
Self::V1 { .. } => false,
Self::V2 {
indexation_availability,
..
} => indexation_availability.contains(&kind),
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ impl DatabaseDescription for OffChain {
type Column = fuel_core_graphql_api::storage::Column;
type Height = BlockHeight;

// TODO[RC]: Do we bump this due to extended metadata?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it will be right to increase it, but then you need to handle it here:

image

But maybe we don't need version function at all, if we version enum of the metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the comment and decided to keep the version() at 0, as it used to be, because I can't really feel the notion of this version number.

I'm good to update it, remove it or refactor if needed, but preferably in a follow-up PR.

fn version() -> u32 {
0
}
Expand Down
10 changes: 10 additions & 0 deletions crates/fuel-core/src/database/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ use fuel_core_storage::{
StorageInspect,
};

use super::database_description::IndexationKind;

/// The table that stores all metadata about the database.
pub struct MetadataTable<Description>(core::marker::PhantomData<Description>);

Expand Down Expand Up @@ -74,4 +76,12 @@ where

Ok(metadata)
}

pub fn indexation_available(&self, kind: IndexationKind) -> StorageResult<bool> {
let Some(metadata) = self.storage::<MetadataTable<Description>>().get(&())?
else {
return Ok(false)
};
Ok(metadata.indexation_available(kind))
Comment on lines +81 to +85
Copy link
Member

Choose a reason for hiding this comment

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

super nit: Don't you think an if let branch is cleaner? I'm allergic to early returns :P

Suggested change
let Some(metadata) = self.storage::<MetadataTable<Description>>().get(&())?
else {
return Ok(false)
};
Ok(metadata.indexation_available(kind))
let is available = if let Some(metadata) = self.storage::<MetadataTable<Description>>().get(&())? {
metadata.indexation_available(kind)
} else {
false
};
Ok(is_available)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong opinions here, but I'm quite fond of early returns, tbh :)
Maybe in such short functions they don't help much, but if you're ok, I'd prefer to leave this as is.

}
}
2 changes: 1 addition & 1 deletion crates/fuel-core/src/graphql_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ impl Default for Costs {
}

pub const DEFAULT_QUERY_COSTS: Costs = Costs {
balance_query: 40001,
balance_query: 40001, /* Cost will depend on whether balances index is available or not, but let's keep the default high to be on the safe side */
coins_to_spend: 40001,
get_peers: 40001,
estimate_predicates: 40001,
Expand Down
5 changes: 3 additions & 2 deletions crates/fuel-core/src/graphql_api/api_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ use tower_http::{
pub type Service = fuel_core_services::ServiceRunner<GraphqlService>;

pub use super::database::ReadDatabase;
use super::ports::worker;

pub type BlockProducer = Box<dyn BlockProducerPort>;
// In the future GraphQL should not be aware of `TxPool`. It should
Expand Down Expand Up @@ -229,7 +230,7 @@ pub fn new_service<OnChain, OffChain>(
) -> anyhow::Result<Service>
where
OnChain: AtomicView + 'static,
OffChain: AtomicView + 'static,
OffChain: AtomicView + worker::OffChainDatabase + 'static,
OnChain::LatestView: OnChainDatabase,
OffChain::LatestView: OffChainDatabase,
{
Expand All @@ -241,7 +242,7 @@ where
genesis_block_height,
on_database,
off_database,
);
)?;
let request_timeout = config.config.api_request_timeout;
let concurrency_limit = config.config.max_concurrent_queries;
let body_limit = config.config.request_body_bytes_limit;
Expand Down
17 changes: 13 additions & 4 deletions crates/fuel-core/src/graphql_api/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ use std::{
sync::Arc,
};

use super::ports::worker;

mod arc_wrapper;

/// The on-chain view of the database used by the [`ReadView`] to fetch on-chain data.
Expand All @@ -86,6 +88,8 @@ pub struct ReadDatabase {
on_chain: Box<dyn AtomicView<LatestView = OnChainView>>,
/// The off-chain database view provider.
off_chain: Box<dyn AtomicView<LatestView = OffChainView>>,
/// The flag that indicates whether the Balances cache table is enabled.
balances_enabled: bool,
}

impl ReadDatabase {
Expand All @@ -95,19 +99,22 @@ impl ReadDatabase {
genesis_height: BlockHeight,
on_chain: OnChain,
off_chain: OffChain,
) -> Self
) -> Result<Self, StorageError>
where
OnChain: AtomicView + 'static,
OffChain: AtomicView + 'static,
OffChain: AtomicView + worker::OffChainDatabase + 'static,
OnChain::LatestView: OnChainDatabase,
OffChain::LatestView: OffChainDatabase,
{
Self {
let balances_enabled = off_chain.balances_enabled()?;

Ok(Self {
batch_size,
genesis_height,
on_chain: Box::new(ArcWrapper::new(on_chain)),
off_chain: Box::new(ArcWrapper::new(off_chain)),
}
balances_enabled,
})
}

/// Creates a consistent view of the database.
Expand All @@ -120,6 +127,7 @@ impl ReadDatabase {
genesis_height: self.genesis_height,
on_chain: self.on_chain.latest_view()?,
off_chain: self.off_chain.latest_view()?,
balances_enabled: self.balances_enabled,
})
}

Expand All @@ -135,6 +143,7 @@ pub struct ReadView {
pub(crate) genesis_height: BlockHeight,
pub(crate) on_chain: OnChainView,
pub(crate) off_chain: OffChainView,
pub(crate) balances_enabled: bool,
}

impl ReadView {
Expand Down
27 changes: 26 additions & 1 deletion crates/fuel-core/src/graphql_api/ports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,10 @@ use fuel_core_types::{
},
tai64::Tai64,
};
use std::sync::Arc;
use std::{
collections::BTreeMap,
sync::Arc,
};

pub trait OffChainDatabase: Send + Sync {
fn block_height(&self, block_id: &BlockId) -> StorageResult<BlockHeight>;
Expand All @@ -71,6 +74,19 @@ pub trait OffChainDatabase: Send + Sync {

fn tx_status(&self, tx_id: &TxId) -> StorageResult<TransactionStatus>;

fn balance(
&self,
owner: &Address,
asset_id: &AssetId,
base_asset_id: &AssetId,
) -> StorageResult<u64>;

fn balances(
&self,
owner: &Address,
base_asset_id: &AssetId,
) -> StorageResult<BTreeMap<AssetId, u64>>;

fn owned_coins_ids(
&self,
owner: &Address,
Expand Down Expand Up @@ -273,6 +289,10 @@ pub mod worker {
},
},
graphql_api::storage::{
balances::{
CoinBalances,
MessageBalances,
},
da_compression::*,
old::{
OldFuelBlockConsensus,
Expand Down Expand Up @@ -315,6 +335,9 @@ pub mod worker {

/// Creates a write database transaction.
fn transaction(&mut self) -> Self::Transaction<'_>;

/// Checks if Balances cache functionality is available.
fn balances_enabled(&self) -> StorageResult<bool>;
}

pub trait OffChainDatabaseTransaction:
Expand All @@ -327,6 +350,8 @@ pub mod worker {
+ StorageMutate<OldTransactions, Error = StorageError>
+ StorageMutate<SpentMessages, Error = StorageError>
+ StorageMutate<RelayedTransactionStatuses, Error = StorageError>
+ StorageMutate<CoinBalances, Error = StorageError>
+ StorageMutate<MessageBalances, Error = StorageError>
+ StorageMutate<DaCompressedBlocks, Error = StorageError>
+ StorageMutate<DaCompressionTemporalRegistryAddress, Error = StorageError>
+ StorageMutate<DaCompressionTemporalRegistryAssetId, Error = StorageError>
Expand Down
5 changes: 5 additions & 0 deletions crates/fuel-core/src/graphql_api/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use fuel_core_types::{
};
use statistic::StatisticTable;

pub mod balances;
pub mod blocks;
pub mod coins;
pub mod contracts;
Expand Down Expand Up @@ -113,6 +114,10 @@ pub enum Column {
DaCompressionTemporalRegistryScriptCode = 21,
/// See [`DaCompressionTemporalRegistryPredicateCode`](da_compression::DaCompressionTemporalRegistryPredicateCode)
DaCompressionTemporalRegistryPredicateCode = 22,
/// Coin balances per user and asset.
CoinBalances = 23,
/// Message balances per user.
MessageBalances = 24,
Comment on lines +117 to +120
Copy link
Collaborator

Choose a reason for hiding this comment

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

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,
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

impl Column {
Expand Down
76 changes: 76 additions & 0 deletions crates/fuel-core/src/graphql_api/storage/balances.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
use fuel_core_storage::{
blueprint::plain::Plain,
codec::{
postcard::Postcard,
raw::Raw,
},
structured_storage::TableWithBlueprint,
Mappable,
};
use fuel_core_types::{
fuel_tx::{
Address,
AssetId,
},
fuel_vm::double_key,
};
use rand::{
distributions::Standard,
prelude::Distribution,
Rng,
};

pub type Amount = u64;

double_key!(BalancesKey, Address, address, AssetId, asset_id);
impl Distribution<BalancesKey> for Standard {
fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> BalancesKey {
let mut bytes = [0u8; BalancesKey::LEN];
rng.fill_bytes(bytes.as_mut());
BalancesKey::from_array(bytes)
}
}

impl core::fmt::Display for BalancesKey {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR, but is there any benefit in putting the Display implementation inside the double_key macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was considering this, but after giving this a second thought I came to the conclusion that Display implementation is often pretty specific to a particular use-case.

For example, if one part is the "private key" I may want to display ***** in the log. If Display is automatically provided, this may go unnoticed.

fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
write!(f, "address={} asset_id={}", self.address(), self.asset_id())
}
}

/// This table stores the balances of coins per owner and asset id.
pub struct CoinBalances;

impl Mappable for CoinBalances {
type Key = BalancesKey;
type OwnedKey = Self::Key;
type Value = Amount;
type OwnedValue = Self::Value;
}

impl TableWithBlueprint for CoinBalances {
type Blueprint = Plain<Raw, Postcard>;
type Column = super::Column;

fn column() -> Self::Column {
Self::Column::CoinBalances
}
}

/// This table stores the balances of messages per owner.
pub struct MessageBalances;

impl Mappable for MessageBalances {
type Key = Address;
type OwnedKey = Self::Key;
type Value = Amount;
type OwnedValue = Self::Value;
}

impl TableWithBlueprint for MessageBalances {
type Blueprint = Plain<Raw, Postcard>;
type Column = super::Column;

fn column() -> Self::Column {
Self::Column::MessageBalances
}
}
Loading
Loading