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

feat(core): multi-pubkey DB support #2093

Open
wants to merge 207 commits into
base: dev
Choose a base branch
from
Open

feat(core): multi-pubkey DB support #2093

wants to merge 207 commits into from

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Apr 1, 2024

This pull request enables support for managing multiple public keys in mm2 databases. Initially, a single database instance is generated using the startup public key rmd160. With this update, the mm_ctx will manage multiple database instances by leveraging the public key from MmCoin::account_db_id if defined, or defaulting to the mm2 startup public key.

Completed

  • Introduced sql_connection_pool for managing all instances of shared and sqlite connections from one place
  • Introduced async_sql_connection_pool for managing all instances of async sqlite connections from one place
  • Refactored indexeddb driver implementation to allow multi pubkey support
  • Refactored SwapContext, MySwapStorage, MakerSwapStorage, TakerSwapStorage NftCtx, UtxoBlockHeaderStorage, Zcoin storages(web only), lp_ordermatch and other storage impl to support multipubkey
  • Implement database migrations upon new database instance creation or account activation using a custom public key.
  • swap_kick_start and orders_kick_start unfinished swaps/orders upon new database instance creation or account activation using a custom public key.

Checklists for next PR

  • TODO: discuss and enhance get_public_key, get_public_key_hash, show_priv_key rpc for multi pubkey dbs in a another PR

Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan onur-ozkan added enhancement New feature or request in progress Changes will be made from the author labels Apr 1, 2024
@borngraced borngraced self-assigned this Apr 9, 2024
@borngraced borngraced marked this pull request as ready for review April 10, 2024 20:01
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Next review iteration!

Comment on lines 502 to 511
#[cfg(not(target_arch = "wasm32"))]
{
let dbdir = ctx.dbdir();
fs::create_dir_all(&dbdir).map_to_mm(|e| MmInitError::ErrorCreatingDbDir {
path: dbdir.clone(),
error: e.to_string(),
})?;
}

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 the base db dir should be created here to allow the below to work

pub(super) async fn save_encrypted_passphrase(
ctx: &MmArc,
wallet_name: &str,
encrypted_passphrase_data: &EncryptedData,
) -> WalletsStorageResult<()> {
let wallet_path = ctx.wallet_file_path(wallet_name);
ensure_file_is_writable(&wallet_path).map_to_mm(WalletsStorageError::FsWriteError)?;
mm2_io::fs::write_json(encrypted_passphrase_data, &wallet_path, true)
.await
.mm_err(|e| WalletsStorageError::FsWriteError(e.to_string()))
}
which is run as part of this function
// This either initializes the cryptographic context or sets up the context for "no login mode".
initialize_wallet_passphrase(&ctx).await?;
and saving mnmonic file will fail otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the catch !

mm2src/mm2_main/src/lp_native_dex.rs Outdated Show resolved Hide resolved
@@ -5378,12 +5418,11 @@ pub struct HistoricalOrder {
conf_settings: Option<OrderConfirmationsSettings>,
}

pub async fn orders_kick_start(ctx: &MmArc) -> Result<HashSet<String>, String> {
pub async fn orders_kick_start(ctx: &MmArc, db_id: Option<&str>) -> Result<HashSet<String>, String> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to swaps kickstarts, we need to only kickstart orders if the coin is activated with the same db_id as the order's db_id ref.

let taker_coin = loop {
match lp_coinfind(&ctx, &taker_coin_ticker).await {
Ok(Some(c)) => {
if taker_coin_db_id == c.account_db_id().await.as_deref() {
break c;
};
info!(
"Can't kickstart the swap {} until the coin {} is activated with pubkey: {}",
swap.uuid(),
taker_coin_ticker,
taker_coin_db_id.unwrap_or(&ctx.rmd160.to_string())
);
Timer::sleep(5.).await;
},
Ok(None) => {
info!(
"Can't kickstart the swap {} until the coin {} is activated",
swap.uuid(),
taker_coin_ticker
);
Timer::sleep(5.).await;
},
Err(e) => {
error!("Error {} on {} find attempt", e, taker_coin_ticker);
return;
},
};
};
let maker_coin = loop {
match lp_coinfind(&ctx, &maker_coin_ticker).await {
Ok(Some(c)) => {
if maker_coin_db_id == c.account_db_id().await.as_deref() {
break c;
};
info!(
"Can't kickstart the swap {} until the coin {} is activated with pubkey: {}",
swap.uuid(),
maker_coin_ticker,
maker_coin_db_id.unwrap_or(&ctx.rmd160.to_string())
);
Timer::sleep(5.).await;
},
Ok(None) => {
info!(
"Can't kickstart the swap {} until the coin {} is activated",
swap.uuid(),
maker_coin_ticker
);
Timer::sleep(5.).await;
},
Err(e) => {
error!("Error {} on {} find attempt", e, maker_coin_ticker);
return;
},
};
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry if I wasn't clear about this. I meant that if the other coin of the order was with a different db_id. Similar to what we done with swaps before if you remember.

Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Some more review notes related to your latest fixes!

DerivationMethod::HDWallet(hd_wallet) => hd_wallet.get_enabled_address().await.map(|addr| {
// Skip the first byte of the uncompressed public key before converting to the eth address.
let pubkey = Public::from_slice(&addr.pubkey().as_bytes()[1..]);
public_to_address(&pubkey).to_string()
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 you should use display_eth_address function and strip the 0x at the start

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 you should use display_eth_address function and strip the 0x at the start

I think the below would be much simpler than what you implemented

let address = display_eth_address(&addr.address());
address.trim_start_matches("0x").to_string()

let pubkey = dhash160(activated_key.public().as_bytes()).to_string();
// Skip the first byte of the uncompressed public key before converting to the eth address.
let pubkey = Public::from_slice(&activated_key.public().as_bytes()[1..]);
let pubkey = public_to_address(&pubkey).to_string();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

Comment on lines 503 to 510
#[cfg(not(target_arch = "wasm32"))]
{
let dbdir = ctx.dbdir(None);
fs::create_dir_all(&dbdir).map_to_mm(|e| MmInitError::ErrorCreatingDbDir {
path: dbdir.clone(),
error: e.to_string(),
})?;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be before initialize_wallet_passphrase as it was done before

// This either initializes the cryptographic context or sets up the context for "no login mode".
initialize_wallet_passphrase(&ctx).await?;

You don't need to create the default db dir, only the global one. We probably also need to ensure_dir_is_writable, just in case.

@@ -5378,12 +5418,11 @@ pub struct HistoricalOrder {
conf_settings: Option<OrderConfirmationsSettings>,
}

pub async fn orders_kick_start(ctx: &MmArc) -> Result<HashSet<String>, String> {
pub async fn orders_kick_start(ctx: &MmArc, db_id: Option<&str>) -> Result<HashSet<String>, String> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry if I wasn't clear about this. I meant that if the other coin of the order was with a different db_id. Similar to what we done with swaps before if you remember.

@shamardy shamardy self-requested a review August 5, 2024 12:25
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Thanks you for the fixes! Next review iteration where I only focus on lp_ordermatch.rs!

Please merge with latest dev to get docker tests to pass. We should also start writing all the testcases we discussed before related to swaps / orders when we activate one of the coins with a different db_id (whether it's the one that has the swap/orders data or not). I remember we discussed a lot of these cases.

Comment on lines 1585 to 1586
base_coin_account_id: Option<String>,
rel_coin_account_id: Option<String>,
Copy link
Collaborator

@shamardy shamardy Aug 5, 2024

Choose a reason for hiding this comment

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

Please add doc comments for these. And similar others.

coins.insert(order.request.base.clone());
coins.insert(order.request.rel.clone());
taker_orders.insert(order.request.uuid, order);
let order_base_coin_account_id = order.base_coin_account_id.clone().unwrap_or(ctx.default_db_id());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Taker order is not always loaded from rel coin db_id as it depends on if the order is buy or sell ref

pub fn account_id(&self) -> &Option<String> {
match self.request.action {
TakerAction::Buy => &self.rel_coin_account_id,
TakerAction::Sell => &self.base_coin_account_id,
}

Maybe we should add a function called other_coin_account_id to TakerOrder and MakerOrder and use it here and here
let order_rel_coin_db_id = order.rel_coin_account_id.clone().unwrap_or(ctx.default_db_id());
and any other appropriate place.

Copy link
Member

Choose a reason for hiding this comment

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

fixed

#[cfg(target_arch = "wasm32")]
ordermatch_db: ConstructibleDb<OrdermatchDb>,
}

#[allow(unused)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you add this? I can see that this is actually used.

@@ -2970,12 +2999,13 @@ fn lp_connect_start_bob(ctx: MmArc, maker_match: MakerMatch, maker_order: MakerO
},
};

let account_db_id = maker_coin.account_db_id().await;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we use the orders db_id to have swap files/data saved in the same place as the order files/data?

@@ -3121,6 +3153,7 @@ fn lp_connected_alice(ctx: MmArc, taker_order: TakerOrder, taker_match: TakerMat
);

let now = now_sec();
let account_db_id = taker_coin.account_db_id().await;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

storage.load_order_from_history(req.uuid, db_id.as_deref()).await,
&storage.select_order_status(req.uuid, db_id.as_deref()).await,
) {
info!("Order with UUID=({})", req.uuid);
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for this info

maker_orders_ctx.add_order(ctx.weak(), order.clone(), None);
let order_rel_coin_db_id = order.rel_coin_account_id.clone().unwrap_or(ctx.default_db_id());

loop {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This loop and the other one will never end if the other coin is not enabled with the right address / db_id. We should rethink this whole kickstarting approach. This is where the code looks for if the coin for an order is enabled or not

if let Ok(Some(coin)) = lp_coinfind(&ctx, &ticker).await {
let balance = match coin.my_spendable_balance().compat().await {
Ok(balance) => balance,
Err(_) => continue,
};
if Some(&balance) != current_balance.as_ref() {
let coins_ctx = CoinsContext::from_ctx(&ctx).unwrap();
coins_ctx.balance_updated(&coin, &balance).await;
current_balance = Some(balance);
}
}

Copy link
Member

Choose a reason for hiding this comment

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

fixed

Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

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

Huge work!
Here is the next review iteration

Comment on lines +790 to +802
#[cfg(not(target_arch = "wasm32"))]
{
let pubkey = {
let pubkey = Public::from_slice(activated_key.public().as_bytes());
let addr = display_eth_address(&public_to_address(&pubkey));
addr.trim_start_matches("0x").to_string()
};

run_db_migration_for_new_pubkey(ctx, pubkey)
.await
.map_to_mm(EthActivationV2Error::InternalError)?;
}

Copy link
Member

Choose a reason for hiding this comment

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

Could you please clarify why this block is only for non-wasm target? is it bcz IDB automatically handles schema changes?

Copy link
Member

Choose a reason for hiding this comment

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

WASM doesn't need theses operations only SQL needs running migration when opening db and also additional directory checks which of course WASM doesn't need

}

/// Test method for initializing a single-user database connection in-memory.
pub fn init_test(ctx: &MmArc) -> Result<(), String> { Self::init_impl_test(ctx, None, DbIdConnKind::Single) }
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this is used only in tests, we can add #[cfg(all(feature = "for-tests", not(target_arch = "wasm32")))] annotation for this function

strange that clippy didnt warn about unused functions in prod code.
Screenshot 2024-08-08 at 17 41 59

}

/// Initialize a database connection.
pub async fn init_test(ctx: &MmArc, db_id: Option<&str>) -> Result<(), String> {
Copy link
Member

Choose a reason for hiding this comment

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

here too, lets add #[cfg(all(feature = "for-tests", not(target_arch = "wasm32")))] annotation

Copy link
Member

@borngraced borngraced Aug 8, 2024

Choose a reason for hiding this comment

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

I had some problem doing this(init_test isn't really used directly in tests and some other functions also uses it which aren't annotated with tests cfg) but I guess I can pick it up again after working on more important stuffs in this PR.. thanks for the catch !

Comment on lines +640 to +641
#[derive(Debug, Serialize)]
pub struct NftsTransferHistoryLists {
Copy link
Member

Choose a reason for hiding this comment

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

Please add doc comm for new NftsTransferHistoryLists structure

@@ -653,7 +662,7 @@ pub struct NftTransferHistoryFilters {
}

/// Contains parameters required to update NFT transfer history and NFT list.
#[derive(Debug, Deserialize)]
#[derive(Debug, Deserialize, Clone)]
Copy link
Member

Choose a reason for hiding this comment

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

its not critical but please make Clone first, as we try to keep alphabetical order in derive

Comment on lines +1706 to +1733
pub async fn find_unique_nft_account_ids(
ctx: &MmArc,
chains: Vec<Chain>,
) -> Result<HashMap<String, Vec<Chain>>, String> {
let cctx = try_s!(CoinsContext::from_ctx(ctx));
let coins = cctx.coins.lock().await;
let coins = coins.values().collect::<Vec<_>>();

let mut active_id_chains = HashMap::new();
for coin in coins.iter() {
if coin.is_available() {
// Use default if no db_id
let db_id = coin
.inner
.account_db_id()
.await
.unwrap_or_else(|| ctx.rmd160.to_string());
let entry = active_id_chains.entry(db_id).or_insert_with(Vec::new);
if let Ok(chain) = Chain::from_ticker(coin.inner.ticker()) {
if chains.contains(&chain) {
entry.push(chain);
}
}
}
}

Ok(active_id_chains)
}
Copy link
Member

Choose a reason for hiding this comment

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

It could be simplified

async fn find_unique_nft_account_ids(
    ctx: &MmArc,
    chains: Vec<Chain>,
) -> Result<HashMap<String, Vec<Chain>>, String> {
    let cctx = try_s!(CoinsContext::from_ctx(ctx));
    let coins = cctx.coins.lock().await;

    let mut active_id_chains: HashMap<String, Vec<Chain>> = HashMap::new();
    for coin in coins.values() {
        if coin.is_available() {
            // Use default if no db_id
            let db_id = coin
                .inner
                .account_db_id()
                .await
                .unwrap_or_else(|| ctx.rmd160.to_string());
            if let Ok(chain) = Chain::from_ticker(coin.inner.ticker()) {
                if chains.contains(&chain) {
                    active_id_chains.entry(db_id).or_default().push(chain);
                }
            }
        }
    }

    Ok(active_id_chains)
}

You can avoid collecting values into a Vec, and directly iterate over coins.values().
instead of creating entry object, you can call .entry(db_id).or_default().
Also seems like there is no need in pub

Comment on lines +1738 to +1754
let coins = coins.values().collect::<Vec<_>>();

for coin in coins.iter() {
if coin.is_available() {
// Use default if no db_id
let db_id = coin
.inner
.account_db_id()
.await
.unwrap_or_else(|| ctx.rmd160.to_string());
if let Ok(chain) = Chain::from_ticker(coin.inner.ticker()) {
if chains == chain {
return Ok(Some((db_id, chain)));
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Here you also dont have to collect coins into vector

    let coins = cctx.coins.lock().await;

    for coin in coins.values() {
        if coin.is_available() {
            // Use default if no db_id
            let db_id = coin
                .inner
                .account_db_id()
                .await
                .unwrap_or_else(|| ctx.rmd160.to_string());
            if let Ok(chain) = Chain::from_ticker(coin.inner.ticker()) {
                if chains == chain {
                    return Ok(Some((db_id, chain)));
                }
            }
        }
    }

Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

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

I have question

@@ -744,7 +746,7 @@ mod wasm_test {
// async fn create_to_address_fails_on_unverified_notes() {
// // init blocks_db
// let ctx = mm_ctx_with_custom_db();
// let blockdb = BlockDbImpl::new(&ctx, TICKER.to_string(), PathBuf::new()).await.unwrap();
// let blockdb = BlockDbImpl::new(&ctx, TICKER.to_string(), PathBuf::new(), None).await.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

There is a todo above Todo: Uncomment after improving tx creation time
could you tell what the problem with this?
I remember that there was an issue with blocks synchronisation, which was fixed by adding sync params. but what about tx creation time?

Copy link
Member

Choose a reason for hiding this comment

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

generating zcoin txs uses lot of resource and takes time to run...there're some optimization for native targets that cannot be achieved with WASM yet

@shamardy shamardy self-assigned this Aug 9, 2024
@shamardy shamardy added in progress Changes will be made from the author and removed under review labels Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in progress Changes will be made from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implementing Per-Coin Database Paths Based on PubkeyHash for Enhanced Modularization
7 participants