Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not overpay funding transaction fees by 2x #2032

Merged
merged 3 commits into from
Feb 14, 2024
Merged

Conversation

luckysori
Copy link
Contributor

Fixes #2022.

3348fd7 adds 3 patches to our rust-dlc branch1:

Only the last one is relevant for this PR, but the other ones are probably good too.

Footnotes

  1. Atm the changes live in a separate branch: https://github.com/p2pderivatives/rust-dlc/commits/feature/ln-dlc-channels-10101-with-sign-psbt-fee-fix/.

@luckysori luckysori requested review from bonomat and holzeis February 14, 2024 09:55
@luckysori luckysori self-assigned this Feb 14, 2024
@luckysori luckysori changed the title Fix/rust dlc fees Do not overpay funding transaction fess by 2x Feb 14, 2024
@luckysori luckysori changed the title Do not overpay funding transaction fess by 2x Do not overpay funding transaction fees by 2x Feb 14, 2024
@luckysori luckysori enabled auto-merge February 14, 2024 10:09
Arc<Node<TenTenOneInMemoryStorage, InMemoryStore>>,
#[tokio::test(flavor = "multi_thread")]
#[ignore]
async fn can_open_channel_with_min_inputs() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move this test to rust-dlc eventually, but our tests are easier to write for me.

Comment on lines +265 to +268
let expected_fund_tx_fee = 252 * fee_rate_sats_per_vbyte;

// This also depends on the fee rate, but the formula is a bit more involved.
let fee_reserve = 880;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can work on using constants defined by rust-dlc, to make all this clearer and less flaky on rust-dlc upgrades. But I think this suffices for now.

Copy link
Contributor

@holzeis holzeis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice 👍

I think that deserves a CHANGELOG entry ;)

@luckysori luckysori added this pull request to the merge queue Feb 14, 2024
@luckysori luckysori removed this pull request from the merge queue due to a manual request Feb 14, 2024
Once the funding transaction is constructed, `rust-dlc` allocates more
fees than are needed to pay for the funding transaction.
@luckysori luckysori added this pull request to the merge queue Feb 14, 2024
Merged via the queue into main with commit c92ed93 Feb 14, 2024
19 of 20 checks passed
@luckysori luckysori deleted the fix/rust-dlc-fees branch February 14, 2024 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fixing the fee calculation in rust-dlc
2 participants