wallet: use `outpoint` when estimating input size #35228

l0rinc commented at 9:23 PM on May 6, 2026: contributor

Problem

CalculateMaximumSignedInputSize() is passed the outpoint being sized, but a previous refactor stopped using that context when estimating the signed input size. This could make externally selected inputs look slightly smaller than they really are.

Fix

Pass the outpoint through again when estimating the signed input size. Add a regression test for the external-input case.

[!NOTE] the branch name still reflects the previous state of this PR, where the unused parameter was removed instead of wired back in

DrahtBot added the label Wallet on May 6, 2026

l0rinc renamed this:
~~wallet: remove the unused `COutPoint` parameter.~~
wallet: remove the unused `COutPoint` parameter
on May 6, 2026

DrahtBot commented at 9:23 PM on May 6, 2026: 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/35228.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101
Stale ACK	w0xlt, stickies-v, pablomartin4btc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34617 (fees: wallet: remove block policy fee estimator internals from wallet by ismaelsadeeq)
#34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)

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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

MaxInputWeight(*desc, CTxIn{outpoint}, coin_control, true, can_grind_r) in src/wallet/spend.cpp

w0xlt commented at 7:12 AM on May 7, 2026: contributor

ACK fd5bb5f94f181274c3797f65de679fa7f5b5b81a

stickies-v approved

stickies-v commented at 7:59 AM on May 7, 2026: contributor

ACK fd5bb5f94f181274c3797f65de679fa7f5b5b81a

pablomartin4btc commented at 8:29 AM on May 7, 2026: member

ACK fd5bb5f94f181274c3797f65de679fa7f5b5b81a

achow101 commented at 8:56 AM on May 7, 2026: member

Actually I'm not sure that this is correct. I thought there was a test for the thing that the parameter would have controlled, but it seems like there maybe isn't. Will need to look at this further in more detail.

l0rinc marked this as a draft on May 7, 2026

l0rinc commented at 9:35 AM on May 7, 2026: contributor

I'm not sure that this is correct.

The original take was to use the dead method parameter instead, see: https://github.com/l0rinc/bitcoin/pull/161/changes#diff-6e06b309cd494ef5da4e78aa0929a980767edd12342137f268b9219167064d13R97

pablomartin4btc commented at 9:42 AM on May 7, 2026: member

I can see the param use within the function was removed in commit 9b7ec39 at #26567, perhaps the function signature should have been updated there?

l0rinc commented at 2:38 PM on May 7, 2026: contributor

Maybe @darosior knows more about the expected behavior

darosior commented at 8:22 AM on May 13, 2026: member

@achow101 is right. outpoint should be forwarded to MaxInputWeight, not removed.

This parameter was introduced in d54c5c8b1b1a38b5b38e6878aea0fa8d6c1ad7e9 to unify signature size detection between the legacy DummySignTx and DummySignInput.

#26567 modernized those and extracted the signature size detection into UseMaxSig: https://github.com/bitcoin/bitcoin/blob/10ca73c02cbff59f2134c0c7da3b8d0a7e727475/src/wallet/spend.cpp#L54-L59

That is called from: https://github.com/bitcoin/bitcoin/blob/10ca73c02cbff59f2134c0c7da3b8d0a7e727475/src/wallet/spend.cpp#L69-L72

And the call site of MaxInputWeight, that replaced legacy DummySignInput, provides it a dummy txin instead of forwarding its outpoint parameter as a CTxIn: https://github.com/bitcoin/bitcoin/blob/10ca73c02cbff59f2134c0c7da3b8d0a7e727475/src/wallet/spend.cpp#L97

I think this was an oversight in #26567, specifically this line. My bad. Having a test that fail on this regression would have been useful.

Here is what i think the patch should be instead of deleting the outpoint parameter:

diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 0a6d00561e0..9384299d9a1 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -94,7 +94,7 @@ int CalculateMaximumSignedInputSize(const CTxOut& txout, const COutPoint outpoin
     if (!provider) return -1;
 
     if (const auto desc = InferDescriptor(txout.scriptPubKey, *provider)) {
-        if (const auto weight = MaxInputWeight(*desc, {}, coin_control, true, can_grind_r)) {
+        if (const auto weight = MaxInputWeight(*desc, CTxIn{outpoint}, coin_control, true, can_grind_r)) {
             return static_cast<int>(GetVirtualTransactionSize(*weight, 0, 0));
         }
     }

Thanks for triggering this discussion that uncovered a regression i introduced, @l0rinc!

l0rinc renamed this:
~~wallet: remove the unused `COutPoint` parameter~~
wallet: use outpoint when estimating input size
on May 13, 2026

l0rinc renamed this:
~~wallet: use outpoint when estimating input size~~
wallet: use `outpoint` when estimating input size
on May 13, 2026

wallet: use outpoint when estimating input size

`CalculateMaximumSignedInputSize()` is passed the outpoint being sized, but that context was not used when estimating the signed input size.
Pass the outpoint through so externally selected inputs are not underestimated.

Co-authored-by: Antoine Poinsot <darosior@protonmail.com>

cd8d3bd937

l0rinc force-pushed on May 13, 2026

l0rinc commented at 8:48 AM on May 13, 2026: contributor

Thanks a lot for checking, @darosior. I have reverted the code to the original implementation, which rewired the parameter. I adjusted the PR description and commit message, and added you as co-author.

l0rinc marked this as ready for review on May 13, 2026

achow101 commented at 10:36 PM on May 13, 2026: member

ACK cd8d3bd937b5515ea000408eb07d2ae3cd1aa417

The reason this was not caught by a test is because the tests for transaction creation are largely functional tests, and this regression only impacts fee calculation that is used by coin selection. It is almost impossible to surface with a functional test, even though we do have functional tests that test the behavior for external inputs. I would expect that this would be caught be checking the fee that fundrawtransaction returns, but the that fee is calculated by CalculateMaximumSignedTxSize which does call MaxInputWeight correctly. The only way to have observed this in a functional test would be observing coin selection doing something unexpected, but with single digit sat fee differences, it would still be hard to detect.

DrahtBot requested review from w0xlt on May 13, 2026

DrahtBot requested review from pablomartin4btc on May 13, 2026

DrahtBot requested review from stickies-v on May 13, 2026