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

Allow to force the selection of utxos #4

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

louneskmt
Copy link

I added a forceSelection property to the utxos argument of coinselect and the algorithms addUntilReach and avoidChange. This can be useful if we want to use some base UTXOs and if necessary complete with a selection (e.g. when spending an anchor output). The changes aren't significant, we just need to check before the loop if the forced UTXOs already cover the targets value + fee, in that case we return directly. That duplicates a bit of code so let me know if there is a better way.

Tests are passing, I added some to cover this new behavior.

Another thing that could be useful in the case of an anchor output spending scenario is to be able to have an empty targets array. In that case, it should just select UTXOs to pay for the fees without any precise targets value. This may be in a follow-up PR.

@landabaso
Copy link
Member

landabaso commented Jan 5, 2024

This proposal to integrate a forceSelection feature would add valuable functionality to the library.
I am concerned about the potential complexity that adding specific use cases might introduce to the API.

For instance, in a scenario I am interested in, I have time-unlocked UTXOs that I prefer to be spent first (prioritized) but without the necessity to force spend all of them. A preferSelect flag would be beneficial here. Regarding your use case, it may be interesting to consider the value contribution of each UTXO in high-fee situations. Sometimes it may not be economically worth to unstick a tx. Your current proposal does not address this, possibly because in your use case, other UTXOs in the stuck transaction that are not passed to the algo provide a lot of value. However, to consider to scenarios where the value contribution is important, introducing another forceSelectionIfContributes flag could be a solution.

While your use case is valid and necessay, I am considering ways to implement it without overcomplicating the codebase as more use cases appear (see the 3 possible flags just as an example). I suggest a focused approach on each use case by designing separate algos.

Implementing CPFP would require further modifications and additional code anyway if I am not wrong. It requires calculating the effective fee rate for the entire transaction chain, involving the vsize and fee of previous transactions. This would mean augmenting the utxos array with more data beyond forceSelection.

Even with your proposal, determining the right fee rate to achieve the desired effective fee rate presents a challenge, as it depends on the utxos that will be selected, which isn't predetermined.

While I was thinking in your proposal, I conceptualized a possible implementation for CPFP and, the way I see it, I would need having the txId, vsize and fee of the parent txs anyway and all the algos would require even more changes:

export function CPFP({
  utxos,
  parentUtxos,
  targets,
  remainder,
  effectiveFeeRate
}: {
  /** utxos of regular confirmed txs - pass them in the order that you prefer
   * being added */
  utxos: Array<OutputWithValue>;
  /** utxos corresponding to unconfirmed txs.
   * txId is passed to cluster utxos which belong to the same unconfirmed tx.
   */
  parentUtxos: Array<
    OutputWithValue & { vsize: number; fee: number; txId: string }
  >;
  /** targets can be an empty array */
  targets: Array<OutputWithValue>;
  remainder: OutputInstance;
  /** the effective fee rate is the desired ratio between the sum of all the fees
   * involved /  the sum of the sizes of all the the txs involved
   * including the unconfirmed txs and the new tx that will be created
   */
  effectiveFeeRate: number;
}) {
  //selectedUtxos = parentUtxos
  //Compute the totalVsize of parent unconfirmed txs (only one per unconfirmed txId, not per parentUtxo)
  //Compute the totalFee of parent txs (one per unconfirmed txId, not per parentUtxo)
  //totalFee = f(totalFee, feeForTargets /*if any*/)
  //totalVsize += vsizeOfNewTxWithParentUtxosAndTargets (targets, if any)
  //totalVsize += vsizeIncreaseForRemainder (if necessary)

  //while (totalFee / totalVSize < effectiveFeeRate)
  //  pick a candidateUtxo
  //  selectedUtxos.push(candidateUtxo)
  //  totalFee = f(totalFee, feeForCandidateUtxo)
  //  totalFee = f(totalFee, feeForRemained /*if necessary*/)
  //  totalVsize += vsizeIncreaseForCandidateUtxo
  //  totalFee += vsizeIncreaseForRemainer (if necessary)
  //Return selectedUtxos
}

This approach focuses on selecting UTXOs based on the effective fee rate while considering the total size and fee of the parent transactions.

Do you think a more focused solution would better meet your needs? I'm not outright rejecting the idea of implementing CPFP, but I am considering how to add this feature without overly complicating the codebase as we address increasing use cases.

@louneskmt
Copy link
Author

Using this PR, I've managed to do a CPFP: get my wallet UTXOs, set forceSelection on an unconfirmed UTXO from the transaction I want to bump and add it to the utxos array, use a dummy wpkh descriptor as a target with a small value (1000 sats) and use an actual descriptor for the change. I calculated the fee rate to pass to coinselect as actualFeeRate = 2 * targetFeeRate - previousFeeRate (where target fee rate is the effective fee rate I want and previous fee rate the one of the transaction to bump). Then I remove the dummy output from the result and add back actualFeeRate * dummyOutput.outputWeight() / 4 to the change. (see code below)

I don't think implementing CPFP in this library makes much sense as most of the calculation etc can be done before / after the coinselection and while building the transaction. What's only needed for this to work well is to be able to properly select UTXOs to pay for fees while force selecting some UTXOs. It feels like if we start adding algos for everything like CPFP, we'll get too many quite quickly. So, even if that complicates a bit the codebase, maybe we can come up with meaningful flags that can be useful in various situations, without having to write distinctive algos?

Btw, for my example above, allowing the targets to be empty would save some steps (adding a dummy output then removing it and adding back the sats). The thing is that all the algos for now are based on the target value to reach. Maybe we could say that if the targets are empty, then the target value should be the fees to pay, and that would change based on the utxosSoFar? Not sure if you are following me, and this could be moved to a different issue.

    const targetFeeRate = await getFeeRate()
    const actualFeeRate = 2 * targetFeeRate - previousFeeRate

    // selectUtxos is just a wrapper that converts descriptor string to Output and sums the selected utxos amount to inAmount
    const selectionResult = selectUtxos(config)({
      utxos: [
        {
          descriptor: anchorDescriptor,
          value: transaction.outs[anchorOutputIndex]!.value,
          txHex: transaction.toHex(),
          vout: anchorOutputIndex,
          utxo: `${transaction.getId()}:${anchorOutputIndex}`,
          forceSelection: true,
        },
        ...spendableUtxos,
      ],
      targets: [
        {
          descriptor: dummyDescriptor('wpkh'),
          value: 1000,
        },
      ],
      remainderDescriptor: cpfpChangeDescriptor,
      feeRate: actualFeeRate,
    })

    const cpfpChangeOutput = new Output({
      // For now, we send back to the anchor wallet change. Can be changed.
      descriptor: cpfpChangeDescriptor,
      network: config.network,
    })

    const psbt = new Psbt({ network: config.network })
    const finalizers: Array<
      ReturnType<InstanceType<typeof Output>['updatePsbtAsInput']>
    > = []

    for (const utxo of selectionResult.utxos) {
      finalizers.push(
        utxo.output.updatePsbtAsInput({
          psbt,
          txHex: utxo.txHex,
          vout: utxo.vout,
        })
      )
    }

    cpfpChangeOutput.updatePsbtAsOutput({
      psbt,
      value:
        selectionResult.inAmount -
        selectionResult.fee +
        // We calculated the fee for two outputs but we keep only a single change so we need to add back the fee for one output
        (actualFeeRate * cpfpChangeOutput.outputWeight()) / 4,
    })

@landabaso
Copy link
Member

landabaso commented Jan 6, 2024

I calculated the fee rate to pass to coinselect as actualFeeRate = 2 * targetFeeRate - previousFeeRate (where target fee rate is the effective fee rate I want and previous fee rate the one of the transaction to bump)

This point is exactly what I was trying to address in my last message. I spent quite some time re-reading your initial issue, reviewing your PR, and conceptualizing a possible implementation for CPFP that might suit your needs.
I believe the method of CPFP you propose could lead to issues, and I would advise against it.

Imagine you need to unstick a previous transaction that was a large multi-sig or had a very large witness. For instance, a parent transaction of 4000 bytes stuck at 10 sats/b, and you need to bump it to 100 sats/b. According to your approach, you would choose your utxos based on this fee rate: 2 * 100 - 10 = 190 sats/vb. Now, suppose the child transaction you create to unstick the parent is about 210 bytes with 2 inputs (1 for the stuck utxo and 1 new input) and 1 output. The effective fee rate calculated by miners would be: totalFee / totalSize = (4000 * 10 + 190 * 210) / 4210 = 19 sats/vb. As you can see, the real effectiveFeeRate (19 sat/b and NOT 100 sat/b) would not be sufficient to unstick the parent transaction.

In your particular use case, your proposal might work because the sizes of the parent and child transactions are similar. However, when implementing a feature like this, it's essential to consider a broader range of scenarios, ensuring that all cases are adequately covered.

In essence, for creating a CPFP tx it's necessary to consider the parent transaction's size and fee for each utxo you need to unstick, ensuring it's done once per parent transaction (not per utxo). Hence, a solution like the one I proposed might be more appropriate.

I don't think implementing CPFP in this library makes much sense as most of the calculation etc can be done before / after the coinselection

I still believe that a properly implemented CPFP requires a more focused approach, involving more flags or optional pareams in the coinselect algorithms beyond forceSelect, such as the vsize, fee, and txId of the parent tx for each utxo you wish to "forceSelect".

My concern is that as more use cases arise, we might end up adding an increasing number of flags and optional parameters to each utxo, leading to a complex codebase filled with if-else statements.

Please take some time to carefully review my proposed solution and assess whether it meets your needs. I'm open to the possibility that I might have missed some aspects, and I'm more than willing to discuss this further if you'd like.

@louneskmt
Copy link
Author

Hey! Sorry for the long delay, I was working on something else and kinda forgot to answer.

You're right, my solution seems to be working in my specific case but really is not generic enough. Thanks for the explanation.

If you think that a separate method for CPFP makes sense, I can give a shot at implementing it based on what you proposed.

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.

2 participants