Skip to content

fix: compilation error due to conflict between #6826 (SignHash refactoring) and #6613 (Platform ban)#6862

Merged
PastaPastaPasta merged 1 commit intodashpay:developfrom
knst:fix-develop-signhash
Sep 30, 2025
Merged

fix: compilation error due to conflict between #6826 (SignHash refactoring) and #6613 (Platform ban)#6862
PastaPastaPasta merged 1 commit intodashpay:developfrom
knst:fix-develop-signhash

Conversation

@knst
Copy link
Collaborator

@knst knst commented Sep 30, 2025

Issue being fixed or feature implemented

Conflict between #6826 (SignHash refactoring) and #6613 (Platform ban) causes compile error on develop.

What was done?

Fixed compilation error.

How Has This Been Tested?

Run build

Breaking Changes

N/A

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
  • I have assigned this pull request to a milestone

@knst knst added this to the 23 milestone Sep 30, 2025
@github-actions
Copy link

github-actions bot commented Sep 30, 2025

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Sep 30, 2025

Walkthrough

  • In src/net_processing.cpp, calls to llmq::BuildSignHash were replaced with llmq::SignHash.
  • The sign hash variable now uses auto and the returned object requires calling Get() to obtain the underlying value.
  • Signature verification was updated to pass signHash.Get() to ban_msg.m_signature.VerifyInsecure(...).
  • No other control flow or error handling changes were made.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title explicitly summarizes the primary change by stating it fixes the compilation error resulting from merging the SignHash refactoring with the platform ban feature, which accurately reflects the core issue addressed by the pull request.
Description Check ✅ Passed The description clearly relates to the change by outlining the compile error conflict, summarizing the applied fix, describing the test method, and noting checklist items, which directly reflect the content of the changeset.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2d3e95f and cc13f15.

📒 Files selected for processing (1)
  • src/net_processing.cpp (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

  • src/net_processing.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: linux64_multiprocess-build / Build source
  • GitHub Check: linux64_tsan-build / Build source
  • GitHub Check: mac-build / Build source
  • GitHub Check: linux64_nowallet-build / Build source
  • GitHub Check: linux64_sqlite-build / Build source
  • GitHub Check: linux64_ubsan-build / Build source
  • GitHub Check: linux64-build / Build source
  • GitHub Check: linux64_fuzz-build / Build source
  • GitHub Check: win64-build / Build source
  • GitHub Check: arm-linux-build / Build source

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🧪 Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

  • Public repositories are always opted into early access features.
  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

Comment @coderabbitai help to get the list of available commands and usage tips.

kwvg
kwvg approved these changes Sep 30, 2025
View reviewed changes
Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

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

ACK cc13f15

kwvg
kwvg requested changes Sep 30, 2025
View reviewed changes
Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

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

Data race identified by CI

PastaPastaPasta
PastaPastaPasta approved these changes Sep 30, 2025
View reviewed changes
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; we at least compile now

kwvg
kwvg approved these changes Sep 30, 2025
View reviewed changes
Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

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

As an interim measure to allow develop to compile

LGTM cc13f15

@PastaPastaPasta PastaPastaPasta merged commit 54e2588 into dashpay:develop Sep 30, 2025
32 of 38 checks passed
@knst knst deleted the fix-develop-signhash branch September 30, 2025 17:47
@knst knst mentioned this pull request Oct 1, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

@kwvg kwvg kwvg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

23

Development

Successfully merging this pull request may close these issues.

3 participants

@knst @PastaPastaPasta @kwvg