fix: Disallow MPTClearRequireAuth if is set

[yinyiqian1]

• If domain exists, issuer MUST never clear the RequireAuth flag for MPT.
• some XRPL_ASSERT should not replaced by returning tefINTERNAL to avoid node crash when new features added in the future.

High Level Overview of Change

Context of Change

API Impact

• Public API: New feature (new methods and/or new fields)
• Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• Peer protocol change (must be backward compatible or bump the peer protocol version)

[mvadari]

Changing the asserts to returning tefINTERNAL requires an amendment.

[lmaisons]

For the sake of the less initiated: What happens now when the assert fires? How does the client percieve the current behaviour?

I guess I half-expected this to be so far into "undefined" that tefINTERNAL would be the first version the client was aware of.

[mvadari]

For the sake of the less initiated: What happens now when the assert fires? How does the client percieve the current behaviour?

I guess I half-expected this to be so far into "undefined" that tefINTERNAL would be the first version the client was aware of.

Asserts only fire in debug builds. In release builds (which is what people run in production), it does nothing.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.8%. Comparing base (852fbe9) to head (d42dbb0).
⚠️ Report is 4 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6712     +/-   ##
=========================================
+ Coverage     81.6%   81.8%   +0.2%     
=========================================
  Files         1010    1010             
  Lines        75992   76089     +97     
  Branches      7605    7529     -76     
=========================================
+ Hits         62002   62230    +228     
+ Misses       13990   13859    -131     

Files with missing lines	Coverage Δ
...ibxrpl/tx/transactors/token/MPTokenIssuanceSet.cpp	100.0% <100.0%> (ø)

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[yinyiqian1]

Changing the asserts to returning tefINTERNAL requires an amendment.

@mvadari Just added

[yinyiqian1]

@mvadari @lmaisons Could you help review this PR when you are free? Thanks!

[mvadari]

@yinyiqian1 if you're using fixSecurity3_1_3 then this has to be merged into the 3.1.3 staging branch first in a separate PR.

cc @bthomee on whether this is/should be slated for 3.1.3

[bthomee]

@yinyiqian1 if you're using fixSecurity3_1_3 then this has to be merged into the 3.1.3 staging branch first in a separate PR.

cc @bthomee on whether this is/should be slated for 3.1.3

I think we can squeeze this one in, since it benefits from leveraging the new fix amendment. I don't think it will be too much effort to cherry-pick it into the staging branch after merging into develop.

[yinyiqian1]

@yinyiqian1 if you're using fixSecurity3_1_3 then this has to be merged into the 3.1.3 staging branch first in a separate PR.

cc @bthomee on whether this is/should be slated for 3.1.3

@mvadari I just created a separate PR to merge into staging 3.1.3: #6940. It cherry-picks the commits from current 6712 and resolves conflicts.

[vlntb]

LGTM

[ximinez]

This review can be dismissed and the PR merged after the questions below are answered. (To be fair, the first question is more of an opinion with a question tacked on to the end.)

[mathbunnyru]

Removing the "Ready to merge label" until Ed's questions are answered

As promised, my questions have been answered, so I'm dismissing this review.