test: fix `test_invalid_tx_in_compactblock` in `p2p

brunoerg commented at 9:54 pm on December 2, 2024: contributor

test_invalid_tx_in_compactblock tests that we don’t get disconnected if we relay a compact block with valid header, but invalid transactions.

In this test, after sending the block with invalid transactions, this test checks two things: the tip in the receiver node did not advance and the sender did not get disconnected. However, even if the block contains only valid transactions, the tip would not advance because the receiver does not have all transactions to reconstruct the valid and would request them back. This PR fixes it by sending all the transactions.

Also, comparing block hash (int) using is not can lead to subtle bugs, this PR fixes it by replacing it to !=.

Can be tested by applying:

 0diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
 1index 274ef9532c..419153a32f 100755
 2--- a/test/functional/p2p_compactblocks.py
 3+++ b/test/functional/p2p_compactblocks.py
 4@@ -723,11 +723,8 @@ class CompactBlocksTest(BitcoinTestFramework):
 5         utxo = self.utxos[0]
 6 
 7         block = self.build_block_with_transactions(node, utxo, 5)
 8-        del block.vtx[3]
 9         block.hashMerkleRoot = block.calc_merkle_root()
10         # Drop the coinbase witness but include the witness commitment.
11-        add_witness_commitment(block)
12-        block.vtx[0].wit.vtxinwit = []
13         block.solve()
14 
15         # Make sure node has the transactions to reconstruct the block

test: replace `is not` to `!=` when comparing block hash ee1b9bef00

DrahtBot commented at 9:54 pm on December 2, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31406.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	instagibbs, lucasbalieiro, glozow
Stale ACK	i-am-yuvi

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

DrahtBot added the label Tests on Dec 2, 2024

i-am-yuvi approved

i-am-yuvi commented at 9:25 am on December 3, 2024: none

Tested ACK 9e278f7f13395219ab056ba8c2810d3ce5c41d65

The test works correctly
For those who didn’t understand why its better to use != instead of is not here, since we are converting the tip hex to int and is not checks for object identity (if two references point to different objects in memory) which might not accurately capture intended comparison whereas != checks the numerical values directly.

lucasbalieiro approved

lucasbalieiro commented at 5:23 pm on December 3, 2024: none

Tested ACK: https://github.com/bitcoin/bitcoin/commit/9e278f7f13395219ab056ba8c2810d3ce5c41d65

Tested in my environment:

0$ lsb_release -a
1No LSB modules are available.
2Distributor ID: Ubuntu
3Description: Ubuntu 24.04.1 LTS
4Release: 24.04
5Codename: noble

Everything ran smoothly. I understand the necessity of the change to ensure correct comparisons between integer values.

For future reference, the comments from @i-am-yuvi are supported by the following documentation, which reinforces the fix proposed in this PR:

fanquake requested review from instagibbs on Dec 5, 2024

test: make sure node has all transactions 7239ddb7ce

in test/functional/p2p_compactblocks.py:734 in 9e278f7f13 outdated

729@@ -730,6 +730,10 @@ def test_invalid_tx_in_compactblock(self, test_node):
730         block.vtx[0].wit.vtxinwit = []
731         block.solve()
732 
733+        # Make sure node has the transactions to reconstruct the block
734+        for i in range(1, len(block.vtx)):

instagibbs commented at 5:05 pm on December 5, 2024:

all these transactions being sent are in the prefill_list which means its in the compact block message itself, can you explain what you’re solving here?

instagibbs commented at 5:13 pm on December 5, 2024:

to be honest I’m confused as to what the test is trying to cover as-is in master, it’s introducing 2 consensus errors, the one that is detected first is a coinbase witness issue

brunoerg commented at 5:34 pm on December 5, 2024:

all these transactions being sent are in the prefill_list which means its in the compact block message itself, can you explain what you’re solving here?

What this test (as-is) is doing is basically:

Create a block but dropping the coinbase witness and including the witness commitment.
Send the compact block with all transactions prefilled, and verify that we don’t get disconnected.
Check that the tip does not advance

The problem is, regardless the step 1, the tip will not advance anyway because, even if the block being valid, since the receiver does not have all transactions, it will request the missing ones back and until it doesn’t have all of them, the tip will not advance anyway. In this PR, I send the full txs before sending the compact block, so we can make sure the tip will not really advance due to “step 1”.

instagibbs commented at 5:47 pm on December 5, 2024:

fyi the block also deleted the 4th tx, making the final transaction invalid since they’re a chain. The last tx or two are simply invalid and won’t be in the mempool anyways.

This fails:

 0diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
 1index 274ef9532c..7b4eb8f986 100755
 2--- a/test/functional/p2p_compactblocks.py
 3+++ b/test/functional/p2p_compactblocks.py
 4@@ -731,10 +731,11 @@ class CompactBlocksTest(BitcoinTestFramework):
 5         block.solve()
 6 
 7         # Make sure node has the transactions to reconstruct the block
 8         for i in range(1, len(block.vtx)):
 9             test_node.send_and_ping(msg_tx(block.vtx[i]))
10+            assert block.vtx[i].sha256 in node.getrawmempool()
11 
12         # Now send the compact block with all transactions prefilled, and
13         # verify that we don't get disconnected.
14         comp_block = HeaderAndShortIDs()
15         comp_block.initialize_from_block(block, prefill_list=[0, 1, 2, 3, 4], use_witness=True)

Send the compact block with all transactions prefilled, and verify that we don’t get disconnected.

is contradicted by

since the receiver does not have all transactions

?

My logs suggest that all the transactions are found in the compact block since they’re passing merkle checks:

“2024-12-05T17:07:10.895018Z [msghand] [validation.cpp:4594] [ProcessNewBlock] [error] ProcessNewBl ock: AcceptBlock FAILED (bad-witness-nonce-size, CheckWitnessMalleation : invalid witness reserved value size)”

with the coinbase malleation removed even more clear:

2024-12-05T17:11:05.392987Z [msghand] [validationinterface.cpp:249] [BlockChecked] [validation] BlockChecked: block hash=34f96e2a2d2fe4ed23eb09f39a770a1946a22aac30ce46371c57473f26b52cfb state=bad-txns-inputs-missingorspent, CheckTxInputs: inputs missing/spent

instagibbs commented at 5:56 pm on December 5, 2024:

If you mean you want something more easily verifiable in case the block is actually correct somehow, then just change the prefill arg to make it the whole block?

0prefill_list=list(range(len(block.vtx)))

brunoerg commented at 7:08 pm on December 5, 2024:

If you mean you want something more easily verifiable in case the block is actually correct somehow, then just change the prefill arg to make it the whole block?

Yes, sorry for the confusion. “Send the compact block with all transactions prefilled” is basically the necessary transactions for the block, I meant the receiver doesn’t have ALL txs in fact. As-is, if we don’t delete vtx[3] then it would request it back and the tip wouldn’t advance anyway. I’ll take your suggestion.

brunoerg commented at 7:14 pm on December 5, 2024:

Done.

brunoerg force-pushed on Dec 5, 2024

instagibbs commented at 8:17 pm on December 5, 2024: member

ACK 7239ddb7cec38ab5a8aca93c18fb9efa6376421d

seems like an improvement in test reliability

DrahtBot requested review from i-am-yuvi on Dec 5, 2024

DrahtBot requested review from lucasbalieiro on Dec 5, 2024

lucasbalieiro approved

lucasbalieiro commented at 9:29 pm on December 5, 2024: none

Tested ACK for commit 7239ddb

Testing Procedure

1. PR Branch

The tests were run on the PR branch with the following results:

2. Against Master Branch

To verify consistency, I applied the changes from the modified file to the master branch and reran the tests.

Default build ran successfully, and the modified test passed

Environment Details

0$ lsb_release -a
1No LSB modules are available.
2Distributor ID:	Ubuntu
3Description:	Ubuntu 22.04.5 LTS
4Release:	22.04
5Codename:	jammy
6
7$ python3 --version
8Python 3.10.12

The changes work as expected on both the PR branch and the master branch when the modified file changes are applied. Let me know if additional tests or details are required.

glozow commented at 11:29 am on December 6, 2024: member

ACK 7239ddb7cec38ab5a8aca93c18fb9efa6376421d

glozow merged this on Dec 6, 2024

glozow closed this on Dec 6, 2024