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

katana: simplify database backend by removing in-memory provider #2571

Merged
merged 4 commits into from
Oct 23, 2024
Merged

katana: simplify database backend by removing in-memory provider #2571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
63d0330
feat(katana-db): ephemeral database environment (#2567)
kariy Oct 22, 2024
59c6dcf
refactor(katana): remove `InMemoryProvider` (#2569)
kariy Oct 22, 2024
81b6fa7
feat(katana): implement `Arbitrary` trait for all db types (#2568)
kariy Oct 22, 2024
a3bdf5e
Merge branch 'main' into katana/simplify-in-memory-provider
kariy Oct 22, 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
41 changes: 40 additions & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ sozo-signers = { path = "crates/sozo/signers" }
sozo-walnut = { path = "crates/sozo/walnut" }

anyhow = "1.0.89"
arbitrary = { version = "1.3.2", features = [ "derive" ] }
assert_fs = "1.1"
assert_matches = "1.5.0"
async-trait = "0.1.82"
Expand Down Expand Up @@ -244,4 +245,4 @@ alloy-transport = { version = "0.3", default-features = false }

starknet = "0.12.0"
starknet-crypto = "0.7.1"
starknet-types-core = "0.1.6"
starknet-types-core = { version = "0.1.6", features = [ "arbitrary" ] }
5 changes: 5 additions & 0 deletions crates/katana/cairo/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,8 @@ cairo-lang-starknet-classes = "2.7.0"
cairo-lang-utils = "2.7.0"
cairo-vm = "1.0.1"
starknet_api = { git = "https://github.com/dojoengine/sequencer", tag = "v0.8.0-rc3.2" }

[features]
# Some types that we used from cairo-vm implements the `Arbitrary` trait,
# only under the `test_utils` feature. So we expose through this feature.
cairo-vm-test-utils = [ "cairo-vm/test_utils" ]
15 changes: 10 additions & 5 deletions crates/katana/core/src/backend/storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
BlockHashOrNumber, BlockIdOrTag, FinalityStatus, SealedBlockWithStatus,
};
use katana_primitives::chain_spec::ChainSpec;
use katana_primitives::da::L1DataAvailabilityMode;
use katana_primitives::state::StateUpdatesWithDeclaredClasses;
use katana_primitives::version::ProtocolVersion;
use katana_provider::providers::db::DbProvider;
Expand Down Expand Up @@ -182,7 +183,12 @@
forked_block.l1_data_gas_price.price_in_wei.to_u128().expect("should fit in u128");
block.header.l1_data_gas_prices.strk =
forked_block.l1_data_gas_price.price_in_fri.to_u128().expect("should fit in u128");
block.header.l1_da_mode = forked_block.l1_da_mode;
block.header.l1_da_mode = match forked_block.l1_da_mode {
starknet::core::types::L1DataAvailabilityMode::Blob => L1DataAvailabilityMode::Blob,

Check warning on line 187 in crates/katana/core/src/backend/storage.rs

View check run for this annotation

Codecov / codecov/patch

crates/katana/core/src/backend/storage.rs#L186-L187 

Added lines #L186 - L187 were not covered by tests
starknet::core::types::L1DataAvailabilityMode::Calldata => {
L1DataAvailabilityMode::Calldata

Check warning on line 189 in crates/katana/core/src/backend/storage.rs

View check run for this annotation

Codecov / codecov/patch

crates/katana/core/src/backend/storage.rs#L189 

Added line #L189 was not covered by tests
}
};
Comment on lines +186 to +191
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Clean type conversion implementation, sensei!

The match statement properly handles the conversion between different L1DataAvailabilityMode types. However, we could make it more concise.

Consider simplifying the match statement:

-        block.header.l1_da_mode = match forked_block.l1_da_mode {
-            starknet::core::types::L1DataAvailabilityMode::Blob => L1DataAvailabilityMode::Blob,
-            starknet::core::types::L1DataAvailabilityMode::Calldata => {
-                L1DataAvailabilityMode::Calldata
-            }
-        };
+        block.header.l1_da_mode = match forked_block.l1_da_mode {
+            starknet::core::types::L1DataAvailabilityMode::Blob => L1DataAvailabilityMode::Blob,
+            starknet::core::types::L1DataAvailabilityMode::Calldata => L1DataAvailabilityMode::Calldata,
+        };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
block.header.l1_da_mode = match forked_block.l1_da_mode {
starknet::core::types::L1DataAvailabilityMode::Blob => L1DataAvailabilityMode::Blob,
starknet::core::types::L1DataAvailabilityMode::Calldata => {
L1DataAvailabilityMode::Calldata
}
};
block.header.l1_da_mode = match forked_block.l1_da_mode {
starknet::core::types::L1DataAvailabilityMode::Blob => L1DataAvailabilityMode::Blob,
starknet::core::types::L1DataAvailabilityMode::Calldata => L1DataAvailabilityMode::Calldata,
};


let block = block.seal_with_hash_and_status(forked_block.block_hash, status);
let state_updates = chain.state_updates();
Expand Down Expand Up @@ -216,7 +222,7 @@
Block, FinalityStatus, GasPrices, Header, SealedBlockWithStatus,
};
use katana_primitives::da::L1DataAvailabilityMode;
use katana_primitives::fee::TxFeeInfo;
use katana_primitives::fee::{PriceUnit, TxFeeInfo};
use katana_primitives::genesis::constant::{
DEFAULT_ETH_FEE_TOKEN_ADDRESS, DEFAULT_LEGACY_ERC20_CASM, DEFAULT_LEGACY_ERC20_CLASS_HASH,
DEFAULT_LEGACY_UDC_CASM, DEFAULT_LEGACY_UDC_CLASS_HASH, DEFAULT_UDC_ADDRESS,
Expand All @@ -226,20 +232,19 @@
use katana_primitives::trace::TxExecInfo;
use katana_primitives::transaction::{InvokeTx, Tx, TxWithHash};
use katana_primitives::{chain_spec, Felt};
use katana_provider::providers::in_memory::InMemoryProvider;
use katana_provider::providers::db::DbProvider;
use katana_provider::traits::block::{
BlockHashProvider, BlockNumberProvider, BlockProvider, BlockWriter,
};
use katana_provider::traits::state::StateFactoryProvider;
use katana_provider::traits::transaction::{TransactionProvider, TransactionTraceProvider};
use starknet::core::types::PriceUnit;
use starknet::macros::felt;

use super::Blockchain;

#[test]
fn blockchain_from_genesis_states() {
let provider = InMemoryProvider::new();
let provider = DbProvider::new_ephemeral();

let blockchain = Blockchain::new_with_chain(provider, &chain_spec::DEV)
.expect("failed to create blockchain from genesis block");
Expand Down
2 changes: 1 addition & 1 deletion crates/katana/executor/benches/concurrent.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ fn concurrent(c: &mut Criterion) {
let mut group = c.benchmark_group("Concurrent.Simulate");
group.warm_up_time(Duration::from_millis(200));

let provider = test_utils::test_in_memory_provider();
let provider = test_utils::test_provider();
let flags = SimulationFlag::new().skip_validate();

let tx = tx();
Expand Down
2 changes: 1 addition & 1 deletion crates/katana/executor/benches/execution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ fn executor_transact(c: &mut Criterion) {
let mut group = c.benchmark_group("Invoke.ERC20.transfer");
group.warm_up_time(Duration::from_millis(200));

let provider = test_utils::test_in_memory_provider();
let provider = test_utils::test_provider();
let flags = SimulationFlag::new();
Comment on lines +21 to 22
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding warm cache benchmarks

Since we're now using a database backend, it would be valuable to benchmark both cold and warm cache scenarios to understand the performance characteristics better.

Consider adding a warm cache variant like this:

 let provider = test_utils::test_provider();
 let flags = SimulationFlag::new();
+
+// Add warm cache benchmark
+let warm_provider = provider.clone();
+// Pre-warm the cache by executing a dummy transaction
+blockifier(&mut group, &warm_provider, &flags, &envs, tx.clone());

Committable suggestion was skipped due to low confidence.


let tx = tx();
Expand Down
6 changes: 3 additions & 3 deletions crates/katana/executor/src/implementation/blockifier/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ mod tests {
};
use katana_primitives::utils::class::{parse_compiled_class, parse_sierra_class};
use katana_primitives::{address, Felt};
use katana_provider::providers::in_memory::InMemoryProvider;
use katana_provider::providers::db::DbProvider;
use katana_provider::traits::contract::ContractClassWriter;
use katana_provider::traits::state::{StateFactoryProvider, StateProvider, StateWriter};
use starknet::macros::felt;
Expand Down Expand Up @@ -267,7 +267,7 @@ mod tests {
let legacy_class_hash = felt!("0x111");
let legacy_class = DEFAULT_LEGACY_ERC20_CASM.clone();

let provider = InMemoryProvider::new();
let provider = katana_provider::test_utils::test_provider();
provider.set_nonce(address, nonce).unwrap();
provider.set_class_hash_of_contract(address, class_hash).unwrap();
provider.set_storage(address, storage_key, storage_value).unwrap();
Expand Down Expand Up @@ -452,7 +452,7 @@ mod tests {

#[test]
fn fetch_non_existant_data() -> anyhow::Result<()> {
let db = InMemoryProvider::new();
let db = DbProvider::new_ephemeral();
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Ohayo sensei! After analyzing the codebase, I can now provide a final response about the InMemoryProvider usage.

The InMemoryProvider references need to be updated in test templates

The test files still use InMemoryProvider in their template definitions, specifically in:

  • class.rs: test_historical_class_read template
  • contract.rs: test_historical_storage_read template
  • storage.rs: test_historical_storage_read template

However, these templates are already being correctly applied with DbProvider in the actual test implementations. The InMemoryProvider is only referenced in the template definitions but not actually used in the running tests, as evidenced by the working test implementations using DbProvider.

• crates/katana/storage/provider/tests/class.rs: Update template parameter from InMemoryProvider to DbProvider
• crates/katana/storage/provider/tests/contract.rs: Update template parameter from InMemoryProvider to DbProvider
• crates/katana/storage/provider/tests/storage.rs: Update template parameter from InMemoryProvider to DbProvider

🔗 Analysis chain

Good choice using ephemeral storage for tests, sensei!

The switch to DbProvider::new_ephemeral() maintains the temporary nature of test storage while using the standardized database backend. The edge case handling for non-existent storage keys is well documented in the test.

Let's verify that all test cases using InMemoryProvider have been updated:

Let me gather more context about these test files to understand if they need to be updated.

Let me check if there are any other files that might be using InMemoryProvider, particularly in the fixtures module that's being imported by these test files.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining instances of InMemoryProvider in test files
rg "InMemoryProvider" --type rust --type-add "rust:*.rs"

Length of output: 457

Script:

#!/bin/bash
# Check the content of these test files to understand the context
echo "=== class.rs ==="
cat crates/katana/storage/provider/tests/class.rs
echo -e "\n=== contract.rs ==="
cat crates/katana/storage/provider/tests/contract.rs
echo -e "\n=== storage.rs ==="
cat crates/katana/storage/provider/tests/storage.rs

Length of output: 18575

Script:

#!/bin/bash
# Check the fixtures module content
echo "=== fixtures.rs ==="
cat crates/katana/storage/provider/tests/fixtures.rs

Length of output: 7421


let address = address!("0x1");
let class_hash = felt!("0x123");
Expand Down
27 changes: 19 additions & 8 deletions crates/katana/executor/src/implementation/blockifier/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,14 @@
};
use katana_primitives::chain::NamedChainId;
use katana_primitives::env::{BlockEnv, CfgEnv};
use katana_primitives::fee::TxFeeInfo;
use katana_primitives::fee::{PriceUnit, TxFeeInfo};
use katana_primitives::state::{StateUpdates, StateUpdatesWithDeclaredClasses};
use katana_primitives::trace::{L1Gas, TxExecInfo, TxResources};
use katana_primitives::transaction::{
DeclareTx, DeployAccountTx, ExecutableTx, ExecutableTxWithHash, InvokeTx, TxType,
};
use katana_primitives::{class, event, message, trace, Felt};
use katana_provider::traits::contract::ContractClassProvider;
use starknet::core::types::PriceUnit;
use starknet::core::utils::parse_cairo_short_string;

use super::state::{CachedState, StateDb};
Expand Down Expand Up @@ -480,15 +479,15 @@
}
}

fn to_api_da_mode(mode: starknet::core::types::DataAvailabilityMode) -> DataAvailabilityMode {
fn to_api_da_mode(mode: katana_primitives::da::DataAvailabilityMode) -> DataAvailabilityMode {
match mode {
starknet::core::types::DataAvailabilityMode::L1 => DataAvailabilityMode::L1,
starknet::core::types::DataAvailabilityMode::L2 => DataAvailabilityMode::L2,
katana_primitives::da::DataAvailabilityMode::L1 => DataAvailabilityMode::L1,
katana_primitives::da::DataAvailabilityMode::L2 => DataAvailabilityMode::L2,

Check warning on line 485 in crates/katana/executor/src/implementation/blockifier/utils.rs

View check run for this annotation

Codecov / codecov/patch

crates/katana/executor/src/implementation/blockifier/utils.rs#L485 

Added line #L485 was not covered by tests
}
}

fn to_api_resource_bounds(
resource_bounds: starknet::core::types::ResourceBoundsMapping,
resource_bounds: katana_primitives::fee::ResourceBoundsMapping,
) -> ResourceBoundsMapping {
let l1_gas = ResourceBounds {
max_amount: resource_bounds.l1_gas.max_amount,
Expand Down Expand Up @@ -572,7 +571,9 @@
actual_fee: exec_info.transaction_receipt.fee.0,
revert_error: exec_info.revert_error.clone(),
actual_resources: TxResources {
vm_resources: exec_info.transaction_receipt.resources.vm_resources,
vm_resources: to_execution_resources(
exec_info.transaction_receipt.resources.vm_resources,
),
n_reverted_steps: exec_info.transaction_receipt.resources.n_reverted_steps,
data_availability: L1Gas {
l1_gas: exec_info.transaction_receipt.da_gas.l1_data_gas,
Expand Down Expand Up @@ -626,7 +627,7 @@
entry_point_type,
calldata,
retdata,
execution_resources: call.resources,
execution_resources: to_execution_resources(call.resources),
events,
l2_to_l1_messages: l1_msg,
storage_read_values,
Expand Down Expand Up @@ -655,6 +656,16 @@
message::OrderedL2ToL1Message { order, from_address, to_address, payload }
}

fn to_execution_resources(
resources: ExecutionResources,
) -> katana_primitives::trace::ExecutionResources {
katana_primitives::trace::ExecutionResources {
n_steps: resources.n_steps,
n_memory_holes: resources.n_memory_holes,
builtin_instance_counter: resources.builtin_instance_counter,
}
}

#[cfg(test)]
mod tests {

Expand Down
6 changes: 3 additions & 3 deletions crates/katana/executor/tests/fixtures/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use katana_primitives::transaction::{
use katana_primitives::utils::class::{parse_compiled_class, parse_sierra_class};
use katana_primitives::version::CURRENT_STARKNET_VERSION;
use katana_primitives::{address, Felt};
use katana_provider::providers::in_memory::InMemoryProvider;
use katana_provider::providers::db::DbProvider;
use katana_provider::traits::block::BlockWriter;
use katana_provider::traits::state::{StateFactoryProvider, StateProvider};
use starknet::macros::felt;
Expand Down Expand Up @@ -70,7 +70,7 @@ pub fn chain() -> ChainSpec {
#[rstest::fixture]
pub fn state_provider(chain: &ChainSpec) -> Box<dyn StateProvider> {
let states = chain.state_updates();
let provider = InMemoryProvider::new();
let provider = DbProvider::new_ephemeral();

let block = SealedBlockWithStatus {
status: FinalityStatus::AcceptedOnL2,
Expand All @@ -81,7 +81,7 @@ pub fn state_provider(chain: &ChainSpec) -> Box<dyn StateProvider> {
.insert_block_with_states_and_receipts(block, states, vec![], vec![])
.expect("able to insert block");

<InMemoryProvider as StateFactoryProvider>::latest(&provider).unwrap()
provider.latest().unwrap()
}

// TODO: update the txs to include valid signatures
Expand Down
2 changes: 1 addition & 1 deletion crates/katana/executor/tests/simulate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ use fixtures::{executor_factory, state_provider};
use katana_executor::{ExecutionOutput, ExecutorFactory, SimulationFlag};
use katana_primitives::block::GasPrices;
use katana_primitives::env::BlockEnv;
use katana_primitives::fee::PriceUnit;
use katana_primitives::transaction::ExecutableTxWithHash;
use katana_provider::traits::state::StateProvider;
use rstest_reuse::{self, *};
use starknet::core::types::PriceUnit;
use starknet::macros::felt;

#[rstest::fixture]
Expand Down
1 change: 0 additions & 1 deletion crates/katana/node/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ katana-executor.workspace = true
katana-pipeline.workspace = true
katana-pool.workspace = true
katana-primitives.workspace = true
katana-provider.workspace = true
katana-rpc.workspace = true
katana-rpc-api.workspace = true
katana-tasks.workspace = true
Expand Down
4 changes: 2 additions & 2 deletions crates/katana/node/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ use katana_pool::ordering::FiFo;
use katana_pool::validation::stateful::TxValidator;
use katana_pool::TxPool;
use katana_primitives::env::{CfgEnv, FeeTokenAddressses};
use katana_provider::providers::in_memory::InMemoryProvider;
use katana_rpc::dev::DevApi;
use katana_rpc::metrics::RpcServerMetrics;
use katana_rpc::saya::SayaApi;
Expand Down Expand Up @@ -187,7 +186,8 @@ pub async fn build(mut config: Config) -> Result<Node> {
let db = katana_db::init_db(db_path)?;
(Blockchain::new_with_db(db.clone(), &config.chain)?, Some(db))
} else {
(Blockchain::new_with_chain(InMemoryProvider::new(), &config.chain)?, None)
let db = katana_db::init_ephemeral_db()?;
(Blockchain::new_with_db(db.clone(), &config.chain)?, Some(db))
Comment on lines +189 to +190
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Ohayo sensei! We need to clean up the remaining InMemoryProvider references!

The verification revealed that while the main code has moved to using ephemeral DB, there are still test files using InMemoryProvider. The test files need to be updated to use DbProvider instead, as they already have the test cases duplicated for both providers.

Points that need attention:

  • test_historical_storage_read in storage.rs still uses InMemoryProvider
  • test_historical_storage_read in contract.rs still uses InMemoryProvider
  • test_historical_class_read in class.rs still uses InMemoryProvider
  • The in_memory module still exists but appears to be used only for caching (cache.rs) and state management (state.rs)
🔗 Analysis chain

Ohayo sensei! The database initialization change looks good!

The replacement of the in-memory provider with an ephemeral database aligns perfectly with the PR's objective of simplifying the database backend. This change standardizes the database interaction pattern across the codebase.

Let's verify that all in-memory provider references have been properly removed:

The previous results show some references to InMemoryProvider in tests and imports from in_memory modules. Let's dig deeper to understand if these are legitimate remaining usages or if they need to be cleaned up as part of the in-memory provider removal.

The previous results show the existence of in_memory related code and test references. Let's examine the actual content of these files to understand if they are part of the old implementation that needs to be removed or if they serve a different purpose.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to InMemoryProvider
# to ensure complete removal across the codebase

echo "Checking for InMemoryProvider references..."
rg "InMemoryProvider" 

echo "Checking for in-memory related imports..."
rg "use.*in_memory" 

echo "Checking for in-memory related test configurations..."
rg "in_memory.*provider"

Length of output: 1461

Script:

#!/bin/bash
# Let's check for the actual InMemoryProvider implementation and its usage

# Check for InMemoryProvider struct/type definition
echo "=== Checking InMemoryProvider definition ==="
ast-grep --pattern 'struct InMemoryProvider'

# Check for InMemoryProvider imports in test files
echo -e "\n=== Checking InMemoryProvider imports in tests ==="
rg "use.*InMemoryProvider" -A 2

# Check for any remaining in-memory provider related code
echo -e "\n=== Checking in-memory provider module structure ==="
fd "in_memory" --type d

# Check content of in_memory related files
echo -e "\n=== Checking content of in_memory related files ==="
fd "in_memory" --type f --exec echo "=== {} ===" \; --exec head -n 5 {} \;

Length of output: 695

Script:

#!/bin/bash
# Let's examine the specific files that still contain InMemoryProvider references

echo "=== Checking the content of test files with InMemoryProvider ==="
cat crates/katana/storage/provider/tests/storage.rs
echo -e "\n=== Checking the content of contract test ==="
cat crates/katana/storage/provider/tests/contract.rs
echo -e "\n=== Checking the content of class test ==="
cat crates/katana/storage/provider/tests/class.rs

echo -e "\n=== Checking the in_memory module content ==="
cat crates/katana/storage/provider/src/providers/in_memory/mod.rs

echo -e "\n=== Checking if InMemoryProvider is still exported in lib.rs ==="
rg "pub.*InMemoryProvider" crates/katana/storage/provider/src/lib.rs

Length of output: 19192

};

let block_context_generator = BlockContextGenerator::default().into();
Expand Down
Loading
Loading