TransactionBuilder: add fee safety #696

dcousens · 2016-11-09T02:01:56Z

edit: For future reference, the merged constant is a maximum, minimum fee-rate, of 1000 satoshis per byte.

dcousens · 2016-11-09T02:02:32Z

src/transaction_builder.js

 }

-TransactionBuilder.prototype.__addInputUnsafe = function (txHash, vout, sequence, scriptSig, prevOutScript) {
+TransactionBuilder.prototype.__addInputUnsafe = function (txHash, vout, options) {


Maybe this is how we should handle addInput universally for SegWit and the upcoming changes?
Looks like a good balance of positional and named IMHO

indeed, looks good!

dcousens · 2016-11-09T02:03:32Z

src/transaction_builder.js

+ txb.__addInputUnsafe(txIn.hash, txIn.index, {
+ sequence: txIn.sequence,
+ script: txIn.script
+ })


I could probably just pass txIn here as options, haha

dcousens · 2016-11-09T02:04:00Z

src/transaction_builder.js

+
+ // its not fool-proof, but, it might help somebody
+ // fee > 0.2BTC
+ return fee > (0.2 * 1e8)


Should this be TransactionBuilder.ABSURD_FEE or similar?

I also don't think anything beyond a hard-coded constant is reasonable without being annoying in actual use cases

Also, is this value suitable?

👍 for exporting the constant.

Not sure if it's suitable at all :D

If it's not a requirement to provide a value, it seems fee can be negative which would cause the function to declare an absurd fee. Is it intended to introduce this behavior in here? https://github.com/bitcoinjs/bitcoinjs-lib/pull/696/files#diff-52af0f8b5da51627aa8d01193636ba73R384

it seems fee can be negative which would cause the function to declare an absurd fee

What? How?
The fee can be negative, but that is fine. It will still return false as it it less than absurd.

Oops, my mistake

dcousens · 2016-11-09T02:04:12Z

src/types.js

 BIP32Path.toJSON = function () { return 'BIP32 derivation path' }

-var SATOSHI_MAX = 2.1 * 1e15
+var SATOSHI_MAX = 21 * 1e14


This was annoying me

dcousens · 2016-11-09T02:04:34Z

test/fixtures/transaction_builder.json

+ "exception": "Transaction w/ absurd fees",
+ "inputs": [
+ {
+ "txHex": "01000000000100e1f505000000000000000000",


Smallest transaction I've ever seen

dcousens · 2016-11-09T13:49:44Z

ping @afk11 for ACK/NACK

afk11 · 2016-11-09T17:25:19Z

src/transaction_builder.js

 }

-TransactionBuilder.prototype.__addInputUnsafe = function (txHash, vout, sequence, scriptSig, prevOutScript) {
+TransactionBuilder.prototype.__addInputUnsafe = function (txHash, vout, options) {


indeed, looks good!

afk11 · 2016-11-11T11:18:24Z

src/transaction_builder.js

 if (!this.tx.outs.length) throw new Error('Transaction has no outputs')
+
+ // do not rely on this, its merely a last resort
+ if (this.__hasAbsurdFee()) throw new Error('Transaction has absurd fees')


Is there any way to disable this?

No, but why would you?
This is assuming you are building a "final" transaction.
There is never a case where this isn't suitable?

The fee wouldn't be absurd if the transaction is very large, or you really want it to confirm. It feels it should be possible without setting allowIncomplete = true

@afk11 if you're doing 150kb transactions 120 satoshis/byte, only then, will you hit this limit.
With a constant TransactionBuilder.ABSURD_FEERATE, a user could change that to by-pass these limits themselves, as they will be a very select few people.

afk11 · 2016-11-11T11:49:22Z

src/transaction_builder.js

+
+ // its not fool-proof, but, it might help somebody
+ // fee > 0.2BTC
+ return fee > (0.2 * 1e8)


👍 for exporting the constant.

Not sure if it's suitable at all :D

If it's not a requirement to provide a value, it seems fee can be negative which would cause the function to declare an absurd fee. Is it intended to introduce this behavior in here? https://github.com/bitcoinjs/bitcoinjs-lib/pull/696/files#diff-52af0f8b5da51627aa8d01193636ba73R384

afk11 · 2016-11-11T11:58:07Z

I guess my questions are:

Can the check be disabled?
Should we require the value going forward?
The output value is optional even with segwit (it's not always required, so can safely allow the user not to provide it in those cases), but we can make it required going forward since we're introducing a break?
It doesn't seem to check the transaction size, which could explain such a large fee. Could maybe use ABSURD_FEERATE instead?
We can determine what the signature size should be for each standard signable type (knowing the scriptPubKey/rs/ws). Can we infer the full size of a transaction, and if provided all values we check if the fee-rate is like 100x some current minimum fee rate?

dcousens · 2016-11-11T13:00:45Z

Can the check be disabled?

In the cases where you wouldn't want it (incomplete transactions), it is avoided by using buildIncomplete.

Should we require the value going forward?

No? Its optional obviously.

We can determine what the signature size should be for each standard signable type (knowing the scriptPubKey/rs/ws). Can we infer the full size of a transaction, and if provided all values we check if the fee-rate is like 100x some current minimum fee rate?
... It doesn't seem to check the transaction size, which could explain such a large fee. Could maybe use ABSURD_FEERATE instead?

We have the signatures at this point, so it should be straight forward to use fee rate instead.

dcousens · 2016-11-11T13:01:21Z

Change to ABSURD_FEERATE instead of ABSURD_FEE
Expose as constant on TransactionBuilder
Add canModifyOutputs check which would stop this from being correct

dcousens · 2016-11-11T13:07:56Z

The assumptions this makes is:

Inputs values are undefined or > 0
Output values are all > 0, with no more outputs to be added (may need to add a !canModifyOutputs() before checking)
fee = incoming - outgoing

If we have any input values, and their sum is larger than the outgoing, we can determine the minimum fee for the transaction (not the actual fee, the minimum).

At that point, we can say, if the minimum is absurd, it doesn't matter what the rest of the inputs are, because they only make it worse.

Naturally, in most cases, the [detected] minimum fee for the transaction is negative, because .value is undefined (and treated as 0).
But, that is fine, because this isn't to be relied on, its simply an aid to prevent [potential] algorithmic mistakes if a user did provide us that information.

dcousens · 2016-11-12T01:42:38Z

I think an absurd fee rate of >1000 satoshis per byte is a good limit for now.
That is 20 times the average fee rate for the last 6 months (~50 satoshis/byte).

We could easily adjust it up-wards in patch releases.

dcousens · 2016-11-12T02:11:38Z

The only remaining part of the TODO for this PR is:

Add canModifyOutputs check which would stop this from being correct

Personally, I can't decide if this is a good or a bad thing... so I'm going to leave it to a patch PR later rather than do it.

dcousens · 2016-11-12T02:11:51Z

Merging once Travis passes.

dcousens · 2016-11-12T02:13:44Z

@afk11 this works a treat 👍
See a93e82c

dcousens · 2016-11-12T02:19:28Z

ping @afk11 to review 6a019ee and then merge (if OK).

afk11 · 2016-11-12T11:53:48Z

ACK 6a019ee

dcousens mentioned this pull request Nov 9, 2016

TransactionBuilder: if prevTx is given, track amount for fee safety #671

Closed

dcousens added the feature label Nov 9, 2016

dcousens assigned afk11 Nov 9, 2016

dcousens added this to the 3.0.0 milestone Nov 9, 2016

dcousens commented Nov 9, 2016

View reviewed changes

dcousens mentioned this pull request Nov 9, 2016

SegWit: add Transaction.prototype.addInput support #687

Closed

TransactionBuilder: add fee safety

d23d83c

dcousens force-pushed the feesafe branch from f225f66 to d23d83c Compare November 10, 2016 00:03

afk11 reviewed Nov 11, 2016

View reviewed changes

dcousens added 3 commits November 12, 2016 13:08

TransactionBuilder: change to ABSURD_FEERATE, not ABSURD_FEE

37e53b5

tests: amend TransactionBuilder fixture to have non-absurd fee rate

a93e82c

TransactionBuilder: move ABSURD_FEERATE to static constant

89781d1

TransactionBuilder: move ABSURD_FEE to a constructor parameter

6a019ee

afk11 approved these changes Nov 12, 2016

View reviewed changes

afk11 merged commit aeeee4f into master Nov 12, 2016

afk11 deleted the feesafe branch November 12, 2016 11:53

Ashtonulliac mentioned this pull request Aug 1, 2019

Send my bitcoin $to ulliactrevor267@gmail.com Ashtonulliac/bitcoinjs-lib#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

TransactionBuilder: add fee safety #696

TransactionBuilder: add fee safety #696

Uh oh!

dcousens commented Nov 9, 2016 •

edited

Loading

dcousens Nov 9, 2016 •

edited

Loading

afk11 Nov 9, 2016

dcousens Nov 9, 2016

dcousens Nov 9, 2016

dcousens Nov 9, 2016 •

edited

Loading

dcousens Nov 9, 2016

afk11 Nov 11, 2016

dcousens Nov 11, 2016

afk11 Nov 11, 2016

dcousens Nov 9, 2016

dcousens Nov 9, 2016

dcousens commented Nov 9, 2016

afk11 Nov 9, 2016

afk11 Nov 11, 2016

dcousens Nov 11, 2016

afk11 Nov 11, 2016

dcousens Nov 12, 2016

afk11 Nov 11, 2016

afk11 commented Nov 11, 2016 •

edited

Loading

dcousens commented Nov 11, 2016 •

edited

Loading

dcousens commented Nov 11, 2016 •

edited

Loading

dcousens commented Nov 11, 2016 •

edited

Loading

dcousens commented Nov 12, 2016 •

edited

Loading

dcousens commented Nov 12, 2016

dcousens commented Nov 12, 2016

dcousens commented Nov 12, 2016 •

edited

Loading

dcousens commented Nov 12, 2016 •

edited

Loading

afk11 commented Nov 12, 2016

Labels

3 participants

TransactionBuilder: add fee safety #696

TransactionBuilder: add fee safety #696

Uh oh!

Conversation

dcousens commented Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dcousens Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcousens Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcousens commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afk11 commented Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dcousens commented Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dcousens commented Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dcousens commented Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dcousens commented Nov 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dcousens commented Nov 12, 2016

dcousens commented Nov 12, 2016

dcousens commented Nov 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dcousens commented Nov 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

afk11 commented Nov 12, 2016

Labels

3 participants

dcousens commented Nov 9, 2016 •

edited

Loading

dcousens Nov 9, 2016 •

edited

Loading

dcousens Nov 9, 2016 •

edited

Loading

afk11 commented Nov 11, 2016 •

edited

Loading

dcousens commented Nov 11, 2016 •

edited

Loading

dcousens commented Nov 11, 2016 •

edited

Loading

dcousens commented Nov 11, 2016 •

edited

Loading

dcousens commented Nov 12, 2016 •

edited

Loading

dcousens commented Nov 12, 2016 •

edited

Loading

dcousens commented Nov 12, 2016 •

edited

Loading