Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

document disabling utxo_freeze_threshold #8035

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ln2max
Copy link

@ln2max ln2max commented Oct 24, 2022

Add documentation to the docstring of the appropriate function
explaining how to disable the #6960 test for dusting transactions

Why this is a problem: when testing new wallet configurations it is helpful to
make some very low value test transactions to ensure everything works (without
losing too much money if something is not correctly set up and the private key
is not recoverable).

In this case the test transaction may fall below the threshold.

TODO:

  1. it may make sense to document passing this option on the command
    line as well, since editing the config file by hand is a recipe for
    trouble.
    Unfortunately this is not as simple as doing:
electrum --unconf_utxo_freeze_threshold=0

and I can't find how electrum takes config arguments on the command
line. Maybe someone else knows?

  1. this is pretty deep in the codebase. Most users who run into this behaviour
    and find it to be erroneous may not read this far into the code. An alternate
    way to document it would be to include a note about the freezing behaviour in
    the error that pops up, if frozen funds are the reason a transaction cannot be
    sent.

Add documentation to the docstring of the appropriate function
explaining how to disable the spesmilo#6960 test for dusting transactions

TODO: it may make sense to document passing this option on the command
line as well, since editing the config file by hand is a recipe for
trouble.

Unfortunately this is not as simple as doing:
```
electrum --unconf_utxo_freeze_threshold=0
```
and I can't find how electrum takes config arguments on the command
line. Maybe someone else knows?
@SomberNight
Copy link
Member

SomberNight commented Oct 24, 2022

it may make sense to document passing this option on the command line as well, since editing the config file by hand is a recipe for trouble.
Unfortunately this is not as simple as doing:

electrum --unconf_utxo_freeze_threshold=0

and I can't find how electrum takes config arguments on the command line. Maybe someone else knows?

There are the setconfig/getconfig commands.

$ ./run_electrum -o getconfig unconf_utxo_freeze_threshold
$ ./run_electrum -o setconfig unconf_utxo_freeze_threshold 1234
true
$ ./run_electrum -o getconfig unconf_utxo_freeze_threshold
1234
$ ./run_electrum -o setconfig unconf_utxo_freeze_threshold null
true
$ ./run_electrum -o getconfig unconf_utxo_freeze_threshold
$

Users should not directly edit the config file. Not even power users IMHO.
I get it that a developer might, and that's fine, but this is not behaviour we want to encourage. Like you say, the syntax is non-trivial to get right.

An alternate way to document it would be to include a note about the freezing behaviour in
the error that pops up, if frozen funds are the reason a transaction cannot be sent.

The Qt GUI already has this.
If you try to send more than available, the error message e.g. says Not enough funds (0.041 mBTC (0.80 EUR) are frozen).
If you click "max" and everything is frozen, you get a similar error.

def get_text_not_enough_funds_mentioning_frozen(self) -> str:

If you click "max" and some (but not all) coins are frozen, there is a forced tooltip:
msg += "\n" + _("Some coins are frozen: {} (can be unfrozen in the Addresses or in the Coins tab)").format(frozen_bal)

Most users who run into this behaviour and find it to be erroneous may not read this far into the code.

If using the Qt GUI, as above, there are already hints that the utxo is frozen. (also e.g. the wallet balance piechart shows the coins as frozen)
If the user then goes to the Coins tab, they will see that's indeed the case and can manually unfreeze the utxo.

This config variable is not documented and I agree the behaviour might be unintuitive to users, but I think it would be overkill to expose it directly in the settings.

```
in ~/.electrum/config. Make sure to back up your config file first as
JSON syntax is somewhat difficult to get right, and the config file
will be overwritten with defaults if you get it wrong.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advice text here applies to editing the config in general. Should we maybe document setconfig and getconfig better instead?

- document that coins must be un-frozen using the Coins tab
- document using getconfig/setconfig as better than manually editing the
  config
@ln2max
Copy link
Author

ln2max commented Oct 27, 2022 via email

@SomberNight
Copy link
Member

SomberNight commented Oct 27, 2022

So I would suggest:

  1. having the freeze also show in the Addresses tab, not just in the Coins tab (which is not shown by default, and I think is not an intuitive place to look for un-freezing UTXOs).

In the Coins tab, there is one line per utxo, each with one column for outpoint and one column for address. Users can freeze addresses and coins ~independently. A coin being frozen is reflected by changing the background colour of the outpoint column, and an address being frozen has different bgcolor in the address column.
In the Addresses tab, it is not clear how to display "some coins are frozen" in the Addresses tab. That functionality is not there atm. I've sketched it out in #8040 but some more interface work would be needed (starting with figuring out how it should look like in the first place).

  1. documenting the setconfig/getconfig commands.
    I have updated my branch to reflect these changes.

I don't think it's a good approach to teach the reader about getconfig/setconfig in the docstring of every method that might use a config key. There should be a single (more central) place for it, e.g. it could go into simple_config.py.

@ln2max
Copy link
Author

ln2max commented Oct 28, 2022 via email

@ln2max
Copy link
Author

ln2max commented Oct 28, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants