tracing: fix `coin_selection:aps_create_tx_internal` calling logic #25003

theStack · 2022-04-27T14:44:25Z

According to the documentation, the tracepoint coin_selection:aps_create_tx_internal "Is called when the second CreateTransactionInternal with Avoid Partial Spends enabled completes."

Currently it is only called if the second call to CreateTransactionInternal succeeds, i.e. the third parameter is always true and we don't get notified in the case that it fails. This PR fixes this by moving the tracepoint call and the use_aps boolean variable outside the if body.

mzumsande · 2022-04-27T15:19:24Z

Good catch!

I'm not sure about the original intention (maybe also the documentation is imprecise?), but if aps_create_tx_internal is called irrespective of the second CreateTransactionInternal result now, I don't see a point in having the attempting_aps_create_tx tracepoint anymore.

theStack · 2022-04-27T16:03:31Z

I'm not sure about the original intention (maybe also the documentation is imprecise?), but if aps_create_tx_internal is called irrespective of the second CreateTransactionInternal result now, I don't see a point in having the attempting_aps_create_tx tracepoint anymore.

Oh, I didn't see that we have this attempting_aps_create_tx tracepoint to be honest. So there is already an implicit way to see if the second CreateTransactionInternal call failed, and it could indeed be that the documentation is not clear enough. What made me sceptical about the conditional tracepoint call though is the fact that there is a "succeeded" parameter (where currently the result of the first CreateTransactionInternal is passed, i.e. always "true").

I'd conclude then that we can remove either

the attempting_aps_create_tx tracepoint (if this PR is preferred), or
the success parameter from the aps_create_tx_internal tracepoint (with an according documentation update, e.g. "is called when completed successfully")

achow101 · 2022-04-27T16:27:20Z

I'm not sure about the original intention (maybe also the documentation is imprecise?), but if aps_create_tx_internal is called irrespective of the second CreateTransactionInternal result now, I don't see a point in having the attempting_aps_create_tx tracepoint anymore.

It helps us know whether the selected_coins tracepoint (that will happen within CreateTransactionInternal) is for the APS CreateTransactionInternal without having to look backwards when aps_create_tx_internal is seen.

achow101 · 2022-04-27T16:27:57Z

ACK 5849fa0

DrahtBot · 2022-04-28T02:03:41Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25218 (refactor: introduce generic 'Result' classes and connect it to CreateTransaction and GetNewDestination 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.

furszy

Code ACK 5849fa0

furszy · 2022-05-04T14:49:46Z

src/wallet/spend.cpp

            if (use_aps) {
                tx = tx2;
                nFeeRet = nFeeRet2;
                nChangePosInOut = nChangePosInOut2;
            }
        }
+        TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, res2, nFeeRet2, nChangePosInOut2);


small off-topic question (that has nothing to do with this PR) is why this trace context is called "coin_selection" when CreateTransactionInternal could fail for other reasons (like missing solving data, signing, etc).

0xB10C

Changes look good to me, however I'm not too involved with coin selection.

Might make sense for @xekyo to review/comment on this PR too. He's using the coin_selection tracepoints.

murchandamus · 2022-05-10T17:38:02Z

I'm not sure about the original intention (maybe also the documentation is imprecise?), but if aps_create_tx_internal is called irrespective of the second CreateTransactionInternal result now, I don't see a point in having the attempting_aps_create_tx tracepoint anymore.

It helps us know whether the selected_coins tracepoint (that will happen within CreateTransactionInternal) is for the APS CreateTransactionInternal without having to look backwards when aps_create_tx_internal is seen.

If I understand this correctly, the way this tracepoint is activated, it would log a result as having been produced per APS, whenever the APS result is preferred. Among other things, this would include if there are two equivalent (or even matching) solutions, or if there wasn't any partial-spend concern with the original solution in the first place.
This results in three different concerns:

This feels like a potential footgun for someone trying to understand coin selection outcomes. They might want to know a) how often partial-spends even occurred in the first place, and b) how often the opportunistic APS protected them from it. From what I understand, they cannot actually learn either of these numbers. Instead, a subscriber of the tracepoint would get an inflated sense of how often they were protected from partial spends.
Since the APS solution is preferred even when it has a slightly worse fee, when the general coin selection result and the opportunistic APS result differ, the APS result may actually have a worse waste score and get preferred for the transaction building, disimproving the outcome for the user even when there was no partial-spend in the original solution.
The coin selection is run twice while that might be completely unnecessary in many cases.

All together, I think both the design for this tracepoint and the design for the opportunistic APS may have the same solution: only run the opportunistic APS if the original selection result includes a partial spend: this would speed up coin selection, remove the potential for disimproving the selection result, and give useful numbers to the subscribers of the tracepoint.

While I think this PR improves over the original deployment, if I got the above right, it might be better to remove this tracepoint due to not providing a clear signal instead.

murchandamus · 2022-05-10T20:36:44Z

I guess some of those comments would have better fit on #24644, sorry. cc: @achow101 ^

achow101 · 2022-05-16T19:37:15Z

it might be better to remove this tracepoint due to not providing a clear signal instead.

While I agree the signal is not particularly clear and this could result in some misleading information, the tracepoint is still necessary as sometimes the APS solution is used and is different from the original solution, so it would be even more misleading if we were to only record the original.

However, I also agree that we should change how the opportunistic APS works, but that is outside the scope of this PR.

murchandamus · 2022-05-16T20:30:07Z

Okay, will re-review.

murchandamus · 2022-05-16T20:36:04Z

ACK 5849fa0, but we really should fix both the APS and the tracepoints logging logic.

0xB10C · 2022-05-16T22:18:01Z

Test interface_usdt_coinselection.py passes for 5849fa0.

theStack · 2022-05-17T14:05:51Z

Rebased on master (necessary since #20640 has been merged).

According to the documentation, the tracepoint `coin_selection:aps_create_tx_internal` "Is called when the second `CreateTransactionInternal` with Avoid Partial Spends enabled completes." Currently it is only called if the second call to `CreateTransactionInternal` succeeds, i.e. the third parameter is always `true` and we don't get notified in the case that it fails. Fix this by introducing a boolean variable for the result of the call and moving the tracepoint call outside the if body.

murchandamus · 2022-05-17T16:06:50Z

re-ack: 6b63673

furszy

re-ACK 6b63673

furszy · 2022-05-04T14:49:46Z

src/wallet/spend.cpp

            if (use_aps) {
                tx = tx2;
                nFeeRet = nFeeRet2;
                nChangePosInOut = nChangePosInOut2;
            }
        }
+        TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, res2, nFeeRet2, nChangePosInOut2);


small off-topic question (that has nothing to do with this PR) is why this trace context is called "coin_selection" when CreateTransactionInternal could fail for other reasons (like missing solving data, signing, etc).

It's called coin selection because the tracepoint primarily measures the results of performing coin selection.

furszy · 2022-05-19T14:07:44Z

src/wallet/spend.cpp

@@ -993,12 +993,13 @@ std::optional<CreatedTransactionResult> CreateTransaction(
        tmp_cc.m_avoid_partial_spends = true;
        bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results
        std::optional<CreatedTransactionResult> txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, error2, tmp_cc, fee_calc_out, sign);
+        // 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};


nit: could be written as:

const bool use_aps = txr_grouped && (txr_grouped->fee <= txr_ungrouped->fee + wallet.m_max_aps_fee);

(removing the optional has_value() call).

furszy · 2022-05-19T14:10:13Z

src/wallet/spend.cpp

+        // 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 : 0);


same code nit here, I don't see a reason to use txr_grouped.has_value() when there is a cleaner way of doing it with the bool() operator (by default std::optional is set to nullopt).

achow101 · 2022-05-26T17:07:09Z

ACK 6b63673

…al ca…

fanquake requested review from 0xB10C and achow101 April 27, 2022 14:45

theStack force-pushed the 202204-tracing_fix_coin_selection-aps_create_tx_internal_log branch from 3c86ed3 to 5849fa0 Compare April 27, 2022 14:50

DrahtBot added the Wallet label Apr 27, 2022

This was referenced Apr 28, 2022

[Draft / POC] Silent Payments #24897

Closed

wallet: return error msg for "too-long-mempool-chain" #24845

Merged

0xB10C mentioned this pull request Apr 28, 2022

wallet: add tracepoints and algorithm information to coin selection #24644

Merged

furszy approved these changes May 4, 2022

View reviewed changes

0xB10C reviewed May 4, 2022

View reviewed changes

theStack mentioned this pull request May 16, 2022

wallet, refactor: return out-params of CreateTransaction() as optional struct #20640

Merged

murchandamus mentioned this pull request May 16, 2022

Coin Selection tracepoint overreports use of APS #25150

Open

DrahtBot added the Needs rebase label May 17, 2022

theStack force-pushed the 202204-tracing_fix_coin_selection-aps_create_tx_internal_log branch from 5849fa0 to 8148f0d Compare May 17, 2022 14:04

DrahtBot removed the Needs rebase label May 17, 2022

theStack force-pushed the 202204-tracing_fix_coin_selection-aps_create_tx_internal_log branch from 8148f0d to 6b63673 Compare May 17, 2022 14:12

furszy approved these changes May 19, 2022

View reviewed changes

DrahtBot mentioned this pull request May 26, 2022

refactor: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination #25218

Merged

achow101 merged commit a0e8aff into bitcoin:master May 26, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 28, 2022

Merge bitcoin#25003: tracing: fix coin_selection:aps_create_tx_intern…

b8ba7a8

…al ca…

bitcoin locked and limited conversation to collaborators May 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tracing: fix `coin_selection:aps_create_tx_internal` calling logic #25003

tracing: fix `coin_selection:aps_create_tx_internal` calling logic #25003

theStack commented Apr 27, 2022 •

edited

Loading

mzumsande commented Apr 27, 2022 •

edited

Loading

theStack commented Apr 27, 2022 •

edited

Loading

achow101 commented Apr 27, 2022

achow101 commented Apr 27, 2022

DrahtBot commented Apr 28, 2022 •

edited

Loading

furszy left a comment

furszy May 4, 2022

0xB10C left a comment

murchandamus commented May 10, 2022 •

edited

Loading

murchandamus commented May 10, 2022

achow101 commented May 16, 2022

murchandamus commented May 16, 2022

murchandamus commented May 16, 2022 •

edited

Loading

0xB10C commented May 16, 2022 •

edited

Loading

theStack commented May 17, 2022

murchandamus commented May 17, 2022

furszy left a comment

furszy May 4, 2022

achow101 May 26, 2022

furszy May 19, 2022 •

edited

Loading

furszy May 19, 2022

achow101 commented May 26, 2022

tracing: fix coin_selection:aps_create_tx_internal calling logic #25003

tracing: fix coin_selection:aps_create_tx_internal calling logic #25003

Conversation

theStack commented Apr 27, 2022 • edited Loading

mzumsande commented Apr 27, 2022 • edited Loading

theStack commented Apr 27, 2022 • edited Loading

achow101 commented Apr 27, 2022

achow101 commented Apr 27, 2022

DrahtBot commented Apr 28, 2022 • edited Loading

Conflicts

furszy left a comment

Choose a reason for hiding this comment

furszy May 4, 2022

Choose a reason for hiding this comment

0xB10C left a comment

Choose a reason for hiding this comment

murchandamus commented May 10, 2022 • edited Loading

murchandamus commented May 10, 2022

achow101 commented May 16, 2022

murchandamus commented May 16, 2022

murchandamus commented May 16, 2022 • edited Loading

0xB10C commented May 16, 2022 • edited Loading

theStack commented May 17, 2022

murchandamus commented May 17, 2022

furszy left a comment

Choose a reason for hiding this comment

furszy May 4, 2022

Choose a reason for hiding this comment

achow101 May 26, 2022

Choose a reason for hiding this comment

furszy May 19, 2022 • edited Loading

Choose a reason for hiding this comment

furszy May 19, 2022

Choose a reason for hiding this comment

achow101 commented May 26, 2022

tracing: fix `coin_selection:aps_create_tx_internal` calling logic #25003

tracing: fix `coin_selection:aps_create_tx_internal` calling logic #25003

theStack commented Apr 27, 2022 •

edited

Loading

mzumsande commented Apr 27, 2022 •

edited

Loading

theStack commented Apr 27, 2022 •

edited

Loading

DrahtBot commented Apr 28, 2022 •

edited

Loading

murchandamus commented May 10, 2022 •

edited

Loading

murchandamus commented May 16, 2022 •

edited

Loading

0xB10C commented May 16, 2022 •

edited

Loading

furszy May 19, 2022 •

edited

Loading