[consensus] Add opt-in replay protection for 2x chain (using SIGHASH) #131

matsjj · 2017-10-10T15:27:29Z

This is a WIP branch.

The initial idea is to take something that is currently forbidden in bitcoin, but only checked by full nodes, and allow it. By doing this, we allow wallets to create transactions that are only valid on this chain.

For this PR I've chosen a new SIGHASH id, similar to the approach taken by Bitcoin Cash (except that we don't require this SIGHASH, we barely allow it). We need to assess whether there are any light wallets that do actually do any sanity checks on the SIGHASH field. Unless there are, this change should satisfy the condition, of not breaking consumer wallets.

I am aware that this change adds some technical debt. Ideally we also add a sunset clause, although it would need to have a large time horizon (2 years+).

There are other approaches, as described in #34 (comment) - they might have other merits or downsides, I am open for discussion.

Tests haven't been written/fixed yet, as I first want to see if this approach has consensus.

SIGHASH_2X_REPLAY_PROTECT can be used instead of SIGHASH_SINGLE

jcansdale · 2017-10-10T16:22:31Z

I'm concerned that any opt-in replay protection is an exploitation waiting to happen for L2 solutions. This would be compounded when adding a sunset clause (which ideally would be included).

The solution would also need to be implemented on Bitcoin Unlimited, Classic and any non-Satoshi nodes. This would would a challenge given the time constraints.

My preference would be replay protection that doesn't rely on consensus level changes (e.g. consuming outputs from a 1M-tx).

matsjj · 2017-10-10T16:48:22Z

I'm concerned that any opt-in replay protection is an exploitation waiting to happen for L2 solutions. This would be compounded when adding a sunset clause (which ideally would be included).

Replay protection in this direction is generally not affecting L2 solutions. After all, we just expanded the rule set. Only for the sunset clause this would be an issue. But even then, at least for LN, signatures are explicitly checked, the SIGHASH is not even part of what is transmitted over the wire. (current protocol only transmits 64B, R and S value. Any other value than SIGHASH_ALL would fail the payment channel immediately)

The solution would also need to be implemented on Bitcoin Unlimited, Classic and any non-Satoshi nodes. This would would a challenge given the time constraints.

Correct, this is a downside. This change needs to get propagated to any full-validating source code. So far this is a very minimal change though, and we could generalise, such that it could be reused for future forks.

apoelstra · 2017-10-10T17:24:06Z

This patch appears to only affect policy and not consensus.

matsjj · 2017-10-10T17:35:15Z

This patch appears to only affect policy and not consensus.

Mind to elaborate? :)

From my understanding, adding the flag to allow for an additional SIGHASH type in ConnectBlock

https://github.com/btc1/bitcoin/pull/131/files#diff-24efdb00bfbe56b140fb006b562cc70bR1738

is a consensus change (making a previously invalid block, one that contains a transaction with a new SIGHASH type, valid).

jcansdale · 2017-10-10T17:48:16Z

Only for the sunset clause this would be an issue. But even then, at least for LN, signatures are explicitly checked, the SIGHASH is not even part of what is transmitted over the wire. (current protocol only transmits 64B, R and S value. Any other value than SIGHASH_ALL would fail the payment channel immediately)

Interesting, thanks for the clarification. It does seem like an interesting solution.

After all, we just expanded the rule set.

I'm wondering if it's even possible to sunset hard-forks? Won't the expanded rule need to remain, even if it's only there to verify pre-sunset transactions?

Also, there is software I'd like to use with Bitcoin Cash, but can't because of its expanded rules. This would be an immediate problem for me if this was implemented. See:
https://twitter.com/jcansdale/status/907674539609518080

Rolling out a hard-fork (from the perspective of BU) in such a short time-frame is perhaps the biggest issue though.

apoelstra · 2017-10-10T19:28:13Z

Mind to elaborate? :)

You do modify ConnectBlock to add the flag SCRIPT_VERIFY_ALLOW_2X_SIGHASH but this flag is only checked in interpreter.cpp if SCRIPT_VERIFY_STRICTENC is set.

This will sufficiently invalidate the signatures for the 1X chain.

So far this was only a policy rule. With this change, we require bit 8 of nHashType to be set for transactions that have nHashType = SIGHASH_2X_REPLAY_PROTECT. This change is based off of feedback from arubi and apoelstra.

matsjj · 2017-10-11T12:07:11Z

Thanks @apoelstra for the feedback.

I changed this proposal to require a specific bit set in the 4B nHashType when the 1B at the end of the signature has a certain value. It's not enough to just allow a new nHashType, as none of them are explicitly forbidden in the first place (I was mistaken here). With this change 1X and 2X will produce an incompatible CheckSig result for SIGHASH_2X, resulting in the ability to replay protect certain transactions.

Passing in _true_ all times means bitcoind is able to create replay protected outputs post fork. Ideally we have a check for current block height here.

Sjors · 2017-10-12T13:57:16Z

src/validation.cpp

+        const int nBlockHeight = chainActive.Height();
+        if (nBlockHeight >= Params().GetConsensus().SW2XHeight) {
+            scriptVerifyFlags |= SCRIPT_VERIFY_ALLOW_2X_SIGHASH;
+        }


What about relaying anti-replay transactions to other nodes once they are in the mempool? If the mempool RPC returns these transactions to a non-upgraded btc1 node, wouldn't that node get upset because you're sending it invalid transactions?

Perhaps the mempool` RPC call could exclude these transactions until a hardcoded date just before the fork? That gives node operators some time to upgrade.

Sjors · 2017-10-12T13:59:28Z

@petertodd I assume you have another puzzle ready to go? :-)

BTCgithub · 2017-10-13T19:06:34Z

Please take a moment to read the thread below before considering adding -ANY- replay protection to SegWit2x. It's important that the Legacy 1x chain does -NOT- survive or continue after the 2MB upgrade. Try to ignore the politics and chatter in the thread as there are some good points made regarding the Legacy 1x chain's nodes mempool flooding/circle of death after the upgrade.

'How is 1x even going to survive a 50/50 hash power stand off?'
https://www.reddit.com/r/btc/comments/75xxws/core_shills_how_is_1x_even_going_to_survive_a/?st=j8pj8ati&sh=866e3125

Sjors

Still wrapping my head around this. If I understand correctly this only contains code that verifies the transaction. It might be useful to add code to the reference wallet for generating such a transaction.

Sjors · 2017-10-31T09:48:33Z

src/script/interpreter.cpp

@@ -205,7 +207,8 @@ bool CheckSignatureEncoding(const vector<unsigned char> &vchSig, unsigned int fl
    } else if ((flags & SCRIPT_VERIFY_LOW_S) != 0 && !IsLowDERSignature(vchSig, serror)) {
        // serror is set
        return false;
-    } else if ((flags & SCRIPT_VERIFY_STRICTENC) != 0 && !IsDefinedHashtypeSignature(vchSig)) {
+    } else if ((flags & SCRIPT_VERIFY_STRICTENC) != 0 


@apoelstra does this mean the replay protection won't work for certain non-standard transactions? And that would cause replay issues if in the future these transactions become standard?

Or does everything work fine as long as these future wallets use splitting transactions that are standard under todays rules?

I think it's fine, it means that transactions which have SIGHASH_2X_REPLAY_PROTECT become standard so that they'll be propagated on 2x. But these transactions are inherently replay protected by the "set bit 8" rule below (the signature itself can be valid on only one chain), there would be no risk if they became standard or were otherwise communicated directly to miners on the original chain.

Sjors · 2017-10-31T10:00:09Z

src/script/interpreter.cpp

@@ -183,12 +183,14 @@ bool static IsLowDERSignature(const valtype &vchSig, ScriptError* serror) {
    return true;
 }

-bool static IsDefinedHashtypeSignature(const valtype &vchSig) {
+bool static IsDefinedHashtypeSignature(const valtype &vchSig, const bool allowReplay) {


I'm confused by the variable name allowReplay. If I understand correctly this boolean will be true after the fork height is reached. Before this height it's false, so the OR statement below is what is was before. Maybe rename it to something like isSighash2xReplayProtectDefined (but less ugly)?

Sjors · 2017-10-31T10:23:00Z

src/script/interpreter.cpp

@@ -1262,6 +1266,11 @@ bool TransactionSignatureChecker::CheckSig(const vector<unsigned char>& vchSigIn
        return false;
    int nHashType = vchSig.back();
    vchSig.pop_back();
+
+    // If nHashType is set to SIGHASH_2X_REPLAY_PROTECT we require bit 8 be set to 1


Suggest adding to comment to explain how having bit 8 set makes this transaction invalid for Core nodes. I assume this is different than the policy triggered by SIGHASH_2X_REPLAY_PROTECT that causes IsDefinedHashtypeSignature to return false on Core nodes?

Sjors · 2017-10-31T10:23:34Z

src/script/interpreter.cpp

@@ -1262,6 +1266,11 @@ bool TransactionSignatureChecker::CheckSig(const vector<unsigned char>& vchSigIn
        return false;
    int nHashType = vchSig.back();
    vchSig.pop_back();
+
+    // If nHashType is set to SIGHASH_2X_REPLAY_PROTECT we require bit 8 be set to 1
+    // Requiring bit 8 to be set to 1 at all times would break backward compatibility


Suggest adding: "Requiring rather than merely permitting"

Sjors · 2017-10-31T10:26:20Z

src/script/interpreter.cpp

+    // If nHashType is set to SIGHASH_2X_REPLAY_PROTECT we require bit 8 be set to 1
+    // Requiring bit 8 to be set to 1 at all times would break backward compatibility
+    if (fReplayProtection && (nHashType & 0x1f) == SIGHASH_2X_REPLAY_PROTECT)
+        nHashType |= (1U << 8); 


I believe the code guidelines require this to have brackets or be on the same line as the if statement.

So this sets bit 8 (0 based, little endian) to 1? This is used when generating / verifying the signature, but it's not actually writing out the transaction? So in that case a Core node wouldn't know about the need to set bit 8, and thus would generate a different signature?

So a wallet that generates a replay protected legacy transaction should not set bit 8 at the end of the serialized transaction? It should only do that during the signature generation?

I found this Stack Overflow answer about legacy transactions useful: "Append the SIGHASH type, serialized as a 32-bit little-endian integer"

It would be nice to add this sort of thing in a comment somewhere :-)

Does this work the same with SegWit transactions (which use BIP143 for the signature pre-image).

Just to address my own confusion here: 32 bits are used in the transaction pre-image, the thing that's signed / verified in memory. It's not put in the final transaction that's broadcast, only the first byte is. What's still not clear for me, from reading the BIP143 examples, is where the SIGHASH_TYPE type goes for SegWit transactions; but I suppose in the witness, with the first byte appended to the signature as well?

I posted my last question here as well.

Sjors · 2017-11-01T06:45:03Z

It's interesting to compare this approach to Spoonnet (which is mandatory though):

Hardfork network version bit is 0x02000000. A tx is invalid if the highest nVersion byte is not zero, and the network version bit is not set.

Masked tx version is nVersion with the highest byte masked. If masked version is 3 or above, sighash for OP_CHECKSIG alike is calculated using BIP143, except 0x02000000 is added to the nHashType (the nHashType in signature is still a 1-byte value)

So the SIGHASH_TYPE in the serialized transaction is left alone, and instead the nVersion is changed. In the pre-image, rather than setting a bit, you just add the version number. Give each fork a unique version number and presto :-)

jgarzik · 2017-11-08T20:22:54Z

Closing - not needed

matsjj added 2 commits October 10, 2017 16:15

Allow SIGHASH_2X_REPLAY_PROTECT = 4 as a possible SIGHASH

f65593b

SIGHASH_2X_REPLAY_PROTECT can be used instead of SIGHASH_SINGLE

Fix null/0 typo

a32adf3

Fix boolean/bool typo

ba8f009

Sjors mentioned this pull request Oct 10, 2017

Opt-in segwit2x-only transactions #125

Closed

Fix 2XHeight => SW2XHeight

e9fcd14

Allow new SIGHASH type in mempool after fork height

c3308f1

matsjj added 3 commits October 11, 2017 12:13

Require bit 8 of full SIGHASH to be set for replay protected signatures

f02e1dc

This will sufficiently invalidate the signatures for the 1X chain.

Fix typo flags => scriptVerifyFlags

174b7e8

Make this a consensus rule

a56fbaf

So far this was only a policy rule. With this change, we require bit 8 of nHashType to be set for transactions that have nHashType = SIGHASH_2X_REPLAY_PROTECT. This change is based off of feedback from arubi and apoelstra.

matsjj added 4 commits October 11, 2017 13:25

Fix typo replayProtection => fReplayProtection

041d886

Fix parenthesis warnings

303f345

Fix access to SW2XHeight

c0f8583

Fix warning related to shadowing existing function

66780a9

Passing in _true_ all times means bitcoind is able to create replay protected outputs post fork. Ideally we have a check for current block height here.

JaredR26 mentioned this pull request Oct 12, 2017

[consensus] Add opt-in transaction avoidance #134

Closed

matsjj changed the title ~~Add opt-in replay protection towards 1X chain~~ [consensus] Add opt-in replay protection for 2x chain (using SIGHASH) Oct 12, 2017

Sjors reviewed Oct 12, 2017

View reviewed changes

Sjors reviewed Oct 31, 2017

View reviewed changes

jgarzik closed this Nov 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[consensus] Add opt-in replay protection for 2x chain (using SIGHASH) #131

[consensus] Add opt-in replay protection for 2x chain (using SIGHASH) #131

matsjj commented Oct 10, 2017

jcansdale commented Oct 10, 2017

matsjj commented Oct 10, 2017 •

edited

apoelstra commented Oct 10, 2017

matsjj commented Oct 10, 2017

jcansdale commented Oct 10, 2017

apoelstra commented Oct 10, 2017

matsjj commented Oct 11, 2017

Sjors Oct 12, 2017

Sjors commented Oct 12, 2017

BTCgithub commented Oct 13, 2017

Sjors left a comment

Sjors Oct 31, 2017

Sjors Oct 31, 2017

apoelstra Oct 31, 2017

Sjors Oct 31, 2017

Sjors Oct 31, 2017

Sjors Oct 31, 2017

Sjors Oct 31, 2017

Sjors Oct 31, 2017

Sjors Oct 31, 2017

Sjors Nov 1, 2017

Sjors commented Nov 1, 2017

jgarzik commented Nov 8, 2017

[consensus] Add opt-in replay protection for 2x chain (using SIGHASH) #131

[consensus] Add opt-in replay protection for 2x chain (using SIGHASH) #131

Conversation

matsjj commented Oct 10, 2017

jcansdale commented Oct 10, 2017

matsjj commented Oct 10, 2017 • edited

apoelstra commented Oct 10, 2017

matsjj commented Oct 10, 2017

jcansdale commented Oct 10, 2017

apoelstra commented Oct 10, 2017

matsjj commented Oct 11, 2017

Choose a reason for hiding this comment

Sjors commented Oct 12, 2017

BTCgithub commented Oct 13, 2017

Sjors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors commented Nov 1, 2017

jgarzik commented Nov 8, 2017

matsjj commented Oct 10, 2017 •

edited