Skip to content

Conversation

@kwvg
Copy link
Collaborator

@kwvg kwvg commented Sep 1, 2024

Additional Information

  • In bitcoin#22831, when restarting the node in rpc_net.py's test_addpeeraddress(), existing commands need to be appended to extra_args to ensure they're retained when the node is restarted (default behavior is to overwrite the argument list with extra_args) to prevent the test from hanging and then failing due to missing fast DIP3 activation params from the arguments list.

    Missing arguments result in a block database sanity check failure on restart due to bad-qc-premature arising from the activation height being higher than the height of a block with a quorum commitment.

  • NodeContext was moved from TestingSetup to BasicTestingSetup in bitcoin#18571 (dash#4844) but connman as a BasicTestingSetup variable still stuck around (despite NodeContext's presence making this unnecessary).

    To prepare for bitcoin#22910, the remnant variable has been replaced with m_node.connman and adjustments have been made to that effect.

Breaking Changes

None observed.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas (note: N/A)
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@kwvg kwvg added this to the 21.2 milestone Sep 1, 2024
@kwvg kwvg marked this pull request as ready for review September 2, 2024 14:38
UdjinM6
UdjinM6 previously approved these changes Sep 3, 2024
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 1f66c09a66ce9d55279775f7a03d460e9e46f1a0

@github-actions
Copy link

github-actions bot commented Sep 3, 2024

This pull request has conflicts, please rebase.

kwvg added 15 commits September 3, 2024 14:53
…on restart with asmap

We need to continue inheriting the existing set of arguments to prevent
block invalidation due to missing arguments that allow for fast DIP3
activation (will manifest as `bad-qc-premature`)
`NodeContext` member was moved from `TestingSetup` to
`BasicTestingSetup` in bitcoin#18571 but `connman` (which was a remnant
from when `NodeContext` was absent) wasn't removed.

Let's resolve that.
…tests, make addrman consistency check ratio easier to change
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 f032119

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 f032119

@PastaPastaPasta PastaPastaPasta merged commit 18625ae into dashpay:develop Sep 4, 2024
PastaPastaPasta added a commit that referenced this pull request Sep 12, 2024
, bitcoin#25619, bitcoin#27214, bitcoin#26261, bitcoin#27745, bitcoin#27529, bitcoin#28341, partial bitcoin#25331 (addrman backports: part 4)

e544d3c fmt: apply `clang-format-diff.py` suggestions, satisfy linter (Kittywhiskers Van Gogh)
7da74ff merge bitcoin#28341: Use HashWriter over legacy CHashWriter (Kittywhiskers Van Gogh)
c798b49 merge bitcoin#27529: fix `feature_addrman.py` on big-endian systems (Kittywhiskers Van Gogh)
7d149c9 merge bitcoin#27745: select addresses by network follow-up (Kittywhiskers Van Gogh)
1d82994 merge bitcoin#26261: cleanup `LookupIntern`, `Lookup` and `LookupHost` (Kittywhiskers Van Gogh)
231ff82 merge bitcoin#27214: Enable selecting addresses by network (Kittywhiskers Van Gogh)
e825595 merge bitcoin#25619: avoid overriding non-virtual ToString() in CService and use better naming (Kittywhiskers Van Gogh)
2e9b48a merge bitcoin#26847: track AddrMan totals by network and table, improve precision of adding fixed seeds (Kittywhiskers Van Gogh)
79a550e merge bitcoin#26040: comment "add only reachable addresses to addrman" (Kittywhiskers Van Gogh)
1adb635 merge bitcoin#25678: skip querying dns seeds if -onlynet disables IPv4 and IPv6 (Kittywhiskers Van Gogh)
2d99be0 partial bitcoin#25331: Add HashWriter without ser-type and ser-version (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on #6243

  * Dependent on #5167

  ## Breaking Changes

  None observed.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK e544d3c

Tree-SHA512: 63f014142c39c47bda3ac85dc6afeee8f2bfec71f033631bca16d41bb0785f4b090b3c860ddc3b3cf6c4a23558d3d102144fc83b065130c3f9ab91d0de8e4457
@thephez thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Sep 18, 2024
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

LGTM, small follow-up #6283

@UdjinM6 UdjinM6 modified the milestones: 21.2, 22 Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC Some notable changes to RPC params/behaviour/descriptions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants