Skip to content

Conversation

@t1plarnd
Copy link
Collaborator

No description provided.

@t1plarnd t1plarnd requested a review from KyrylR as a code owner January 16, 2026 15:24
@KyrylR KyrylR added the greptile Mark this PR to be reviewed by Greptile label Jan 16, 2026
@greptile-apps
Copy link

greptile-apps bot commented Jan 16, 2026

Greptile Summary

Adds Criterion-based benchmarks for the coin-store crate to measure UTXO query performance under different filter configurations (default, entropy, contracts, token tags).

The PR includes:

  • Shared benchmark setup code (common.rs) that creates a test database with 10 transactions containing options contracts
  • Four separate benchmark suites testing query performance with increasingly complex filters
  • Removal of unused hex dev-dependency
  • Addition of simplicityhl-core as a regular dependency for testnet constants

The benchmarks follow a standard pattern: setup database with test data, run queries with black_box to prevent optimization, then cleanup. Each benchmark configures criterion with 10 samples and 10-second measurement windows.

Confidence Score: 4/5

  • This PR is safe to merge with minor style improvements recommended
  • The PR adds well-structured benchmark code that follows Criterion best practices. The implementation is functionally correct and won't affect production code. Score reduced by 1 due to minor style issues: hardcoded database path that could cause race conditions if benchmarks run concurrently, use of identical keypairs limiting data diversity, and an empty error message.
  • Pay attention to crates/coin-store/benches/common.rs for the style improvements mentioned in comments

Important Files Changed

Filename Overview
crates/coin-store/Cargo.toml Added benchmark dependencies and configuration, removed unused hex dependency
crates/coin-store/benches/common.rs Created shared benchmark setup code with database and filter initialization

Sequence Diagram

sequenceDiagram
    participant B as Benchmark Suite
    participant C as common::setup_db()
    participant S as Store
    participant DB as SQLite Database
    
    B->>C: Initialize benchmark
    C->>DB: Remove old /tmp/benchmark_stress.db
    C->>S: Store::create()
    S->>DB: Create tables and run migrations
    
    loop 10 iterations
        C->>C: Generate keypair (same key)
        C->>C: setup_tx_and_contract()
        C->>C: Generate random entropy
        C->>C: Build option creation tx
        C->>S: insert_transaction(tx, blinder_keys)
        S->>DB: Mark inputs as spent (noop)
        S->>DB: Insert UTXOs
        C->>S: add_contract(source, args, tpg)
        S->>DB: Store contract metadata
        C->>S: insert_contract_token(tpg, asset_id)
        S->>DB: Store token mapping
        C->>C: Build filters (default, entropy, contracts, token)
    end
    
    C-->>B: Return (store, filters, db_path)
    
    loop Benchmark iterations
        B->>S: query_utxos(filters)
        S->>DB: Execute SQL queries with filters
        DB-->>S: Return matching UTXOs
        S-->>B: UtxoQueryResult
    end
    
    B->>DB: Remove /tmp/benchmark_stress.db
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

6 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile


for _i in 0..10 {
let secp = Secp256k1::new();
let keypair = Keypair::from_secret_key(&secp, &SecretKey::from_slice(&[1u8; 32]).unwrap());
Copy link

Choose a reason for hiding this comment

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

style: same keypair used in every iteration - all UTXOs will have identical blinding keys, which may not accurately represent real-world query performance with diverse data

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/coin-store/benches/common.rs
Line: 95:95

Comment:
**style:** same keypair used in every iteration - all UTXOs will have identical blinding keys, which may not accurately represent real-world query performance with diverse data

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main mission of thins fn is to set up a database for query testing, and in my opinion, it does not affect query processing time.

store
.insert_contract_token(&tpg_for_token.clone(), option_asset_id, "Name of token")
.await
.expect("");
Copy link

Choose a reason for hiding this comment

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

style: empty error message won't help debug if insert_contract_token fails

Suggested change
.expect("");
.expect("Failed to insert contract token");
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/coin-store/benches/common.rs
Line: 122:122

Comment:
**style:** empty error message won't help debug if `insert_contract_token` fails

```suggestion
            .expect("Failed to insert contract token");
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is my mistake, I will fix it.its my bad, i will fix that

(Vec<UtxoFilter>, Vec<UtxoFilter>, Vec<UtxoFilter>, Vec<UtxoFilter>),
String,
) {
let path = "/tmp/benchmark_stress.db";
Copy link

Choose a reason for hiding this comment

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

style: hardcoded database path could cause conflicts if multiple benchmarks run concurrently - consider using a unique path per benchmark or test run

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/coin-store/benches/common.rs
Line: 84:84

Comment:
**style:** hardcoded database path could cause conflicts if multiple benchmarks run concurrently - consider using a unique path per benchmark or test run

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my opinion, the main way to optimize a database is to configure it to use one (possibly large) function and only configure filters. So if I have one database with a dataset, I don't need to store it in different places.

Store,
(Vec<UtxoFilter>, Vec<UtxoFilter>, Vec<UtxoFilter>, Vec<UtxoFilter>),
String,
) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's Result here to avoid unwraps


mod common;

fn criterion_benchmark(c: &mut Criterion) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to make this function async?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

greptile Mark this PR to be reviewed by Greptile

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants