slow jam attack impl - #114
Conversation
c337e06 to
3f0edb2
Compare
3f0edb2 to
b09f8fa
Compare
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
Looking good! I think we just need to tighten up validation a bit and then this will be good to go.
In a follow up, I think that it would be interesting to re-use this logic to do slot jamming (rather than the liquidity jamming that we have here) - it'll probably end up cheaper (reputation for ~200 above-dust htlcs should be cheaper than the full liquidity of the channel).
| { | ||
| fn setup_for_network(&self) -> Result<NetworkSetup, BoxError> { | ||
| // Validate that attacker receiver has channel with target. | ||
| self.target_channels |
There was a problem hiding this comment.
I think that we should add some stricter checks here, just so that we can be sure that everything is happening as expected:
channel_to_jamis intarget_channelsattacker_senderhas a channel with the node that ownschannel_to_jam- The
sanity_check_nodehas a channel withchannel_to_jamnode - The honest
receiver_pubkeyhas a channel withtarget
Specifically the last one, because this is how we check we've succeeded with a test payment. If we don't have a target -- receiver_pubkey channel, we'll still get a failed payment and think that we succeeded but actually it was just a bad route.
There was a problem hiding this comment.
will add for these 2
channel_to_jamis intarget_channels- The honest receiver_pubkey has a channel with target
for
- attacker_sender has a channel with the node that owns channel_to_jam
- The sanity_check_node has a channel with channel_to_jam node
would be hard to make those checks here since we don't have a list of all the channels (only the target channels) but I think that for
attacker_sender has a channel with the node that owns channel_to_jam
is implicitly checked if build_reputation succeeds. If attacker_sender didn't have a channel with the target's peer then build_reputation would fail and subsequently cause the attack to return an error.
The sanity_check_node has a channel with channel_to_jam node
This could be checked by making a test payment before the attack and check that it succeeds. Then if payment over same route over channel_to_jam fails then it means the channel is jammed (it could've failed for liquidity reasons but can also check for reason in results file and verify that it failed because of no resources)
There was a problem hiding this comment.
would be hard to make those checks here since we don't have a list of all the channels
We can use self.network_graph.read_only().channels() and then just check that the channel_to_jam's node_one / node_two are the attacker_sender and channel_to_jam pubkeys.
We could probably use the LDK graph for the target_channels as well, but we need them so much that it's worth keeping a list of them around IMO.
is implicitly checked if build_reputation succeeds.
This could be checked by making a test payment before the attack and check that it succeeds.
We can implicitly check some of these while running the simulation, but I think we should aim to fail-fast if we're spun up with a topology that we don't like (rather than pass validation of the network, then fail to run the simulation). Should be do-able with the LDK network graph? Even if it's just checking the pubkeys as suggested above.
There was a problem hiding this comment.
I completely blanked out on fact that I had the network_graph 🤦♂️
Added checks for these two:
- attacker_sender has a channel with the node that owns channel_to_jam
- The sanity_check_node has a channel with channel_to_jam node
e1d2079 to
ecd5233
Compare
|
addressed comments in fixup |
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
Really close, just some last validation gripes please 🙏
| self.honest_sender.0 | ||
| ))?; | ||
|
|
||
| let check_payment = async |expect_succeed: bool| -> Result<(), BoxError> { |
There was a problem hiding this comment.
I don't think we gain all that much making this a closure, could be implemented on attack instead?
Don't have strong feelings here, so take it or leave it!
| { | ||
| fn setup_for_network(&self) -> Result<NetworkSetup, BoxError> { | ||
| // Validate that attacker receiver has channel with target. | ||
| self.target_channels |
There was a problem hiding this comment.
would be hard to make those checks here since we don't have a list of all the channels
We can use self.network_graph.read_only().channels() and then just check that the channel_to_jam's node_one / node_two are the attacker_sender and channel_to_jam pubkeys.
We could probably use the LDK graph for the target_channels as well, but we need them so much that it's worth keeping a list of them around IMO.
is implicitly checked if build_reputation succeeds.
This could be checked by making a test payment before the attack and check that it succeeds.
We can implicitly check some of these while running the simulation, but I think we should aim to fail-fast if we're spun up with a topology that we don't like (rather than pass validation of the network, then fail to run the simulation). Should be do-able with the LDK network graph? Even if it's just checking the pubkeys as suggested above.
ecd5233 to
c28db78
Compare
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
LGTM!
Just one nit, feel free to ignore or add on squash. Feel free to go ahead with merge once squashed 🥇
| let mut attacker_channel_present = false; | ||
| let mut honest_sender_present = false; | ||
| for channel in peer_channels { | ||
| if let Some(channel_info) = graph.channel(*channel) { |
There was a problem hiding this comment.
nit: should we fail if the channel we got from graph.node.channels isn't present here?
There was a problem hiding this comment.
I would think there is something very wrong in LDK if we got it in the list of a node's channels but not in this channel call. But I don't mind adding.
c28db78 to
aaf9dcc
Compare
2 participants
No description provided.