Skip to content
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

backport: merge bitcoin#27452, #29347, #29356, #29353, #29452, #29483, #30545, #31383 (BIP324 backports: part 5) #6488

Merged
merged 9 commits into from
Dec 15, 2024

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Dec 14, 2024

Additional Information

  • When backporting bitcoin#27452 in feature_anchors.py, P2P_SERVICES (NODE_NETWORK | NODE_HEADERS_COMPRESSED) has been replaced with NODE_NETWORK as the former evaluates to a value greater than 256 (specifically 2049), which causes test failure. The replacement value is acceptable as NODE_NETWORK is the desired service flag expected by Dash Core (source).

    Test failure:
    dash@89afd55ae77e:/src/dash$ ./test/functional/feature_anchors.py
    2024-12-14T12:31:22.244000Z TestFramework (INFO): PRNG seed is: 8365703189892653614
    2024-12-14T12:31:22.244000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_j0ya02yu
    2024-12-14T12:31:22.776000Z TestFramework (INFO): When node starts, check if anchors.dat doesn't exist
    2024-12-14T12:31:22.776000Z TestFramework (INFO): Add 2 block-relay-only connections to node
    2024-12-14T12:31:24.781000Z TestFramework (INFO): Add 5 inbound connections to node
    2024-12-14T12:31:29.843000Z TestFramework (INFO): Check node connections
    2024-12-14T12:31:30.848000Z TestFramework (INFO): Check the addresses in anchors.dat
    2024-12-14T12:31:30.848000Z TestFramework (INFO): Perturb anchors.dat to test it doesn't throw an error during initialization
    2024-12-14T12:31:31.356000Z TestFramework (INFO): When node starts, check if anchors.dat doesn't exist anymore
    2024-12-14T12:31:31.357000Z TestFramework (INFO): Ensure addrv2 support
    2024-12-14T12:31:32.364000Z TestFramework (INFO): Add 256-bit-address block-relay-only connections to node
    2024-12-14T12:31:33.368000Z TestFramework (INFO): Check for addrv2 addresses in anchors.dat
    2024-12-14T12:31:33.369000Z TestFramework (ERROR): Unexpected exception caught during testing
    Traceback (most recent call last):
      File "/src/dash/test/functional/test_framework/test_framework.py", line 161, in main
        self.run_test()
      File "/src/dash/./test/functional/feature_anchors.py", line 135, in run_test
        new_data[services_index] = P2P_SERVICES
    ValueError: byte must be in range(0, 256)
    2024-12-14T12:31:33.870000Z TestFramework (INFO): Stopping nodes
    2024-12-14T12:31:33.871000Z TestFramework (WARNING): Not cleaning up dir /tmp/dash_func_test_j0ya02yu
    2024-12-14T12:31:33.871000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/dash_func_test_j0ya02yu/test_framework.log
    

Breaking Changes

None expected.

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 22.1 milestone Dec 14, 2024
@kwvg kwvg marked this pull request as ready for review December 14, 2024 18:39
@UdjinM6 UdjinM6 added the RPC Some notable changes to RPC params/behaviour/descriptions label Dec 14, 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 c6f23a7

@@ -360,7 +360,7 @@ static RPCHelpMan addconnection()
{
{"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The IP address and port to attempt connecting to."},
{"connection_type", RPCArg::Type::STR, RPCArg::Optional::NO, "Type of connection to open (\"outbound-full-relay\", \"block-relay-only\", \"addr-fetch\" or \"feeler\")."},
{"v2transport", RPCArg::Type::BOOL, RPCArg::Default{false}, "Attempt to connect using BIP324 v2 transport protocol"},
{"v2transport", RPCArg::Type::BOOL, RPCArg::Optional::NO, "Attempt to connect using BIP324 v2 transport protocol"},
Copy link

Choose a reason for hiding this comment

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

29356: nit: probably partial because of missing bool use_v2transport = self.Arg<bool>(2); part, not sure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

self.Arg<bool>(2) is the equivalent of request.params[2].get_bool(), the newer syntax being introduced in bitcoin#28230 and fleshed out in bitcoin#29277, backporting them out of order brings little benefit but would make backports until that point annoying at best, so it was decided to skip that change but not mark it as partial as the behaviour should be identical.

Though the !request.params[2].isNull() half of use_v2transport is a no-op as v2transport is no longer optional and is a nit that can be addressed later on.

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 c6f23a7;

looks good enough to me :) a few nits you can consider and if you want, can apply in another PR.

@PastaPastaPasta PastaPastaPasta merged commit e2095bd into dashpay:develop Dec 15, 2024
19 of 24 checks passed
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.

3 participants