Force distinct destinations in CWallet::CreateTransaction #12419

pull promag wants to merge 1 commits into bitcoin:master from promag:2018-02-distinct-destinations changing 2 files +14 −1

promag commented at 1:20 AM on February 13, 2018: member

Check that all transaction destinations, including change address if specified, are distinct.

The error is also raised in the UI: <img width="922" alt="screen shot 2018-02-13 at 01 17 49" src="https://user-images.githubusercontent.com/3534524/36128715-c25a7720-105b-11e8-8b07-4cc4836c0b52.png">
promag renamed this:
~~wallet: Force distinct destinations in CWallet::CreateTransaction~~
Force distinct destinations in CWallet::CreateTransaction
on Feb 13, 2018
fanquake added the label Wallet on Feb 13, 2018
promag commented at 1:21 AM on February 13, 2018: member

A similar check exists in sendmany but at the moment the errors are different. Not sure it should change there or try to make this equal (the problem is that the errors in CWallet are translated and RPC errors aren't).
laanwj commented at 10:23 AM on February 13, 2018: member

Concept ACK. I think this should be enforced consistently.
promag commented at 11:19 AM on February 13, 2018: member

Can you be more specific @laanwj?
laanwj commented at 1:32 PM on February 13, 2018: member

Huh? No, I mean that exactly what this PR does, checking it in CreateTransaction is good.
promag commented at 3:13 PM on February 13, 2018: member

Ah ok, misunderstood you.
jonasschnelli commented at 7:46 AM on February 17, 2018: contributor

utACK e82d17991418bdc3cfe60407a66735c265f8cd21
Sjors commented at 3:47 PM on February 20, 2018: member

Concept ACK, but now there's two different error messages depending on whether the duplicate is a change address or a regular destination:

<img width="627" alt="schermafbeelding 2018-02-20 om 16 46 18" src="https://user-images.githubusercontent.com/10217/36433814-c0360c0e-165d-11e8-9f58-f22b9e144f6c.png"> <img width="645" alt="schermafbeelding 2018-02-20 om 16 46 42" src="https://user-images.githubusercontent.com/10217/36433815-c0573690-165d-11e8-8cec-63aab8148e6a.png">
promag commented at 4:43 PM on March 6, 2018: member

@Sjors I guess we could remove the old check?
promag commented at 5:57 PM on March 6, 2018: member

Should all checks for duplicate addresses be removed?
Sjors commented at 10:13 PM on March 6, 2018: member

@promag I suggest reusing the original warning.
Empact commented at 6:44 AM on May 21, 2018: member

Concept ACK, agree the original warning is more clear.
DrahtBot closed this on Jul 20, 2018
DrahtBot commented at 8:29 PM on July 20, 2018: member

The last travis run for this pull request was 157 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
DrahtBot reopened this on Jul 20, 2018
ken2812221 commented at 1:44 PM on September 18, 2018: contributor

Concept ACK
meshcollider commented at 1:48 AM on November 11, 2018: contributor

This looks good but the comments above need addressing (feel free to cherry-pick from my 201811_duplicate_dest_check branch)

EDIT: This will also prevent fundrawtransaction from funding a transaction with duplicate outputs, correct? Is that what we want? If someone has gone to the effort of creating a transaction with duplicate outputs manually, maybe we should allow it there?
DrahtBot added the label Needs rebase on Apr 27, 2019
promag force-pushed on May 21, 2019

wallet: Force distinct destinations in CWallet::CreateTransaction

Check that all transaction destinations, including change address
if specified, are distinct.

04f45ea52c

promag force-pushed on May 21, 2019
DrahtBot removed the label Needs rebase on May 21, 2019
promag commented at 10:59 AM on June 27, 2019: member

Closing for now, as @meshcollider mentions, this approach breaks the following case:

https://github.com/bitcoin/bitcoin/blob/3077f11dadffbc8f2575449fc0177c91a9c3e046/test/functional/wallet_groups.py#L76-L83
promag closed this on Jun 27, 2019
promag deleted the branch on Jun 27, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels