Implement address labeling #1015

kristapsk · 2021-09-09T09:04:44Z

Adds new method setlabel to wallet-tool.py that allows to set label for wallet address. Setting it to empty string removes label. Labels are shown as additional column in display and displayall methods.

Full functionality for now is cli only, in Qt GUI is only read-only displaying implemented.

kristapsk · 2021-09-09T14:14:26Z

Could there be some other information indexed by addresses we would like to store in a wallet file in future? Then I should change AddressLabelsManager to AddressManager.

PulpCattel · 2021-09-19T14:47:12Z

I love the feature, tested a bit and I was able to create labels for both INTERNAL and EXTERNAL addresses, I was able to see them with both display and displayall method, and I was able to remove them with:

python3 wallet-tool.py joinmarket.jmdat setlabel tb1q*** ""

Maybe we could make it such that not providing a label arg, instead of throwing an error, removes the label.

For now cli only, GUI can be added later.

IDK if it was intentional, I can see the labels in QT: in the JM Wallet tab, under the Used/New column. They are kinda hidden ATM, and labels probably deserve their own column, but there is that.

PulpCattel · 2021-09-19T14:48:30Z

README.md

@@ -26,6 +26,7 @@ For a quick introduction to Joinmarket you can watch [this demonstration](https:
 * GUI to support Taker role, including tumbler/automated coinjoin sequence.
 * PayJoin - [BIP78](https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki) to pay users of other wallets (e.g. merchants), as well as between two compatible wallet users (Joinmarket, Wasabi, others). This is a way to boost fungibility/privacy while paying.
 * Protection from [forced address reuse](https://en.bitcoin.it/wiki/Privacy#Forced_address_reuse) attacks.
+* Address labeling


Suggested change

* Address labeling

* Address labeling.

Minor nit, we could make all this feature lines consistent by either always putting a dot at the end or by never putting it.

kristapsk · 2021-09-21T16:58:20Z

IDK if it was intentional, I can see the labels in QT

It wasn't intentional, I didn't test this with Qt. Will look into this.

kristapsk · 2021-09-21T21:13:19Z

Implemented proper displaying of labels in a separate column in Qt GUI.

PulpCattel · 2021-09-23T10:22:40Z

Tested eb94c5e:

QT) New column Label under JM Wallet tab.
QT) New right-click option to copy the label to clipboard, only appears for addresses that have a label, and if we remove the label the option correctly disappears.
Tested the exception when we pass it a valid but unknown (i.e., not in our wallet), address.

kristapsk · 2021-09-23T19:49:57Z

Maybe we could make it such that not providing a label arg, instead of throwing an error, removes the label.

Was thinking about this. User could accidentally forget to add that argument (for example, press Ctrl+V and Enter, but for some reason it wasn't in clipboard). Better approach in this case could be asking explicit question to user, like "Address label will be removed. Are you sure? (Y/N)".

PulpCattel · 2021-09-23T20:14:12Z

User could accidentally forget to add that argument

Makes sense, my feeling was that the convenience gain justifies the possible mistakes. If we were changing the label anyway, it shouldn't be a problem to lose it by accident, we can just set it to the new desired value right after.
And if the problem is that we may not notice we removed it, then I guess we could have the same problem regardless, because we may not notice we copy-pasted the wrong label, or we didn't press enter, etc. I.e., user should double-check the correct label has been inserted anyway.

Adding an extra interaction may also hurt a little bit external scripts that want to work with labels.

For me it's fine either way.

kristapsk · 2021-09-23T20:29:26Z

Added returning label also in wallet-tool.py showutxos and displaying in Qt GUI's Coins tab.

kristapsk · 2021-09-23T20:31:15Z

@PulpCattel I don't have so strong opinion here. Maybe others will have thoughts on this too. In any case, seems like a minor detail.

kristapsk · 2021-10-16T08:56:49Z

Nobody else plans to review / test this?

Also this is still an open question:

Could there be some other information indexed by addresses we would like to store in a wallet file in future? Then I should change AddressLabelsManager to AddressManager.

jmclient/jmclient/wallet.py

jmclient/jmclient/wallet_utils.py

AdamISZ · 2021-10-17T20:27:51Z

jmclient/jmclient/wallet_utils.py

@@ -1584,6 +1602,12 @@ def wallet_tool_main(wallet_root_path):
            jmprint("args: [master public key]", "error")
            sys.exit(EXIT_ARGERROR)
        return wallet_createwatchonly(wallet_root_path, args[1])
+    elif method == "setlabel":
+        if len(args) < 4:
+            jmprint("args: address label", "error")


I think it's certainly very reasonable to have the mechanism for removing a label be setlabel address "", but maybe someone might not be 100% sure about that. A very minor comment though, feel free to leave it.

AdamISZ · 2021-10-17T20:28:52Z

My testing so far seems to show this as working correctly, except:

If you try to setlabel on a timelocked address, it didn't show an error (or crash) but the label was not set. I haven't traced it in the code yet, it may be something simple.

AdamISZ · 2021-10-21T17:14:04Z

I want to drop this note before I forget: now that we have an RPC exposed, we have to be careful about any changes to the structure of what is returned by wallet_display ; it may be harmless to add another field or it may not but it must be checked.

kristapsk · 2021-10-23T18:20:27Z

now that we have an RPC exposed, we have to be careful about any changes to the structure of what is returned by wallet_display

@AdamISZ Shouldn't jmclient/test/test_wallet_rpc.py catch these?

AdamISZ · 2021-10-24T08:14:47Z

now that we have an RPC exposed, we have to be careful about any changes to the structure of what is returned by wallet_display

@AdamISZ Shouldn't jmclient/test/test_wallet_rpc.py catch these?

Mostly yes, but there is a subtlety, which I was unsure about when writing the above: the wallet_display will return a big json object. Even if one checks every key at every level of the tree, it doesn't necessarily mean it's wrong to have another key which is not documented. For now we don't check that anyway, but I'm saying if we did it wouldn't necessarily error and it wouldn't necessarily be a failure vector (the client could just ignore it), but we might want them to know about the new feature.

AdamISZ · 2021-10-25T20:35:12Z

Nobody else plans to review / test this?

Also this is still an open question:

Could there be some other information indexed by addresses we would like to store in a wallet file in future? Then I should change AddressLabelsManager to AddressManager.

It's a bit unclear to me. Note for example that Wasabi (iirc, it was certainly like this when they started), labels utxos not addresses. I'm not actually that clear, now I think of it, why you chose to go with address rather than utxo labels. Note that, when I implemented freezing of utxos, I did it by adding a metadata dict to the persisted data in the wallet, so that we remember it, but disabled is just one key in that dict, leaving it possible for us to add others (like a label field for example).

joinmarket-clientserver/jmclient/jmclient/wallet.py

Line 224 in d5299c4

if b'disabled' not in self._utxo_meta[(txid, index)]:

If I was going to do labelling, I probably would have done that. I'm sure a case can be made for a per-address label, but it's probably better if the user understands to label specific utxos. Also this would have meant we don't need another Manager object.

PulpCattel · 2021-10-26T07:34:05Z

Note for example that Wasabi (iirc, it was certainly like this when they started), labels utxos not addresses.

I clearly remember that Wasabi creates a label as soon as you generate an address, when there is no UTXO yet, and this string is saved in the wallet.json file on disk.
By a not-so-deep search, it seems Wasabi uses, or at least used, a combo of address labels and transaction labels to automatically generate a coin/UTXO label.
An address gets a label when you generate it, then this label is concatenated with the label that you set each time you make a transaction spending from that address.

zkSNACKs/WalletWasabi#2455 (comment)
https://docs.wasabiwallet.io/using-wasabi/Receive.html#the-importance-of-labeling

AdamISZ · 2021-10-26T08:14:06Z

@PulpCattel thanks for figuring it out in detail, it's clearly somewhat relevant to the discussion here. I had the privilege of doing literally the first mainnet transaction with Wasabi's first released version :) I remember the guys pointing out to me that I was forced to label what I remember as being a utxo, but given that doc, most likely my memory is wrong and it was an address.

It's interesting for these wallets because of course we have this one-usage rule meaning that probably 95-99% of the time the distinction will be academic. But I suppose a good argument is: since addresses effectively label utxos themselves with that pseudonym, there wouldn't be a point in labelling utxos differently (arguable point, but you can say that). So labels at an address level are probably more intuitive.

Given all this I guess we should stick with @kristapsk 's current approach.

kristapsk · 2021-10-26T08:18:25Z

Proper way is to label addresses, because any UTXOs related to the same address are also related from blockchain analysis perspective. Also, as said above, more intuitive from user's perspective.

So, only question remains - should it be AddressLabelsManager or AddressManager...

PulpCattel · 2021-10-26T08:20:59Z

Another cool feature of labeling addresses, is being able to do it before actually having any coin. So, for example, one can reserve an address for something and having it labeled as such. We can't, at least trivially, do that with UTXOs because we do not have one yet.

AdamISZ · 2021-10-26T08:21:34Z

I mean I wouldn't worry about the name; that can always be changed since it's internal. The only question to be concerned about is the structure of data persisted in the file. In the case of the utxo manager, when I added the metadata key it was done in such a way that previously created wallets could be opened fine, because there was an 'if metadata not in keys of dict, ignore' logic was used. In other words, I'm sure we can make new data in future that's backwards compatible.

AdamISZ · 2021-10-26T08:34:55Z

Just for the sake of communication clarity: from my POV this is basically ready to go except for the timelock address case I mentioned (which may be trivial or not, I just didn't look at it).

kristapsk · 2021-10-26T08:51:35Z

Another cool feature of labeling addresses, is being able to do it before actually having any coin.

Yes, that's actually one of the use cases for me personally. I could specify JM address for something where payment will happen weeks or months later, very easy to forgot.

except for the timelock address case I mentioned (which may be trivial or not, I just didn't look at it).

Almost forgot that one, will look into that (unless somebody else does).

kristapsk · 2021-10-29T22:22:19Z

You cannot get / set labels for timelocked addresses because set_address_label() and get_address_label() calls is_known_addr() which is defined in BaseWallet and does not know anything about timelocked addresses. It's late now, will look into that tomorrow.

kristapsk · 2021-10-29T22:23:46Z

Or not. There is no exception raised, hmmm... Anyway, will look into that tomorrow. :)

AdamISZ · 2021-10-30T09:33:39Z

I took a quick look. You just need to repeat your references to label here:

joinmarket-clientserver/jmclient/jmclient/wallet_utils.py

Lines 473 to 484 in d5299c4

    
           label = wallet_service.get_address_label(addr) 
        
           balance, used = get_addr_status( 
        
               path, utxos[m], k >= unused_index, address_type) 
        
           if showprivkey: 
        
               privkey = wallet_service.get_wif_path(path) 
        
           else: 
        
               privkey = '' 
        
           if (displayall or balance > 0 or 
        
                   (used == 'new' and address_type == 0)): 
        
               entrylist.append(WalletViewEntry( 
        
                   wallet_service.get_path_repr(path), m, address_type, k, addr, 
        
                   [balance, balance], priv=privkey, used=used, label=label))

in the section starting on line 490, for fidelity bond mixdepth.

kristapsk · 2021-10-30T13:05:12Z

@AdamISZ Thanks!

kristapsk · 2021-10-30T13:07:11Z

So this looks ready, I plan to squash and merge this in a few days, unless there are some more comments.

AdamISZ · 2021-10-30T13:45:07Z

tACK on 9defcd7 - specifically, tested setting labels and deleting labels works including fidelity bonds and including Qt.

kristapsk force-pushed the address-labels branch from 11341fa to b7fc070 Compare September 9, 2021 09:33

PulpCattel reviewed Sep 19, 2021

View reviewed changes

kristapsk force-pushed the address-labels branch from b7fc070 to eb94c5e Compare September 21, 2021 21:12

kristapsk force-pushed the address-labels branch from eb94c5e to fe9fa71 Compare September 23, 2021 20:26

kristapsk force-pushed the address-labels branch from fe9fa71 to b97b044 Compare September 23, 2021 21:23

AdamISZ reviewed Oct 17, 2021

View reviewed changes

jmclient/jmclient/wallet.py Show resolved Hide resolved

AdamISZ reviewed Oct 17, 2021

View reviewed changes

jmclient/jmclient/wallet.py Outdated Show resolved Hide resolved

AdamISZ reviewed Oct 17, 2021

View reviewed changes

jmclient/jmclient/wallet_utils.py Outdated Show resolved Hide resolved

AdamISZ reviewed Oct 17, 2021

View reviewed changes

Implement address labeling

21c0e3e

kristapsk force-pushed the address-labels branch from 9defcd7 to 21c0e3e Compare November 2, 2021 09:32

kristapsk merged commit b1b53a0 into JoinMarket-Org:master Nov 2, 2021

kristapsk deleted the address-labels branch November 2, 2021 09:44

kristapsk mentioned this pull request Dec 16, 2021

API: Labels missing for wallet branch entries #1118

Closed

Reckless-Satoshi mentioned this pull request Jul 28, 2022

Add address labeling function to wallet menu openoms/joininbox#87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement address labeling #1015

Implement address labeling #1015

kristapsk commented Sep 9, 2021 •

edited

kristapsk commented Sep 9, 2021

PulpCattel commented Sep 19, 2021 •

edited

PulpCattel Sep 19, 2021

kristapsk commented Sep 21, 2021 •

edited

kristapsk commented Sep 21, 2021

PulpCattel commented Sep 23, 2021

kristapsk commented Sep 23, 2021

PulpCattel commented Sep 23, 2021

kristapsk commented Sep 23, 2021

kristapsk commented Sep 23, 2021

kristapsk commented Oct 16, 2021

AdamISZ Oct 17, 2021

AdamISZ commented Oct 17, 2021

AdamISZ commented Oct 21, 2021

kristapsk commented Oct 23, 2021

AdamISZ commented Oct 24, 2021

AdamISZ commented Oct 25, 2021

PulpCattel commented Oct 26, 2021 •

edited

AdamISZ commented Oct 26, 2021

kristapsk commented Oct 26, 2021

PulpCattel commented Oct 26, 2021

AdamISZ commented Oct 26, 2021

AdamISZ commented Oct 26, 2021

kristapsk commented Oct 26, 2021

kristapsk commented Oct 29, 2021

kristapsk commented Oct 29, 2021

AdamISZ commented Oct 30, 2021

kristapsk commented Oct 30, 2021

kristapsk commented Oct 30, 2021

AdamISZ commented Oct 30, 2021

Implement address labeling #1015

Implement address labeling #1015

Conversation

kristapsk commented Sep 9, 2021 • edited

kristapsk commented Sep 9, 2021

PulpCattel commented Sep 19, 2021 • edited

PulpCattel Sep 19, 2021

Choose a reason for hiding this comment

kristapsk commented Sep 21, 2021 • edited

kristapsk commented Sep 21, 2021

PulpCattel commented Sep 23, 2021

kristapsk commented Sep 23, 2021

PulpCattel commented Sep 23, 2021

kristapsk commented Sep 23, 2021

kristapsk commented Sep 23, 2021

kristapsk commented Oct 16, 2021

AdamISZ Oct 17, 2021

Choose a reason for hiding this comment

AdamISZ commented Oct 17, 2021

AdamISZ commented Oct 21, 2021

kristapsk commented Oct 23, 2021

AdamISZ commented Oct 24, 2021

AdamISZ commented Oct 25, 2021

PulpCattel commented Oct 26, 2021 • edited

AdamISZ commented Oct 26, 2021

kristapsk commented Oct 26, 2021

PulpCattel commented Oct 26, 2021

AdamISZ commented Oct 26, 2021

AdamISZ commented Oct 26, 2021

kristapsk commented Oct 26, 2021

kristapsk commented Oct 29, 2021

kristapsk commented Oct 29, 2021

AdamISZ commented Oct 30, 2021

kristapsk commented Oct 30, 2021

kristapsk commented Oct 30, 2021

AdamISZ commented Oct 30, 2021

kristapsk commented Sep 9, 2021 •

edited

PulpCattel commented Sep 19, 2021 •

edited

kristapsk commented Sep 21, 2021 •

edited

PulpCattel commented Oct 26, 2021 •

edited