What the developer wrote
Authored by SomberNight
transaction: add method tx.is_all_segwit()
What changed, and why it matters
This commit adds a helper method to check whether every input of a Bitcoin transaction uses SegWit, and starts using it in place of a weaker 'any input is SegWit' check when validating Lightning funding transactions. SegWit fixes 'transaction malleability,' where the transaction ID (txid) can be changed by someone else after signing. For Lightning, the funding txid must be stable because both parties rely on it to set up the channel. The old check only required at least one SegWit input, which could still allow non-SegWit inputs to be re-signed or malleated, changing the txid. The commit also adds a TODO noting that a zero-confirmation code path still doesn't perform this check at all, which the authors already flag as unsafe.
Review whether the zero-confirmation channel_establishment_flow path in lnpeer.py should enforce is_all_segwit() before mainnet use, as the commit's own TODO states the current behavior is unsafe. Otherwise, this commit appears to be a defensive hardening fix and should be included in the next release.
Security signals we found
Replaced weaker malleability check with stronger all-inputs SegWit check in Lightning funding transaction validation
Code comment explicitly tied the change to transaction malleability and funding txid stability
Prior code contained a '# FIXME needs is_all_segwit' indicating the old check was known to be insufficient
New helper includes references to BIP-62 and Bitcoin Core policy explaining malleability risks
A TODO marks the zero-confirmation channel path as still missing the check and explicitly calls it unsafe
Evidence from the diff
The patch introduces transaction.Transaction.is_all_segwit(), which returns True only if every txin.is_segwit() is True. It documents why partial SegWit coverage is insufficient: non-SegWit inputs remain trivially malleable by signers (different nonces) and by miners (third-party malleability is mostly non-standard, but a miner can mutate). lnpeer.py’s channel_establishment_flow now rejects funding transactions unless is_all_segwit() is true, replacing the prior is_any_segwit() check that the code itself had marked with ‘# FIXME needs is_all_segwit’. The timelock_recovery plugin replaces per-input all(…) assertions with the same helper. transaction.py’s txid() cache path also refactors its local all_segwit check to use the new method. A TODO is added in on_open_channel for the zeroconf path, which still does not validate the funding transaction’s SegWit status.
Changed components
electrum/transaction.pyelectrum/lnpeer.pyelectrum/plugins/timelock_recovery/qt.pyInspect captured patch +20 / −6
### electrum/lnpeer.py
@@ -1181,7 +1181,7 @@ async def channel_establishment_flow(
raise Exception('op_return output not found in funding tx')
# must not be malleable
funding_tx.set_rbf(False)
- if not funding_tx.is_any_segwit(): # FIXME needs "is_all_segwit"
+ if not funding_tx.is_all_segwit():
raise Exception('Funding transaction is not segwit')
funding_txid = funding_tx.txid()
assert funding_txid
@@ -1481,6 +1481,7 @@ async def on_open_channel(self, payload):
if is_zeroconf:
# FIXME shouldn't we wait until funding_tx is at least in the mempool?!
# We haven't even validated funding_tx really contains the multisig funding output!
+ # (TODO also check funding_tx.is_all_segwit())
# This is unsafe. MUST be reworked before mainnet usage.
chan.set_state(ChannelState.FUNDED)
self.send_channel_ready(chan)
### electrum/plugins/timelock_recovery/qt.py
@@ -237,22 +237,22 @@ def update_transactions():
if not context.alert_tx or context.alert_tx.txid() != new_alert_tx.txid():
context.alert_tx = new_alert_tx
alert_changed = True
- assert all(tx_input.is_segwit() for tx_input in context.alert_tx.inputs())
+ assert context.alert_tx.is_all_segwit()
alert_tx_complete_label.setText(_("✓ Signed") if context.alert_tx.is_complete() else "")
alert_tx_fee_label.setText(_("Fee: {}").format(self.config.format_amount_and_units(context.alert_tx.get_fee())))
new_recovery_tx = context.make_unsigned_recovery_tx(fee_policy)
if alert_changed or not context.recovery_tx or context.recovery_tx.txid() != new_recovery_tx.txid():
context.recovery_tx = new_recovery_tx
context.add_input_info_to_recovery_tx()
- assert all(tx_input.is_segwit() for tx_input in context.recovery_tx.inputs())
+ assert context.recovery_tx.is_all_segwit()
recovery_tx_complete_label.setText(_("✓ Signed") if context.recovery_tx.is_complete() else "")
recovery_tx_fee_label.setText(_("Fee: {}").format(self.config.format_amount_and_units(context.recovery_tx.get_fee())))
if create_cancel_cb.isChecked():
new_cancellation_tx = context.make_unsigned_cancellation_tx(fee_policy)
if alert_changed or not context.cancellation_tx or context.cancellation_tx.txid() != new_cancellation_tx.txid():
context.cancellation_tx = new_cancellation_tx
context.add_input_info_to_cancellation_tx()
- assert all(tx_input.is_segwit() for tx_input in context.cancellation_tx.inputs())
+ assert context.cancellation_tx.is_all_segwit()
cancellation_tx_complete_label.setText(_("✓ Signed") if context.cancellation_tx.is_complete() else "")
cancellation_tx_fee_label.setText(_("Fee: {}").format(self.config.format_amount_and_units(context.cancellation_tx.get_fee())))
else:
### electrum/transaction.py
@@ -1174,6 +1174,20 @@ def is_any_segwit(self, *, guess_for_address: bool = False) -> bool:
return any(txin.is_segwit(guess_for_address=guess_for_address)
for txin in self.inputs())
+ def is_all_segwit(self, *, guess_for_address: bool = False) -> bool:
+ """Returns whether *all* inputs are segwit.
+
+ If not, the txid is trivially malleable:
+ - by any signer, who can e.g. re-sign the non-segwit inputs using different nonces
+ - by miners: most third-party malleability results in the tx being non-standard,
+ so at least arbitrary tx relaying nodes cannot do it. But if they mine the tx, they can.
+
+ ref https://github.com/bitcoin/bips/blob/master/bip-0062.mediawiki#motivation
+ ref https://github.com/bitcoin/bitcoin/blob/05bc2f53ce0cb239c17dbdd6b261bd2db7d2a940/src/policy/policy.h#L118-L131
+ """
+ return all(txin.is_segwit(guess_for_address=guess_for_address)
+ for txin in self.inputs())
+
def invalidate_ser_cache(self):
self._cached_network_ser = None
self._cached_txid = None
@@ -1237,8 +1251,7 @@ def to_qr_data(self) -> Tuple[str, bool]:
def txid(self) -> Optional[str]:
if self._cached_txid is None:
self.deserialize()
- all_segwit = all(txin.is_segwit() for txin in self.inputs())
- if not all_segwit and not self.is_complete():
+ if not self.is_all_segwit() and not self.is_complete():
return None
try:
ser = self.serialize_to_network(force_legacy=True)Why this scored 44/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.