-
Notifications
You must be signed in to change notification settings - Fork 1.2k
refactor: replace CDeterministicMNStateDiff
macros with boost::hana
#6636
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
Conversation
boost::hana
in CDeterministicMNStateDiff
, avoid no default constructor compile error, avoid bug in simplifiedmns_merkleroots
testboost::hana
in CDeterministicMNStateDiff
, avoid no default constructor compile error, avoid bug in simplifiedmns_merkleroots
test
boost::hana
in CDeterministicMNStateDiff
, avoid no default constructor compile error, avoid bug in simplifiedmns_merkleroots
testCDeterministicMNStateDiff
macros with boost::hana
WalkthroughThe changes refactor the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🪛 GitHub Actions: Clang Diff Format Checksrc/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)
🔇 Additional comments (7)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this 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
📒 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 whitelistedThe 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 CIThe 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) { |
There was a problem hiding this comment.
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));
}
}
There was a problem hiding this comment.
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.
2b300e9
to
7150d23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 7150d23
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 7150d23
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 0073e9a
…`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
Additional Information
Dependency for refactor: section off masternode network information to
MnNetInfo
, lay some groundwork for managing multiple entries #6627Currently, we rely on a set of macros to use a different set of instructions for (de)serializing
pubKeyOperator
inCDeterministicMNStateDiff
, one of those macros is the followingdash/src/evo/dmnstate.h
Lines 219 to 226 in bcd14b0
If we pretend
DMN_STATE_DIFF_ALL_FIELDS
only pertains topubKeyOperator
, the macro would expand asEven though
READWRITE(obj.state.pubKeyOperator)
is logically unreachable, it is something the compiler still has to evaluate and it can becauseREADWRITE(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
wherenewThing
is astd::shared_ptr<Interface>
that is implemented by the serializable typeImplementation
, the unreachable but still evaluable expressionREADWRITE(obj.state.newThing)
cannot be evaluated as you cannot do anything with a pure virtual class, whichInterface
is even though the code right before it uses a wrapper to handlenewThing
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