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

Danger: fee_rate in FundRawTransactionOptions may cause broken code losing money #320

Open
Kixunil opened this issue Nov 23, 2023 · 3 comments
Milestone

Comments

@Kixunil
Copy link

Kixunil commented Nov 23, 2023

So apparently Bitcoin Core accepts both fee_rate and feeRate with different semantics. Serialization in this crate uses the camel case version which is not documented and thus anyone using it may falsely assume it's the lower case one since that is how the field is named (by Rust convention).

This really needs to be changed to FeeRate from the bitcoin crate and handled properly.

Note: Found by seeing this: https://twitter.com/rot13maxi/status/1727667840012923302

Note2: if author of the tweet or any of the 15 people who liked it and bookmarked it sees this: Why the fuck you didn't open this issue already?! How we're supposed to know about a critical problem if you keep it on twitter? I don't spend time on twitter so that I can improve the libraries you depend on. I found this by insane luck that someone shared an article which shared the tweet which had that response.

@apoelstra
Copy link
Member

Whew. Glad you caught this! I think probably we need to add a feeRate parameter, deprecate the fee_rate one, and error out if anybody uses it. The result should be an immediate visible failure, and since it's failing to send funds (vs failing to receive, or to verify, or whatever) this should be a safe failure mode.

FWIW, I am not on Twitter either but fairly frequently, when people tweet about my things, somebody will send a link to the tweet on Signal so that I'm able to react if need be. Possibly this happened, or was about to happen, with Steven. Meanwhile the people posting on Twitter are probably not aware that I (or you) are missing from there.

I agree this isn't a very reliable or reasonable communication method but it seems like it's how the world works.

@Kixunil
Copy link
Author

Kixunil commented Nov 26, 2023

Why would that be better than changing the type? I think the type communicates the meaning much better and then we don't have ugly non-idiomatic feeRate field name. The units are already convertible (outside of ridiculous values) so it should work fine.

@apoelstra
Copy link
Member

Why would that be better than changing the type?

Oh, I misunderstood your comment to mean renaming the field. Yes, changing the type would be better.

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

No branches or pull requests

3 participants