Add Fee Estimation in ChannelMonitor#334
Conversation
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Awesome! I'm surprised this didn't have follow-on requirements in the functional tests (which probably means we should expand our functional tests, but hey, at least this patch is easy).
src/ln/channelmonitor.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn get_claim_tx_weight(inputs: Vec<InputDescriptors>, output_scriptpubkey: &Script) -> u64 { |
There was a problem hiding this comment.
Isn't this missing a *4 somewhere? Weight is 4x the size for non-witness bits, but less for the witness bits. It would be nice to just commit a test that generates some transactions and checks this function.
There was a problem hiding this comment.
Damn right this definitely need test! I'm gonna to add a unit one for get_claim_tx_weight on this PR, and was planning to extend functional ones for bump txn thing IMO it gonna to need serious testing
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Probably take the testing commit from https://github.com/TheBlueMatt/rust-lightning/tree/2019-04-334-review and make sure it works afterwards.
src/ln/channelmonitor.rs
Outdated
| value: htlc.amount_msat / 1000, //TODO: - fee | ||
| }), | ||
| }; | ||
| single_htlc_tx.output[0].value -= fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::HighPriority) * (single_htlc_tx.get_weight() + Self::get_witnesses_weight(&vec![if htlc.offered { InputDescriptors::RevokedOfferedHTLC } else { InputDescriptors::RevokedReceivedHTLC }])) / 1000; |
There was a problem hiding this comment.
Should drop the TODO: - fee two lines up.
There was a problem hiding this comment.
Gah, unrelated to this PR but isnt there a missing spendable_outputs.push() two lines below here?
There was a problem hiding this comment.
Seems you're right, but rightly likely they should move from place after #333 so added a note to todo it there
| inputs.push(input); | ||
| values.push((tx.output[transaction_output_index as usize].value, payment_preimage)); | ||
| total_value += htlc.amount_msat / 1000; | ||
| input_descriptors.push(if htlc.offered { InputDescriptors::OfferedHTLC } else { InputDescriptors::ReceivedHTLC }); |
There was a problem hiding this comment.
I think this only makes sense for OfferedHTLCs (ie HTLCs to us). See, however, #337.
There was a problem hiding this comment.
Agree our current is likely boggus, will test it but you may tag it as "good first issue" IMO it's pretty straightforward to test
There was a problem hiding this comment.
But as code branches are, for now and only if it's temporary, seems to me that makes sense for both. You may claim an offered HTLC with preimage tx before remote timeout it with its HTLC-timeout and you may claim a received HTLC with timeout tx before remote get it with its HTLC-success.
src/ln/channelmonitor.rs
Outdated
| } | ||
|
|
||
| fn get_witnesses_weight(inputs: &Vec<InputDescriptors>) -> u64 { | ||
| let mut tx_weight = 0; |
There was a problem hiding this comment.
Doesn't this need to start with 2 for the flags bytes?
There was a problem hiding this comment.
Aaaah in fact I was adding them in test to get the right count but it didn't trigger me that it wasn't normal and that I should get them from get_witnesses_weight....
|
Just took the assertion commit, spendable output will have to move after claim tx confirmation delay so will fix it here. |
2 participants
May go before or after #305 but should go before #333