Fix BIP68 activation test #9739

1. 
   jnewbery commented at 6:19 pm on February 10, 2017: member

   #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.

2. Fix BIP68 activation test 99c0e81b95
3. 
   jnewbery commented at 6:20 pm on February 10, 2017: member

   @sdaftuar
4. fanquake added the label Tests on Feb 10, 2017
5. 
   jnewbery commented at 0:57 am on February 11, 2017: member

   @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.

6. 
   gmaxwell commented at 8:30 pm on February 13, 2017: contributor

   That means that we were relaying version 2 transactions before CSV was activated. Oops.

   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.

7. in qa/rpc-tests/bip68-sequence.py: in 99c0e81b95 outdated

   50@@ -51,15 +51,12 @@ def run_test(self):
   51         print("Running test BIP68 not consensus before versionbits activation")
   52         self.test_bip68_not_consensus()
   53 
   54-        print("Verifying nVersion=2 transactions aren't standard")
   55-        self.test_version2_relay(before_activation=True)
   56+        print("Verifying nVersion=2 transactions are standard")
   57+        self.test_version2_relay()
   

   ---

   ---

   

   sdaftuar commented at 7:34 pm on February 21, 2017:

   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).
8. sdaftuar changes_requested
9. 
   sdaftuar commented at 7:35 pm on February 21, 2017: member

   @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.)
10. jnewbery force-pushed on Feb 21, 2017
11. 
    jnewbery commented at 9:55 pm on February 21, 2017: member

    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.

12. Move tx version 2 standardness check to after bip68 activation f5aba8a3c5
13. jnewbery force-pushed on Feb 22, 2017
14. 
    jnewbery commented at 11:25 pm on February 22, 2017: member

    @sdaftuar pointed out a missing closing parentheses. Fixed in https://github.com/bitcoin/bitcoin/pull/9739/commits/f5aba8a3c5f10bf42595cbaf0439976cd48c57b1
15. 
    sdaftuar commented at 2:55 pm on February 23, 2017: member

    utACK f5aba8a
16. 
    MarcoFalke commented at 4:05 pm on February 23, 2017: member

    utACK f5aba8a3c5f10bf42595cbaf0439976cd48c57b1
17. laanwj merged this on Mar 6, 2017
18. laanwj closed this on Mar 6, 2017

    ---

19. laanwj referenced this in commit 9d5fcbfb08 on Mar 6, 2017
20. PastaPastaPasta referenced this in commit e35dabdc4a on Dec 30, 2018
21. PastaPastaPasta referenced this in commit 4dac146b38 on Dec 31, 2018
22. PastaPastaPasta referenced this in commit b35502906c on Dec 31, 2018
23. PastaPastaPasta referenced this in commit d12d0456f1 on Dec 31, 2018
24. PastaPastaPasta referenced this in commit d074244783 on Jan 2, 2019
25. PastaPastaPasta referenced this in commit 3775232867 on Jan 2, 2019
26. PastaPastaPasta referenced this in commit 540dba6ec6 on Jan 3, 2019
27. PastaPastaPasta referenced this in commit c91ebc14c8 on Jan 21, 2019
28. PastaPastaPasta referenced this in commit c454f6582a on Jan 25, 2019
29. PastaPastaPasta referenced this in commit d6240f062c on Jan 29, 2019
30. PastaPastaPasta referenced this in commit 57b320dfc6 on Feb 1, 2019
31. PastaPastaPasta referenced this in commit 23f0745a8b on Feb 1, 2019
32. PastaPastaPasta referenced this in commit c903e7f929 on Feb 1, 2019
33. PastaPastaPasta referenced this in commit b821dfa7dc on Feb 1, 2019
34. MarcoFalke locked this on Sep 8, 2021

Contributors
jnewbery gmaxwell sdaftuar MarcoFalke

Labels
Tests