bitcoinjs / coinselect

An unspent transaction output (UTXO) selection module for bitcoin.
MIT License
179 stars 101 forks source link

Reevaluating 'Dust' Considerations When Adding Change in Transactions #86

Open landabaso opened 9 months ago

landabaso commented 9 months ago

EDIT [Nov-19]: Upon further examination, it appears that Bitcoin Core indeed employs a similar approach. The main difference is that Core allows the user to set a custom dustRelayFee (default 3 sat/vbyte), which is independent of the feeRate. In contrast, the coinselect algorithm in this repository sets the dustRelayFee equal to the feeRate. This seems to be about optimizing blockchain efficiency, but I'm uncertain if the strategy of tying dustRelayFee directly to the current fee rates in this repo is the most effective approach. I'll keep this issue open for a while longer to welcome any additional feedback or insights.

This issue is raised for discussion, especially since I believe these algorithms are being utilized by various wallets (I am also working on similar ideas too).

I'd like to discuss a specific aspect of the coinselect algorithms, particularly concerning this line of code:

https://github.com/bitcoinjs/coinselect/blob/3c72703a0d28380c7565a4ccd7f9b66655d0d86c/utils.js#L50

From a network efficiency perspective, avoiding the addition of Waste to transactions is beneficial, making this a good decision for network optimization (block space used).

However, when focusing on the user's interests, it's generally advantageous to receive change back, provided it aligns with the designated fee rate. Doing the opposite results in additional fees being ceded to miners. And in the future, should transaction fees decrease, these change UTXOs may become more viable for spending.

For example, look at the test fixtures, particularly this one:

https://github.com/bitcoinjs/coinselect/blob/3c72703a0d28380c7565a4ccd7f9b66655d0d86c/test/fixtures/index.json#L618

 {
    "description": "inputs used in order of DESCENDING",
    "feeRate": 10,
    "inputs": [
      20000,
      {
        "script": {
          "length": 300
        },
        "value": 10000
      },
      10000
    ],
    "outputs": [
      25000
    ],
    "expected": {
      "fee": 5000,
      "inputs": [
        {
          "i": 0,
          "value": 20000
        },
        {
          "i": 2,
          "value": 10000
        }
      ],
      "outputs": [
        {
          "value": 25000
        }
      ]
    }
  },

In this specific case, I believe the expected solution should include a change output. Assuming a P2PKH scenario, if you do the numbers, the change should have been 1260 sats.

A potential solution, if deemed appropriate, would be to bypass the Dust check when adding change. A potential solution, if deemed appropriate, would be doing as Core:

  // is it worth a change output?
  if (remainderAfterExtraOutput > (148 + 34) * 3) {
    outputs = outputs.concat({ value: remainderAfterExtraOutput })
  }

I believe the criterion for Dust should primarily apply when deciding whether to add a new UTXO or not.

In terms of Dust, Bitcoin Core employs a specific policy for UTXOs: it avoids adding a UTXO if the value transferred is less than two-thirds of its total value a similar approach on new outputs.

You can find more details about this policy in their code:

https://github.com/bitcoin/bitcoin/blob/d752349029ec7a76f1fd440db2ec2e458d0f3c99/src/policy/policy.cpp#L26

As for the strategy concerning the addition of change outputs in transactions, especially in cases that might involve Dust, I have yet to investigate what Core does further.

I'm keen to hear thoughts on this issue.

landabaso commented 9 months ago

Regarding dust, it's important to understand that the UTXO coin selection algorithms in this repository do not add change outputs if they are below a dust threshold. However, users bear the responsibility to ensure that their target outputs do not fall into the 'dust' category, which is crucial for preventing the creation of transactions that might not be relayed by Bitcoin nodes, depending on their policies. Also, it's noteworthy that the dust metric used here differs from the one employed in Bitcoin Core (see message above).

For illustration, consider this test case, where the output value (1) is below the 546 satoshi dust threshold for P2PKH outputs: https://github.com/bitcoinjs/coinselect/blob/3c72703a0d28380c7565a4ccd7f9b66655d0d86c/test/fixtures/accumulative.json#L54

As this repository focuses on non-SegWit transactions, a straightforward measure to ensure compliance with dust policies could be to verify that all target values are at least 546 satoshis. This threshold assumes the Bitcoin Core default dustRelayFee of 3000 satoshis per kilobyte.

Note: This issue does not address a bug, but rather proposes an idea for potential future consideration and improvement.

junderw commented 9 months ago

I've given you write access to this repo if you'd like to work on it.

landabaso commented 9 months ago

I've given you write access to this repo if you'd like to work on it.

Thanks! I've been deeply engaged in developing a coinselect repository that leverages descriptors, and my work is heavily influenced by this repository. That's why I've been looking so closely into it.

Once I finish up with my current work, I plan to come back and make some basic changes here too, especially to align the dust calculation with Bitcoin Core and to prevent creating targets that might not get relayed.

Looking forward to contributing more soon.

junderw commented 9 months ago

Feel free to make a breaking release. This repo is outdated in many ways. Just let me know when you want a version published (you can tag it on Github first and I'll just publish those)

tankakatan commented 4 months ago

Hello everyone, are there any updates on this? @landabaso, have you had a chance to work on the issue?