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

rename scan to scan_with_keychain #1117

Closed
wants to merge 6 commits into from
+39 −61
Closed

rename scan to scan_with_keychain #1117

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7e91849
BREAKING CHANGE: In `electrum` crate the method `scan` of trait `Elec…
448-OG Sep 13, 2023
2fa5fc6
BREAKING CHANGE: In `electrum` crate rename the `ElectrumExt` trait m…
448-OG Sep 13, 2023
9655e31
Chore: Run cargo fmt
448-OG Sep 13, 2023
2a9a2e0
BREAKING CHANGE: In `electrum` crate rename the `ElectrumExt` trait m…
448-OG Sep 15, 2023
25fb58a
Fix: Replace previous docs `ElectrumExt::scan` with `ElectrumClient::…
448-OG Sep 15, 2023
95d0826
docs(electrum): Dropped usage of `NOTE` and replaced it with `Note th…
448-OG Sep 15, 2023
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
20 changes: 10 additions & 10 deletions crates/bdk/src/wallet/coin_selection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -836,7 +836,7 @@ mod test {
let drain_script = ScriptBuf::default();
let target_amount = 250_000 + FEE_AMOUNT;

let result = LargestFirstCoinSelection::default()
let result = LargestFirstCoinSelection
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these changes on coin selection slipped in because of the rebase. You have to remove them. There is more other files. Just cross check

Copy link
Author

Choose a reason for hiding this comment

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

These changes were as a result of running clippy. If you run clippy on master you will get recommendations to remove them. Since in rust tuple struct LargestFirstCoinSelection you dont need to call ::default() to initialize it

Copy link
Member

Choose a reason for hiding this comment

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

Generally it's better to handle fixing clippy changes in a separate commit or PR to make the review easier. Also a nit on commit messages, instead of BREAKING CHANGE: I'd use the notation refactor!(<module name>): .... See: https://www.conventionalcommits.org/en/v1.0.0/

And as @LLFourn mentioned we may end up incorporating this rename into another PR but if you have time it wouldn't hurt to fix this one up in case we can use it.

.coin_select(
utxos,
vec![],
Expand All @@ -857,7 +857,7 @@ mod test {
let drain_script = ScriptBuf::default();
let target_amount = 20_000 + FEE_AMOUNT;

let result = LargestFirstCoinSelection::default()
let result = LargestFirstCoinSelection
.coin_select(
utxos,
vec![],
Expand All @@ -878,7 +878,7 @@ mod test {
let drain_script = ScriptBuf::default();
let target_amount = 20_000 + FEE_AMOUNT;

let result = LargestFirstCoinSelection::default()
let result = LargestFirstCoinSelection
.coin_select(
vec![],
utxos,
Expand All @@ -900,7 +900,7 @@ mod test {
let drain_script = ScriptBuf::default();
let target_amount = 500_000 + FEE_AMOUNT;

LargestFirstCoinSelection::default()
LargestFirstCoinSelection
.coin_select(
vec![],
utxos,
Expand All @@ -918,7 +918,7 @@ mod test {
let drain_script = ScriptBuf::default();
let target_amount = 250_000 + FEE_AMOUNT;

LargestFirstCoinSelection::default()
LargestFirstCoinSelection
.coin_select(
vec![],
utxos,
Expand All @@ -935,7 +935,7 @@ mod test {
let drain_script = ScriptBuf::default();
let target_amount = 180_000 + FEE_AMOUNT;

let result = OldestFirstCoinSelection::default()
let result = OldestFirstCoinSelection
.coin_select(
vec![],
utxos,
Expand All @@ -956,7 +956,7 @@ mod test {
let drain_script = ScriptBuf::default();
let target_amount = 20_000 + FEE_AMOUNT;

let result = OldestFirstCoinSelection::default()
let result = OldestFirstCoinSelection
.coin_select(
utxos,
vec![],
Expand All @@ -977,7 +977,7 @@ mod test {
let drain_script = ScriptBuf::default();
let target_amount = 20_000 + FEE_AMOUNT;

let result = OldestFirstCoinSelection::default()
let result = OldestFirstCoinSelection
.coin_select(
vec![],
utxos,
Expand All @@ -999,7 +999,7 @@ mod test {
let drain_script = ScriptBuf::default();
let target_amount = 600_000 + FEE_AMOUNT;

OldestFirstCoinSelection::default()
OldestFirstCoinSelection
.coin_select(
vec![],
utxos,
Expand All @@ -1018,7 +1018,7 @@ mod test {
let target_amount: u64 = utxos.iter().map(|wu| wu.utxo.txout().value).sum::<u64>() - 50;
let drain_script = ScriptBuf::default();

OldestFirstCoinSelection::default()
OldestFirstCoinSelection
.coin_select(
vec![],
utxos,
Expand Down
2 changes: 1 addition & 1 deletion crates/bdk/tests/wallet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3085,7 +3085,7 @@ fn test_taproot_script_spend_sign_exclude_some_leaves() {
.values()
.map(|(script, version)| TapLeafHash::from_script(script, *version))
.collect();
let included_script_leaves = vec![script_leaves.pop().unwrap()];
let included_script_leaves = [script_leaves.pop().unwrap()];
let excluded_script_leaves = script_leaves;

assert!(
Expand Down
2 changes: 1 addition & 1 deletion crates/chain/tests/test_keychain_txout_index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ fn test_wildcard_derivations() {
let _ = txout_index.reveal_to_target(&TestKeychain::External, 25);

(0..=15)
.chain(vec![17, 20, 23].into_iter())
.chain(vec![17, 20, 23])
.for_each(|index| assert!(txout_index.mark_used(&TestKeychain::External, index)));

assert_eq!(txout_index.next_index(&TestKeychain::External), (26, true));
Expand Down
21 changes: 9 additions & 12 deletions crates/chain/tests/test_tx_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,18 +709,15 @@ fn test_chain_spends() {
let _ = graph.insert_tx(tx_1.clone());
let _ = graph.insert_tx(tx_2.clone());

[95, 98]
.iter()
.zip([&tx_0, &tx_1].into_iter())
.for_each(|(ht, tx)| {
let _ = graph.insert_anchor(
tx.txid(),
ConfirmationHeightAnchor {
anchor_block: tip.block_id(),
confirmation_height: *ht,
},
);
});
[95, 98].iter().zip([&tx_0, &tx_1]).for_each(|(ht, tx)| {
let _ = graph.insert_anchor(
tx.txid(),
ConfirmationHeightAnchor {
anchor_block: tip.block_id(),
confirmation_height: *ht,
},
);
});

// Assert that confirmed spends are returned correctly.
assert_eq!(
Expand Down
34 changes: 11 additions & 23 deletions crates/electrum/src/electrum_ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,59 +139,47 @@ pub trait ElectrumExt {
///
/// - `prev_tip`: the most recent blockchain tip present locally
/// - `keychain_spks`: keychains that we want to scan transactions for
/// - `txids`: transactions for which we want updated [`Anchor`]s
/// - `outpoints`: transactions associated with these outpoints (residing, spending) that we
/// want to included in the update
///
/// The scan for each keychain stops after a gap of `stop_gap` script pubkeys with no associated
/// transactions. `batch_size` specifies the max number of script pubkeys to request for in a
/// single batch request.
448-OG marked this conversation as resolved.
Show resolved Hide resolved
fn scan<K: Ord + Clone>(
/// ##### NOTE: Scanning with keychain is very inefficient and should be used only when restoring
448-OG marked this conversation as resolved.
Show resolved Hide resolved
/// from seed words.
fn scan_with_keychain<K: Ord + Clone>(
&self,
prev_tip: Option<CheckPoint>,
keychain_spks: BTreeMap<K, impl IntoIterator<Item = (u32, ScriptBuf)>>,
txids: impl IntoIterator<Item = Txid>,
outpoints: impl IntoIterator<Item = OutPoint>,
448-OG marked this conversation as resolved.
Show resolved Hide resolved
stop_gap: usize,
batch_size: usize,
) -> Result<(ElectrumUpdate, BTreeMap<K, u32>), Error>;

/// Convenience method to call [`scan`] without requiring a keychain.
///
/// [`scan`]: ElectrumExt::scan
fn scan_without_keychain(
fn sync(
448-OG marked this conversation as resolved.
Show resolved Hide resolved
&self,
prev_tip: Option<CheckPoint>,
misc_spks: impl IntoIterator<Item = ScriptBuf>,
txids: impl IntoIterator<Item = Txid>,
outpoints: impl IntoIterator<Item = OutPoint>,
_txids: impl IntoIterator<Item = Txid>,
_outpoints: impl IntoIterator<Item = OutPoint>,
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we completely remove _txids and _outpoints? I don't see their utility

Copy link
Author

Choose a reason for hiding this comment

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

They are used in bdk/example-crates/example_electrum/src/main.rs line 249, do I remove the code in examples too to reflect this?

Copy link
Contributor

Choose a reason for hiding this comment

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

txids and outpoints is meant to return you any relevant transactions to these items. They can't just be _ignored. A more significant refactoring is needed so that this method does not call scan_with_keychain but implements its own scanning logic.

batch_size: usize,
) -> Result<ElectrumUpdate, Error> {
let spk_iter = misc_spks
.into_iter()
.enumerate()
.map(|(i, spk)| (i as u32, spk));

let (electrum_update, _) = self.scan(
prev_tip,
[((), spk_iter)].into(),
txids,
outpoints,
usize::MAX,
batch_size,
)?;
let (electrum_update, _) =
self.scan_with_keychain(prev_tip, [((), spk_iter)].into(), usize::MAX, batch_size)?;

Ok(electrum_update)
}
}

impl ElectrumExt for Client {
fn scan<K: Ord + Clone>(
fn scan_with_keychain<K: Ord + Clone>(
&self,
prev_tip: Option<CheckPoint>,
keychain_spks: BTreeMap<K, impl IntoIterator<Item = (u32, ScriptBuf)>>,
txids: impl IntoIterator<Item = Txid>,
outpoints: impl IntoIterator<Item = OutPoint>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we are removing txids and outpoints it doesn't make sense to be calling populate_with_txids and populate_with_outpoints here. Should separate methods be created to get these updates? @LLFourn @evanlinjin

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes the separate method is scan. scan is still meant to be doing this.

stop_gap: usize,
batch_size: usize,
) -> Result<(ElectrumUpdate, BTreeMap<K, u32>), Error> {
Expand All @@ -201,8 +189,8 @@ impl ElectrumExt for Client {
.collect::<BTreeMap<K, _>>();
let mut scanned_spks = BTreeMap::<(K, u32), (ScriptBuf, bool)>::new();

let txids = txids.into_iter().collect::<Vec<_>>();
let outpoints = outpoints.into_iter().collect::<Vec<_>>();
let txids = Vec::<_>::new();
let outpoints = Vec::<_>::new();

let (electrum_update, keychain_update) = loop {
let (tip, _) = construct_update_tip(self, prev_tip.clone())?;
Expand Down
11 changes: 2 additions & 9 deletions example-crates/example_electrum/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,14 +146,7 @@ fn main() -> anyhow::Result<()> {
};

client
.scan(
tip,
keychain_spks,
core::iter::empty(),
core::iter::empty(),
stop_gap,
scan_options.batch_size,
)
.scan_with_keychain(tip, keychain_spks, stop_gap, scan_options.batch_size)
.context("scanning the blockchain")?
}
ElectrumCommands::Sync {
Expand Down Expand Up @@ -252,7 +245,7 @@ fn main() -> anyhow::Result<()> {
drop((graph, chain));

let electrum_update = client
.scan_without_keychain(tip, spks, txids, outpoints, scan_options.batch_size)
.sync(tip, spks, txids, outpoints, scan_options.batch_size)
.context("scanning the blockchain")?;
(electrum_update, BTreeMap::new())
}
Expand Down
2 changes: 1 addition & 1 deletion example-crates/wallet_electrum/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
relevant_txids,
},
keychain_update,
) = client.scan(prev_tip, keychain_spks, None, None, STOP_GAP, BATCH_SIZE)?;
) = client.scan_with_keychain(prev_tip, keychain_spks, STOP_GAP, BATCH_SIZE)?;

println!();

Expand Down