search

rust-bitcoin / rust-bitcoincore-rpc

Rust RPC client library for the Bitcoin Core JSON-RPC API.
343 stars 258 forks source link

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

Open Kixunil opened 1 year ago

Kixunil commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.