Opening this to see if people think this is the right solution.

Very nice find!

The code wants to know the change signature size so it can figure out if it would have created change that was worthless to spend (because the spending will take more fees than the output is worth).

Assuming the size is smallest plausible signature size would be conservative: it'll overvalue the change, at a risk of keeping change it could have eliminated. I think doing that would be strictly superior to not using BnB at all, since preventing BnB will always keep change it should eliminate.

So, e.g. assuming any spend of an unknown type will take at least 32(txid)+4(vout)+4(sequence)+1(len)+64*0.25(signature) = 57 bytes would work. (don't assume my math is right. :) )

Hm yes I suppose you're right. Not running BnB is admitting defeat even if we actually end up being able to discard a larger value. I'll take a look at constructing something that makes a minimal segwit input weight.

Isn't the problem that coin_selection_params.change_spend_size is set to std::numeric_limits<size_t>::max() (SIZE_MAX) instead of the intended -1 since change_spend_size is of type size_t?

[cling]$ size_t change_spend_size = -1;
[cling]$ change_spend_size
(unsigned long) 18446744073709551615
[cling]$ (size_t)-1
(unsigned long) 18446744073709551615
[cling]$ #include <limits>
[cling]$ std::numeric_limits<size_t>::max()
(unsigned long) 18446744073709551615

The definition of change_spend_size:

CalculateMaximumSignedInputSize(…) returns -1 here:

Thechange_spend_size assignment is performed here:

Related open pull requests in need of review:

• build: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan) #14252 – build: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan) (includes -fsanitize=integer which tackles unsigned integer wraparounds (non-UB!))
• mempool: Fix unintended unsigned integer wraparound in CTxMemPool::UpdateAncestorsOf(bool add, …) when add is false #14226 – mempool: Fix unintended unsigned integer wraparound in CTxMemPool::UpdateAncestorsOf(bool add, …) when add is false
• Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations #14224 – Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations
• Fix unsigned integer wrap-around in GetBlockProofEquivalentTime #11551 – Fix unsigned integer wrap-around in GetBlockProofEquivalentTime
• Avoid unintentional unsigned integer wraparounds #11535 – Avoid unintentional unsigned integer wraparounds

Please help reviewing those :-)

@practicalswift yes that's why the assert is finally hit, but it's faulty logic regardless

Pushed a fix in line with @gmaxwell suggestions. It's pretty hardcoded, so I'd rather make this more programmatic, perhaps giving a "hint" to the dummy signer of what type it should be?

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #14711 (Remove uses of chainActive and mapBlockIndex in wallet code by ryanofsky)
• #10973 (Refactor: separate wallet from node by ryanofsky)
• #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

Concept ACK

Updated the PR to use a function that uses DummySigner flow with an arbitrary valid pubkey as the P2SH-P2WPKH target to estimate the size more programatically.

utACK 198830e06bf4560cfb962e29c73c5b1357d90b37

How about a higher-level test exhibiting the original failure/assert condition? Absent that we don't have regression coverage or direct failure exhibition.
Never mind! The test/functional/rpc_psbt.py change covers this.

The functional test could be better explained: We need it to try BnB coin selection with a change output that is a script that it doesn't have knowledge of.

rebased, just a test conflict

utACK 0fb2e69

utACK 0fb2e69

utACK 0fb2e69

utACK

@instagibbs Would you mind creating a backport for 0.17 of this?

@MarcoFalke done

Backported in #14851.