wallet: handle MiniMiner bump fee calculation failures #34698

1. 
   shuv-amp commented at 9:12 AM on February 28, 2026: contributor

   Fixes #29711.

   When MiniMiner cannot calculate individual bump fees, wallet funding can index missing bump-fee entries and return map::at to users.

   Treat an empty bump-fee map for non-empty requested outpoints as a calculation failure. Make AvailableCoins return util::Result<CoinsResult> so transaction creation and sendall can propagate the wallet error instead of leaking an internal exception.

   Add a functional regression test in wallet_spend_unconfirmed.py covering the large unconfirmed set scenario from #29711. The test checks auto-selected inputs, manually selected inputs, and sendall, and verifies that spending succeeds again after mining a block.

   Tested:

   • cmake --build build
   • build/bin/test_bitcoin --catch_system_errors=no
   • build/bin/test_bitcoin --run_test=miniminer_tests --catch_system_errors=no
   • build/bin/test_bitcoin --run_test=spend_tests --catch_system_errors=no
   • build/bin/test_bitcoin --run_test=coinselector_tests/bump_fee_test --catch_system_errors=no
   • build/bin/test_bitcoin --run_test=wallet_tests/ListCoinsTest --catch_system_errors=no
   • build/bin/test_bitcoin --run_test=wallet_tests/BasicOutputTypesTest --catch_system_errors=no
   • python3 build/test/functional/test_runner.py wallet_spend_unconfirmed.py wallet_sendall.py wallet_send.py wallet_fundrawtransaction.py wallet_bumpfee.py wallet_create_tx.py wallet_basic.py
   • build/bin/bench_bitcoin -filter='WalletAvailableCoins|WalletCreateTxUseOnlyPresetInputs|WalletCreateTxUsePresetInputsAndCoinSelection' -sanity-check
2. DrahtBot added the label Wallet on Feb 28, 2026
3. 
   DrahtBot commented at 9:12 AM on February 28, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   External sites

   • Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/34698
   • Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/34698.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/34698#author-ralph

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline and AI policy for information on the review process.

   Type	Reviewers
   ACK	achow101

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #35511 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35511.svg">} (RFC: consensus: Make CAmount a class by hodlinator)
   • #35302 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35302.svg">} (Silent Payments: Sending (take 2) by Eunovo)

   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.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

4. in src/node/mini_miner.cpp:309 in 5f94606927

    305 | @@ -306,9 +306,9 @@ std::map<Txid, uint32_t> MiniMiner::Linearize()
    306 |      return m_inclusion_order;
    307 |  }
    308 |  
    309 | -std::map<COutPoint, CAmount> MiniMiner::CalculateBumpFees(const CFeeRate& target_feerate)
    310 | +std::optional<std::map<COutPoint, CAmount>> MiniMiner::CalculateBumpFees(const CFeeRate& target_feerate)
   

   ---

   ---

   

   achow101 commented at 10:43 PM on March 24, 2026:

   Changing this to be std::optional seems a bit unnecessary to me as it's equivalently easy to check for !empty(). That probably reduces the diff by half.

   ---

   

   shuv-amp commented at 2:05 AM on March 25, 2026:

   Makes sense. I'll simplify this by keeping the map return type and detecting failure based on whether entries were produced for all requested outpoints, rather than changing the interface to return std::optional.

5. shuv-amp force-pushed on Mar 25, 2026
6. shuv-amp force-pushed on Mar 25, 2026
7. in src/wallet/spend.cpp:336 in 69b9601da8

    331 | +
    332 | +CoinsResult AvailableCoins(const CWallet& wallet,
    333 | +                           const CCoinControl* coinControl,
    334 | +                           std::optional<CFeeRate> feerate,
    335 | +                           const CoinFilterParams& params,
    336 | +                           bool& bump_fee_calculation_failed)
   

   ---

   ---

   

   achow101 commented at 9:32 PM on April 30, 2026:

   I don't think this is the right way to propagate errors out of AvailableCoins. This is already returning CoinsResult which can be extended to include error codes/states, a la ValidationState. Or this could return a util::Result<CoinsResult> to contain the error message on error.

   ---

   

   shuv-amp commented at 9:26 PM on August 19, 2026:

   Took the second option, AvailableCoins now returns util::Result<CoinsResult>

8. shuv-amp force-pushed on May 1, 2026
9. DrahtBot added the label CI failed on May 1, 2026
10. shuv-amp force-pushed on May 2, 2026
11. DrahtBot removed the label CI failed on May 5, 2026
12. sedited requested review from murchandamus on Jun 7, 2026
13. wallet: handle MiniMiner bump fee calculation failures

    calculateIndividualBumpFees returns an empty map when MiniMiner cannot
    calculate fees for requested unconfirmed outpoints, such as when
    GatherClusters hits its transaction limit. FetchSelectedInputs and
    AvailableCoins then index the map and can surface map::at to users.
    
    Return an error when the map is empty for non-empty requested outpoints.
    Make AvailableCoins return util::Result<CoinsResult> so transaction creation
    and sendall can propagate this error. No-feerate callers keep the existing
    behavior by unwrapping the result.
    
    The mini_miner fuzz target asserted that the calculation is always ready,
    which no longer holds once that limit is hit, so handle both outcomes
    there, and add unit coverage that hitting the limit yields no bump fees.
    
    Add a functional regression test for the #29711 scenario. Cover
    auto-selected inputs, manually selected inputs, and sendall, and check that
    spending succeeds again after mining a block.

    a34afcd436
14. shuv-amp force-pushed on Aug 19, 2026
15. 
    shuv-amp commented at 9:30 PM on August 19, 2026: contributor

    Rebased on master.

    Changes since the last round of review:

    • AvailableCoins returns util::Result<CoinsResult> rather than reporting through an out-parameter, so the error travels with the result.
    • CreateTransactionInternal moves the CoinsResult instead of copying it.
    • Added a comment at both call sites for why an empty bump-fee map means the calculation gave up, and comments in the functional test for the cluster limit the scenario relies on.
16. sedited requested review from achow101 on Oct 2, 2026
17. 
    achow101 commented at 8:48 PM on October 2, 2026: member

    ACK a34afcd4366dd49b74922486bbce8a3fbd71ed83

18. 
    DevaanshPathak commented at 6:45 AM on October 9, 2026: none

    Independent reproduce + review notes

    Reproduced on pristine master (Debug) and on v30.2:

    • send #501 ok (mempool size 501)
    • send #502 → error code: -1 / map::at
    • mine 1 block → subsequent send succeeds

    Root cause matches the PR: GatherClusters returns empty when size > 500 → MiniMiner not ready → CalculateBumpFees returns empty map → wallet .at() throws. Combined bump-fee path already fails with a descriptive error (nullopt); this PR correctly aligns the individual path via util::Result<CoinsResult>.

    Suggestions (non-blocking)

    1. Please keep documenting (or asserting) that MiniMiner returns either a full map or empty — never partial — so empty() && !outpoints.empty() stays a sound failure detector.
    2. Confirm send maps the new util::Error to RPC_WALLET_ERROR (not -1), same as the sendall path in this PR.
    3. The 500+ sequential send functional path is a good end-to-end check; if CI time bites, the new miniminer_tests cluster-501 case already covers the MiniMiner contract tightly.

    Approach LGTM relative to achow101's earlier feedback (util::Result + keep map return type). Happy to help with rebase/test-runtime follow-ups if useful.

19. bitcoin blocked a user on Oct 9, 2026

Contributors

shuv-ampDrahtBotachow101DevaanshPathak

Review Requested
achow101murchandamus