Permalink
Browse files

Revert merge of 2017_optin_replay_jcansdale

  • Loading branch information...
jgarzik committed Oct 9, 2017
1 parent 18a77e6 commit 98c0af58c29efbecba25818adb5531fa8c3d0506
View
@@ -157,11 +157,6 @@ bool IsStandardTx(const CTransaction& tx, std::string& reason, const bool witnes
return false;
}
if (tx.ReplayProtected()) {
reason = "replay-protected";
return false;
}
return true;
}
@@ -113,18 +113,3 @@ std::string CTransaction::ToString() const
str += " " + tx_out.ToString() + "\n";
return str;
}
bool CTransaction::ReplayProtected() const
{
// scriptAddress: 3Bit1xA4apyzgmFNT2k8Pvnd6zb6TnwcTi, scriptSig: 04148f33be, scriptPubKey: OP_HASH160 6e0b7d51f9f68ba28cc33a6844d41a1880c58a19 OP_EQUAL
static const CScript noReplay =
CScript() << OP_HASH160 << ParseHex("6e0b7d51f9f68ba28cc33a6844d41a1880c58a19") << OP_EQUAL;
for (const auto& txout : this->vout) {
if (txout.scriptPubKey == noReplay) {
return true;
}
}
return false;
}
@@ -347,8 +347,6 @@ class CTransaction
std::string ToString() const;
bool ReplayProtected() const;
bool HasWitness() const
{
for (size_t i = 0; i < vin.size(); i++) {
@@ -797,28 +797,6 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
t.vout[0].scriptPubKey = CScript() << OP_RETURN;
t.vout[1].scriptPubKey = CScript() << OP_RETURN;
BOOST_CHECK(!IsStandardTx(t, reason));
// scriptAddress: 3Bit1xA4apyzgmFNT2k8Pvnd6zb6TnwcTi, scriptSig: 04148f33be, scriptPubKey: OP_HASH160 6e0b7d51f9f68ba28cc33a6844d41a1880c58a19 OP_EQUAL
t.vout.resize(1);
t.vout[0].scriptPubKey = CScript() << OP_HASH160 << ParseHex("6e0b7d51f9f68ba28cc33a6844d41a1880c58a19") << OP_EQUAL;
BOOST_CHECK(!IsStandardTx(t, reason));
}
BOOST_AUTO_TEST_CASE(test_ReplayProtected)
{
CMutableTransaction t;
// scriptAddress: 3Bit1xA4apyzgmFNT2k8Pvnd6zb6TnwcTi, scriptSig: 04148f33be, scriptPubKey: OP_HASH160 6e0b7d51f9f68ba28cc33a6844d41a1880c58a19 OP_EQUAL
t.vout.resize(1);
t.vout[0].scriptPubKey = CScript() << OP_HASH160 << ParseHex("6e0b7d51f9f68ba28cc33a6844d41a1880c58a19") << OP_EQUAL;
CTransaction tx1(t);
BOOST_CHECK(tx1.ReplayProtected());
// scriptAddress: 3EZT2iZWYCpy1uXX6XtjX429TnsZ3vKvVV, scriptSig: 777777, scriptPubKey: OP_HASH160 8d2b43ea8081126af5bc392612b1471a0c4fe503 OP_EQUAL
t.vout.resize(1);
t.vout[0].scriptPubKey = CScript() << OP_HASH160 << ParseHex("8d2b43ea8081126af5bc392612b1471a0c4fe503") << OP_EQUAL;
CTransaction tx2(t);
BOOST_CHECK(!tx2.ReplayProtected());
}
BOOST_AUTO_TEST_SUITE_END()
View
@@ -3063,9 +3063,6 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c
for (const auto& tx : block.vtx)
{
nSigOps += GetLegacySigOpCount(*tx);
if (fSegwitSeasoned && tx->ReplayProtected())
return state.DoS(100, false, REJECT_INVALID, "bad-blk-rptx", false, "replay-protected tx in block");
}
if (nSigOps * WITNESS_SCALE_FACTOR > MaxBlockSigOpsCost(fSegwitSeasoned))
return state.DoS(100, false, REJECT_INVALID, "bad-blk-sigops", false, "out-of-bounds SigOpCount");
@@ -180,23 +180,15 @@ def run_test(self):
# Test hard fork at block 1583
assert_equal(self.height, 584)
b = [self.nodes[0].getblockhash(n) for n in range(1, 11)]
b = [self.nodes[0].getblockhash(n) for n in range(1, 10)]
txids = [self.nodes[0].getblock(h)['tx'][0] for h in b]
spend_tx = [FromHex(CTransaction(), self.nodes[0].getrawtransaction(txid)) for txid in txids]
for tx in spend_tx:
tx.rehash()
large_tx = [self.create_tx(t, 0, 1, length=500000) for t in spend_tx]
spend_tx_extra = spend_tx[:1]
spend_tx_large = spend_tx[1:]
self.generate_blocks(998, 4)
large_tx = [self.create_tx(t, 0, 1, length=500000) for t in spend_tx_large]
# P2SH: 3Bit1xA4apyzgmFNT2k8Pvnd6zb6TnwcTi
noreplay_tx = self.create_tx(spend_tx_extra[0], 0, 0, CScript([OP_HASH160, hex_str_to_bytes("6e0b7d51f9f68ba28cc33a6844d41a1880c58a19"), OP_EQUAL]))
self.generate_blocks(997, 4)
self.generate_blocks(1, 4, txs=[noreplay_tx]) # noreplay-tx is ok
self.generate_blocks(1, 4, "bad-blk-length", txs=[large_tx[0], large_tx[1]]) # block too large
self.generate_blocks(1, 4, txs=[large_tx[0]]) # large txs is ok
@@ -215,7 +207,6 @@ def run_test(self):
assert_equal(self.height, 6584)
self.generate_blocks(1, 4, txs=[large_tx[4], large_tx[5], large_tx[6]]) # large block ok
self.generate_blocks(1, 4, "bad-blk-rptx", txs=[noreplay_tx]) # noreplay-tx is invalid after the fork
def generate_blocks(self, number, version, error = None, txs = []):

16 comments on commit 98c0af5

@arisAlexis

This comment has been minimized.

Show comment
Hide comment
@arisAlexis

arisAlexis Oct 9, 2017

democratic

arisAlexis replied Oct 9, 2017

democratic

@ReneFroger

This comment has been minimized.

Show comment
Hide comment
@ReneFroger

ReneFroger Oct 9, 2017

And this change is done without any notification, which is unusual given your previous modifications.
Why did you removed the replay protection in the past? Of all the large code base Bitcoin codebase, only this change? What is the benefit of deleting a safe feature? What's benefit for S2X to be dangerous for other forks? Why not improve Bitcoin code for your fork, or bug fixing, or contribute something at least? Why make it just more dangerous? I'm trying to understand you. It's same as turning off ABS or airbag for your car, which makes no sense at all.

Now you're rolling back after some users complained, and spread further without proper testing.

Only the thing that I could guess, if you want to turn off the methaphor airbags, your intention is to cause unnecessary chaos in the bitcoin space? You can do just better for the progress of financial sovereignty, instead of causing havoc and chaos for personal reasons.

ReneFroger replied Oct 9, 2017

And this change is done without any notification, which is unusual given your previous modifications.
Why did you removed the replay protection in the past? Of all the large code base Bitcoin codebase, only this change? What is the benefit of deleting a safe feature? What's benefit for S2X to be dangerous for other forks? Why not improve Bitcoin code for your fork, or bug fixing, or contribute something at least? Why make it just more dangerous? I'm trying to understand you. It's same as turning off ABS or airbag for your car, which makes no sense at all.

Now you're rolling back after some users complained, and spread further without proper testing.

Only the thing that I could guess, if you want to turn off the methaphor airbags, your intention is to cause unnecessary chaos in the bitcoin space? You can do just better for the progress of financial sovereignty, instead of causing havoc and chaos for personal reasons.

@IljaDaderko

This comment has been minimized.

Show comment
Hide comment
@IljaDaderko

IljaDaderko Oct 9, 2017

If this is not leading up to better replay protection implementation, then this is most "rage about loosing" commit I've ever seen.

IljaDaderko replied Oct 9, 2017

If this is not leading up to better replay protection implementation, then this is most "rage about loosing" commit I've ever seen.

@JaredR26

This comment has been minimized.

Show comment
Hide comment
@JaredR26

JaredR26 Oct 9, 2017

@jgarzik is discussing alternatives for replay protection that he thinks will be better suited in slack. There are varying opinions on every side, though no 2x supporter is espousing the idea that 2x adds replay protection that will break compatibility with existing SPV wallets.

Feel free to join in slack with your opinions on the best optional replay protection approaches.

JaredR26 replied Oct 9, 2017

@jgarzik is discussing alternatives for replay protection that he thinks will be better suited in slack. There are varying opinions on every side, though no 2x supporter is espousing the idea that 2x adds replay protection that will break compatibility with existing SPV wallets.

Feel free to join in slack with your opinions on the best optional replay protection approaches.

@CosmicHemorroid

This comment has been minimized.

Show comment
Hide comment
@CosmicHemorroid

CosmicHemorroid Oct 9, 2017

@JaredR26 Let's remind everyone the secret @jgarzik won't reveal. This is being reversed because Peter Todd and David Harding found a security vulnerability that would allow LN users to steal funds from each other. Notice how btc1 devs did not find it? Good luck with your fork.

CosmicHemorroid replied Oct 9, 2017

@JaredR26 Let's remind everyone the secret @jgarzik won't reveal. This is being reversed because Peter Todd and David Harding found a security vulnerability that would allow LN users to steal funds from each other. Notice how btc1 devs did not find it? Good luck with your fork.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Oct 9, 2017

@CosmicHemorroid Harding was directly referenced in open slack, as one of several valid peer reviews. We take good feedback.

jgarzik replied Oct 9, 2017

@CosmicHemorroid Harding was directly referenced in open slack, as one of several valid peer reviews. We take good feedback.

@JaredR26

This comment has been minimized.

Show comment
Hide comment
@JaredR26

JaredR26 Oct 9, 2017

@CosmicHemorroid LIGHTNING HAS USERS??

JaredR26 replied Oct 9, 2017

@CosmicHemorroid LIGHTNING HAS USERS??

@CosmicHemorroid

This comment has been minimized.

Show comment
Hide comment
@CosmicHemorroid

CosmicHemorroid Oct 9, 2017

@jgarzik Point taken. I should have checked slack first.

CosmicHemorroid replied Oct 9, 2017

@jgarzik Point taken. I should have checked slack first.

@arisAlexis

This comment has been minimized.

Show comment
Hide comment
@arisAlexis

arisAlexis Oct 9, 2017

@JaredR26 "@jgarzik is discussing alternatives for replay protection that he thinks will be better suited in slack" is this the most centralized open source project with a one man show in your opinion or do you know of another one?

arisAlexis replied Oct 9, 2017

@JaredR26 "@jgarzik is discussing alternatives for replay protection that he thinks will be better suited in slack" is this the most centralized open source project with a one man show in your opinion or do you know of another one?

@pekatete

This comment has been minimized.

Show comment
Hide comment
@pekatete

pekatete Oct 9, 2017

@CosmicHemorroid

found a security vulnerability that would allow LN users to steal funds from each other.

LN is vapourware and is being developed under the toxic core environment that's full of innovation gatekeeping. You know that so stop trolling.

pekatete replied Oct 9, 2017

@CosmicHemorroid

found a security vulnerability that would allow LN users to steal funds from each other.

LN is vapourware and is being developed under the toxic core environment that's full of innovation gatekeeping. You know that so stop trolling.

@pekatete

This comment has been minimized.

Show comment
Hide comment
@pekatete

pekatete Oct 9, 2017

@CosmicHemorroid - now you are proper trolling if you want to tag the btc1 project as hostile & toxic. No one is surprised that it is ONLY core devs finding these security vulnerabilities as they've made the environment so toxic to non core devs they dare not tread.
You should be asking yourself why it is only core devs finding these bugs, I mean, this is a planet of billions with at least a few hundred (if not thousand) devs competent to work on bitcoin code. So why only core devs? It is telling, isn't it?

pekatete replied Oct 9, 2017

@CosmicHemorroid - now you are proper trolling if you want to tag the btc1 project as hostile & toxic. No one is surprised that it is ONLY core devs finding these security vulnerabilities as they've made the environment so toxic to non core devs they dare not tread.
You should be asking yourself why it is only core devs finding these bugs, I mean, this is a planet of billions with at least a few hundred (if not thousand) devs competent to work on bitcoin code. So why only core devs? It is telling, isn't it?

@pekatete

This comment has been minimized.

Show comment
Hide comment
@pekatete

pekatete Oct 9, 2017

@CosmicHemorroid

they are the most familiar with the code

Innovtion gatekeeping that serves their roadmap of being critical and essential to bitcoin development (and you've fallen for it by the looks of it)

the best at what they do.

At being toxic, I agree.

The environment being fostered in the btc1 project will attract more developers to the new reference client link

pekatete replied Oct 9, 2017

@CosmicHemorroid

they are the most familiar with the code

Innovtion gatekeeping that serves their roadmap of being critical and essential to bitcoin development (and you've fallen for it by the looks of it)

the best at what they do.

At being toxic, I agree.

The environment being fostered in the btc1 project will attract more developers to the new reference client link

@jheathco

This comment has been minimized.

Show comment
Hide comment
@jheathco

jheathco Oct 9, 2017

@ALL please keep it technical - we have enough toxicity in virtually every other channel that we don't need it in github as well.

jheathco replied Oct 9, 2017

@ALL please keep it technical - we have enough toxicity in virtually every other channel that we don't need it in github as well.

@droptv

This comment has been minimized.

Show comment
Hide comment

droptv replied Oct 9, 2017

@Lancelight

This comment has been minimized.

Show comment
Hide comment
@Lancelight

Lancelight Oct 10, 2017

Well @jgarzik this is a step in the right direction. I'd rather have no protection at all than that broken ass opt-in crap that people would be exploiting left and right. Thanx for the revert, glad you are finally seeing the light. I hope to see a true replay protection method soon. Who cares if wallets have to upgrade to support it like its some sort of altcoin. They will do so or the users of those wallets will complain and go elsewhere. It will just go to show that 2x can stand on its technical merits rather than hostile takeover attempts (well, that is IF you implement a replay protection thats worthwhile of course, I hope this commit is a step in that direction).

Lancelight replied Oct 10, 2017

Well @jgarzik this is a step in the right direction. I'd rather have no protection at all than that broken ass opt-in crap that people would be exploiting left and right. Thanx for the revert, glad you are finally seeing the light. I hope to see a true replay protection method soon. Who cares if wallets have to upgrade to support it like its some sort of altcoin. They will do so or the users of those wallets will complain and go elsewhere. It will just go to show that 2x can stand on its technical merits rather than hostile takeover attempts (well, that is IF you implement a replay protection thats worthwhile of course, I hope this commit is a step in that direction).

@zanza321

This comment has been minimized.

Show comment
Hide comment
@zanza321

zanza321 Oct 11, 2017

Core should start considering to add in replay protection on their end, and possible POW change also? I know they have expressed desire to do so in the past, I think it would be courageous of them to stand up to these Chinese bully miners and fire them.

zanza321 replied Oct 11, 2017

Core should start considering to add in replay protection on their end, and possible POW change also? I know they have expressed desire to do so in the past, I think it would be courageous of them to stand up to these Chinese bully miners and fire them.

Please sign in to comment.