Skip to content

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Apr 20, 2025

Additional Information

  • Dependency for refactor: section off masternode network information to MnNetInfo, lay some groundwork for managing multiple entries  #6627

  • Currently, we rely on a set of macros to use a different set of instructions for (de)serializing pubKeyOperator in CDeterministicMNStateDiff, one of those macros is the following

    dash/src/evo/dmnstate.h

    Lines 219 to 226 in bcd14b0

    #define DMN_STATE_DIFF_LINE(f) \
    if (strcmp(#f, "pubKeyOperator") == 0 && (obj.fields & Field_pubKeyOperator)) {\
    SER_READ(obj, read_pubkey = true); \
    READWRITE(CBLSLazyPublicKeyVersionWrapper(const_cast<CBLSLazyPublicKey&>(obj.state.pubKeyOperator), obj.state.nVersion == CProRegTx::LEGACY_BLS_VERSION)); \
    } else if (obj.fields & Field_##f) READWRITE(obj.state.f);
    DMN_STATE_DIFF_ALL_FIELDS
    #undef DMN_STATE_DIFF_LINE

    If we pretend DMN_STATE_DIFF_ALL_FIELDS only pertains to pubKeyOperator, the macro would expand as

    if (strcmp("pubKeyOperator", "pubKeyOperator") == 0 && (obj.fields & Field_pubKeyOperator)) {
        SER_READ(obj, read_pubkey = true);
        READWRITE(CBLSLazyPublicKeyVersionWrapper(const_cast<CBLSLazyPublicKey&>(obj.state.pubKeyOperator), obj.state.nVersion == CProRegTx::LEGACY_BLS_VERSION));
    } else if (obj.fields & Field_pubKeyOperator) READWRITE(obj.state.pubKeyOperator);

    Even though READWRITE(obj.state.pubKeyOperator) is logically unreachable, it is something the compiler still has to evaluate and it can because READWRITE(obj.state.pubKeyOperator) is still a valid expression.

    But if we need to carve out a similar different rule in a later PR for newThing where newThing is a std::shared_ptr<Interface> that is implemented by the serializable type Implementation, the unreachable but still evaluable expression READWRITE(obj.state.newThing) cannot be evaluated as you cannot do anything with a pure virtual class, which Interface is even though the code right before it uses a wrapper to handle newThing correctly.

    To sidestep this issue, we need to be able to constexpr evaluate what field is being (de)serialized and decide the (de)serialization logic for it accordingly, which will exclude all other logic that doesn't apply at compile time. The current macro-based solution doesn't allow for that.

    While std::tuple allows for storing a heterogenous collection of elements, iterating through it proves to be difficult. std::apply proves to be too restrictive for what we need to do (source) and the capability needed to do this properly, "expansion statements" could be available in C++26 (source), which is a long time from now.

    So, our best option is to use a library that specializes in working with heterogenous collections and thankfully, such a library is already available in Boost called Hana (source) and it is headers-only (source, list of all libraries that need building, Hana is not on the list) and is therefore, already available to us.

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 (note: N/A)
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@kwvg kwvg added this to the 23 milestone Apr 20, 2025
@kwvg kwvg changed the title refactor, fix: replace macros with boost::hana in CDeterministicMNStateDiff, avoid no default constructor compile error, avoid bug in simplifiedmns_merkleroots test refactor: replace macros with boost::hana in CDeterministicMNStateDiff, avoid no default constructor compile error, avoid bug in simplifiedmns_merkleroots test Apr 20, 2025
@kwvg kwvg changed the title refactor: replace macros with boost::hana in CDeterministicMNStateDiff, avoid no default constructor compile error, avoid bug in simplifiedmns_merkleroots test refactor: replace CDeterministicMNStateDiff macros with boost::hana Apr 21, 2025
@kwvg kwvg marked this pull request as ready for review April 21, 2025 18:07
Copy link

coderabbitai bot commented Apr 21, 2025

Walkthrough

The changes refactor the CDeterministicMNStateDiff class by replacing a macro-based enumeration of fields with a compile-time Boost.Hana tuple containing member descriptors. A new private nested template struct Member pairs pointers-to-members of CDeterministicMNState with their bitmask flags and provides access methods. The static constexpr tuple members holds all such descriptors, enabling uniform iteration over fields. The constructor, serialization methods, and ApplyToState method are rewritten to use this tuple for comparing, serializing, deserializing, and applying differences. Special handling for the pubKeyOperator field, which depends on the nVersion, is maintained within this framework. Additionally, two Boost.Hana headers are added to the linting script’s expected Boost includes list to reflect the new dependencies. No public or exported interfaces are altered beyond internal implementation changes.


📜 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 7150d23 and 0073e9a.

📒 Files selected for processing (2)
  • src/evo/dmnstate.h (4 hunks)
  • test/lint/lint-includes.py (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • test/lint/lint-includes.py
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/evo/dmnstate.h

[error] 178-244: Clang format differences found. The file does not conform to the required clang-format style. Please run clang-format to fix formatting issues.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (7)
src/evo/dmnstate.h (7)

18-20: Good addition of Boost.Hana headers.

These headers provide the necessary functionality for the refactoring, enabling compile-time operations on heterogeneous collections which is exactly what's needed for this task.


179-187: Well-designed Member template struct.

Nice implementation of a compile-time field descriptor that pairs pointers-to-members with their corresponding bitmask flags. The use of C++17's auto non-type template parameters is appropriate here.


188-210: Good implementation of the Boost.Hana tuple.

The static constexpr tuple provides a clean, type-safe way to enumerate all fields in the class. The proper use of the macro and subsequent #undef prevents namespace pollution.


221-231: Clean implementation of field comparison with proper special-case handling.

The implementation effectively compares fields and handles the special case for pubKeyOperator which depends on nVersion. This is a good replacement for the previous macro-based approach.


238-255: Properly handles complex serialization logic.

The special case handling for pubKeyOperator is well-implemented with the if constexpr for compile-time branching. This ensures the correct behavior while keeping the serialization logic maintainable.


264-269: Clean implementation of ApplyToState method.

Using boost::hana::for_each simplifies the code while maintaining the same functionality. This is more maintainable than the previous approach.


179-254:

❓ Verification inconclusive

Fix Clang format issues.

The pipeline has failed with Clang format differences found in lines 178-244. Please run clang-format on the file to fix these formatting issues.


🏁 Script executed:

#!/bin/bash
# Run clang-format on the file to see what changes are needed
clang-format -style=file src/evo/dmnstate.h | diff -u src/evo/dmnstate.h -

Length of output: 8993


Run clang-format locally to fix formatting

The sandbox environment doesn’t have clang-format installed, so I can’t verify style diffs here. Please install clang-format and run the following locally to correct any formatting issues:

clang-format -style=file -i src/evo/dmnstate.h

Then commit the changes if any diffs are produced.

✨ Finishing Touches
  • 📝 Generate Docstrings

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/evo/dmnstate.h (1)

243-245: Unused type alias creates -Wunused-local-typedefs warning

using T1 = std::decay_t<decltype(member.get(obj.state))>; is never
referenced. Removing it keeps the loop clean and avoids compiler
warnings.

🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check

[error] 178-245: Clang format differences found. The file does not conform to the required code style formatting.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7e584f6 and 710dec9.

📒 Files selected for processing (2)
  • src/evo/dmnstate.h (4 hunks)
  • test/lint/lint-includes.py (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/evo/dmnstate.h

[error] 178-245: Clang format differences found. The file does not conform to the required code style formatting.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: linux64_tsan-test / Test source
🔇 Additional comments (2)
test/lint/lint-includes.py (1)

27-30: Boost.Hana headers correctly whitelisted

The two hana headers are now properly whitelisted, keeping the list alphabetically sorted (date_time < hana < multi_index).
No further action required.

src/evo/dmnstate.h (1)

178-245: Style violations reported by CI

The clang‑format job failed on this hunk. Running the project’s
clang-format -i src/evo/dmnstate.h (or the helper script in
contrib/devtools/) should fix spacing around commas and the long
parameter list in the tuple.

🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check

[error] 178-245: Clang format differences found. The file does not conform to the required code style formatting.

READWRITE(CBLSLazyPublicKeyVersionWrapper(const_cast<CBLSLazyPublicKey&>(obj.state.pubKeyOperator), obj.state.nVersion == CProRegTx::LEGACY_BLS_VERSION));
}
} else {
if (obj.fields & member.mask) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like conditions in implementation are changed after refactoring:

A = is pubKeyOperator
B = obj.fields & pubkeyOperator
C = obj.fields & mask

if ((!A || !B) && C) is changed to if (!A && C)

It seems as this change is valid. Let's simplify code furthermore:

if (obj.fields & member.mask) {
    if constexpr (BaseType::mask == Field_pubKeyOperator) {
        SER_READ(obj, read_pubkey = true);
        READWRITE(CBLSLazyPublicKeyVersionWrapper(const_cast<CBLSLazyPublicKey&>(obj.state.pubKeyOperator), obj.state.nVersion == CProRegTx::LEGACY_BLS_VERSION));
    } else {
        READWRITE(member.get(obj.state));
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would this simplification retain the compile-time nature of the constexpr evaluation? From what I understand, attempting a constexpr evaluation within a non-constexpr evaluation will downgrade the compile-time evaluation to runtime.

@kwvg kwvg force-pushed the misc_refac_3 branch 2 times, most recently from 2b300e9 to 7150d23 Compare April 22, 2025 11:52
@kwvg kwvg requested a review from knst April 22, 2025 11:54
knst
knst previously approved these changes Apr 22, 2025
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.

utACK 7150d23

UdjinM6
UdjinM6 previously approved these changes Apr 22, 2025
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 7150d23

Copy link

This pull request has conflicts, please rebase.

@kwvg kwvg dismissed stale reviews from UdjinM6 and knst via 0073e9a April 22, 2025 15:05
@kwvg kwvg requested review from knst and UdjinM6 April 22, 2025 15:08
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 0073e9a

@PastaPastaPasta PastaPastaPasta merged commit 90ea1da into dashpay:develop Apr 22, 2025
23 of 29 checks passed
PastaPastaPasta added a commit that referenced this pull request May 8, 2025
…`MnNetInfo`, lay some groundwork for managing multiple entries

1d52678 refactor: track every `MnNetInfo` entry in the unique property map (Kittywhiskers Van Gogh)
bcb8a7d refactor: impl `GetEntries()`, adapt to support multiple addresses (Kittywhiskers Van Gogh)
ecc9368 refactor: implement `MnNetInfo::ToString()` for printing internal state (Kittywhiskers Van Gogh)
2e9bde0 refactor: move service validation to `MnNetInfo`, run during setting (Kittywhiskers Van Gogh)
03ec604 fix: correct `simplifiedmns_merkleroots` unit test (Kittywhiskers Van Gogh)
bac4a27 refactor: move address lookup to `MnNetInfo::AddEntry()` (Kittywhiskers Van Gogh)
e1783cb refactor: remove direct access to `MnNetInfo::addr` (Kittywhiskers Van Gogh)
e868aff refactor: use const-ref when accessing `MnNetInfo::addr` if read-only (Kittywhiskers Van Gogh)
aaabc35 refactor: section off masternode service to `MnNetInfo` (Kittywhiskers Van Gogh)
2c6dd05 fix: avoid potential "no user-provided default constructor" error (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Depends on #6626

  * Depends on #6635

  * Depends on #6636

  * Dependency for #6629

  * The `simplifiedmns_merkleroots` test constructs 15 `CSimplifiedMNListEntry` elements and populates them with a `CService`. So far, since we allowed direct assignment of the `CService` without validation, no checks were made to see if they would pass validation but if we start enforcing validation rules _when setting values_, two problems emerge.

    * Using non-default ports on mainnet (`BasicTestingSetup` is mainnet by default, [source](https://github.com/dashpay/dash/blob/bcd14b05cec7d94986f114ca17bbdadbee701d9b/src/test/util/setup_common.h#L100)), this has been resolved by using `RegTestingSetup` (which is based on regtest) instead.

    * As the index is used to generate the address, starting from 0, the first address is `0.0.0.0`, which is not a valid `CService` address ([source](https://github.com/dashpay/dash/blob/bcd14b05cec7d94986f114ca17bbdadbee701d9b/src/test/net_tests.cpp#L140)) and therefore, would fail validation ([source](https://github.com/dashpay/dash/blob/bcd14b05cec7d94986f114ca17bbdadbee701d9b/src/evo/deterministicmns.cpp#L1219)). This has been resolved by changing the index to start from 1.

  * To avoid a potential "no user-provided default constructor" compile-time error, we now explicitly request the default constructor

    <details>

    <summary>Compile error:</summary>

    ```
    In file included from evo/deterministicmns.cpp:5:
    ./evo/deterministicmns.h:404:24: error: default initialization of an object of const type 'const ExampleType' without a user-provided default constructor
      404 |         static const T nullValue;
          |                        ^
          |                                 {}
    evo/deterministicmns.cpp:479:18: note: in instantiation of function template specialization 'CDeterministicMNList::AddUniqueProperty<ExampleType>' requested here
      479 |             if (!AddUniqueProperty(*dmn, domain)) {
          |                  ^
    ```

   </details>

  * The reason why we track individual entries _within_ `MnNetInfo` in the unique properties map instead of `MnNetInfo` is that while `MnNetInfo`-like objects (because `MnNetInfo` itself only stores one value) could check _internally_ for duplicates, the uniqueness requirement for addresses is _across_ ProTx'es and therefore, we are concerned not so much as _how_ the addresses are bundled (`MnNetInfo`) but if the individual addresses (`CService`) are unique _across_ all known addresses.

  * We cannot use `const auto&` when iterating through `GetEntries()` as it uses `std::reference_wrapper<const T>` and `auto` will take on the type of `const std::reference_wrapper<const T>&` instead of the underlying `const T&` as intended, to trigger the conversion to the underlying reference, we have to explicitly specify the type, hence the usage of `const T&`.

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [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:
  knst:
    utACK 1d52678
  UdjinM6:
    utACK 1d52678

Tree-SHA512: 0f13f51fff6c279e8c4e3f88ea4db925f4137e25b030d28afd48b5db9c073421d5bb3a3dc3e067ee4f559887bec9e8a981d2e6ae5f2d0a042e5817a3d59ea0bf
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.

4 participants