Skip to content
This repository has been archived by the owner on Jun 3, 2018. It is now read-only.

Make protected transactions mandatory on UAHF chain (and enforce SCRIPT_VERIFY_STRICTENC) #17

Merged
merged 3 commits into from
Jul 25, 2017

Conversation

ftrader
Copy link
Contributor

@ftrader ftrader commented Jul 24, 2017

This enforces the use of SIGHASH_FORKID and SCRIPT_VERIFY_STRICTENC on
by UAHF-compatible clients once the fork has activated.

It makes the OP_RETURN protection obsolete, but it is not removed in order
to avoid a hard fork change w.r.t. existing deployments.

The strengthening of replay protections is a feature requested by miners
and exchanges to protect Bitcoin users from accidental loss and theft of
their Bitcoin Cash.

…PT_VERIFY_STRICTENC)

This enforces the use of SIGHASH_FORKID and SCRIPT_VERIFY_STRICTENC on
by UAHF-compatible clients once the fork has activated.

It makes the OP_RETURN protection obsolete, but it is not removed in order
to avoid a hard fork change w.r.t. existing deployments.

The strengthening of replay protections is a feature requested by miners
and exchanges to protect Bitcoin users from accidental loss and theft of
their Bitcoin Cash.
@NiKiZe
Copy link

NiKiZe commented Jul 24, 2017

This means that no existing clients at all will be compatible with the fork? I've asked about this before and then got the clear answer that it will stay compatible.

@digitsu
Copy link

digitsu commented Jul 24, 2017

I don't think this should be mandatory. If you want to protect an outgoing txn then by all means do so. But forcing clients to do so excludes most hardware wallets from being used on BitcoinCash out of the box. Besides, a replayed legacy txn being received only has the effect of losing potential legacy BTC for the sender. As the onus is on the sender to not do something that can potentially lose them BTC (and not BitcoinCash as the comment implies) opt-in protection is sufficient.

@gandrewstone
Copy link

I disagree with this change. ETH/ETC replay made a momentary news splash and then it was no big deal. I am too busy working on code to enumerate all the reasons but I'm sure other people will do so.

And we are 8 days to the split and still contemplating design changes!?

@ftrader
Copy link
Contributor Author

ftrader commented Jul 24, 2017

We have weighed this decision before making this change proposal, which I believe is in the interest of existing Bitcoin users. All this means is that businesses and users who have not upgraded their software and systems will be protected from replay ab initio.

We recognize that not everyone will be ready to support the new format on August 1 2017, but we believe that since the SIGHASH_FORKID protection is based on BIP143, a large number of clients can be adapted relatively soon to sign in a compatible way.

This means that no existing clients at all will be compatible with the fork?

Electrum Cash will support the protections already, and we are in touch with other client (and wallet) developers to ensure adoption.

@gandrewstone
Copy link

Who is "we"? I am concerned that there seems to be no formal decision making process here.

@deadalnix
Copy link
Collaborator

This has been an almost unanimous reaction from all market actors that talked to us. Miners, payment processors and merchants. This is obviously not 100% of the actors in the space, but I think it is fair to assume actors that reached to us are among the most aware of what's going on than average, so if this is a problem to them, this will be a bigger problem for others.

@digitsu as you mention hardware wallet, know that trezor has been open to add BCC support, and is also asking for replay protection. I think playing nicely with people like trezor is in the best interest of both parties.

@gandrewstone The change is made as a soft fork to avoid panic upgrade a few day before the date. It is why we are keeping the OP_RETURN protection as well, while it is not really needed anymore with this spec change. There is no need for a panic upgrade.

the following are true in combination:
- the nHashType has bit 6 set (mask 0x40)
- adding a magic 'fork id' value to the nHashType before the hash is
calculated allows a successful signature verification as per REQ-6-3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I thought it would be simpler to rewrite it as a single sentence.

~~constraint introduced by REQ-6-1.~~

NOTE 3: (DEPRECATED) ~~If bit 6 is not set, only the unmodified nHashType will be used~~
~~to compute the hash and verify the signature.~~
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumes this strikes the text. Not sure how I feel about this because the old text is in git history anyway.

Copy link
Contributor Author

@ftrader ftrader Jul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to leave the note numbering, but I wanted to show clearly that these notes are no longer applicable.

Strikethrough seemed the best option.

Keeping the note numbering and just adding means I don't have to wonder in future when someone is talking about a number. I know that it's obsolete.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'm good with that.

consider such transactions invalid).
Once the fork has activated, a transaction shall be deemed invalid if
its nHashType does not have bit 6 set (SIGHASH_FORKID, mask 0x40) and it
is not using the new digest algorithm described in REQ-6-3.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the removed paragraph is clearer so maybe it should be tweaked instead of rewritten to express this is mandatory now ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really mind - I can restore something like a tweaked version of the original one, and then you can see if you like it better.

@gandrewstone
Copy link

I'm getting very concerned with the lack of transparency here. This is one of the major issues that many BUers had with Core, and yet here it is happening again. Let us create a decision making process before making major 11th hour decisions.

@digitsu
Copy link

digitsu commented Jul 24, 2017 via email

@ftrader
Copy link
Contributor Author

ftrader commented Jul 24, 2017

You going to support Keepkey and Ledger as well?

Yes, if they want to support Bitcoin Cash we absolutely will support them.

@digitsu
Copy link

digitsu commented Jul 24, 2017 via email

@ftrader
Copy link
Contributor Author

ftrader commented Jul 24, 2017

Better to have people wait a little while until their wallet provides an update, rather than losing their Bitcoin Cash because someone created a replayable transaction, whether by accident or intent.

@HostFat
Copy link

HostFat commented Jul 24, 2017

This can be already useful to extract private keys, offline, from many kind of seeds
https://github.com/iancoleman/bip39

As I wrote on slack, I think that developing (forking already working project) a javascript offline signing tool will easily make it possible to already support a larger number of cases.

https://github.com/OutCast3k/coinbin

Maybe there are others.

This isn't strictly related to this issue, but if there is some trusty javascript dev that it is reading now, maybe he can do it.

@digitsu
Copy link

digitsu commented Jul 24, 2017 via email

@deadalnix
Copy link
Collaborator

Sure but correct me if I’m wrong you can’t lose your Bitcoin Cash if you are using a bitcoin cash node or wallet. What you may lose is your segwitcoins.

Same works both way. If you use a Segwit Coin compatible software, you can't lose your Segwit Coin, but you can lose your Bitcoin Cash. This is creating a lot of confusion in the space. We are getting a lot of traction, so we need to act professionally. Creating chaos is fun, but doesn't foster trust.

@digitsu
Copy link

digitsu commented Jul 24, 2017 via email

@NiKiZe
Copy link

NiKiZe commented Jul 24, 2017

@deadalnix @ftrader If you merge this then it is a altcoin (and needs a port change) that can never become Bitcoin.
Indeed a clarification on how to make sure you should construct your transaction for:

  1. Spending only on BCC chain
  2. Spending only on legacy chain
  3. Spending the same transaction on both chains (this must be compatible with all existing clients, otherwise upgrading the network will take to long and cause unnecessary issues)

So better explanations and finalization is needed - not more confusion!
On top of that the more tolls that can be created for all 3 of the above scenarios the better.

@deadalnix
Copy link
Collaborator

So are you adding this replay protection in some hope that segwitcoin will reciprocate?

No this protection is being added because there is demand for it. It has been explained already, if you are going to ignore these explanation then this discussion is not going to lead anywhere.

@NiKiZe If you think such clarification is useful (I do think it is) then I advice you start working on it. Arguing here is not helping the clarification to write itself.

@ftrader
Copy link
Contributor Author

ftrader commented Jul 24, 2017

The wording Freetrader used is incorrect.

The wording I used was carefully chosen. Yes, people who lose their money because they had in in custody of third parties (who make mistakes or get defrauded) is a real possibility and one we'd like to eliminate by making replay impossible.

@prusnak
Copy link

prusnak commented Jul 24, 2017

It is in your coin's best interest to have replay protection. Implementing mandatory SIGHASH_FORKID in the hardware wallet firmware is very easy and we will release firmware update promptly.

Not having replay protection is much worse for Bitcoin Cash than it is for Bitcoin.

I honestly don’t care if anyone loses segwitcoin. I do care if people lost BitcoinCash.

@digitsu This is exactly what will happen if you do not implement proper replay protection.

@slush0
Copy link

slush0 commented Jul 24, 2017

Adding replay protection by using SIGHASH_FORKID is trivial for wallets, even for hardware wallets. We'll happily add it into firmware. But without proper replay protection, it will be complete mess and people will DO lose money on both chains. This will be definitely much worse problem for BCC than for BTC, because you'll be one to blame for this.

Please be responsible and use SIGHASH_FORKID. You'll make less enemies among developers, who'll otherwise need to spend huge effort to fix your poor architecture decisions on application level.

@bitcartel
Copy link

bitcartel commented Jul 24, 2017

BIP148 has no replay protection. Why is UAHF held to a different standard?

@HostFat
Copy link

HostFat commented Jul 24, 2017

@bitcartel
I think that this project can have higher standards than it :)

@roybadami
Copy link

Mandatory replay protection would make existing timelocked transactions unspendable on the BCC chain. I think that's enough reason not to do it.

@inaltoasinistra
Copy link

inaltoasinistra commented Jul 24, 2017

@digitsu What about make mandatory the SIGHASH_FORKID only for 1 month after the split?

So users can split their BCC and BTC makeing transactions with their legacy wallets for a while; and hardware wallets will work out of the box with Bitcoin Cash from September.

EDIT: also @roybadami issue is resolved if mandatory SIGHASH_FORKID is temporary

@digitsu
Copy link

digitsu commented Jul 24, 2017 via email

@slush0
Copy link

slush0 commented Jul 24, 2017

@digitsu As you only care about BCC, there's obviously no reason to advocate such protection to you. Things are different for normal users, who, surprisingly, might want to handle both coins at the same time.

You may want to play nicely with people and businesses, if you want it to succeed. Accordingly, you may expect more or less friendly integration of your project to existing infrastructure, like wallets and exchanges.

If you don't care what people or businesses wants, you should not be surprised that those people and businesses won't care about your project and won't give you appropriate support.

Currently, optional replay protection requires non-trivial changes in wallets, to not let people screw with theirs BTC and BCC holdings. We may put some effort into it to make it work smoothly for your chain. That's your motivation to help us make it happen.

Take it or leave it.

@JaredR26
Copy link

JaredR26 commented Jul 25, 2017

FYI, strong replay protection is absolutely necessary given BCC's different definition of anyone-can-spend. Without it, transactions that properly follow the rules of a different chain entirely will cause innocent users to lose BCC, simply for following the proper rules of that other chain. While I might not care enough to sell my BCC if it is a clean hardfork, if there's some way my normal, acceptable transactions on an unrelated chain can cause my BCC to be stolen, I'll simply sell my BCC to make sure that I gain something out of the farce. BCC needs to make sure that this cannot be done - even if someone manually modifies transactions by hand - or else it is only going to cannibalize its own value by taking coins from innocent potential adopters.

@junderw
Copy link

junderw commented Jul 25, 2017

I do care if people lost BitcoinCash.

Let's say Bob is a BitcoinCash supporter, and he wants to send his "Segwit Coin" to an exchange so he can sell it to buy more BitcoinCash.

Bob doesn't understand much about replay protection or whatnot, he is a simple fellow.

The exchange he uses has already upgraded to SegWit. Bob goes to deposit his Segwit Coin and unintentionally also deposits his BitcoinCash. OOPS! Oh well, the exchange was nice and watches for BitcoinCash as well, so he is currently waiting for confirmations on Segwit Coin and BitcoinCash. No biggie, just withdraw the Bitcoin Cash along with the extra that he buys when he sells Segwit Coin! Yay for him!

But wait, someone on BitcoinCash side sees the Segwit output of Bob's replayable transaction, and since it is ANYONE_CAN_PAY on BitcoinCash, Bob's deposit is soon stolen by someone who saw it (anyone really).

The exchange realizes that Bob's deposit of BitcoinCash was stolen, and since their official stance is "well, if you don't split it yourself before depositing, we don't take any responsibility if your coins get stolen on BitcoinCash" Bob's out of luck.

I would say that a complete two-way replay protection as suggested in this PR will help protect BitcoinCash supporters' BitcoinCash coins from seedy individuals who would try to trick non-technical users into sending BitcoinCash/SegWit merged UTXOs to them.

Leaving this hole open will leave many BitcoinCash supporters vulnerable to scams and theft, and only serve to give businesses and exchanges a big headache.

ACK

@HostFat
Copy link

HostFat commented Jul 25, 2017

From the point of view of a BCC supporter, I think that we can agree that it is possible to upgrade a cryptocurrency with hard forks.
Hard forks means that with every upgrade there is a high, and mostly certainly, probability to cut the "old network" from the new.
If we agree on both things (upgrading with hard forks and so cutting the old network), then I don't see a huge problem with this kind of modification, it is part of the roadmap that we are already supporting, I guess....

@digitsu
Copy link

digitsu commented Jul 25, 2017 via email

@digitsu
Copy link

digitsu commented Jul 25, 2017 via email

@HostFat
Copy link

HostFat commented Jul 25, 2017

What do you think about OP_RETURN method working as opposite way? So that any normal "legacy tx" will work on BCC only IF THERE IS some kind of OP_RETURN text?

On this way, adding support for BCC on wallets/projects will be even easier/faster.

@junderw
Copy link

junderw commented Jul 25, 2017

@digitsu How would the user be able to discern if an exchange is using segwit or not if the exchange uses P2SH? Many exchanges already use P2SH for deposits (for multisig) and if they switched to Segwit behind P2SH the user could not possibly know.

You would rather that all BitcoinCash users not use exchanges that utilize multi-sig? Only use exchanges that have P2PKH deposit addresses like Bitstamp did when they got hacked?

BitcoinCash is not usable if every user is required to ask exchanges to reveal the redeemscript before they can rest easy depositing funds.

@HostFat Very good idea. Require the new sighash algorithm UNLESS a specific OP_RETURN is included... hmmm, how about a short magic number (maybe 4 bytes) instead of an unnecessarily long string, while we're at it.

@HostFat
Copy link

HostFat commented Jul 25, 2017

@junderw
I would prefer something like "bitcoincash.org" but I'm sure that someone will be probably against this :D

@junderw
Copy link

junderw commented Jul 25, 2017

@HostFat There's no reason for any more than a 4 byte magic number tbh. Anything longer would need to serve another purpose.

@HostFat
Copy link

HostFat commented Jul 25, 2017

@junderw
There are some javascript tools as https://github.com/OutCast3k/coinbin that gives the possibility to sign offline even with OP_RETURN, so even if it short, it must be some "text" that the common user can easily enter on the interface.

@NiKiZe
Copy link

NiKiZe commented Jul 25, 2017

If full replay protection is implemented there is a few things that still needs to be supported... existing nLock transactions, and existing wallets which for some reason can't be upgraded (which have funds in them that would be lost if one transaction wasn't accepted on both chains)

@junderw
Copy link

junderw commented Jul 25, 2017

@HostFat 4 characters of a-zA-Z0-9 would give over 14 million possibilities, so should be sufficient for a magic number, as long as the 4 characters weren't a common word. 3cPx or something should be fine. Also avoid using only 0-9a-fA-F because some OP_RETURN parsers would view that as 2 bytes instead of 4 ASCII bytes.

@NiKiZe Perhaps only transactions with a non-zero nLocktime are replayable. Seems like a good compromise which doesn't require an extra OP_RETURN.

Reworded positively and in bullet style similar to original
@ftrader
Copy link
Contributor Author

ftrader commented Jul 25, 2017

@deadalnix : please have a look at my reformulation of 6-2 (based on your review comment)

@inaltoasinistra
Copy link

@junderw Some wallets (at least GreenAddress and Electrum) use nLockTime in all the transactions

@kanzure
Copy link

kanzure commented Jul 25, 2017

Strong replay protection would demonstrate that the BCC maintainers are interested in maintaining a minimum level of security for transactions. It also has the benefit of making a bunch of exchanges quite more willing to participate, and also it signals to your community that hard-forks will be used to loosen rules but not in any sort of way as to encourage money loss against BCC users or potential BCC users.

I am not sure if any of you will find this useful but let's say that strong (on-by-default) replay protection isn't on its own convincing.... so how about also considering an explicit signaling mechanism in transactions for users to opt-out of the on-by-default strong replay protection? In this scenario, users would modify their transactions (to add some signaling detail) and explicitly opt-out of the default replay protection mechanisms. For example, (and I have not evaluated this specific proposal) it could be something like SIGHASH_FORKID requirement unless OP_RETURN specifying a choice to opt-out of replay protection).

@@ -208,6 +219,15 @@ digest based on BIP143. Revisions were made to account for non-Segwit
deployment.


### REQ-6-4 (mandatory use of SCRIPT_VERIFY_STRICTENC)

Once the fork has activated, a transaction shall be deemed invalid if
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence is a bit odd, as far as I know the flag is not shipped in the transaction. But this spec gives the impression that that is exactly what is required.

It might be nice to refer to some bip (62 I believe) or other page that explains what this does, as I suspect that btcd etc don't use this exact enum value. And the enum value itself doesn't actually explain enough about the idea to implement it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zander is correct. This flag is not set on the transaction, but passed to the script verification machinery.

Copy link
Contributor Author

@ftrader ftrader Jul 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomz : you are correct it's not in the transaction, I must change that. Unfortunately I can't find one single consolidated place for this (BIP62 is withdrawn).

Best description of what it does here seems to be the code itself, starting from the definition in src/script/interpreter.h:

// Passing a non-strict-DER signature or one with undefined hashtype to a
// checksig operation causes script failure. Evaluating a pubkey that is not
// (0x04 + 64 bytes) or (0x02 or 0x03 + 32 bytes) by checksig causes script
// failure.

Other implementation will hopefully be able to comply with this by following the C++ code here.

I'll amend the wording to make it clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated @tomz

@h0jeZvgoxFepBQ2C
Copy link

h0jeZvgoxFepBQ2C commented Jul 25, 2017

I honestly don’t care if anyone loses segwitcoin.

I can really not understand how someone with this opinion is still allowed to comment here. Such a behaviour is totally unprofessional and shows that some contributors are completely unaware about the risks of their own actions.

If you don't want the hardfork to look like a joke, you should implement replay protection asap and take care that these destructive opinions won't be part of this project anymore - otherwise I can guarantee that this HF is DOA and it would be better for you to not waste your time here and go fishing instead.

Also multiple announcements from exchanges about the non-support of BCC in the last hours are clearly showing that the current way of development is not accepted by the community and I urge you to take the appropriate consequences.

@deadalnix
Copy link
Collaborator

@lichtamberg please calm down. People can disagree with each other without considering the other camp evil or retarded or declaring thing DOA. This attitude is stopping right now, one way or another.

@zander
Copy link
Contributor

zander commented Jul 25, 2017

I didn't realize so many people have a strong opinion on this.

First of all, Github is not a forum. It is for reviews, so people that feel they are not being listened to are probably better off going to an actual forum. Use IRC, email or slack or reddit or something that actually is a forum.

All indications are that companies and long time professionals in the industry are all behind the idea of strong replay protection. So many companies that work with people and Bitcoin payments have stated the same opinion.

People that are interested in the reasons; the utterly simple and straight forward one is this. It makes things clear and easy to people that don't care about technical details, that don't have the level of knowledge to triple check it actually is secure. Because without this replay protection people paying BCC to vendors can loose their BTC. And vice versa, people paying BTC can loose their BCC.
And just as important, people paying SW tx on the BTC chain can loose their coins on BCC to miners which honestly doesn't reflect well on BCC. I don't want to stand behind a coin that steals other peoples coins, if you feel BCC should be allowed to steal coins, I am not your friend.

I fully support the ideas behind this PR and we should have this replay protection in place already as an option, we just have to make it mandatory.

@rubensayshi
Copy link

the length of the opreturn is a problem imo,
46 bytes is a significant amount of bytes (~+30% for 1 input, 2 output tx, ~+12% for a 1 input 2 output 2of3 p2sh tx) that needs to be paid for in fees

others have already pointed out that a 4 byte value would be sufficient, hell, make it 8 if you're paranoid, but 46 is a huge overkill!

@deadalnix
Copy link
Collaborator

deadalnix commented Jul 25, 2017

I don't care much about the fact that the anyonecanspend used by segwit is now a problem. People have been warning about this for ages and it was ignored. Some wanted segwit, now they got it, with anyonecanspend, as specified.

I don't care much either about the fee in the OP_RETURN . Bitcoin leadership wanted a fee market, no they got it. If someone is dissatisfied about this situation, here is the wrong place to complain. We are splitting away because stupid decision have been made and this whole fee market story is one of them. There is a good reason the value is big: to avoid colliding with other legit uses of OP_RETURN, of which there are many.

There is no point in repeating both of these arguments. They simply show failure of Bitcoin's leadership and are no concerns to Bitcoin Cash.

My concern is more for average joe, who is going to lose BTC or BCC because he/she doesn't use replay protected txns. Making it mandatory also has the added benefit that it fixes quadratic hashing once and for all, and add signature cover value on all transaction, which is also good to take. Making it mandatory at one point in the future was always on ABC's roadmap. We are just talking about doing it at fork point, which seems to solve numerous concerns from users and businesses.

Thanks to Tom Zander for pointing out that its confusing.
Unfortunately, the specification situation around SCRIPT_VERIFY_STRICTENC
is also not such that we can point at a clear specification here.

Implementors should consider the code as reference for the moment.
@deadalnix deadalnix merged commit f6a3864 into master Jul 25, 2017
@zander
Copy link
Contributor

zander commented Jul 25, 2017

Thanks!

@ftrader
Copy link
Contributor Author

ftrader commented Jul 25, 2017

Thanks for all your input.

@digitsu
Copy link

digitsu commented Jul 25, 2017 via email

@kanzure
Copy link

kanzure commented Jul 25, 2017

But you also don't give up the flood of fee paying transactions that can showcase why BCC is better.

You could achieve the same effect by generating transactions that copy the same number of inputs/outputs and output values and even set a lower fee, to make that demonstration. It would have approximately the same semantic meaning too.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet