Add Fee Estimation in ChannelMonitor #334
Conversation
ariard
force-pushed
the
2019-04-fee-estimation-monitor
branch
from
c472359 to
3937747
Compare
src/ln/channelmonitor.rs
Outdated
| @@ -475,6 +490,32 @@ impl ChannelMonitor { | |||
| } | |||
| } | |||
|
|
|||
| 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
ariard
force-pushed
the
2019-04-fee-estimation-monitor
branch
3 times, most recently
from
ef7b9fd to
b28825e
Compare
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
| @@ -1165,6 +1209,7 @@ impl ChannelMonitor { | |||
| 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
| @@ -1370,16 +1417,18 @@ impl ChannelMonitor { | |||
| 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
| @@ -475,6 +486,36 @@ impl ChannelMonitor { | |||
| } | |||
| } | |||
|
|
|||
| 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....
ariard
force-pushed
the
2019-04-fee-estimation-monitor
branch
from
b28825e to
d0b3aad
Compare
|
Just took the assertion commit, spendable output will have to move after claim tx confirmation delay so will fix it here. |
ariard
force-pushed
the
2019-04-fee-estimation-monitor
branch
from
d0b3aad to
1c7ba90
Compare
May go before or after #305 but should go before #333