Remove CryptoConditionsSuite stub amendment

[mDuo13]

Issue Description

The CryptoConditionsSuite amendment is not fully implemented but it is listed in the code as an amendment that can be enabled. There should not be stub amendments in production releases, for several reasons, including:

• If later versions introduce a full version of the same amendment, earlier servers won't become properly amendment blocked despite lacking the code
• Any functionality tied to the stub amendment is likely undesirable in production. For example, it has been speculated that CryptoConditionsSuite, if it were to become enabled, might allow the creation of unfinishable conditional escrows.

We should remove CryptoConditionsSuite and its associated functionality from v1.6.0 ASAP. If we later introduce a full version of the amendment, we should ensure one of the following conditions is met:

• The new, full amendment has a different name, or
• Mainnet servers that had the stub amendment are already amendment blocked by a different amendment being enabled (e.g. a different v1.6.0+ amendment has already gained a majority. So this could only happen after v1.6.0 is fully released.)

[MarkusTeufelberger]

This amendment is HIGHLY popular amongst non-Ripple validators (26 vote for including it right now, 10 against): https://xrpscan.com/amendment/86E83A7D2ECE3AD5FA87AB2195AE015C950469ABF0B72EAACED318F74886AE90

In fact, only 5 entities on the default UNL are voting against it and it is only vetoed because Ripple has more than one vote at the moment.

If the numbers were reversed (only a few entities actually support it), it might be different, but in this current state it seems that most validators seem to actually want this to go live in its current state. Why remove the code for such a popular and widely supported (even/especially by defaultUNL validators!) amendment?

[mDuo13]

I believe that the widespread support for this amendment is probably due to a misconception, stemming from validators trusting that Ripple would not put an unfinished amendment into a release branch. Unfortunately, in this case, the truth is that this amendment is a stub and it almost certainly does not do what they think it does.

They may be reading the description on the Known Amendments page:

Implements several types of crypto-conditions from the official crypto-conditions specification for use in EscrowCreate and EscrowFinish transactions. Without this amendment, only the PREIMAGE-SHA-256 type is supported.

Caution: This amendment is still in development. The version from rippled v0.60.0 to present does not implement the full functionality.

.. and missing the "Caution" callout or thinking it's outdated. Or they may be automatically voting in favor of most new amendments, on a premise like, "If it's good enough for Ripple to release it, it must be good for the network."

Tragically, that is not the case for CryptoConditionsSuite as it stands currently. Most condition types are unimplemented in the released versions of rippled.

Unless the validators actually provide a statement with regards to their votes on this topic, I think it's safe to assume that they are following the default and voting in favor of new features. While that discussion is related, this particular amendment should not have made it into a production release in its current form, and we should rectify that.

[nbougalis]

I agree: the amendment is basically a stub, left-over because of legacy considerations. It really should be removed from the code base entirely to avoid accidental activation. If and when further cryptoconditions are implemented, they should each be on different amendments.

[MarkusTeufelberger]

This would indicate that the recommended validators either don't even know how to configure their votes or that their votes on this matter (the amendment is 3(!) votes away from passing) are meaningless for rippled developers. Both not very nice things to have. Personally I'd like to see explicit voting first before dropping widely supported amendments to see how widely supported they actually are in case you suspect that operators of recommended validators are not acting in the best interest of actively stewarding the XRPL.

Lastly, just because rippled doesn't implement this doesn't mean that others haven't implemented it. There is a nonzero chance that someone has merged the existing code e.g. from #2170. In my opinion this conflates the protocol with its (reference?) implementation. CryptoConditions is a spec + amendment that's apparently highly popular amongst Ripple-UNL validators (and even more popular amongst non-Ripple-UNL ones). This includes all academic validator operators and also (with the exception of Ripple and Alloy) all commercial ones - including a company run by a former Ripple CTO as well as the validator run by xrpchat, a large community hub.

[seelabs]

If we do want to do this I created a PR here: #3453

[nbougalis]

This would indicate that the recommended validators either don't even know how to configure their votes or that their votes on this matter (the amendment is 3(!) votes away from passing) are meaningless for rippled developers. Both not very nice things to have. Personally I'd like to see explicit voting first before dropping widely supported amendments to see how widely supported they actually are in case you suspect that operators of recommended validators are not acting in the best interest of actively stewarding the XRPL.

I suspect that a large number of validator operators don't take active stances on amendments and rely, instead, on the default behavior: vote in support of all amendments a server understands.

Lastly, just because rippled doesn't implement this doesn't mean that others haven't implemented it.

Sure. This change doesn't affect any such implementations at all. In fact, even if this amendment activated, the behavior of the code would not change:

EscrowCreate.cpp:

    if (auto const cb = ctx.tx[~sfCondition])
    {
        using namespace ripple::cryptoconditions;

        std::error_code ec;

        auto condition = Condition::deserialize(*cb, ec);
        if (!condition)
        {
            JLOG(ctx.j.debug())
                << "Malformed condition during escrow creation: "
                << ec.message();
            return temMALFORMED;
        }

        // Conditions other than PrefixSha256 require the
        // "CryptoConditionsSuite" amendment:
        if (condition->type != Type::preimageSha256 &&
            !ctx.rules.enabled(featureCryptoConditionsSuite))
            return temDISABLED;
    }

The existing implementation of Condition::deserialize(*cb, ec) will return nullptr and set error to either error::unsupported_type or error::unknown_type, which means that, whether this change is enabled or not, no rippled instance will process this transaction locally and it will simply return temMALFORMED.

So, the amendment check below is a vestigial safeguard; indeed, this whole amendment, is a no-longer-useful left-over.

There is a nonzero chance that someone has merged the existing code e.g. from #2170.

There is, but if they did, this change wouldn't affect them: their server would likely continue voting in support of this amendment and if, perchance, the amendment ever activated, their server would have processed transactions differently than the reference implementation which the majority of operators use.

And considering the above snippet of code, amendment or not, not much would change for servers that haven't.

I'd argue that this uncertainty about who's using this amendment identifier and what it means to them is, itself, reason enough to remove the amendment identifier to prevent confusion.

In my opinion this conflates the protocol with its (reference?) implementation.

This is reasonable criticism, and certainly a discussion ought to be had about amendments, how they should be introduced and whether a public spec should be mandator for any change that introduces such a transaction breaking change.

But let's also consider this: support for amendments that haven't yet reached majority is open-ended (that is, there's not time limit built into the process that says "ah, this didn't activate in time, it's no longer considered) but open-ended doesn't necessarily mean perpetual. If an amendment has been in the codebase for a couple of years but hasn't yet been enabled, it's not unreasonable to move to remove its implementation and if the implementation is removed, the server should not continue advertising support for the amendment. What we can argue about is when it makes sense to remove said identifier/implementation and what the implications are.

CryptoConditions is a spec + amendment that's apparently highly popular amongst Ripple-UNL validators (and even more popular amongst non-Ripple-UNL ones). This includes all academic validator operators and also (with the exception of Ripple and Alloy) all commercial ones - including a company run by a former Ripple CTO as well as the validator run by xrpchat, a large community hub.

It's true that the amendment seems highly popular. But there are two issues:

1. The amendment, even if activated, does nothing on the most popularonly implementation of the protocol.
2. The existing amendment ID can't be used to gate new code.

I am supportive of, we could just do nothing: leave this amendment in and if it ever activates, big whoop...

Honestly, that we are having this conversation is a good thing but also represents a failure on the part of the Ripple team my part, as the original author of this code. I should have separated each condition under a separate amendment, but I didn't and I should not have introduced an amendment for half-baked code. In my defense, back them we were much more lax about amedments.

[seelabs]

I vote to remove the amendment from the code base. Right now, enabling the amendment doesn't enable any new behavior. Since it's already shipped, if we (or someone in the community) did merge a cryptoconditions implementation, that implementation would need a new amendment anyway. If it didn't, we'd have some systems where the amendment would be on with an empty implementation and some with a real implementation.

Since this amendment can never be anything other than a no-op, I think it makes sense to remove it.

[gnurag]

From anecdotal evidence I believe a lot of validators auto-update rippled, and therefore auto-vote in favor of new amendments from day 0. At this point I would take the popularity of amendments measured by votes with a pinch of salt. My thoughts on CryptoConditionsSuite are:

1. Remove CryptoConditionsSuite amendment code. If its unimplemented, it shouldn't be shipped and definitely shouldn't be activated.
2. An RFC style discussion over amendment specs would be great to have. We had great response to the DeletableAccounts and MemoFee specs and I learnt a lot from those RFCs. Morever, I can't read C++, so reading the spec helps me.
3. Splitting CryptoConditionsSuite into individual CryptoConditions makes sense.
4. I'd still recommend implicit voting on amendments, till we're sure the validators are actively participating in the amendment voting process.

Additionally, I am wondering what are the reasons (apart from network stability) why Ripple funds the development of new amendments in the reference implementation, yet continues vetoing them for this long.

[alloynetworks]

As one of the validators who is vetoing this amendment, I would agree that removing it makes sense. As has been pointed out earlier, most auto updating validators passively accept amendments .

[MarkusTeufelberger]

removed in 1.6.0-b8

[intelliot]

Fixed by #3453 and released in 1.6.0