Refactor Feature name management and creation: (Improve amendment management and default votes) #3877
Conversation
There was a problem hiding this comment.
There is a compile warning:
src/ripple/protocol/impl/Feature.cpp:232:21: warning: unused variable ‘feature’ [-Wunused-variable] 232 | auto const& feature = features.emplace_back(name, f);
and a compile error:
src/ripple/protocol/impl/Feature.cpp:129:53: required from here boost_1_75_0/boost/container_hash/extensions.hpp:305:30: error: no matching function for call to ‘hash_value(const ripple::base_uint<256>&)’ 305 | return hash_value(val);
Fixed. Thanks for catching these. Ironically, I actually ran across the same error in VS, and fixed it there, which is why I only had to move code. I didn't follow up with the other builds. |
|
I'm still in the middle of this review. However I have some significant changes to The biggest change has to do with the way the Wallet db handles Secondly, vetoed is stored as We can't change what is stored in the database, but we can change how we talk about what is stored in the database. So I suggest we add an A smaller point is the I tried out both of these changes. If you'd like to look at the results, here's what I got: scottschurr@bd282fd I'm not insisting that these are good changes. But I hope you'll look them over and see what you think. Thanks. |
There was a problem hiding this comment.
Very nice changes! The unit test coverage looks pretty good. And I like that we're getting the first use of boost::multi_index into the code base.
I made a few suggestions for changes along the way. All of these suggestions are open for discussion. But I'm going to hold off approving this pull request until we've had a chance to talk through the suggestions.
src/ripple/app/main/Application.cpp
Outdated
| supportedAmendments.append(saHashes); | ||
| auto const supportedAmendments = []() { | ||
| auto const& amendments = detail::supportedAmendments(); | ||
| std::vector<AmendmentTable::FeatureInfo> hashes; |
There was a problem hiding this comment.
Calling this vector hashes is now misleading. FeatureInfo has more than just hashes. Perhaps supported would be a better name?
There was a problem hiding this comment.
Cleaned up the names in this entire block
| auto getName2 = [](auto const& amendment, auto const& name) { | ||
| if (name.empty()) | ||
| return featureToName(amendment); | ||
| return name; | ||
| }; | ||
| auto getName = [&getName2](auto const& amd) { | ||
| return getName2(amd.first, amd.second); | ||
| }; |
There was a problem hiding this comment.
While this two-generic-lambda approach works, I find the result very difficult to read using just a text view (no IDE). I think you can get nearly the same functionality with a lot more readability
- The generic lambdas do not need to be generic. They are always called with the same arguments. Using
autofor the arguments undoubtably makes them easier to type. But, for my eyes, theautohides the types that the lambda expects. This hinders readability and makes it more difficult to modify the code in the future. - I feel like the
getNamelambda does not really provide a lot of functionality. It's just taking apair<some,thing>and pulling it apart. That can be done almost as easily at the call site.
So I suggest having one lambda that looks like this:
auto getName = [](uint256 const& amendment,
std::string const& name) -> std::string {
if (name.empty())
return featureToName(amendment);
return name;
};
Then call it with the appropriate arguments. One example might look like this:
persistVote(a.first, getName(a.first, a.second), false); // upvote
I'm not gonna insist on this change. But I was unable to "just read" the code as it stands. I had to poke at it and compile a few changes in; I had to experiment to uncover the intent.
There was a problem hiding this comment.
Fixed by pulling in your experimental commit.
| Feature const* feature = getByName(name); | ||
| if (feature) | ||
| return feature->feature; | ||
| return std::nullopt; |
There was a problem hiding this comment.
This line is currently not exercised by the unit tests. I wonder if there's an easy way to exercise it...
| @@ -36,6 +37,19 @@ namespace ripple { | |||
| class AmendmentTable | |||
| { | |||
| public: | |||
| struct FeatureInfo | |||
| { | |||
| FeatureInfo() = delete; | |||
There was a problem hiding this comment.
FWIW, deleting the default constructor is a noop. C++ won't automatically make a default constructor for you if you provide any kind of a non-default constructor (see https://en.cppreference.com/w/cpp/language/default_constructor). But I'm fine if you leave the line in the code since it documents your intention.
There was a problem hiding this comment.
Yeah, I wanted to be explicit.
src/test/app/AmendmentTable_test.cpp
Outdated
| BEAST_EXPECTS( | ||
| table->isEnabled(supportedID) == | ||
| (allEnabled.find(supportedID) != allEnabled.end())); | ||
| (allEnabled.find(supportedID) != allEnabled.end()), | ||
| a + | ||
| (table->isEnabled(supportedID) ? " enabled " | ||
| : " disabled ") + | ||
| (allEnabled.find(supportedID) != allEnabled.end() | ||
| ? " found" | ||
| : " not found")); |
There was a problem hiding this comment.
I found reading this chunk of source code to be a bit of a trial, particularly the part where text formatting is happening. I'm sure adding the string was a really good thing for understanding failing cases. And I know we're working within the constraints of clang-format. Still, is it possible to make this code easier to read? Thanks.
There was a problem hiding this comment.
Yeah. This cleaned up nicely.
src/ripple/protocol/impl/Feature.cpp
Outdated
| #include <cstring> | ||
|
|
||
| namespace ripple { | ||
|
|
||
| //------------------------------------------------------------------------------ | ||
| enum class Supported : bool { no = false, yes }; |
There was a problem hiding this comment.
I'm a little concerned about declaring Supported at namespace ripple scope. There are a huge number of things in the code base that are supported. In this case we're only talking about whether an Amendment or Feature is supported. Yeah, this declaration is only visible to humans in this file. But the compiler leaks the name ripple::Supported out to the entire application.
Consider renaming this to AmendmentSupported, which I'll admit is a little unwieldy. Another option would be to declare it as a member of the Feature class and use the class to limit the scope. Or maybe you have a yet better idea.
I won't insist, I'm just a bit concerned.
There was a problem hiding this comment.
Wouldn't it only leak the name to parts of the application in the same TU?
I put it in the detail namespace. I can't say I'm entirely happy with the way the variable initialization looks now, but it works.
There was a problem hiding this comment.
I'm pretty sure the name leaks into the entire executable through the linker.
What you could do is put the declaration in an unnamed namespace like this:
namespace
{
enum class Supported : bool { no = false, yes };
}
According to cppreference, for an unnamed name space...
Its members have potential scope from their point of declaration to the end of the translation unit, and have internal linkage.
https://en.cppreference.com/w/cpp/language/namespace
I think an unnamed namespace would provide the smallest possible scope.
ximinez
force-pushed
the
internalveto2
branch
2 times, most recently
from
96daa54 to
bfaf343
Compare
|
Rebased on to 1.8.0-b4. Added new amendments, but did not make any other code changes. |
|
@ximinez, I left some comments over a month ago. Do you have time to go through those comments? Once that's done then maybe we can mark this as passed... |
manojsdoshi
added
the
Passed
|
@scottschurr Soon:tm:... I haven't forgotten, but I've had other things going on that pushed this to the backburner. |
ximinez
force-pushed
the
internalveto2
branch
2 times, most recently
from
76aecff to
b35da48
Compare
|
@scottschurr I think that all your questions and issues have now been resolved. CC: @intelliot @gregtatcam |
* Only require adding the new feature names in one place. (Also need to increment a counter, but a check on startup will catch that.) * Allows rippled to have the code to support a given amendment, but not vote for it by default. This allows the amendment to be enabled in a future version without necessarily amendment blocking these older versions. * The default vote is carried with the amendment name in the list of supported amendments. * The amendment table is constructed with the amendment and default vote.
|
"Merged as part of #3948" |
|
Note: Released in version 1.8.0 |
* Only require adding the new feature names in one place. (Also need to increment a counter, but a check on startup will catch that.) * Allows rippled to have the code to support a given amendment, but not vote for it by default. This allows the amendment to be enabled in a future version without necessarily amendment blocking these older versions. * The default vote is carried with the amendment name in the list of supported amendments. * The amendment table is constructed with the amendment and default vote.
intelliot
changed the title
High Level Overview of Change
This is a follow-up / second attempt to #3458 at changing default voting amendment behavior. It's more comprehensive, and simplifies the process quite a bit, but it is not intended to be the definitive answer to the question of how to manage amendment voting on the network.
It is organized into three commits. Each of the commits should build, but I don't guarantee that they'll pass unit tests. I think it makes more sense to review the changes as a single unit, but they're split up in case there are questions of how something evolved, or if the reviewers absolutely hate some subset of the changes.
As stated above, the scope of this change is intentionally relatively small. It does not address some of the advanced and controversial questions of amendment voting behavior including, but not limited to:
Context of Change
There are two significant problems related to amendment management.
supportedAmendmentslist) by default. This makes it impossible to have multiple versions of the software support a feature without voting for it, without manual intervention from validator operators.These changes resolve those issues (and only those issues) by
Type of Change
Before / After
Before
To create an amendment, the name has to be specified (as a string) in two places:
featureNameslistgetRegisteredFeatureto define and initialize aunit256variable.Additionally, a variable need to be declared (at least if you want to use it anywhere else in the code):
extern uint256 constvariable declarationLater, when a feature is development complete, and ready to support, the name also needs to be added (as a string) to
supportedAmendments()listThere is no way to add an amendment to
supportedAmendments()and not vote for it without operator intervention.After
To create an amendment, the name has to be specified in one place:
registerFeatureto define and initialize aunit256variable.Additionally, the
numFeaturescounter must be incremented. However, if this step is skipped, an assert on startup will catch it and remind the developer (reducing potential errors).The variable still needs to be declared:
extern uint256 constvariable declarationLater, when a feature is development complete, and ready to support, the existing call to
registerFeatureonly needs to have one of it's parameters modified. Another parameter toregisterFeaturespecifies the default voting behavior for supported amendments: yes or abstain.Test Plan
The "CryptoConditionsSuite" is listed as supported, and has not yet been enabled on the Mainnet, but is "not ready for prime time". However, it cannot be removed, because it could be enabled at any time, and removing it would cause all future versions to be amendment blocked, and cause other problems. Any current node that has not explicitly vetoed it will vote for it. These votes can be captured and verified on any network (main or test) that hasn't enabled it yet.
The updated registration for this amendment specifies it as
Supported::yes, DefaultVote::abstain, which means it's still supported, but will not be voted for without explicit operator action. Votes captured from updated nodes on any network where it's not enabled will show that amendment not being voted for.