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

Remain functionally compatible with pre-25.0 sendrawtransaction #306

Closed
ShitcoinsSuck opened this issue Jun 26, 2023 · 3 comments
Closed

Comments

@ShitcoinsSuck
Copy link

Recently bitcoin core added an optional parameter to the sendrawtransaction call to limit intentional burns. It was implmented in this commit. This was released in Bitcoin Core 25.0.

Whereas there was no default limit prior to 25.0, the default is now set to 0, meaning sending non-zero values to OP_RETURN via sendrawtransaction will fail by default.

To keep pre-25.0 functionality intact, users of that RPC call (this library among others) will need to send the new maxburnamount parameter in the RPC call, which will also require sending the previously unset maxfeerate.

Changing the send_raw_transaction function body to the following change would accomplish it:

        let server_version=self.version()?;
        if server_version < 250000 {
            self.call("sendrawtransaction", &[tx.raw_hex().into()])
        }
        else {
            // default max rate in bitcoin core is 0.1 COIN
            let default_max_raw_tx_fee_rate : f64 = 0.1;
            // default max burn set to max COIN supply to be compatible with pre-25.0 functionality
            let default_max_burn : i64 = 21000000;
            self.call("sendrawtransaction", &[tx.raw_hex().into(), into_json(default_max_raw_tx_fee_rate)?, into_json(default_max_burn)?])
        }
ShitcoinsSuck pushed a commit to ShitcoinsSuck/rust-bitcoincore-rpc that referenced this issue Jun 26, 2023
…endrawtransaction be setting maxburnamount to max supply
@stevenroose
Copy link
Collaborator

Ok, I checked the issue you crated. I'd NACK this. It's not our goal to make new versions of Core behave like old versions. Users that upgrade to a newer Core release should be aware of the changes.

The only thing we could do here is support the two optional arguments so that users can actually pass them in from 0.25 on.

Maybe even have both send_raw_transaction and send_raw_transaction_with_args or something like that.

@ShitcoinsSuck
Copy link
Author

I'm open to fixing this other ways, but let me first explain why I still see this as the cleanest route to solving the problem introduced by the new parameter:

As an aside, send raw transaction already implies that one should be able to send the raw transaction. I.e. any safety checks should be done on the user side rather than in the node. It's kind of silly to require putting the amount twice in the same function--once in the tx param and once in the new parameter. In other words this new param did not help anything--only broke existing use cases. That's more of a complaint about the pointlessness of the new param than this issue, but I think it's relevant as to why I believe this is the best way to address it.

From the user perspective, the new param broke existing functionality, many with no way to remedy if they already upgraded to 25 (especially non-technical node runners using Umbrel with no way to downgrade).

So how to keep the functionality? My argument is that it should be at the layer that directly communicates with core. In that way, higher level users are unaffected and their use cases work exactly as before.

Otherwise we create an entirely new path and keep an abandoned old path. Where does it stop, in the user-facing wallet? Should they have a new send tx screen that requires verifying the maxburnamount as well as inputting the amount? That's the UX that the core change seems to suggest... That we need to pass down a new, pointless parameter from electrum->electrs->rust-bitcoincore-rpc->core.

But in any case we can try a new send_raw_transaction_with_args, which then will require getting electrs to call that instead. Or in the worst case, electrs has the same view that it should be a new function, and then we'll change all client wallets of electrum servers as well... In order to maintain the same functionality, we'll ultimately need to change every single user of the RPC call, every single electrum server on top of that, and every single user-facing app.

That's why I believe maintaining existing functionality in the direct connection to core is ultimately the cleanest route.

@stevenroose
Copy link
Collaborator

I kinda agree with your argument that these new parameters are annoying.

But this crate is aiming to be purely a Rust wrapper around the RPCs that Core has, not a utility crate around Core's RPC.

We already lack people with interest in maintaining this crate, so I'm against adding more complexity than just being as close as a pure client as we can.

I think your criticism is better directed at Core itself.

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

2 participants