Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

use higher default FEE_PER_KB and FEE_SECURITY_MARGIN #125

Merged
merged 2 commits into from
Mar 13, 2017

Conversation

gabegattis
Copy link
Contributor

No description provided.

@gabegattis
Copy link
Contributor Author

I increased each value by a factor of 10. This is arbitrary, but I don't know what else to set them to. The transaction fee market is extremely volatile. If the recent market trends continue, we could be back in this same situation soon. Users of bitcore-lib really need to use an outside source to get a suggested fee/kb.

I am starting to question the wisdom of having these values at all. I am wondering if we should remove them in the next major release.

Feedback is appreciated.

@matiu
Copy link
Contributor

matiu commented Mar 3, 2017

LGTM.

The original value was also arbitrary. It is just a very high value in case the programmer make and error and the fee turns out very high.

@visvirial
Copy link

Hi, I am the original reporter of this issue: #123

There are some options to fix this problem.

  1. Increase the default value as is done in this PR.
  2. Remove the checking functionality to allow arbitrary high fees.
  3. Fetch recommended fee rate from some web services, such as https://bitcoinfees.21.co/.
  4. Allow users to override the default value easily (by passing some options?).
  5. (Other options. Reply as a comment)

There are some cons. and pros. for each options. How do you guys feel?

@gabegattis
Copy link
Contributor Author

@visvirial We have already talked about removing the checking functionality entirely in the next major (api breaking) release. We have not made a decision yet, but most of us think that it would be a good idea.

bitcore-lib is meant to be a completely offline library, so we will not add a feature to fetch from external sources. We already make this data available at https://github.com/bitpay/insight-api#utility-methods Users of bitcore-lib should get fee recommendations form a source like that.

Allowing users to override the default constants is another option. We might do this if we decide to keep the checking functionality long term.

@gabegattis
Copy link
Contributor Author

@matiu @pnagurny @kleetus I fixed the tests. This is ready for review now.

@@ -80,6 +80,6 @@
"change", ["3BazTqvkvEBcWk7J4sbgRnxUw6rjYrogf9"],
"sign", ["L2U9m5My3cdyN5qX1PH4B7XstGDZFWwyukdX8gj8vsJ3fkrqArQo"],
"sign", ["L4jFVcDaqZCkknP5KQWjCBgiLFxKxRxywNGTucm3jC3ozByZcbZv"],
"serialize", "010000000220c24f763536edb05ce8df2a4816d971be4f20b58451d71589db434aca98bfaf00000000fc00473044022077fca9eb2544894068c47028855b0cf147526e9a54d993b7aa028908526944ea02203223ca379fa06b5544c02ed74b3ebb9734e2a9e09bca9b572aa56443a3be4d8d0147304402205caaf5666489ab005f280d30afbcda4d8f6f7195b0a13de89bc1e80f58219f5e02205414938c9d0496f5b45c1f45c028c019b3a956549938c09d983a3cc03e819f05014c695221020483ebb834d91d494a3b649cf0e8f5c9c4fcec5f194ab94341cc99bb440007f2210271ebaeef1c2bf0c1a4772d1391eab03e4d96a6e9b48551ab4e4b0d2983eb452b2103a659828aabe443e2dedabb1db5a22335c5ace5b5b7126998a288d63c99516dd853aeffffffffa0644cd1606e081c59eb65fe69d4a83a3a822da423bc392c91712fb77a192edc00000000fdfd0000483045022100c4c98f6cc0a313aee264ab8171927de590ab495b78f26159e56ba49fc26b1e3802206a12c4d41863756e35f72bd365d862da907272bcb2a949d1d2f64c1867d88ce90147304402207035e6083876dcd5512b40bb3d81e2b38393a62f962f8b701efc066db446ae500220121d38105bb58d8b8ad78bbef212c1f958124d47186bcc1ddcccfc0480eb7eb8014c695221020483ebb834d91d494a3b649cf0e8f5c9c4fcec5f194ab94341cc99bb440007f2210271ebaeef1c2bf0c1a4772d1391eab03e4d96a6e9b48551ab4e4b0d2983eb452b2103a659828aabe443e2dedabb1db5a22335c5ace5b5b7126998a288d63c99516dd853aeffffffff03f04902000000000017a9144de752833233fe69a20064f29b2ca0f6399c8af387007102000000000017a9144de752833233fe69a20064f29b2ca0f6399c8af3873b8f04000000000017a9146c8d8b04c6a1e664b1ec20ec932760760c97688e8700000000"
"serialize", "010000000220c24f763536edb05ce8df2a4816d971be4f20b58451d71589db434aca98bfaf00000000fdfd0000473044022024b955f8bf6aaf0741da011e3214eaec7040cd12694303471cefc6ba0cc4ec290220124738015033a465636dec1524a6f956a229e69d31aef6c7a98b2a291f3cfc6701483045022100e6ae6c43240e8a11a6de2d034501c2a366c0ccdf069c7828de0791f05e68e787022028b80bd36c2b2ae63fe7afb491da6c0ce23fbbb982450962c817b20f0bb24075014c695221020483ebb834d91d494a3b649cf0e8f5c9c4fcec5f194ab94341cc99bb440007f2210271ebaeef1c2bf0c1a4772d1391eab03e4d96a6e9b48551ab4e4b0d2983eb452b2103a659828aabe443e2dedabb1db5a22335c5ace5b5b7126998a288d63c99516dd853aeffffffffa0644cd1606e081c59eb65fe69d4a83a3a822da423bc392c91712fb77a192edc00000000fc00483045022100ae7f136cf906dc37d34d5035b8d2001c6a783773b74507ba83080e73e903623f0220023baf7738395268f7097e5586130f682b911fd49b83b265f8fa481f2a6b1ee90146304302201d60f512a8b37663d85c123933053e0354f13d89daf699ca600defa03d4a1dab021f41042b6e4ba30311fc3a68c228c3725f3b0f05a4453ef19408e6a4ae30a2b0014c695221020483ebb834d91d494a3b649cf0e8f5c9c4fcec5f194ab94341cc99bb440007f2210271ebaeef1c2bf0c1a4772d1391eab03e4d96a6e9b48551ab4e4b0d2983eb452b2103a659828aabe443e2dedabb1db5a22335c5ace5b5b7126998a288d63c99516dd853aeffffffff03f04902000000000017a9144de752833233fe69a20064f29b2ca0f6399c8af387007102000000000017a9144de752833233fe69a20064f29b2ca0f6399c8af387ab2f03000000000017a9146c8d8b04c6a1e664b1ec20ec932760760c97688e8700000000"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When reviewing, parse these txs and do a toObject() on them. You will see that the old one used a 10000 satoshi fee. The new one is 10x higher. Everything else is the same

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kleetus
Copy link
Contributor

kleetus commented Mar 10, 2017

Review complete. LGTM.

@kleetus kleetus merged commit 50e7d24 into bitpay:master Mar 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants