Expose more info in PaymentForwarded

[TheBlueMatt]

When this was added I figured we should be conservative to avoid users logging exact forwarding history (this is bad for network-wide privacy!) but that doesn't mean important info isn't missing from them that users will want. Ultimately I think we have to air on the side of features, not privacy, sadly - while we shouldn't bother to include the payment hash, I think, we should include things like the source + sink nodes/channels in the event.

[atalw]

Do we want the source key to be a PublicKey type or HTLCSource type? Don't have easy access to data for both so I think we'll have to move things around a bit. Also could you please elaborate a little on what sink nodes and sink channels are?

[TheBlueMatt]

No, HTLCSource is an internal type with a bunch of additional data and cryptographic material we need to fail backwards. We'll want to use a PublicKey and/or channel id (as a [u8; 32]) maybe both.

[keyuebao]

Having a channel id here would be super helpful 👍

[ghost]

If no one is already working on this, I'd like to take this on. I can probably have this completed by next weekend.

(p.s. I appreciate the good first issue labels, ty)

Notes for self

Event is described here

rust-lightning/lightning/src/util/events.rs

Lines 343 to 363 in 7671ae5

	/// This event is generated when a payment has been successfully forwarded through us and a
	/// forwarding fee earned.
	PaymentForwarded {
	/// The fee, in milli-satoshis, which was earned as a result of the payment.
	///
	/// Note that if we force-closed the channel over which we forwarded an HTLC while the HTLC
	/// was pending, the amount the next hop claimed will have been rounded down to the nearest
	/// whole satoshi. Thus, the fee calculated here may be higher than expected as we still
	/// claimed the full value in millisatoshis from the source. In this case,
	/// `claim_from_onchain_tx` will be set.
	///
	/// If the channel which sent us the payment has been force-closed, we will claim the funds
	/// via an on-chain transaction. In that case we do not yet know the on-chain transaction
	/// fees which we will spend and will instead set this to `None`. It is possible duplicate
	/// `PaymentForwarded` events are generated for the same payment iff `fee_earned_msat` is
	/// `None`.
	fee_earned_msat: Option<u64>,
	/// If this is `true`, the forwarded HTLC was claimed by our counterparty via an on-chain
	/// transaction.
	claim_from_onchain_tx: bool,
	},

and at quick glance, the references to update are

rust-lightning/lightning/src/util/events.rs

Lines 523 to 528 in 7671ae5

	&Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx } => {
	7u8.write(writer)?;
	write_tlv_fields!(writer, {
	(0, fee_earned_msat, option),
	(2, claim_from_onchain_tx, required),
	});

and

rust-lightning/lightning/src/util/events.rs

Lines 684 to 695 in 7671ae5

	7u8 => {
	let f = || {
	let mut fee_earned_msat = None;
	let mut claim_from_onchain_tx = false;
	read_tlv_fields!(reader, {
	(0, fee_earned_msat, option),
	(2, claim_from_onchain_tx, required),
	});
	Ok(Some(Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx }))
	};
	f()
	},

and

rust-lightning/lightning/src/ln/channelmanager.rs

Lines 4295 to 4298 in 6b8ad4e

	pending_events.push(events::Event::PaymentForwarded {
	fee_earned_msat,
	claim_from_onchain_tx: from_onchain,
	});

There's also tests, mostly calling this macro

rust-lightning/lightning/src/ln/functional_test_utils.rs

Lines 1330 to 1342 in 7671ae5

	macro_rules! expect_payment_forwarded {
	($node: expr, $expected_fee: expr, $upstream_force_closed: expr) => {
	let events = $node.node.get_and_clear_pending_events();
	assert_eq!(events.len(), 1);
	match events[0] {
	Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx } => {
	assert_eq!(fee_earned_msat, $expected_fee);
	assert_eq!(claim_from_onchain_tx, $upstream_force_closed);
	},
	_ => panic!("Unexpected event"),
	}
	}
	}

[atalw]

I've been working on this and am nearing completion. There is one bug (unexpected behaviour?) which is causing a few tests to fail so maybe you could help out. Please refer to these comments:
Comment one
Comment two

[TheBlueMatt]

We should still expose the destination/sink channel id, #1419 only exposed the source.

[atalw]

I'll add that next!

[TheBlueMatt]

You may want to take a look at #1231, which had actually implemented that half as well, but wasn't actively being worked on. Should be able to copy a bunch of that work.