forked from bitcoin/bitcoin
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
refactor: Removed old evo/deterministicmns parts #5113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
PastaPastaPasta
merged 1 commit into
dashpay:develop
from
ogabrielides:evo_deterministicmns_cleanup
Dec 27, 2022
Merged
refactor: Removed old evo/deterministicmns parts #5113
PastaPastaPasta
merged 1 commit into
dashpay:develop
from
ogabrielides:evo_deterministicmns_cleanup
Dec 27, 2022
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
5 tasks
UdjinM6
approved these changes
Dec 26, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
PastaPastaPasta
approved these changes
Dec 27, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK for squash merge
gades
pushed a commit
to cosanta/cosanta-core
that referenced
this pull request
Nov 20, 2023
fadfd84 test: Remove unused connect_nodes_bi (MarcoFalke) fa3b9ee scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke) faaee1e test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke) 1111bb9 test: Reformat python imports to aid scripted diff (MarcoFalke) Pull request description: By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side). This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times. Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests. Historic background: `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug dashpay#5113 and dashpay#5138, which has long been fixed in dashpay#5157 and dashpay#5662. ACKs for top commit: laanwj: ACK fadfd84 jonasschnelli: utACK fadfd84 - more of less a cleanup PR. promag: Tested ACK fadfd84, ran extended tests. Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24
gades
pushed a commit
to piratecash/pirate
that referenced
this pull request
Dec 9, 2023
fadfd84 test: Remove unused connect_nodes_bi (MarcoFalke) fa3b9ee scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke) faaee1e test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke) 1111bb9 test: Reformat python imports to aid scripted diff (MarcoFalke) Pull request description: By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side). This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times. Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests. Historic background: `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug dashpay#5113 and dashpay#5138, which has long been fixed in dashpay#5157 and dashpay#5662. ACKs for top commit: laanwj: ACK fadfd84 jonasschnelli: utACK fadfd84 - more of less a cleanup PR. promag: Tested ACK fadfd84, ran extended tests. Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
What was done?
Removed code related to the upgrade for old
CDeterministicMNListDiff
format to new format.This was implemented in #3017.
I believe we can safely remove this now
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only