feat: Support ConfidentialMPTMirrorUpdate - #8192
yinyiqian1 wants to merge 13 commits into
Conversation
d511ca9 to
7f169ef
Compare
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
f4162cd to
ac4e06e
Compare
ac4e06e to
dc25085
Compare
yinyiqian1
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
The ConfidentialMPTMirrorUpdate transactor's preflight/preclaim logic for mode detection, staleness checks, and epoch bookkeeping is well thought out and heavily tested. The one real gap is that ZK-proof verification is explicitly stubbed out ("todo: check zkproof") in both preflight and preclaim, meaning the currently-shipped implementation never verifies the equality proof that is supposed to guarantee the new ciphertext encodes the same balance as the mirror it replaces — anyone satisfying the mode/staleness checks can currently set an arbitrary encrypted balance. The rest of the diff (mirror-epoch split in ConfidentialTransfer.cpp, new Protocol.h constant, macro/autogen wiring, test helpers) looks consistent with existing patterns.
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
Equality-proof verification is not implemented, and proof-length validation and required API documentation updates remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds ConfidentialMPTMirrorUpdate support for confidential MPT key rotation, including transaction processing, generated APIs, helpers, and tests.
Changes:
- Registers the transaction and generated wrappers.
- Implements mirror updates, epoch handling, validation, and fees.
- Adds JTX helpers and protocol/application tests.
File summaries
| File | Description |
|---|---|
src/tests/libxrpl/protocol_autogen/transactions/ConfidentialMPTMirrorUpdateTests.cpp |
Tests generated transaction APIs. |
src/test/jtx/mpt.h |
Adds mirror-update test arguments and APIs. |
src/test/jtx/impl/utility.cpp |
Applies confidential transaction fees. |
src/test/jtx/impl/mpt.cpp |
Builds mirror-update test transactions. |
src/test/app/ConfidentialMPTKeyRotation_test.cpp |
Adds key-rotation integration coverage. |
src/libxrpl/tx/transactors/token/ConfidentialMPTMirrorUpdate.cpp |
Implements transaction validation and mirror updates. |
src/libxrpl/protocol/ConfidentialTransfer.cpp |
Implements mirror epoch helpers. |
include/xrpl/tx/transactors/token/ConfidentialMPTMirrorUpdate.h |
Declares the transactor. |
include/xrpl/protocol/Protocol.h |
Defines the equality-proof length constant. |
include/xrpl/protocol/detail/transactions.macro |
Registers the transaction and fields. |
include/xrpl/protocol/ConfidentialTransfer.h |
Declares mirror epoch helpers. |
include/xrpl/protocol_autogen/transactions/ConfidentialMPTMirrorUpdate.h |
Provides generated transaction wrappers and builder. |
Review details
Suppressed comments (3)
include/xrpl/protocol/detail/transactions.macro:1147
- This PR adds a new
libxrpltransaction type and protocol fields, but the PR description leaves both thelibxrpland public-API impact boxes unchecked (and no API changelog entry is included). Please update the API-impact checklist and the required changelog/documentation for this externally visible transaction.
TRANSACTION(ttCONFIDENTIAL_MPT_MIRROR_UPDATE, 92, ConfidentialMPTMirrorUpdate,
({.delegable = Delegation::Delegable, .amendment = featureConfidentialMPTKeyRotation}),
({
{sfMPTokenIssuanceID, SoeRequired},
{sfHolder, SoeOptional},
{sfIssuerEncryptedAmount, SoeOptional},
{sfAuditorEncryptedAmount, SoeOptional},
{sfZKProof, SoeRequired},
src/libxrpl/tx/transactors/token/ConfidentialMPTMirrorUpdate.cpp:80
- The required
sfZKProofis accepted without any size check, so malformed transactions can pass preflight with a proof of arbitrary length. Validate it againstkEcEqualityProofLengthhere (before the ciphertext format checks or alongside them) and returntemMALFORMEDwhen it does not match.
// todo: check zkproof
src/test/app/ConfidentialMPTKeyRotation_test.cpp:1386
- This newly added comment has subject–verb agreement error: “issuance” is singular, so “have” should be “has.”
// The issuance have not enabled confidential balances.
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Adds the ConfidentialMPTMirrorUpdate transaction (issuer/holder key-rotation mirror migration) plus supporting protocol plumbing and tests. The core mode/holder/staleness validation logic in preclaim looks sound and well-covered by tests. The main gap is that the new sfZKProof field is required by the schema but is never actually validated: no length check (despite a new kEcEqualityProofLength constant being introduced specifically for it) and no cryptographic verification, so any content (or garbage) currently passes. The ConfidentialTransfer.h/.cpp refactor splitting setMirrorEpochs into setIssuerMirrorEpoch/setAuditorMirrorEpoch is a clean, behavior-preserving extraction.
There was a problem hiding this comment.
🟡 Changes recommended
Critical compilation and security findings remain, along with missing validation and required coverage/documentation.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
src/libxrpl/tx/transactors/token/ConfidentialMPTMirrorUpdate.cpp:78
kEcEqualityProofLengthis introduced for this transaction, but preflight never checkssfZKProofagainst it. As a result, a present proof of any length (including an empty VL) is accepted and the mirror update can be applied, unlike the other confidential MPT transactions that reject malformed proof lengths. Add the fixed-length check now even if cryptographic verification is deferred.
include/xrpl/protocol/detail/transactions.macro:1147- This adds a new public transaction type and fields, but the required API changelog entry is missing. Prior public transaction additions are documented in
API-CHANGELOG.md(for example, lines 241–251); without an entry, clients and release documentation will not discover this amendment-gated transaction.
src/libxrpl/tx/transactors/token/ConfidentialMPTMirrorUpdate.cpp:87
- The shared confidential-transaction fee test enumerates the existing confidential MPT transactions but does not submit
ConfidentialMPTMirrorUpdate(src/test/app/ConfidentialTransfer_test.cpp:7081-7125). The newcalculateBaseFeepath is therefore untested; add a successful mirror-update case that verifies the confidential fee multiplier.
XRPAmount
ConfidentialMPTMirrorUpdate::calculateBaseFee(ReadView const& view, STTx const& tx)
{
return Transactor::calculateBaseFee(view, tx, kConfidentialFeeMultiplier);
}
src/test/app/Delegate_test.cpp:2759
- This count update adds the new transaction to the delegable set without adding delegated test cases for it. The warning above requires every new delegable transaction's standard scenarios to be duplicated in a delegated context, but
MPTMirrorUpdatehas nodelegateargument and the new key-rotation suite contains no delegated issuer/holder flows. Raising the count alone suppresses that security check; add those cases before changing the expected count, or do not mark the transaction delegable.
std::size_t const expectedDelegableCount = 57;
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
| (*sleMptoken)[sfAuditorEncryptedBalance] = ctx_.tx[sfAuditorEncryptedAmount]; | ||
| setAuditorMirrorEpoch(*sleIssuance, *sleMptoken); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missed sfIssuerMirrorEncryptionKey update
There was a problem hiding this comment.
This will be in a separate PR tracked in another ticket.
(That PR should initialize and update the sfIssuerMirrorEncryptionKey and sfInitialIssuerEncryptionKey)
| */ | ||
| struct MPTMirrorUpdate | ||
| { | ||
| std::optional<Account> account = std::nullopt; |
There was a problem hiding this comment.
std::nullopt; is default value, not needed here
There was a problem hiding this comment.
Let's keep this as it is now. Because all the other places in this file use this style. If we decide to remove, we can have another refactor PR.
There was a problem hiding this comment.
This is a well-structured, heavily-tested addition of the ConfidentialMPTMirrorUpdate transactor. I traced the preflight/preclaim/doApply logic for both issuer-mode and holder-self-migration flows (including epoch staleness checks, auditor late-registration, and the canonical-zero-inbox requirement for holder migrations) against the extensive test suite and didn't find a correctness or security bug in the changed code. ZKProof verification is intentionally deferred to a follow-up PR and is safe since the gating amendment (featureConfidentialMPTKeyRotation) is not yet enabled, as called out in the description. The one item below is a design-intent question rather than a confirmed bug.
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate coverage and delegation findings remain unresolved, along with API documentation and comment fixes.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
include/xrpl/protocol/detail/transactions.macro:1147
- This adds a new public transaction type and fields, but the PR does not update
API-CHANGELOG.mdto document the API addition. Please add an entry describingConfidentialMPTMirrorUpdate, its fields, and the amendment gate so API consumers can discover the new submit/sign format.
src/test/app/ConfidentialMPTKeyRotation_test.cpp:1388
- The comment has a subject–verb agreement error: “issuance” is singular, so “have” should be “has.”
// The issuance have not enabled confidential balances.
src/test/jtx/impl/utility.cpp:81
- The new type is added to the confidential fee auto-fill list, but the existing
testConfidentialMPTBaseFeecoverage still exercises only Convert, MergeInbox, Send, ConvertBack, and Clawback. A regression in this transaction'scalculateBaseFeeor fee handling would therefore pass; add MirrorUpdate to the normal, insufficient-fee, and excessive-fee checks.
txType == jss::ConfidentialMPTClawback || txType == jss::ConfidentialMPTMirrorUpdate)
src/test/jtx/mpt.h:375
- This transaction is declared delegable in
include/xrpl/protocol/detail/transactions.macro:1141, but the new JTX argument has nodelegate(orticketSeq) member. Unlike the existing confidential argument structs insrc/test/jtx/mpt.h:311-318and:337-362,MPTTester::submittherefore cannot forward delegated signing for this transaction, so the issuer and holder migration flows have no delegated success/permission coverage despite the delegable-count change. Add those fields and delegated tests for both modes before expanding this surface.
std::optional<Buffer> zkProof = std::nullopt;
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
Well-structured, heavily-tested addition of the ConfidentialMPTMirrorUpdate transactor (issuer + holder self-migration modes for re-encrypting confidential MPT mirrors). The refactor of setMirrorEpochs into per-mirror helpers is clean and correctly wired into the new transactor's doApply. One correctness question worth resolving before merge: the preclaim precondition that gates an auditor-only migration on the issuer mirror already being current is applied only in issuer mode, not in holder self-migration mode, which looks like an unintentional asymmetry.
There was a problem hiding this comment.
The ConfidentialMPTMirrorUpdate transaction is well-structured with thorough preflight/preclaim gating (issuer/holder mode discrimination, mirror-staleness checks, inbox-canonical-zero requirement for holder self-migration) and solid test coverage across both modes and multi-rotation scenarios. I did not find correctness or security bugs in the new transactor logic, epoch-setter split, or invariant wiring — the only note is a minor consistency nit on the new proof-length constant.
There was a problem hiding this comment.
This is a large, well-structured addition of the ConfidentialMPTMirrorUpdate transactor with thorough preflight/preclaim validation and an extensive test suite. I traced the field mappings in doApply (IssuerEncryptedAmount→IssuerEncryptedBalance, AuditorEncryptedAmount→AuditorEncryptedBalance) for the classic copy-paste swap bug and found them correct, and the epoch-setting refactor in ConfidentialTransfer.cpp is a clean, correct split of the prior combined function. I did not find high-confidence correctness or security bugs in the added lines; the one item below is a design-consistency question rather than a clear defect, and the ZKProof field being unverified is already explicitly and appropriately called out in the PR description as deferred, amendment-gated follow-up work.
There was a problem hiding this comment.
🟡 Changes recommended
Add the API changelog entry and delegate/ticket fields to the JTX helper.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/test/jtx/mpt.h:376
ConfidentialMPTMirrorUpdateis declaredDelegation::Delegableininclude/xrpl/protocol/detail/transactions.macro:1141, but this JTX argument type has nodelegateorticketSeqmembers. As a result,MPTTester::mirrorUpdatecannot construct delegated or ticket-based mirror updates, unlike the other delegable confidential-MPT helpers in this file, so those protocol paths cannot be exercised through the standard test helper. Add both optional fields sosubmit()can forward them.
std::optional<Buffer> issuerEncryptedAmount = std::nullopt;
std::optional<Buffer> auditorEncryptedAmount = std::nullopt;
std::optional<Buffer> zkProof = std::nullopt;
std::optional<XRPAmount> fee = std::nullopt;
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Lite
| #if TRANSACTION_INCLUDE | ||
| # include <xrpl/tx/transactors/token/ConfidentialMPTMirrorUpdate.h> | ||
| #endif | ||
| TRANSACTION(ttCONFIDENTIAL_MPT_MIRROR_UPDATE, 92, ConfidentialMPTMirrorUpdate, |
High Level Overview of Change
Adds the
ConfidentialMPTMirrorUpdatetransaction (type 92, gated onfeatureConfidentialMPTKeyRotation),which re-encrypts a holder's mirrored confidential balances under the ElGamal public keys currently registered
on the
MPTokenIssuance.Transaction shape
MPTokenIssuanceIDHolderIssuerEncryptedAmountAuditorEncryptedAmountZKProofIssuerEncryptedAmount/AuditorEncryptedAmountmust be present.Supported flows
Issuer mode, submitted by the issuer with
Holderfield:IssuerEncryptedBalanceunder the issuer's new key.AuditorEncryptedBalanceunder the auditor's new key.MPTokenIssuanceSetafterholders already hold confidential balances, this sets each holder's initial
AuditorEncryptedBalance.Holder self-migration, submitted by the holder with
Holderfield absent. The holder decrypts their ownConfidentialBalanceSpendingand re-encrypts it under the relevant new key(s):IssuerEncryptedBalanceunder the issuer's new key.AuditorEncryptedBalanceunder the auditor's new key,or sets it for the first time when the auditor key was registered after the holder already held a confidential balance.
Please Note: The zkproof related logic is not included in this PR. It will be added in a separate PR. It is safe now
since the amendment is not enabled. We are still working in progress.
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)