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

luke-jr commented at 0:09 am on February 13, 2017: member

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 commented at 5:37 am on February 13, 2017: member

@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 commented at 6:44 pm on February 13, 2017: member

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 commented at 3:42 am on February 14, 2017: member

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 commented at 1:43 pm on February 14, 2017: member

@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.

fanquake added the label Mempool on Feb 14, 2017

fanquake added the label Validation on Feb 14, 2017

luke-jr commented at 8:13 pm on February 17, 2017: member

Context: https://bitcointalk.org/index.php?topic=334316.0;viewResults

luke-jr force-pushed on Mar 3, 2017

luke-jr force-pushed on Aug 27, 2017

luke-jr force-pushed on Aug 29, 2017

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

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

caf9b0d661

txmempool: Store pointers to transactions claiming SPKs 6f19051ff5

Treat SPK conflicts the same as RBF-optin TxIn conflicts (except never DoS ban) 9b75ab5b39

luke-jr force-pushed on Aug 29, 2017

MarcoFalke added the label Needs rebase on Jun 6, 2018

ken2812221 commented at 2:39 am on September 10, 2018: contributor

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

luke-jr commented at 3:52 am on September 10, 2018: member

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

junderw commented at 6:10 am on September 10, 2018: contributor

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 commented at 8:08 am on September 10, 2018: member

@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 commented at 9:16 am on September 10, 2018: contributor

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 commented at 9:44 am on September 10, 2018: contributor

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.

On Mon, 10 Sep 2018, 11:18 Jonathan Underwood, notifications@github.com wrote:

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.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9749#issuecomment-419845302, or mute the thread https://github.com/notifications/unsubscribe-auth/AA9jSqiTaDeHkxhQy3-hGWC_4S0kssdDks5uZi5vgaJpZM4L-rdw .

junderw commented at 10:17 am on September 10, 2018: contributor

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 added the label Up for grabs on Dec 12, 2018

DrahtBot commented at 7:23 pm on December 12, 2018: member

DrahtBot closed this on Dec 12, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke locked this on Dec 16, 2021

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