Make the base fee configurable in ChannelConfig #975
Conversation
|
0.1 since none of our immediate users need payment forwarding, but without this we will spuriously fail payments. |
|
Oh whoops, this needs #970. |
|
Moving this back to 0.0.99 as I have another PR that should be based on it which needs to go in 0.0.99. |
| // Note that the behavior here should be identical to the above block - we | ||
| // should NOT reveal the existence or non-existence of a private channel if | ||
| // we don't allow forwards outbound over them. | ||
| break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None)); |
There was a problem hiding this comment.
Side-note, if we allow forwards outbound, i believe you still a timing attack to break the confidentiality of private channel by forwarding probing HTLC through a hub with tlv_payload's short_channel_id built from observed P2WSH outputs on the transaction logs. If the hub consumes a long delay to sent back an error, it means an update_add_htlc has been tried and rejected (for lack of payment_secret) by a counterparty on the other-side. If the hub consumes a short delay to sent back an error, it means the error has been generated by the hub, no such private channel is attached to the hub.
Does it work ? I've never tried it though i do remember talking about it with @naumenkogs a while back.
There was a problem hiding this comment.
Right, for something with accept_forwards_to_priv_channels set we do have such a timing attack. At some point we need to move more things behind the forwarding delay (including claims, this, etc), its just low on the priority list :/.
There was a problem hiding this comment.
Tracked in #680, yeah agree low-priority for now...
lightning/src/util/config.rs
Outdated
| /// update messages sent to notify all nodes of our updated relay fee. | ||
| /// | ||
| /// Default value: 1000. | ||
| pub fee_base_msat: u32, |
There was a problem hiding this comment.
Maybe add a note that default setting is matching market-rate as of June 2021 and that a routing node operator may lower it to increase competitivity as it's factor likely accounted by implementation autopilots at payment path building.
Also, we could add a different logic where the fee_base_msat increase in function of the channel load, thus making the last remaining HTLC slots more expensive to harden the liquidity cost of jamming the channel and divert the jamming flow to cheaper nodes ? Though I concede it's hard to evaluate utility of such mitigation without precise a jamming attacker, especially laying out attacker economic rationality of targeting this node.
Though i agree with you on not caring about fee_base_msat for flood-and-lood attacks, the cost risk of going onchain with any HTLC should be accounted as part of channel mitigation with some kind of compensation. A flood-and-lood attacker, motivated to steal HTLC, won't care about high-HTLC cost ?
There was a problem hiding this comment.
Maybe add a note that default setting is matching market-rate as of June 2021
done.
Also, we could add a different logic where the fee_base_msat increase in function of the channel load, thus making the last remaining HTLC slots more expensive to harden the liquidity cost of jamming the channel and divert the jamming flow to cheaper nodes ? Though I concede it's hard to evaluate utility of such mitigation without precise a jamming attacker, especially laying out attacker economic rationality of targeting this node.
Yea, there's a few things I'd love to do with the feebase, but it doesn't belong in ChannelManager, IMO. I've been idly thinking of a separate crate that "manages" some stuff like this for routing nodes - auto-rebalances channels by intercepting payment events before handing them to the user, manages feerates based on various criteria, etc. Its a conversation for later, though.
Though i agree with you on not caring about fee_base_msat for flood-and-lood attacks, the cost risk of going onchain with any HTLC should be accounted as part of channel mitigation with some kind of compensation
Yea, but ultimately I think this is a question for the spec and pre-paying a fee followed by a fee rebate when its claimed, or something like that.
There was a problem hiding this comment.
I've been idly thinking of a separate crate that "manages" some stuff like this for routing nodes - auto-rebalances channels by intercepting payment events before handing them to the user, manages feerates based on various criteria, etc. Its a conversation for later, though.
Yeah we're aligned on this, better to get channel liquidity management in another crate!
Yea, but ultimately I think this is a question for the spec and pre-paying a fee followed by a fee rebate when its claimed, or something like that.
Yeap, same here beyond the scope of our implementation. Just to be sure we agree on what this new fee_base_msat is not covering.
lightning/src/util/config.rs
Outdated
| @@ -224,6 +236,23 @@ pub struct UserConfig { | |||
| pub peer_channel_config_limits: ChannelHandshakeLimits, | |||
| /// Channel config which affects behavior during channel lifetime. | |||
| pub channel_options: ChannelConfig, | |||
| /// If a channel is not set to be publicly announced, we reject HTLCs which were set to be | |||
There was a problem hiding this comment.
nit: maybe we should dissociate HTLC "rejection" at update_add_htlc processing from "forward failure", what do you think about "fail forward" ?
There was a problem hiding this comment.
You mean in the docs? I feel like "reject" communicates whats going on, especially to a user who doesn't know about internal distinctions of HTLC rejections. "we fail HTLCs which were set" is slightly less clear to me, but also has roughly the same meaning in non-technical english.
There was a problem hiding this comment.
Well, i think it's yet another anticipation of introducing custom forward policy (PendingHTLCsRelayable), which would a new source of rejection cases. We'll cleanup at that time i guess.
| /// ensure you are not exposed to any forwarding risk. | ||
| /// | ||
| /// Note that because you cannot change a channel's announced state after creation, there is no | ||
| /// way to disable forwarding on public channels retroactively. Thus, in order to change a node |
There was a problem hiding this comment.
I think #680 by allowing to load custom routing policy would make it easy to disallow forwarding in-flight on a named public channels by dropping the HTLC. Yep iptables for LN routing nodes!
There was a problem hiding this comment.
Right, if users really want to do so, I guess we should let them, but they really shouldn't, ideally, want to do so - if you aren't "really" routing payments, your channel shouldn't be public. I wonder if we should only apply #680 to routing out over private channels? Anyway, we can debate it there.
TheBlueMatt
force-pushed
the
2021-06-fix-fee-calc
branch
2 times, most recently
from
ab8b7aa to
0cc8feb
Compare
|
Rebased on latest git now that #970 is merged. |
TheBlueMatt
force-pushed
the
2021-06-fix-fee-calc
branch
2 times, most recently
from
690b17b to
e8ac6a2
Compare
Codecov Report
@@ Coverage Diff @@
## main #975 +/- ##
==========================================
+ Coverage 90.74% 90.76% +0.01%
==========================================
Files 60 60
Lines 30735 30914 +179
==========================================
+ Hits 27891 28058 +167
- Misses 2844 2856 +12
Continue to review full report at Codecov.
|
TheBlueMatt
force-pushed
the
2021-06-fix-fee-calc
branch
from
e8ac6a2 to
2b4bb60
Compare
TheBlueMatt
force-pushed
the
2021-06-fix-fee-calc
branch
from
2b4bb60 to
be75d5f
Compare
TheBlueMatt
force-pushed
the
2021-06-fix-fee-calc
branch
2 times, most recently
from
78bc058 to
b287400
Compare
| default_config.channel_options.cltv_expiry_delta = 6*6; | ||
| default_config.channel_options.announced_channel = true; | ||
| default_config.peer_channel_config_limits.force_announced_channel_preference = false; | ||
| default_config.own_channel_config.our_htlc_minimum_msat = 1000; // sanitization being done by the sender, to exerce receiver logic we need to lift of limit |
There was a problem hiding this comment.
nit: can't parse exerce -- could this comment be clarified?
There was a problem hiding this comment.
I believe it was meant as exercise, but I have no idea what it means overall lol. Maybe @ariard remembers?
There was a problem hiding this comment.
IIRC, it's related to test_override_0msat_htlc_minimum, where we want to pass 0-msat value as a config and verify that our implementation-sanitization logic at channel opening bumps correctly to 1. See commit 9e03d2b. It's a bit gross, rational is for covering spec requirement in BOLT2
A receiving node:
receiving an amount_msat equal to 0, OR less than its own htlc_minimum_msat:
SHOULD fail the channel.
Yes exerce is to be understood as exercise, pardon my french :p
There was a problem hiding this comment.
Thanks! Updated the comment to describe it.
| use ln::channelmanager::CLTV_FAR_FAR_AWAY; | ||
| use bitcoin::secp256k1; | ||
| fn test_fee_failures() { | ||
| // When we check for amount_below_minimum below, we want to test that we're using the *right* |
There was a problem hiding this comment.
Where is this check...? Can't tell if this comment is intentionally the same as test_onion_failure's
Also, could you add a link to the bug mentioned in the commit msg, to the commit msg?
There was a problem hiding this comment.
Its copied from the below test, but, indeed, doesn't make sense here.
TheBlueMatt
force-pushed
the
2021-06-fix-fee-calc
branch
2 times, most recently
from
cfc8efe to
3dcf5c4
Compare
| // In an earlier version, we spuriously failed to forward payments if the expected feerate | ||
| // changed between the channel open and the payment. |
There was a problem hiding this comment.
Is this a reference to the msat-vs-sat bug mentioned in the commit msg? Maybe missing something, but where can I find more information about the msat-vs-sat bug?
There was a problem hiding this comment.
No, literally current code is "we decide what we think the fee should be when we receive a payment to forward, and dont bother to update our announcements".....
There was a problem hiding this comment.
Oops, sorry, an earlier version of this patchset had separated out the bugfixes for the two bugs, but now they're in the same, I've updated the commit message to be more clear.
| channel_monitors.insert(monitor_a.get_funding_txo().0, &mut monitor_a); | ||
| channel_monitors.insert(monitor_b.get_funding_txo().0, &mut monitor_b); | ||
| <(BlockHash, ChannelManager<EnforcingSigner, &test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestLogger>)>::read(&mut nodes_1_read, ChannelManagerReadArgs { | ||
| default_config: no_announce_cfg, |
There was a problem hiding this comment.
Just to be clear, users couldn't do exactly what this test is doing, to change their config on restart?
There was a problem hiding this comment.
Nope, users can change the non-channel parts of the config at restart (but not at runtime...).
| let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); | ||
| let mut no_announce_cfg = test_default_channel_config(); | ||
| no_announce_cfg.channel_options.announced_channel = false; | ||
| no_announce_cfg.accept_forwards_to_priv_channels = false; |
There was a problem hiding this comment.
Guess we shouldn't need this because it's false by default?
There was a problem hiding this comment.
Sure, I just like making the test robust against a change to default values in the future.
TheBlueMatt
force-pushed
the
2021-06-fix-fee-calc
branch
2 times, most recently
from
69374ba to
d303097
Compare
| /// | ||
| /// Note that we do not check if you are currently connected to the given peer. If no | ||
| /// connection is available, the outbound `open_channel` message may fail to send, resulting in | ||
| /// the channel eventually being silently forgotten. |
There was a problem hiding this comment.
What do you mean by "being silently forgotten", can't remember this case is covered by peer_disconnected ?
There was a problem hiding this comment.
It is, it will be silently forgotten when we disconnect or restart.
TheBlueMatt
force-pushed
the
2021-06-fix-fee-calc
branch
from
d303097 to
12aa66d
Compare
|
Rebased and added a commit (that only makes sense if we merge this for 0.0.99) to use the same deserialization version check as here in the fields from #984. |
TheBlueMatt
force-pushed
the
2021-06-fix-fee-calc
branch
2 times, most recently
from
923a69a to
cb68789
Compare
TheBlueMatt
force-pushed
the
2021-06-fix-fee-calc
branch
3 times, most recently
from
6041cd5 to
09d51ea
Compare
|
Squashed with only the typo fix changed: |
|
Concept ACK defaulting to 1 sat and making configurable :) |
| @@ -4460,7 +4460,7 @@ fn is_unsupported_shutdown_script(their_features: &InitFeatures, script: &Script | |||
| return !script.is_p2pkh() && !script.is_p2sh() && !script.is_v0_p2wpkh() && !script.is_v0_p2wsh() | |||
| } | |||
|
|
|||
| const SERIALIZATION_VERSION: u8 = 1; | |||
| const SERIALIZATION_VERSION: u8 = 2; | |||
There was a problem hiding this comment.
One follow-up is documenting our serialization philosophy in a doc/serialization.md, explain when/why we introduce changes, the odd/even scheme with TLV, that bindings should stay in-sync and what actions user should take in consequence?
Maybe it could be part of LDK docs, but when it's a library interface I would favor the stability guarantees to be documented in-tree ?
There was a problem hiding this comment.
Yea, we're kinda still figuring it out and doing a case-by-case "what do you want the old clients to do". Def could add to CONTRIBUTING that we have to meticulously document any backwards compat concerns in the CHANGELOG :)
lightning/src/util/config.rs
Outdated
| /// this node. | ||
| /// | ||
| /// Default value: 1000. | ||
| pub fee_base_msat: u32, |
There was a problem hiding this comment.
One readability improvement could be to make it matching the method outbound_forwarding_fee_base_msat. Even precise the type of packet forwarded, HTLC (we can't exclude to have different default routing policies if some future packets are more CPU-intensive like PTLC/n-ary outcome DLC due to MuSig rounds). And maybe align get_fee_proportional_millionths() naming, like get_outbound_forwarding_fee_proportional_msat?
Yep comment quite inspired by Core inconsistency around its units either used internally or exposed to user (kilo-sat per vbytes, btc per vbyte, WU, packages evaluated in KvB, ...)
There was a problem hiding this comment.
I just mentioned its the outbound leg in the docs, adding it to the variable name seemed like a useless distinction.
lightning/src/util/config.rs
Outdated
| @@ -197,6 +208,7 @@ impl Default for ChannelConfig { | |||
| fn default() -> Self { | |||
| ChannelConfig { | |||
| fee_proportional_millionths: 0, | |||
| fee_base_msat: 1000, | |||
There was a problem hiding this comment.
Note, mutating to 1001 breaks test_priv_forwarding_rejection but the lower-boundary value 999 doesn't break test coverage :/
edit: but covered by fuzzer :)
There was a problem hiding this comment.
Hmm, well that's mostly cause test_priv_forwarding_rejection isn't intended to test the default fee value :). The onion_route_tests and others will fail if we don't properly check the forwarding fee, though.
|
|
||
| assert!(nodes[0].node.list_usable_channels()[0].is_public); | ||
| assert_eq!(nodes[1].node.list_usable_channels().len(), 2); | ||
| assert!(!nodes[2].node.list_usable_channels()[0].is_public); |
There was a problem hiding this comment.
Hmmmm, i was suggesting to add the following diff but I do have non-deterministic failure, i think it's inherited from list_usable_channels()
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 29071ab0..11154f5c 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -7960,6 +7960,8 @@ fn test_priv_forwarding_rejection() {
assert!(nodes[0].node.list_usable_channels()[0].is_public);
assert_eq!(nodes[1].node.list_usable_channels().len(), 2);
+ assert!(nodes[1].node.list_usable_channels()[0].is_public);
+ assert!(!nodes[1].node.list_usable_channels()[1].is_public);
assert!(!nodes[2].node.list_usable_channels()[0].is_public);
// We should always be able to forward through nodes[1] as long as its out through a public
There was a problem hiding this comment.
The ordering of the channels is non-deterministic - its based on the random seed used for the hashmap, even, so this will fail randomly.
TheBlueMatt
force-pushed
the
2021-06-fix-fee-calc
branch
from
09d51ea to
f557df9
Compare
|
Squashed a few trivial changes to address all review feedback, will take after CI. Full diff from Val's review: |
TheBlueMatt
force-pushed
the
2021-06-fix-fee-calc
branch
3 times, most recently
from
c3be826 to
7658421
Compare
This was missed prior to 0.0.98, so requires a backwards-compatibility wrapper inside the `Channel` serialization logic, but it's not very complicated to do so.
Currently the base fee we apply is always the expected cost to
claim an HTLC on-chain in case of closure. This results in
significantly higher than market rate fees [1], and doesn't really
match the actual forwarding trust model anyway - as long as
channel counterparties are honest, our HTLCs shouldn't end up
on-chain no matter what the HTLC sender/recipient do.
While some users may wish to use a feerate that implies they will
not lose funds even if they go to chain (assuming no flood-and-loot
style attacks), they should do so by calculating fees themselves;
since they're already charging well above market-rate,
over-estimating some won't have a large impact.
Worse, we current re-calculate fees at forward-time, not based on
the fee we set in the channel_update. This means that the fees
others expect to pay us (and which they calculate their route based
on), is not what we actually want to charge, and that any attempt
to forward through us is inherently race-y.
This commit adds a configuration knob to set the base fee
explicitly, defaulting to 1 sat, which appears to be market-rate
today.
[1] Note that due to an msat-vs-sat bug we currently actually
charge 1000x *less* than the calculated cost.
|
Turns out upstream changes made the new tests fail, applied this diff on top of a clean rebase. |
TheBlueMatt
force-pushed
the
2021-06-fix-fee-calc
branch
from
7658421 to
352594b
Compare
Private nodes should never wish to forward HTLCs at all, which we support here by disabling forwards out over private channels by default. As private nodes should not have any public channels, this suffices, without allowing users to disable forwarding over channels announced in the routing graph already. Closes lightningdevkit#969
Instead of interpreting the backwards compatibility data in Channel serialization, use the serialization version bump present in 0.0.99 as the flag to indicate if a channel should be read in backwards compatibility.
TheBlueMatt
force-pushed
the
2021-06-fix-fee-calc
branch
from
352594b to
4cc0d9d
Compare
Currently the base fee we apply is always the expected cost to
claim an HTLC on-chain in case of closure. This results in
significantly higher than market rate fees [1], and doesn't really
match the actual forwarding trust model anyway - as long as
channel counterparties are honest, our HTLCs shouldn't end up
on-chain no matter what the HTLC sender/recipient do.
While some users may wish to use a feerate that implies they will
not lose funds even if they go to chain (assuming no flood-and-loot
style attacks), they should do so by calculating fees themselves;
since they're already charging well above market-rate,
over-estimating some won't have a large impact.
This commit adds a configuration knob to set the base fee
explicitly, defaulting to 1 sat, which appears to be market-rate
today.
[1] Note that due to an msat-vs-sat bug we currently actually
charge 1000x less than the calculated cost.