Feat/fix fee calculations#312
Conversation
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All three blocking defects identified by both reviewers are real and confirmed in lib/transaction/transaction.js. The new _estimateFee produces NaN in the default surcharge branch, returns fractional duffs for non-1000-multiple feePerKb, and the new _estimateSize hard-codes P2PKH script size for every unsigned input (regressing multisig/P2SH fee estimation). Additional suggestions and a JSDoc bug are also valid.
🔴 3 blocking | 🟡 4 suggestion(s) | 💬 2 nitpick(s)
Out-of-scope follow-up suggestions (4)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
Transaction.prototype.isFullySignedsemantically means "isPartiallySigned" — Pre-existing quirk:isFullySigneduses_.some, returning true if at least one input is signed. The new_estimateSizeis gated on this and is the proximate cause of the multi-input under-estimation flagged above, but renaming or changing the semantics has broad API impact and is out of scope for this PR. The in-PR fix should iterate per input instead of relying on the global flag.- Follow-up: Open a separate issue to either rename to
isPartiallySignedor switch to_.everyafter a deprecation cycle.
- Follow-up: Open a separate issue to either rename to
- New
minimumFee()is missing from TypeScript declarations —typings/transaction/Transaction.d.tsexposesfeePerKb()then jumps straight tochange(); the newminimumFeesetter is not declared. TS consumers will not be able to call it without augmentation. This is an API-surface follow-up, separate from the fee-calculation defects above.- Follow-up: Follow-up PR to add
minimumFee(amount: number): Transactionto typings and cover withnpm run test:types.
- Follow-up: Follow-up PR to add
- Pre-existing GovObject/Proposal
Invalid Timespanbaseline failures — Per the project review skill,npm teston master can fail in GovObject/Proposal tests unrelated to this PR. CI should distinguish baseline failures from PR-introduced regressions.- Follow-up: Track baseline GovObject test repair in a dedicated issue.
- Karma client mocha timeout raised to 60s —
karma.conf.jsaddsclient.mocha.timeout = 60000. This is unrelated to fee logic. If needed because the new 30-input fee tests are slow in the browser, the PR description should call that out; otherwise it belongs in a separate change.- Follow-up: If unrelated to the fee tests, split into a separate PR with justification.
| if (amountAvailable > fee + minimumFee) { | ||
| fee += (minimumFee || Transaction.MINIMUM_FEE); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: fee + minimumFee evaluates to NaN in the default path — surcharge branch never fires
Transaction._estimateFee receives minimumFee as the raw this._minimumFee (line 1066), which is undefined for any transaction that has not explicitly called .minimumFee(...). At line 1085, amountAvailable > fee + minimumFee becomes amountAvailable > NaN, which is always false, so the fee += (minimumFee || MINIMUM_FEE) surcharge is silently skipped in the default code path. The branch only works for callers that explicitly call the new setter, producing different fees depending on whether the setter was used. Apply the (minimumFee || Transaction.MINIMUM_FEE) fallback consistently to both the comparison and the assignment (or hoist it into a local).
| if (amountAvailable > fee + minimumFee) { | |
| fee += (minimumFee || Transaction.MINIMUM_FEE); | |
| } | |
| Transaction._estimateFee = function (size, amountAvailable, feePerKb, minimumFee) { | |
| var minFee = minimumFee || Transaction.MINIMUM_FEE; | |
| var feePerByte = (feePerKb || Transaction.FEE_PER_KB) / 1000; | |
| var fee = Math.ceil(size * feePerByte); | |
| if (fee < minFee) { | |
| return minFee; | |
| } | |
| if (amountAvailable > fee + minFee) { | |
| fee += minFee; | |
| } | |
| return fee; | |
| }; |
source: ['claude', 'codex']
| var feePerByte = (feePerKb || Transaction.FEE_PER_KB) / 1000; | ||
| var fee = size * feePerByte; | ||
| if(fee<(minimumFee || Transaction.MINIMUM_FEE)){ | ||
| return (minimumFee || Transaction.MINIMUM_FEE) | ||
| } | ||
| if (amountAvailable > fee + minimumFee) { | ||
| fee += (minimumFee || Transaction.MINIMUM_FEE); | ||
| } | ||
| return Math.ceil(size / 1000) * (feePerKb || Transaction.FEE_PER_KB); | ||
| return fee; |
There was a problem hiding this comment.
🔴 Blocking: Per-byte formula returns fractional duffs and will throw downstream
feePerByte = (feePerKb || FEE_PER_KB) / 1000; fee = size * feePerByte is fractional whenever feePerKb is not a multiple of 1000 (e.g., feePerKb(1234) or feePerKb(1500)). The result is passed to _updateChangeOutput (line 1015) which builds new Output({ satoshis: changeAmount = available - fee }). The Output setter calls $.checkArgument(JSUtil.isNaturalNumber(num)) (lib/transaction/output.js) and throws 'Output satoshis is not a natural number'. The previous algorithm always returned an integer via Math.ceil(size/1000) * feePerKb. Wrap with Math.ceil (or Math.round) to preserve the integer-satoshi contract.
| var feePerByte = (feePerKb || Transaction.FEE_PER_KB) / 1000; | |
| var fee = size * feePerByte; | |
| if(fee<(minimumFee || Transaction.MINIMUM_FEE)){ | |
| return (minimumFee || Transaction.MINIMUM_FEE) | |
| } | |
| if (amountAvailable > fee + minimumFee) { | |
| fee += (minimumFee || Transaction.MINIMUM_FEE); | |
| } | |
| return Math.ceil(size / 1000) * (feePerKb || Transaction.FEE_PER_KB); | |
| return fee; | |
| Transaction._estimateFee = function (size, amountAvailable, feePerKb, minimumFee) { | |
| var minFee = minimumFee || Transaction.MINIMUM_FEE; | |
| var feePerByte = (feePerKb || Transaction.FEE_PER_KB) / 1000; | |
| var fee = Math.ceil(size * feePerByte); | |
| if (fee < minFee) { | |
| return minFee; | |
| } | |
| if (amountAvailable > fee + minFee) { | |
| fee += minFee; | |
| } | |
| return fee; | |
| }; |
source: ['claude', 'codex']
| Transaction.prototype._estimateSize = function () { | ||
| var result = Transaction.MAXIMUM_EXTRA_SIZE; | ||
| _.each(this.inputs, function (input) { | ||
| result += input._estimateSize(); | ||
| }); | ||
| _.each(this.outputs, function (output) { | ||
| result += output.script.toBuffer().length + 9; | ||
| }); | ||
| return result; | ||
| // string format HEX -> 2 symbols == 1 byte | ||
| var serializedSize = this.toString().length / 2; | ||
| if ( !this.isFullySigned() ) { | ||
| // every signed input contains some additional bytes | ||
| serializedSize += this.inputs.length * PublicKeyHashInput.SCRIPT_MAX_SIZE; | ||
| } | ||
| if ( this._missingChange() ) { | ||
| serializedSize += Transaction.CHANGE_OUTPUT_MAX_SIZE; | ||
| } | ||
|
|
||
| return serializedSize; | ||
| }; |
There was a problem hiding this comment.
🔴 Blocking: Hard-coded PublicKeyHashInput.SCRIPT_MAX_SIZE undersizes non-P2PKH unsigned inputs
_estimateSize now adds this.inputs.length * PublicKeyHashInput.SCRIPT_MAX_SIZE (107 bytes) for every unsigned input. The previous implementation summed polymorphic input._estimateSize(). PublicKeyInput.SCRIPT_MAX_SIZE is 73 (lib/transaction/input/publickey.js:103), MultiSigInput._estimateSize at lib/transaction/input/multisig.js:252 and MultiSigScriptHashInput._estimateSize at lib/transaction/input/multisigscripthash.js:206 return larger threshold-dependent values (a 2-of-3 P2SH input is ~256+ bytes). For unsigned multisig/P2SH transactions, fees are now significantly under-estimated, which produces under-funded transactions after signing. Restore the per-input polymorphic dispatch.
| Transaction.prototype._estimateSize = function () { | |
| var result = Transaction.MAXIMUM_EXTRA_SIZE; | |
| _.each(this.inputs, function (input) { | |
| result += input._estimateSize(); | |
| }); | |
| _.each(this.outputs, function (output) { | |
| result += output.script.toBuffer().length + 9; | |
| }); | |
| return result; | |
| // string format HEX -> 2 symbols == 1 byte | |
| var serializedSize = this.toString().length / 2; | |
| if ( !this.isFullySigned() ) { | |
| // every signed input contains some additional bytes | |
| serializedSize += this.inputs.length * PublicKeyHashInput.SCRIPT_MAX_SIZE; | |
| } | |
| if ( this._missingChange() ) { | |
| serializedSize += Transaction.CHANGE_OUTPUT_MAX_SIZE; | |
| } | |
| return serializedSize; | |
| }; | |
| Transaction.prototype._estimateSize = function () { | |
| // string format HEX -> 2 symbols == 1 byte | |
| var serializedSize = this.toString().length / 2; | |
| if (!this.isFullySigned()) { | |
| _.each(this.inputs, function (input) { | |
| serializedSize += input._estimateSize(); | |
| }); | |
| } | |
| if (this._missingChange()) { | |
| serializedSize += Transaction.CHANGE_OUTPUT_MAX_SIZE; | |
| } | |
| return serializedSize; | |
| }; |
source: ['claude', 'codex']
| Transaction.prototype._estimateSize = function () { | ||
| var result = Transaction.MAXIMUM_EXTRA_SIZE; | ||
| _.each(this.inputs, function (input) { | ||
| result += input._estimateSize(); | ||
| }); | ||
| _.each(this.outputs, function (output) { | ||
| result += output.script.toBuffer().length + 9; | ||
| }); | ||
| return result; | ||
| // string format HEX -> 2 symbols == 1 byte | ||
| var serializedSize = this.toString().length / 2; | ||
| if ( !this.isFullySigned() ) { | ||
| // every signed input contains some additional bytes | ||
| serializedSize += this.inputs.length * PublicKeyHashInput.SCRIPT_MAX_SIZE; | ||
| } | ||
| if ( this._missingChange() ) { | ||
| serializedSize += Transaction.CHANGE_OUTPUT_MAX_SIZE; | ||
| } | ||
|
|
||
| return serializedSize; | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: isFullySigned() is semantically partial-signed; gate misfires for mixed inputs
Transaction.prototype.isFullySigned (line 1286) uses _.some(...), returning true as soon as ANY input is signed. The new _estimateSize gates the per-input script-size buffer on !isFullySigned(). A transaction with one signed input and N-1 unsigned inputs will skip the buffer entirely, under-estimating fees. Iterate per input and add the buffer only for inputs where input.isFullySigned() is false (this naturally also fixes the polymorphic-size issue above). The underlying isFullySigned semantics quirk is pre-existing and out of scope to rename here.
source: ['claude']
| Transaction.prototype._estimateSize = function () { | ||
| var result = Transaction.MAXIMUM_EXTRA_SIZE; | ||
| _.each(this.inputs, function (input) { | ||
| result += input._estimateSize(); | ||
| }); | ||
| _.each(this.outputs, function (output) { | ||
| result += output.script.toBuffer().length + 9; | ||
| }); | ||
| return result; | ||
| // string format HEX -> 2 symbols == 1 byte | ||
| var serializedSize = this.toString().length / 2; | ||
| if ( !this.isFullySigned() ) { | ||
| // every signed input contains some additional bytes | ||
| serializedSize += this.inputs.length * PublicKeyHashInput.SCRIPT_MAX_SIZE; | ||
| } | ||
| if ( this._missingChange() ) { | ||
| serializedSize += Transaction.CHANGE_OUTPUT_MAX_SIZE; | ||
| } | ||
|
|
||
| return serializedSize; | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: _missingChange() doesn't reflect actual-output state inside _updateChangeOutput
_missingChange() returns !this._changeScript. Inside _updateChangeOutput (line 1005), the change output is removed via _removeOutput(this._changeIndex) at line 1011 while _changeScript is still set, then getFee() is called. _estimateSize reads _missingChange() === false and skips CHANGE_OUTPUT_MAX_SIZE, even though toString() does not currently contain the change output. The estimate is short by ~62 bytes. The gate likely wants to check whether the change output is currently in this.outputs (e.g., _.isUndefined(this._changeIndex)), not whether a change script has been configured.
source: ['claude']
| Transaction.prototype._estimateSize = function () { | ||
| var result = Transaction.MAXIMUM_EXTRA_SIZE; | ||
| _.each(this.inputs, function (input) { | ||
| result += input._estimateSize(); | ||
| }); | ||
| _.each(this.outputs, function (output) { | ||
| result += output.script.toBuffer().length + 9; | ||
| }); | ||
| return result; | ||
| // string format HEX -> 2 symbols == 1 byte | ||
| var serializedSize = this.toString().length / 2; | ||
| if ( !this.isFullySigned() ) { | ||
| // every signed input contains some additional bytes | ||
| serializedSize += this.inputs.length * PublicKeyHashInput.SCRIPT_MAX_SIZE; | ||
| } | ||
| if ( this._missingChange() ) { | ||
| serializedSize += Transaction.CHANGE_OUTPUT_MAX_SIZE; | ||
| } | ||
|
|
||
| return serializedSize; | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: _estimateSize triggers full toString() on every mutator, creating O(N²) work
Each from/to/fee/feePerKb/change/minimumFee call invokes _updateChangeOutput → getFee → _estimateFee → _estimateSize, which now fully serializes the transaction via this.toString(). Building a tx with N inputs and M outputs incrementally becomes O((N+M)²). The previous implementation summed per-component max sizes in O(N+M). The 30-input tests added here pass, but larger downstream callers will feel this. Prefer summing component sizes (matching the polymorphic-input fix above) or cache toString() per dirty-state.
source: ['claude']
| /** | ||
| * Manually set the minimum fee's for this transaction. Beware that this resets all the signatures | ||
| * for inputs (in further versions, SIGHASH_SINGLE or SIGHASH_NONE signatures will not | ||
| * be reset). | ||
| * | ||
| * @param {number} amount satoshis per KB to be sent | ||
| * @return {Transaction} this, for chaining | ||
| */ | ||
| Transaction.prototype.minimumFee = function (amount) { | ||
| $.checkArgument(_.isNumber(amount), 'amount must be a number'); | ||
| this._minimumFee = amount; | ||
| this._updateChangeOutput(); | ||
| return this; | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: New public Transaction.prototype.minimumFee setter has no direct test coverage
The PR adds a new public setter minimumFee(amount) but no new test exercises it — all rely on the default Transaction.MINIMUM_FEE = 255. Combined with the NaN bug in _estimateFee flagged above, this means the setter's two intended branches (floor and surcharge) are completely unverified. Add at least one test that calls .minimumFee(custom) and asserts both (a) the floor (fee falls below custom → returns custom) and (b) the surcharge branch (fee ≥ custom with sufficient available).
source: ['claude']
| /** | ||
| * Manually set the minimum fee's for this transaction. Beware that this resets all the signatures | ||
| * for inputs (in further versions, SIGHASH_SINGLE or SIGHASH_NONE signatures will not | ||
| * be reset). | ||
| * | ||
| * @param {number} amount satoshis per KB to be sent | ||
| * @return {Transaction} this, for chaining | ||
| */ | ||
| Transaction.prototype.minimumFee = function (amount) { | ||
| $.checkArgument(_.isNumber(amount), 'amount must be a number'); | ||
| this._minimumFee = amount; | ||
| this._updateChangeOutput(); |
There was a problem hiding this comment.
💬 Nitpick: JSDoc for minimumFee is wrong (copy-pasted from feePerKb)
The JSDoc says @param {number} amount satoshis per KB to be sent and the prose says Manually set the minimum fee's (stray apostrophe). The minimum fee is a flat duff amount, not satoshis-per-KB.
| /** | |
| * Manually set the minimum fee's for this transaction. Beware that this resets all the signatures | |
| * for inputs (in further versions, SIGHASH_SINGLE or SIGHASH_NONE signatures will not | |
| * be reset). | |
| * | |
| * @param {number} amount satoshis per KB to be sent | |
| * @return {Transaction} this, for chaining | |
| */ | |
| Transaction.prototype.minimumFee = function (amount) { | |
| $.checkArgument(_.isNumber(amount), 'amount must be a number'); | |
| this._minimumFee = amount; | |
| this._updateChangeOutput(); | |
| /** | |
| * Manually set the minimum fee for this transaction (in duffs). Beware that this | |
| * resets all the signatures for inputs (in further versions, SIGHASH_SINGLE or | |
| * SIGHASH_NONE signatures will not be reset). | |
| * | |
| * @param {number} amount minimum fee in duffs | |
| * @return {Transaction} this, for chaining | |
| */ | |
| Transaction.prototype.minimumFee = function (amount) { | |
| $.checkArgument(_.isNumber(amount), 'amount must be a number'); | |
| this._minimumFee = amount; | |
| this._updateChangeOutput(); | |
| return this; | |
| }; |
source: ['claude']
| Transaction._estimateFee = function (size, amountAvailable, feePerKb, minimumFee) { | ||
| var feePerByte = (feePerKb || Transaction.FEE_PER_KB) / 1000; | ||
| var fee = size * feePerByte; | ||
| if(fee<(minimumFee || Transaction.MINIMUM_FEE)){ | ||
| return (minimumFee || Transaction.MINIMUM_FEE) | ||
| } | ||
| if (amountAvailable > fee + minimumFee) { | ||
| fee += (minimumFee || Transaction.MINIMUM_FEE); | ||
| } | ||
| return Math.ceil(size / 1000) * (feePerKb || Transaction.FEE_PER_KB); | ||
| return fee; | ||
| }; | ||
|
|
||
| Transaction.prototype._estimateSize = function () { | ||
| var result = Transaction.MAXIMUM_EXTRA_SIZE; | ||
| _.each(this.inputs, function (input) { | ||
| result += input._estimateSize(); | ||
| }); | ||
| _.each(this.outputs, function (output) { | ||
| result += output.script.toBuffer().length + 9; | ||
| }); | ||
| return result; | ||
| // string format HEX -> 2 symbols == 1 byte | ||
| var serializedSize = this.toString().length / 2; | ||
| if ( !this.isFullySigned() ) { | ||
| // every signed input contains some additional bytes | ||
| serializedSize += this.inputs.length * PublicKeyHashInput.SCRIPT_MAX_SIZE; | ||
| } | ||
| if ( this._missingChange() ) { | ||
| serializedSize += Transaction.CHANGE_OUTPUT_MAX_SIZE; | ||
| } | ||
|
|
||
| return serializedSize; | ||
| }; |
There was a problem hiding this comment.
💬 Nitpick: Inconsistent formatting in new _estimateFee / _estimateSize
if(fee<(minimumFee || Transaction.MINIMUM_FEE)){ lacks spaces around the operator and after if/{; the early-return line lacks a trailing semicolon. _estimateSize uses if ( ... ) with extra inner spaces. Both diverge from the surrounding style used throughout the file. Run npm run lint and apply fixes.
source: ['claude']
2 participants
Issue being fixed or feature implemented
In this PR was removed incorrect calculations for TX Fee.
This is important because the current
dashcore-librelease can't properly auto-calculate fees for transactions with many inputs.What was done?
MINIMUM_FEEconstant which contains minimum fee size in duffs (255)MINIMUM_FEE_estimateSizealghoritmAt this moment alghoritm for fee calculation works randomly, and with long steps. Updated code allows to calculate fee based on serialized transaction length
How Has This Been Tested?
Breaking Changes
Checklist: