Repository navigation
Feat/fix fee calculations #312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -102,6 +102,8 @@ Transaction.FEE_PER_KB = 1000; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Transaction.CHANGE_OUTPUT_MAX_SIZE = 20 + 4 + 34 + 4; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Transaction.MAXIMUM_EXTRA_SIZE = 4 + 9 + 9 + 4; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Transaction.MINIMUM_FEE = 255; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Transaction.TYPES = registeredTransactionTypes; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Transaction.CURRENT_VERSION = CURRENT_VERSION; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -832,6 +834,21 @@ Transaction.prototype.feePerKb = function (amount) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return this; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * 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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+837
to
+850
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: New public The PR adds a new public setter source: ['claude']
Comment on lines
+845
to
+850
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 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'] |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* Output management */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1046,7 +1063,7 @@ Transaction.prototype.getFee = function () { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Transaction.prototype._estimateFee = function () { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var estimatedSize = this._estimateSize(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var available = this._getUnspentValue(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return Transaction._estimateFee(estimatedSize, available, this._feePerKb); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return Transaction._estimateFee(estimatedSize, available, this._feePerKb, this._minimumFee); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Transaction.prototype._getUnspentValue = function () { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1059,23 +1076,30 @@ Transaction.prototype._clearSignatures = function () { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Transaction._estimateFee = function (size, amountAvailable, feePerKb) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var fee = Math.ceil(size / 1000) * (feePerKb || Transaction.FEE_PER_KB); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (amountAvailable > fee) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 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.
Suggested change
source: ['claude'] |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if(fee<(minimumFee || Transaction.MINIMUM_FEE)){ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return (minimumFee || Transaction.MINIMUM_FEE) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (amountAvailable > fee + minimumFee) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fee += (minimumFee || Transaction.MINIMUM_FEE); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1085
to
+1086
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 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'] |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1085
to
1087
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking:
Suggested change
source: ['claude', 'codex'] |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return Math.ceil(size / 1000) * (feePerKb || Transaction.FEE_PER_KB); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return fee; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1080
to
+1088
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking: Per-byte formula returns fractional duffs and will throw downstream
Suggested change
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() ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 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'] |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // every signed input contains some additional bytes | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| serializedSize += this.inputs.length * PublicKeyHashInput.SCRIPT_MAX_SIZE; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1094
to
+1096
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 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'] |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( this._missingChange() ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| serializedSize += Transaction.CHANGE_OUTPUT_MAX_SIZE; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1098
to
+1100
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 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'] |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return serializedSize; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
1091
to
1103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking: Hard-coded
Suggested change
source: ['claude', 'codex']
Comment on lines
1091
to
1103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion:
source: ['claude']
Comment on lines
1091
to
1103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion:
source: ['claude']
Comment on lines
1091
to
1103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Each source: ['claude']
Comment on lines
+1079
to
1103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 Nitpick: Inconsistent formatting in new
source: ['claude'] |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Transaction.prototype._removeOutput = function (index) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 Nitpick: JSDoc for
minimumFeeis wrong (copy-pasted fromfeePerKb)The JSDoc says
@param {number} amount satoshis per KB to be sentand the prose saysManually set the minimum fee's(stray apostrophe). The minimum fee is a flat duff amount, not satoshis-per-KB.source: ['claude']