Fix bip68-sequence rpc test #11399

jl2012 commented at 10:16 AM on September 25, 2017: contributor

The test mined 1 extra block for the ACTIVE state. Test added to catch the right moment of LOCKED_IN->ACTIVE transaction

fanquake added the label Tests on Sep 25, 2017

jl2012 force-pushed on Sep 25, 2017

in test/functional/bip68-sequence.py:374 in 25bd48ff6c outdated

 370 | @@ -371,8 +371,10 @@ def activateCSV(self):
 371 |          # activation should happen at block height 432 (3 periods)
 372 |          min_activation_height = 432
 373 |          height = self.nodes[0].getblockcount()
 374 | -        assert(height < min_activation_height)

promag commented at 5:34 PM on September 25, 2017:

Nit, spaces around operators.

jnewbery commented at 2:45 PM on September 26, 2017:

also, prefer assert_greater_than() where possible (since it will print the result of both sides of the inequality if the assert fails)

promag commented at 5:35 PM on September 25, 2017: member

utACK 25bd48f.

in test/functional/bip68-sequence.py:376 in 25bd48ff6c outdated

 370 | @@ -371,8 +371,10 @@ def activateCSV(self):
 371 |          # activation should happen at block height 432 (3 periods)
 372 |          min_activation_height = 432
 373 |          height = self.nodes[0].getblockcount()
 374 | -        assert(height < min_activation_height)
 375 | -        self.nodes[0].generate(min_activation_height-height)
 376 | +        assert(min_activation_height-height > 2)
 377 | +        self.nodes[0].generate(min_activation_height-height-2)
 378 | +        assert(get_bip9_status(self.nodes[0], 'csv')['status'] == 'locked_in')

jnewbery commented at 2:45 PM on September 26, 2017:

prefer assert_equal()

in test/functional/bip68-sequence.py:371 in 25bd48ff6c outdated

 370 | @@ -371,8 +371,10 @@ def activateCSV(self):
 371 |          # activation should happen at block height 432 (3 periods)

jnewbery commented at 2:52 PM on September 26, 2017:

Seems like this could do with more commenting. Activation happens at the end of 3 periods since:

BIP68 bit is not yet defined in period 1
Signaling takes place in period 2
BIP68 is locked in during period 3.

getblockchaininfo will show BIP68 as active on at block 431 (144 * 3 -1) since it's returning whether BIP68 is active for the next block.

jnewbery commented at 2:52 PM on September 26, 2017: member

Concept ACK. Some nits inline.

jl2012 force-pushed on Sep 27, 2017

Fix bip68-sequence rpc test 49f869fe91

jl2012 force-pushed on Sep 27, 2017

jl2012 commented at 3:53 PM on September 27, 2017: contributor

@jnewbery fixed

in test/functional/bip68-sequence.py:375 in 49f869fe91

 373 |          min_activation_height = 432
 374 |          height = self.nodes[0].getblockcount()
 375 | -        assert(height < min_activation_height)
 376 | -        self.nodes[0].generate(min_activation_height-height)
 377 | -        assert(get_bip9_status(self.nodes[0], 'csv')['status'] == 'active')
 378 | +        assert_greater_than(min_activation_height - height, 2)

jnewbery commented at 8:19 PM on September 27, 2017:

Definitely a matter of personal taste, but:

assert_greater_than(min_activation_height - 2, height)

seems to me to convey the meaning better: that the current height is less than (activation height - 2).

jnewbery commented at 8:20 PM on September 27, 2017: member

One taste nit inline.

Obviously up to you whether you think that's clearer or not.

Tested ACK 49f869fe91716785b3276925d64bf8955feff69f either way.

jonasschnelli commented at 3:10 AM on September 28, 2017: contributor

utACK 49f869fe91716785b3276925d64bf8955feff69f

laanwj commented at 12:48 PM on October 2, 2017: member

Agree with @jnewbery's comment, though I don't think that is serious enough to hold up the merge utACK 49f869f

laanwj merged this on Oct 2, 2017

laanwj closed this on Oct 2, 2017

laanwj referenced this in commit 557aba6ce7 on Oct 2, 2017

MarcoFalke referenced this in commit 4bf7b271c8 on Oct 3, 2017

MarcoFalke referenced this in commit 2370b9041c on Oct 3, 2017

MarcoFalke referenced this in commit a9457b0cc2 on Oct 3, 2017

MarcoFalke referenced this in commit 4092025614 on Oct 3, 2017

MarcoFalke referenced this in commit a825d4af5e on Oct 3, 2017

codablock referenced this in commit 377a8fae2e on Sep 24, 2019

barrystyle referenced this in commit 808cd9ab08 on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021