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.
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.
I'm not sure about the original intention (maybe also the documentation is imprecise?), but if
aps_create_tx_internalis called irrespective of the secondCreateTransactionInternalresult now, I don't see a point in having theattempting_aps_create_txtracepoint 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
attempting_aps_create_tx tracepoint (if this PR is preferred), oraps_create_tx_internal tracepoint (with an according documentation update, e.g. "is called when completed successfully")
I'm not sure about the original intention (maybe also the documentation is imprecise?), but if
aps_create_tx_internalis called irrespective of the secondCreateTransactionInternalresult now, I don't see a point in having theattempting_aps_create_txtracepoint 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.
ACK 5849fa0ed9049468d7a350e289069522b3a21dbf
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
1007 | tx = tx2;
1008 | nFeeRet = nFeeRet2;
1009 | nChangePosInOut = nChangePosInOut2;
1010 | }
1011 | }
1012 | + 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).
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.
Code ACK 5849fa0e
I'm not sure about the original intention (maybe also the documentation is imprecise?), but if
aps_create_tx_internalis called irrespective of the secondCreateTransactionInternalresult now, I don't see a point in having theattempting_aps_create_txtracepoint 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_internalis 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:
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.
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.
Okay, will re-review.
ACK 5849fa0ed9049468d7a350e289069522b3a21dbf, but we really should fix both the APS and the tracepoints logging logic.
Test interface_usdt_coinselection.py passes for 5849fa0ed9049468d7a350e289069522b3a21dbf.
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.
re-ack: 6b636730f4befee39d57fcfd51298f3015dbf563
992 | @@ -993,12 +993,13 @@ std::optional<CreatedTransactionResult> CreateTransaction( 993 | tmp_cc.m_avoid_partial_spends = true; 994 | bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results 995 | std::optional<CreatedTransactionResult> txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, error2, tmp_cc, fee_calc_out, sign); 996 | + // if fee of this alternative one is within the range of the max fee, we use this one 997 | + 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).
992 | @@ -993,12 +993,13 @@ std::optional<CreatedTransactionResult> CreateTransaction( 993 | tmp_cc.m_avoid_partial_spends = true; 994 | bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results 995 | std::optional<CreatedTransactionResult> txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, error2, tmp_cc, fee_calc_out, sign); 996 | + // if fee of this alternative one is within the range of the max fee, we use this one 997 | + const bool use_aps{txr_grouped.has_value() ? (txr_grouped->fee <= txr_ungrouped->fee + wallet.m_max_aps_fee) : false}; 998 | + TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, txr_grouped.has_value(), 999 | + 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).
re-ACK 6b636730
ACK 6b636730f4befee39d57fcfd51298f3015dbf563