AustEcon / bitsv

BitSV: Bitcoin made easy. Documentation:
https://AustEcon.github.io/bitsv
MIT License
96 stars 28 forks source link

Support custom transaction fee under 1 sat per byte #63

Closed gitzhou closed 4 years ago

AustEcon commented 4 years ago

Hey. Thanks for this!

I assume there are no side effects to be aware of in terms of propagating a 0.5sat/byte fee across the network and having it show on Whatsonchain etc. ? I want the default to be at the lowest, safe level (emphasis on the word safe). But if that is now at 0.5 sat/byte then great :slightly_smiling_face:

But OTOH, if the bitsv built-in broadcasting APIs don't like it, we need to keep the default fee at the safe level but make it so that when a lower fee is preferred that bitsv will accommodate and allow tx creation and round up to nearest satoshi. (i.e. perhaps the user has access to a miner that does accept the lower tx fee rate... and may not care if block explorers show it or not while it is unconfirmed).

I'll need a moment to check this over.

Looks like bitindex testnet is down so failed CI is not your fault. I tried mattercloud url (which is the same server as bitindex afaict) and that is also down.

gitzhou commented 4 years ago

Thanks, Aust. Sorry for my less consideration.

Unconfirmed tx of fee 0.5 can be broadcasted with BitIndex API inside(others not tested yet). WoC and Blockchair are both ok with it.

Depends on your comments, I prefer not to change the default transaction fee if it's safer to keep 1 sat/B, yet it would be great to give support to a custom fee of 0.5

What would you say about this? 😆

ghost commented 4 years ago

As a rule of thumb, the default speed should be medium, not fast or slow. It's best to start in the middle range for such a thing, so someone can go up or down if wanted.

In which case, I think the most fair compromise would be medium is 1 sat/byte, fast is 2 sat/byte, and slow is 0.5 sat/byte. So the default would stay the same numerically, but be moved to the middle of the range.

What do you think?

gitzhou commented 4 years ago

As a rule of thumb, the default speed should be medium, not fast or slow. It's best to start in the middle range for such a thing, so someone can go up or down if wanted.

In which case, I think the most fair compromise would be medium is 1 sat/byte, fast is 2 sat/byte, and slow is 0.5 sat/byte. So the default would stay the same numerically, but be moved to the middle of the range.

What do you think?

Yes, I agree

gitzhou commented 4 years ago

I pushed a new commit per our discussion.

ghost commented 4 years ago

Looks pretty good to me, thank you! However, I would import DEFAULT_FEE_MEDIUM and use it with op_return instead of hardcoding the 1 there. It's pedantic but I think clearer. And future updates to the constants would reflect wherever they are used.

What do you think, @AustEcon?

gitzhou commented 4 years ago

I would import DEFAULT_FEE_MEDIUM and use it with op_return instead of hardcoding the 1 there.

Does this mean to change the following method

def create_op_return_tx(self, list_of_pushdata, outputs=None, fee=1, unspents=None, leftover=None, combine=False):

to

def create_op_return_tx(self, list_of_pushdata, outputs=None, fee=DEFAULT_FEE_MEDIUM, unspents=None, leftover=None, combine=False):

BTW, should we change the comments?

:param fee: The number of satoshi per byte to pay to miners. By default
            BitSV will use a fee of 1 sat/byte
:type fee: ``float``

change it to the following

:param fee: The number of satoshi per byte to pay to miners. By default
            BitSV will use a fee of DEFAULT_FEE_MEDIUM
:type fee: ``float``
AustEcon commented 4 years ago

Looks pretty good to me, thank you! However, I would import DEFAULT_FEE_MEDIUM and use it with op_return instead of hardcoding the 1 there. It's pedantic but I think clearer. And future updates to the constants would reflect wherever they are used.

What do you think, @AustEcon?

Yes - I agree. Not a huge deal in this case but it is good practice to do this. Avoids inconsistencies. Centralizes control to where the constant is defined.

Up to you re: the comments @gitzhou

AustEcon commented 4 years ago

I've personally taken it for a spin now - that was the main holdup on my end sorry... Works fine. I'm happy. Thanks @gitzhou. Can merge after that last little change is in. 😃

gitzhou commented 4 years ago

Haha, great thanks to @teran-mckinney and @AustEcon 😄

gitzhou commented 4 years ago

To avoid misunderstanding, change the PR title

AustEcon commented 4 years ago

Thanks. I had already merged so also edited the commit messages a bit too.

ghost commented 4 years ago

Thank you!

gitzhou commented 4 years ago

Thank YOU! @teran-mckinney 😄