Optimize Firo Spark mint tx generation and fix fee < vSize error#1295
Open
reubenyap wants to merge 1 commit intocypherstack:stagingfrom
Open
Optimize Firo Spark mint tx generation and fix fee < vSize error#1295reubenyap wants to merge 1 commit intocypherstack:stagingfrom
reubenyap wants to merge 1 commit intocypherstack:stagingfrom
Conversation
81a6f32 to
cf8525d
Compare
## Performance
Pre-compute signing keys, addresses, and wallet-owned address set before
the main loop. The original code called getRootHDNode() (expensive
mnemonic-to-seed derivation), a per-UTXO DB lookup for derivationPath,
and a per-output DB lookup for walletOwns, all inside nested loops. For
N inputs across M fee-estimation iterations, this was O(N*M) redundant
work. Also caches getCurrentReceivingSparkAddress() and
getCurrentChangeAddress() since neither can change within the function.
## Fee-less-than-vSize bug fix
The dummy transaction built for fee estimation is signed with real keys
over different data than the final real transaction. bitcoindart's ECDSA
signing (RFC 6979, low-S enforced, low-R not enforced) produces DER
signatures whose length varies by up to ~4 bytes per input depending on
the random r value's high bit. For P2PKH inputs (Firo's default), this
variance counts at full weight, so with 10+ inputs the dummy vs real
vSize can differ by more than the original 10-byte buffer, tripping the
nFeeRet < data.vSize check.
Scale the buffer linearly with input count:
final nBytesBuffer = 10 + 4 * setCoins.length;
This matches what Firo's own C++ wallet does in DummySignatureCreator
(src/wallet/wallet.h:1436): "Helper for producing a bunch of max-sized
low-S signatures (eg 72 bytes)". Extra fee cost: ~4 sats per input at
1 sat/byte.
## Subsidiary fixes
- mintedValue <= BigInt.zero (was == BigInt.zero): catches negative
mintedValue when a UTXO group's total is less than the computed fee
and subtractFeeFromAmount=false. Matches the C++ reference
!MoneyRange(mintedValue) || mintedValue == 0.
- Clarified the fee < vSize error message: the check is effectively a
min-relay-fee check (feeRate < 1 sat/byte), not a fee/size mismatch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cf8525d to
51d4357
Compare
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
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
Two changes to
_createSparkMintTransactionsinspark_interface.dart:fee is less than vSizeerror on many-input mints (e.g., anonymize-all).Performance optimization
Hot-loop operations moved out of the nested loops:
getRootHDNode()mainDB.getAddress(walletId, ...)mainDB...walletIdEqualTo(...).valueEqualTo(...).findFirst()forwalletOwnsSet<String>lookupgetCurrentReceivingSparkAddress()/getCurrentChangeAddress()For an N-input, M-fee-iteration mint this eliminates ~N×M expensive async calls.
Fee bug fix
Root cause. The dummy tx built for fee estimation is signed with the real keys over different data than the final tx.
bitcoindart's ECDSA path (bip32-dart/ecurve.sign) uses RFC 6979 and enforces low-S but not low-R, sor's high bit is random — DER signatures vary by up to ~4 bytes per input. For Firo P2PKH (bip44) inputs this counts at full weight; with ~10+ inputs the dummy-vs-real vSize delta exceeds the original 10-byte buffer and tripsnFeeRet < data.vSize.Fix. Scale the buffer with input count:
Extra cost: ~4 sats per input at 1 sat/byte. Matches what Firo's own C++ wallet does — see
DummySignatureCreatorinsrc/wallet/wallet.h:1436("max-sized low-S signatures (eg 72 bytes)" → fee estimation is always an upper bound).Subsidiary fixes
mintedValue <= BigInt.zero(was== BigInt.zero): catches negativemintedValuewhen a UTXO group's total is less than the computed fee andsubtractFeeFromAmount=false. Matches the C++ reference's!MoneyRange(mintedValue) || mintedValue == 0.fee < vSizeerror message and added a comment: the check is a disguised min-relay-fee check (feeRate < 1 sat/byte), not a fee/size accounting error.Why not retry?
An earlier iteration used a retry loop that restored UTXOs, outputs, and fee state when the check fired. Investigation confirmed the size variance has a deterministic upper bound (ECDSA DER max per input), so a scaled buffer is cleaner — no state to restore, no wasted mint-proof generation, no risk of loop bugs.
Test plan
fee is less than vSize.🤖 Generated with Claude Code