Skip to content

Force distinct destinations in CWallet::CreateTransaction #12419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
promag wants to merge 1 commit into from
Closed

Force distinct destinations in CWallet::CreateTransaction #12419

promag wants to merge 1 commit into from

Conversation

@promag
Copy link
Contributor

@promag promag commented Feb 13, 2018 •
edited by DrahtBot
Loading

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

The error is also raised in the UI:
screen shot 2018-02-13 at 01 17 49

@promag promag changed the title wallet: Force distinct destinations in CWallet::CreateTransaction Force distinct destinations in CWallet::CreateTransaction Feb 13, 2018
@promag
Copy link
Contributor Author

promag commented Feb 13, 2018

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
Copy link
Member

laanwj commented Feb 13, 2018

Concept ACK. I think this should be enforced consistently.

@promag
Copy link
Contributor Author

promag commented Feb 13, 2018

Can you be more specific @laanwj?

@laanwj
Copy link
Member

laanwj commented Feb 13, 2018

Huh? No, I mean that exactly what this PR does, checking it in CreateTransaction is good.

@promag
Copy link
Contributor Author

promag commented Feb 13, 2018

Ah ok, misunderstood you.

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Feb 17, 2018

utACK e82d179

@Sjors
Copy link
Member

Sjors commented Feb 20, 2018

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

schermafbeelding 2018-02-20 om 16 46 18

schermafbeelding 2018-02-20 om 16 46 42

@promag promag mentioned this pull request Feb 26, 2018
Closed
@promag
Copy link
Contributor Author

promag commented Mar 6, 2018

@Sjors I guess we could remove the old check?

@promag
Copy link
Contributor Author

promag commented Mar 6, 2018

Should all checks for duplicate addresses be removed?

@Sjors
Copy link
Member

Sjors commented Mar 6, 2018

@promag I suggest reusing the original warning.

@Empact
Copy link
Contributor

Empact commented May 21, 2018

Concept ACK, agree the original warning is more clear.

@DrahtBot DrahtBot closed this Jul 20, 2018
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 20, 2018

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 DrahtBot reopened this Jul 20, 2018
@promag promag mentioned this pull request Jul 30, 2018
Merged
@ken2812221
Copy link
Contributor

ken2812221 commented Sep 18, 2018

Concept ACK

@practicalswift practicalswift mentioned this pull request Oct 22, 2018
Closed
@meshcollider
Copy link
Contributor

meshcollider commented Nov 11, 2018 •
edited
Loading

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?

@promag promag mentioned this pull request Nov 20, 2018
Merged
@promag promag force-pushed the 2018-02-distinct-destinations branch from e82d179 to 3ba0298 Compare May 21, 2019 11:23
@promag
wallet: Force distinct destinations in CWallet::CreateTransaction
04f45ea 
Check that all transaction destinations, including change address
if specified, are distinct.
@promag promag force-pushed the 2018-02-distinct-destinations branch from 3ba0298 to 04f45ea Compare May 21, 2019 11:33
@promag
Copy link
Contributor Author

promag commented Jun 27, 2019

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

bitcoin/test/functional/wallet_groups.py

Lines 76 to 83 in 3077f11

# Fill node2's wallet with 10000 outputs corresponding to the same
# scriptPubKey
for i in range(5):
raw_tx = self.nodes[0].createrawtransaction([{"txid":"0"*64, "vout":0}], [{addr2[0]: 0.05}])
tx = FromHex(CTransaction(), raw_tx)
tx.vin = []
tx.vout = [tx.vout[0]] * 2000
funded_tx = self.nodes[0].fundrawtransaction(ToHex(tx))

@promag promag closed this Jun 27, 2019
@promag promag deleted the 2018-02-distinct-destinations branch June 27, 2019 10:59
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@promag @laanwj @jonasschnelli @Sjors @Empact @DrahtBot @ken2812221 @meshcollider @fanquake