feat: get last bitcoin fees from sweep tables (#809) #810
Conversation
| /// This test asserts that the `get_btc_state` function returns the correct | ||
| /// `SignerBtcState` when there are no sweep transactions available, i.e. | ||
| /// the `last_fees` field should be `None`. | ||
| #[cfg_attr(not(feature = "integration-tests"), ignore)] |
There was a problem hiding this comment.
Does this test run in CI? I would think that it doesn't, but I'm not sure. If not, then I think there are others that we need to move. And I've made this mistaken in some tests as well 😞.
There was a problem hiding this comment.
@djordon hmm, I'm not sure of the best approach. Here we're testing get_btc_state() which is private, so it needs to be here, but otoh it also needs postgres, so it needs to be run as an integration-test. Should I just make it public and move the test, or do you have a better suggestion?
There was a problem hiding this comment.
Moved the tests in 2b22025 and verified they run in CI
|
I decided to go and check myself and I didn't find those tests in the CI output logs. Also, we should have CI explicitly output which tests were ignored. |
Yeah I'd make it public. |
| let total: u64 = self | ||
| .iter() | ||
| .map(|tx| tx.fee) | ||
| .try_fold(0u64, |acc, fee| acc.checked_add(fee)) | ||
| .ok_or(Error::ArithmeticOverflow)?; |
There was a problem hiding this comment.
Hmmm, these fees cannot add up to anything more than u64::MAX, even if we added up all bitcoins (measured in sats).
There was a problem hiding this comment.
Nope, but the code otherwise is technically not panic-safe, which is what we've been striving for.
Same argument could be made for the vsize-- we should realistically never have transactions larger than a handful of vKb, and it would take a crazy number (u32::MAX+1) of transactions holding u32::MAX, but the code can still panic.
| if total_size == 0 { | ||
| return Err(Error::DivideByZero); | ||
| } |
There was a problem hiding this comment.
This can only happen if the vector is empty, so we could just return zero.
There was a problem hiding this comment.
I don't think zero is what we want -- compute_transaction_fee() matches on an Option<> and None would not be the same as Fees::ZERO.
But I changed the signature and added a check and return Ok(None) if it's empty as I could describe that mode in the trait description and it matches its use (basis for calculation is missing). We did this check manually before in the coordinator, so it's essentially just moved here.
The DivideByZero remains because you could still have a bug elsewhere i.e. when pulling these out of the database which defaults vsize to zero (i.e. the code still has a panic-able path otherwise).
|
@matteojug yeah, crap, those generated files snuck in on me. |
Description
Closes: #809
Changes
Removes the
BitcoinInteract::get_last_fees()method (described in #541) and replaces it with logic to retrieve theFeesfrom the latest sweep transaction package (described in #809).Testing Information
RBF tests updated to use the
sweep_transactionsmethod.Checklist: