Fix fee calculations #8236
Fix fee calculations #8236
Conversation
rustyrussell
force-pushed
the
fix-fee-calculations
branch
from
9883f24 to
9f96cba
Compare
| @@ -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.
rustyrussell
force-pushed
the
fix-fee-calculations
branch
2 times, most recently
from
37288ab to
583b32c
Compare
|
I just spoke with the VLS team, and the HSMD wire change will not work as is: they generate the decoder from Rust structs, which are very close to what I described during our call as the protobuf "ideal", however it also means that since these composite messages are essentially composed of smaller internal messages, there is no place to attach any logic, such as "in version A do this, in version B do this", not to speak that they don't hand in the negotiated version as context to the deserializer. That means that VLS can effectively not differentiate between CLN v.25.02 and CLN v25.05 messages. We either need to:
|
|
Why does the UTxO struct need to be changed at all? It already includes the scriptPubKey, which can be directly examined to determine its type, as I did in #8234. Does changing the struct provide any additional information that cannot be inferred from the scriptPubKey? |
Because using an enum is clearer. When we add another spend type, all the case statements will give compile errors for not handling it. Not doing this in the first place is the root problem, which is why we didn't automatically catch all the places which should have been updated for taproot. |
rustyrussell
force-pushed
the
fix-fee-calculations
branch
3 times, most recently
from
3d5d1ba to
6e6faaf
Compare
We're about to fix the feerate calculations in various places, and one side effect is that we end up trying to add an empty anchor if none is necessary (and failing, but we log a nasty message about it). So don't do that, and fix the test which expected it. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Not just the key index. Also, remove FIXME: wallet_can_spend is no longer slow with lots of inputs! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's NULL, but the case covered up that it's the wrong type! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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. I tested this hadn't accidentally changed the wire format by disabling version checks and using an old hsmd with the altered daemons and running the test suite. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is such a simple struct that we can actually define it in csv. This prevents us from accidentally breaking the ABI in future. I tested this hadn't accidentally changed the wire format by disabling version checks and using an old hsmd with the altered daemons and running the test suite. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…allocate. The renaming makes it clear that it's HSM specific. And it has no pointers, so we can have an array instead of an array of pointers. I tested this hadn't accidentally changed the wire format by disabling version checks and using an old hsmd with the altered daemons and running the test suite. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
fix-fee-calculations
branch
from
6e6faaf to
98e283b
Compare
To actually evaluate spend cost, we need to know whether it's taproot or not. Using an enum (rather than making callers examine the script) means we can ensure all cases are handled. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
No code change, just following convention. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Due to a bug elsewhere I actually triggered this path, and it broadcast the tx anyway, *then* closed the channel. We should abandon the channel if we can, instead. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We previously treated it as a P2WPKH, which is wrong. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Fixed: wallet: fees are much closer to target feerate when doing txprepare/fundchannel.
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au> Changelog-Fixed: Protocol: anchors' fees are now much closer to the feerate targets.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
They're not quite right (more work needed), but disable those checks for now. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
fix-fee-calculations
branch
from
98e283b to
46d76db
Compare
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.
Importantly, this avoids any changes to the HSM ABI (thanks Christian!) and makes it harder to accidentally break that in future.
Closes: #8164