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

Allow disabling of BIP69 on transaction send window #9252

Open
errge opened this issue Oct 16, 2024 · 12 comments · May be fixed by #9269
Open

Allow disabling of BIP69 on transaction send window #9252

errge opened this issue Oct 16, 2024 · 12 comments · May be fixed by #9269

Comments

@errge
Copy link

errge commented Oct 16, 2024

Unfortunately the world created a protocol, which requires the end-user to send transaction in a very specific format (the outputs ordering and the exact spot of the memo (OP_RETURN script) matters.

The protocol is called Thorchain and you can see the docs here: https://dev.thorchain.org/concepts/sending-transactions.html#utxo-chains

As you can see, they say very explicitly in which order the transaction has to order the outputs, and they even have a danger field saying that otherwise funds will be lost. It's kind of unfortunate they call this "randomised" order, because the whole point of BIP69 is to be consistent, not random, but well, thor people are runed :)

Would it be possible to add a feature to electrum similar to "Enable output value rounding" called "Do not reorder outputs (disable BIP69)"? Would a PR to that effect be appreciated?

@SomberNight
Copy link
Member

somewhat related #8849

@errge
Copy link
Author

errge commented Oct 16, 2024

If BIP-69 is going to be removed anyway, an alternative quick fix for Thor compatibility (if that's a goal at all, I guess), would be to add a heuristic: turn off BIP-69 if any of the outputs are a "script(...)" output from the end-user. Anybody entering something like that, should surely know how to check their ordering on the confirmation screen before sign+broadcast...

@SomberNight
Copy link
Member

Well the idea there is to replace bip69 with random order, so that would not help your use case either.

To be clear, in your use case, you are explicitly specifying the "change" output also as part of pay-to-multi, right? Because if you don't allocate all the coins and let Electrum create a change output automatically, that too can destroy your ordering (where should it put that?).

@errge
Copy link
Author

errge commented Oct 16, 2024

Yes, I'm being explicit about change with a "!" line, for sure.

So, I think this is how we could write out the heuristics, that I'd prefer:

  • if "script(...)" is being used, then disable BIP-69 ordering,
  • if change is needed (e.g. not all inputs are used), put the change at the end (and maybe display a warning that change is at the end),
  • if change is not needed (e.g. inputs == outputs), just use the provided ordering.

Alternatively here is a stricter heuristics, that fires less, but still let's experts deal with protocols like thor:

  • if "script(...)" is being used AND inputs == outputs, disable BIP-69.

In both cases, an expert end-user can recreate any transaction needed for thorswap (and maybe for other protocols).

@errge
Copy link
Author

errge commented Oct 16, 2024

Actually, if random gets implemented, it's still better for me than today, because I can just retry 3-6 times until I get the wanted ordering given 3 outputs. :)

But this is more a joke than a serious resolution proposal.

@SomberNight
Copy link
Member

SomberNight commented Oct 16, 2024

We could have a config key wallet_sort_tx_io, which can take string literal values "bip69" or "manual" (or in the future e.g. "random"). Whether we expose that config key to the GUI is another question, but for the expert use case, you could just set it via the CLI or Qt console.

EDIT: I don't think heuristics are a good fit here. Also, note that this particular protocol is using raw scripts (op_return in this case) but another might use p2sh/p2wsh/etc-style hidden scripts (i.e. addresses) and still require a specific ordering of outputs.

@errge
Copy link
Author

errge commented Oct 16, 2024 via email

@SomberNight
Copy link
Member

SomberNight commented Oct 16, 2024

Well if this feature is important for you then feel free to do it and create a PR. :)

@errge errge linked a pull request Oct 21, 2024 that will close this issue
@errge
Copy link
Author

errge commented Oct 21, 2024

Prepared the PR, no heuristics, and UI is presented.

Good point about hidden scripts, better to be explicit.

@ecdsa
Copy link
Member

ecdsa commented Nov 13, 2024

Could you explain why the Thorchain protocol does require a certain output order?
If the user is at risk of losing funds because they might enter a wrong order in the GUI, this looks like a footgun to me.
Similarly, the user might forget to enable/disable bip69 in the PR you proposed.

I think it would be safer to write a plugin for Thorchain, that ensures the transaction is correct.
Note that release 4.6 of Electrum will allow users to install third-party plugins, that do not need to be approved by us.

@errge
Copy link
Author

errge commented Nov 14, 2024

I'm not affiliated with Thorchain, so I can not explain the design choices of their protocols. Money is not lost to my knowledge, worst case is transaction reject and return of funds, as far as I'm aware.

https://dev.thorchain.org/concepts/sending-transactions.html

My opinion is, that designing custom plugins explicitly for a use-case is of course good in the long run and provides the best possible UI, but I also don't see why is it a big deal, to provide the feature to be able to precisely specify my transaction output, how I want it to look.

Once more, I'm not a Thorchain person, and I'm not hired to write plugins or support the exchange officially. I wanted to use electrum for my own use as a tool for myself (not for anybody else or in any official capacity), and electrum failed me as a power user, by not being able to turn off the reordering of output legs, even when I fully specified the transaction with the already included transaction/script DSL (that is impossible to find for a beginner). So I opened a pull request, in the hopes that I can make this amazing tool better.

Electrum (very correctly IMHO) already supports features that are not the most secure recommendations from the original bitcoin papers/designs, e.g. disabling of change addresses.

I understand if this PR is a reject, but I would like to ask you once more to reconsider. Thank you for your attention!

@errge
Copy link
Author

errge commented Nov 14, 2024

If you are worried about people losing money and blaming you, I'm happy also with the power-user solution: we remove the UI part of this pull request, and user has to set the flag themselves, no way for a beginner to set the flag then.

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 a pull request may close this issue.

3 participants