wallet: make coinbase that will mature on the next block available for selection #32123

pull luisschwab wants to merge 1 commits into bitcoin:master from luisschwab:fix/off-by-one-coinbase changing 14 files +71 −49
  1. luisschwab commented at 9:31 pm on March 22, 2025: contributor

    Closes #32098.

    Currently, only coinbase UTXOs that are mature at height h are available for coin selection. This PR makes UTXOs that mature at height h+1 available for selection.

    Since a transaction spending this output will be accepted by the mempool, it should be available for selection.

    BDK implements this logic1, which I believe to be the correct one.

     0$ bitcoin-cli -regtest createwallet wallet
     1{
     2  "name": "wallet"
     3}
     4
     5$ export ADDRESS=$(bitcoin-cli -regtest -rpcwallet=wallet getnewaddress)
     6
     7$ bitcoin-cli -regtest generatetoaddress 101 $ADDRESS
     8[
     9  "467fd440d0ed89d0cad64fa64d0320968b33a32c681b6c3f5a5ed1d1c016f45a",
    10  "1b2246eaa35689710acc5d1bb2923197993233fb9186f0bd561cb546c7667ca7",
    11  ...
    12  "5d5ee44ada395ab1d978a77596ed3893be750840229e7234a85a01984057798a",
    13  "7f8a573a6824edc5342cf86e5e7c0a6ea5664305ef21808456fc98b2621b41cf"
    14]
    15
    16$ bitcoin-cli -regtest -rpcwallet=wallet getbalance
    17- 50.00000000
    18+ 100.00000000
    
  2. DrahtBot commented at 9:31 pm on March 22, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32123.

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31250 (wallet: Disable creating and loading legacy wallets by achow101)
    • #28710 (Remove the legacy wallet and BDB dependency by achow101)
    • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label Wallet on Mar 22, 2025
  4. DrahtBot added the label CI failed on Mar 22, 2025
  5. luisschwab force-pushed on Mar 22, 2025
  6. luisschwab force-pushed on Mar 22, 2025
  7. luisschwab force-pushed on Mar 23, 2025
  8. luisschwab force-pushed on Mar 23, 2025
  9. fjahr commented at 12:03 pm on March 23, 2025: contributor

    I understand what you are saying about correctness but @sipa ’s concerns with propagation issues are also hard to ignore. Bitcoin Core just tends to be on the more conservative side when it comes to protecting users and BDK might leave protections like this to be handled by the actual wallet implemeters that use BDK.

    A potential middle ground could be that current behavior is maintained but users get a useful hint in return why they can’t spend their funds yet. Then there could be a flag added that allows to spend the funds (-f for force or so). Still not sure if this would achieve enough conceptual agreement but I think the chances are higher than with the current change.

  10. sipa commented at 12:17 pm on March 23, 2025: member

    @luisschwab There are other examples of coins which the Bitcoin Core wallet won’t let users spend, for example, unconfirmed coins (unless they are self-created/change). Just like here, that’s not a protocol rule, but it’s a bad idea, and it’s perfectly possible to bypass by using raw transactions.

    Now, given how unlikely it is in 2025 for any user to use the Bitcoin Core wallet directly for mining payouts, the odds that this is still actually protecting anyone are probably negligible. So @fjahr , I think offering a way to disable this behavior would be huge overkill. It probably doesn’t matter either way in practice.

    But on the other hand, I just see no reason to change this. If direct user mining payouts were common, this would be a useful rule. There is nothing incorrect about the current behavior, and I categorically disagree that the wallet should just allow anything that’s legal in the protocol without question. If someone really wants to bypass this, they can always use raw transactions.

  11. luisschwab commented at 4:19 pm on March 25, 2025: contributor
    What do you think @murchandamus?
  12. maflcko commented at 2:20 pm on April 1, 2025: member
    #32098 (comment) could be a reason that this is fine to change, but the CI is failing on this pull
  13. luisschwab force-pushed on Apr 1, 2025
  14. luisschwab commented at 6:40 pm on April 1, 2025: contributor

    but the CI is failing on this pull

    Not sure what is going on with CI, other PRs are failing on Windows as well.

    edit: https://github.com/bitcoin/bitcoin/pull/32184/ fixes it

  15. DrahtBot removed the label CI failed on Apr 1, 2025
  16. wallet: make coinbase output that will mature on the next block available for selection 4619b73c54
  17. luisschwab force-pushed on Apr 1, 2025
  18. achow101 commented at 7:40 pm on April 10, 2025: member

    -0

    I agree with the rationale given by the others to not make this change.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-04-19 00:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me