test: use assert_greater_than util #30019

kevkevinpal · 2024-05-02T01:56:45Z

In the functional tests there are lots of cases where we assert < which we now swap with assert_greater_than to be more readable

This is motivated/uses logic from this PR which was closed #28528
This partially helps #23119

I've broken it up to just assert_greater_than to keep the PR smaller as suggested in #28528 (comment)

Similar change for assert_not_equal

I did not use the scripted-diff for the last commit since it either included a <= or there was already a () which did not match the pattern of the rest

if you run this command on this branch it should come back empty
git grep -nri -e "assert .*< " --and --not -e "assert .*!=" -- ./test/functional

DrahtBot · 2024-05-02T01:56:47Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	edilmedeiros

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29680 (wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict by Eunovo)
#29500 (test: create assert_not_equal util by kevkevinpal)
#28307 (rpc, wallet: fix incorrect segwit redeem script size limit by furszy)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

paplorinc

I'm not sure I understand what's wrong with the original operators, this seems less readable to me

paplorinc · 2024-05-02T10:22:55Z

test/functional/rpc_blockchain.py

 assert size > 6400
- assert size < 64000
+ assert_less_than(size, 64000)


This looks worse now

paplorinc · 2024-05-02T10:24:07Z

test/functional/test_framework/util.py

@@ -78,6 +78,10 @@ def assert_greater_than(thing1, thing2):
 if thing1 <= thing2:
 raise AssertionError("%s <= %s" % (str(thing1), str(thing2)))

+def assert_less_than(*args):


why so general, the previous one was a simple operator, why obfuscate it as such?

that is fair I initially did it like this because there was one scenario where we used two < operators but I now switched to using assert_greater_than and now broke that statement up to two statements

glozow

Why not just use assert_greater_than and assert_greater_than_or_equal?

In the functional tests there are lots of cases where we assert < which this util will replace, we also are adding the imports and the following commit will add the assertion

-BEGIN VERIFY SCRIPT- git grep -l -E "assert .*<{1} [\=\<]{0}" -- ':(exclude)*script.py' ':(exclude)*rpc_createmultisig.py' ':(exclude)*feature_taproot.py' ':(exclude)*wallet_create_tx.py' ':(exclude)*p2p.py' ./test/functional | xargs sed -i.bak -e "/assert .*< / s/assert \(.*\) <\([^#]*\)/assert\2 < \1/" git grep -l -E "assert .*<{1} [\=\<]{0}" -- ':(exclude)*script.py' ':(exclude)*rpc_createmultisig.py' ':(exclude)*feature_taproot.py' ':(exclude)*wallet_create_tx.py' ':(exclude)*p2p.py' ./test/functional | xargs sed -i.bak -e "/assert .*< / s/assert \(.*\) <\([^#]*\)/assert_greater_than(\1,\2)/g" -e "s/assert_greater_than(\(.*\)\(#.*\)/assert_greater_than(\1 \2/g" -e "/assert_greater_than(/ s/\ \ ,/,/g" -END VERIFY SCRIPT-

For the cases which the scripted diff did not make sense these were manually updated, like if we had multiple operators on one line or if it was wrapped in () already

kevkevinpal · 2024-05-03T22:09:12Z

pushed to fe07516

we now use assert_greater_than instead of creating a new util

04d8f07 contains a scripted-diff which looks for assert.*< and then swaps the two values and replaces the assert with assert_greater_than

paplorinc

The errors will improve this way, even though I wouldn't call the code more readable.
And to not make it less readable, I suggested a few other places where these changes could be applied.

paplorinc · 2024-05-04T16:27:54Z

test/functional/wallet_create_tx.py

@@ -45,7 +46,8 @@ def test_anti_fee_sniping(self):
 self.generate(self.nodes[0], 1)
 txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)
 tx = self.nodes[0].gettransaction(txid=txid, verbose=True)['decoded']
- assert 0 < tx['locktime'] <= 201
+ assert_greater_than(tx['locktime'], 0)
+ assert tx['locktime'] <= 201


Suggested change

assert tx['locktime'] <= 201

assert_greater_than(202, tx['locktime'])

paplorinc · 2024-05-04T16:31:14Z

test/functional/wallet_conflicts.py

@@ -104,7 +105,7 @@ def test_block_conflicts(self):

 self.log.info("Verify, after the reorg, that Tx_A was accepted, and tx_AB and its Child_Tx are conflicting now")
 # Tx A was accepted, Tx AB was not.
- assert conflicted_AB_tx["confirmations"] < 0
+ assert_greater_than(0, conflicted_AB_tx["confirmations"])
 assert conflicted_A_tx["confirmations"] > 0


this looks like an actual assert_greater_than

Suggested change

assert conflicted_A_tx["confirmations"] > 0

assert_greater_than(conflicted_A_tx["confirmations"], 0)

paplorinc · 2024-05-04T16:32:58Z

test/functional/test_framework/p2p.py

@@ -744,7 +745,8 @@ def listen(cls, p2p, callback, port=None, addr=None, idx=1):
 for connections, call `callback`."""

 if port is None:
- assert 0 < idx <= MAX_NODES
+ assert_greater_than(idx, 0)
+ assert idx <= MAX_NODES


Suggested change

assert idx <= MAX_NODES

assert_greater_than(MAX_NODES + 1, idx)

Why MAX_NODES + 1?

idx can equal MAX_NODES according to the code - alternatively could use assert_greater_than_or_equal, if that's more readable

paplorinc · 2024-05-04T16:34:52Z

test/functional/wallet_abandonconflict.py

@@ -53,7 +54,7 @@ def run_test(self):
 assert_raises_rpc_error(-5, 'Transaction not eligible for abandonment', lambda: alice.abandontransaction(txid=txA))

 newbalance = alice.getbalance()
- assert balance - newbalance < Decimal("0.001")  #no more than fees lost
+ assert_greater_than(Decimal("0.001"), balance - newbalance) #no more than fees lost


Do we need the Decimal here?
Seems basically the same as https://github.com/bitcoin/bitcoin/pull/30019/files?diff=split&w=1#diff-c1987c24fa3e5c788d16de5359500a19a1fea4f379e4288d076cf92be89541dcR478

This is needed as this specific test is full of Decimal objects. This is what you get if you take it out.

❯ test/functional/wallet_abandonconflict.py 2024-05-07T21:36:52.495000Z TestFramework (INFO): PRNG seed is: 7791852828985173276 2024-05-07T21:36:52.495000Z TestFramework (INFO): Initializing test directory /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30 2024-05-07T21:36:55.004000Z TestFramework (ERROR): Unexpected exception caught during testing Traceback (most recent call last): File "/Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/test_framework/test_framework.py", line 132, in main self.run_test() File "/Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/wallet_abandonconflict.py", line 57, in run_test assert_greater_than("0.001", balance - newbalance) #no more than fees lost ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/test_framework/util.py", line 78, in assert_greater_than if thing1 <= thing2: ^^^^^^^^^^^^^^^^ TypeError: '<=' not supported between instances of 'str' and 'decimal.Decimal' 2024-05-07T21:36:55.057000Z TestFramework (INFO): Stopping nodes 2024-05-07T21:36:55.165000Z TestFramework (WARNING): Not cleaning up dir /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30 2024-05-07T21:36:55.165000Z TestFramework (ERROR): Test failed. Test logging available at /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30/test_framework.log 2024-05-07T21:36:55.165000Z TestFramework (ERROR): 2024-05-07T21:36:55.165000Z TestFramework (ERROR): Hint: Call /Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/combine_logs.py '/var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30' to consolidate all logs 2024-05-07T21:36:55.165000Z TestFramework (ERROR): 2024-05-07T21:36:55.165000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log. 2024-05-07T21:36:55.165000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues 2024-05-07T21:36:55.166000Z TestFramework (ERROR):

I see you are a newcomer too. When reviewing code, better to open the files in the text editor and check for a broader context than what the github interface will present.

In my case, I use emacs with ediff, but all major IDEs will have a nice diff interface to inspect the PRs.

I see you are a newcomer too.

this isn't necessary...

assert_greater_than("0.001", balance - newbalance)

I wasn't suggesting comparing it to a string, of course, but to a decimal, as in the example I've provided, i.e.:

assert_greater_than(0.001, balance - newbalance)

This seems to be working for me, is it failing for you?

Oh, I see.

Still, better to leave to preserve potential bugs since this is refactoring.

paplorinc · 2024-05-04T16:36:02Z

test/functional/rpc_blockchain.py

@@ -339,7 +339,7 @@ def _test_gettxoutsetinfo(self):
 assert_equal(res['bestblock'], node.getblockhash(HEIGHT))
 size = res['disk_size']
 assert size > 6400


Suggested change

assert size > 6400

assert_greater_than(size, 6400)

paplorinc · 2024-05-04T16:38:01Z

test/functional/test_framework/script.py

@@ -813,7 +814,7 @@ def BIP341_sha_outputs(txTo):

 def TaprootSignatureMsg(txTo, spent_utxos, hash_type, input_index = 0, scriptpath = False, script = CScript(), codeseparator_pos = -1, annex = None, leaf_ver = LEAF_VERSION_TAPSCRIPT):
 assert (len(txTo.vin) == len(spent_utxos))


Suggested change

assert (len(txTo.vin) == len(spent_utxos))

assert_equal(len(txTo.vin), len(spent_utxos))

edilmedeiros

Concept ACK.

I have run all the tests that have changed, it seems ok.

Thanks for the contribution. But, since you are already refactoring this out, I believe it would be way better to not leave any more assert behind that could be changed to assert_equal or assert_greater_than. Otherwise, looks like we are not improving the code base as there will be more than one style of asserts in some files.

See the diffs in the comments to find some suggestions.

To find others, grep each file for 'assert ' (with the space).

edilmedeiros · 2024-05-07T21:09:32Z

test/functional/rpc_createmultisig.py

 assert bal1 == 0
 assert bal2 == self.moved


Change these to assert_equal is an easy gain here.

edilmedeiros · 2024-05-07T21:11:53Z

test/functional/test_framework/p2p.py

@@ -744,7 +745,8 @@ def listen(cls, p2p, callback, port=None, addr=None, idx=1):
 for connections, call `callback`."""

 if port is None:
- assert 0 < idx <= MAX_NODES
+ assert_greater_than(idx, 0)
+ assert idx <= MAX_NODES


Why MAX_NODES + 1?

edilmedeiros · 2024-05-07T21:37:32Z

test/functional/wallet_abandonconflict.py

@@ -53,7 +54,7 @@ def run_test(self):
 assert_raises_rpc_error(-5, 'Transaction not eligible for abandonment', lambda: alice.abandontransaction(txid=txA))

 newbalance = alice.getbalance()
- assert balance - newbalance < Decimal("0.001")  #no more than fees lost
+ assert_greater_than(Decimal("0.001"), balance - newbalance) #no more than fees lost


This is needed as this specific test is full of Decimal objects. This is what you get if you take it out.

❯ test/functional/wallet_abandonconflict.py 2024-05-07T21:36:52.495000Z TestFramework (INFO): PRNG seed is: 7791852828985173276 2024-05-07T21:36:52.495000Z TestFramework (INFO): Initializing test directory /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30 2024-05-07T21:36:55.004000Z TestFramework (ERROR): Unexpected exception caught during testing Traceback (most recent call last): File "/Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/test_framework/test_framework.py", line 132, in main self.run_test() File "/Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/wallet_abandonconflict.py", line 57, in run_test assert_greater_than("0.001", balance - newbalance) #no more than fees lost ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/test_framework/util.py", line 78, in assert_greater_than if thing1 <= thing2: ^^^^^^^^^^^^^^^^ TypeError: '<=' not supported between instances of 'str' and 'decimal.Decimal' 2024-05-07T21:36:55.057000Z TestFramework (INFO): Stopping nodes 2024-05-07T21:36:55.165000Z TestFramework (WARNING): Not cleaning up dir /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30 2024-05-07T21:36:55.165000Z TestFramework (ERROR): Test failed. Test logging available at /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30/test_framework.log 2024-05-07T21:36:55.165000Z TestFramework (ERROR): 2024-05-07T21:36:55.165000Z TestFramework (ERROR): Hint: Call /Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/combine_logs.py '/var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30' to consolidate all logs 2024-05-07T21:36:55.165000Z TestFramework (ERROR): 2024-05-07T21:36:55.165000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log. 2024-05-07T21:36:55.165000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues 2024-05-07T21:36:55.166000Z TestFramework (ERROR):

edilmedeiros · 2024-05-07T21:39:39Z

test/functional/wallet_abandonconflict.py

@@ -53,7 +54,7 @@ def run_test(self):
 assert_raises_rpc_error(-5, 'Transaction not eligible for abandonment', lambda: alice.abandontransaction(txid=txA))

 newbalance = alice.getbalance()
- assert balance - newbalance < Decimal("0.001")  #no more than fees lost
+ assert_greater_than(Decimal("0.001"), balance - newbalance) #no more than fees lost


I see you are a newcomer too. When reviewing code, better to open the files in the text editor and check for a broader context than what the github interface will present.

In my case, I use emacs with ediff, but all major IDEs will have a nice diff interface to inspect the PRs.

edilmedeiros · 2024-05-07T21:51:26Z

test/functional/feature_rbf.py

diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py index cf48826e6a..415e4f1b95 100755 --- a/test/functional/feature_rbf.py +++ b/test/functional/feature_rbf.py @@ -15,6 +15,7 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, assert_greater_than, + assert_greater_than_or_equal, assert_raises_rpc_error, ) from test_framework.wallet import MiniWallet @@ -446,9 +447,9 @@ class ReplaceByFeeTest(BitcoinTestFramework): num_txs_invalidated = len(root_utxos) + (num_tx_graphs * txs_per_graph) if failure_expected: - assert num_txs_invalidated > MAX_REPLACEMENT_LIMIT + assert_greater_than(num_txs_invalidated, MAX_REPLACEMENT_LIMIT) else: - assert num_txs_invalidated <= MAX_REPLACEMENT_LIMIT + assert_greater_than_or_equal(MAX_REPLACEMENT_LIMIT, num_txs_invalidated) # Now attempt to submit a tx that double-spends all the root tx inputs, which # would invalidate `num_txs_invalidated` transactions.

edilmedeiros · 2024-05-07T22:04:08Z

test/functional/p2p_segwit.py

index ff67207c4e..2a4cc9dfba 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -85,6 +85,7 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, assert_greater_than, + assert_greater_than_or_equal, softfork_active, assert_raises_rpc_error, ) @@ -376,14 +377,14 @@ class SegWitTest(BitcoinTestFramework): self.test_node.send_message(msg_headers()) self.test_node.announce_block_and_wait_for_getdata(block1, use_header=False) - assert self.test_node.last_message["getdata"].inv[0].type == blocktype + assert_equal(self.test_node.last_message["getdata"].inv[0].type, blocktype) test_witness_block(self.nodes[0], self.test_node, block1, True) block2 = self.build_next_block() block2.solve() self.test_node.announce_block_and_wait_for_getdata(block2, use_header=True) - assert self.test_node.last_message["getdata"].inv[0].type == blocktype + assert_equal(self.test_node.last_message["getdata"].inv[0].type, blocktype) test_witness_block(self.nodes[0], self.test_node, block2, True) # Check that we can getdata for witness blocks or regular blocks, @@ -412,8 +413,8 @@ class SegWitTest(BitcoinTestFramework): block = self.build_next_block() self.update_witness_block_with_transactions(block, []) # This gives us a witness commitment. - assert len(block.vtx[0].wit.vtxinwit) == 1 - assert len(block.vtx[0].wit.vtxinwit[0].scriptWitness.stack) == 1 + assert_equal(len(block.vtx[0].wit.vtxinwit), 1) + assert_equal(len(block.vtx[0].wit.vtxinwit[0].scriptWitness.stack), 1) test_witness_block(self.nodes[0], self.test_node, block, accepted=True) # Now try to retrieve it... rpc_block = self.nodes[0].getblock(block.hash, False) @@ -539,7 +540,7 @@ class SegWitTest(BitcoinTestFramework): # Verify that if a peer doesn't set nServices to include NODE_WITNESS, # the getdata is just for the non-witness portion. self.old_node.announce_tx_and_wait_for_getdata(tx) - assert self.old_node.last_message["getdata"].inv[0].type == MSG_TX + assert_equal(self.old_node.last_message["getdata"].inv[0].type, MSG_TX) # Since we haven't delivered the tx yet, inv'ing the same tx from # a witness transaction ought not result in a getdata. @@ -807,7 +808,7 @@ class SegWitTest(BitcoinTestFramework): block_3.vtx[0].vout[-1].nValue += 1 block_3.vtx[0].rehash() block_3.hashMerkleRoot = block_3.calc_merkle_root() - assert len(block_3.vtx[0].vout) == 4 # 3 OP_returns + assert_equal(len(block_3.vtx[0].vout), 4) # 3 OP_returns block_3.solve() test_witness_block(self.nodes[0], self.test_node, block_3, accepted=True) @@ -838,7 +839,7 @@ class SegWitTest(BitcoinTestFramework): block.solve() block.vtx[0].wit.vtxinwit[0].scriptWitness.stack.append(b'a' * 5000000) - assert block.get_weight() > MAX_BLOCK_WEIGHT + assert_greater_than(block.get_weight(), MAX_BLOCK_WEIGHT) # We can't send over the p2p network, because this is too big to relay # TODO: repeat this test with a block that can be relayed @@ -850,7 +851,7 @@ class SegWitTest(BitcoinTestFramework): assert_greater_than(MAX_BLOCK_WEIGHT, block.get_weight()) assert_equal(None, self.nodes[0].submitblock(block.serialize().hex())) - assert self.nodes[0].getbestblockhash() == block.hash + assert_equal(self.nodes[0].getbestblockhash(), block.hash) # Now make sure that malleating the witness reserved value doesn't # result in a block permanently marked bad. @@ -875,7 +876,7 @@ class SegWitTest(BitcoinTestFramework): # Test that witness-bearing blocks are limited at ceil(base + wit/4) <= 1MB. block = self.build_next_block() - assert len(self.utxo) > 0 + assert_greater_than(len(self.utxo), 0) # Create a P2WSH transaction. # The witness script will be a bunch of OP_2DROP's, followed by OP_TRUE. @@ -896,7 +897,7 @@ class SegWitTest(BitcoinTestFramework): for _ in range(NUM_OUTPUTS): parent_tx.vout.append(CTxOut(child_value, script_pubkey)) parent_tx.vout[0].nValue -= 50000 - assert parent_tx.vout[0].nValue > 0 + assert_greater_than(parent_tx.vout[0].nValue, 0) parent_tx.rehash() child_tx = CTransaction() @@ -924,7 +925,7 @@ class SegWitTest(BitcoinTestFramework): assert_equal(block.get_weight(), MAX_BLOCK_WEIGHT + 1) # Make sure that our test case would exceed the old max-network-message # limit - assert len(block.serialize()) > 2 * 1024 * 1024 + assert_greater_than(len(block.serialize()), 2 * 1024 * 1024) test_witness_block(self.nodes[0], self.test_node, block, accepted=False, reason='bad-blk-we ight') @@ -934,7 +935,7 @@ class SegWitTest(BitcoinTestFramework): block.vtx[0].vout.pop() add_witness_commitment(block) block.solve() - assert block.get_weight() == MAX_BLOCK_WEIGHT + assert_equal(block.get_weight(), MAX_BLOCK_WEIGHT) test_witness_block(self.nodes[0], self.test_node, block, accepted=True) @@ -1098,7 +1099,7 @@ class SegWitTest(BitcoinTestFramework): # This script is 19 max pushes (9937 bytes), then 64 more opcode-bytes. long_witness_script = CScript([b'a' * MAX_SCRIPT_ELEMENT_SIZE] * 19 + [OP_DROP] * 63 + [OP_ TRUE]) - assert len(long_witness_script) == MAX_WITNESS_SCRIPT_LENGTH + 1 + assert_equal(len(long_witness_script), MAX_WITNESS_SCRIPT_LENGTH + 1) long_script_pubkey = script_to_p2wsh_script(long_witness_script) block = self.build_next_block() @@ -1122,7 +1123,7 @@ class SegWitTest(BitcoinTestFramework): # Try again with one less byte in the witness script witness_script = CScript([b'a' * MAX_SCRIPT_ELEMENT_SIZE] * 19 + [OP_DROP] * 62 + [OP_TRUE] ) - assert len(witness_script) == MAX_WITNESS_SCRIPT_LENGTH + assert_equal(len(witness_script), MAX_WITNESS_SCRIPT_LENGTH) script_pubkey = script_to_p2wsh_script(witness_script) tx.vout[0] = CTxOut(tx.vout[0].nValue, script_pubkey) @@ -1151,7 +1152,7 @@ class SegWitTest(BitcoinTestFramework): for _ in range(10): tx.vout.append(CTxOut(int(value / 10), script_pubkey)) tx.vout[0].nValue -= 1000 - assert tx.vout[0].nValue >= 0 + assert_greater_than_or_equal(tx.vout[0].nValue, 0) block = self.build_next_block() self.update_witness_block_with_transactions(block, [tx]) @@ -1358,7 +1359,7 @@ class SegWitTest(BitcoinTestFramework): temp_utxo.append(UTXO(tx.sha256, 0, tx.vout[0].nValue)) self.generate(self.nodes[0], 1) # Mine all the transactions - assert len(self.nodes[0].getrawmempool()) == 0 + assert_equal(len(self.nodes[0].getrawmempool()), 0) # Finally, verify that version 0 -> version 2 transactions # are standard @@ -1622,7 +1623,7 @@ class SegWitTest(BitcoinTestFramework): # Create a slight bias for producing more utxos num_outputs = random.randint(1, 11) random.shuffle(temp_utxos) - assert len(temp_utxos) > num_inputs + assert_greater_than(len(temp_utxos), num_inputs) tx = CTransaction() total_value = 0 for i in range(num_inputs):

edilmedeiros

Another thing: I have the impression that the individual commits do not work individually. If so, think about squashing them all together. I don't think this will break the bank for other reviewers.

edilmedeiros · 2024-05-07T22:42:31Z

All right, better than my review would be to convert this to a scripted diff as you are doing in #29500.

DrahtBot added the Tests label May 2, 2024

paplorinc reviewed May 2, 2024

View reviewed changes

glozow reviewed May 2, 2024

View reviewed changes

This was referenced May 2, 2024

wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict #29680

Open

test: create assert_not_equal util #29500

Open

rpc, wallet: fix incorrect segwit redeem script size limit #28307

Open

kevkevinpal added 3 commits May 3, 2024 15:55

test: use assert_greater_than util and add to where imports are needed

e231cc7

In the functional tests there are lots of cases where we assert < which this util will replace, we also are adding the imports and the following commit will add the assertion

test: using assert_greater_than for edge cases

fe07516

For the cases which the scripted diff did not make sense these were manually updated, like if we had multiple operators on one line or if it was wrapped in () already

kevkevinpal force-pushed the testFrameworkLessthan branch from c51464c to fe07516 Compare May 3, 2024 22:04

kevkevinpal changed the title ~~test: create assert_less_than util~~ test: use assert_greater_than util May 3, 2024

paplorinc suggested changes May 4, 2024

View reviewed changes

edilmedeiros suggested changes May 7, 2024

View reviewed changes

edilmedeiros reviewed May 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: use assert_greater_than util #30019

test: use assert_greater_than util #30019

kevkevinpal commented May 2, 2024 •

edited

DrahtBot commented May 2, 2024 •

edited

paplorinc left a comment

paplorinc May 2, 2024

paplorinc May 2, 2024

kevkevinpal May 3, 2024

glozow left a comment •

edited

kevkevinpal commented May 3, 2024

paplorinc left a comment

paplorinc May 4, 2024

paplorinc May 4, 2024

paplorinc May 4, 2024

edilmedeiros May 7, 2024

paplorinc May 8, 2024 •

edited

paplorinc May 4, 2024

edilmedeiros May 7, 2024

edilmedeiros May 7, 2024

paplorinc May 8, 2024

edilmedeiros May 8, 2024

paplorinc May 4, 2024

paplorinc May 4, 2024

edilmedeiros left a comment

edilmedeiros May 7, 2024

edilmedeiros May 7, 2024

edilmedeiros May 7, 2024

edilmedeiros May 7, 2024

edilmedeiros May 7, 2024

edilmedeiros May 7, 2024

edilmedeiros left a comment

edilmedeiros commented May 7, 2024

	assert tx['locktime'] <= 201
	assert_greater_than(202, tx['locktime'])

	assert conflicted_A_tx["confirmations"] > 0
	assert_greater_than(conflicted_A_tx["confirmations"], 0)

	assert idx <= MAX_NODES
	assert_greater_than(MAX_NODES + 1, idx)

		@@ -813,7 +814,7 @@ def BIP341_sha_outputs(txTo):

		def TaprootSignatureMsg(txTo, spent_utxos, hash_type, input_index = 0, scriptpath = False, script = CScript(), codeseparator_pos = -1, annex = None, leaf_ver = LEAF_VERSION_TAPSCRIPT):
		assert (len(txTo.vin) == len(spent_utxos))

	assert (len(txTo.vin) == len(spent_utxos))
	assert_equal(len(txTo.vin), len(spent_utxos))

test: use assert_greater_than util #30019

Are you sure you want to change the base?

test: use assert_greater_than util #30019

Conversation

kevkevinpal commented May 2, 2024 • edited

DrahtBot commented May 2, 2024 • edited

Code Coverage

Reviews

Conflicts

paplorinc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glozow left a comment • edited

Choose a reason for hiding this comment

kevkevinpal commented May 3, 2024

paplorinc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paplorinc May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edilmedeiros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edilmedeiros left a comment

Choose a reason for hiding this comment

edilmedeiros commented May 7, 2024

kevkevinpal commented May 2, 2024 •

edited

DrahtBot commented May 2, 2024 •

edited

glozow left a comment •

edited

paplorinc May 8, 2024 •

edited