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

Reserved address pool #531

Merged
merged 1 commit into from
Oct 30, 2024
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
2 changes: 1 addition & 1 deletion cli/Cargo.lock

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

2 changes: 1 addition & 1 deletion lib/Cargo.lock

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

2 changes: 1 addition & 1 deletion lib/bindings/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ impl UniffiBindingLogger {
impl log::Log for UniffiBindingLogger {
fn enabled(&self, m: &Metadata) -> bool {
// ignore the internal uniffi log to prevent infinite loop.
return m.level() <= Level::Trace && *m.target() != *"breez_sdk_liquid_bindings";
m.level() <= Level::Trace && *m.target() != *"breez_sdk_liquid_bindings"
}

fn log(&self, record: &Record) {
Expand Down
2 changes: 1 addition & 1 deletion lib/core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ flutter_rust_bridge = { version = "=2.4.0", features = [
log = { workspace = true }
lwk_common = "0.7.0"
lwk_signer = "0.7.0"
lwk_wollet = { git = "https://github.com/dangeross/lwk", branch = "savage-try-headers-subscribe" }
lwk_wollet = { git = "https://github.com/dangeross/lwk", branch = "savage-full-scan-to-index" }
#lwk_wollet = "0.7.0"
rusqlite = { version = "0.31", features = ["backup", "bundled"] }
rusqlite_migration = "1.0"
Expand Down
49 changes: 29 additions & 20 deletions lib/core/src/chain_swap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,24 +225,26 @@ impl ChainSwapHandler {
}

async fn rescan_outgoing_chain_swap(&self, swap: &ChainSwap) -> Result<()> {
let address = Address::from_str(&swap.claim_address)?;
let claim_tx_id = swap.claim_tx_id.clone().ok_or(anyhow!("No claim tx id"))?;
let script_pubkey = address.assume_checked().script_pubkey();
let script_history = self
.bitcoin_chain_service
.lock()
.await
.get_script_history(script_pubkey.as_script())?;
let claim_tx_history = script_history
.iter()
.find(|h| h.txid.to_hex().eq(&claim_tx_id) && h.height > 0);
if claim_tx_history.is_some() {
info!(
"Outgoing Chain Swap {} claim tx is confirmed. Setting the swap to Complete",
swap.id
);
self.update_swap_info(&swap.id, Complete, None, None, None, None)
.await?;
if let Some(claim_address) = &swap.claim_address {
let address = Address::from_str(claim_address)?;
let claim_tx_id = swap.claim_tx_id.clone().ok_or(anyhow!("No claim tx id"))?;
let script_pubkey = address.assume_checked().script_pubkey();
let script_history = self
.bitcoin_chain_service
.lock()
.await
.get_script_history(script_pubkey.as_script())?;
let claim_tx_history = script_history
.iter()
.find(|h| h.txid.to_hex().eq(&claim_tx_id) && h.height > 0);
if claim_tx_history.is_some() {
info!(
"Outgoing Chain Swap {} claim tx is confirmed. Setting the swap to Complete",
swap.id
);
self.update_swap_info(&swap.id, Complete, None, None, None, None)
.await?;
}
}
Ok(())
}
Expand Down Expand Up @@ -698,9 +700,16 @@ impl ChainSwapHandler {
ensure_sdk!(swap.claim_tx_id.is_none(), PaymentError::AlreadyClaimed);

debug!("Initiating claim for Chain Swap {swap_id}");
// Derive a new Liquid address for an incoming swap, or use the set Bitcoin address for an outgoing swap
let claim_address = match swap.direction {
Direction::Incoming => {
Some(self.onchain_wallet.next_unused_address().await?.to_string())
}
Direction::Outgoing => swap.claim_address.clone(),
};
let claim_tx = self
.swapper
.create_claim_tx(Swap::Chain(swap.clone()), None)?;
.create_claim_tx(Swap::Chain(swap.clone()), claim_address)?;

// Set the swap claim_tx_id before broadcasting.
// If another claim_tx_id has been set in the meantime, don't broadcast the claim tx
Expand Down Expand Up @@ -910,7 +919,6 @@ impl ChainSwapHandler {
swap.id
);

let refund_address = self.onchain_wallet.next_unused_address().await?.to_string();
let SwapScriptV2::Liquid(swap_script) = swap.get_lockup_swap_script()? else {
return Err(PaymentError::Generic {
err: "Unexpected swap script type found".to_string(),
Expand All @@ -925,6 +933,7 @@ impl ChainSwapHandler {
.script_pubkey();
let utxos = liquid_chain_service.get_script_utxos(&script_pk).await?;

let refund_address = self.onchain_wallet.next_unused_address().await?.to_string();
let SdkTransaction::Liquid(refund_tx) = self.swapper.create_refund_tx(
Swap::Chain(swap.clone()),
&refund_address,
Expand Down
6 changes: 6 additions & 0 deletions lib/core/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,12 @@ impl From<anyhow::Error> for PaymentError {
}
}

impl From<rusqlite::Error> for PaymentError {
fn from(_: rusqlite::Error) -> Self {
Self::PersistError
}
}

impl From<SdkError> for PaymentError {
fn from(err: SdkError) -> Self {
Self::Generic {
Expand Down
23 changes: 21 additions & 2 deletions lib/core/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,16 @@ pub struct ConnectWithSignerRequest {
pub config: Config,
}

/// A reserved address. Once an address is reserved, it can only be
/// reallocated to another payment after the block height expiration.
#[derive(Clone, Debug)]
pub(crate) struct ReservedAddress {
/// The address that is reserved
pub(crate) address: String,
/// The block height that the address is reserved until
pub(crate) expiry_block_height: u32,
}

/// The send/receive methods supported by the SDK
#[derive(Clone, Debug, EnumString, Serialize, Eq, PartialEq)]
pub enum PaymentMethod {
Expand Down Expand Up @@ -600,7 +610,8 @@ impl FromSql for Direction {
pub(crate) struct ChainSwap {
pub(crate) id: String,
pub(crate) direction: Direction,
pub(crate) claim_address: String,
/// The Bitcoin claim address is only set for Outgoing Chain Swaps
pub(crate) claim_address: Option<String>,
dangeross marked this conversation as resolved.
Show resolved Hide resolved
pub(crate) lockup_address: String,
pub(crate) timeout_block_height: u32,
pub(crate) preimage: String,
Expand Down Expand Up @@ -823,8 +834,16 @@ pub(crate) struct ReceiveSwap {
pub(crate) claim_fees_sat: u64,
/// Persisted as soon as a claim tx is broadcast
pub(crate) claim_tx_id: Option<String>,
/// Persisted only when the lockup tx is broadcast
pub(crate) lockup_tx_id: Option<String>,
/// The address reserved for a magic routing hint payment
pub(crate) mrh_address: String,
/// The script pubkey for a magic routing hint payment
pub(crate) mrh_script_pubkey: String,
/// Persisted only if a transaction is sent to the `mrh_address`
pub(crate) mrh_tx_id: Option<String>,
/// Until the lockup tx is seen in the mempool, it contains the swap creation time.
/// Afterwards, it shows the lockup tx creation time.
/// Afterwards, it shows the lockup tx creation time.
pub(crate) created_at: u32,
pub(crate) state: PaymentState,
}
Expand Down
151 changes: 151 additions & 0 deletions lib/core/src/persist/address.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
use anyhow::Result;
use log::debug;
use rusqlite::{Row, Transaction, TransactionBehavior};

use crate::error::PaymentError;

use super::{Persister, ReservedAddress};

impl Persister {
pub(crate) fn next_expired_reserved_address(
&self,
tip: u32,
) -> Result<Option<ReservedAddress>> {
let mut con = self.get_connection()?;
let tx = con.transaction_with_behavior(TransactionBehavior::Immediate)?;
// Get the next expired reserved address
let query = Self::get_reserved_address_query(vec!["expiry_block_height < ?1".to_string()]);
let res = match tx.query_row(&query, [tip], Self::sql_row_to_reserved_address) {
Ok(reserved_address) => {
// Delete the reserved address
Self::delete_reserved_address_inner(&tx, &reserved_address.address)?;
Some(reserved_address)
}
Err(_) => None,
};
tx.commit()?;

Ok(res)
}

fn get_reserved_address_query(where_clauses: Vec<String>) -> String {
let mut where_clause_str = String::new();
if !where_clauses.is_empty() {
where_clause_str = String::from("WHERE ");
where_clause_str.push_str(where_clauses.join(" AND ").as_str());
}

format!(
"
SELECT
address,
expiry_block_height
FROM reserved_addresses
{where_clause_str}
ORDER BY expiry_block_height ASC
ok300 marked this conversation as resolved.
Show resolved Hide resolved
LIMIT 1
"
)
}

pub(crate) fn insert_or_update_reserved_address(
&self,
address: &str,
expiry_block_height: u32,
) -> Result<(), PaymentError> {
let con = self.get_connection()?;
con.execute(
"INSERT OR REPLACE INTO reserved_addresses (
address,
expiry_block_height
)
VALUES (?, ?)
",
(&address, expiry_block_height),
)?;
debug!(
"Reserved address {} until block height {}",
address, expiry_block_height
);

Ok(())
}

pub(crate) fn delete_reserved_address(&self, address: &str) -> Result<(), PaymentError> {
let mut con = self.get_connection()?;
let tx = con.transaction()?;
Self::delete_reserved_address_inner(&tx, address)?;
tx.commit()?;

Ok(())
}

fn delete_reserved_address_inner(tx: &Transaction, address: &str) -> Result<(), PaymentError> {
tx.execute(
"DELETE FROM reserved_addresses WHERE address = ?",
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Not sure what's the best way to delete data in the context of real-type sync.

IMO DELETE statements could lead to edge-cases when 2 SDK instances sync, one has the data (here, the reserved_address row) and the other doesn't. Depending on the ordering and timing, it could be that one or the other is treated as "the truth".

In contrast, sync would be more resilient if we delete by setting a flag, i.e. reserved = false. Any such conflict during synchronization could then be resolved simply: if any of the two have reserved = false, then its false.

@roeierez @hydra-yse what do you think, does this matter? Or it makes no difference for real-time sync?

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 reserved address table currently only stores MRH addresses used in receive swaps. These are the two cases where a delete would occur:

  • A pay to the MRH address is seen while there is no swap progress.
  • The reserved address block height expires and the address is reused for another purpose (claim, refund, MRH, direct address). In the MRH case it is re-added to the reserved address table with a later expiry.

Also to note is that the last_derivation_index from the cache needs to be synced.

Maybe it's best to add this as a task of the real-time sync tracking issue

[address],
)?;

Ok(())
}

fn sql_row_to_reserved_address(row: &Row) -> rusqlite::Result<ReservedAddress> {
Ok(ReservedAddress {
address: row.get(0)?,
expiry_block_height: row.get(1)?,
})
}
}

#[cfg(test)]
mod tests {
use anyhow::Result;

use crate::test_utils::persist::new_persister;

#[test]
fn test_next_expired_reserved_address() -> Result<()> {
let (_temp_dir, storage) = new_persister()?;
let address = "tlq1pq2amlulhea6ltq7x3eu9atsc2nnrer7yt7xve363zxedqwu2mk6ctcyv9awl8xf28cythreqklt5q0qqwsxzlm6wu4z6d574adl9zh2zmr0h85gt534n";

storage.insert_or_update_reserved_address(&address, 100)?;

let maybe_reserved_address = storage.next_expired_reserved_address(99)?;
// Under the expiry, not popped
assert!(maybe_reserved_address.is_none());

let maybe_reserved_address = storage.next_expired_reserved_address(100)?;
// Equal to expiry, not popped
assert!(maybe_reserved_address.is_none());

let maybe_reserved_address = storage.next_expired_reserved_address(101)?;
// Address expired, popped
assert!(maybe_reserved_address.is_some());

let maybe_reserved_address = storage.next_expired_reserved_address(102)?;
// Address already popped
assert!(maybe_reserved_address.is_none());

Ok(())
}

#[test]
fn test_delete_reserved_address() -> Result<()> {
let (_temp_dir, storage) = new_persister()?;
let address = "tlq1pq2amlulhea6ltq7x3eu9atsc2nnrer7yt7xve363zxedqwu2mk6ctcyv9awl8xf28cythreqklt5q0qqwsxzlm6wu4z6d574adl9zh2zmr0h85gt534n";

storage.insert_or_update_reserved_address(&address, 100)?;

let maybe_reserved_address = storage.next_expired_reserved_address(99)?;
// Under the expiry, not popped
assert!(maybe_reserved_address.is_none());

storage.delete_reserved_address(&address)?;

let maybe_reserved_address = storage.next_expired_reserved_address(101)?;
// Over the expired, but already deleted
assert!(maybe_reserved_address.is_none());

Ok(())
}
}
Loading
Loading