Skip to content

Conversation

ogabrielides
Copy link

@ogabrielides ogabrielides commented Dec 21, 2022

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:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a 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

@PastaPastaPasta PastaPastaPasta merged commit c87ff11 into dashpay:develop Dec 27, 2022
@ogabrielides ogabrielides deleted the evo_deterministicmns_cleanup branch December 27, 2022 09:34
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants