Uh oh!
There was an error while loading. Please reload this page.
Add min feerate checks - #1552
Conversation
| L::Target: Logger, | ||
| { | ||
| let mut updated_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64; | ||
| assert!(updated_feerate >= FEERATE_FLOOR_SATS_PER_KW as u64); |
There was a problem hiding this comment.
Mh, do we really want to assert and panic here if something goes wrong?
Couldn't this just default to the floor, i.e., updated_feerate = max(updated_feerate, FEERATE_FLOOR_SATS_PER_KW)?
Just raising the question, but maybe panicking out is exactly what we want here?
There was a problem hiding this comment.
Hmm yeah I get what you mean, but if we're getting strange feerates from our estimator then maybe we'd prefer to panic instead of silently defaulting? Although, I do think we need some sort of panic message at the least which is just an extra arg here. 🤷♂️
Would also be keen to know if this is what we normally want with these kinds of things in LDK.
There was a problem hiding this comment.
I add the same thought if we should assert or panic. Not sure if we have yet consistent defensive programming recommendations.
TheBlueMatt
left a comment
There was a problem hiding this comment.
I'm not a huge fan of asserting on these, I agree its nice to check the users' response, but if we're gonna add code around all the fee estimation fetches, we should just do the cmp::max ourself IMO.
dunxen
commented
Jun 21, 2022
Cool, that sounds good. Happy to do this. Do we then just get rid of the trait comments about bothering with |
TheBlueMatt
commented
Jun 21, 2022
I think we should leave it on the trait, or at least mention that we'll always use at least 253. |
ariard
commented
Jun 21, 2022
Personally, I would prefer if we still wrap that check around |
TheBlueMatt
commented
Jun 21, 2022
I'm not sure I undersood - are you agreeing that you'd rather see a wrapper of the FeeEstimator that does the max for us? |
ariard
commented
Jun 21, 2022
Yes, if that what you suggested as a could-be direction with your previous comment :) ? |
TheBlueMatt
commented
Jun 22, 2022
Heh, yea, sorry, that wasn't clear, yes, I was suggesting we just wrap the trait in some helper that does the max. |
b992a6a to
5922330CompareCodecov Report
@@ Coverage Diff @@## main #1552 +/- ##
==========================================
+ Coverage 90.81% 91.01% +0.19%
==========================================
Files 80 80 Lines 44534 45507 +973 Branches 44534 45507 +973 ==========================================
+ Hits 40445 41417 +972 - Misses 4089 4090 +1
Continue to review full report at Codecov.
|
| /// | ||
| /// This method can be implemented with the following unit conversions: | ||
| /// * max(satoshis-per-byte * 250, 253) | ||
| /// * max(satoshis-per-kbyte / 4, 253) |
There was a problem hiding this comment.
The unit conversions are still really useful.
| F::Target: FeeEstimator, | ||
| { | ||
| pub(crate) fn new(fee_estimator: &'a F) -> Self | ||
| where |
There was a problem hiding this comment.
You shoul dbe able to drop the where clause here - its redundant.
| /// Wraps a `FeeEstimator` so that any fee estimations provided by it | ||
| /// are bounded below by `FEERATE_FLOOR_SATS_PER_KW` (253 sats/KW) | ||
| pub(crate) struct LowerBoundedFeeEstimator<'a, F: Deref>(pub(crate) &'a F) |
There was a problem hiding this comment.
Instead of storing a reference to a deref to a feeestimator and constructing the lowerboundedestimator whenever we need to get fees, lets just store the full deref to a feeestimator here, and then update places in the crate to store a LowerBoundedFeeEstimator, so its always that way and its a bit harder to forget to do the wrapping.
| } | ||
| } | ||
| impl<F: Deref> FeeEstimator for LowerBoundedFeeEstimator<'_, F> |
There was a problem hiding this comment.
Lets not bother with the trait and just implement directly, then in the codebase make things take a LowerBoundFeeEstimator.
There was a problem hiding this comment.
Yeah, will make things simpler and then there's no forgetting to create a LowerBoundFeeEstimator as you mentioned :)
| /// Minimum relay fee as required by bitcoin network mempool policy. | ||
| pub const MIN_RELAY_FEE_SAT_PER_1000_WEIGHT: u64 = 4000; | ||
| /// Minimum feerate that takes a sane approach to rounding | ||
| pub const FEERATE_FLOOR_SATS_PER_KW: u32 = 253; |
There was a problem hiding this comment.
"Minimum feerate that takes a sane approach to bitcoind weight-to-vbytes rounding"
I don't know if this subtlety is documented anywhere well, so feel free to ref Rusty's commit : ElementsProject/lightning@2e687b9
4093743 to
1956024Compare| /// (ie 1 satoshi-per-byte rounded up to ensure later round-downs don't put us below 1 satoshi-per-byte). | ||
| /// | ||
| /// This method can be implemented with the following unit conversions: | ||
| /// This wrapped method will be implemented with the following unit conversions: |
There was a problem hiding this comment.
The point of listing the conversions is the user may need to convert from whatever unit their backend feerate API returns to the units here. Thus, its not that it "will" be implemented with the conversions, but that users "can" implement it using the listed conversions (but drop the max/253 bit) if they want.
There was a problem hiding this comment.
Oh yeah, of course. I didn't even realise that was the main point of the comment here. I had just considered the max part. Will fix.
| updates: &ChannelMonitorUpdate, | ||
| broadcaster: &B, | ||
| fee_estimator: &F, | ||
| fee_estimator: &LowerBoundedFeeEstimator<F>, |
There was a problem hiding this comment.
This method is public - let's drop the wrapper from the parameter.
There was a problem hiding this comment.
Seems we run into a few tricky issues here as we cannot clone a Deref to a FeeEstimator so that we can wrap it further down the line. Unless I'm missing something subtle and rusty. The main problem is we take in &F here and in some other places and we can't move out of the shared reference when we need to wrap.
There was a problem hiding this comment.
Uhhhh, right, uhhhh, uhhhh, so I think it'll work if we impl <D: Deref> FeeEstimator for D where D::Target: FeeEstimator. It doesn't currently break anything in test, dont think it'll break downstream stuff.
There was a problem hiding this comment.
Alright, let me go that direction and see :)
There was a problem hiding this comment.
In general I think we need to go that way for all of our traits, but that's a rather large change and dealing with the bindings for it sounds....un-fun.
| /// Wraps a `Deref` to a `FeeEstimator` so that any fee estimations provided by it | ||
| /// are bounded below by `FEERATE_FLOOR_SATS_PER_KW` (253 sats/KW) | ||
| pub struct LowerBoundedFeeEstimator<F: Deref>(pub F) |
There was a problem hiding this comment.
I don't think we want this to be public at all - it should be pub(crate) instead, I think.
| /// are bounded below by `FEERATE_FLOOR_SATS_PER_KW` (253 sats/KW) | ||
| pub struct LowerBoundedFeeEstimator<F: Deref>(pub F) | ||
| where | ||
| F::Target: FeeEstimator; |
There was a problem hiding this comment.
nit: does this really need its own line?
There was a problem hiding this comment.
Think rustfmt got me here 😅. Will fix.
dunxen
commented
Jul 4, 2022
Rebasing and directly modifying commits just this one last time before more review as a lot has changed from the previous revision. |
1956024 to
98ae8bfCompareTheBlueMatt
commented
Jul 5, 2022
Let me know when you're ready for more review here. |
7179109 to
9ec469fCompare| /// | ||
| /// This method can be implemented with the following unit conversions: | ||
| /// The following unit conversions can be used to convert to sats/KW. Note that it is not | ||
| /// necessary to use max() as the minimum of 253 will be enforced by LDK: |
There was a problem hiding this comment.
nit: then why even mention the max?
There was a problem hiding this comment.
True story. Remove this and just leave in the max in the example conversion or also get rid of that?
There was a problem hiding this comment.
I'd think just skip the max but leave the conversions.
| } | ||
| for (_, request) in bump_candidates.iter_mut() { | ||
| if let Some((new_timer, new_feerate, bump_tx)) = self.generate_claim_tx(height, &request, &&*fee_estimator, &&*logger) { | ||
| if let Some((new_timer, new_feerate, bump_tx)) = self.generate_claim_tx(height, &request, &fee_estimator, &&*logger) { |
There was a problem hiding this comment.
nit: you can drop the & on the fee_estimator entirely. Fewer &s good - we're already calling a fee estimator that is a reference to a reference to a reference :(
There was a problem hiding this comment.
Yeah seems like quite the onion 😅
| let mut should_persist = NotifyOption::SkipPersist; | ||
| let new_feerate = self.fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal); | ||
| let new_feerate = LowerBoundedFeeEstimator::new(&self.fee_estimator).get_est_sat_per_1000_weight(ConfirmationTarget::Normal); |
There was a problem hiding this comment.
Instead of peppering these everywhere, the ChannelManager::fee_estimator field should be a LowerBoundedFeeEstimator, then its very hard to screw up and forget to wrap.
There was a problem hiding this comment.
Hmm, ok so no need to worry about serialisation here? Field is completely ignored in public contract?
There was a problem hiding this comment.
The fee estimator itself? No, its not serialized, the user passes in a new fee estimator to the ChannelManager deserialize method.
| return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id)); | ||
| } | ||
| let (closing_signed, tx) = try_chan_entry!(self, chan_entry.get_mut().closing_signed(&self.fee_estimator, &msg), channel_state, chan_entry); | ||
| let (closing_signed, tx) = try_chan_entry!(self, chan_entry.get_mut().closing_signed(&&self.fee_estimator, &msg), channel_state, chan_entry); |
There was a problem hiding this comment.
You should be able to drop a & here?
There was a problem hiding this comment.
Hmm yeah I think I can now with how FeeEstimator currently looks.
There was a problem hiding this comment.
So yeah we don't need it in this case, but if we make ChannelManager::fee_estimator a LowerBoundedFeeEstimator, then it seems necessary as we're working with something that's not a Deref to a FeeEstimator.
There was a problem hiding this comment.
Right, you'll need a single & here, which is a reference to a LowerBoundedFeeEstimator which holds a Deref to a FeeEstimator. Its still two references, but shouldn't need to touch this line of code, I belive?
There was a problem hiding this comment.
I think the main issue here is that the signature expects &F for first argument and F: Deref where F::Target: FeeEstimator.
So it's a ref to a deref to a FeeEstimator, so that's a bit of a problem here :/
Errors with just a single & in the case where ChannelManager::fee_estimator is a LowerBoundedFeeEstimator, because of the signature.
There was a problem hiding this comment.
Right, but I thought we were moving to channel.rs only ever seeing LowerBoundedFeeEstimators, never a FeeEstimator itself.
There was a problem hiding this comment.
Sorry, thought that was a public API but seems like Channel itself is not. So I can go ahead and adjust the signature.
There was a problem hiding this comment.
Ah! okay, wasn't sure what the confusion was. Thanks.
| } | ||
| let feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal); | ||
| let feerate = LowerBoundedFeeEstimator::new(fee_estimator).get_est_sat_per_1000_weight(ConfirmationTarget::Normal); |
There was a problem hiding this comment.
We don't need to wrap the estimator in channel.rs anymore, no? Plus a few places further down.
TheBlueMatt
left a comment
There was a problem hiding this comment.
LGTM. Feel free to squash and then let's land this!
| if !self.is_outbound() { | ||
| if let Some(msg) = &self.pending_counterparty_closing_signed.take() { | ||
| return self.closing_signed(fee_estimator, &msg); | ||
| return self.closing_signed(&fee_estimator, &msg); |
There was a problem hiding this comment.
You shouldnt need to touch this line now.
`LowerBoundedFeeEstimator` is a wrapper for `Deref`s to `FeeEstimator`s that limits the get_est_sat_per_1000_weight() method to no less than 253 sats/kW.
fb0a015 to
7bc6d0eCompare
Fixes#1016