Fix BIP68 activation test #9739
Conversation
|
@morcos has pointed out version 2 transactions were actually made standard in the CSV PR #7648 (not #7562 as I claimed above). That means that we were relaying version 2 transactions before CSV was activated. Oops. Anyway, the point still stands. BIP68 is now well and truly activated, so we don't need to test pre-activation standardness. |
I think that is fine and that it was the intent-- so long as they were valid CSV transactions, which they were, because the CSV rules were enforced against the mempool prior to activation. |
sdaftuar
left a comment
sdaftuar
left a comment
There was a problem hiding this comment.
@gmaxwell This is an aside but for the sake of anyone following this conversation, we actually didn't start relaying version 2 transactions before CSV was activated -- we merged #7835 prior to release, to prevent that from happening. (If there is a class of transactions that we think are valid but that miners for whatever reason are likely to not mine, then that opens up some mempool-related DoS vectors.)
qa/rpc-tests/bip68-sequence.py
Outdated
| print("Verifying nVersion=2 transactions aren't standard") | ||
| self.test_version2_relay(before_activation=True) | ||
| print("Verifying nVersion=2 transactions are standard") | ||
| self.test_version2_relay() |
There was a problem hiding this comment.
I think we should move this to below the CSV activation. Testing that relaying of nVersion=2 transactions post activation makes sense, whereas testing that it works pre-activation is strange -- it's okay in this case that we allow it (because on mainnet activation happened a long time ago), but that is not how the code used to behave prior to activation (because relaying those transactions prior to validation opens up some DoS vectors).
jnewbery
force-pushed
the
fixbip68testing
branch
from
2491e5e to
b336a71
Compare
|
ok, I've moved the standardness check to after BIP68 activation. I've also added a comment to say transaction version 2 is always standard for current bitcoin core versions to avoid any confusion for people reading the code. I've verified that the test still runs. Once you're happy and have ACKed the changes, I'll squash for merging. |
jnewbery
force-pushed
the
fixbip68testing
branch
from
b336a71 to
f5aba8a
Compare
|
utACK f5aba8a |
|
utACK f5aba8a |
6 participants
#7562 made version 2 transactions standard, which should have broken the test in bip68-sequence.py that version 2 transactions are non-standard before BIP 68 activation. However, there was a syntax error in the test case, so it didn't break.
In fact, it's fine for version 2 transactions to be standard before BIP 68 activation since BIP 68 was activated in July 2016, so it'd have to be a pretty big re-org to go back to pre-activated times (and even then this is a non-consensus change).
Best fix here is to remove the test that version 2 transactions are non-standard prior to BIP68 activation.