backport: Merge #23474: test: scripted-diff cleanups after generate* changes #6584
Conversation
WalkthroughThe changes modify multiple functional tests by removing or altering explicit synchronization calls such as 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (32)
💤 Files with no reviewable changes (23)
🚧 Files skipped from review as they are similar to previous changes (9)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
test/functional/feature_backwards_compatibility.py(0 hunks)test/functional/feature_coinstatsindex.py(0 hunks)test/functional/feature_dip4_coinbasemerkleroots.py(2 hunks)test/functional/feature_fee_estimation.py(0 hunks)test/functional/feature_index_prune.py(0 hunks)test/functional/feature_llmq_dkgerrors.py(1 hunks)test/functional/feature_llmq_rotation.py(5 hunks)test/functional/feature_llmq_simplepose.py(2 hunks)test/functional/mempool_packages.py(0 hunks)test/functional/mining_getblocktemplate_longpoll.py(0 hunks)test/functional/p2p_blockfilters.py(0 hunks)test/functional/p2p_compactblocks_hb.py(0 hunks)test/functional/p2p_node_network_limited.py(1 hunks)test/functional/p2p_sendheaders.py(0 hunks)test/functional/rpc_coinjoin.py(1 hunks)test/functional/rpc_rawtransaction.py(0 hunks)test/functional/test_framework/test_framework.py(0 hunks)test/functional/wallet_abandonconflict.py(0 hunks)test/functional/wallet_backup.py(0 hunks)test/functional/wallet_balance.py(0 hunks)test/functional/wallet_basic.py(0 hunks)test/functional/wallet_hd.py(0 hunks)test/functional/wallet_importdescriptors.py(1 hunks)test/functional/wallet_keypool_topup.py(0 hunks)test/functional/wallet_listreceivedby.py(0 hunks)test/functional/wallet_mnemonicbits.py(1 hunks)test/functional/wallet_multisig_descriptor_psbt.py(0 hunks)test/functional/wallet_orphanedreward.py(0 hunks)test/functional/wallet_reorgsrestore.py(0 hunks)test/functional/wallet_send.py(0 hunks)test/functional/wallet_txn_clone.py(0 hunks)test/functional/wallet_txn_doublespend.py(0 hunks)
💤 Files with no reviewable changes (24)
- test/functional/wallet_listreceivedby.py
- test/functional/mining_getblocktemplate_longpoll.py
- test/functional/wallet_multisig_descriptor_psbt.py
- test/functional/wallet_txn_doublespend.py
- test/functional/wallet_backup.py
- test/functional/p2p_blockfilters.py
- test/functional/feature_fee_estimation.py
- test/functional/feature_backwards_compatibility.py
- test/functional/wallet_orphanedreward.py
- test/functional/wallet_txn_clone.py
- test/functional/wallet_abandonconflict.py
- test/functional/feature_coinstatsindex.py
- test/functional/wallet_keypool_topup.py
- test/functional/p2p_compactblocks_hb.py
- test/functional/mempool_packages.py
- test/functional/wallet_send.py
- test/functional/wallet_reorgsrestore.py
- test/functional/wallet_balance.py
- test/functional/wallet_hd.py
- test/functional/p2p_sendheaders.py
- test/functional/feature_index_prune.py
- test/functional/rpc_rawtransaction.py
- test/functional/test_framework/test_framework.py
- test/functional/wallet_basic.py
✅ Files skipped from review due to trivial changes (2)
- test/functional/rpc_coinjoin.py
- test/functional/wallet_importdescriptors.py
🧰 Additional context used
🪛 GitHub Actions: Check Potential Conflicts
test/functional/feature_dip4_coinbasemerkleroots.py
[error] 1-1: Conflicts detected with multiple pull requests.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (10)
test/functional/p2p_node_network_limited.py (1)
70-70: LGTM! Improved synchronization clarity.The change from
self.no_opto an explicit synchronization lambda makes the code's intent clearer and ensures proper synchronization between the relevant nodes. This is particularly important as these blocks are crucial for testing the NODE_NETWORK_LIMITED functionality.test/functional/wallet_mnemonicbits.py (1)
2-2: LGTM!The copyright year update is accurate and consistent with other files in the PR.
test/functional/feature_llmq_dkgerrors.py (2)
2-2: LGTM!The copyright year update is accurate and consistent with other files in the PR.
88-88: LGTM!The use of
sync_fun=self.no_opwithgenerateis consistent with the PR's objective of cleaning up synchronization calls. Since this is part of the healing process and immediate synchronization isn't required, this change is appropriate.test/functional/feature_llmq_simplepose.py (2)
2-2: LGTM: Copyright year updated.Updated copyright year to reflect current year 2025.
112-112:❓ Verification inconclusive
LGTM: Simplified block generation call.
Removed unnecessary sync_fun parameter in generate call, which aligns with the function's purpose of performing less synchronization checks as documented in the method's comments.
Let's verify if this change is consistent with other test files:
🏁 Script executed:
#!/bin/bash # Description: Check for similar sync_fun parameter usage patterns in other test files # Search for generate calls with sync_fun parameter rg 'generate.*sync_fun.*=.*self\.no_op' test/functional/Length of output: 19228
LGTM: Simplified block generation call.
The updated call in
test/functional/feature_llmq_simplepose.pynow omits thesync_funparameter—this appears intentional based on the method’s comments aiming for reduced synchronization overhead. Note that while many other tests continue to passsync_fun=self.no_op, this file’s change seems context-specific. Please verify that this divergence is deliberate and that the behavior in llmq_simplepose remains correct.test/functional/feature_dip4_coinbasemerkleroots.py (3)
2-2: Update copyright year.The copyright year has been updated to reflect the current year.
1-321:❓ Verification inconclusive
Resolve potential conflicts before merging.
The GitHub Actions pipeline has detected potential conflicts with other pull requests. Please rebase this branch on the latest master and resolve any conflicts.
🏁 Script executed:
#!/bin/bash # Description: Check for potential merge conflicts with master # Fetch the latest master git fetch origin master # Check for conflicts git merge-tree $(git merge-base HEAD origin/master) origin/master HEADLength of output: 287
Action Required: Verify and Resolve Merge Conflicts
The GitHub Actions pipeline reported potential conflicts, but the automated merge-tree command using
origin/masterfailed with an error. This suggests that the default remote branch might not be named "master" (it could be "main" or another name). Please verify manually that your branch is rebased onto the correct base branch (e.g., "origin/main") and that no merge conflicts exist. You can check available remote branches with:git branch -rThen, if "origin/main" is the appropriate base, run:
git fetch origin main git merge-tree $(git merge-base HEAD origin/main) origin/main HEADResolve any conflicts revealed by these commands before merging.
🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] 1-1: Conflicts detected with multiple pull requests.
317-317:❓ Verification inconclusive
Verify the impact of switching to
sync_all().The change from
sync_blocks()tosync_all()in theconfirm_mnsfunction is part of a broader pattern of synchronization method updates. Whilesync_all()provides more comprehensive synchronization by including transactions and other state information, we should verify that this change doesn't introduce any timing issues or performance impacts.
🏁 Script executed:
#!/bin/bash # Description: Search for other instances of sync_blocks() to ensure consistency # and identify potential patterns in sync method usage. echo "Searching for sync_blocks() usage patterns..." rg -A 3 'sync_blocks\(\)' test/functional/ echo "Searching for sync_all() usage patterns..." rg -A 3 'sync_all\(\)' test/functional/Length of output: 48885
Double-check the synchronization impact of using
sync_all()The update in the
confirm_mnsfunction to callsync_all()—which now offers broader state synchronization (including transactions and additional details)—appears consistent with changes observed across several test files (e.g., in wallet tests and DIP3-related tests). However, since many parts of the codebase still rely onsync_blocks(), please verify that this change does not introduce timing or performance overhead in time-sensitive scenarios. In particular:
- Confirm that using
sync_all()intest/functional/feature_dip4_coinbasemerkleroots.py(line 317) does not delay synchronization compared to previous tests.- Check for any subtle performance regressions or intermittent issues resulting from additional synchronization steps.
test/functional/feature_llmq_rotation.py (1)
2-2: LGTM!Copyright year updated to 2025.
|
|
||
| self.log.info("Mine single block, wait for chainlock") | ||
| self.generate(self.nodes[0], 1, sync_fun=self.no_op) | ||
| self.generate(self.nodes[0], 1) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify test stability after removing synchronization.
The removal of sync_fun parameter from generate calls might affect test reliability. While this aligns with the broader changes to simplify synchronization across test files (as mentioned in the PR objectives), we should ensure that the test remains stable, especially given the complex nature of LLMQ rotation testing.
Also applies to: 118-118, 199-199, 391-391
🏁 Script executed:
#!/bin/bash
# Description: Check test stability by running it multiple times
# Note: This is a basic stability check. In practice, the test framework would handle this.
for i in {1..3}; do
echo "Running test iteration $i"
python3 test/functional/feature_llmq_rotation.py
if [ $? -ne 0 ]; then
echo "Test failed on iteration $i"
exit 1
fi
doneLength of output: 982
Action Required: Test failure due to missing dependency 'dash_hash'
The removal of the sync_fun parameter remains valid, but the test now fails because the module dash_hash is missing. This failure prevents verification of test stability for LLMQ rotation. Please ensure that:
- The dependency
dash_hashis either added to the project (e.g., updating requirements or including it in the environment) or removed from the import intest/functional/test_framework/messages.pyif it’s no longer needed. - Re-run the stability checks for all affected lines (87, 118, 199, and 391) once the issue is addressed.
knst
force-pushed
the
bp-v23-p8
branch
2 times, most recently
from
c4d9bec to
9de8ac1
Compare
|
Somehow, |
|
This pull request has conflicts, please rebase. |
The sync calls are redundant after a call to generate, because generate already syncs itself. -BEGIN VERIFY SCRIPT- perl -0777 -pi -e 's/(generate[^\n]*\)[^\n]*)(\n|\s)+self.sync_(all|blocks)\([^\)]*\)\n/\1\n/g' $(git grep -l generate ./test) -END VERIFY SCRIPT-
The previous diff touched most files in ./test/, so bump the headers to avoid having to touch them again for a bump later. BEGIN VERIFY SCRIPT- ./contrib/devtools/copyright_header.py update ./test/ END VERIFY SCRIPT-
3 participants
What was done?
The majority of changes in this backport are scripted, better to keep commits as they are rather than squash together.
Original description:
How Has This Been Tested?
Run unit functional tests.
Breaking Changes
N/A
Checklist: