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

Bugfix: set prefix extractor for all columns that need it #1004

Merged
merged 2 commits into from
Feb 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions crates/fuel-core/src/schema/coin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use crate::{
U64,
},
};
use anyhow::anyhow;
use async_graphql::{
connection::{
Connection,
Expand Down Expand Up @@ -100,6 +101,13 @@ impl CoinQuery {
last: Option<i32>,
before: Option<String>,
) -> async_graphql::Result<Connection<UtxoId, Coin, EmptyFields, EmptyFields>> {
// Rocksdb doesn't support reverse iteration over a prefix
if matches!(last, Some(last) if last > 0) {
return Err(
anyhow!("reverse pagination isn't supported for this resource").into(),
)
}

let query = CoinQueryContext(ctx.data_unchecked());
crate::schema::query_pagination(after, before, first, last, |start, direction| {
let owner: fuel_tx::Address = filter.owner.into();
Expand Down
9 changes: 9 additions & 0 deletions crates/fuel-core/src/schema/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::{
U64,
},
};
use anyhow::anyhow;
use async_graphql::{
connection::{
Connection,
Expand Down Expand Up @@ -133,6 +134,14 @@ impl ContractBalanceQuery {
Connection<AssetId, ContractBalance, EmptyFields, EmptyFields>,
> {
let query = ContractQueryContext(ctx.data_unchecked());

// Rocksdb doesn't support reverse iteration over a prefix
if matches!(last, Some(last) if last > 0) {
return Err(
anyhow!("reverse pagination isn't supported for this resource").into(),
)
}

crate::schema::query_pagination(after, before, first, last, |start, direction| {
let balances = query
.contract_balances(
Expand Down
9 changes: 9 additions & 0 deletions crates/fuel-core/src/schema/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use super::{
},
};
use crate::query::MessageQueryContext;
use anyhow::anyhow;
use async_graphql::{
connection::{
Connection,
Expand Down Expand Up @@ -86,6 +87,14 @@ impl MessageQuery {
let start = *start;

let messages = if let Some(owner) = owner {
// Rocksdb doesn't support reverse iteration over a prefix
if matches!(last, Some(last) if last > 0) {
return Err(anyhow!(
"reverse pagination isn't supported for this resource"
)
.into())
}

query
.owned_messages(&owner.0, start.map(Into::into), direction)
.into_boxed()
Expand Down
8 changes: 8 additions & 0 deletions crates/fuel-core/src/schema/tx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use crate::{
},
state::IterDirection,
};
use anyhow::anyhow;
use async_graphql::{
connection::{
Connection,
Expand Down Expand Up @@ -153,6 +154,13 @@ impl TxQuery {
before: Option<String>,
) -> async_graphql::Result<Connection<TxPointer, Transaction, EmptyFields, EmptyFields>>
{
// Rocksdb doesn't support reverse iteration over a prefix
if matches!(last, Some(last) if last > 0) {
return Err(
anyhow!("reverse pagination isn't supported for this resource").into(),
)
}

let query = TransactionQueryContext(ctx.data_unchecked());
let owner = fuel_types::Address::from(owner);

Expand Down
12 changes: 11 additions & 1 deletion crates/fuel-core/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,17 @@ impl FuelService {
// initialize database
let database = match config.database_type {
#[cfg(feature = "rocksdb")]
DbType::RocksDb => Database::open(&config.database_path)?,
DbType::RocksDb => {
// use a default tmp rocksdb if no path is provided
if config.database_path.as_os_str().is_empty() {
warn!(
"No RocksDB path configured, initializing database with a tmp directory"
);
Database::default()
} else {
Database::open(&config.database_path)?
}
}
DbType::InMemory => Database::in_memory(),
#[cfg(not(feature = "rocksdb"))]
_ => Database::in_memory(),
Expand Down
3 changes: 3 additions & 0 deletions crates/fuel-core/src/service/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ impl Config {
Self {
addr: SocketAddr::new(Ipv4Addr::new(127, 0, 0, 1).into(), 0),
database_path: Default::default(),
#[cfg(feature = "rocksdb")]
database_type: DbType::RocksDb,
#[cfg(not(feature = "rocksdb"))]
database_type: DbType::InMemory,
chain_conf: chain_conf.clone(),
manual_blocks_enabled: false,
Expand Down
7 changes: 6 additions & 1 deletion crates/fuel-core/src/state/rocks_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,13 @@ impl RocksDb {
opts.create_if_missing(true);
opts.set_compression_type(DBCompressionType::Lz4);

// All double-keys should be configured here
match column {
Column::OwnedCoins | Column::TransactionsByOwnerBlockIdx => {
Column::OwnedCoins
| Column::TransactionsByOwnerBlockIdx
| Column::OwnedMessageIds
| Column::ContractsAssets
| Column::ContractsState => {
Comment on lines +131 to +135
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

// prefix is address length
opts.set_prefix_extractor(SliceTransform::create_fixed_prefix(32))
}
Expand Down
7 changes: 6 additions & 1 deletion tests/tests/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,12 @@ async fn test_contract_balance(
#[rstest]
#[tokio::test]
async fn test_5_contract_balances(
#[values(PageDirection::Forward, PageDirection::Backward)] direction: PageDirection,
#[values(PageDirection::Forward)] direction: PageDirection,
// #[values(PageDirection::Forward, PageDirection::Backward)] direction: PageDirection,
// Rocksdb doesn't support reverse seeks using a prefix, we'd need to implement a custom
// comparator to support this usecase.
// > One common bug of using prefix iterating is to use prefix mode to iterate in reverse order. But it is not yet supported.
// https://github.com/facebook/rocksdb/wiki/Prefix-Seek#limitation
) {
let mut test_builder = TestSetupBuilder::new(SEED);
let (_, contract_id) = test_builder.setup_contract(
Expand Down
14 changes: 13 additions & 1 deletion tests/tests/messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ async fn messages_by_owner_returns_messages_for_the_given_owner() {
// create some owners
let owner_a = Address::new([1; 32]);
let owner_b = Address::new([2; 32]);
let owner_c = Address::new([3; 32]);

// create some messages for owner A
let first_msg = MessageConfig {
Expand Down Expand Up @@ -138,12 +139,23 @@ async fn messages_by_owner_returns_messages_for_the_given_owner() {
assert_eq!(result.results.len(), 1);

assert_eq!(result.results[0].recipient.0 .0, owner_b);

// get the messages from Owner C
let result = client
.messages(Some(&owner_c.to_string()), request.clone())
.await
.unwrap();

// verify that Owner C has no messages
assert_eq!(result.results.len(), 0);
}

#[rstest]
#[tokio::test]
async fn messages_empty_results_for_owner_with_no_messages(
#[values(PageDirection::Forward, PageDirection::Backward)] direction: PageDirection,
#[values(PageDirection::Forward)] direction: PageDirection,
//#[values(PageDirection::Forward, PageDirection::Backward)] direction: PageDirection,
// reverse iteration with prefix not supported by rocksdb
#[values(Address::new([16; 32]), Address::new([0; 32]))] owner: Address,
) {
let srv = FuelService::new_node(Config::local_node()).await.unwrap();
Expand Down