Fix min relay fee to be 1s/vB #3457
Conversation
arik-so
force-pushed
the
min_relay_fee_fix
branch
from
b8f4153 to
7d6b1f3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3457 +/- ##
==========================================
+ Coverage 89.69% 89.71% +0.01%
==========================================
Files 130 130
Lines 107614 107629 +15
Branches 107614 107629 +15
==========================================
+ Hits 96528 96557 +29
+ Misses 8678 8671 -7
+ Partials 2408 2401 -7 ☔ View full report in Codecov by Sentry. |
| @@ -176,7 +176,7 @@ pub trait FeeEstimator { | |||
| } | |||
|
|
|||
| /// Minimum relay fee as required by bitcoin network mempool policy. | |||
| pub const MIN_RELAY_FEE_SAT_PER_1000_WEIGHT: u64 = 4000; | |||
| pub const MIN_RELAY_FEE_SAT_PER_1000_WEIGHT: u64 = 253; | |||
There was a problem hiding this comment.
Could we delete this in favor of FEERATE_FLOOR_SATS_PER_KW below ? Different types, but same number.
There was a problem hiding this comment.
I don't think we can, because one is the regular broadcast fee rate, and the other is the eviction/incremental fee rate per RBF rule 4 (https://gist.github.com/glozow/25d9662c52453bd08b4b4b1d3783b9ff)
There was a problem hiding this comment.
that said, looking at these values
"mempoolminfee": 0.00001373,
"minrelaytxfee": 0.00001000,
"incrementalrelayfee": 0.00001000I don't know when minrelaytxfee and incrementalrelayfee wouldn't align short of an explicit config change.
There was a problem hiding this comment.
Thank you where did you find them ? I've been grepping the bitcoin repo, can't see them.
I agree these are two different settings - I suggest we clarify one of them is the incremental fee rate, the other is the broadcast fee rate, as you've described - "MIN_RELAY_FEE" and "FEERATE_FLOOR" is a little too similar.
There was a problem hiding this comment.
oh I just called getmempoolinfo on my node's RPC lol
There was a problem hiding this comment.
fair point, honestly both names suck
There was a problem hiding this comment.
could use the names from bitcoin core, minrelaytxfee and incrementalrelayfee
There was a problem hiding this comment.
let's start with renaming minrelaytxfee to incrementalrelayfee for now, and we can get more ambitious with the other one in a separate PR
lightning/src/ln/functional_tests.rs
Outdated
| @@ -1312,18 +1312,21 @@ fn test_duplicate_htlc_different_direction_onchain() { | |||
|
|
|||
| let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1); | |||
|
|
|||
| let payment_value_sats = 546; | |||
There was a problem hiding this comment.
Why was the value changed to 546?
Also the naming is bound to cause confusion: payment_value_sats sounds like it should correspond to the same payment as the payment_preimage and payment_hash variables, but it actually is a different payment.
Also, we should define the variables closer to where they are used.
There was a problem hiding this comment.
I added a comment explaining the reasoning, and revised the variable once again. The fee bumping was succeeding much more often with the lower minrelayfee, resulting in indeterminism depending on whether the block connection style skipped intermediates or not. Now the value is fine-tuned to accommodate a single 25% fee bump.
lightning/src/ln/functional_tests.rs
Outdated
| assert!(claim_txn.len() >= 3); | ||
| assert!(claim_txn.len() <= 5); |
There was a problem hiding this comment.
Shouldn't the claim transactions be deterministic? Why are we accepting a range of transactions now?
It would be good to add a comment explaining the transactions expected to be broadcast.
There was a problem hiding this comment.
Funnily enough, with intermediate blocks being skipped, it is theoretically fine to have non-determinism, but I adjusted the values such that it can only happen once.
lightning/src/ln/functional_tests.rs
Outdated
| let htlc_value_a_msats = 847_000; | ||
| let htlc_value_b_msats = 546_000; |
There was a problem hiding this comment.
Why are the payment values changed?
There was a problem hiding this comment.
I added a comment explaining these. In principle they don't even have to differ, but both 842 and 843 only allow one bump. I could make one of them the necessary satoshi amount for two bumps - 1 if you like?
lightning/src/ln/functional_tests.rs
Outdated
| assert_eq!(remote_txn[0].input[0].previous_output.txid, chan.3.compute_txid()); | ||
| let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1000000, 59000000); | ||
| let (payment_preimage, payment_hash, ..) = route_payment(&nodes[0], &[&nodes[1]], htlc_value_a_msats); | ||
| route_payment(&nodes[1], &vec!(&nodes[0])[..], htlc_value_b_msats).0; |
There was a problem hiding this comment.
Return value is unused.
| route_payment(&nodes[1], &vec!(&nodes[0])[..], htlc_value_b_msats).0; | |
| route_payment(&nodes[1], &vec!(&nodes[0])[..], htlc_value_b_msats); |
lightning/src/ln/functional_tests.rs
Outdated
| assert_eq!(node_txn.len(), 3); | ||
| // plus, depending on the block connection style, two further bumps | ||
| assert!(node_txn.len() >= 3); | ||
| assert!(node_txn.len() <= 6); |
There was a problem hiding this comment.
If additional bumps can occur, we should verify all of them below (not just the first bump).
There was a problem hiding this comment.
They no longer should be able to.
lightning/src/ln/functional_tests.rs
Outdated
| // preimage and timeout sweeps from remote commitment + preimage sweep bump | ||
| assert_eq!(node_txn.len(), 3); | ||
| // plus, depending on the block connection style, two further bumps |
There was a problem hiding this comment.
So the bumping strategy can vary depending on how the user calls the block connection APIs?
How specifically is bumping affected, and could this lead to transactions not confirming in time?
There was a problem hiding this comment.
So technically it's not the bumping strategy as the block connection strategy. The bumping strategy is technically the same, but in one case there's a time jump and it lacks the opportunity to attempt intermediate bumps.
arik-so
force-pushed
the
min_relay_fee_fix
branch
3 times, most recently
from
9af75bc to
cd4c50f
Compare
Bitcoin Core relay policy does not require 16s/vB, which it was previously set to.
arik-so
force-pushed
the
min_relay_fee_fix
branch
from
cd4c50f to
1634965
Compare
| } | ||
|
|
||
| let new_feerate = new_fee * 1000 / predicted_weight; | ||
| log_trace!(logger, "Fee rate bumped by {}s from {} s/KWU ({} s) to {} s/KWU ({} s) (naive: {} s/KWU ({} s))", new_fee - previous_fee, previous_feerate, previous_fee, new_feerate, new_fee, naive_new_feerate, naive_new_fee); |
There was a problem hiding this comment.
This naive feerate/fee logging is a little confusing. IMO we could just remove it.
There was a problem hiding this comment.
I felt it was quite valuable for debugging purposes seeing whether the bump would be failing simply due to the 25% increment or due to the incremental relay fee. Do you think perhaps a different phrasing might help?
There was a problem hiding this comment.
I think the log message you added above clarifies things. We can remove the "(naive: ..)" logging from this log line now.
arik-so
force-pushed
the
min_relay_fee_fix
branch
from
1634965 to
6aca2d3
Compare
|
You're right, I was just tinkering with some values for the test. The potential list of confirmation target / fee strategy combinations would be rather exhaustive, so I focused on the main scenario that was not previously being caught, but let me know if there are others you think I should add to the test. |
| } | ||
|
|
||
| let new_feerate = new_fee * 1000 / predicted_weight; | ||
| log_trace!(logger, "Fee rate bumped by {}s from {} s/KWU ({} s) to {} s/KWU ({} s) (naive: {} s/KWU ({} s))", new_fee - previous_fee, previous_feerate, previous_fee, new_feerate, new_fee, naive_new_feerate, naive_new_fee); |
There was a problem hiding this comment.
I think the log message you added above clarifies things. We can remove the "(naive: ..)" logging from this log line now.
|
|
||
| let remaining_output_amount = input_amounts - new_fee; | ||
| if remaining_output_amount < dust_limit_sats { | ||
| log_warn!(logger, "Can't new-estimation bump new claiming tx, output amount {} would end up below dust threshold {}", remaining_output_amount, dust_limit_sats); |
There was a problem hiding this comment.
Nit: "new-estimation bump" seems confusing.
| log_warn!(logger, "Can't new-estimation bump new claiming tx, output amount {} would end up below dust threshold {}", remaining_output_amount, dust_limit_sats); | |
| log_warn!(logger, "Can't bump new claiming tx, output amount {} would end up below dust threshold {}", remaining_output_amount, dust_limit_sats); |
|
|
||
| let remaining_output_amount = input_amounts - bumped_fee; | ||
| if remaining_output_amount < dust_limit_sats { | ||
| log_warn!(logger, "Can't new-estimation bump new claiming tx, output amount {} would end up below dust threshold {}", remaining_output_amount, dust_limit_sats); |
There was a problem hiding this comment.
Nit: "new-estimation bump" seems confusing.
| log_warn!(logger, "Can't new-estimation bump new claiming tx, output amount {} would end up below dust threshold {}", remaining_output_amount, dust_limit_sats); | |
| log_warn!(logger, "Can't bump new claiming tx, output amount {} would end up below dust threshold {}", remaining_output_amount, dust_limit_sats); |
|
|
||
| let bumped_fee_rate = feerate_bump(predicted_weight_units, input_satoshis, 546, 253, &fee_rate_strategy, confirmation_target, &fee_estimator, &logger); | ||
| assert!(bumped_fee_rate.is_none()); | ||
| logger.assert_log_regex("lightning::chain::package", regex::Regex::new(r"Can't new-estimation bump new claiming tx, output amount 545 would end up below dust threshold 546").unwrap(), 1); |
There was a problem hiding this comment.
This doesn't seem right...
- Weight: 1000
- Input amount: 861
- Previous feerate: 253
new_fee = max(1.25 * 253, 253 + 253) = 506remaining_output_amount = 861 - 506 = 355
Yet test test expects output amount of 545.
What am I missing?
There was a problem hiding this comment.
Here it notices that the remaining amount is insufficient even before applying the min relay fee, which only happens at the end of feerate_bump. It's an early termination path.
There was a problem hiding this comment.
Got it, thanks for explaining. By the way, it looks like that early termination path can be removed since the dust check is repeated here as well.
There was a problem hiding this comment.
Yeah, I was wondering myself if I should add that. The reason I did was because there were a bunch of "amount too small" terminations specifically within the 25% bump clause, which I guess might be a helpful hint for users, but those checks would technically also be preëmpted by a single final check ensuring the remaining amount was at or above the dust threshold.
Technically this method could be cleaned up quite significantly, but at the potential expense of logic traceability. You figure that's a non-issue?
There was a problem hiding this comment.
Or rather, because there was a 25%-bump-specific amount-too-small-check and one in the else clause, as opposed to a singular one that would come after the bump strategy processing.
There was a problem hiding this comment.
My intuition is that during debugging most of the path can be inferred by the amount of the fee bump (e.g., is it 25% or the incremental relay fee), especially for a short, fairly simple function like this one. In cases where something unexpected could happen, that's when logging is more helpful.
For instance, I'm not sure it's super helpful to have logs like this:
... Initiating fee rate bump from X to Y.
... Attempting forced 25% fee rate bump from X to (X*1.25)
... Naive fee bump of (X*.25) does not meet min relay fee of 253
... Fee rate bumped by 253 from X to (X+253)
when something like this communicates almost as much:
... Fee rate bumped from X to X+253
|
So both the modified unit tests pass highly consistently for me; in fact, I haven't been able to get them to fail once, and I tried hard-coding the block connection strategies to ensure it worked for every single one. Can you elaborate on what looks wrong to you? The unit tests are now performing the same checks as they had been originally. |
arik-so
force-pushed
the
min_relay_fee_fix
branch
from
6aca2d3 to
9db6c88
Compare
I was referring to the unit test that had a remaining output amount I didn't expect, which you clarified for me. The unit tests look correct to me, though I would prefer if they didn't rely so heavily on whitebox testing with the log pattern matching. It would also be good to add a short description to each test case explaining what it's testing. |
|
Ah gotcha. Yeah, I can remove the log output checks, and will add some comments |
Bitcoin Core relay policy does not require 16s/vB, which it was
previously set to.
This should address #3438.