S3RK

Results 67 comments of S3RK

I think there is a silent merge conflict with #29325 Otherwise, to the best of my understanding, the code is implemented according to [BIP-326](https://github.com/bitcoin/bips/blob/master/bip-0326.mediawiki) and LGTM

@luke-jr do you want to have the "use_txids" for all addresses throughout the whole blockchain or only for a subset of them?

I'm still not convinced about the approach of storing programmatically generated data in the address book, which is supposed to store user entered data. For that reason I'm leaning Approach...

Concept ACK. I think we should definitely support the described use case. For the approach, I think we already have some implementation ready for the second alternative you mentioned. #26728...

How would it work if coinselection produced result without change output?

Approach ACK I think setting change related params to 0 in case where we reuse existing output is the correct way to go.

> We don't expect users to have an address as both a normal output and as the change address, in the same way that we don't expect users to have...

> There isn't, but it also doesn't make sense to me for someone to want to do that. Why specify an existing amount instead of just dropping it and letting...

Approach ACK Commit message in 0a31864bbe6ab8cbc081f56ea490e123bf25ed7c says that `GetChange()` relied to special meaning of `change_cost == 0`, but actually it was `GetSelectionWaste()` function which was in fault.

Happy to reack, but you need to fix clang-tidy first ``` wallet/test/coinselector_tests.cpp:893:43: error: argument name 'current_fee' in comment does not match parameter name 'fee' [bugprone-argument-comment,-warnings-as-errors] 893 | add_coin(1 * COIN,...