wallet: Use correct effective value when checking target

aureleoules commented at 1:10 pm on September 29, 2022: member

Fixes #26185. The following assert failed because it was not checked in the parent function.

https://github.com/bitcoin/bitcoin/blob/2bd9aa5a44b88c866c4d98f8a7bf7154049cba31/src/wallet/coinselection.cpp#L391

aureleoules commented at 1:10 pm on September 29, 2022: member

@1440000bytes could you check if this fixes your issue? Thanks.

maflcko added this to the milestone 24.0 on Sep 29, 2022

fanquake added the label Wallet on Sep 29, 2022

fanquake added the label Needs backport (24.x) on Sep 29, 2022

fanquake requested review from achow101 on Sep 29, 2022

fanquake commented at 2:10 pm on September 29, 2022: member

Also includes a quick refactor.

Generally, we avoid throwing “quick refactors” into bug fixes. Especially if they are going to be backported. Or, at least the refactor should be a separate commit, so if it doesn’t need backporting, it doesn’t have to be.

aureleoules force-pushed on Sep 29, 2022

aureleoules commented at 2:15 pm on September 29, 2022: member

Generally, we avoid throwing “quick refactors” into bug fixes.

Alright, removed.

aureleoules commented at 2:37 pm on September 29, 2022: member

Added a unit test.

ghost commented at 5:03 pm on September 29, 2022: none

@aureleoules I want to clarify one thing:

Is this a bug only on Windows or other platforms? If its a bug on some linux distro as well it will be easier for me to test

ghost commented at 5:52 pm on September 29, 2022: none

@aureleoules I want to clarify one thing:

Is this a bug only on Windows or other platforms? If its a bug on some linux distro as well it will be easier for me to test

Or maybe that should be job of testers.. Give me one day I will test it.

sipa commented at 6:45 pm on September 29, 2022: member

This is a logic bug in the wallet. It’s entirely platform independent.

DrahtBot commented at 0:47 am on September 30, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25729 (wallet: Check max transaction weight in CoinSelection by aureleoules)
#25685 (wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection by furszy)

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.

ghost commented at 3:21 pm on September 30, 2022: none

I don’t understand whats happening in this issue. I compiled PR branch and tested it on Windows, it fixes the previous error and bitcoind or bitcoin-qt does not crash. However, returns “insufficient funds”. @sipa said this is platform independent but I tried to run the same command on Fedora with v24.0rc1 and it returns PSBT with a change:

Windows (PR Branch):

 0bitcoin-cli listunspent
 1
 2[
 3  {
 4    "txid": "e911734bf711009dd6057c88bc598df2d31c18d1cc6bd6c790a704b519d77e68",
 5    "vout": 0,
 6    "address": "2N7uBTqqQ99ArbgmBuw2AANsf24bMVdYxgp",
 7    "label": "",
 8    "redeemScript": "522102de62833c851509cf13c0c63e0dc3ef7b74f7d86b17e9090fab68c09830cb2f2e2103cf11a2bc1371dad3691bce59e37d85b12aae6b030b533d4e7c5aedeb47a5ec0e52ae",
 9    "scriptPubKey": "a914a0bf75baf8f3a4ae761c2faa2516b8540e993d8987",
10    "amount": 0.00100000,
11    "confirmations": 595,
12    "spendable": false,
13    "solvable": true,
14    "desc": "sh(multi(2,[aa130fed]02de62833c851509cf13c0c63e0dc3ef7b74f7d86b17e9090fab68c09830cb2f2e,[00890d8d]03cf11a2bc1371dad3691bce59e37d85b12aae6b030b533d4e7c5aedeb47a5ec0e))#j8w6g6zg",
15    "parent_descs": [
16    ],
17    "safe": true
18  }
19]
20
21bitcoin-cli walletcreatefundedpsbt "[{\"txid\":\"e911734bf711009dd6057c88bc598df2d31c18d1cc6bd6c790a704b519d77e68\",\"vout\":0}]" "[{\"tb1q9fxp2p0sm93jdlul8v3svzn7x69zcd9ljsgans\":\"0.0009\"}]"
22
23Insufficient funds (code -4)

Fedora (v24.0rc1):

 0bitcoin-cli listunspent
 1
 2[
 3  {
 4    "txid": "e911734bf711009dd6057c88bc598df2d31c18d1cc6bd6c790a704b519d77e68",
 5    "vout": 0,
 6    "address": "2N7uBTqqQ99ArbgmBuw2AANsf24bMVdYxgp",
 7    "label": "",
 8    "redeemScript": "522102de62833c851509cf13c0c63e0dc3ef7b74f7d86b17e9090fab68c09830cb2f2e2103cf11a2bc1371dad3691bce59e37d85b12aae6b030b533d4e7c5aedeb47a5ec0e52ae",
 9    "scriptPubKey": "a914a0bf75baf8f3a4ae761c2faa2516b8540e993d8987",
10    "amount": 0.00100000,
11    "confirmations": 595,
12    "spendable": false,
13    "solvable": true,
14    "desc": "sh(multi(2,[aa130fed]02de62833c851509cf13c0c63e0dc3ef7b74f7d86b17e9090fab68c09830cb2f2e,[00890d8d]03cf11a2bc1371dad3691bce59e37d85b12aae6b030b533d4e7c5aedeb47a5ec0e))#j8w6g6zg",
15    "parent_descs": [
16    ],
17    "safe": true
18  }
19]
20
21bitcoin-cli  walletcreatefundedpsbt "[{\"txid\":\"e911734bf711009dd6057c88bc598df2d31c18d1cc6bd6c790a704b519d77e68\",\"vout\":0}]" "[{\"tb1q9fxp2p0sm93jdlul8v3svzn7x69zcd9ljsgans\":\"0.0009\"}]"
22
23{
24  "psbt": "cHNidP8BAHECAAAAAWh+1xm1BKeQx9ZrzNEYHNPyjVm8iHwF1p0AEfdLcxHpAAAAAAD9////AsYlAAAAAAAAFgAUJT0nNy2sgypq59m0DmCbgIWp0m6QXwEAAAAAABYAFCpMFQXw2WMm/587IwYKfjaKLDS/AAAAAAABAHICAAAAAQzC3aNgVZV/fAJNlVsvxAMFuTtvRKqlPocBAGlOSHxDAAAAAAD+////AqCGAQAAAAAAF6kUoL91uvjzpK52HC+qJRa4VA6ZPYmHm52Yh1IGAAAWABQN4fccLGiwTrWIb1+7fsWapgno0aOsAQABBEdSIQLeYoM8hRUJzxPAxj4Nw+97dPfYaxfpCQ+raMCYMMsvLiEDzxGivBNx2tNpG85Z432FsSquawMLUz1OfFrt60el7A5SriIGAt5igzyFFQnPE8DGPg3D73t099hrF+kJD6towJgwyy8uBKoTD+0iBgPPEaK8E3Ha02kbzlnjfYWxKq5rAwtTPU58Wu3rR6XsDgQAiQ2NACICAiOsbsXVfUGqw6ybbE+LIjii6xEJVH7K9GgwIxu6pruVEMw4p/EAAACAAQAAgAcAAIAAAA==",
25  "fee": 0.00000330,
26  "changepos": 0
27}

in src/wallet/test/coinselector_tests.cpp:929 in 1e60f5d4d5 outdated

921@@ -922,5 +922,52 @@ BOOST_AUTO_TEST_CASE(effective_value_test)
922     BOOST_CHECK_EQUAL(output5.GetEffectiveValue(), nValue); // The effective value should be equal to the absolute value if input_bytes is -1
923 }
924 
925+BOOST_AUTO_TEST_CASE(SelectCoins_effective_value_test)
926+{
927+    // Test that effective value is used in coin selection
928+    // This test creates an external coin with a high effective value
929+    // The coin will not be selected because its effective value is less than the target

glozow commented at 5:17 pm on September 30, 2022:

No coin selection is actually happening, as m_allow_other_inputs = false. The effective value is too low, not too high

0    // Test that effective value is used to check the value of preset inputs.
1    // This test creates a coin in which the effective value is too low to cover the fees, but is higher than the target value.
2    // The coin is selected using coin control, with m_allow_other_inputs = false. SelectCoins should fail.

murchandamus commented at 7:17 pm on September 30, 2022:

The effective value of a UTXO is effValue = value - fee, so rather than the effective value being too low to cover the fees, the effective value is too low to create the output after deducting the fees:

0    // Test that the effective value is used to check whether preset inputs provide sufficient funds when subtract_fee_outputs is not used.
1    // This test creates a coin whose value is higher than the target but whose effective value is lower than the target.
2    // The coin is selected using coin control, with m_allow_other_inputs = false. SelectCoins should fail due to insufficient funds.

glozow commented at 5:31 pm on September 30, 2022: member

@1440000bytes So IIUC you have 1 coin worth 0.001 BTC (as shown by your listunspent). You tried to create a transaction paying 0.0009 using walletcreatefundedpsbt, passing that coin in manually. Since you specified an input manually, m_allow_other_inputs is false. https://github.com/bitcoin/bitcoin/blob/f59e91511a3aa8b2770eeec7034ddc1a9dec918b/src/wallet/rpc/spend.cpp#L1659-L1663 This needs to fund the payment and the fees for the transaction, including the cost to spend the coin. 0.001 covers the payment and non-input fees, but not the cost to spend. So it is indeed the case that the wallet has insufficient funds.

cause of the crash and why the fix works:

SelectionResult::AddInput changes m_use_effective to true: https://github.com/bitcoin/bitcoin/blob/f59e91511a3aa8b2770eeec7034ddc1a9dec918b/src/wallet/spend.cpp#L584 https://github.com/bitcoin/bitcoin/blob/f59e91511a3aa8b2770eeec7034ddc1a9dec918b/src/wallet/coinselection.cpp#L441-L445 GetSelectedValue() is 0.001 and GetSelectedEffectiveValue() is something less than 0.0009. So this condition doesn’t hit: https://github.com/bitcoin/bitcoin/blob/f59e91511a3aa8b2770eeec7034ddc1a9dec918b/src/wallet/spend.cpp#L585 But the assertion does, since selected_effective_value is something less than 0.0009.

glozow commented at 5:36 pm on September 30, 2022: member

concept ACK 1e60f5d4d5e9990e00ddbaa0cfacf3c5731d73f9 whether or not it’s the cause of the crash in #26185, this is a bug fix, as AFAICT it’s incorrect to not use effective value here

glozow requested review from murchandamus on Sep 30, 2022

glozow requested review from S3RK on Sep 30, 2022

murchandamus commented at 6:10 pm on September 30, 2022: contributor

I don’t understand whats happening in this issue. I compiled PR branch and tested it on Windows, it fixes the previous error and bitcoind or bitcoin-qt does not crash. However, returns “insufficient funds”.

@sipa said this is platform independent but I tried to run the same command on Fedora with v24.0rc1 and it returns PSBT with a change: @1440000bytes: I missed this at first, but could you confirm that the first output you posted is resulting from running PR/26203 (this branch) on Windows, and the second output is from running v24.0rc1 on Fedora?

mzumsande commented at 6:43 pm on September 30, 2022: contributor

Concept ACK.

The assert can also be reproduced by adjusting the amount in the psbt just between the regimes where it succeeds and the regime with insufficient funds. E.g. on an empty regtest datadir:

0./bitcoind -regtest -fallbackfee=0.001
1./bitcoin-cli -regtest -generate 101
2./bitcoin-cli -regtest listunspent -> use txid in next call
3./bitcoin-cli -regtest walletcreatefundedpsbt "[{\"txid\":\"<txid>\",\"vout\":0}]" "[{\"<some address>\":\"49.9999\"}]"

will lead to a crash on master (which is fixed by this PR).

ghost commented at 6:52 pm on September 30, 2022: none

This needs to fund the payment and the fees for the transaction, including the cost to spend the coin. 0.001 covers the payment and non-input fees, but not the cost to spend. So it is indeed the case that the wallet has insufficient funds.

What is cost to spend?

@1440000bytes: I missed this at first, but could you confirm that the first output you posted is resulting from running PR/26203 (this branch) on Windows, and the second output is from running v24.0rc1 on Fedora?

Yes

and both have same config:

 0# Windows
 1
 2signet=1
 3txindex=1
 4
 5signet.rpcport=18332
 6rpcuser=user
 7rpcpassword=pass
 8
 9fallbackfee=0.0004
10
11
12# Fedora
13
14signet=1
15txindex=1
16
17[signet] 
18rpcport=18332
19rpcuser=user
20rpcpassword=pass
21
22fallbackfee=0.0004

in src/wallet/coinselection.h:348 in f560f6b8e4 outdated

343@@ -344,6 +344,8 @@ struct SelectionResult
344     CAmount GetTarget() const { return m_target; }
345 
346     SelectionAlgorithm GetAlgo() const { return m_algo; }
347+
348+    bool UseEffectiveValue() const { return m_use_effective; }

murchandamus commented at 6:57 pm on September 30, 2022:

I don’t think we need this new method, as we have the necessary information in the callsite already (see below).

in src/wallet/spend.cpp:590 in f560f6b8e4 outdated

586+
587+        if (result.UseEffectiveValue() && result.GetSelectedEffectiveValue() < nTargetValue) {
588+            return std::nullopt;
589+        } else if (result.GetSelectedValue() < nTargetValue) {
590+            return std::nullopt;
591+        }

murchandamus commented at 7:05 pm on September 30, 2022:

We always use the effective value of UTXOs except when the recipient pays the fees via SFFO, so you can go by the negation of the corresponding coin selection parameter:

0        if (!coin_selection_params.m_subtract_fee_outputs && result.GetSelectedEffectiveValue() < nTargetValue) {
1            return std::nullopt;
2        } else if (result.GetSelectedValue() < nTargetValue) {
3            return std::nullopt;
4        }

Perhaps it would be nicer if we dropped the negation and swapped if and else if branches:

0        if (coin_selection_params.m_subtract_fee_outputs && result.GetSelectedValue() < nTargetValue) {
1            return std::nullopt;
2        } else if (result.GetSelectedEffectiveValue() < nTargetValue) {
3            return std::nullopt;
4        }

or more compactly:

0        if (result.GetSelectedEffectiveValue() < nTargetValue || coin_selection_params.m_subtract_fee_outputs && result.GetSelectedValue() < nTargetValue) {
1            return std::nullopt;
2        }

aureleoules commented at 11:35 pm on October 1, 2022:

Thanks, I pushed your first edited version for readability, the two others translate to another statement that is incorrect though.

murchandamus commented at 5:56 pm on October 4, 2022:

Ah thanks for catching that

murchandamus commented at 7:21 pm on September 30, 2022: contributor

Concept ACK, have a couple small improvement suggestions:

aureleoules force-pushed on Oct 1, 2022

wallet: Use correct effective value when checking target 76b79c1a17

test: Check external coin effective value is used in CoinSelection d0d9cf7aea

aureleoules force-pushed on Oct 1, 2022

aureleoules commented at 11:44 pm on October 1, 2022: member

Thank you @Xekyo @glozow @mzumsande for the reviews, I addressed your comments. Thanks @1440000bytes for testing. push diff here: https://github.com/bitcoin/bitcoin/compare/1e60f5d4d..d0d9cf7aea2ff36a14a19e6999400e4070b7b0c9

S3RK commented at 7:36 am on October 3, 2022: contributor

Overall the change looks correct to me. I was trying to backport the test to see where the bug was introduced, but didn’t succeed due to too many refactors. Is it a regression from 23.0 or not?

glozow commented at 2:58 pm on October 3, 2022: member

reACK d0d9cf7aea2ff36a14a19e6999400e4070b7b0c9

glozow requested review from murchandamus on Oct 3, 2022

furszy approved

furszy commented at 5:58 pm on October 3, 2022: member

ACK d0d9cf7a

Happily, #25685 fixes this too. I introduced the same preset-inputs result selection target check inside aa5fdbad for another reason there. So.. the unit test introduced here is also passing there without having to cherry-pick the bug fix 76b79c1a.

glozow merged this on Oct 4, 2022

glozow closed this on Oct 4, 2022

fanquake removed the label Needs backport (24.x) on Oct 4, 2022

glozow commented at 9:05 am on October 4, 2022: member

Happily, #25685 fixes this too.

That’s good to hear, though I think this is more appropriate for a backport. #26242

sidhujag referenced this in commit 5669e799cc on Oct 4, 2022

glozow referenced this in commit 2e5706d601 on Oct 5, 2022

murchandamus commented at 6:01 pm on October 5, 2022: contributor

Post-merge ACK d0d9cf7aea2ff36a14a19e6999400e4070b7b0c9

achow101 commented at 4:05 pm on October 6, 2022: member

Post merge ACK d0d9cf7aea2ff36a14a19e6999400e4070b7b0c9

aureleoules deleted the branch on Nov 2, 2022

bitcoin locked this on Nov 2, 2023

wallet: Use correct effective value when checking target #26203

Conflicts