Skip to content
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.

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

If -spkreuse=0, ensure transactions in mempool always have unique scriptPubKeys #9749

Closed
wants to merge 3 commits into from

Conversation

@luke-jr
Copy link
Member

luke-jr commented Feb 13, 2017

Exceptions:

  • Multiple inputs in the same transaction are allowed to spend against the same scriptPubKey
  • The same scriptPubKey may be used in the mempool as both first an output, and then spent in a later transaction's input

Changes since original 2013 patch (pre-squashed):

  • Refactor: Move CScript::ScriptPubkeyReuseHash to ScriptHashkey(CScript) in txmempool
  • Update mempool duplicate-scriptPubKey limiting with C++11 and misc formatting improvements
  • Bugfix: Use bitwise operators for mempool SPK states
  • Use CValidationState for SPK reuse rejections
  • Move mapTxSPK to CTxMemPoolEntry.mapSPK
  • Make SPK reuse filtering optional (use -spkreuse)

Known issues:

  • This breaks RBF in most usage scenarios.
  • Someone could watch for transactions and spam dust to block them on nodes using this.
@luke-jr
Copy link
Member Author

luke-jr commented Feb 13, 2017

@sdaftuar pointed out on IRC that someone could watch the p2p network and screw up people by sending dust to SPKs they see. I also noticed this breaks common RBF usage since the SPK conflict rejects the transaction before it can be replaced.

To address both of these, I refactored the logic so that it treats non-inherent (that is, within the same tx) SPK conflicts in the same manner as TxIn conflicts. Although in the SPK case, the RBF optin flag is ignored, and a conflict in the parents of the transaction don't cause a DoS ban.

@morcos
Copy link
Member

morcos commented Feb 13, 2017

As mentioned on IRC, I'm opposed to this feature. Would not object to more optional safeguards against reuse in the wallet code though.

@luke-jr
Copy link
Member Author

luke-jr commented Feb 14, 2017

And as mentioned on IRC, I don't like the idea of changing Core's direction to where it is no longer a reference implementation, but a specific political agenda to the exclusion of others. If you don't like the feature, just don't use it.

@sdaftuar
Copy link
Member

sdaftuar commented Feb 14, 2017

@luke-jr Please explain in the pull request, and ideally in code comments, what this patch is intended to do and why. Concept NACK policy change PRs that are made without providing motivation.

@luke-jr
Copy link
Member Author

luke-jr commented Feb 17, 2017

@luke-jr luke-jr force-pushed the luke-jr:unique_spk_mempool branch from 2d5aa77 to fe4be7b Mar 3, 2017
@luke-jr luke-jr force-pushed the luke-jr:unique_spk_mempool branch 2 times, most recently Aug 27, 2017
luke-jr added 3 commits Feb 13, 2017
…iptPubKeys

Exceptions:
- Multiple inputs in the same transaction are allowed to spend against the same scriptPubKey
- The same scriptPubKey may be used in the mempool as both first an output, and then spent in a later transaction's input
@luke-jr luke-jr force-pushed the luke-jr:unique_spk_mempool branch to 9b75ab5 Aug 29, 2017
@ken2812221
Copy link
Contributor

ken2812221 commented Sep 10, 2018

Concept NACK. For now, many exchanges are using a single address for an account.

@luke-jr
Copy link
Member Author

luke-jr commented Sep 10, 2018

Such exchanges are broken. That's all the more reason to enforce this.

@junderw
Copy link

junderw commented Sep 10, 2018

It's not that exchanges are broken. It's that the exchange users want to "set it and forget it." with everything.

They have a bot that does automatic arbitrage? Deposit address for exchange A, then B, set in a config file. Set it and forget it.

They send a lot of money from one exchange in one country to another in a different country? Not a bot trader? But Exchange B has all these hoops to go through every time you add a new withdrawal address: 2FA, SMS, EMAIL, draw blood, inform next of kin, sacrifice goat, do 28 jumping jacks, etc... so the user wants to just... Set it and forget it.

We were switching out addresses and refusing to recognize deposits to old addresses...

But then tons of customer support tickets later, the customers had spoken and they want one "set it and forget it" address.......

@luke-jr just out of curiosity, what would you think if Exchanges tried to push towards BIP47 payment codes only?

@luke-jr
Copy link
Member Author

luke-jr commented Sep 10, 2018

@junderw Such a thing is undefined behaviour / unsupported. It shouldn't be done. Exchanges don't have to actively block it, but they shouldn't encourage it either. (There's no reason generating a new address needs to have hoops though?)

(I don't recommend implementing BIP 47, but that's off-topic here.)

@junderw
Copy link

junderw commented Sep 10, 2018

There's no reason generating a new address needs to have hoops though?

The hoops are for registering withdrawal addresses. It has become a sort of standard for exchanges to "register" withdrawal addresses. Then add extra steps to make adding a new withdrawal address. This way even if someone logs in to your account with username + password + 2FA somehow, they need some 3rd (or 4th, 5th 18th... lol) factor to add withdrawal addresses.

Because of this, even if our exchange allowed them to generate new addresses on every viewing of the deposit page, this would be a new address they need to register with all the other exchanges withdrawals...

I was thinking maybe a BIP47 payment code would be a good "set it and forget it" replacement where we could take the heavy lifting of generating new addresses and the user doesn't have to keep registering withdrawal addresses... but looking into it, BIP47 has an insane overhead and doesn't really do much for privacy... (though to be honest, privacy is kinda hard when we have a copy of your passport, so most exchanges put these things on a low priority)

So unfortunately, until there is some better way for exchanges to manage this, I would like to respectfully Concept NACK.

I would be open to it if there was an alternative to BIP47 and it was widely adopted among exchanges.

@jtimon
Copy link
Member

jtimon commented Sep 10, 2018

@junderw
Copy link

junderw commented Sep 10, 2018

There is a better way for exchanges to manage this: they can simply require
the user to provide an address every time they want to withdraw.

This method results in more successful account hacks (with actual lost funds) compared to registering addresses.

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 12, 2018

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.
@DrahtBot DrahtBot closed this Dec 12, 2018
@laanwj laanwj removed the Needs rebase label Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.