Fix fee calculations #8236
Open
Fix fee calculations #8236
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustyrussell
force-pushed
the
fix-fee-calculations
branch
from
9883f24 to
9f96cba
Compare
cdecker
reviewed
Apr 15, 2025
| @@ -36,7 +36,7 @@ struct utxo *fromwire_utxo(const tal_t *ctx, const u8 **ptr, size_t *max) | |||
| fromwire_bitcoin_outpoint(ptr, max, &utxo->outpoint); | |||
| utxo->amount = fromwire_amount_sat(ptr, max); | |||
| utxo->keyindex = fromwire_u32(ptr, max); | |||
| utxo->is_p2sh = fromwire_bool(ptr, max); | |||
There was a problem hiding this comment.
This might upset VLS in a rather sneaky way: we monitor hsmd_wire.csv to get notified if any messages change. By changing the fromwire and towire implementations that the generated code uses the message changes but no indication is left of it. Can we either create a new type or append a new field instead?
There was a problem hiding this comment.
Good point. I'll make a note and preserve the wire format!
There was a problem hiding this comment.
Once we get rid of the old "closing info" UTXOs, we can actually ensure all the info is in the PSBT, and never send the utxos anyway.
Not just the key index. Also, wallet_can_spend is no longer slow with lots of inputs! Signed-off-by: Rusty Russell <[email protected]>
I'm about to update our utxo type, but Christian spotted that this is part of the ABI for the hsm. So make that a private "hsm_utxo" type, to insulate it from changes. In particular, the HSM versions only contain the fields that the hsm cares about, and the wire format is consistent (even though that *did* include some of those fields, they are now dummies). In the long term, this should be removed from the ABI: once we no longer have "close_info" utxos, this information should already be in the PSBT. Signed-off-by: Rusty Russell <[email protected]>
To actually evaluate spend cost, we need to know whether it's taproot or not. Signed-off-by: Rusty Russell <[email protected]>
No code change, just following convention. Signed-off-by: Rusty Russell <[email protected]>
We previously treated it as a P2WPKH, which is wrong. Signed-off-by: Rusty Russell <[email protected]>
We use this for anchors, in which case we have a minimum value for change. If we don't take this into account, we end up with a lower feerate once we actually create the tx. Signed-off-by: Rusty Russell <[email protected]>
We didn't add the weight of the two sigs! The BOLT defines that to be a worst-case 73 byte sig, but that turns out to be an overestimate (and this is not required for consensus) so we assume everyone grinds. Signed-off-by: Rusty Russell <[email protected]>
We need one byte for the number of witness elements. Some callers added it themselves, but it's always needed. So document and fix the callers. Signed-off-by: Rusty Russell <[email protected]>
Now we've fixed various underlying weight mis-estimation, we can do a few final tweaks and test that anchors work as intended. 1. We assumed a p2wpkh output, but modern change is p2tr. 2. We handed `2` instead of `1` to bitcoin_tx_core_weight (which doesn't matter, but was weird). 3. Make change calculation clearer. I'm not sure the previous one was wrong, but it was harder to understand. 4. Fix the test and make it clearly test that we are aiming for (and achieving) the right feerate. Signed-off-by: Rusty Russell <[email protected]> Changelog-Fixed: Protocol: anchors' fees are now much closer to the feerate targets.
Signed-off-by: Rusty Russell <[email protected]>
This is legal! And we actually do this in tests, but we didn't check the psbt
was spendable (the next patch does, indirectly, by testing feerate):
```
# Discard prep4 and get all funds again
l1.rpc.txdiscard(prep5['txid'])
# You can have one which is all, but not two.
prep5 = l1.rpc.txprepare([{addr: Millisatoshi(amount * 3 * 1000)},
{addr: 'all'}])
# Feerate should be ~ as we asked for
> assert normal_feerate_perkw - 1 < feerate_from_psbt(bitcoind, l1, prep5['psbt']) < normal_feerate_perkw + 1
E AssertionError: assert (7500 - 1) < -1091803.9574935874
```
Changelog-Fixed: JSON-RPC: `txprepare` with `all` as well as one or more non-all amount fields now produces a valid PSBT.
Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
rustyrussell
force-pushed
the
fix-fee-calculations
branch
from
9f96cba to
37288ab
Compare
…on't fail. One in 256 times, we will grind a signature to 70 bytes (or shorter). This breaks our feerate tests. Unfortunately the grinding is deterministic, so there doesn't seem to be a way to avoid it. So we add a log message, and then we skip the feerate test if it happens. Signed-off-by: Rusty Russell <[email protected]>
This is inspired by a patch from @whitslack, which overlapped with this series. Most importantly, there was only one call to bitcoin_tx_simple_input_weight(), and it is better to be explicit with that one. This also changes our funder calculation to assume our own input is taproot, which it is likely to be given we've defaulted to taproot for outputs for change addresses since 23.08. Signed-off-by: Rusty Russell <[email protected]>
rustyrussell
force-pushed
the
fix-fee-calculations
branch
from
37288ab to
583b32c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a superset of @whitslack's #8234 which fixes all the weight calculations and adds tests. His PR reminded me to finish this off, since it was sitting to the side while we worked on the point release.
Closes: #8164