Skip to content
Open
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
16 changes: 8 additions & 8 deletions src/chain/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ pub(crate) mod bitcoind;
mod electrum;
mod esplora;

use std::collections::HashMap;
use std::collections::{HashMap, HashSet};
use std::sync::{Arc, Mutex};
use std::time::Duration;

Expand Down Expand Up @@ -84,7 +84,7 @@ impl WalletSyncStatus {

pub(crate) struct ChainSource {
kind: ChainSourceKind,
registered_txids: Mutex<Vec<Txid>>,
registered_txids: Mutex<HashSet<Txid>>,
tx_broadcaster: Arc<Broadcaster>,
logger: Arc<Logger>,
}
Expand Down Expand Up @@ -113,7 +113,7 @@ impl ChainSource {
node_metrics,
)?;
let kind = ChainSourceKind::Esplora(esplora_chain_source);
let registered_txids = Mutex::new(Vec::new());
let registered_txids = Mutex::new(HashSet::new());
Ok((Self { kind, registered_txids, tx_broadcaster, logger }, None))
}

Expand All @@ -133,7 +133,7 @@ impl ChainSource {
node_metrics,
);
let kind = ChainSourceKind::Electrum(electrum_chain_source);
let registered_txids = Mutex::new(Vec::new());
let registered_txids = Mutex::new(HashSet::new());
(Self { kind, registered_txids, tx_broadcaster, logger }, None)
}

Expand All @@ -156,7 +156,7 @@ impl ChainSource {
);
let best_block = bitcoind_chain_source.poll_best_block().await.ok();
let kind = ChainSourceKind::Bitcoind(bitcoind_chain_source);
let registered_txids = Mutex::new(Vec::new());
let registered_txids = Mutex::new(HashSet::new());
(Self { kind, registered_txids, tx_broadcaster, logger }, best_block)
}

Expand All @@ -180,7 +180,7 @@ impl ChainSource {
);
let best_block = bitcoind_chain_source.poll_best_block().await.ok();
let kind = ChainSourceKind::Bitcoind(bitcoind_chain_source);
let registered_txids = Mutex::new(Vec::new());
let registered_txids = Mutex::new(HashSet::new());
(Self { kind, registered_txids, tx_broadcaster, logger }, best_block)
}

Expand Down Expand Up @@ -214,7 +214,7 @@ impl ChainSource {
}
}

pub(crate) fn registered_txids(&self) -> Vec<Txid> {
pub(crate) fn registered_txids(&self) -> HashSet<Txid> {
self.registered_txids.lock().expect("lock").clone()
}

Expand Down Expand Up @@ -472,7 +472,7 @@ impl ChainSource {

impl Filter for ChainSource {
fn register_tx(&self, txid: &Txid, script_pubkey: &Script) {
self.registered_txids.lock().expect("lock").push(*txid);
self.registered_txids.lock().expect("lock").insert(*txid);
match &self.kind {
ChainSourceKind::Esplora(esplora_chain_source) => {
esplora_chain_source.register_tx(txid, script_pubkey)
Expand Down
2 changes: 1 addition & 1 deletion src/payment/bolt11.rs
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ impl Bolt11Payment {
_ => 0,
};
if let Some(invoice_amount_msat) = details.amount_msat {
if claimable_amount_msat < invoice_amount_msat - skimmed_fee_msat {
if claimable_amount_msat < invoice_amount_msat.saturating_sub(skimmed_fee_msat) {
log_error!(
self.logger,
"Failed to manually claim payment {} as the claimable amount is less than expected",
Expand Down
4 changes: 2 additions & 2 deletions src/payment/unified.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,9 @@ impl UnifiedPayment {
pub fn receive(
&self, amount_sats: u64, description: &str, expiry_sec: u32,
) -> Result<String, Error> {
let onchain_address = self.onchain_payment.new_address()?;
let amount_msats = amount_sats.checked_mul(1_000).ok_or(Error::InvalidAmount)?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Rather than doing this here, shouldn't we rather do the check in BOLT11/BOLT12 receive methods, as this method is just meant to forward to them, with as little custom logic on top as possible. Apart from that we might also be okay to punt on it as we intend to switch to an LightningAmount type soon everywhere, that should enforce such checks at conversion time across the board.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we wanted to do this check in the bolt11/12 functions then we'd have to have those take sats rather than msats. I think this is the right location for this. LightningAmount is the correct choice eventually.


let amount_msats = amount_sats * 1_000;
let onchain_address = self.onchain_payment.new_address()?;

let bolt12_offer =
match self.bolt12_payment.receive_inner(amount_msats, description, None, None) {
Expand Down
12 changes: 12 additions & 0 deletions tests/integration_tests_rust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1680,6 +1680,18 @@ async fn generate_bip21_uri() {
assert!(uni_payment.contains("lno="));
}

#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
async fn unified_receive_rejects_msat_overflow() {
let (bitcoind, electrsd) = setup_bitcoind_and_electrsd();
let chain_source = random_chain_source(&bitcoind, &electrsd);
let node = setup_node(&chain_source, random_config(true));

assert_eq!(
Err(NodeError::InvalidAmount),
node.unified_payment().receive(u64::MAX, "asdf", 4_000)
);
}

#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
async fn unified_send_receive_bip21_uri() {
let (bitcoind, electrsd) = setup_bitcoind_and_electrsd();
Expand Down