wallet: skip the PEGIN tx feature on elements

[vincenzopalazzo]

This patch is proposing a temporary patch for elements, some of our users try to use elements and get the stack trace on the bottom.

The crash happens because the psbt_input_from_tx_input_pegin is called and libwally do not support the PEGIN tx feature. This patch removes the PEGIN feature because cln should not care about this kind of transaction. I am not sure about this change so this requires more eyes.

Fixes #6955

lightningd: bitcoin/psbt.c:65: struct wally_psbt *new_psbt(const tal_t *, const struct wally_tx *): Assertion `wally_err == WALLY_OK' failed. lightningd: FATAL SIGNAL 6 (version v23.11-84-ga59dbbd-modded) 0x19f6a0 send_backtrace
	/home/vincent/github/work/lightning/common/daemon.c:33
0x19fb29 crashdump
	/home/vincent/github/work/lightning/common/daemon.c:75
0x4ba170f ???
	???:0
0x4bf183c ???
	???:0
0x4ba1667 ???
	???:0
0x4b894b7 ???
	???:0
0x4b893db ???
	???:0
0x4b99d25 ???
	???:0
0x1bd052 new_psbt
	/home/vincent/github/work/lightning/bitcoin/psbt.c:65
0x1c6a45 pull_bitcoin_tx
	/home/vincent/github/work/lightning/bitcoin/tx.c:665
0x1bbcd0 bitcoin_block_from_hex
	/home/vincent/github/work/lightning/bitcoin/block.c:213
0x123f90 getrawblockbyheight_callback
	/home/vincent/github/work/lightning/lightningd/bitcoind.c:486
0x16ece7 plugin_response_handle

[cdecker]

I'd be really surprised if this were to work: we are patching the features on a transaction from a block, masking away some features, which then somehow change the way we parse, but we still parse successfully?

Maybe @jgriffiths can help us out with this one?

[jgriffiths]

I'd need a script reproducing this to implement the required wally functionality I think, I'm not particularly familiar with pegins.

edit: I'm also wondering why you need to make psbts out of every tx in the block; that surely can't be right. I suspect you just want the wtxs? You couldn't do anything with a psbt from a fully signed block tx except extract the tx from it that you started with.

In the short term perhaps you could add a flag to pull_bitcoin_tx to avoid making the psbt, and see if there is any fallout for this use case.

[grubles]

FWIW the patch appears to get CLN past the point in which it would crash previously. Uptime is probably around 48 hours with no crash.

$ lightning-cli --network=liquid getinfo
{
   "id": "0374de216903bc43c7befe276a1a8856535f4fae3390188fee0d40faaea94c7e94",
   "alias": "STRANGEMONKEY",
   "color": "0374de",
   "num_peers": 0,
   "num_pending_channels": 0,
   "num_active_channels": 0,
   "num_inactive_channels": 0,
   "address": [
      {
         "type": "ipv4",
         "address": "",
         "port": 9737
      }
   ],
   "binding": [
      {
         "type": "ipv4",
         "address": "",
         "port": 9737
      }
   ],
   "version": "v23.11-85-gd7b3df6",
   "blockheight": 2673647,
   "network": "liquid",
   "fees_collected_msat": 0,
   "lightning-dir": "/home/user/.lightning/liquid",
   "our_features": {
      "init": "08a0000a0a69a2",
      "node": "88a0000a0a69a2",
      "channel": "",
      "invoice": "02000002024100"
   }
}

[jgriffiths]

FWIW the patch appears to get CLN past the point in which it would crash previously. Uptime is probably around 48 hours with no crash.

The patch works by ensuring that any pegin information is ignored and as a result that all the psbts created from pegins are silently created in an invalid state. As above, this code shouldn't need to create psbts at all, which is both more correct and significantly more efficient

I think the psbt creation inside pull_bitcoin_tx is part of the gradual shift from using txs to using psbt internally in cln. Whatever this code is iterating blocks for, there should be no need to create psbts or reference them. The correct fix is to avoid creating psbts as above (or add pegin support for psbts in wally, which will take considerably longer).

[vincenzopalazzo]

I'd be really surprised if this were to work: we are patching the features on a transaction from a block, masking away some features, which then somehow change the way we parse, but we still parse successfully?

Hmm, I'm not sure! It seems that libwally has never supported Peg-in, so I was working under the following assumption, because I do not know how Peg-in works on Liquid 😸

This patch removes the PEGIN feature because cln should not care about this kind of transaction.

---

I think the psbt creation inside pull_bitcoin_tx is part of the gradual shift from using txs to using psbt internally in cln. Whatever this code is iterating blocks for, there should be no need to create psbts or reference them. The correct fix is to avoid creating psbts as above (or add pegin support for psbts in wally, which will take considerably longer).

Yes, I believe this is the method that Core Lightning uses to locate the transaction within the block. If the transactions are found, then the PSB is being passed somewhere. I don't remember the code anymore, but I think this is the only situation where using the PSB makes sense. 😄

Also, @lvaccaro pointed out to me that parsing the PSBT while scanning the block might be something I should consider, without resorting to hacky code to solve crashes

---

Thanks for testing @grubles

I was able also to sync liquid without any problem! but I am not happy with this solution is hacky

[jgriffiths]

If the transactions are found, then the PSB is being passed somewhere.

Right. I don't see that any relevant-to-cln txs will ever have pegin information present. So the correct fix here is:

• Add a flag to pull_bitcoin_tx to skip psbt creation (alternately, add a pull_bitcoin_tx_only function which doesn't call new_psbt)
• Skip the psbt creation when iterating the block txs in bitcoin_block_from_hex
• When the interesting/relevant tx(s) have been located, set the psbt member using tx->psbt = new_psbt(tx, tx->wtx); before passing the tx on to its consumer

[jgriffiths]

Please see #6983 which attempts my suggested fix above.