[NishantBansal2003]

ref: #209 (comment)

Some more oracles added in this PR:

open_channel

• chain_hash != regtest
• channel_reserve_satoshis >= funding_satoshis
• dust_limit_satoshis > 10_000 sat
• to_self_delay > 2016
• feerate_per_kw == 0 for non 0FC channels

accept_channel

• dust_limit_satoshis > 10_000 sat
• htlc_minimum_msat > max_htlc_value_in_flight_msat
• htlc_minimum_msat > open_channel.funding_satoshis
• to_self_delay == 0
• max_accepted_htlcs == 0
• pubkey match any pubkeys from open_channel and accept_channel
• per_commitment_point should not be reused

Notes

• CLN defaults to_self_delay to 6 in accept_channel on regtest and 144 on mainnet, so I think we should only check that to_self_delay != 0
• CLN defaults the feerate_per_kw floor to 1 on regtest and 253 on mainnet when accepting the open_channel message, so I think we should only check that feerate_per_kw != 0

[morehouse]

It turns out LDK actually reuses payment_basepoints within a pool of 1000 points:

• https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/src/commit/15c5d9b8459e852f2b33586e586e3e90307114ee/lightning/src/sign/mod.rs#L154-L162
• https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/src/commit/15c5d9b8459e852f2b33586e586e3e90307114ee/lightning/src/sign/mod.rs#L2246-L2278

They do this to assist in recovery of funds after data loss. AFAICT, this is a safe legitimate thing to do since the private key for payment_basepoint is never revealed.

So we probably need to rethink our approach here. I think we should refine the oracle to flag only these two cases:

• target uses any pubkey that we know the privkey for
• target PCP matches any other target key (except for aliases/retransmit exceptions)

We could do this with a single map that distinguishes between the above types of pubkeys. Something like:

enum KeyOrigin {
  Ours { instruction: usize },
  TargetBasepoint { channel: ChannelId, field: &'static str },
  TargetPcp { channel: ChannelId, field: &'static str, commitment_number: u64 },
}
revealed_pubkeys: HashMap<PublicKey, KeyOrigin>

The checks we would implement:

• RecvAcceptChannel: check all received pubkeys against the map, reporting a violation if:
  • any basepoints are present as Ours or TargetPcp
  • the first_per_commitment_point matches any entry
• RecvChannelReady: check second_per_commitment_point against the map, reporting a violation if it matches any entry, unless the entry is TargetPcp with the exact same channel and commitment_number
• RecvRevokeAndAck: check next_per_commitment_point against the map, reporting a violation if it matches any entry, unless the entry is TargetPcp with the exact same channel and commitment_number

[morehouse]

sample_pubkey(1) is also used in the sample open_channel, which makes this test look like it's going to fail once the channel_ready oracle is added.

Suggested change

	let target_pcp = sample_pubkey(1);
	let target_pcp = sample_pubkey(9);

[NishantBansal2003]

I assume this is just to clear up the confusion? since we don't execute any negotiate_channel instructions that would add open_channel keys to revealed

[morehouse]

Yeah the suggested change is just the minimal change that helps slightly with the confusion. An even better change would rewrite the test so that we're not relying on this hidden behavior (i.e. we injected the negotiation without processing the corresponding open_channel).

[morehouse]

BTW I still have some refactorings planned for the executor tests that should make it easier to use the full flow rather than injecting the negotiation as a shortcut. My eventual goal is to avoid injecting any test state into the executor unless absolutely necessary.

[NishantBansal2003]

An even better change would rewrite the test so that we're not relying on this hidden behavior (i.e. we injected the negotiation without processing the corresponding open_channel).

Yeah, I also want to avoid injecting negotiations, but currently sample_funding_negotiation().accept_channel reuses pubkeys whose params are used for the further funding flow recv operations. So we won't be able to reach that state without injecting, otherwise we get an oracle violation. But I think this could be simplified once we have instructions for the full flow

[morehouse]

Ready for squash.