[tests] Change feature_csv_activation.py to use BitcoinTestFramework #11817

 119 | +    if srlb:
 120 | +        locktime |= SEQ_RANDOM_LOW_BIT
 121 | +    return locktime
 122 | +
 123 | +def all_rlt_txs(txs):
 124 | +    return [txs[tx]['tx'] for tx in txs]

ryanofsky commented at 8:56 PM on December 20, 2017:

In commit "Remove nested loops from bip68-112-113-p2p.py"

Should be able to avoid a lookup with [tx['tx'] for tx in txs.values()]

jnewbery commented at 3:32 AM on February 14, 2018:

Yes, that's better. Fixed

 136 | @@ -152,24 +137,18 @@ def create_test_block(self, txs, version=536870912):
 137 |          return block
 138 |  
 139 |      def create_bip68txs(self, bip68inputs, txversion, locktime_delta=0):
 140 | -        txs = []
 141 | +        """Returns a dictionary of bip68 transactions with different bits set."""

ryanofsky commented at 9:03 PM on December 20, 2017:

In commit "Remove nested loops from bip68-112-113-p2p.py"

It seems better to continue returning a list than a dictionary. A dictionary is just slower and introduces nondeterminism in the test for no reason.

jnewbery commented at 9:42 PM on February 14, 2018:

yes - returning a list is much better. Changed. Thanks

 254 | +        self.nodes[0].setmocktime(long_past_time - 100)  # enough so that the generated blocks will still all be before long_past_time
 255 | +        self.coinbase_blocks = self.nodes[0].generate(1 + 16 + 2 * 32 + 1)  # 82 blocks generated for inputs
 256 | +        self.nodes[0].setmocktime(0)  # set time back to present so yielded blocks aren't in the future as we advance last_block_time
 257 | +        self.tipheight = 82  # height of the next block to build
 258 |          self.last_block_time = long_past_time
 259 |          self.tip = int("0x" + self.nodes[0].getbestblockhash(), 0)

conscott commented at 4:52 PM on February 16, 2018:

To go along with @ryanofsky earlier comments in related PRs. This can be simplified to int(hash,16)

 352 |  
 353 |          self.nodes[0].setmocktime(self.last_block_time + 600)
 354 | -        inputblockhash = self.nodes[0].generate(1)[0] # 1 block generated for inputs to be in chain at height 572
 355 | +        inputblockhash = self.nodes[0].generate(1)[0]  # 1 block generated for inputs to be in chain at height 572
 356 |          self.nodes[0].setmocktime(0)
 357 |          self.tip = int("0x" + inputblockhash, 0)

conscott commented at 4:52 PM on February 16, 2018:

Same here. Can be int(hash,16)

 161 | +            tx = self.create_transaction(self.nodes[0], bip68inputs[i], self.nodeaddress, Decimal("49.98"))
 162 | +            tx.nVersion = txversion
 163 | +            tx.vin[0].nSequence = locktime + locktime_delta
 164 | +            tx = self.sign_transaction(self.nodes[0], tx)
 165 | +            tx.rehash()
 166 | +            txs.append({'tx': tx, 'sdf': sdf, 'srhb': srhb, 'stf': stf, 'srlb': srlb})

MarcoFalke commented at 9:31 PM on March 21, 2018:

In commit c5427d1f4e3652a1b55968c752fb362b31996186

The "flags" to indicate if the random high- or low-bit was set are never read. Might as well not include it in the object.

jnewbery commented at 1:39 PM on March 22, 2018:

Yep, good spot. I've removed the 'srhb' and 'srlb' keys.

 198 | +            if (varyOP_CSV):
 199 | +                signtx.vin[0].scriptSig = CScript([locktime, OP_CHECKSEQUENCEVERIFY, OP_DROP] + list(CScript(signtx.vin[0].scriptSig)))
 200 | +            else:
 201 | +                signtx.vin[0].scriptSig = CScript([BASE_RELATIVE_LOCKTIME, OP_CHECKSEQUENCEVERIFY, OP_DROP] + list(CScript(signtx.vin[0].scriptSig)))
 202 | +            tx.rehash()
 203 | +            txs.append({'tx': signtx, 'sdf': sdf, 'srhb': srhb, 'stf': stf, 'srlb': srlb})

MarcoFalke commented at 9:32 PM on March 21, 2018:

Same here

jnewbery commented at 1:40 PM on March 22, 2018:

fixed as above

 461 | -        for b25 in range(2):
 462 | -            for b22 in range(2):
 463 | -                for b18 in range(2):
 464 | -                    fail_txs.append(bip112txs_vary_OP_CSV_9_v2[0][b25][b22][b18])  # 16/16 of vary_OP_CSV_9
 465 | -
 466 | +        fail_txs = all_rlt_txs(bip112txs_vary_nSequence_9_v2)  # 16/16 of vary_nSequence_9

MarcoFalke commented at 9:32 PM on March 21, 2018:

In commit c5427d1f4e3652a1b55968c752fb362b31996186:

Any reason you remove the trailing comments in all lines but not in this one?

jnewbery commented at 1:39 PM on March 22, 2018:

I missed this one. Now removed.

 418 | -        yield TestInstance([[self.create_test_block(bip68success_txs), True]])
 419 | +        self.sync_blocks([self.create_test_block(bip68success_txs)])
 420 |          self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash())
 421 |          for tx in bip68heighttxs:
 422 | -            yield TestInstance([[self.create_test_block([tx]), False]])
 423 | +            self.sync_blocks([self.create_test_block([tx])], False)

MarcoFalke commented at 9:52 PM on March 21, 2018:

In commit cb5b047a6f6fb58afee:

Nit: Sometimes you use named args for the bool, and sometimes you don't. Might as well use the success=False everywhere.

jnewbery commented at 1:39 PM on March 22, 2018:

sounds good. Updated.