Internal amendment vetoes: #3458

ximinez · 2020-06-24T14:06:13Z

Spec

Allows a version 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.
Provides a new configuration file section: [unveto_amendments], which
can be used to override the hard-coded veto list, and is intended for
use on test networks (testnet, devnet). Will also override
[veto_amendments], but since that's in the same configuration file,
that would be silly.

This change is

mDuo13 · 2020-06-24T22:03:40Z

I think [unveto_amendments] sounds like a double-negative and redundant with the [amendments] stanza (at least, how it's supposed to work).

I am torn on the concept of the code itself essentially containing different hard-coded opinions for different amendments. That's another potentially confusing thing to document, especially since a natural extension of this functionality is for server versions to have different hard-coded preferences. Imagine:

v1.6.0: fixNonUrgentThing is default-no
v1.7.0: fixNonUrgentThing is default-yes

Doing this would allow a "longer than 1 full release cycle" path for breaking upgrades. Which sounds pretty cool! But also, imagine how the table of amendments in the documentation is supposed to explain this. "v1.5.0: CryptoConditionsSuite is default-yes. 1.6.0 through v1.7.0: default-no. v1.8.0: default-yes"

ximinez · 2020-06-25T17:59:36Z

I think [unveto_amendments] sounds like a double-negative and redundant with the [amendments] stanza (at least, how it's supposed to work).

I consider it a double-negative, too, but I chose "unveto" to match the internal function name (https://github.com/ripple/rippled/blob/develop/src/ripple/rpc/handlers/Feature1.cpp#L67-L73). I considered using [amendments], but the things it's doing right now are so wacky, that I didn't want to mess with it and risk breaking something that looks broken but isn't.

If I can get buy-in from the community, validators, and team, I'd have no problem with repurposing [amendments].

I am torn on the concept of the code itself essentially containing different hard-coded opinions for different amendments. That's another potentially confusing thing to document, especially since a natural extension of this functionality is for server versions to have different hard-coded preferences. Imagine:

v1.6.0: fixNonUrgentThing is default-no
v1.7.0: fixNonUrgentThing is default-yes

I agree that's a problem, but I think it's intrinsic in fixing the current problem that arises from rippled starting to vote for an amendment as soon as it's supported (unless explicitly vetoed). I also think that our default process with new amendments will be to add it to the veto / downvote list for at least one release, which I think will at least make the documentation consistent.

Doing this would allow a "longer than 1 full release cycle" path for breaking upgrades. Which sounds pretty cool!

Yep! That's the intent!

But also, imagine how the table of amendments in the documentation is supposed to explain this. "v1.5.0: CryptoConditionsSuite is default-yes. 1.6.0 through v1.7.0: default-no. v1.8.0: default-yes"

That's why we need to remove CryptoConditionsSuite entirely. 😀 I know I mentioned somewhere adding it to the veto list as a possible compromise, but I don't think that's the optimal solution.

MarkusTeufelberger · 2020-06-25T19:23:14Z

Removing default votes instead of adding another way to have default votes would be preferable imho.

mDuo13 · 2020-06-25T21:39:19Z

We've had this discussion before: it is not possible to "remove default votes" because "not making a choice" is, itself, a choice.

MarkusTeufelberger · 2020-06-25T21:52:13Z

Not if the server doesn't start if you don't make a choice? I mean you'd choose to not run a server then.

intelliot · 2020-06-26T01:11:15Z

Some rippled operators would like to delegate their choice to the rippled developers. Some servers are configured to auto-update, and there must a default for those cases.

ximinez · 2020-06-26T19:13:50Z

@MarkusTeufelberger

Removing default votes instead of adding another way to have default votes would be preferable imho.

Depending on how you look at it, this removes the default "yes" vote, at least on a case-by-case basis. The way that voting is implemented, there are really only two choices: "Yes" and "no/abstain". Without this feature, an operator can only change their vote from "yes" to "no/abstain". With it, and especially if all new amendments are added to the internal "no/abstain" list, then operators will now have the choice to vote "yes". This opens up the opportunity for some interesting changes. The most obvious is that we can now release a version of ripple that supports an amendment but won't vote "yes" for it by default. That gives operators and validators much more time to upgrade to the most recent version, especially as more versions are released in the future - for example, if 1.7 is released without any new amendments (however unlikely), then operators that are happy with 1.6 can stick with it for the entire 1.7 release cycle.

But now consider what happens if a new feature is actually in high demand. For example, everybody falls in love with Tickets, and they want it ASAP. Everybody, including the validators rushes out to install that version, but it obviously doesn't activate. With the unveto_amendments config, UNL validator operators can now proactively vote "yes" and potentially get the amendment enabled before the next release. This is significant because it is a form of communication that there is a demand not just the feature, but for this ability. On the other hand, if nobody votes for it, we remove it from the "no" list in the next version, and it gets enabled shortly thereafter, it tells us that operators are perfectly happy with voting for the defaults.

It's not a perfect solution, of course, but I think it's a step in the right direction, and maybe more importantly gives us more time to come up with a better solution while still being able to release new code.

MarkusTeufelberger · 2020-06-28T14:33:48Z

Some rippled operators would like to delegate their choice to the rippled developers. Some servers are configured to auto-update, and there must a default for those cases.

How about pre-filling the [voting] and other (currently undocumented in the file) sections regarding amendment votes in the example config that gets shipped and installed by default instead of auto-votes? Similar to the default log level being changed to warning by default with that config.

Without this feature, an operator can only change their vote from "yes" to "no/abstain".

I agree that conceptually this is a better situation than the current one (only "default yes/explicit no"), I still hope that the case of "explicit yes/explicit no" will eventually become the default to make situations like the ones with SHAmapv2 or CryptoConditions less awkward and more informed.

ximinez · 2020-06-29T15:37:15Z

How about pre-filling the [voting] and other (currently undocumented in the file) sections regarding amendment votes in the example config that gets shipped and installed by default instead of auto-votes? Similar to the default log level being changed to warning by default with that config.

I'm not terribly familiar with the installer process, but I don't think it will overwrite an existing configuration file, at least not by default. So this kind of solution will only work for brand new installations, which is a relative minority of nodes.

Without this feature, an operator can only change their vote from "yes" to "no/abstain".

I agree that conceptually this is a better situation than the current one (only "default yes/explicit no"), I still hope that the case of "explicit yes/explicit no" will eventually become the default to make situations like the ones with SHAmapv2 or CryptoConditions less awkward and more informed.

I see your point, and there may be definite advantages to that kind of change, but we're not there yet.

MarkusTeufelberger · 2020-06-29T16:26:26Z

dpkg has the conffiles directive (https://www.debian.org/doc/debian-policy/ap-pkg-conffiles.html), RPM has %config: https://rpm-packaging-guide.github.io/#files

This way default config changes can work in the "auto-update" case too (though I'd strongly recommend that validators do not auto-update).

intelliot · 2020-07-30T23:28:51Z

@mDuo13 Regarding:

Doing this would allow a "longer than 1 full release cycle" path for breaking upgrades. Which sounds pretty cool! But also, imagine how the table of amendments in the documentation is supposed to explain this. "v1.5.0: CryptoConditionsSuite is default-yes. 1.6.0 through v1.7.0: default-no. v1.8.0: default-yes"

When it comes to documenting the default vote, I think the documentation only needs to consider the latest stable version of rippled.

If people want more detail than that, then they're probably at the point where they should configure their server to explicitly vote for or against the particular amendment. (If they really want to know what the default was in a previous version -- which would be a very unusual scenario, I think -- then they can look at the code.)

So the documentation would be like:

"As of rippled v1.6.0, CryptoConditionsSuite is default-no."

In the particular case of CryptoConditionsSuite, it will not be defaulted to yes in the future. However, other amendments may start off as default-no and later transition to default-yes. Again, I think the docs should just tell me what the default is in the latest stable version of rippled. I usually don't care about other versions.

scottschurr · 2020-08-10T21:54:28Z

Since there are config-related changes being proposed here, I think it would be really handy to see a spec of some sort describing usage and some of the design trade-offs. I think some of those discussions are easier to have from a higher-level spec perspective, rather than while grinding through the bits.

For instance, it seems like this feature or something similar might help those people who are managing the various test nets. That might be worth discussing in a spec. Also, if we think folks might want to delegate which amendments they up- or down-vote, would it makes sense to leverage the existing UNL machinery to get down-votes over the web?

Thoughts?

ximinez · 2020-08-11T21:55:06Z

@scottschurr This started as a simple "code wins arguments" proof of concept to show that it could be done. But at this point, I think you're right. I'll start on that soon ™️ .

gregtatcam

👍 LGTM

ximinez · 2020-08-26T20:53:39Z

@scottschurr and @gregtatcam I just opened a PR at https://github.com/ripple/rippled-specs/pull/26 for a spec to define these changes. As I noted there, I need to update some of the internal documentation, and will be pushing that soon. I've assigned you both to that PR on the theory that you're relatively familiar with these changes, and thus may be more likely to spot discrepancies between the code and the spec.

scottschurr

I think there are still changes afoot in this pull request. But, regardless, I'd like to see:

The featureToName() functionality should be exercised in the unit tests.
The AmendmentTable::isVetoed() interfaces are unused other than in unit tests. I suggest removing that interface.

scottschurr · 2020-09-02T22:24:54Z

src/ripple/app/main/Application.cpp

+                return section;
+            };
+        Section const supportedAmendments = buildAmendmentList(
+            Section("Supported Amendments"), detail::supportedAmendments());


There are getting to be enough places that use supportedAmendments() that it may be time to move them out of the detail namespace. What do you think?

I'm torn. This example isn't new new place where supportedAmendments() is called. In fact, it looks like this is the only place outside of test code where it's called. I think the only calls I added are in Feature_test.cpp. Leaving it inside detail signals that it shouldn't be called casually.

My opinion is to leave it.

scottschurr · 2020-09-02T22:25:55Z

src/ripple/app/main/Application.cpp

+
+        Section const downVotedAmendments = buildAmendmentList(
+            config_->section(SECTION_VETO_AMENDMENTS),
+            detail::downVotedAmendments());


Consider moving downVotedAmendments() out of the detail namespace as well.

Wherever they end up, they should definitely be left together.

scottschurr · 2020-09-02T22:32:14Z

src/ripple/app/misc/AmendmentTable.h

@@ -51,6 +51,8 @@ class AmendmentTable
    isEnabled(uint256 const& amendment) const = 0;
    virtual bool
    isSupported(uint256 const& amendment) const = 0;
+    virtual bool
+    isVetoed(uint256 const& amendment) const = 0;


My initial inclination is to call this isDownVoted(). But unfortunately we can't get rid of the vetoed nomenclature altogether. It's part of the feature RPC command. Since we can't change that, it makes changing the rest of it less clear cut. Thoughts?

Actually, it looks like isVetoed() is only called by unit tests? Is this a useful entry point to add? My inclination is to remove this method since it looks like its unused.

I added it to be able to verify the changed [amendment] behavior, but since those changes got reverted, then it makes sense to take this back out.

scottschurr · 2020-09-02T23:53:27Z

src/ripple/protocol/impl/Feature.cpp

+detail::FeatureCollections::featureToName(uint256 const& f) const
+{
+    auto const i = featureToIndex.find(f);
+    return i == featureToIndex.end() || i->second > numFeatures()


Should the second part be i->second >= numFeatures()?

Arguably, yes. Good catch. However, I also realized that featureToIndex must be the same size as numFeatures(), so the second part is redundant. Just to be safe, I added an assert and removed the second part entirely.

scottschurr · 2020-09-03T00:17:26Z

src/ripple/protocol/impl/Feature.cpp

@@ -74,14 +74,32 @@ detail::FeatureCollections::bitsetIndexToFeature(size_t i) const
    return features[i];
 }

+std::string
+detail::FeatureCollections::featureToName(uint256 const& f) const


FeatureCollections::featureToName() is untouched by the unit tests. That probably ought to be fixed.

scottschurr · 2020-09-03T01:00:13Z

src/ripple/protocol/Feature.h

+/** Amendments that this server won't vote for by default. Overrides the default
+   vote behavior of `supportedAmendments()` */
+std::vector<std::string> const&
+downVotedAmendments();


Have you tested how downVotedAmendments() interacts with unit tests? I think that we want to run unit tests with the downVotedAmendments() enabled. Is that your expectation as well? I suppose we can test (and) fix that when this capability is integrated with the Negative UNL or with Tickets...

That's a good question. There is the initFee function in TxQ_test.cpp where it runs up to a flag ledger to make a fee change pseudo transaction, but it also creates all the supported amendment pseudo transactions. I don't check the individual amendments, but I do check the counts, accounting for downVotedAmendments(), and the counts match, regardless of which or how many amendments I put into the down vote list. I think that's sufficient.

scottschurr · 2020-09-03T01:05:06Z

src/ripple/protocol/impl/Feature.cpp

+    // that development of the feature is complete. Any future behavior
+    // changes will require another amendment.
+    //
+    // In general, any new amendments added to this list should also be added to


I don't think this is quite right. I suspect that most fix amendments we will want to have enabled as quickly as possible.

Slightly reworded.

scottschurr · 2020-09-03T01:37:55Z

src/ripple/app/misc/impl/AmendmentTable.cpp

@@ -262,6 +262,8 @@ class AmendmentTableImpl final : public AmendmentTable
    isEnabled(uint256 const& amendment) const override;
    bool
    isSupported(uint256 const& amendment) const override;
+    virtual bool
+    isVetoed(uint256 const& amendment) const override;


Generally in this code base the virtual is considered redundant if override is present. But I'm in favor of removing this method altogether, so it probably doesn't matter...

The method was removed, but that was a mistake, so thanks for catching it.

scottschurr · 2020-09-03T16:36:31Z

src/ripple/protocol/impl/Feature.cpp

+    // enabled, but never vote for the amendment by default. Additionally, some
+    // tests will fail if an entry in this list is not in supportedAmendments.
+    //
+    // Note that this list can be overridden by the `[amendments]` config


Is this still correct?

It is not. Good catch.

scottschurr · 2020-09-03T16:37:48Z

src/ripple/protocol/impl/Feature.cpp

+    //
+    // To enable automatically voting for the amendment, simply remove it from
+    // this list. There should never be a need to comment entries out aside from
+    // testing.


Nice comment!

scottschurr

👍 Looks great! Thanks for the added unit test.

intelliot · 2020-09-10T17:13:26Z

src/ripple/app/main/Application.cpp

+                for (auto const& name : amendments)
+                {
+                    auto const f = getRegisteredFeature(name);
+                    // BOOST_ASSERT(f);


Why is this commented?

IIRC, it's because I was hitting it during some testing with bogus feature names. Looks like, I forgot to uncomment it. Fixed.

I don't see a fix pushed to github yet

intelliot

I recall that there was a bug where the [amendments] stanza in rippled.cfg did not do what the docs claimed. Has that been fixed, and/or does this PR fix it?

Is the [amendments] stanza the correct way for a validator operator to override what's in downVotedAmendments?

Here are the docs

ximinez · 2020-09-10T17:53:56Z

I recall that there was a bug where the [amendments] stanza in rippled.cfg did not do what the docs claimed. Has that been fixed, and/or does this PR fix it?

Is the [amendments] stanza the correct way for a validator operator to override what's in downVotedAmendments?

It has not been fixed. While working through the spec, we decided fixing it was out of scope, and left it alone. Operators can still use the "feature" RPC to override the down votes.
https://github.com/ripple/rippled-specs/pull/26#issuecomment-683096418

ximinez · 2020-09-11T15:31:52Z

In double-checking the rebase, I discovered that there's a unit test in 1.7.0-b1 that depends on the number of supportedAmendments. I fixed the test, and pushed the fix in a separate commit in front of the down vote changes. I then modified the main commit that test to account for the down votes. Finally, I pushed a fold commit to address the commented assert that @intelliot asked about.

If for some reason, this PR doesn't get merged, the unit test fix will need to be spun off into a separate PR before we change the supported amendments list.

scottschurr

@ximinez, thanks for fixing that TxQ unit test. What you have looks like it will be a lot more self-maintaining in the face of future change than what I'd done previously. 👍

intelliot · 2020-09-11T19:04:49Z

src/ripple/app/main/Application.cpp

+                for (auto const& name : amendments)
+                {
+                    auto const f = getRegisteredFeature(name);
+                    assert(f);


Just curious: How do we decide when to use BOOST_ASSERT vs assert? I guess they are synonymous?

They're basically synonymous, but the boost version adds the dependency on boost. In general, AFAIK, we're moving away from BOOST_ASSERT, but nobody has bothered to do a global search and replace.

intelliot

LGTM

gregtatcam

LGTM 👍

* Allows a version 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.

ximinez · 2020-10-15T15:52:16Z

Squashed and rebased on to 1.7.0-b3

ximinez · 2020-11-12T18:47:15Z

Closing pending further design discussions

intelliot · 2020-11-12T19:42:03Z

Sad - I kind of liked this PR. Hope this code/feature is revived in some form in the future.

ximinez · 2020-11-12T23:14:20Z

@intelliot There are efforts underway.

ximinez force-pushed the internalveto branch 2 times, most recently from 87da13c to 6aaeba8 Compare July 1, 2020 23:09

ximinez force-pushed the internalveto branch from 6aaeba8 to 365b9de Compare July 13, 2020 21:50

ximinez marked this pull request as ready for review August 6, 2020 23:25

ximinez requested review from scottschurr and gregtatcam August 6, 2020 23:27

ximinez assigned gregtatcam and scottschurr Aug 6, 2020

ximinez added the Amendment label Aug 6, 2020

ximinez force-pushed the internalveto branch from a083be1 to e764663 Compare August 10, 2020 15:10

gregtatcam approved these changes Aug 13, 2020

View reviewed changes

ximinez force-pushed the internalveto branch from d096fae to 63833df Compare August 19, 2020 22:56

ximinez force-pushed the internalveto branch from 7a2432d to b994775 Compare August 31, 2020 18:01

scottschurr suggested changes Sep 3, 2020

View reviewed changes

scottschurr approved these changes Sep 10, 2020

View reviewed changes

ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Sep 10, 2020

intelliot reviewed Sep 10, 2020

View reviewed changes

ximinez force-pushed the internalveto branch from 4f8be72 to 33303ab Compare September 11, 2020 15:16

ximinez closed this Sep 11, 2020

ximinez reopened this Sep 11, 2020

scottschurr approved these changes Sep 11, 2020

View reviewed changes

intelliot reviewed Sep 11, 2020

View reviewed changes

intelliot approved these changes Sep 11, 2020

View reviewed changes

gregtatcam approved these changes Sep 14, 2020

View reviewed changes

ximinez force-pushed the internalveto branch from 7e3ffd7 to a001dbf Compare September 28, 2020 16:51

This was referenced Oct 12, 2020

Proposed 1.7.0-b3 #3623

Closed

Proposed 1.7.0-b3 #3628

Closed

Proposed 1.7.0-b3 #3629

Closed

ximinez force-pushed the internalveto branch from a001dbf to a4db914 Compare October 15, 2020 15:47

Internal amendment down votes:

1612564

* Allows a version 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.

ximinez force-pushed the internalveto branch from a4db914 to 1612564 Compare October 15, 2020 15:51

ximinez closed this Nov 12, 2020

ximinez mentioned this pull request Jul 1, 2021

Refactor Feature name management and creation: (Improve amendment management and default votes) #3877

Closed

Internal amendment vetoes: #3458

Internal amendment vetoes: #3458

Conversation

ximinez commented Jun 24, 2020 • edited by intelliot Loading

mDuo13 commented Jun 24, 2020

ximinez commented Jun 25, 2020

MarkusTeufelberger commented Jun 25, 2020

mDuo13 commented Jun 25, 2020

MarkusTeufelberger commented Jun 25, 2020

intelliot commented Jun 26, 2020

ximinez commented Jun 26, 2020

MarkusTeufelberger commented Jun 28, 2020

ximinez commented Jun 29, 2020

MarkusTeufelberger commented Jun 29, 2020

intelliot commented Jul 30, 2020

scottschurr commented Aug 10, 2020

ximinez commented Aug 11, 2020

gregtatcam left a comment

Choose a reason for hiding this comment

ximinez commented Aug 26, 2020

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intelliot left a comment

Choose a reason for hiding this comment

ximinez commented Sep 10, 2020

ximinez commented Sep 11, 2020

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intelliot left a comment

Choose a reason for hiding this comment

gregtatcam left a comment

Choose a reason for hiding this comment

ximinez commented Oct 15, 2020

ximinez commented Nov 12, 2020

intelliot commented Nov 12, 2020

ximinez commented Nov 12, 2020

ximinez commented Jun 24, 2020 •

edited by intelliot

Loading