Fix min relay fee to be 1s/vB #3457
Conversation
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
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
| @@ -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.
| 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.
| 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?
| 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); |
| 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).
| // 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?
Bitcoin Core relay policy does not require 16s/vB, which it was
previously set to.
This should address #3438.