txauthor+wallet: validate transaction output and fee arithmetic - #1348
Draft
Abdulkbk wants to merge 8 commits into
Draft
txauthor+wallet: validate transaction output and fee arithmetic#1348Abdulkbk wants to merge 8 commits into
Abdulkbk wants to merge 8 commits into
Conversation
Transaction construction currently does every amount computation with raw `btcutil.Amount` arithmetic: output values are summed without bounds, the rate-by-size product is formed unchecked, and a fee that lands outside the representable range is clamped to `MaxSatoshi` rather than reported. Add the checked primitives that construction will be built on. `CheckOutputs` validates an output set for nil elements, negative values, individually over-maximum values, and an aggregate above `MaxSatoshi`, returning the checked total so callers reuse the sum instead of re-deriving it. A nil or empty set stays valid and totals zero, since a sweep authors with no non-change outputs. `CheckedFeeForSerializeSize` rejects a fee rate at or below zero before it is ever multiplied, reports a product overflow, and requires the rounded fee to remain within `0..MaxSatoshi` instead of clamping. The one-kilo-virtual-byte floor is preserved, and is unconditionally positive now that the rate is. Both are backed by an internal checked add and its subtraction counterpart, which report an overflow rather than wrapping. No caller uses these yet.
Route transaction construction through the checked primitives. The output set is validated and its total taken from `CheckOutputs`, the fee rate is rejected at or below zero by `CheckedFeeForSerializeSize`, and both happen before the first `fetchInputs` call, so a malformed request never reaches a callback. The per-iteration fee and change arithmetic depends on the inputs that were selected, so it follows selection, but it still completes before the change script is allocated. The target the inputs must cover is now formed once per iteration and the sufficiency comparison tests that same value, rather than re-adding the target and the fee. The change is derived by subtracting the required fee from the sufficiency remainder instead of re-deriving it from the input total, and both subtractions are checked. `SumOutputValues` is left in place for external callers but marked deprecated: it ignores a nil element, admits a negative or over-maximum value, and wraps silently on overflow. The table case that authored at a zero fee rate now uses a positive rate whose maximum required fee consumes the input exactly, which is what it was actually testing: that a zero change output is not appended.
The root module pinned the tagged `wallet/txauthor v1.4.0`, so the checked output and fee arithmetic added there would not reach TxCreator. Add the local replace directive alongside the ones `wtxmgr` and `wallet/txsizes` already carry, and retidy.
The authoring boundary now reports the checked-arithmetic violations txauthor detects. Translate them once, where both public wrappers pass through, so callers of `CreateTransaction` and `FundPsbt` match on the wallet's own sentinels rather than a nested module's. Classes the wallet already names keep their identity: an output value violation reports the same `txrules` error the intent-level check reports, and a non-positive fee rate reports `ErrMissingFeeRate`. Only the classes the wallet had no name for gain one. The originating error stays in the chain, and anything unrecognised, an `InputSourceError` above all, is returned untouched so "cannot fund this" stays distinguishable from "will not author this". The remaining use of the unchecked `SumOutputValues` is replaced by `CheckOutputs`, leaving no maintained path on the unsafe sum.
Review of btcsuite#1348 found that three of the four sentinels the translation layer introduced had no reachable producer, and that composing a wallet sentinel with the txauthor error it wraps printed the same clause twice. Stop naming the conditions no caller can observe. A nil output cannot occur: authorTransaction takes its outputs by value, so a nil element is unrepresentable. The checked add and subtract guards cannot fire either, since CheckOutputs bounds the target and the sufficiency checks bound the rest. Both now fall through and return the txauthor error unchanged, which drops ErrNilTxOutput and ErrAmountOverflow. ErrFeeOutOfRange stays. DefaultMaxFeeRate keeps it out of reach at its default value, but that bound is a mutable package variable meant to become configurable, and raised far enough an ordinary multi-input transaction prices a fee above MaxSatoshi. Its docstring now says so. A non-positive rate reported ErrMissingFeeRate, which reads wrong for a rate the caller supplied: a rate of -1000 rendered as "missing fee rate". It gets ErrInvalidFeeRate instead, leaving ErrMissingFeeRate to the intent check. Translation now carries the wallet identity on a small error type rather than a second %w verb. Its message is the originating error's alone, while errors.Is resolves the wallet sentinel through Is and the txauthor error through Unwrap. This mirrors AmbiguousTxCommitError in wallet/internal/db/runtime. A negative output value used to render "transaction output amount is negative: transaction output amount is negative: index 1 has -1"; it now renders the second half only, and a test pins that.
A negative size used to reach the multiplication and was caught only because the resulting fee came out negative. If the product wrapped, the quotient could land back inside 0..MaxSatoshi and yield a plausible but wrong fee, which the final bound alone cannot rule out. Reject it alongside the non-positive rate, before either operand is used. Both operands are non-negative from there on, so the fee can no longer come out negative and that half of the final bound goes with it. Also from review: name countInputTypes' results, since four values of the same type are otherwise transposable at the call site with nothing but a slightly wrong fee to show for it; record at the authoring loop that the checked add and subtract calls cannot currently fail, so a reader does not go hunting for the input that trips them; and note on txrules.FeeForSerializeSize that it now shares its rounding rule with the checked variant. Drops TestCheckOutputsSumOverflow, which never called CheckOutputs and only repeated TestAddAmounts' overflow case under a name that implied otherwise.
translateAuthorError is documented as the single translation point for both public wrappers, but every test drove authorTransaction directly or called the translator in isolation, so nothing pinned the FundPsbt half. Fund a packet whose two outputs are each below the maximum but together exceed it. That is the one checked-arithmetic condition a public wrapper can reach: validateTxIntent bounds each output on its own and never sums them. Assert both identities resolve and that a refused funding leaves the caller's packet as it arrived. Only the lookups source preparation actually performs are registered. The change script is never requested, since authoring rejects the output set first, and AssertExpectations runs on cleanup.
Nothing between the deprecated wallet interface and txauthor bounds the fee rate: txToOutputs backs CreateSimpleTx, SendOutputs and SendOutputsWithInput, and hands whatever the caller passed straight to NewUnsignedTransaction. The checked fee helper now refuses a rate at or below zero, so a zero rate that used to author a zero-fee transaction returns an error instead. The in-tree legacy RPC always supplies txrules.DefaultRelayFeePerKb and is unaffected, but external callers of the deprecated interface set their own rate. Record the change against a funded fixture, so the refusal cannot be mistaken for a shortfall, and show a positive rate still authors against the same coins.
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.
Task 357 — validate transaction output and fee arithmetic.
Transaction construction did every amount computation with raw
btcutil.Amountarithmetic:
SumOutputValueswrapped silently, the target-plus-fee addition wasunchecked and formed twice per iteration, the sufficiency and change
subtractions were unchecked, and
txrules.FeeForSerializeSizeaccepted anon-positive rate and clamped an unrepresentable fee to
MaxSatoshiinstead ofreporting it.
wallet/txauthornow owns the checked primitives:CheckOutputs— rejects a nil element, a negative or over-MaxSatoshivalue,and an aggregate above
MaxSatoshi, returning the checked total for reuse. Anil set stays valid and totals zero, so a change-only sweep still authors.
CheckedFeeForSerializeSize— rejects a rate at or below zero and a negativesize before multiplying, reports a product overflow, and requires the rounded
fee to land in
0..MaxSatoshi.NewUnsignedTransactioncarries their results through construction: the outputset and the fee rate are validated before the first
fetchInputscall, thetarget is formed once and reused for the sufficiency test, and change is
derived from the sufficiency remainder rather than re-derived from the input
total.
SumOutputValuesstays exported for external callers but is deprecatedand no longer used.
TxCreator translates the result once, at the shared authoring boundary, so
CreateTransactionandFundPsbtboth report the wallet's own sentinels.Output value violations reuse the
txruleserrorsvalidateTxIntentalreadyreports; only
ErrOutputTotalExceedsMax,ErrFeeOutOfRangeandErrInvalidFeeRateare new. Conditions no caller can observe are not named atthe wallet level and pass through unchanged, alongside
InputSourceError.Target-plus-fee is checked for overflow only, never against
MaxSatoshi: asingle output of exactly
MaxSatoshimust still reach the input source and failfor want of coins, which
txcreator output boundariesasserts.Notes for review
CreateSimpleTx/SendOutputs/SendOutputsWithInput/FundPsbtDeprecatedandtxauthorbounded the fee rate, so a zero rate usedto author a zero-fee transaction and now returns an error. The in-tree legacy
RPC always passes
txrules.DefaultRelayFeePerKband is unaffected, butexternal callers set their own rate. Pinned by
TestTxToOutputsRejectsNonPositiveFeeRate.validateTxIntentfront-runs theper-output bounds and the fee-rate bounds, so the aggregate output total is
the only class a public wrapper reaches today; it has no intent-level
counterpart.
ErrFeeOutOfRangeis reachable onceDefaultMaxFeeRateisraised — it is a mutable package variable meant to become configurable, and at
9e15 sat/kvb any transaction over ~234 vbytes prices a fee above
MaxSatoshi.The rest is defence for the internal boundary.
btcunitoverflow, separate defect.CalcSatPerKVBytecomputesfee * kiloin int64, soNewSatPerKVBytesilently wraps to a negative rateabove ~9.22e15 sat/kvb (
NewSatPerKVByte(3e16).Val()returns-6893488147419104). Outside this task's scope; needs its own roadmap task.and change subtraction to be checked, so its subtraction counterpart ships
with it. Both stay unexported — every call site is bounded by the checks
around it and cannot currently fail, which is recorded at the loop.
replacefor the localtxauthor, matching the oneswtxmgrandwallet/txsizesalready carry. Without it none of this reachesTxCreator. It is load-bearing, as
wtxmgr's already is on this branch, andwants converting to a tagged release before
sql-walletlands.countInputTypesis extracted so the added error handling does not pushNewUnsignedTransactionpast thefunlenlimit.Testing
Nested-module and wallet tests cover nil, negative, individually over-maximum,
checked-sum overflow, aggregate-over-maximum, zero and negative direct fee
rates, negative size, fee-product overflow, a rounded fee above
MaxSatoshi,target-plus-fee overflow, the smallest valid positive rate, and the exact
maximum boundaries; every failure case asserts both callbacks went unused.
Translation is covered per class, through
FundPsbt, and for messagestability.
Full
go test ./wallet/..., thetxauthormodule tests, and the complete itestsuite pass. golangci-lint reports 0 issues on the root module.