Repository navigation
Auto-convert basecoin and CT to RingCT - #1055
Conversation
✅ Additional Testing — June 10, 2026Completed full end-to-end test on fresh wallet using latest binary (commit Transaction Flow
Debug Log2026-06-10T18:35:22Z AutoConvert: converting 10.00 basecoin -> CT On-Chain Verification
|
f456afc to
e847010
Compare
seanPhill
added
the
Issue Type: Feature Request
|
This PR auto-converts basecoin and CT to RingCT on every block, closing #1033 and #1054. Tested on mainnet, conversions confirm correctly each block, and autoconvert=0 is available for anyone who needs funds to stay at the base layer instead. @seanPhill — given your comment confirmed this solves the issues you raised, do we want more testing? Let me know if there's additional testing you'd want to see, or any case you think isn't covered yet. |
46af28a to
5e0e48a
Compare
|
Force pushed a substantially reworked v2 and updated the description above. After digging deeper into the wallet internals I found real problems with my original approach: a failed broadcast could strand CT balances (the wallet marks inputs pending spend at build time and nothing undid it), the Behavior changes if you tried v1: the default minimum is now 10 VEIL ( Testing and review before merge. |
|
Pushed a failure-path test suite for the unwind machinery:
Local run: |
ohcee
changed the title
When -autoconvert is enabled, a wallet scheduler task periodically converts batches of confirmed outputs toward RingCT. Consensus only permits RingCT outputs from CT or RingCT inputs, so basecoin batches convert to CT and later cycles lift the matured CT outputs to RingCT. * runs off the validation path on the wallet scheduler with random jitter between attempts * converts at most 32 outputs with 12 or more confirmations per transaction, once the batch reaches -autoconvertthreshold (default 10) * tests mempool acceptance before committing to the wallet * unwinds the transaction record and pending spend flags if a built transaction cannot be broadcast, and backs off exponentially after repeated failures * skips during IBD, before full sync, while locked or staking only unlocked, and when wallet broadcast is disabled Testing and review before merge.
The one part of the feature mainnet happy-path testing cannot reach: a conversion rejected by the mempool must unwind completely. autoconvert_mempool_reject_unwinds drives the public AutoConvertToRingCT() through its gates with mock time. A conflicting spend of one of the wallet's own coinbases, signed with the raw key and placed in the mempool from outside the wallet, guarantees the next conversion attempt builds successfully (record saved, inputs marked pending-spend) and then fails mempool test-acceptance. Asserts the unwind leaves no transaction record, no wallet transaction, and the balance and spendable-coin set bit-identical. The backoff probe doubles as proof the failure really executed: a failed attempt backs off exactly 600s while a NOTHING_TO_CONVERT pass reschedules within at most 540s, so probing at +570s with the conflict cleared distinguishes a genuine failure (still blocked) from an attempt that silently never ran (would convert and fail the mempool-empty check). Past the backoff the conversion must then go through for real: a batch spending all mature coinbases, CT outputs, record and wallet transaction keyed by the same txid. unwind_restores_pending_spend locks the do/undo symmetry directly: MarkInputsAsPendingSpend + SaveRecord (what AddStandardInputs and AddBlindedInputs do at build time) followed by UnwindPendingTransaction restores ORF_PENDING_SPEND and erases the record. The fixture wires a wallet the way CreateWalletFromFile does (HD seed, zerocoin wallet, AnonWallet on a mock db, stealth/blinding contexts) and creates an in-memory pzerocoinDB, which ConnectBlock needs once the regtest chain passes height 20 (accumulator checksums every 10 blocks). X16RT blocks after the PowUpdateTimestamp always retarget to the pow limit, so extending the fixture chain is safe from the regtest DGW overflow.
09eb5db to
9f4d975
Compare
seanPhill
left a comment
seanPhill
left a comment
There was a problem hiding this comment.
utACK
As this is opt-in this can be merged, and further testnet testing can be done.
If anything in the NITS or suggestions (help text, for example) can be done, it can be done later.
PR Review: #1055 – Auto-convert basecoin and CT to RingCT
Overall recommendation: Approve with minor comments / additional testing recommended before merge.
This is a well-thought-out and carefully reworked implementation of a feature that addresses real user pain points (#1033 and #1054). Moving the logic out of BlockConnected into a wallet scheduler task, staging the two conversion hops, adding a mempool pre-check, and implementing proper unwind on failure are all significant improvements over any earlier approach. The design shows a clear understanding of the wallet’s internal state machine and the consensus constraints around RingCT inputs.
Strengths
Correct default and safety posture: The feature is fully opt-in (autoconvert=0 by default). This is the right call. Exchanges, services, and any operator who needs balances to remain in basecoin for accounting or operational reasons are not surprised. Combined with the existing -exchangesandservicesmode, the risk surface for accidental conversion is appropriately low.
Robust failure handling: The combination of “test mempool acceptance before committing wallet state” + AnonWallet::UnwindPendingTransaction + exponential backoff is excellent. The two unit tests (autoconvert_mempool_reject_unwinds and unwind_restores_pending_spend) give good confidence that stranded inputs are avoided.
Privacy-conscious scheduling: Time-based execution with jitter (instead of block-synchronous) and the 12-confirmation maturity requirement are sensible choices that reduce correlation risk.
Practical batching limits: Cap of 32 inputs and a configurable threshold (default 10 VEIL) keep transaction size and frequency under control.
Clear documentation in the PR description: The two-hop explanation, consensus rule reference, and explicit privacy trade-off note are helpful for future maintainers and users.
Specific feedback / questions
Interaction with -exchangesandservicesmode
The PR description does not explicitly state whether the autoconvert scheduler respects (or is affected by) -exchangesandservicesmode. Even though autoconvert is opt-in, it would be cleaner to either:
Document that the two flags are independent, or
Make autoconvert a no-op when exchangesandservicesmode is active (defensive).
A one-line comment or short note in the help text would remove any ambiguity for exchange operators.
Wallet locking / staking-only unlock
Skipping when the wallet is locked or unlocked only for staking is correct. Please confirm (or add a brief comment) that the scheduler does not attempt to unlock the wallet itself under any circumstances.
Restart / pending conversion behaviour
The pending testing list already calls this out, which is good. A quick note in the PR (or a small functional test) confirming that a wallet restart while a conversion is pending leaves no permanent pending-spend flags would be reassuring.
Threshold and privacy trade-off
Default of 10 VEIL feels reasonable. The PR already solicits feedback on this value — I would keep it. The consolidation risk is correctly called out; users who care about input isolation can simply leave the feature disabled.
Logging
When a conversion batch is built and broadcast (or fails and is unwound), is there a clear, non-spammy log line at a reasonable level (info/debug)? Operators monitoring exchange wallets will want to see these events without having to dig through debug logs.
Testing status (as of the latest description)
Completed work looks solid (regtest multi-batch conversion, failure/unwind paths, jitter observation, and a fresh-wallet flow).
Still outstanding items that I would like to see before final merge:
Longer testnet soak under real mempool pressure and fee conditions.
Explicit matrix for encrypted wallets (locked / staking-only / fully unlocked).
Restart-with-pending-conversion case.
Optional: a higher-level functional/regtest test that exercises the full basecoin → CT → RingCT path end-to-end and asserts final balances and no residual pending spends.
Minor nits
The help text for -autoconvert and -autoconvertthreshold should explicitly state that the feature is disabled by default and that conversion is two-hop.
Consider adding a short note in the release notes / user-facing docs once this lands, explaining the opt-in nature and the privacy vs convenience trade-off.
Summary
This is high-quality work. The rework to a scheduler-based, staged, failure-resilient design is the right architecture. The opt-in default correctly protects exchanges and operators who need basecoin balances.
Once the remaining test matrix items are exercised (or at least the restart and encrypted-wallet cases are manually verified), this is ready to merge.
Nice job on the careful state management and the clear documentation of trade-offs.
Veil-Project
left a comment
Veil-Project
left a comment
There was a problem hiding this comment.
ACK
The description itself recommends more "soak testing"on testnet.
This update is fine, and more testnet soaking will begin tonight.
Upon this commit it will contain numerous important and useful upgrades.
3 participants
Summary
Implements opt-in automatic conversion of basecoin and CT balances to RingCT, addressing #1033 and #1054.
This is a substantially reworked version of the original PR based on a deeper review of the wallet's transaction building internals. The conversion no longer runs inside
BlockConnected. It now runs as a periodic wallet scheduler task with confirmation depth, batching, and failure handling safeguards, and the two conversion hops are staged instead of chained as unconfirmed transactions.BlockConnectedis untouched by this PR.How it works
When
-autoconvert=1is set, a wallet scheduler task runs every 60 seconds. For each loaded wallet it:-autoconvertthreshold), builds a single batched transaction for that hopWhy two hops
Consensus only permits RingCT outputs when the inputs are CT or RingCT (
bad-txns-ringct-output-no-anonin, consensus/tx_verify.cpp), so basecoin cannot convert to RingCT in one transaction. Basecoin batches therefore convert to CT, and once those CT outputs reach 12 confirmations a later cycle lifts them to RingCT. Unlike the original design, the two hops are never chained while unconfirmed. Each stage only ever spends outputs with 12 or more confirmations.Failure handling
AddStandardInputs/AddBlindedInputspersist a transaction record and mark inputs pending spend at build time. If anything fails after that point (build, mempool test, or commit), the newAnonWallet::UnwindPendingTransactionremoves the record and clears the pending spend flags so the outputs remain spendable. Previously a failed broadcast could leave CT outputs invisible to the balance until a manualrescanringctwallet.Privacy considerations
Configuration
Disabled by default (opt-in). In
veil.conf:Both options appear in
veild -help.Safety gates
-walletbroadcast=0)Testing status
Done so far (macOS arm64, daemon build):
-autoconvertthreshold=5. Four basecoin→CT batches (32/32/32/13 inputs, respecting the input cap) followed by the CT→RingCT lift after maturity. The entire balance arrived in RingCT with about 0.0073 in total fees and nothing stranded in any poolStill to do:
Closes
#1033
#1054