Skip to content

Conversation

@PastaPastaPasta
Copy link
Member

No description provided.

@PastaPastaPasta PastaPastaPasta added this to the 18.1 milestone Aug 5, 2022
@PastaPastaPasta PastaPastaPasta requested a review from UdjinM6 August 5, 2022 19:28
@github-actions
Copy link

github-actions bot commented Aug 6, 2022

This pull request has conflicts, please rebase.

@UdjinM6
Copy link

UdjinM6 commented Aug 8, 2022

pls rebase to include #4957

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.

pls see 1bef0b21d697b02a673e3af5fa434cb0e2b90739 (ok, I see why it's not the right way to fix it) and a125cb4f9c914b2d4653b63fd76c62335838dabf

@PastaPastaPasta
Copy link
Member Author

Why the first commit?

@UdjinM6
Copy link

UdjinM6 commented Aug 9, 2022

Why the first commit?

template version
template< class InputIt1, class InputIt2 >
bool equal( InputIt1 first1, InputIt1 last1, InputIt2 first2 );
(until C++20)
template< class InputIt1, class InputIt2 >
constexpr bool equal( InputIt1 first1, InputIt1 last1, InputIt2 first2 );
(since C++20)

https://en.cppreference.com/w/cpp/algorithm/equal

@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Aug 9, 2022

This is the one we're using here, still exists after c++20 (it simply becomes constexpr)

image

@UdjinM6
Copy link

UdjinM6 commented Aug 9, 2022

Hmm.. No...

More like
Screenshot 2022-08-10 at 00 01 09
for src/validation.cpp and
Screenshot 2022-08-10 at 00 01 01
for src/evo/simplifiedmns.cpp and src/rpc/server.cpp

you changed it to be (7) instead which broke tests ("bad-cb-height")

@github-actions
Copy link

This pull request has conflicts, please rebase.

# Conflicts:
#	src/validation.cpp
@PastaPastaPasta PastaPastaPasta changed the title Refactor: use more ranges refactor: use more ranges Sep 5, 2024
@PastaPastaPasta
Copy link
Member Author

@UdjinM6 I've revived this, please review


if (std::equal(vecAllowedParam.begin(), vecAllowedParam.end(),
request.params.getValues().begin(),
if (ranges::equal(vecAllowedParam, request.params.getValues(),
Copy link

Choose a reason for hiding this comment

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

this broke rpc_platform_filter.py

@github-actions
Copy link

This pull request has conflicts, please rebase.

@DashCoreAutoGuix
Copy link

❌ Backport Verification - CATASTROPHIC FAILURE

Original Bitcoin commit: UNKNOWN
Reviewed commit hash: f03afb2dd5-verify-1753721748

Critical violations:

  • No Bitcoin commit reference found: This PR lacks proper Bitcoin backport documentation and commit references
  • CI pipeline failure: GitLab CI is failing, indicating code issues
  • Historical implementation issues: Previous reviewer feedback indicates this implementation broke tests ("bad-cb-height")
  • Not a proper Bitcoin backport: This appears to be either a standalone Dash refactoring or an improperly documented backport

Analysis:
This PR titled "refactor: use more ranges" does not follow the standard Bitcoin backport format. It lacks:

  • Bitcoin PR number reference in title/description
  • Bitcoin commit hash documentation
  • Proper backport justification

The verification system is designed specifically for Bitcoin Core backports to Dash Core. PRs that are not proper Bitcoin backports should not go through this verification process.

Recommendation:
If this is intended as a Bitcoin backport, please:

  1. Add proper Bitcoin commit/PR references
  2. Update the title to follow format: "Merge bitcoin#XXXXX: description"
  3. Fix CI failures
  4. Address the test breakage issues mentioned in previous reviews

If this is a standalone Dash improvement, it should be processed outside the backport verification system.

This PR has been automatically closed due to catastrophic validation failures. Please create a new PR with proper backport documentation if this is indeed a Bitcoin backport.

@DashCoreAutoGuix
Copy link

Automatically closed due to catastrophic validation failures. Please see the detailed analysis above and create a new PR with a proper backport implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants