Add the ability to mark amendments as obsolete

[ximinez]

High Level Overview of Change

• Prevents the server from ever voting on the given amendment,
  regardless of operator configuration.
• Also prevents the "feature" command from changing the amendment's
  vote.

Context of Change

This comment: #4282 (comment) pointed out an ongoing issue - there are amendments that simply shouldn't be voted on because they're broken, there's no actual code implementing them, or other reasons. Unfortunately, they need to remain supported on the main net servers on the off chance that they do get enabled. Remember: If they get enabled, any versions that don't support them will be amendment blocked.

Up to now, the communication to let operators know not to vote for these amendments has been done via back channels, blog posts, etc. Why not use code to both communicate and enforce this message?

A determined operator is still not absolutely prevent from voting against an amendment. They are free to change the source and compile locally, and still stick to the rules of the protocol.

Type of Change

• New feature (non-breaking change which adds functionality)
• Tests (You added tests for code that already exists, or your new feature included in this PR)

Before / After

Before: there are two options for an amendment's DefaultVote behaviour: yes and no.

After: there are three options for an amendments VoteBehavior: DefaultYes, DefaultNo, and Obsolete.

To go along with this, for the feature admin API, there's a new possible value for the vetoed field. For obsolete amendments, the vetoed field has the value "Obsolete".

Test Plan

There are currently four amendments marked Obsolete that haven't passed yet:

1. CryptoConditionsSuite
2. NonFungibleTokensV1
3. fixNFTokenDirV1
4. fixNFTokenNegOffer

Test cases:

1. Try voting for them. Betcha can't.
2. Run this code against a test network running majority older versions. Get one of these amendments enabled. Verify that this code doesn't get amendment blocked.

Future Tasks

Amendment governance is an ongoing discussion. I am not going to rehash all the details here.

[RichardAH]

Seems reasonable. You might also consider placing any UNL validator found voting for an obsoleted amendment on the nUNL for each flag ledger that voting occurs. However if you do this then the code change needs to come with an amendment for the nUNL behaviour change.

[MarkusTeufelberger]

This would leak even more info about your UNL though?

I'm not sure if "dead" amendments should be carried in code as "Obsolete" forever, on the other hand it only adds a few lines compared to the alternative of just deleting the obsolete amendments completely and seems relatively safe compared to the current situation.

[ckeshava]

This would leak even more info about your UNL though?

Can you describe more? Do you mean: If a validator removes a node from its UNL (places it on nUNL) and publishes its ValidatorList, then external world can take a diff between the versions of ValidatorList and discern the one validator on it past UNL?

[MarkusTeufelberger]

Any validator that gets put by your validator on a nUNL proposal is currently trusted by your validator. If I can get on a nUNL proposal just by proposing an obsolete amendment, it is a good way to make all other validators that have me on their UNL leak that information. Validations are not logged or stored and proposals are even more ephemeral, so this would be largely unnoticed or look like an accidental misconfiguration.

[RichardAH]

Any validator that gets put by your validator on a nUNL proposal is currently trusted by your validator. If I can get on a nUNL proposal just by proposing an obsolete amendment, it is a good way to make all other validators that have me on their UNL leak that information. Validations are not logged or stored and proposals are even more ephemeral, so this would be largely unnoticed or look like an accidental misconfiguration.

I don't know why you think the UNL of validators should be a secret to begin with? I would actually rather they publish / volunteer that information. Whether or not I want to trust your validator strongly depends on who else your validator listens to.

[MarkusTeufelberger]

I don't know why you think the UNL of validators should be a secret to begin with?

Because this is one of the main design goals of the algorithm for example to be able to add validators to your UNL that might be viewed controversially in your jurisdiction. A good UNL would for example have lots of validators on there that are very unlikely to collude, but by that very nature it means that there will be some raised eyebrows if you start adding Iranian, Chinese and Russian validators in addition to the current recommended, western centric one. There is also no way built in to verify or attest that a certain host is on my UNL, so even if someone volunteers that data, it can be trivially faked or false. This is not the right place to explain this though - I'm just pointing out that UNLs are supposed to be private (and if you want to volunteer some info about yours, that's your personal choice) and publishing a nUNL proposal is already leaking information. Adding more ways to add up on there just makes the situation worse.

[RichardAH]

Because this is one of the main design goals of the algorithm for example to be able to add validators to your UNL that might be viewed controversially in your jurisdiction. A good UNL would for example have lots of validators on there that are very unlikely to collude, but by that very nature it means that there will be some raised eyebrows if you start adding Iranian, Chinese and Russian validators in addition to the current recommended, western centric one. There is also no way built in to verify or attest that a certain host is on my UNL, so even if someone volunteers that data, it can be trivially faked or false. This is not the right place to explain this though - I'm just pointing out that UNLs are supposed to be private (and if you want to volunteer some info about yours, that's your personal choice) and publishing a nUNL proposal is already leaking information. Adding more ways to add up on there just makes the situation worse.

Given that validations are relayed and can be/are generated pseudonymously, and the keys can be changed on an existing validator and it appears as a new validator, I think this is putting a hat on a hat. If you want the location of your validator to be secret then it already is: simply don't tell anyone where it runs.

Network safety requires very high (I would strongly argue 100%) UNL overlap. Hypothetical privacy concerns that are already dealt with by pseudonymity should not trump real practical fork safety.

[scottschurr]

This is a clever extension of the existing code. I like it for the most part. There is one part of the testing implementation that I think is dangerous, and that needs to be replaced. But, on the whole, nicely done.

Oh, and I looked at the code coverage of the new/revised code. That looks good too. Good tests. Thanks!

[scottschurr]

I'll be honest. I don't like this template one bit. Yes, it's clever. But it adds capabilities to a std::vector that, according to the standard, a vector should not have.

And, despite the comment, it it currently important that the definition be a template. This file uses the template both for a std::vector<std::string> and a std::vector<AmendmentTable::FeatureInfo>. Please remove this template. The two places that it is used are more safely handled by using typical (more verbose) code.

[ximinez]

Fixed / rewritten.

[wojake]

While amendment governance is the main topic, I'd like to share a new idea, an additional vote 'HaveNotDecided' would be created in this new mechanism.

Yes: Enable, participated
No: Disable, participated
HaveNotDecided: Disable, didn't participate

HaveNotDecided would technically be considered as a "No" vote since this vote represents a validator's vote to not enable an amendment, but the HaveNotDecided vote would be classified as a vote that represents a validator's non-participation in amendment governance.

If an operator votes Yes or No for a particular amendment, this would indicate their participation in amendment governance. Otherwise, the default vote would be HaveNoteDecided which represents their passive status in governance (not participating).

Before the addition of HaveNotDecided, Yes would indicate a validator's wish to enable an amendment and their participation in amendment governance but No wouldn't technically mean a validator's participation in amendment governance.

This new vote addition would benefit the network and VL publishers as we're able to identify which validator hasn't participated in amendment governance and in what sequence of amendments. Rather than relying on a No vote & Fee reserve to speculate if a validator isn't participating in amendment governance, HaveNotDecided would allow us to properly analyze if a validator is participating or not. Their decision to enable or disable an amendment (Yes/No) would be their right to do so.

Is this necessary?

And if you're wondering if all of this is necessary as validators are actively participating in amendment governance (as of the NFT amendment, only 2 hasn't voted Yes): It really is important. Validators have the final say in the XRPL's advancements in terms of features and we should be able to properly analyze which validator is participating and which are not, being able to effectively analyze such data gives us the ability to add and remove validators from a VL, safely and with assurance that an analysis is correct.

We should not wait until things go south again (validators turn passive).

Let's take a recent event as an example: #4282
There are passive validator operators on the dUNL and there were no real transparent way to analyze which validator is accualty passive and which is accualty voting No. This new vote would help.

Current suggested approach

Discussed this matter with the others and it seems like including this as a new field on validation messages would be a do-able approach. Validation messages are discarded after consensus has been reached, ledger size will not increase.

[ximinez]

I'm not sure if "dead" amendments should be carried in code as "Obsolete" forever, on the other hand it only adds a few lines compared to the alternative of just deleting the obsolete amendments completely and seems relatively safe compared to the current situation.

Two counter-points:

1. In the current code, "dead" amendments have to be carried forever, because there's no way to safely get rid of them. (See also:

   rippled/src/ripple/protocol/impl/Feature.cpp

   Lines 449 to 456 in 175c9b2

   	// The following amendments are obsolete, but must remain supported because they could
   	// potentially get enabled.
   	//
   	// Obsolete features are (usually) not in the ledger, and may have code
   	// controlled by the feature. They need to be supported because at some
   	// time in the past, the feature was supported and votable, but never
   	// passed. So the feature needs to be supported in case it is ever
   	// enabled (added to the ledger).

   )
2. As noted at

   rippled/src/ripple/protocol/impl/Feature.cpp

   Lines 458 to 459 in 175c9b2

   	// If a feature remains obsolete for long enough that no clients are able
   	// to vote for it, the feature can be removed (entirely?) from the code.

   , this new feature would allow dead features to be removed from the code once there are no versions in the wild which are capable of voting for it.

[ximinez]

Seems reasonable.

Thanks! Could you submit a review approval?

You might also consider placing any UNL validator found voting for an obsoleted amendment on the nUNL for each flag ledger that voting occurs. However if you do this then the code change needs to come with an amendment for the nUNL behaviour change.

This is an interesting idea, but it's beyond the intended scope of this PR.

[scottschurr]

👍 The most recent changes look good. Thanks.

[mDuo13]

Just to be clear, if an "Obsolete" amendment were to (somehow) become confirmed by consensus, the server still has the code to process transactions according to that amendment so it would not be amendment blocked, and it would continue to function the same as if it had been voting "no" on the amendment normally?

I am not sure this is an improvement over the old way of simply removing obsolete amendments from the code. Suppose an amendment has some very bad code associated with it (like, it causes corrupted ledgers or crashes) and becomes enabled. Is it better to jump off the cliff with the rest of consensus, or is it better to say, "Hey, the situation is not OK, I'm stopping here"?

Personally I'd rather legitimate servers become amendment blocked and maybe even stop the whole network rather than enable a known-broken amendment. That's the "correctness over forward progress" principle, right? I guess the pragmatic counter-argument is that some bugs aren't so bad that they're worth stopping the whole network even if some undesirable stuff happens?

I'd also like to preserve a path to actually, fully removing the obsolete code from the server, given enough time. I suppose it would make sense to extend XLS-11. One approach would be to give obsolete amendments the same longevity as the legacy code for enabled amendments, and retire the code two years after it's marked as obsolete. Or, it could be less, because the legacy code for obsolete amendments isn't needed for interpreting historical data (on Mainnet at least).

[ximinez]

Just to be clear, if an "Obsolete" amendment were to (somehow) become confirmed by consensus, the server still has the code to process transactions according to that amendment so it would not be amendment blocked, and it would continue to function the same as if it had been voting "no" on the amendment normally?

Yes, that is correct.

I am not sure this is an improvement over the old way of simply removing obsolete amendments from the code. Suppose an amendment has some very bad code associated with it (like, it causes corrupted ledgers or crashes) and becomes enabled. Is it better to jump off the cliff with the rest of consensus, or is it better to say, "Hey, the situation is not OK, I'm stopping here"?

Personally I'd rather legitimate servers become amendment blocked and maybe even stop the whole network rather than enable a known-broken amendment. That's the "correctness over forward progress" principle, right? I guess the pragmatic counter-argument is that some bugs aren't so bad that they're worth stopping the whole network even if some undesirable stuff happens?

Have we every actually removed an obsolete amendment? I know we have one (CryptoConditionsSuite) that has no actual code associated with it, but it's still there.

Fortunately, the "very bad code" scenario hasn't happened, AFAIK, and I don't think this change really attempts to solve that problem. If that were to come up, we'd probably have to deal with it differently.

I think the existing governance model can avoid this problem pretty well, too.

I'd also like to preserve a path to actually, fully removing the obsolete code from the server, given enough time. I suppose it would make sense to extend XLS-11. One approach would be to give obsolete amendments the same longevity as the legacy code for enabled amendments, and retire the code two years after it's marked as obsolete. Or, it could be less, because the legacy code for obsolete amendments isn't needed for interpreting historical data (on Mainnet at least).

The Obsolete amendment functionality gives us that path. The idea is that right now, any amendment that "everybody knows" is obsolete hangs around in the code indefinitely, just in case it ever does get enabled. Now that it can be marked "Obsolete" in code, once the network is only running versions that can't vote for it, it could be fully removed from the code (or in the case of the old NFT amendments, the code can be changed to use the current version). We wouldn't have to wait any prescribed amount of time, because the data will show when it's "safe".

[intelliot]

@RichardAH feel free to review this, at your convenience.

[HowardHinnant]

Nicely done!

[ximinez]

Thanks!