fix(rpc/wallet): RC33 battery cleanup — 9 user-reachable defects#405
Open
JohnnyLawDGB wants to merge 22 commits intoDigiByte-Core:feature/digidollar-v1from
Open
fix(rpc/wallet): RC33 battery cleanup — 9 user-reachable defects#405JohnnyLawDGB wants to merge 22 commits intoDigiByte-Core:feature/digidollar-v1from
JohnnyLawDGB wants to merge 22 commits intoDigiByte-Core:feature/digidollar-v1from
Conversation
…LAR active (RH-54) Before this fix, IsOpSuccess() statically excluded 0xbb..0xbf from the OP_SUCCESSx range. This broke BIP342 forward-compatibility: old nodes that don't know SCRIPT_VERIFY_DIGIDOLLAR should see these bytes as unconditional successes, not execute them. Fix: restore the full BIP342 OP_SUCCESSx set in IsOpSuccess(). Add IsOpSuccessForFlags() in the interpreter that short-circuits only when SCRIPT_VERIFY_DIGIDOLLAR is NOT active — so pre-activation nodes keep OP_SUCCESSx semantics and post-activation nodes evaluate the opcodes. Also adds a Tapscript-only guard: DD opcodes return BAD_OPCODE in legacy/witness-v0 script contexts regardless of the activation flag.
…(RH-113, RH-114, RH-115) Three related fixes to validation.cpp: RH-114: IdentifyScriptType() and all P2TR output detection sites used a fragile 34-byte + OP_1 heuristic. A 34-byte script starting with OP_1 but missing the required OP_PUSHBYTES_32 opcode (an impostor) was accepted as a DD output. Replaced with IsCanonicalP2TROutput() which calls IsWitnessProgram() for the proper BIP341 check. RH-115: Oracle price validation and collateral ratio checks were wrapped in `if (!ctx.skipOracleValidation)`. This let IBD/catch-up nodes accept mints that caught-up nodes reject, creating a potential chain split. Collateral and oracle checks are now unconditional. RH-113: FindDDOpReturn() returned the first legacy OP_DIGIDOLLAR marker it found and never looked for the modern "DD" pushdata format. A tx with both formats would use the legacy one. Now the modern format wins; legacy is only returned if no modern marker is present.
… reorg (RH-121) Two fixes to src/validation.cpp: DD contextual validation in PreChecks() was running before CheckTxInputs() cached the input coins. This meant DD validation used CoinsTip() directly, missing unconfirmed parents and allowing stale txindex data to resolve DD amounts across reorgs. Moved DD validation to after CheckTxInputs() so it uses the mempool-aware view with all inputs already fetched. MaybeUpdateMempoolForReorg() now re-validates DD transactions when resurrecting them after a reorg. A DD tx valid on the old chain may have depended on a collateral vault or oracle state that no longer exists on the new chain. Without re-validation, invalid DD txs could re-enter the mempool and cause downstream failures. Also moved the non-final tx check to before DD validation so non-final DD transactions are cheaply rejected without triggering oracle lookups.
…rse (RH-29) A coinbase with more than one OP_RETURN OP_ORACLE output was accepted; only the first one was read. An attacker could stuff a second malformed oracle output to confuse indexers or cause ambiguity in bundle selection. Now any coinbase with >1 oracle output is rejected outright. Malformed pushdata during bundle parsing used break (silently produced a short/garbage data buffer) instead of return false (hard rejection). All four pushdata parse paths (direct push, OP_PUSHDATA1, OP_PUSHDATA2, unrecognized byte) now return false immediately on truncation or unknown encoding. Phase One bundle size check is now exact (== 18) rather than a lower-bound (< 18) to reject padded bundles.
…oinbase AddOracleBundleToBlock() mutates the coinbase transaction after GenerateCoinbaseCommitment() has already set the block header merkle root. Any caller that uses the returned block template directly (GBT consumers, mining software that skips IncrementExtraNonce()) would see a stale merkle root that does not match the actual coinbase. Recalculate after injection.
…check (RH-116) getmockoracleprice was callable on any chain before DigiDollar activated, which could return a price that does not correspond to any live oracle state and mislead callers about the system's actual oracle readiness. Now returns a clear error if DigiDollar is not yet active.
…unded (RH-109) BuildTransferTransaction() calculated the required fee and constructed a change output but never checked whether the selected DGB fee inputs actually covered that fee before continuing. A caller with zero or insufficient fee inputs would get a silently malformed transaction with a negative change output. Now returns a descriptive error immediately.
…tection OnMintConnected() and OnRedeemDisconnected() capped totalDDSupply at MAX_DIGIDOLLAR, treating it as a hard supply ceiling. DigiDollar has no global supply cap — MAX_DIGIDOLLAR is a per-output serialization bound. Replaced the duplicate cap+overflow guard in both functions with a shared AddClampedAmount() helper that protects against int64_t overflow without imposing an artificial supply limit.
…acle fixes - rh54: rewritten to test BIP342 OP_SUCCESSx compatibility instead of the old static exclusion approach; verifies DD opcodes fail as BAD_OPCODE in legacy script contexts - redteam T1-06b: updated to expect BAD_OPCODE in legacy execution and correct Tapscript behavior post-activation - skip_oracle: updated expected results now that skipOracleValidation no longer bypasses collateral checks; insufficient-collateral must reject in all sync states - validation: added test for non-canonical OP_1 impostor rejection and legacy DD OP_RETURN format fallback behavior - opcodes, rh11, rh16, rh29, transfer, txbuilder: minor updates for aligned API and message changes from the production fixes above
…and RPC gating tests - wallet_digidollar_transfer_ancestor_reorg.py (new): verifies that a DD transfer whose parent mint is reorganized out is correctly evicted from the mempool and cannot be re-broadcast without a valid chain ancestor - digidollar_activation_boundary.py: aligned with updated activation predicate behavior after oracle/activation cleanup - digidollar_rpc_gating.py: updated for getmockoracleprice activation gate - test_runner.py: registers the new ancestor-reorg test
…tion ledger REPO_MAP_DIGIDOLLAR.md: MAX_DIGIDOLLAR documented as a per-output serialization bound, not a global supply cap. DigiDollar total supply is theoretically unlimited; the only constraint is available collateral and per-block minting rate. AlertThresholds::MAX_DD_SUPPLY clarified as a monitoring alert threshold, not a consensus limit. reports/red_hornet_ledger.md: appended full continuation campaign log covering the 2026-04-29 Red Hornet session (waves, findings, fix assignments, and proposed commit split recorded by vulnerability ID).
Captures all locked V1 decisions (14 total), P0 launch blockers with TDD requirements, 7-phase wave execution model (Pre-Wave + Waves 1-6), commit standards, and full-suite regression gate requirements for each wave. Replaces the prior ad-hoc planning in session memory.
configure.ac: _CLIENT_VERSION_RC 33 → 34 digibyte_wallet.png: version text updated to v9.26.0-rc34 DigiDollar
Recent RC33 follow-up changes intentionally tightened DigiDollar behavior, but several unit tests still expected the old rules. DD script opcodes are now Tapscript-only. Legacy/P2SH script tests should see BAD_OPCODE for raw 0xbb-0xbe, while opcode attack and oracle tests need to exercise the Tapscript path where those opcodes are valid. Mint validation also no longer lets skipOracleValidation bypass collateral and lock-period checks. Lock-height fixtures now validate historical mints at their original block height and calculate the required collateral instead of relying on stale hard-coded amounts. Other fixtures now fund fee/collateral cases correctly, assert transaction builder success before dereferencing results, and pass the required P2SH/WITNESS/TAPROOT flags for Taproot script checks.
…onse fields
Five user-reachable defects surfaced by the Apr 30 – May 2 RC33 security battery.
None affects safety, all undermine audit posture under the commercial-grade bar.
T1-A: mintdigidollar / estimatecollateral docstrings hardcoded mainnet limits
(max $100K) but testnet enforces $10K, regtest $1K. Docstring now
enumerates per-network limits and points callers at runtime checks.
T1-B: calculatecollateralrequirement silently bucketed arbitrary lock_days into
the next consensus tier (and extrapolated past 3650 to 200%). Advisory
diverged from mintdigidollar's strict lock_tier 0-9 enforcement, so a
caller asking "100 days?" got a quote for a position they couldn't mint.
Now strict-matches against the consensus tier set; rejects unknown days
with a list of valid ones.
T2-C: TransferTxBuilder::ValidateTransferParams returned a generic "Invalid
transfer parameters" for every failure mode (bad address, sub-min amount,
missing key, bad fee rate, etc). Now plumbs a specific reason string
via an optional out-parameter so the caller surfaces actionable detail.
T2-D: TransferDigiDollar reported "No spendable DD UTXOs found. Make sure mint
transaction is confirmed." for ALL SelectDDCoins failures, including
cases where the wallet had ample confirmed UTXOs but no input combination
produced ≥minOutput change (dust-change rejection). Now distinguishes:
no UTXOs (original message), insufficient total (with figures), and
dust-change-unavoidable (with min cents) so integrators can act.
T2-A/B: senddigidollar / sendmanydigidollar response always reported
inputs_used: 0 and fee_paid: 0.00000000 because the post-build
mapWallet lookup raced against the wallet's own indexing. Now plumbs
the authoritative fee + vin count out of TxBuilderResult via new
out-parameters on TransferDigiDollar{,Many}, eliminating the race.
T4-A: getoracleprice price_cents truncated sub-cent prices to 0 (integer
division priceMicroUSD / 10000). Now uses round-half-up so the field at
least matches the standard "round to nearest" convention; help text
now points sub-cent callers at price_micro_usd. (At DGB's current
~$0.0037 price, prices below 0.5 cent still round to 0 — that's
arithmetically correct rather than a truncation bug.)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
T3-A (wallet stale state after redeem): redeemdigidollar would happily build a
duplicate redeem tx for an already-closed position. Consensus rejected it
(bad-collateral-release-excessive, ~460 DGB over) but the wallet had no
guard. Now checks pos.is_active before building; rejects with a clear message
and notes that the wallet will reactivate on reorg/drop.
Reproduction (RC33): redeem position → success → redeem same position again
→ "Redemption transaction rejected by mempool: bad-collateral-release-excessive"
Fix verified: returns "Position … has already been redeemed. If a prior redeem
transaction was dropped or reorged out, the wallet will reactivate the
position automatically."
T4-B (consensus_price_micro_usd=0 while reporters agree): ScanOracleDataFromChain
only set res.consensus_price when an on-chain bundle was found within the scan
window. If the most recent bundle was older than the freshness check, the field
read 0 even though 16 oracles were live and agreeing. Added a fallback: derive
the consensus from the assembled oracle_data (median), or fall back to
OracleIntegration::GetCurrentOraclePriceMicroUSD() so getalloracleprices and
getoracleprice stay internally consistent.
Fix verified: getalloracleprices now returns
consensus_price_micro_usd: 3840 (matches the 15 reporting oracles).
T5-A (wallet always consumed ALL DD UTXOs): SelectDDCoins sorted smallest-first
and combined until target_amount + valid change. With (100×30, 7000) and a
5000-cent target, it consumed all 31 inputs. Under nation-state bar this is a
privacy leak (every send reveals total DD balance) and a fee-weaponization
vector (an attacker can permanently inflate the victim's tx size by bombing
dust DD UTXOs).
Replaced with two-pass best-fit-then-largest-first:
1. Best-fit single: smallest single UTXO that covers the target with valid
change. One input, no consume-all behavior.
2. Fallback: largest-first greedy when no single UTXO fits.
Existing dust-change guard handles unsalvageable cases (returns false, then
T2-D's clearer error fires).
Fix verified: send 4900 from (100, 9900) → 1 DD input (the 9900 UTXO),
change 5000, 100-cent UTXO preserved. Old behavior would have used 2 DD
inputs and consumed the 100.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JaredTate
force-pushed
the
feature/digidollar-v1
branch
from
98085b1 to
14fca30
Compare
CI on PR DigiByte-Core#405 reported 4 unit-test failures caused by tests that hardcoded the OLD behavior I deliberately replaced. (The other 5 failures upstream CI reported were verified pre-existing on plain feature/digidollar-v1 HEAD — they are wave1 red-team tests gating fixes that haven't landed yet.) - digidollar_txbuilder_tests/transfer_transaction_invalid_address: expected the generic "Invalid transfer parameters" catch-all that ValidateTransferParams used to return. Now expects the specific "Invalid DD address" reason that the per-failure plumbing produces. - digidollar_rpc_tests/test_lock_blocks_calculation: passed lock_days=1 (which the strict-validation T1-B fix rejects, since 1 day is not a canonical tier) and used 5760 as the expected lock_blocks. Switched to the smallest documented tier (30 days, 172800 blocks). - digidollar_rpc_tests/test_oracle_price_format: asserted price_cents == micro_usd / 10000 (floor). T4-A switched to round-half-up so sub-cent prices don't silently floor to 0; updated the expected formula to (micro_usd + 5000) / 10000. - digidollar_wallet_tests/test_select_dd_coins_exact_match: asserted the smallest-first selector's "first total >= target" output (2 UTXOs, total 35000) for a target where one UTXO is an exact match. T5-A's best-fit-first selector now picks the single 250-DD UTXO; updated the assertions to selected.size() == 1, total == 25000. - digidollar_wallet_tests/test_select_dd_coins_multiple_utxos: asserted ≥3 UTXOs for a 300-DD target where no single UTXO covers. T5-A's largest-first fallback combines 200 + 150 = 350 in 2 UTXOs. After these test updates, this branch's failing-test set is byte-identical to plain upstream/feature/digidollar-v1 HEAD (19 pre-existing wave1 red-team failures, all unrelated to this PR's surface). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Author
|
CI feedback addressed in 7f4d358: Of 9 reported failures, 4 were mine, 5 were pre-existing. Pushed test-only fixes for the 4 caused by this PR:
Verified pre-existing on plain
After this push, this branch's failing-test set on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Eight user-reachable defects (1 verified-by-fix unverified-by-RPC) surfaced by an RC33 security battery re-running the Mar 30 P0 sprint under a commercial-release / nation-state hardening bar. None affect protocol safety; all are reporting, UX, privacy, or wallet-state defects that misalign documentation with implementation or weaponize fragmentation. Three are filing-grade (T1-A, T1-B, T2-D) — they actively contradict the RPC's documented contract.
DD conservation invariant verified intact across 9 destructive transfer attempts and 5 mint/redeem cycles before fixes; smoke-tested working for all 9 fixes after.
Defects fixed
calculatecollateralrequirementaccepts arbitrarylock_days; silently buckets to nearest tier; extrapolates past 3650GetCollateralRatioForLockTime(used elsewhere validly)consensus.collateralRatioskeys in the RPC handler before lookup; clear error listing valid days730from valid setsenddigidollar/sendmanydigidollarresponse intermittently reportsinputs_used: 0,fee_paid: 0.00mapWalletlookup raced wallet indexingTxBuilderResultvia newdgb_fee_out/inputs_used_outparameters onTransferDigiDollar{,Many}(default-null, non-breaking)BuildTransferTransactionreturns"Invalid transfer parameters"for every failure modeValidateTransferParamsonly returnedboolstd::string* reasonout-param; specific cause per failureTransferDigiDollarreturns"No spendable DD UTXOs found. Make sure mint transaction is confirmed."for three distinct failure modes (no UTXOs, insufficient total, dust-change-unavoidable)SelectDDCoinsfailure: distinguishes empty/insufficient/dustredeemdigidollarbuilds duplicate redeem tx for already-closed positions; consensus catches withbad-collateral-release-excessive(~460 DGB over)pos.is_activeis_active; clean error noting wallet auto-reactivates on reorg/dropgetoracleprice price_centstruncates sub-cent to 0priceMicroUSD / 10000(x+5000)/10000; help text points sub-cent callers atprice_micro_usdgetalloracleprices consensus_price_micro_usd: 0while reporters agreeoracle_data, thenOracleIntegration::GetCurrentOraclePriceMicroUSD()SelectDDCoinssmallest-first one-pass with consume-all-on-dust-changeVerification
Smoke-tested against testnet23 with a freshly-built RC34-dirty binary, oracle wallet loaded, 16 reporting oracles live:
help mintdigidollarshows per-network limits ✓calculatecollateralrequirement 10000 100→Invalid lock period: 100 days. Valid periods (in days): 30, 90, 180, 365, 730, 1095, 1825, 2555, 3650.✓senddigidollarresponse showsfee_paid: 0.14210, inputs_used: 3✓Position … has already been redeemed.instead ofbad-collateral-release-excessive✓getalloraclepricesreturnsconsensus_price_micro_usd: 3840(matches all 15 reporting oracles) ✓(100, 9900)uses 1 DD input via best-fit single (was: 2 DD inputs via smallest-first) ✓Out of scope (code-level audit items)
This RPC battery cannot reach the following — they remain open for source-level review before any commercial release:
__int128actually wired invalidation.cpp:462,dca.cpp:69/96,health.cpp:912,915,err.cpp:101validation.cpp:2745-2872Test plan
upstream/feature/digidollar-v1HEAD (98085b1a22)nullptr)🤖 Generated with Claude Code