backport: Merge bitcoin#30252: test: Remove redundant verack check #6708
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0000276 test: Remove redundant verack check (MarcoFalke) Pull request description: Currently the sync in `connect_nodes` mentions the `version` and `verack` message types, but only checks the `verack`. Neither check is required, as the `pong` check implies both. In case of failure, the debug log will have to be consulted anyway, so the redundant check doesn't add value. Also clarify in the comments that the goal is to check the flag `fSuccessfullyConnected` indirectly. ACKs for top commit: furszy: utACK 0000276 brunoerg: ACK 0000276 tdb3: ACK 0000276 Tree-SHA512: f9ddcb1436d2f70da462a8dd470ecfc90a534dd6507c23877ef7626e7c02326c077001a42ad0171a87fba5c5275d1970d8c5e5d82c56c8412de944856fdfd6db
WalkthroughThe ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (1)
test/functional/test_framework/test_framework.py (1)
721-722: Consider line length compliance.Static analysis flagged these lines as exceeding the 100-character limit (up to 137 characters). While not affecting functionality, consider reformatting for better readability:
- self.wait_until(lambda: find_conn(from_connection, to_connection_subver, inbound=False) is not None, sleep=0.05) - self.wait_until(lambda: find_conn(to_connection, from_connection_subver, inbound=True) is not None, sleep=0.05) + self.wait_until( + lambda: find_conn(from_connection, to_connection_subver, inbound=False) is not None, + sleep=0.05 + ) + self.wait_until( + lambda: find_conn(to_connection, from_connection_subver, inbound=True) is not None, + sleep=0.05 + )Similar formatting could be applied to the longer
wait_untilcalls on lines 734-735.Also applies to: 734-735
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 721-721: Line too long (120/100)
(C0301)
[convention] 722-722: Line too long (119/100)
(C0301)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/functional/test_framework/test_framework.py(2 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
test/functional/test_framework/test_framework.py
[convention] 721-721: Line too long (120/100)
(C0301)
[convention] 722-722: Line too long (119/100)
(C0301)
[convention] 734-734: Line too long (137/100)
(C0301)
[convention] 735-735: Line too long (136/100)
(C0301)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (3)
test/functional/test_framework/test_framework.py (3)
2-2: LGTM: Copyright year update.The copyright year update to "2014-present" is appropriate and follows common practices for open source projects.
721-722: Good optimization: Reduced polling intervals.The addition of
sleep=0.05reduces polling intervals from the default 0.5 seconds to 0.05 seconds, which aligns with the PR objective to mitigate race conditions in connection establishment. This faster polling should help connections complete before potential timeouts.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 721-721: Line too long (120/100)
(C0301)
[convention] 722-722: Line too long (119/100)
(C0301)
728-735: Excellent logic simplification: Streamlined handshake detection.The change from waiting for explicit 'verack' messages to relying solely on 'pong' messages is a smart optimization. Since 'pong' messages can only be sent after the
fSuccessfullyConnectedflag is set, this approach effectively ensures handshake completion while reducing connection delays. The updated comments clearly explain the rationale and improve code maintainability.This change directly addresses the race condition mentioned in the PR objectives where longer delays increased the chance of disconnection during synchronization.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 734-734: Line too long (137/100)
(C0301)
[convention] 735-735: Line too long (136/100)
(C0301)
5 tasks
|
Looks good but PR title should be updated cause it's not a "fix", it's a "backport" |
Collaborator
Author
it had before some extra changes, but eventually they duplicate #6631 so I dropped them. updated! |
knst
changed the title
UdjinM6
approved these changes
Jun 3, 2025
PastaPastaPasta
added a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 5, 2025
Documents the change to enable BIP157 block filters by default.
PastaPastaPasta
added a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jul 16, 2025
Documents the change to enable BIP157 block filters by default.
PastaPastaPasta
added a commit
that referenced
this pull request
Aug 20, 2025
2b17480 fix(tests): add feature_masternode_params.py to test runner (pasta) bcff97a test: remove unused import from feature_masternode_params (pasta) fcd1b8c doc: add disk space requirement note to release notes (pasta) cec4ae0 test: address code review feedback for feature_masternode_params (pasta) 4e33c24 fix: specify utf8 (pasta) ac5c499 chore: trailing whitespace (pasta) 2fabaa9 fix: Address code review feedback for masternode-only BIP157 (pasta) 90ef2d5 refactor: Enable BIP157 block filters only for masternodes (pasta) d40ae45 test: fix tests incompatible with default peerblockfilters=1 (pasta) d6c11f0 test: fix feature_index_prune.py for new peerblockfilters default (pasta) 2a88db9 test: Fix feature_index_prune.py to explicitly disable indices (pasta) 4412ced test: Fix feature_index_prune.py to work with new blockfilterindex defaults (pasta) e6e9ff8 test: Fix functional tests to work with new blockfilterindex defaults (pasta) ef2a1e3 Fix initialization error when using default blockfilterindex value (pasta) fbde721 docs: Fix PR number in release notes - rename to 6711 (pasta) 0274cd2 docs: Add release notes for PR #6708 (pasta) ac7e03e Enable BIP157 block filters index and serving by default (pasta) Pull request description: ## Issue being fixed or feature implemented This pull request introduces automatic enabling of BIP157 compact block filters specifically for masternodes to improve privacy and functionality for light clients connecting to them. Regular nodes retain the existing default settings. ## What was done? - **Automatic Enablement for Masternodes**: When a node is configured as a masternode (via `-masternodeblsprivkey`), both `-peerblockfilters` and `-blockfilterindex=basic` are now automatically enabled. - **Regular Nodes Unchanged**: Regular (non-masternode) nodes keep the previous defaults with both options disabled (`-peerblockfilters=false` and `-blockfilterindex=0`). - **Opt-out Available**: Masternodes can still explicitly disable these features if needed by setting `-peerblockfilters=0` or `-blockfilterindex=0`. - **Release Notes**: Updated to clearly document this masternode-specific behavior and the ~1GB disk space requirement for the block filter index. - **Tests**: Added functional test to verify the parameter interactions and that the settings are correctly applied only to masternodes. ## How Has This Been Tested? - Built locally - Added new functional test `feature_masternode_params.py` that verifies: - Regular nodes have filters disabled by default - Masternodes have filters auto-enabled - Masternodes can explicitly disable filters - Parameter interaction logging works correctly ## Breaking Changes None - this only affects masternodes and they can opt out if needed. ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 2b17480 Tree-SHA512: 6a11305cf54c7584de4ca8dbe520e022e23f211ec6b0e2ee7679073f75a465ead927814f3f01f77e8576f96a187167511204d1906d380937f98310948933dc83
DashCoreAutoGuix
pushed a commit
to DashCoreAutoGuix/dash
that referenced
this pull request
Aug 22, 2025
Documents the change to enable BIP157 block filters by default.
knst
pushed a commit
to knst/dash
that referenced
this pull request
Sep 15, 2025
…nodes only 2b17480 fix(tests): add feature_masternode_params.py to test runner (pasta) bcff97a test: remove unused import from feature_masternode_params (pasta) fcd1b8c doc: add disk space requirement note to release notes (pasta) cec4ae0 test: address code review feedback for feature_masternode_params (pasta) 4e33c24 fix: specify utf8 (pasta) ac5c499 chore: trailing whitespace (pasta) 2fabaa9 fix: Address code review feedback for masternode-only BIP157 (pasta) 90ef2d5 refactor: Enable BIP157 block filters only for masternodes (pasta) d40ae45 test: fix tests incompatible with default peerblockfilters=1 (pasta) d6c11f0 test: fix feature_index_prune.py for new peerblockfilters default (pasta) 2a88db9 test: Fix feature_index_prune.py to explicitly disable indices (pasta) 4412ced test: Fix feature_index_prune.py to work with new blockfilterindex defaults (pasta) e6e9ff8 test: Fix functional tests to work with new blockfilterindex defaults (pasta) ef2a1e3 Fix initialization error when using default blockfilterindex value (pasta) fbde721 docs: Fix PR number in release notes - rename to 6711 (pasta) 0274cd2 docs: Add release notes for PR dashpay#6708 (pasta) ac7e03e Enable BIP157 block filters index and serving by default (pasta) Pull request description: ## Issue being fixed or feature implemented This pull request introduces automatic enabling of BIP157 compact block filters specifically for masternodes to improve privacy and functionality for light clients connecting to them. Regular nodes retain the existing default settings. ## What was done? - **Automatic Enablement for Masternodes**: When a node is configured as a masternode (via `-masternodeblsprivkey`), both `-peerblockfilters` and `-blockfilterindex=basic` are now automatically enabled. - **Regular Nodes Unchanged**: Regular (non-masternode) nodes keep the previous defaults with both options disabled (`-peerblockfilters=false` and `-blockfilterindex=0`). - **Opt-out Available**: Masternodes can still explicitly disable these features if needed by setting `-peerblockfilters=0` or `-blockfilterindex=0`. - **Release Notes**: Updated to clearly document this masternode-specific behavior and the ~1GB disk space requirement for the block filter index. - **Tests**: Added functional test to verify the parameter interactions and that the settings are correctly applied only to masternodes. ## How Has This Been Tested? - Built locally - Added new functional test `feature_masternode_params.py` that verifies: - Regular nodes have filters disabled by default - Masternodes have filters auto-enabled - Masternodes can explicitly disable filters - Parameter interaction logging works correctly ## Breaking Changes None - this only affects masternodes and they can opt out if needed. ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 2b17480 Tree-SHA512: 6a11305cf54c7584de4ca8dbe520e022e23f211ec6b0e2ee7679073f75a465ead927814f3f01f77e8576f96a187167511204d1906d380937f98310948933dc83
knst
pushed a commit
to knst/dash
that referenced
this pull request
Sep 17, 2025
…nodes only 2b17480 fix(tests): add feature_masternode_params.py to test runner (pasta) bcff97a test: remove unused import from feature_masternode_params (pasta) fcd1b8c doc: add disk space requirement note to release notes (pasta) cec4ae0 test: address code review feedback for feature_masternode_params (pasta) 4e33c24 fix: specify utf8 (pasta) ac5c499 chore: trailing whitespace (pasta) 2fabaa9 fix: Address code review feedback for masternode-only BIP157 (pasta) 90ef2d5 refactor: Enable BIP157 block filters only for masternodes (pasta) d40ae45 test: fix tests incompatible with default peerblockfilters=1 (pasta) d6c11f0 test: fix feature_index_prune.py for new peerblockfilters default (pasta) 2a88db9 test: Fix feature_index_prune.py to explicitly disable indices (pasta) 4412ced test: Fix feature_index_prune.py to work with new blockfilterindex defaults (pasta) e6e9ff8 test: Fix functional tests to work with new blockfilterindex defaults (pasta) ef2a1e3 Fix initialization error when using default blockfilterindex value (pasta) fbde721 docs: Fix PR number in release notes - rename to 6711 (pasta) 0274cd2 docs: Add release notes for PR dashpay#6708 (pasta) ac7e03e Enable BIP157 block filters index and serving by default (pasta) Pull request description: ## Issue being fixed or feature implemented This pull request introduces automatic enabling of BIP157 compact block filters specifically for masternodes to improve privacy and functionality for light clients connecting to them. Regular nodes retain the existing default settings. ## What was done? - **Automatic Enablement for Masternodes**: When a node is configured as a masternode (via `-masternodeblsprivkey`), both `-peerblockfilters` and `-blockfilterindex=basic` are now automatically enabled. - **Regular Nodes Unchanged**: Regular (non-masternode) nodes keep the previous defaults with both options disabled (`-peerblockfilters=false` and `-blockfilterindex=0`). - **Opt-out Available**: Masternodes can still explicitly disable these features if needed by setting `-peerblockfilters=0` or `-blockfilterindex=0`. - **Release Notes**: Updated to clearly document this masternode-specific behavior and the ~1GB disk space requirement for the block filter index. - **Tests**: Added functional test to verify the parameter interactions and that the settings are correctly applied only to masternodes. ## How Has This Been Tested? - Built locally - Added new functional test `feature_masternode_params.py` that verifies: - Regular nodes have filters disabled by default - Masternodes have filters auto-enabled - Masternodes can explicitly disable filters - Parameter interaction logging works correctly ## Breaking Changes None - this only affects masternodes and they can opt out if needed. ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 2b17480 Tree-SHA512: 6a11305cf54c7584de4ca8dbe520e022e23f211ec6b0e2ee7679073f75a465ead927814f3f01f77e8576f96a187167511204d1906d380937f98310948933dc83
knst
pushed a commit
to knst/dash
that referenced
this pull request
Sep 17, 2025
…nodes only 2b17480 fix(tests): add feature_masternode_params.py to test runner (pasta) bcff97a test: remove unused import from feature_masternode_params (pasta) fcd1b8c doc: add disk space requirement note to release notes (pasta) cec4ae0 test: address code review feedback for feature_masternode_params (pasta) 4e33c24 fix: specify utf8 (pasta) ac5c499 chore: trailing whitespace (pasta) 2fabaa9 fix: Address code review feedback for masternode-only BIP157 (pasta) 90ef2d5 refactor: Enable BIP157 block filters only for masternodes (pasta) d40ae45 test: fix tests incompatible with default peerblockfilters=1 (pasta) d6c11f0 test: fix feature_index_prune.py for new peerblockfilters default (pasta) 2a88db9 test: Fix feature_index_prune.py to explicitly disable indices (pasta) 4412ced test: Fix feature_index_prune.py to work with new blockfilterindex defaults (pasta) e6e9ff8 test: Fix functional tests to work with new blockfilterindex defaults (pasta) ef2a1e3 Fix initialization error when using default blockfilterindex value (pasta) fbde721 docs: Fix PR number in release notes - rename to 6711 (pasta) 0274cd2 docs: Add release notes for PR dashpay#6708 (pasta) ac7e03e Enable BIP157 block filters index and serving by default (pasta) Pull request description: ## Issue being fixed or feature implemented This pull request introduces automatic enabling of BIP157 compact block filters specifically for masternodes to improve privacy and functionality for light clients connecting to them. Regular nodes retain the existing default settings. ## What was done? - **Automatic Enablement for Masternodes**: When a node is configured as a masternode (via `-masternodeblsprivkey`), both `-peerblockfilters` and `-blockfilterindex=basic` are now automatically enabled. - **Regular Nodes Unchanged**: Regular (non-masternode) nodes keep the previous defaults with both options disabled (`-peerblockfilters=false` and `-blockfilterindex=0`). - **Opt-out Available**: Masternodes can still explicitly disable these features if needed by setting `-peerblockfilters=0` or `-blockfilterindex=0`. - **Release Notes**: Updated to clearly document this masternode-specific behavior and the ~1GB disk space requirement for the block filter index. - **Tests**: Added functional test to verify the parameter interactions and that the settings are correctly applied only to masternodes. ## How Has This Been Tested? - Built locally - Added new functional test `feature_masternode_params.py` that verifies: - Regular nodes have filters disabled by default - Masternodes have filters auto-enabled - Masternodes can explicitly disable filters - Parameter interaction logging works correctly ## Breaking Changes None - this only affects masternodes and they can opt out if needed. ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 2b17480 Tree-SHA512: 6a11305cf54c7584de4ca8dbe520e022e23f211ec6b0e2ee7679073f75a465ead927814f3f01f77e8576f96a187167511204d1906d380937f98310948933dc83
knst
pushed a commit
to knst/dash
that referenced
this pull request
Sep 18, 2025
…nodes only 2b17480 fix(tests): add feature_masternode_params.py to test runner (pasta) bcff97a test: remove unused import from feature_masternode_params (pasta) fcd1b8c doc: add disk space requirement note to release notes (pasta) cec4ae0 test: address code review feedback for feature_masternode_params (pasta) 4e33c24 fix: specify utf8 (pasta) ac5c499 chore: trailing whitespace (pasta) 2fabaa9 fix: Address code review feedback for masternode-only BIP157 (pasta) 90ef2d5 refactor: Enable BIP157 block filters only for masternodes (pasta) d40ae45 test: fix tests incompatible with default peerblockfilters=1 (pasta) d6c11f0 test: fix feature_index_prune.py for new peerblockfilters default (pasta) 2a88db9 test: Fix feature_index_prune.py to explicitly disable indices (pasta) 4412ced test: Fix feature_index_prune.py to work with new blockfilterindex defaults (pasta) e6e9ff8 test: Fix functional tests to work with new blockfilterindex defaults (pasta) ef2a1e3 Fix initialization error when using default blockfilterindex value (pasta) fbde721 docs: Fix PR number in release notes - rename to 6711 (pasta) 0274cd2 docs: Add release notes for PR dashpay#6708 (pasta) ac7e03e Enable BIP157 block filters index and serving by default (pasta) Pull request description: ## Issue being fixed or feature implemented This pull request introduces automatic enabling of BIP157 compact block filters specifically for masternodes to improve privacy and functionality for light clients connecting to them. Regular nodes retain the existing default settings. ## What was done? - **Automatic Enablement for Masternodes**: When a node is configured as a masternode (via `-masternodeblsprivkey`), both `-peerblockfilters` and `-blockfilterindex=basic` are now automatically enabled. - **Regular Nodes Unchanged**: Regular (non-masternode) nodes keep the previous defaults with both options disabled (`-peerblockfilters=false` and `-blockfilterindex=0`). - **Opt-out Available**: Masternodes can still explicitly disable these features if needed by setting `-peerblockfilters=0` or `-blockfilterindex=0`. - **Release Notes**: Updated to clearly document this masternode-specific behavior and the ~1GB disk space requirement for the block filter index. - **Tests**: Added functional test to verify the parameter interactions and that the settings are correctly applied only to masternodes. ## How Has This Been Tested? - Built locally - Added new functional test `feature_masternode_params.py` that verifies: - Regular nodes have filters disabled by default - Masternodes have filters auto-enabled - Masternodes can explicitly disable filters - Parameter interaction logging works correctly ## Breaking Changes None - this only affects masternodes and they can opt out if needed. ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 2b17480 Tree-SHA512: 6a11305cf54c7584de4ca8dbe520e022e23f211ec6b0e2ee7679073f75a465ead927814f3f01f77e8576f96a187167511204d1906d380937f98310948933dc83
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Functional test p2p_node_network_limited.py is occasionally fails with error while waiting
pongmessage:ot it fails while just waiting connection:
This failure happens, because node 2, once connected to node 0:
Timeline is here (some lines are omitted)
All of that happen while
connect_nodesin python code still validates if nodes already connected and it that would be eventually faster that python code moves to blocks syncing, the test will fail.What was done?
This PR helps
connect_nodesto work faster to increase chance to complete before nodes will be disconnected.The race is still here but it happens less often.
See also https://github.com/dashpay/dash/pull/6631/files which has further improvements for
connect_nodesby reducing default sleeps from 0.5 to 0.05How Has This Been Tested?
This PR should increase reliability of other functional tests as well, because it's not the only test that I have seen failed on connect_nodes; but p2p_node_network_limited.py only one that I tested.
To reproduce this issue I run stress benchmark that occupy all cores on my machine and after that run p2p_node_network_limited.py in 40 jobs.
This PR decreased amount of failures from 25% error rate to just 10%.
Breaking Changes
N/A
Checklist: