Repository navigation
commands: accept script(...) outputs in payto/paytomany, like the GUI - #11007
devdavidejesus wants to merge 3 commits into
Conversation
Coins sent to an OP_RETURN output are unspendable. With script(...) outputs, 'payto "script(OP_RETURN ...)" !' would put the whole wallet balance into the OP_RETURN output, and the CLI has no confirmation dialog like the GUI. Like bitcoind, whose "data" outputs always have zero value, require the amount of an OP_RETURN output to be 0. Other scripts can still carry value.
|
Added a guard: the amount of an OP_RETURN output must be 0, like bitcoind's "data" outputs. Otherwise |
| scriptpubkey = PaymentIdentifier.parse_script(m.group(1)) | ||
| except Exception as e: | ||
| raise UserFacingException(f"Invalid script: {m.group(1)!r}") from e | ||
| if scriptpubkey[:1] == bytes([opcodes.OP_RETURN]) and amount_sat != 0: |
There was a problem hiding this comment.
What if the user wants to burn coins? Not sure if we want to disallow this, if the user pays to scripts they should know how it works, and the Qt GUI allows doing it too.
Note by default Bitcoin Core will reject these transactions anyway when trying to broadcast, see maxburnamount in https://bitcoincore.org/en/doc/31.0.0/rpc/rawtransactions/sendrawtransaction/
There was a problem hiding this comment.
IMO non-zero amounts for OP_RETURN on the CLI should be allowed only iff --iknowwhatimdoing is passed
There was a problem hiding this comment.
If we add an additional iknowwhatimdoing parameter i propose just gating the whole script output functionality behind it. There are other ways beside OP_RETURN scripts that allow burning coins too.
But still in the desktop GUI we allow all of this without any special treatment (and i guess it is realistic to assume CLI users being more technical than GUI users).
There was a problem hiding this comment.
Looked into this a bit more before answering (my earlier comment overstated the risk):
paytoonly creates and signs the transaction; nothing is burned untilbroadcast, which goes through the Electrum server.- Whether it gets rejected there depends on the server: ElectrumX and electrs call
sendrawtransactionwith only the raw tx, so bitcoind's defaultmaxburnamount=0applies, but Fulcrum passesmaxburnamount=21000000(Servers.cpp#L2037), so there it goes through. - As @f321x said, an
OP_RETURN-only gate is easy to get around:script(OP_0)with a value is accepted too, and it is just as unspendable. Gating allscript(...)outputs would also hit zero-valueOP_RETURNoutputs.
So for this PR I'd keep payto like the GUI (pushed that way). If we want a guard, I think it belongs at broadcast time, in Electrum itself and for both the CLI and the GUI, like bitcoind's maxburnamount, so that it doesn't depend on the server. I can do that in a separate PR if you agree.
As suggested in review: use a shared parse_script() in payment_identifier.py for both the GUI and the CLI. It strips surrounding whitespace, like the GUI, and rejects an empty script(), which used to create an output with an empty scriptPubKey. The OP_RETURN amount check is dropped: the Qt GUI allows sending value to an OP_RETURN output, and bitcoind's sendrawtransaction rejects it by default (maxburnamount=0). Co-authored-by: f321x <f@f321x.com>
3 participants
Fixes #4547. Picks up #7203, which was closed as stale.
PaymentIdentifier, which the GUI uses to parse payees, already accepts arbitrary output scripts written asscript(...), e.g.script(OP_RETURN 48656c6c6f). On the command line,payto/paytomanyonly resolved addresses, contacts and aliases. Now they accept the same syntax: thescript(...)parsing moved into a sharedparse_script()inpayment_identifier.py(written by @f321x in review), used by both the GUI and the CLI. It strips surrounding whitespace and rejects an emptyscript().Following the review on #7203: only script parsing is exposed (no openalias), and the existing
payto-to-address test still passes. Tests:test_payto_script_outputintests/test_commands.py(OP_RETURN viapayto, mixed with an address viapaytomany, surrounding whitespace, and malformed or empty scripts, which raise aUserFacingException), andtest_parse_scriptintests/test_payment_identifier.py.Also checked end to end on regtest with Bitcoin Core v31.1.0, with the first version of this PR: the transaction was accepted to the mempool and mined, and its
nulldataoutput decodes toHello. The sharedparse_script()produces the same output script for this example (6a0548656c6c6f).