Skip to content
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

feat: added bumpfee rpc #226

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

sander2
Copy link
Contributor

@sander2 sander2 commented Jun 10, 2022

This adds the bumpfee rpc. Since versions < 21 interpret the fee_rate field differently, the serialization got slightly more complicated.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK e4a5f71

json/src/lib.rs Outdated Show resolved Hide resolved
@stevenroose
Copy link
Collaborator

Looks good, left one comment regarding the FeeRate type.

@sander2
Copy link
Contributor Author

sander2 commented Aug 19, 2022

Thanks for the review guys. @stevenroose I updated the PR according to your comment

json/src/lib.rs Outdated Show resolved Hide resolved
json/src/lib.rs Outdated Show resolved Hide resolved
json/src/lib.rs Show resolved Hide resolved
@sander2
Copy link
Contributor Author

sander2 commented Aug 30, 2022

Thanks @tcharding. Sorry for the slow response, I was on holiday. I updated the PR now. I think this PR is ready to merge now - could someone merge please?

@sander2
Copy link
Contributor Author

sander2 commented Aug 30, 2022

Actually the tests are failing because bumpfee changed from version 0.18. See the rpc doc for 0.18 and 0.19. There were 2 changes:

  • change from a totalFee argument to a fee_rate argument
  • change in behavior: 0.18 can only bump fees by decreasing the return-to-self output, whereas from 0.19 on, new inputs will be added if required.

These changes make it very difficult to provide a uniform interface that works in all versions of bitcoin core. I would propose to only support bumpfee for versions 0.19 and up by returning an error in the rpc for versions < 0.19. Thoughts?

@tcharding
Copy link
Member

I would propose to only support bumpfee for versions 0.19 and up by returning an error in the rpc for versions < 0.19. Thoughts?

I'm don't feel confident to answer this, will wait for @stevenroose for ideas.

@stevenroose
Copy link
Collaborator

Actually the tests are failing because bumpfee changed from version 0.18. See the rpc doc for 0.18 and 0.19. There were 2 changes:

* change from a `totalFee` argument to a `fee_rate` argument

* change in behavior: 0.18 can only bump fees by decreasing the return-to-self output, whereas from 0.19 on, new inputs will be added if required.

These changes make it very difficult to provide a uniform interface that works in all versions of bitcoin core. I would propose to only support bumpfee for versions 0.19 and up by returning an error in the rpc for versions < 0.19. Thoughts?

It should be possible to write a custom deserializer using a stub struct that has both field optionally and then build the real return struct from that? I think we do that in some other methods.

@sander2
Copy link
Contributor Author

sander2 commented Sep 20, 2022

Actually the tests are failing because bumpfee changed from version 0.18. See the rpc doc for 0.18 and 0.19. There were 2 changes:

* change from a `totalFee` argument to a `fee_rate` argument

* change in behavior: 0.18 can only bump fees by decreasing the return-to-self output, whereas from 0.19 on, new inputs will be added if required.

These changes make it very difficult to provide a uniform interface that works in all versions of bitcoin core. I would propose to only support bumpfee for versions 0.19 and up by returning an error in the rpc for versions < 0.19. Thoughts?

It should be possible to write a custom deserializer using a stub struct that has both field optionally and then build the real return struct from that? I think we do that in some other methods.

The problem is not to build the return struct - the problem is that these 2 versions of the rpc behave differently.

What we'd need to do in version 0.18, is to convert the fee_rate argument to a total_fee based on size of the transaction, and to call bumpfee with that. That's relatively straightforward. But this rpc will fail if the return-to-self output can not be reduced sufficiently to achieve that fee, since 0.18 will not add new inputs (unlike 0.19 and up). I assumed that you'd want the library to behave identically for all bitcoin versions. But maybe this is not the case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants