Feat: Add bip329 wallet labels#266
Conversation
- Loads the \'Labels\' state from \'<wallet>.labels.jsonl\' after \'load_wallet_config\'. - Updates \'handle_offline_wallet_subcommand\' to accept mutable label state. - Exports and saves the in-memory labels to disk before the command return.
- Injects label lookups into the `unspent` and `transactions` match arms. - Updates the `--pretty` table formatting to include a new 'Label' column (using an em dash for missing labels). - Appends the label string to the standard JSON output
- Introduces the `Label` variant to `OfflineWalletSubCommand` with strict `clap` conflict rules. - Adds a `parse_txid` helper to utils.rs. - Implements an `add_or_add_update_label` helper in `handlers.rs` to mutate the in-memory label state in place.
|
Hi @tvpeter, I am done with this. You can check it out when you are free. I will start working on the follow-up PR to add import and export of label files. |
- Introduces `ImportLabels` and `ExportLabels` variants to `OfflineWalletSubCommand` for CLI parsing. - Implements label merging from external JSONL files using `try_from_file` and `add_or_update_label`. - Enables backing up the current label state to a user-specified path using `export_to_file`.
|
Hi @tvpeter, I eventually decided to add the import and export of BIP-329 JSONL files' implementation here when, after I was done with the implementation, I saw that it was something that could fit into this PR without bloating it. So, instead of creating a second PR and having reviewers go through the stress of reviewing two PRs that can probably fit into one, I decided to make it one PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #266 +/- ##
==========================================
- Coverage 11.13% 10.59% -0.55%
==========================================
Files 8 8
Lines 2488 2615 +127
==========================================
Hits 277 277
- Misses 2211 2338 +127
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This looks like a very useful feature and one that I'd love to see in the It will be a little more complicated than |
|
Thanks, @notmandatory, for the review and the suggestion! I really like the idea of creating a new Regarding the persistence trait to tie into the user's choice of database, I was thinking we could build an interface that lets users store labels internally in their database of choice (like SQLite or Redb). To keep it efficient, we could mirror bdk_wallet's approach by abstracting the backend and using a changeset pattern to persist only the diffs. To maintain strict BIP-329 compliance, the crate would then provide dedicated import and export methods to convert the internal database state to and from the standard JSONL format. Does this approach align with what you have in mind? |
|
Yes this sounds like a good approach. |
|
Hey @Musab1258, glad to see you're working on this. I've been playing around with bdk-cli and had some thoughts around this My thinking is to create a
Just brainstorming somethings that I think would be useful. Would love to hear know your thoughts |
|
@notmandatory, since I don't have permissions to create a repository in the bitcoindevkit organization, what should the next steps be? Would you set up an empty repository within the organization (similar to how bdk-bip322 is set up) for me to work from?
|
|
@brh28, these are really nice ideas for expanding the labeling functionality. Actually, some of the features you mentioned (like importing a BIP-329 JSONL file and adding labels to existing wallet items) are already implemented in my current PR here. Since @notmandatory suggested implementing it in a new bdk_bip329 crate, we can implement the existing add, import, and export functionalities first. Then we can add the multi-tagging and filter logic later. Also, the examples you made (like cat mywallet.jsonl | bdk_labels import --bip329 and the rest) show that you wanted the tool to be a CLI. But, I think what @notmandatory wants is a crate that can be integrated into other crates and CLI tools @notmandatory may also have something to say about this.
|
|
Thanks @Musab1258 I created my own repo bdk-labels to start implementing some of these features. As you pointed out, many of the features overlap with what you're working on, so it's quite similar to your PR, but perhaps it'll be of interest to you. The biggest feature difference is
|
| LabelRef::Address(addr) => format!("addr:{}", addr.assume_checked_ref()), | ||
| LabelRef::Output(op) => format!("output:{op}"), | ||
| LabelRef::Input(op) => format!("input:{op}"), | ||
| _ => item_ref.to_string(), // Fallback for pubkey/xpub |
There was a problem hiding this comment.
why fallback, rather than handle other types?
Go ahead and create a new repo in your personal account and we can move it into the bitcoindevkit org when it's ready. I also prefer @brh28 's repo name |
@Musab1258 you're correct that I do have a general purpose library in mind for this feature, not something only for use in |
| })?; | ||
|
|
||
| for label in imported_labels.into_iter() { | ||
| add_or_update_label(labels, label); |
There was a problem hiding this comment.
To avoid incidental data loss, I would suggest adding an --override flag on import. If the flag is not provided, either the command fails, or only new labels are added
|
@notmandatory, I have set up the bdk-labels repository on my personal account. Here it is: bdk-labels. @brh28, I would love to add you as a collaborator so we can work together on this crate. I have sent you a collaboration invite. You should receive it in the mail associated with your GitHub account. When will you be free so we can discuss and plan out the building of the crate? |
|
Yeah man, I'll mention here that I'm mainly interested in serving that tagging feature at the moment as it helps with a particular problem I'm having, but I really like the idea of a wallet cli tool, so happy to help where I can |
|
@notmandatory Is it just the Perhaps even a new |
Yes I'm thinking the |
|
Hi @notmandatory, I am done with the implementation, testing, and documentation of the bdk-labels. Can you please take a look at it here when you are free? |
|
Hi @brh28, I am done with the implementation, testing, and documentation of the bdk-labels. Can you please take a look at it here when you are free?
I think the crate is ready for any feature (tagging, for instance) that we might want to add to it.
But, since tagging is not BIP 329 compliant, the bip329 crate we are using in this crate does not implement it. I feel like if we are to do it and still make our bdk-labels crate BIP329 compliant, We should build an abstraction layer on top of the normal labeled wallet (in this bdk-label crate), which implements the tagging feature. My fear is that if we add it directly, the labels in bdk-wallet will not be compatible with other wallets that are BIP329 compliant. |
|
@thunderbiscuit to keep the discussion in one place I'm commenting on your Musab1258/bdk-labels#1 here. I like the idea of keeping the labels feature independent of the Sticking with the
Or we could go all the way and add labels directly as a new field in |
|
I made use of the LabelledWallet adapter because I wanted to add a changeset to the wallet created from bdk_wallet (i.e., a wallet with a label changeset). The pattern allows anyone using the label to add a label(s), validate it against the wallet, and store the result in the changeset. One of the reasons why the changeset is needed in this crate is that it acts as a diff or a staging area. And if it is not present, the crate would have to overwrite the entire database or rewrite the entire JSONL file every single time a user adds a label. But if I am to remove the adapter pattern, the crate will not be able to attach its LabelChangeset to the user's wallet. Unless I implement the LabelChangeset logic directly in bdk-wallet. Which kind of reduces the essence of this crate. But I am willing to move forward with any consensus agreement that is made here. |
va-an
left a comment
va-an
left a comment
There was a problem hiding this comment.
@Musab1258 thanks for the work on BIP-329 labels! Left a few inline comments, please take a look.
One more thing: the address case of label looks write-only. label --address stores an addr record in labels.jsonl, but nothing reads it back - new_address/unused_address don't show a label, and unspent/transactions only surface utxo and txid labels. So an address label goes into the file and becomes invisible, unlike txid/utxo. Maybe display address labels somewhere, or drop --address until they're readable.
Also, don't forget to rebase on master before further changes!
| .iter() | ||
| .find(|l| format_ref_str(&l.ref_()) == target_ref_str) | ||
| .and_then(|l| l.label().map(|s| s.to_string())) | ||
| .unwrap_or_else(|| "—".to_string()) |
There was a problem hiding this comment.
Unlabeled items render as the literal string —. User can set a real label of — and JSON then can't tell "no label" from a — label.
Reproduction (regtest, two received txs, label — set on the first only):
# 1. both txs render "—", though no labels set yet
$ bdk wallet --wallet label_demo transactions | jq '.[] | {txid, label}'
{ "txid": "5fe2524f0c46bb12767d1865114031cd631578ac05422a767c0edf23f5c0aeaa", "label": "—" }
{ "txid": "55d2b85dbf49d78c7070d83f20443e694d90a65498d44bbe84f3c7aa0b55a465", "label": "—" }
# 2. labels file is empty so the "—" above is the default, not a stored label
$ cat labels.jsonl
# 3. set a real "—" label on the first tx only
$ bdk wallet --wallet label_demo label "—" --txid 5fe2524f0c46bb12767d1865114031cd631578ac05422a767c0edf23f5c0aeaa
# 4. file now has exactly one entry, for the first tx
$ cat labels.jsonl
{"type":"tx","ref":"5fe2524f0c46bb12767d1865114031cd631578ac05422a767c0edf23f5c0aeaa","label":"—"}
# 5. output is unchanged - both still "—", labeled and unlabeled indistinguishable
$ bdk wallet --wallet label_demo transactions | jq '.[] | {txid, label}'
{ "txid": "5fe2524f0c46bb12767d1865114031cd631578ac05422a767c0edf23f5c0aeaa", "label": "—" }
{ "txid": "55d2b85dbf49d78c7070d83f20443e694d90a65498d44bbe84f3c7aa0b55a465", "label": "—" }
Fix: return Option<String> (None when absent) so JSON serializes null for unlabeled and "—" only for a real label.
P.S. bdk is my custom zsh fn for bdk-cli with additional cli-args.
| BipLabel::Output(OutputRecord { | ||
| ref_: outpoint, | ||
| label: Some(label_str.clone()), | ||
| spendable: false, |
There was a problem hiding this comment.
label --utxo hardcodes spendable: false, which in BIP-329 marks the UTXO as frozen. Naming a coin shouldn't freeze it. And it can't just be flipped to a hardcoded true either - any fixed value clobbers the existing spendable - a true would silently unfreeze a UTXO the user had frozen, on a mere rename. A naming command must not touch spendable at all.
bdk-cli ignores the field locally, but BIP-329 is for interchange - importing into Sparrow/Nunchuk/etc. shows the UTXO as frozen.
Reproduced (regtest) - just naming a coin writes spendable:false:
$ bdk wallet --wallet label_demo label "my coin" --utxo 55d2b85dbf49d78c7070d83f20443e694d90a65498d44bbe84f3c7aa0b55a465:0
$ cat labels.jsonl | jq
{ "type": "output", "ref": "55d2b85dbf49d78c7070d83f20443e694d90a65498d44bbe84f3c7aa0b55a465:0", "label": "my coin", "spendable": false }
Fix: preserve the existing record's spendable (read-modify), default to true only for a new record - never hardcode it.
| let value = | ||
| handle_offline_wallet_subcommand(wallet, wallet_opts, cli_opts, offline_subcommand) | ||
| .map_err(|e| e.to_string())?; | ||
| let mut labels = bip329::Labels::default(); |
There was a problem hiding this comment.
In REPL mode label reading doesn't seem to work - the "my coin" label shows up outside REPL but is empty inside.
$ bdk wallet --wallet label_demo unspent | jq '.[] | {outpoint, label}'
{ "outpoint": "55d2b85dbf49d78c7070d83f20443e694d90a65498d44bbe84f3c7aa0b55a465:0", "label": "my coin" }
$ bdk repl --wallet label_demo
> wallet unspent
[ { ... "outpoint": "55d2b85dbf49d78c7070d83f20443e694d90a65498d44bbe84f3c7aa0b55a465:0", "label": "—" ... } ]
| let mut labels = match bip329::Labels::try_from_file(&label_file_path) { | ||
| Ok(loaded_labels) => loaded_labels, | ||
| Err(bip329::error::ParseError::FileReadError(io_err)) | ||
| if io_err.kind() == std::io::ErrorKind::NotFound => | ||
| { | ||
| bip329::Labels::default() | ||
| } | ||
| Err(e) => return Err(Error::Generic(format!("Failed to load labels: {e}"))), | ||
| }; |
There was a problem hiding this comment.
Noticed that every wallet command loads (and rewrites) labels.jsonl, and the load is a hard error. Combined with export_to_file writing the file without a trailing newline, appending a record - the usual way to add a line to JSONL - glues it onto the last line and makes the file invalid. After that any command fails, even ones unrelated to labels like balance:
$ echo '{"type":"output","ref":"5fe2524f0c46bb12767d1865114031cd631578ac05422a767c0edf23f5c0aeaa:0","label":"x"}' >> labels.jsonl
$ bdk wallet --wallet label_demo balance
[ERROR bdk_cli] Generic error: Failed to load labels: Unable to parse file: trailing characters at line 1 column 129
So a labels file touched by any other tool (or a partial write) can block the whole wallet CLI, including commands that have nothing to do with labels. Might be worth a trailing newline on export and/or skipping a bad labels file instead of failing the command.
4 participants
Description
This PR adds BIP-329 wallet label support to
bdk-cli, to fix #184. It uses the[bip329]crate to manage label data, persists it in a separatelabels.jsonlfile within the wallet's data directory (e.g.~/.bdk-bitcoin/<wallet_name>/labels.jsonl), and keeps it decoupled from the wallet's SQLite/redb database. It also exports and imports label files.This PR includes the following functionality:
labeloffline subcommand to attach a human-readable label to either a transaction ID, address, or UTXO (outpoint).unspentandtransactionscommands now include aLabelcolumn in both--prettytable output and standard JSON output, showing the stored label or an em dash if none exists.import_labelssubcommand merges labels from an external BIP-329 JSONL file into the wallet's current label state. If a label for the same item already exists, it is overwritten by the imported one.export_labelssubcommand writes the wallet's current label state to a user-specified BIP-329 JSONL file, suitable for backup or use in other BIP-329 compliant wallets.Notes to the reviewers
labels.jsonlfile rather than inside the wallet database. This keeps the label format portable and interoperable with other BIP-329 compliant wallets.Labelimport frombip329is aliased asBipLabelinhandlers.rsto avoid a name clash with theOfflineWalletSubCommand::Labelvariant brought into scope viause crate::commands::OfflineWalletSubCommand::*.import_labelscommand uses the existingadd_or_update_labelhelper to merge incoming labels one at a time, new label references are inserted, and existing label references are overwritten by the imported value.export_labelscommand writes to a user-specified path, which is independent of the wallet's internallabels.jsonlfile. The internal file is always managed automatically by the save-on-exit mechanism, so the two files remain separate.Changelog notice
Added
labelsubcommand toOfflineWalletSubCommandfor tagging transactions, addresses, and UTXOs with human-readable labels.import_labelssubcommand to merge labels from an external BIP-329 JSONL file into the wallet's current label state.export_labelssubcommand to back up the wallet's current label state to a user-specified BIP-329 JSONL file.labels.jsonlfile in the wallet data directory.Labelcolumn inunspentandtransactionsoutput (both--prettyand JSON).parse_txidhelper inutils.rs.bip329 = "0.4.0"dependency inCargo.toml.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
CHANGELOG.mdBugfixes: