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']
|
⛔ Final review complete — 6 blocking finding(s) (commit 3376576) · triage: critical · Phase 2 only (queue backlog) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Verified all six Phase-2 findings against head 3376576 and reproduced the reported runtime failures and missing TypeScript declarations. The fee changes can underfund transactions, reject valid integer fee rates, break serialization with signature checks disabled, and apply the minimum inconsistently. All 1,002 focused transaction tests and the existing type tests pass, but they do not catch these regressions.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — Changing transaction size estimation and automatic fee calculation directly affects funds spent and transaction acceptance, with potential underpayment or overpayment across input counts and change-output handling. - Phase 1 reviewers: not run (skipped for throughput: 56 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer
🔴 6 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `lib/transaction/transaction.js`:
- [BLOCKING] lib/transaction/transaction.js:1098-1100: Include the pending change output before allocating change
_missingChange() checks whether a change address is configured, not whether the transaction currently contains its change output. During _updateChangeOutput(), the change script is already configured and the previous change output has been removed, so this condition skips budgeting for the forthcoming output. A reproduced two-input P2PKH transaction pays 340 duffs but serializes to 373 bytes after signing, below the default 1,000 duffs/kB rate. Its subsequent getFee() call returns 373 rather than the actual 340, so the new getFee() assertions do not detect the underpayment. Include the prospective change output before allocating change and test the actual fee using inputAmount - outputAmount.
- [BLOCKING] lib/transaction/transaction.js:1081: Round the calculated fee to whole duffs
Multiplying the byte count by feePerKb / 1000 can produce fractional duffs for an integer fee rate. A three-input P2PKH transaction using .feePerKb(1200) has a 488-byte estimate before change, producing a fee of 585.6 duffs. Calling .change(address) then throws "Output satoshis is not a natural number" because _updateChangeOutput() subtracts that fractional fee from the available integer amount. Round the calculated fee upward so output amounts remain integral and the requested rate is preserved.
- [BLOCKING] lib/transaction/transaction.js:1094-1096: Estimate signature growth using each input's script type
The new estimator replaces input-specific size estimates with a fixed 107-byte P2PKH allowance for every unsigned input. Supported multisig inputs can require more signature bytes: after clearSignatures() installs a 2-of-3 P2SH redeem script, the input still needs two signatures, whose combined size exceeds this allowance. Replaying the updated final fixture in test/data/tx_creation.json produces exactly its expected serialization, but the transaction pays only 586 duffs for 696 bytes at the default 1,000 duffs/kB rate. This signature-growth underestimate is separate from the omitted change output. Use script-type-aware estimates that account for bytes already serialized and any script-length varint growth.
- [BLOCKING] lib/transaction/transaction.js:1094: Do not require signature verification to estimate transaction size
Transaction.isFullySigned() throws UnableToVerifySignature for generic Input instances, including custom scripts and inputs parsed from serialized transactions. Calling it from _estimateSize() introduces that requirement into the fee checks performed by serialize(), even when disableIsFullySigned is explicitly true. A reproduced transaction spending an OP_TRUE UTXO with previous-output information throws here when serialized with that option; the same transaction serializes successfully when using the base revision's fee and size estimators. Estimate size without requiring signature-verification support so the existing serialization option remains effective.
- [BLOCKING] lib/transaction/transaction.js:1085-1086: Apply minimumFee consistently as a lower bound
This branch adds the minimum fee to a size-based fee that already meets the minimum. It also treats an omitted minimum differently from an explicitly supplied default because fee + undefined is NaN. For example, _estimateFee(500, 100000, 1000) returns 500, while passing 255 as the fourth argument returns 755. In a reproduced two-input P2PKH transaction, explicitly setting .minimumFee(Transaction.MINIMUM_FEE) changes the actual fee from 340 to 595 duffs even though the floor remains 255 and was already satisfied. Normalize the optional minimum once and apply it as a lower bound, not an additional balance-dependent charge.
- [BLOCKING] lib/transaction/transaction.js:845-850: Declare the new minimum-fee API for TypeScript consumers
The PR introduces the public minimumFee() method and MINIMUM_FEE constant without adding them to typings/transaction/Transaction.d.ts. Type-checking new Transaction(undefined).minimumFee(500) and Transaction.MINIMUM_FEE produces TS2339 for both, preventing TypeScript consumers from using the new feature through the package declarations. Add the chainable method and static constant declarations, together with test-d coverage. The existing type tests pass because they do not exercise either addition.
| if ( this._missingChange() ) { | ||
| serializedSize += Transaction.CHANGE_OUTPUT_MAX_SIZE; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Include the pending change output before allocating change
_missingChange() checks whether a change address is configured, not whether the transaction currently contains its change output. During _updateChangeOutput(), the change script is already configured and the previous change output has been removed, so this condition skips budgeting for the forthcoming output. A reproduced two-input P2PKH transaction pays 340 duffs but serializes to 373 bytes after signing, below the default 1,000 duffs/kB rate. Its subsequent getFee() call returns 373 rather than the actual 340, so the new getFee() assertions do not detect the underpayment. Include the prospective change output before allocating change and test the actual fee using inputAmount - outputAmount.
source: ['claude']
| size += Transaction.CHANGE_OUTPUT_MAX_SIZE; | ||
| Transaction._estimateFee = function (size, amountAvailable, feePerKb, minimumFee) { | ||
| var feePerByte = (feePerKb || Transaction.FEE_PER_KB) / 1000; | ||
| var fee = size * feePerByte; |
There was a problem hiding this comment.
🔴 Blocking: Round the calculated fee to whole duffs
Multiplying the byte count by feePerKb / 1000 can produce fractional duffs for an integer fee rate. A three-input P2PKH transaction using .feePerKb(1200) has a 488-byte estimate before change, producing a fee of 585.6 duffs. Calling .change(address) then throws "Output satoshis is not a natural number" because _updateChangeOutput() subtracts that fractional fee from the available integer amount. Round the calculated fee upward so output amounts remain integral and the requested rate is preserved.
| var fee = size * feePerByte; | |
| var fee = Math.ceil(size * feePerByte); |
source: ['claude']
| if ( !this.isFullySigned() ) { | ||
| // every signed input contains some additional bytes | ||
| serializedSize += this.inputs.length * PublicKeyHashInput.SCRIPT_MAX_SIZE; |
There was a problem hiding this comment.
🔴 Blocking: Estimate signature growth using each input's script type
The new estimator replaces input-specific size estimates with a fixed 107-byte P2PKH allowance for every unsigned input. Supported multisig inputs can require more signature bytes: after clearSignatures() installs a 2-of-3 P2SH redeem script, the input still needs two signatures, whose combined size exceeds this allowance. Replaying the updated final fixture in test/data/tx_creation.json produces exactly its expected serialization, but the transaction pays only 586 duffs for 696 bytes at the default 1,000 duffs/kB rate. This signature-growth underestimate is separate from the omitted change output. Use script-type-aware estimates that account for bytes already serialized and any script-length varint growth.
source: ['claude']
| return result; | ||
| // string format HEX -> 2 symbols == 1 byte | ||
| var serializedSize = this.toString().length / 2; | ||
| if ( !this.isFullySigned() ) { |
There was a problem hiding this comment.
🔴 Blocking: Do not require signature verification to estimate transaction size
Transaction.isFullySigned() throws UnableToVerifySignature for generic Input instances, including custom scripts and inputs parsed from serialized transactions. Calling it from _estimateSize() introduces that requirement into the fee checks performed by serialize(), even when disableIsFullySigned is explicitly true. A reproduced transaction spending an OP_TRUE UTXO with previous-output information throws here when serialized with that option; the same transaction serializes successfully when using the base revision's fee and size estimators. Estimate size without requiring signature-verification support so the existing serialization option remains effective.
source: ['claude']
| if (amountAvailable > fee + minimumFee) { | ||
| fee += (minimumFee || Transaction.MINIMUM_FEE); |
There was a problem hiding this comment.
🔴 Blocking: Apply minimumFee consistently as a lower bound
This branch adds the minimum fee to a size-based fee that already meets the minimum. It also treats an omitted minimum differently from an explicitly supplied default because fee + undefined is NaN. For example, _estimateFee(500, 100000, 1000) returns 500, while passing 255 as the fourth argument returns 755. In a reproduced two-input P2PKH transaction, explicitly setting .minimumFee(Transaction.MINIMUM_FEE) changes the actual fee from 340 to 595 duffs even though the floor remains 255 and was already satisfied. Normalize the optional minimum once and apply it as a lower bound, not an additional balance-dependent charge.
source: ['claude']
| 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.
🔴 Blocking: Declare the new minimum-fee API for TypeScript consumers
The PR introduces the public minimumFee() method and MINIMUM_FEE constant without adding them to typings/transaction/Transaction.d.ts. Type-checking new Transaction(undefined).minimumFee(500) and Transaction.MINIMUM_FEE produces TS2339 for both, preventing TypeScript consumers from using the new feature through the package declarations. Add the chainable method and static constant declarations, together with test-d coverage. The existing type tests pass because they do not exercise either addition.
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: