Introduced support for backward-compatible BIP-329 import and BIP-329 export. Bounty by @nvk

[xavierfiechter]

No description provided.

[ecdsa]

Thanks for the pull request. A few remarks:

• I do not see why this should be a plugin. Plugins are generally associated with third party services.
• Electrum already has a format to import and export labels. I am not in favor of having two formats that do the same thing; that is going to confuse users and create technical debt. So, instead of adding a new format, why not replace the existing import/export format with BIP329?
• we have a plugin for synchronizing labels across wallet instances. That plugin might benefit from the new format.

[xavierfiechter]

Hi @ecdsa

@nvk has actually put up a bounty for the development of a BIP-329 label export plugin for Electrum, which was discussed in the Bitcoin Review podcast episode 51. This indicates a genuine interest within the community for this feature.

Plugins in general are designed to be non-invasive and extend the functionality. I did not want to remove existing features that others may rely on. So, plugins are not limited to third-party services IMHO, and they enhance the user experience by introducing new standards like BIP-329.

The motivation behind implementing BIP-329 is to establish a standardized format that works seamlessly across different Bitcoin wallet applications and services. This ensures the portability of labels, even in air-gapped environments.

You said, "Instead of adding a new format, why not replace the existing import/export format with BIP329?"

If you are open to accepting a pull request that replaces the current export/import format with BIP-329, I propose merging this PR now to make BIP-329 available as soon as possible. In the coming months, I can submit another PR to replace the current label export/import functionality with BIP-329 and remove the plugin, as suggested.

Why do I care?

I'd like to mention that I am actively working on labelbase.space, a service that leverages the BIP-329 standard to provide label synchronization across various wallets and systems. Labelbase offers both manual import/export options and API-based synchronization. This demonstrates real-world use cases for the proposed BIP-329.

Additionally, I have received a grant from OpenSats.org to continue developing Labelbase. This grant provides me with the flexibility to dedicate time and effort to such a pull request if Electrum is interested.

[SomberNight]

I concur with ecdsa. We do not want two formats for exporting labels. It just leads to user-confusion and maintenance burden for us. Now that there is a ~standardised format for it, it would be better to replace our custom format instead of also adding the new one. I don't see why this should be a plugin.

Also, we would also want to import using this format, not just export. I am not sure why you would only add export functionality.

---

Ideally, it would be nice if we could import using both the old format and the new one, but not expose this to the UI (instead, the parser should just automatically detect the format). Ideally there should be a unit test that imports labels using old format and another that imports labels using the new format.

Re exporting, only support the new format.

[xavierfiechter]

Okay, I'm going to replace the labels 'export' and 'import' while implementing BIP-329.

[xavierfiechter]

Okay, with commit e96d945 I eliminated the previously added plugin components and introduced support for backward-compatible BIP-329 import and BIP-329 export.

UTXOs are frozen or unfrozen based on the "spendable" parameter, if available.

I have successfully tested various scenarios. IMHO, this PR is now ready to review, and merge.

[ecdsa]

your PR includes commits from master.
please rebase your branch on current master to avoid that.
please also squash your commits; we do not need the old version with plugin in the history.

[xavierfiechter]

https://github.com/spesmilo/electrum/pull/8614/files shows the changes w/o the previous plugin code and w/o commits from master.

Do I really need to squash the commits? If so, I would still have to find out how squashing commits is done.

[accumulator]

Do I really need to squash the commits? If so, I would still have to find out how squashing commits is done.

First, rebase onto spesmilo/master to get rid of the merge commits

git rebase spesmilo/master

You can then combine the two remaining commits with

git reset --mixed HEAD^1
git commit -a --amend

finally, send the rewritten history to github:

git push -f

[xavierfiechter]

Okay, @accumulator @ecdsa @SomberNight now we have 1 commit.

Let me know if anything else should be modified.

[xavierfiechter]

"⚠️ Not enough compute credits to prioritize tasks! Task was canceled!"

Can the tests be triggered manually?

[ecdsa]

you can run the tests locally on your machine, for example
python3 -m unittest electrum/tests/test_lnpeer.py

[xavierfiechter]

Thank you for the hint.

My question was related to these tests.

[ecdsa]

you can see that the flake8 test is not passing. just click on the "details" link to see the error.

[xavierfiechter]

I see.

So this is an issue caused by the underlaying master? I'm aking because this PR has no relevance on the ./electrum/gui/qml/qeswaphelper.py:62:1file.

[xavierfiechter]

Got

 25.74 | I | channel_db.ChannelDB | SQL thread terminated
.
----------------------------------------------------------------------
Ran 39 tests in 25.170s

OK

on my local machine.

[ecdsa]

indeed, that flake8 issue comes from master. but it has been fixed two days ago.
I guess your branch is based on one of the faulty commits. why don't you rebase your branch?

git rebase master
git push -f

[xavierfiechter]

@ecdsa all green, all good.

[accumulator]

I'd rather see the code in electrum/bip329.py not taking a path parameter, but instead a stream object, so we can parse and serialize also from non-file resources (for instance label sync to a server), making it more generic.

So in short, refactor open(path) to outside of BIP329_Parser.

See also https://docs.python.org/3/library/io.html for how io.StringIO is used.

[xavierfiechter]

@accumulator I reworked the PR as requested.

[ecdsa]

It is a bit ugly to replicate the for loop that calls wallet.set_label. That code has been duplicated from wallet.py (legacy path) to bip329.py. In general, you should avoid copy-pasting blocks of code that you want it to be executed in different contexts. (I am not sure if that was already the case in previous versions of this pull request, or if this is a new addition). To avoid that, it would be better not to pass the wallet object to your module, and to return a dict to be used in wallet.import_labels.

Also, import labels calls set_frozen_state_of_coins. From the bip329 spec, it seems that the spendable attribute corresponds to a user decided-action. In that case, bip329 kind of collides with our frozen_addresses logic. It would be nice not to have both logics coexist; that may lead to inconsistencies.

Note: it seems that bip329 only considers the freezing of outputs, not of addresses. Any idea why?