Skip to content
This repository has been archived by the owner on Oct 9, 2019. It is now read-only.

Commit

Permalink
Revert merge of 2017_optin_replay_jcansdale
Browse files Browse the repository at this point in the history
  • Loading branch information
jgarzik committed Oct 9, 2017
1 parent 18a77e6 commit 98c0af5
Show file tree
Hide file tree
Showing 6 changed files with 3 additions and 59 deletions.
5 changes: 0 additions & 5 deletions src/policy/policy.cpp
Expand Up @@ -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;
}

Expand Down
15 changes: 0 additions & 15 deletions src/primitives/transaction.cpp
Expand Up @@ -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;
}

2 changes: 0 additions & 2 deletions src/primitives/transaction.h
Expand Up @@ -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++) {
Expand Down
22 changes: 0 additions & 22 deletions src/test/transaction_tests.cpp
Expand Up @@ -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()
3 changes: 0 additions & 3 deletions src/validation.cpp
Expand Up @@ -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");
Expand Down
15 changes: 3 additions & 12 deletions test/functional/fork-large-block.py
Expand Up @@ -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

Expand All @@ -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 = []):
Expand Down

16 comments on commit 98c0af5

@arisAlexis
Copy link

Choose a reason for hiding this comment

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

democratic

@ReneFroger
Copy link

@ReneFroger ReneFroger commented on 98c0af5 Oct 9, 2017

Choose a reason for hiding this comment

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

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.

@xzilja
Copy link

@xzilja xzilja commented on 98c0af5 Oct 9, 2017

Choose a reason for hiding this comment

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

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

@JaredR26
Copy link

Choose a reason for hiding this comment

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

@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
Copy link

Choose a reason for hiding this comment

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

@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
Copy link
Author

@jgarzik jgarzik commented on 98c0af5 Oct 9, 2017

Choose a reason for hiding this comment

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

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

@JaredR26
Copy link

Choose a reason for hiding this comment

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

@CosmicHemorroid LIGHTNING HAS USERS??

@CosmicHemorroid
Copy link

Choose a reason for hiding this comment

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

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

@arisAlexis
Copy link

Choose a reason for hiding this comment

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

@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
Copy link

@pekatete pekatete commented on 98c0af5 Oct 9, 2017

Choose a reason for hiding this comment

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

@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
Copy link

Choose a reason for hiding this comment

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

@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
Copy link

@pekatete pekatete commented on 98c0af5 Oct 9, 2017

Choose a reason for hiding this comment

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

@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
Copy link

Choose a reason for hiding this comment

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

@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
Copy link

@droptv droptv commented on 98c0af5 Oct 9, 2017

Choose a reason for hiding this comment

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

@Lancelight
Copy link

Choose a reason for hiding this comment

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

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
Copy link

Choose a reason for hiding this comment

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

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.