fix: no Debug on Display implementations

[Dmenec]

Description

Fixes #1933, remove the usage of Debug on Display implemetations mentioned in #1881 (comment)

Changelog notice

• Replaced Debug usage with Display messages for InsertDescriptorError, CalculateFeeError and StoreError.
• InvalidMagicBytes now displays expected and actual bytes in hexadecimal instead of using Debug.
• Added a new short_descriptor function to shorten descriptors for concise error output.
• Added detailed SyncRequest Display for Esplora and Electrum examples.

Checklists

All Submissions:

• I have signed all my commits
• I followed the contribution guidelines
• I ran just check, just fmt and just test before committing

[qatkk]

Concept ACK — this approach looks good, thanks for the changes. I ran the tests locally and they all passed.

While trying to recreate the initial issue’s scenario (#1933), I found that bdk_wallet master still references the older API names (list_canonical_txs, filter_chain_unspents, filter_chain_txouts, from_genesis_hash), whereas this PR’s bdk_chain exposes the newer canonicalization API (canonical_iter) and LocalChain::from_genesis. As a result, building bdk_wallet against this PR failed. Is there any branch in bdk_wallet that already adopts the new APIs, so I can test against them?

I have a few small suggestions below. Most of these are nits, but since this PR is already addressing the area, I thought it’d be reasonable to mention them here:

• It might be nice to add a CI to prevent using Debug for Display in future changes.

• In the short_descriptor function, it could be useful to show the last 8 characters of the descriptor so the full checksum is visible.

• A small note on InvalidMagicBytes: it might be helpful to also show any difference between the length of expected and received byte.

[evanlinjin]

Quality work.

Just a couple requested changes:

• Commit messages should contain a scope (as mentioned in the conventional commits spec). The scope should be the name of the package affected: core, chain, example, etc.
• Commits that break the API should be marked with !.

And a nit:

• Error messages should be somewhat consistent. I personally don't think we should suggest actions in the message, just state what the error is.

[evanlinjin]

@Dmenec are you able to make progress on this?

[Dmenec]

@evanlinjin Pushed the updates. Let me know if everything checks out :)

[evanlinjin]

utACK 532e2ec

I'm happy with how this is looking. Unable to test until I have access to a computer again.

[Dmenec]

Having some trouble with the no_std CI check for the esplora crate... Does it actually support no_std, or should be treated as std‑only?

[oleonardolima]

Having some trouble with the no_std CI check for the esplora crate... Does it actually support no_std, or should be treated as std‑only?

@Dmenec It's now fixed on master, a rebase should fix it.

[nymius]

Nit:

diff --git a/crates/chain/src/tx_graph.rs b/crates/chain/src/tx_graph.rs
index 457a478a..d170eccf 100644
--- a/crates/chain/src/tx_graph.rs
+++ b/crates/chain/src/tx_graph.rs
@@ -258,19 +258,16 @@ impl fmt::Display for CalculateFeeError {
         match self {
             CalculateFeeError::MissingTxOut(outpoints) => {
                 let max_show = 3;
-                let shown = outpoints.iter().take(max_show);
+                let shown: Vec<_> = outpoints.iter().take(max_show).collect();
                 let remaining = outpoints.len().saturating_sub(max_show);
 
                 write!(f, "missing `TxOut` for input(s)")?;
                 if outpoints.is_empty() {
                     write!(f, ": <none>")
                 } else {
-                    write!(f, ": ")?;
-                    for (i, op) in shown.enumerate() {
-                        if i > 0 {
-                            write!(f, ", ")?;
-                        }
-                        write!(f, "{}", op)?;
+                    write!(f, ": {}", shown[0])?;
+                    for op in &shown[1..] {
+                        write!(f, ", {}", op)?;
                     }
                     if remaining > 0 {
                         write!(f, " (+{} more)", remaining)?;

[nymius]

Error: could not open file store

Caused by:
    file has invalid magic bytes: expected=0x62646b5f6578616d706c655f656c65637472756d got=0x62646b5f6578616d706c5f656c65637472756d01

Given the format of the error output I would use invalid magic bytes without the file has.

This is a good opportunity to rewrite the others in the same spirit.

Also, stacking expected and got improves the feedback to the reader:

Error: could not open file store

Caused by:
    invalid magic bytes
expected: 0x62646b5f6578616d706c655f656c65637472756d
got:      0x62646b5f6578616d706c5f656c65637472756d01

[nymius]

nit: "[ SCANNING {pc:3.0}% ] script {} {} {}", instead of "[ SCANNING {pc:03.0}% ] script {} {} {}",

I prefer

[ SCANNING   6% ] outpoint 623cf110e23b15074e807a551a751856eb0a95f23a4a60f125d943d1d6e7cb6a:0

to

[ SCANNING 006% ] outpoint 623cf110e23b15074e807a551a751856eb0a95f23a4a60f125d943d1d6e7cb6a:0

[nymius]

I've tried this with the following bin:

use bdk_chain::bitcoin::key::Secp256k1;
use bdk_chain::indexer::keychain_txout::KeychainTxOutIndex;
use bdk_chain::miniscript::Descriptor;

const DESC: &str = "...";

fn main() {
    let lookahead = 10;
    let use_cache = true;
    let mut index = KeychainTxOutIndex::new(lookahead, use_cache);
    let secp = Secp256k1::new();
    let (desc, _) = Descriptor::parse_descriptor(&secp, DESC).unwrap();
    index.insert_descriptor("some", desc.clone()).unwrap();
    if let Err(e) = index.insert_descriptor("other", desc) {
        eprintln!("{}", e);
    }
}

I think the output is not very clear:

descriptor 'tr([...d45f9rlk' is already in use by another keychain 'some'.

I would keep only a fixed size prefix in the output, including the descriptor type and trying to capture the origin keypath if possible.
@qatkk proposed to increase the suffix to capture the checksum, but without any clear accessors to those components it is a "best guess" effort.
Maybe we should propose the implementation of them at the miniscript level, and add methods to get strings from DescriptorType for example.