Add cpfp and change index getonchaintransactions #386
Conversation
edouardparis
force-pushed
the
add-cpfp-and-change-index-getonchaintransactions
branch
2 times, most recently
from
2c033a2 to
33c5078
Compare
|
Needs rebase :) |
edouardparis
force-pushed
the
add-cpfp-and-change-index-getonchaintransactions
branch
from
33c5078 to
379f959
Compare
Raw hex transactions give not enough information about the UTXO recipients. We introduce in this commit two more fields to the list items: change_index and cpfp_index, respectively the indexes of the change output and cpfp output in a transaction. A cpfp output is an UTXO with a script pubkey derived from the CPFP descriptor. A change output is an UTXO with a script pubkey derived from the Deposit descriptor.
edouardparis
force-pushed
the
add-cpfp-and-change-index-getonchaintransactions
branch
from
379f959 to
535ab9b
Compare
There was a problem hiding this comment.
The implementation looks good to me, and thanks for adding a unit test to the not-unit-tested listonchaintransactions.
However, i don't think it is a good idea to expose a boutique change detection in listonchaintransactions.
Firstly, one can't find all the change outputs of a transaction with a reasonable degree of certitude short of checking all outputs' scriptPubKeys against your deposit descriptor up to the gap limit. It is obviously not reasonable. Short of doing that here we:
- Assume a change output will always reuse the same deposit address
- Only consider the last detected change output (because we reuse the hack from
listspendtxswhich is specific to the behaviour ofgetspendtx)
This will necessarily give wrong information to the user. Absence of such an information is obviously better.
Secondly, onchain transactions are necessarily past transactions. In order to detect change outputs from past transactions, we can just check if the txid of the given transaction is also the txid of a later deposit. It's expensive, but actually robust.
Good thing is: gethistory is doing just that. If one wants to know the amount that went back in for a given spending, they can just use that.
Lastly, i don't think that finding change outputs in historical transactions is a reasonable feature. It is not natural to check for change outputs without context, it's a limitation of how Bitcoin works. i think that an application relying on this should think twice about whatever they want to achieve as it sounds a lot like an anti-feature. (This holds for adding change outputs to the vault history view in the GUI, but really also for any other application that would need this).
For all these reasons, Concept NACK 535ab9b.
| /// Finds the cpfp and change output indexes of a spend transaction by deriving the scripts | ||
| /// from descriptors and checking them against outputs scripts. |
There was a problem hiding this comment.
I think we should be clear in the doc that:
- This is not guaranteed to find a CPFP or change output (esp. a change one), even if it exists
- It will only give the last change output if there are several ones
| let unvault = if [ | ||
| VaultStatus::Unvaulting, | ||
| VaultStatus::Unvaulted, | ||
| VaultStatus::Spending, | ||
| VaultStatus::Spent, | ||
| VaultStatus::Canceling, | ||
| VaultStatus::Canceled, | ||
| VaultStatus::UnvaultEmergencyVaulting, | ||
| VaultStatus::UnvaultEmergencyVaulted, | ||
| ] | ||
| .contains(&db_vault.status) |
There was a problem hiding this comment.
nit: matches!() would be more idiomatic here?
There was a problem hiding this comment.
Aside, we use this in multiple places so it's probably worth having it as a method of VaultStatus, like is_second_stage()
| None | ||
| } | ||
| }) | ||
| .expect("Unvault tx must have a cpfp_index"); |
There was a problem hiding this comment.
Not for long :)
revault/practical-revault#104
| let cancel = if db_vault.status == VaultStatus::Canceling | ||
| || db_vault.status == VaultStatus::Canceled |
There was a problem hiding this comment.
Same nit for matches!
| let cancel = if db_vault.status == VaultStatus::Canceling | |
| || db_vault.status == VaultStatus::Canceled | |
| let cancel = if matches!(db_vault.status, VaultStatus::Canceling | VaultStatus::Canceled) |
|
|
||
| // Some vaults have the same spend as their final transaction. | ||
| // It is better to cache it for performance. | ||
| let mut spend_tx_cache = BTreeMap::<Txid, OnchainTxEntry>::new(); |
| | `change_index` | int or `null` | Index of the change output, `null` if not present | | ||
| | `cpfp_index` | int or `null` | Index of the cpfp output, `null` if not present | |
There was a problem hiding this comment.
We should at the very least document all the pitfalls so consumers of the API don't rely on this. I'm wondering if it's worth exposing a pretty brittle and incorrect output detection at all..
Raw hex transactions give not enough information about
the UTXO recipients. We introduce in this commit two more fields
to the list items: change_index and cpfp_index, respectively
the indexes of the change output and cpfp output in a transaction.
A cpfp output is an UTXO with a script pubkey derived from the CPFP descriptor.
A change output is an UTXO with a script pubkey derived from the Deposit
descriptor.
It adds a test (in relation to #300)