Skip to content

Commit

Permalink
signpsbt: Require input selection by default
Browse files Browse the repository at this point in the history
With the advent of dual-funding and splicing, the risk of users unwittingly signing away funds because their peers or other PSBT services slip malicious inputs in increases.

`fundpsbt` now returns a `utxo_string` identifying the specific inputs it added and `signpsbt` now requires `utxo_string` (or the existing `sigonly` array) by default.

The old behavior is moved to a flag called `unsafe_sign_all`.

Changelog-Chaged: `signpsbt` now requires specifying which inputs to be signed. `fundpsbt` is also updated to provide a digest of inputs in `utxo_string` — a compatible format across both commands.
  • Loading branch information
ddustin committed Nov 10, 2023
1 parent 02ca226 commit 7ffd1a6
Show file tree
Hide file tree
Showing 14 changed files with 139 additions and 39 deletions.
14 changes: 12 additions & 2 deletions .msggen.json
Original file line number Diff line number Diff line change
Expand Up @@ -745,7 +745,8 @@
"FundPsbt.excess_msat": 4,
"FundPsbt.feerate_per_kw": 2,
"FundPsbt.psbt": 1,
"FundPsbt.reservations[]": 6
"FundPsbt.reservations[]": 6,
"FundPsbt.utxo_string": 7
},
"GetinfoAddress": {
"Getinfo.address[].address": 3,
Expand Down Expand Up @@ -1541,7 +1542,8 @@
},
"SignpsbtRequest": {
"SignPsbt.psbt": 1,
"SignPsbt.signonly[]": 2
"SignPsbt.signonly[]": 2,
"SignPsbt.utxo_string": 3
},
"SignpsbtResponse": {
"SignPsbt.signed_psbt": 1
Expand Down Expand Up @@ -3009,6 +3011,10 @@
"added": "pre-v0.10.1",
"deprecated": false
},
"FundPsbt.utxo_string": {
"added": "v23.11",
"deprecated": false
},
"GetRoute": {
"added": "pre-v0.10.1",
"deprecated": null
Expand Down Expand Up @@ -5453,6 +5459,10 @@
"added": "pre-v0.10.1",
"deprecated": false
},
"SignPsbt.utxo_string": {
"added": "v23.11",
"deprecated": false
},
"StaticBackup": {
"added": "pre-v0.10.1",
"deprecated": null
Expand Down
4 changes: 3 additions & 1 deletion contrib/pyln-client/pyln/client/lightning.py
Original file line number Diff line number Diff line change
Expand Up @@ -1570,13 +1570,15 @@ def utxopsbt(self, satoshi, feerate, startweight, utxos, reserve=None, reservedo
}
return self.call("utxopsbt", payload)

def signpsbt(self, psbt, signonly=None):
def signpsbt(self, psbt, signonly=None, utxo_string=None, unsafe_sign_all=None):
"""
Add internal wallet's signatures to PSBT
"""
payload = {
"psbt": psbt,
"signonly": signonly,
"utxo_string": utxo_string,
"unsafe_sign_all": unsafe_sign_all,
}
return self.call("signpsbt", payload)

Expand Down
3 changes: 2 additions & 1 deletion doc/lightning-fundpsbt.7.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ On success, an object is returned, containing:
- **was\_reserved** (boolean): Whether this output was previously reserved (always *false*)
- **reserved** (boolean): Whether this output is now reserved (always *true*)
- **reserved\_to\_block** (u32): The blockheight the reservation will expire
- **utxo\_string** (string, optional): A command seperated list of utxos used to fund the psbt *(added v24.02)*

[comment]: # (GENERATE-FROM-SCHEMA-END)

Expand Down Expand Up @@ -112,4 +113,4 @@ RESOURCES

Main web site: <https://github.com/ElementsProject/lightning>

[comment]: # ( SHA256STAMP:13e35920ba8810db082e3cca62d1141a67498a2756da2479a24eaa62567ff4fe)
[comment]: # ( SHA256STAMP:8c942b9a2303a05e3cda8e3190b4c299ede6020849573ef4c4ca7ee4c4363fae)
18 changes: 13 additions & 5 deletions doc/lightning-signpsbt.7.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,18 @@ DESCRIPTION
BIP-174.

- *psbt*: A string that represents the PSBT value.
- *signonly*: An optional array of input numbers to sign.
- *signonly*: An an array of input numbers to sign.
- *utxo\_string*: An an array of outputs to sign.
- *unsafe\_sign\_all*: A flag to recklessly sign all reserved inputs.

By default, all known inputs are signed, and others ignored: with
*signonly*, only those inputs are signed, and an error is returned if
one of them cannot be signed.
Specify which inputs to sign by either an array of input indices to sign with
*sigonly* or a *utxo\_string*.

*utxo\_string* is in the format of a txid and outnum seperated by a colon.
Multiple utxos can be added by putting a comma inbetween entries. As such:
txid:outnum,txid:outnum... This is the same format returned by fundpsbt.

An error is returned if one of them cannot be signed.

Note that the command will fail if there are no inputs to sign, or
if the inputs to be signed were not previously reserved.
Expand All @@ -30,7 +37,8 @@ EXAMPLE JSON REQUEST
"id": 82,
"method": "signpsbt",
"params": {
"psbt": "some_psbt"
"psbt": "some_psbt",
"utxo_string": "txid:outnum,txid:outnum"
}
}
```
Expand Down
3 changes: 2 additions & 1 deletion doc/lightning-splice_init.7.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ echo $RESULT;

RESULT=$(lightning-cli fundpsbt -k satoshi=100000sat feerate=urgent startweight=800 excess_as_change=true);
INITIALPSBT=$(echo $RESULT | jq -r ".psbt");
UTXO_STRING=$(echo $RESULT | jq -r ".utxo_string");
echo $RESULT;

RESULT=$(lightning-cli splice_init $CHANNEL_ID 100000 $INITIALPSBT);
Expand All @@ -56,7 +57,7 @@ RESULT=$(lightning-cli splice_update $CHANNEL_ID $PSBT);
PSBT=$(echo $RESULT | jq -r ".psbt");
echo $RESULT;

RESULT=$(lightning-cli signpsbt -k psbt="$PSBT");
RESULT=$(lightning-cli signpsbt -k psbt="$PSBT" utxo_string="$UTXO_STRING");
PSBT=$(echo $RESULT | jq -r ".signed_psbt");
echo $RESULT;

Expand Down
5 changes: 5 additions & 0 deletions doc/schemas/fundpsbt.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@
}
}
}
},
"utxo_string": {
"type": "string",
"description": "A command seperated list of utxos used to fund the psbt",
"added": "v23.11"
}
}
}
8 changes: 8 additions & 0 deletions doc/schemas/signpsbt.request.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@
"items": {
"type": "u32"
}
},
"utxo_string": {
"type": "string",
"added": "v23.11"
},
"unsafe_sign_all": {
"type": "bool",
"added": "v23.11"
}
}
}
2 changes: 1 addition & 1 deletion external/lnprototest
12 changes: 8 additions & 4 deletions tests/test_closing.py
Original file line number Diff line number Diff line change
Expand Up @@ -3717,13 +3717,15 @@ def test_closing_anchorspend_htlc_tx_rbf(node_factory, bitcoind):

# We reduce l1's UTXOs so it's forced to use more than one UTXO to push.
fundsats = int(only_one(l1.rpc.listfunds()['outputs'])['amount_msat'].to_satoshi())
psbt = l1.rpc.fundpsbt("all", "1000perkw", 1000)['psbt']
result = l1.rpc.fundpsbt("all", "1000perkw", 1000)
psbt = result['psbt']
utxo_string = result['utxo_string']
# Pay 5k sats in fees, send most to l2
psbt = l1.rpc.addpsbtoutput(fundsats - 20000 - 5000, psbt, destination=l2.rpc.newaddr()['bech32'])['psbt']
# 10x2000 sat outputs for l1 to use.
for i in range(10):
psbt = l1.rpc.addpsbtoutput(2000, psbt)['psbt']
l1.rpc.sendpsbt(l1.rpc.signpsbt(psbt)['signed_psbt'])
l1.rpc.sendpsbt(l1.rpc.signpsbt(psbt, utxo_string=utxo_string)['signed_psbt'])
bitcoind.generate_block(1, wait_for_mempool=1)
sync_blockheight(bitcoind, [l1])

Expand Down Expand Up @@ -3912,12 +3914,14 @@ def test_peer_anchor_push(node_factory, bitcoind, executor, chainparams):
fundsats = int(only_one(l2.rpc.listfunds()['outputs'])['amount_msat'].to_satoshi())
OUTPUT_SAT = 10000
NUM_OUTPUTS = 10
psbt = l2.rpc.fundpsbt("all", "1000perkw", 1000)['psbt']
result = l2.rpc.fundpsbt("all", "1000perkw", 1000)
psbt = result['psbt']
utxo_string = result['utxo_string']
# Pay 5k sats in fees.
psbt = l2.rpc.addpsbtoutput(fundsats - OUTPUT_SAT * NUM_OUTPUTS - 5000, psbt, destination=l3.rpc.newaddr()['bech32'])['psbt']
for _ in range(NUM_OUTPUTS):
psbt = l2.rpc.addpsbtoutput(OUTPUT_SAT, psbt)['psbt']
l2.rpc.sendpsbt(l2.rpc.signpsbt(psbt)['signed_psbt'])
l2.rpc.sendpsbt(l2.rpc.signpsbt(psbt, utxo_string=utxo_string)['signed_psbt'])
bitcoind.generate_block(1, wait_for_mempool=1)
sync_blockheight(bitcoind, [l2])

Expand Down
22 changes: 11 additions & 11 deletions tests/test_opening.py
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ def test_v2_rbf_single(node_factory, bitcoind, chainparams):
assert int(info_2['next_feerate'][:-5]) == rate * 65 // 64

# Sign our inputs, and continue
signed_psbt = l1.rpc.signpsbt(update['psbt'])['signed_psbt']
signed_psbt = l1.rpc.signpsbt(update['psbt'], unsafe_sign_all=True)['signed_psbt']

# Fails because we didn't put enough feerate in.
with pytest.raises(RpcError, match=r'insufficient fee'):
Expand All @@ -412,7 +412,7 @@ def test_v2_rbf_single(node_factory, bitcoind, chainparams):
funding_feerate=next_rate)
update = l1.rpc.openchannel_update(chan_id, bump['psbt'])
assert update['commitments_secured']
signed_psbt = l1.rpc.signpsbt(update['psbt'])['signed_psbt']
signed_psbt = l1.rpc.signpsbt(update['psbt'], unsafe_sign_all=True)['signed_psbt']
l1.rpc.openchannel_signed(chan_id, signed_psbt)

bitcoind.generate_block(1)
Expand Down Expand Up @@ -505,7 +505,7 @@ def test_v2_rbf_abort_retry(node_factory, bitcoind, chainparams):
funding_feerate=next_rate)

update = l1.rpc.openchannel_update(chan_id, bump['psbt'])
signed_psbt = l1.rpc.signpsbt(update['psbt'])['signed_psbt']
signed_psbt = l1.rpc.signpsbt(update['psbt'], unsafe_sign_all=True)['signed_psbt']
l1.rpc.openchannel_signed(chan_id, signed_psbt)

bitcoind.generate_block(1)
Expand Down Expand Up @@ -638,7 +638,7 @@ def test_v2_rbf_liquidity_ad(node_factory, bitcoind, chainparams):
assert update['commitments_secured']

# Sign our inputs, and continue
signed_psbt = l1.rpc.signpsbt(update['psbt'])['signed_psbt']
signed_psbt = l1.rpc.signpsbt(update['psbt'], unsafe_sign_all=True)['signed_psbt']
l1.rpc.openchannel_signed(chan_id, signed_psbt)

# There's data in the datastore now (l2 only)
Expand Down Expand Up @@ -731,7 +731,7 @@ def test_v2_rbf_multi(node_factory, bitcoind, chainparams):
assert update['commitments_secured']

# Sign our inputs, and continue
signed_psbt = l1.rpc.signpsbt(update['psbt'])['signed_psbt']
signed_psbt = l1.rpc.signpsbt(update['psbt'], unsafe_sign_all=True)['signed_psbt']
l1.rpc.openchannel_signed(chan_id, signed_psbt)

# We 2x the feerate to beat the min-relay fee
Expand All @@ -754,7 +754,7 @@ def test_v2_rbf_multi(node_factory, bitcoind, chainparams):
assert update['commitments_secured']

# Sign our inputs, and continue
signed_psbt = l1.rpc.signpsbt(update['psbt'])['signed_psbt']
signed_psbt = l1.rpc.signpsbt(update['psbt'], unsafe_sign_all=True)['signed_psbt']
l1.rpc.openchannel_signed(chan_id, signed_psbt)

bitcoind.generate_block(1)
Expand Down Expand Up @@ -969,7 +969,7 @@ def test_rbf_reconnect_tx_construct(node_factory, bitcoind, chainparams):
# We can call update again! It should short-circuit this time :)
update = l1.rpc.openchannel_update(chan_id, bump['psbt'])
assert update['commitments_secured']
signed_psbt = l1.rpc.signpsbt(update['psbt'])['signed_psbt']
signed_psbt = l1.rpc.signpsbt(update['psbt'], unsafe_sign_all=True)['signed_psbt']
l1.rpc.openchannel_signed(chan_id, signed_psbt)

l2.daemon.wait_for_log('Broadcasting funding tx')
Expand Down Expand Up @@ -1038,7 +1038,7 @@ def test_rbf_reconnect_tx_sigs(node_factory, bitcoind, chainparams):
update = l1.rpc.openchannel_update(chan_id, bump['psbt'])

# Sign our inputs, and continue
signed_psbt = l1.rpc.signpsbt(update['psbt'])['signed_psbt']
signed_psbt = l1.rpc.signpsbt(update['psbt'], unsafe_sign_all=True)['signed_psbt']

# First time we error when we send our sigs
with pytest.raises(RpcError):
Expand Down Expand Up @@ -1210,7 +1210,7 @@ def run_retry():
update = l1.rpc.openchannel_update(chan_id, bump['psbt'])
assert update['commitments_secured']

return l1.rpc.signpsbt(update['psbt'])['signed_psbt']
return l1.rpc.signpsbt(update['psbt'], unsafe_sign_all=True)['signed_psbt']

signed_psbt = run_retry()
l1.rpc.openchannel_signed(chan_id, signed_psbt)
Expand Down Expand Up @@ -1295,7 +1295,7 @@ def run_retry():
update = l1.rpc.openchannel_update(chan_id, bump['psbt'])
assert update['commitments_secured']

return l1.rpc.signpsbt(update['psbt'])['signed_psbt']
return l1.rpc.signpsbt(update['psbt'], unsafe_sign_all=True)['signed_psbt']

signed_psbt = run_retry()
l1.rpc.openchannel_signed(chan_id, signed_psbt)
Expand Down Expand Up @@ -1363,7 +1363,7 @@ def run_retry():
update = l1.rpc.openchannel_update(chan_id, bump['psbt'])
assert update['commitments_secured']

return l1.rpc.signpsbt(update['psbt'])['signed_psbt']
return l1.rpc.signpsbt(update['psbt'], unsafe_sign_all=True)['signed_psbt']

# Make a second inflight
signed_psbt = run_retry()
Expand Down
10 changes: 5 additions & 5 deletions tests/test_splicing.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def test_splice(node_factory, bitcoind):

result = l1.rpc.splice_init(chan_id, 100000, funds_result['psbt'])
result = l1.rpc.splice_update(chan_id, result['psbt'])
result = l1.rpc.signpsbt(result['psbt'])
result = l1.rpc.signpsbt(result['psbt'], utxo_string=funds_result['utxo_string'])
result = l1.rpc.splice_signed(chan_id, result['signed_psbt'])

l2.daemon.wait_for_log(r'CHANNELD_NORMAL to CHANNELD_AWAITING_SPLICE')
Expand Down Expand Up @@ -61,7 +61,7 @@ def test_splice_gossip(node_factory, bitcoind):

result = l1.rpc.splice_init(chan_id, 100000, funds_result['psbt'])
result = l1.rpc.splice_update(chan_id, result['psbt'])
result = l1.rpc.signpsbt(result['psbt'])
result = l1.rpc.signpsbt(result['psbt'], utxo_string=funds_result['utxo_string'])
result = l1.rpc.splice_signed(chan_id, result['signed_psbt'])

wait_for(lambda: only_one(l2.rpc.listpeerchannels(l1.info['id'])['channels'])['state'] == 'CHANNELD_AWAITING_SPLICE')
Expand Down Expand Up @@ -119,7 +119,7 @@ def test_splice_listnodes(node_factory, bitcoind):

result = l1.rpc.splice_init(chan_id, 100000, funds_result['psbt'])
result = l1.rpc.splice_update(chan_id, result['psbt'])
result = l1.rpc.signpsbt(result['psbt'])
result = l1.rpc.signpsbt(result['psbt'], utxo_string=funds_result['utxo_string'])
result = l1.rpc.splice_signed(chan_id, result['signed_psbt'])

l2.daemon.wait_for_log(r'CHANNELD_NORMAL to CHANNELD_AWAITING_SPLICE')
Expand Down Expand Up @@ -208,7 +208,7 @@ def test_invalid_splice(node_factory, bitcoind):

result = l1.rpc.splice_init(chan_id, 100000, funds_result['psbt'])
result = l1.rpc.splice_update(chan_id, result['psbt'])
result = l1.rpc.signpsbt(result['psbt'])
result = l1.rpc.signpsbt(result['psbt'], utxo_string=funds_result['utxo_string'])
result = l1.rpc.splice_signed(chan_id, result['signed_psbt'])

l2.daemon.wait_for_log(r'CHANNELD_NORMAL to CHANNELD_AWAITING_SPLICE')
Expand Down Expand Up @@ -307,7 +307,7 @@ def test_splice_stuck_htlc(node_factory, bitcoind, executor):

result = l1.rpc.splice_init(chan_id, 100000, funds_result['psbt'])
result = l1.rpc.splice_update(chan_id, result['psbt'])
result = l1.rpc.signpsbt(result['psbt'])
result = l1.rpc.signpsbt(result['psbt'], utxo_string=funds_result['utxo_string'])
result = l1.rpc.splice_signed(chan_id, result['signed_psbt'])

l2.daemon.wait_for_log(r'CHANNELD_NORMAL to CHANNELD_AWAITING_SPLICE')
Expand Down
14 changes: 7 additions & 7 deletions tests/test_wallet.py
Original file line number Diff line number Diff line change
Expand Up @@ -807,7 +807,7 @@ def test_sign_external_psbt(node_factory, bitcoind, chainparams):
psbt = bitcoind.rpc.createpsbt(inputs, [{addr: (amount * 3) / 10**8}])

l1.rpc.reserveinputs(psbt)
l1.rpc.signpsbt(psbt)
l1.rpc.signpsbt(psbt, unsafe_sign_all=True)


def test_psbt_version(node_factory, bitcoind, chainparams):
Expand Down Expand Up @@ -913,7 +913,7 @@ def test_sign_and_send_psbt(node_factory, bitcoind, chainparams):

# We require the utxos be reserved before signing them
with pytest.raises(RpcError, match=r"Aborting PSBT signing. UTXO .* is not reserved"):
l1.rpc.signpsbt(funding['psbt'])['signed_psbt']
l1.rpc.signpsbt(funding['psbt'], utxo_string=funding['utxo_string'])['signed_psbt']

# Now we unreserve the singleton, so we can reserve it again
l1.rpc.unreserveinputs(psbt)
Expand All @@ -930,7 +930,7 @@ def test_sign_and_send_psbt(node_factory, bitcoind, chainparams):
l1.rpc.reserveinputs(fullpsbt)

# Sign + send the PSBT we've created
signed_psbt = l1.rpc.signpsbt(fullpsbt)['signed_psbt']
signed_psbt = l1.rpc.signpsbt(fullpsbt, utxo_string=funding['utxo_string'])['signed_psbt']
broadcast_tx = l1.rpc.sendpsbt(signed_psbt)

# Check that it was broadcast successfully
Expand All @@ -947,7 +947,7 @@ def test_sign_and_send_psbt(node_factory, bitcoind, chainparams):

# Now we try signing a PSBT with an output that's already been spent
with pytest.raises(RpcError, match=r"Aborting PSBT signing. UTXO .* is not reserved"):
l1.rpc.signpsbt(fullpsbt)
l1.rpc.signpsbt(fullpsbt, utxo_string=funding['utxo_string'])

# Queue up another node, to make some PSBTs for us
for i in range(total_outs):
Expand All @@ -962,7 +962,7 @@ def test_sign_and_send_psbt(node_factory, bitcoind, chainparams):

# Try to get L1 to sign it
with pytest.raises(RpcError, match=r"No wallet inputs to sign"):
l1.rpc.signpsbt(l2_funding['psbt'])
l1.rpc.signpsbt(l2_funding['psbt'], utxo_string=funding['utxo_string'])

# With signonly it will fail if it can't sign it.
with pytest.raises(RpcError, match=r"is unknown"):
Expand Down Expand Up @@ -1007,7 +1007,7 @@ def test_sign_and_send_psbt(node_factory, bitcoind, chainparams):
for s in sign_success:
assert bitcoind.rpc.decodepsbt(half_signed_psbt)['inputs'][s]['partial_signatures'] is not None

totally_signed = l2.rpc.signpsbt(half_signed_psbt)['signed_psbt']
totally_signed = l2.rpc.signpsbt(half_signed_psbt, unsafe_sign_all=True)['signed_psbt']

broadcast_tx = l1.rpc.sendpsbt(totally_signed)
l1.daemon.wait_for_log(r'sendrawtx exit 0 .* sendrawtransaction {}'.format(broadcast_tx['tx']))
Expand All @@ -1020,7 +1020,7 @@ def test_sign_and_send_psbt(node_factory, bitcoind, chainparams):
output_psbt = bitcoind.rpc.createpsbt([],
[{addr: float((out_total + out_amt).to_btc())}])
psbt = bitcoind.rpc.joinpsbts([l2_funding['psbt'], output_psbt])
l2_signed_psbt = l2.rpc.signpsbt(psbt)['signed_psbt']
l2_signed_psbt = l2.rpc.signpsbt(psbt, unsafe_sign_all=True)['signed_psbt']
l1.rpc.sendpsbt(l2_signed_psbt)

# Re-try sending the same tx?
Expand Down
Loading

0 comments on commit 7ffd1a6

Please sign in to comment.