fix: no `Debug` on `Display` implementations #2083

Dmenec · 2025-12-11T11:30:15Z

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 · 2025-12-21T13:06:33Z

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.

crates/chain/src/indexer/keychain_txout.rs

evanlinjin · 2026-01-14T11:22:13Z

@Dmenec are you able to make progress on this?

Dmenec · 2026-01-14T16:09:10Z

@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.

crates/chain/src/indexer/keychain_txout.rs

Dmenec · 2026-01-21T19:34:25Z

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 · 2026-01-21T22:51:44Z

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.

crates/chain/src/tx_graph.rs

nymius · 2026-01-23T21:32:27Z

crates/file_store/src/lib.rs

-            ),
+            Self::Io(e) => write!(f, "io error trying to read file: {}", e),
+            Self::InvalidMagicBytes { got, expected } => {
+                write!(f, "file has invalid magic bytes: expected=0x")?;


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

Sure! I'll update the ones that are in scope for this PR and push the proposed changes shortly.

Resolved in ae17df2

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

Sorry for the back-and-forth, but my comment invalidates this. Error messages should be single line as per convention! Can use the # for "pretty print".

examples/example_esplora/src/main.rs

nymius · 2026-01-23T21:58:55Z

crates/chain/src/indexer/keychain_txout.rs

-impl<K: core::fmt::Debug> core::fmt::Display for InsertDescriptorError<K> {
+/// Returns a shortened string representation of a descriptor.
+/// Shows the first and last `edge_len` characters, separated by `...`.
+fn short_descriptor(desc: &Descriptor<DescriptorPublicKey>, edge_len: usize) -> String {


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.

cACK — I’ll open a follow‑up PR with the proposal and then apply it here. Thanks for the suggestion!

@nymius that is a good point. For practicality, can we remove the short_descriptor concept in this PR and just print the full descriptor for now?

Let's add it back once it is more readable (including the descriptor type, key path, checksum, etc.)

I agree, let's use full descriptors for now.

…iptorError Replace Debug-based formatting with user-friendly Display messages.

nymius · 2026-01-27T13:55:47Z

crates/chain/src/tx_graph.rs

+                    writeln!(f, "{}", shown[0])?;
+                    for op in &shown[1..] {
+                        writeln!(f, "{}", op)?;
+                    }


I haven't executed this yet, but my guess is it will print the outpoint vertically, as you are using writeln! instead of write!.

Yes, thought maybe it would be a bit more readable:

missing `TxOut` for input(s) aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:0 bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb:1 cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc:2

I'd suggest keeping Display single-line to follow Rust conventions. Callers typically don't expect {} to emit multiple lines, and it composes better with logging/error chains.

If we really want a pretty-printed multi-line version later, we can use the alternate flag ({:#}) for that. I don't think it's necessary for this PR (but feel free to include it).

fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { if f.alternate() { // multi-line version todo!() } else { // single-line version todo!() } }

I don't think we need the multi-line now, can add in the future if needed. Also, what's the rationale for the leading whitespace ?

crates/file_store/src/lib.rs

examples/example_electrum/src/main.rs

…eError Limited to 3 max shown items and added a suffix when there are additional entries.

…rror. Format magic bytes as 0x-prefixed hex.

evanlinjin

Sorry for the bike-shedding. We're very close.

evanlinjin · 2026-01-28T04:40:26Z

crates/chain/src/indexer/keychain_txout.rs

                write!(
                    f,
-                    "attempt to re-assign keychain {keychain:?} already assigned to {existing:?}"
+                    "keychain '{}' is already associated with another descriptor '{}'.",


As per Rust API Guidelines:

The error message given by the Display representation of an error type should be lowercase without trailing punctuation, and typically concise.

Please remove the trailing punctuation thanks!

evanlinjin · 2026-01-28T04:42:20Z

crates/file_store/src/lib.rs

+            Self::Io(e) => write!(f, "io error while reading store file: {e}"),
+            Self::Bincode(e) => write!(f, "bincode error while decoding entry {e}"),
+            Self::InvalidMagicBytes { got, expected } => {
+                writeln!(f, "invalid magic bytes")?;


Also here, please make this single line thanks!

Just to clarify the final intended format:

invalid magic bytes: expected 0x01020304, got 0x10203040

and the one mentioned in #2083 (comment)

missing `TxOut` for input(s): aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:0, ...

Does this look okay? Thank you!

@Dmenec looks good thanks.

Nit: Message for the second one can be simplified to cannot calculate fee, missing output(s):. 😅 Messages for CalculateFeeError should mention that it's a calculate fee error?

Nit: Message for the second one can be simplified to cannot calculate fee, missing output(s):. 😅 Messages for CalculateFeeError should mention that it's a calculate fee error?

What do you think about cannot calculate fee, missing previous output(s)? Since we're referring to the outpoints of the inputs, I think it's clearer. Also agree on mentioning that it's a calculate fee error :)

oleonardolima

Overall it looks good, it just need to address Evan's comments.

oleonardolima · 2026-02-02T14:18:59Z

crates/chain/src/tx_graph.rs

+                    writeln!(f, "{}", shown[0])?;
+                    for op in &shown[1..] {
+                        writeln!(f, "{}", op)?;
+                    }


I don't think we need the multi-line now, can add in the future if needed. Also, what's the rationale for the leading whitespace ?

github-project-automation bot added this to BDK Chain Dec 11, 2025

ValuedMammal assigned Dmenec Dec 12, 2025

ValuedMammal moved this to In Progress in BDK Chain Dec 12, 2025

ValuedMammal added this to the Chain 0.24.0 milestone Dec 12, 2025

Dmenec force-pushed the fix/no-debug-on-display-impls branch from 492114b to b73016e Compare December 14, 2025 14:57

ValuedMammal mentioned this pull request Dec 28, 2025

[DRAFT] Update bdk_chain to latest bitcoindevkit/bdk_wallet#360

Draft

8 tasks

evanlinjin requested changes Jan 6, 2026

View reviewed changes

crates/chain/src/indexer/keychain_txout.rs Outdated Show resolved Hide resolved

crates/chain/src/indexer/keychain_txout.rs Outdated Show resolved Hide resolved

crates/chain/src/indexer/keychain_txout.rs Show resolved Hide resolved

Dmenec force-pushed the fix/no-debug-on-display-impls branch from b73016e to 532e2ec Compare January 14, 2026 16:04

evanlinjin approved these changes Jan 15, 2026

View reviewed changes

crates/chain/src/indexer/keychain_txout.rs Outdated Show resolved Hide resolved

Dmenec force-pushed the fix/no-debug-on-display-impls branch 3 times, most recently from 789d8a3 to d65eed0 Compare January 21, 2026 11:06

Dmenec force-pushed the fix/no-debug-on-display-impls branch from f0698e1 to d65eed0 Compare January 22, 2026 13:06

nymius reviewed Jan 23, 2026

View reviewed changes

crates/chain/src/tx_graph.rs Show resolved Hide resolved

nymius reviewed Jan 23, 2026

View reviewed changes

examples/example_esplora/src/main.rs Outdated Show resolved Hide resolved

nymius reviewed Jan 23, 2026

View reviewed changes

fix(chain)!: remove Debug usage and implement Display for InsertDescr…

81abded

…iptorError Replace Debug-based formatting with user-friendly Display messages.

Dmenec force-pushed the fix/no-debug-on-display-impls branch from d65eed0 to ae17df2 Compare January 27, 2026 13:50

nymius reviewed Jan 27, 2026

View reviewed changes

crates/file_store/src/lib.rs Outdated Show resolved Hide resolved

nymius reviewed Jan 27, 2026

View reviewed changes

examples/example_electrum/src/main.rs Outdated Show resolved Hide resolved

Dmenec added 2 commits January 27, 2026 17:30

docs(core): add detailed SyncRequest output example

adb5b99

fix(chain)!: remove Debug usage and implement Display for CalculateFe…

343c473

…eError Limited to 3 max shown items and added a suffix when there are additional entries.

Dmenec force-pushed the fix/no-debug-on-display-impls branch from ae17df2 to 91abd25 Compare January 27, 2026 16:37

fix(file_store)!: remove Debug usage and implement Display for StoreE…

75d1bd0

…rror. Format magic bytes as 0x-prefixed hex.

Dmenec force-pushed the fix/no-debug-on-display-impls branch from 91abd25 to 75d1bd0 Compare January 27, 2026 16:43

evanlinjin requested changes Jan 28, 2026

View reviewed changes

oleonardolima reviewed Feb 2, 2026

View reviewed changes

fix: no Debug on Display implementations #2083

Are you sure you want to change the base?

fix: no Debug on Display implementations #2083

Conversation

Dmenec commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changelog notice

Checklists

All Submissions:

Uh oh!

qatkk commented Dec 21, 2025

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evanlinjin commented Jan 14, 2026

Uh oh!

Dmenec commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Dmenec commented Jan 21, 2026

Uh oh!

oleonardolima commented Jan 21, 2026

Uh oh!

Uh oh!

nymius Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dmenec Jan 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dmenec Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleonardolima left a comment

fix: no `Debug` on `Display` implementations #2083

fix: no `Debug` on `Display` implementations #2083

Dmenec commented Dec 11, 2025 •

edited

Loading

Dmenec commented Jan 14, 2026 •

edited

Loading

nymius Jan 23, 2026 •

edited

Loading

Dmenec Jan 24, 2026 •

edited

Loading

Dmenec Jan 28, 2026 •

edited

Loading