tapfreighter/[bug]: universe proof transfer not resumed after restart

[Roasbeef]

Background

After we confirm a transfer, and update proofs (only for the file based store), we'll attempt to upload any proofs (if needed) to a universe:

taproot-assets/tapfreighter/chain_porter.go

Lines 1165 to 1190 in fca9d11

	// At this point, the transfer transaction is confirmed on-chain, and
	// we've stored the sender and receiver proofs in the proof archive.
	// We'll now attempt to transfer the receiver proof to the receiver.
	case SendStateReceiverProofTransfer:
	// We'll set the package state to complete early here so the
	// main loop breaks out. We'll continue to attempt proof
	// deliver in the background.
	currentPkg.SendState = SendStateComplete
	p.Wg.Add(1)
	go func() {
	defer p.Wg.Done()
	err := p.transferReceiverProof(&currentPkg)
	if err != nil {
	log.Errorf("unable to transfer receiver "+
	"proof: %v", err)
	p.publishSubscriberEvent(newAssetSendErrorEvent(
	err, SendStateReceiverProofTransfer,
	currentPkg,
	))
	}
	}()
	return &currentPkg, nil

The issue here is that this is run in a goroutine, with the state step function returning immediately. Upon restart, we won't attempt to publish the proofs again. Even if we don't have any proofs, this is important, as only once we transfer all the proofs, do we call ConfirmParcelDelivery: https://github.com/lightninglabs/taproot-assets/blob/main/tapfreighter/chain_porter.go#L795-L808.

The function name is slightly overloaded, but ConfirmParcelDelivery is what will actually finalize the transfer by applying the pending outputs (make new assets w/ the relevant details):

taproot-assets/tapdb/assets_store.go

Lines 2816 to 2842 in fca9d11

	// Since we define that a transfer can only move assets
	// within the same asset ID, we can take any of the
	// inputs as a template for the new asset, since the
	// genesis and group key will be the same. We'll
	// overwrite all other fields.
	templateID := spentAssetIDs[0]
	params := ApplyPendingOutput{
	ScriptKeyID: out.ScriptKeyID,
	AnchorUtxoID: sqlInt64(
	out.AnchorUtxoID,
	),
	Amount: out.Amount,
	LockTime: out.LockTime,
	RelativeLockTime: out.RelativeLockTime,
	//nolint:lll
	SplitCommitmentRootHash: out.SplitCommitmentRootHash,
	SplitCommitmentRootValue: out.SplitCommitmentRootValue,
	SpentAssetID: templateID,
	Spent: isTombstone || isBurn,
	AssetVersion: out.AssetVersion,
	}
	newAssetID, err := q.ApplyPendingOutput(ctx, params)
	if err != nil {
	return fmt.Errorf("unable to apply pending "+
	"output: %w", err)
	}

.

Note that we do have the transfer log, and can resume from that, but this doesn't allow us to also execute this call back that finalizes the transfer.

One other relevant detail is that transferReceiverProof sets the state to SendStateComplete:

taproot-assets/tapfreighter/chain_porter.go

Line 814 in fca9d11

pkg.SendState = SendStateComplete

, but only after it has completed. There's a line before the call that prematurely updates this state:

taproot-assets/tapfreighter/chain_porter.go

Lines 1169 to 1172 in fca9d11

	// We'll set the package state to complete early here so the
	// main loop breaks out. We'll continue to attempt proof
	// deliver in the background.
	currentPkg.SendState = SendStateComplete

Steps to reproduce

Update an itest to initiate a transfer, confirm, but then cause the proof transfer to fail.

Expected behavior

It should continue to both transmit the proofs, but also fix the logic above to call ConfirmParcelDelivery as soon as we get the confirmation notification.

We should also remove the premature update of SendStateComplete. This'll ensure that that case gets re-executed on start up until the proofs are actually sent.

With the above, we also need to read the parcels from disk, to relaunch the state machine for parcels that weren't finalized.

Actual behavior

On restart, lnd won't reattempt proof transfer. However, the proof files on disk are properly updated.

[Roasbeef]

Right after posting this, I realize that we do resume the parcels here: https://github.com/lightninglabs/taproot-assets/blob/main/tapfreighter/chain_porter.go#L164-L177.

So the restart logic is fine, but we still shouldn't let inability to deliver a proof stop us from calling ConfirmParcelDelivery. In an observed case, a user had the wrong/invalid host set for a proof courier, so a connection can't made, so proofs can never be delivered.

[ffranr]

So the restart logic is fine, but we still shouldn't let inability to deliver a proof stop us from calling ConfirmParcelDelivery. In an observed case, a user had the wrong/invalid host set for a proof courier, so a connection can't made, so proofs can never be delivered.

@Roasbeef Here's what I think we should do:

To help ensure reliable proof courier services, the sender should verify successful ping responses from all target proof courier services before attempting the transfer. If a proof cannot be delivered to the proof courier, but the courier did respond to a ping, the existing backoff procedure should be utilized to retry delivery. Should we run out of backoff attempts, the proof must be synced to a public universe server before calling ConfirmParcelDelivery. If the proof is not synced to a public universe server, ConfirmParcelDelivery should not be called, and proof delivery to the target proof courier should be retried later.

This process aims to avoid the scenario where the proof is not delivered to the proof courier, not synced with a public universe server, and the parcel is marked as complete. In that scenario the receiver may never receive their proof.

What do you think?

[jharveyb]

Looking at transferReceiverProof more closely, it looks like we don't record the transfer until proof courier upload succeeds? That seems backwards.

The transfer happened regardless of upload to a courier service. Proofs could also be sent to the receiver without a courier. Seems like we should re-order the DB call ConfirmParcelDelivery to be before the proof courier interaction.

If that SendStateComplete is happening early, I don't see where we record that courier delivery actually succeeded. That could be a separate column in the transfers table IMO.

The restart logic seems to only handle transfers unconfirmed on-chain, not those pending delivery via courier.

A neater solution may be a background loop to deliver undelivered receiver transfer proofs when sent a signal by the chain porter. If we write transfer data before attempting delivery, we can always find undelivered transfer proofs on restart.

[Roasbeef]

A neater solution may be a background loop to deliver undelivered receiver transfer proofs when sent a signal by the chain porter. If we write transfer data before attempting delivery, we can always find undelivered transfer proofs on restart.

Yeah I think this is the way to go. We should decouple the proof transfer from confirming on disk which updates all the relevant records and allows users to send the assets once again. With the way things are, we write the proof courier addr to disk with the transfer outputs. This means that once set, it can't be updated again. Instead, we should split this into two state effectively, where we update everything on disk then continue to transmit in the background. We can do this with another state.