wallet: fix coin selection tracing to return -1 when no change pos #29272

remyers · 2024-01-18T07:56:37Z

This is a bugfix for from when optional was introduced for change_pos in the wallet. When optional change_pos is unset, we should return -1 and not 0.

I added two new checks to the test/functional/interface_usdt_coinselection.py which adds coverage for the situations when normal_create_tx_internal and aps_create_tx_internal events occur with no change.

You can reproduce this bug using the coin-selection-simulation scripts as described in issue #16. You can also run the interface_usdt_coinselection.py test without the changes to wallet/spend.cpp.

DrahtBot · 2024-01-18T07:56:40Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	murchandamus, 0xB10C, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)

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.

remyers · 2024-01-18T07:59:43Z

You might want to take a look at this @murchandamus. I discovered the problem while trying to repo your coingrinder results with the coin-selection-simulation scripts.

0xB10C · 2024-01-18T08:37:27Z

src/wallet/spend.cpp

@@ -1355,7 +1355,7 @@ util::Result<CreatedTransactionResult> CreateTransaction(
        // if fee of this alternative one is within the range of the max fee, we use this one
        const bool use_aps{txr_grouped.has_value() ? (txr_grouped->fee <= txr_ungrouped.fee + wallet.m_max_aps_fee) : false};
        TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, txr_grouped.has_value(),
-               txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() && txr_grouped->change_pos.has_value() ? *txr_grouped->change_pos : 0);
+               txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() && txr_grouped->change_pos.has_value() ? *txr_grouped->change_pos : -1);


nit: I think this tracepoint would be easier to reason about if it was one line per argument. Not sure if this PR is the right place to reformat it.

This seems like a good practice and is consistent with other TRACEx uses in the code where x > 1. This would also makes it easier to check the correct number of parameters is used if PR #26593 is adopted. Fixed in 350606d.

DrahtBot · 2024-01-18T08:42:46Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/20605138868}

0xB10C · 2024-01-18T08:48:41Z

src/wallet/spend.cpp

    TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), bool(res),
-           res ? res->fee : 0, res && res->change_pos.has_value() ? *res->change_pos : 0);
+           res ? res->fee : 0, res && res->change_pos.has_value() ? *res->change_pos : -1);


CI is unhappy here:

(mocktwallet/spend.cpp:1339:5: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'unsigned int' changed the value to 4294967295 (32-bit, unsigned) #0 0x5589c92846e6 in wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1339:5 #1 0x5589c8d8617d in wallet::spend_tests::wallet_duplicated_preset_inputs_test::test_method() src/wallet/test/spend_tests.cpp:100:53 ... SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change wallet/spend.cpp:1339:5 in

I think wrapping the trace parameter in static_cast<int32_t>() is the correct way to resolve this because change_pos is otherwise a std::optional<unsigned int>. In tracing.md change_pos is defined as int32 so it should always be returned as a signed 32-bit int.

.. or maybe static_cast is overkill, a cast with int32_t() seems to be what's recommended in developer-notes.md .

I had my casting on the wrong value - we need to cast the unsigned optional value. Fixed in e701982.

murchandamus · 2024-01-18T21:51:51Z

Returning -1 if there is no change seems reasonable to me, but it looks like it’s cast to an unsigned int somewhere else in the stack. I’m not familiar with the tracing framework interna, so it’s not obvious to me how to fix this.

achow101 · 2024-01-18T22:03:30Z

ACK 8c6d266

murchandamus

tACK with nit: e701982

murchandamus · 2024-01-19T15:45:15Z

test/functional/interface_usdt_coinselection.py

+        self.log.info("Change position is -1 if no change is created with APS and APS was explicitly disabled")
+        # We should have 2 tracepoints in the order:
+        # 1. selected_coins (type 1)
+        # 2. normal_create_tx_internal (type 2)
+        # 3. attempting_aps_create_tx (type 3)
+        # 4. selected_coins (type 1)
+        # 5. aps_create_tx_internal (type 4)
+        wallet.sendtoaddress(address=wallet.getnewaddress(), amount=10, subtractfeefromamount=True, avoid_reuse=False)
+        events = self.get_tracepoints([1, 2, 3, 1, 4])
+        success, use_aps, algo, waste, change_pos = self.determine_selection_from_usdt(events)
+        assert_equal(success, True)
+        assert_equal(change_pos, -1)


There were a couple transactions before, each sending 10 BTC to the wallet itself. So the wallet should still have a balance of 50 – fees BTC, and presumably 3 UTXOs of 10 BTC, 10 BTC, and 30 - fees BTC. I‘m not sure why APS (avoid partial spend) would make a difference here. APS just means that we’ll force use of all coins associated with the same scriptpubkey at once rather than a single UTXO if there is address reuse, but it seems to me that funds are always sent to new addresses here.

It seems to me that this test works because BnB is disabled due subtractfeefromamount being active, Knapsack finding the single input solution and Knapsack being run before SRD and therefore Knapsack’s result being preferred, but it’s not obvious why this transaction would generally end up not having any change if e.g. we change how we pick the input set from the various coin selection algorithm results. It might be more future proof to get the current balance, and then send the entire balance while setting subtractfeefromamount.

Usually I would tend to suggest sendall to make a changeless transaction, but sendall doesn’t use coinselection which is what we are actually trying to test here.

So maybe, these two test-cases could just be folded into one and the distinction for APS could be dropped, or the first should perhaps also spend the entire balance.

Thanks for the explanation; although the next test after this one does what you suggest, it doesn't trigger the fall-through APS path. I'll see if I can rework this test to better match a real APS situation.

Fixed in 39d8621. I also updated the description to make it clearer that the APS path is tested due to APS being initially disabled.

DrahtBot · 2024-01-20T11:46:52Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/20685191675}

murchandamus

ACK, 350606d. The final code LGTM.

Nit: It seems to me that the first three commits are iterating on the same conceptual change. I would prefer if those three commits were squashed into one so we immediately arrive at the final version. The refactor of the trace output should stay a separate commit, though.

furszy

It seems to me that the first three commits are iterating on the same conceptual change.

Also, to keep a clean commits history, each commit should pass the CI tasks independently. And the first one alone is not passing.

remyers · 2024-01-20T14:09:09Z

Nit: It seems to me that the first three commits are iterating on the same conceptual change. I would prefer if those three commits were squashed into one so we immediately arrive at the final version. The refactor of the trace output should stay a separate commit, though.

Good point, I meant to do that; squashed in d55fdb1.

murchandamus

ACK d55fdb1

0xB10C · 2024-01-20T23:08:10Z

ACK d55fdb1

The int32_t cast and the tracing related changes look good to me. Thanks for reformatting the tracepoints! I'm not familiar enough with coin selection to comment on the test changes.

achow101 · 2024-01-23T19:20:27Z

ACK d55fdb1

…when no change pos d55fdb1 Move TRACEx parameters to seperate lines (Richard Myers) 2d58629 wallet: fix coin selection tracing to return -1 when no change pos (Richard Myers) Pull request description: This is a bugfix for from when [optional was introduced](bitcoin@758501b) for `change_pos` in the wallet. When optional `change_pos` is unset, we should return -1 and not 0. I added two new checks to the `test/functional/interface_usdt_coinselection.py` which adds coverage for the situations when `normal_create_tx_internal` and `aps_create_tx_internal` events occur with no change. You can reproduce this bug using the coin-selection-simulation scripts as described in [issue bitcoinops#16](achow101/coin-selection-simulation#16). You can also run the `interface_usdt_coinselection.py` test without the changes to `wallet/spend.cpp`. ACKs for top commit: 0xB10C: ACK d55fdb1 achow101: ACK d55fdb1 murchandamus: ACK d55fdb1 Tree-SHA512: 6efac3b756bdf51debbcb759dc3c4b7a4304626bc047b70025cec02f3a04937ace7712e9558ac71e560fd136005a98c518ac5bb4b90c3282d776beccd0de9749

DrahtBot added the Wallet label Jan 18, 2024

0xB10C reviewed Jan 18, 2024

View reviewed changes

DrahtBot added the CI failed label Jan 18, 2024

0xB10C reviewed Jan 18, 2024

View reviewed changes

DrahtBot mentioned this pull request Jan 18, 2024

tracing: Only prepare tracepoint arguments when actually tracing #26593

Merged

remyers force-pushed the cs-usdt-fixup branch from 8c6d266 to e701982 Compare January 18, 2024 22:34

DrahtBot removed the CI failed label Jan 18, 2024

murchandamus approved these changes Jan 19, 2024

View reviewed changes

DrahtBot requested a review from achow101 January 19, 2024 16:04

remyers force-pushed the cs-usdt-fixup branch from a32af4c to 350606d Compare January 20, 2024 11:46

DrahtBot added the CI failed label Jan 20, 2024

DrahtBot removed the CI failed label Jan 20, 2024

murchandamus reviewed Jan 20, 2024

View reviewed changes

remyers added 2 commits January 20, 2024 14:56

wallet: fix coin selection tracing to return -1 when no change pos

2d58629

Move TRACEx parameters to seperate lines

d55fdb1

remyers force-pushed the cs-usdt-fixup branch from 350606d to d55fdb1 Compare January 20, 2024 14:06

furszy reviewed Jan 20, 2024

View reviewed changes

murchandamus reviewed Jan 20, 2024

View reviewed changes

DrahtBot removed the request for review from achow101 January 23, 2024 19:20

achow101 merged commit 7cb7759 into bitcoin:master Jan 23, 2024

bitcoin locked and limited conversation to collaborators Jan 22, 2025

wallet: fix coin selection tracing to return -1 when no change pos #29272

wallet: fix coin selection tracing to return -1 when no change pos #29272

Uh oh!

Conversation

remyers commented Jan 18, 2024

Uh oh!

DrahtBot commented Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

remyers commented Jan 18, 2024

Uh oh!

0xB10C Jan 18, 2024

Choose a reason for hiding this comment

Uh oh!

remyers Jan 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jan 18, 2024

Uh oh!

0xB10C Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remyers Jan 18, 2024

Choose a reason for hiding this comment

Uh oh!

remyers Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remyers Jan 18, 2024

Choose a reason for hiding this comment

Uh oh!

murchandamus commented Jan 18, 2024

Uh oh!

achow101 commented Jan 18, 2024

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

murchandamus Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remyers Jan 19, 2024

Choose a reason for hiding this comment

Uh oh!

remyers Jan 20, 2024

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jan 20, 2024

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

remyers commented Jan 20, 2024

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

0xB10C commented Jan 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jan 23, 2024

Uh oh!

Uh oh!

DrahtBot commented Jan 18, 2024 •

edited

Loading

remyers Jan 20, 2024 •

edited

Loading

0xB10C Jan 18, 2024 •

edited

Loading

remyers Jan 18, 2024 •

edited

Loading

murchandamus Jan 19, 2024 •

edited

Loading

0xB10C commented Jan 20, 2024 •

edited

Loading