namecoin / namecoin-core

Namecoin full node + wallet based on the current Bitcoin Core codebase.
https://www.namecoin.org/
MIT License
455 stars 146 forks source link

Name support in fundrawtransaction RPC #369

Open yanmaani opened 3 years ago

yanmaani commented 3 years ago

If you have a transaction with more output than input value, fundrawtransaction will add inputs to match and to pay the fee.

It would be nice, especially for atomic name trades, if fundrawtransaction would add missing name inputs too. e.g. turn this:

in = []
out = [NMC 10 -> A, "d/name" -> B]

into this:

in = [NMC 10 <- C, "d/name"<- D]
out = [NMC 10 -> A, "d/name" -> B]

Then you could accept non-interactive offers by feeding them into fundrawtransaction. Here is some (edited) discussion from IRC about this: (emphasis added)

yanmaani: Hey, how do you join raw transactions while keeping their sigs? Y: Say you have one txn that consumes NMC 10.01 and gives you name X, Y: and one transaction that consumes name X and gives someone NMC 10. Y: Then how do I join them? Jeremy Rand: Not sure off the top of my head. For accepting Open offers? Is it actually necessary to join things for that purpose? Wouldn't fundrawtransaction do the job? R: (I guess maybe we'd need to teach fundrawtransaction how to balance name operations, but that's probably desirable anyway.) R: Joining those transactions will make it painful to handle fees optimally. I think fundrawtransaction is designed for this use case, we just need to patch it to balance names as well as currency. R: i.e. if you have a tx that has a name input but no name output (or vice versa), that's an unbalanced transaction, and fundrawtransaction should be patched to balance it (i.e. add the missing name input/output)

Y: If you have two transactions with sigs, how do you join them? Do you take the offer, and do fundrawtransaction and sign to accept? R: Right, you supply the offer to fundrawtransaction, and sign the result to accept it. Y: Does this handle choosing a suitable input for me, adding the change to the name output, etc? R: Yes. fundrawtransaction will add whatever inputs and change are needed to balance the transaction. Right now it only balances the currency amounts, but I think a patch that makes it also balance names (i.e. add a name input/output to balance it) would be worth proposing. R: I suppose there's an edge case where a third party broker obtains a buy offer and a sell offer for the same name and wants to complete the sale, but that's out of scope for this. Y: Our third-party broker could just deserialize the transactions and do it, though. There's no need for it to be in namecoin core. Y: Anyway, this simplifies the name offer API stuff; "make offer" is just make raw txn + add name, and "take offer" is just fund + sign + broadcast Y: You might want a RPC to check if your complete but unfunded trade is correct with respect to an unsigned buy offer, though. Else you'll get scams. R: Maybe have an optional argument to fundrawtransaction that constrains which name is allowed to be added as an input or output? That would at least prevent scams involving the wrong name or the wrong direction. Y: Yes, that's pretty clean, unless someone is stupid enough to try and buy a name they already own. Y: So do we even need psbt for exclusives? Bob sends "name to me, money to you", Sally funds and signs, Bob funds and signs, done? Y: Alternative workflow: Bob sends Open buy offer to Sally, she signs SIGHASH_ALL and broadcasts. R: That's a good question. Is there any significant use case where Exclusive (AKA ANTPY) workflow can't be replaced by an Open (AKA NameTrade) buy offer being transmitted privately and then signed by the seller with SIGHASH_ALL? Y: What if the Seller sends an exclusive offer to buyer, that he can accept immediately? In this proposed workflow, the buyer must be the proposer instead. Y: Let's say I own d/eff, and I want to sell it to the real EFF, but I want some finder's fee. Then I'll have to ask them to put out a buy offer. Not ideal. R: I don't follow how that's a problem. If the seller initiates the workflow, then an Open Buy Offer workflow and an Exclusive Sell Offer workflow have the same number of interactions. The first interaction just doesn't have a partial transaction attached to it. Y: True.

Also, the change should go into the name, because anything above NMC 0.01 is spendable:

Rand: The currency input doesn't need to be an exact match for the sale amount (since you can use a change output for Exclusive offers, or you can put the change in the name output for either Exclusive or Open offers) yanmaani: Wait, you can put the change in the name output?? Can I spend it immediately then? Y: So how does it work if you put your change inside the name output? R: You just create an output with more than 0.01 NMC in it, and then prepend the name script prefix like usual. Whatever the amount of the output is, minus the 0.01 for the name, will be spendable as currency.

domob1812 commented 3 years ago

I'm not sure about this. While it certainly makes the flow a bit easier, fundrawtransaction's main function is to choose coins using wallet logic, which is not trivial to do explicitly. Adding a name input on the other hand is something that is trivial to do for a caller by themselves. As the RPC interface is meant to be used by software (mostly), I think the better solution here is to just let that software call name_show first and add the input by itself.

Also, I'm not a big fan of using higher-than-0.01 NMC values in names. While that is certainly possible of course, it feels like a hack to me and I think we should avoid it as much as possible.

JeremyRand commented 3 years ago

I'm not sure about this. While it certainly makes the flow a bit easier, fundrawtransaction's main function is to choose coins using wallet logic, which is not trivial to do explicitly. Adding a name input on the other hand is something that is trivial to do for a caller by themselves. As the RPC interface is meant to be used by software (mostly), I think the better solution here is to just let that software call name_show first and add the input by itself.

@domob1812 Quoting https://github.com/namecoin/namecoin-core/blob/master/doc/psbt.md#rpcs :

walletcreatefundedpsbt (Creator, Updater) is a wallet RPC that creates a PSBT with the specified inputs and outputs, adds additional inputs and change to it to balance it out, and adds relevant metadata. In particular, for inputs that the wallet knows about (counting towards its normal or watch-only balance), UTXO information will be added. For outputs and inputs with UTXO information present, key and script information will be added which the wallet knows about. It is equivalent to running createrawtransaction, followed by fundrawtransaction, and converttopsbt.

IMO, adding a name input or a name output to balance a transaction that has a name input but no name output (or vice versa) is very much within the stated scope of "add[ing] additional inputs and change to it to balance it out".

Also, I'm not convinced that doing this as 2 separate RPC's is always easy. If we add a name output first and then run fundrawtransaction, then that means we need to figure out the name output address ourselves, and also means that we can't consolidate the name output and the change output into a pure name output. If we run fundrawtransaction first and then add a name output, then fundrawtransaction's calculated fee will no longer be correct because of the extra bytes taken by the name script prefix.

Generally I would rather have this kind of functionality in the RPC interface, where it can properly be subject to integration tests, rather than figuring "the application will handle this", especially something like this where I don't have any confidence that application devs (including me) will find this to be trivial. Keeping the RPC interface too low-level has bitten us before (mainly in cases where the GUI ended up adding logic that would have been better to have in the RPC interface).

Also, I'm not a big fan of using higher-than-0.01 NMC values in names. While that is certainly possible of course, it feels like a hack to me and I think we should avoid it as much as possible.

If you dislike pure name transactions, it would be good if you could explain why in https://github.com/namecoin/meta/issues/43 (I think they're generally a good idea for multiple reasons, as elaborated in that issue).

domob1812 commented 3 years ago

@JeremyRand Thanks for your feedback! I thought the point was to add the name input, not the name output? Because you need to add the name output anyway already when you specify the name operation - just like you add the desired outputs before you call fundrawtransaction for currency transactions? Adding the name input manually is trivial IMHO. (But sure, if you prefer to do this in fundrawtransaction, I have no really strong objection. Just trying to keep daemon complexity to the minimum where possible.)

Also one thing to note: fundrawtransaction only works if all inputs are solvable (independent of which ones you add manually and which ones are present already). In particular, it will not work for processing atomic trades unfortunately.

yanmaani commented 3 years ago

@domob1812

Also one thing to note: fundrawtransaction only works if all inputs are solvable (independent of which ones you add manually and which ones are present already). In particular, it will not work for processing atomic trades unfortunately.

Any reason for this? Could we change it so it does work with atomic name trades?

If you're dealing with fungible currency, then it makes sense not to add random people's coins to it, but if you're just dealing with names, why not? There's only one possible input, right?

domob1812 commented 3 years ago

@yanmaani The reason is fee computation and upstream https://github.com/bitcoin/bitcoin/issues/7879. If we can get upstream to solve this, I would be very happy myself, but I doubt it.

What one can do instead is use fundrawtransaction as the "buyer" of a name, and then just manually add the name input and output to it. This increases the size of the transaction by less than 50%, so you just need to be "a little" generous in the feerate when calling fundrawtransaction. At least, that's the best solution I was able to come up with so far (and that's what we plan to use for trading in Xaya).

JeremyRand commented 3 years ago

@yanmaani The reason is fee computation and upstream bitcoin/bitcoin#7879. If we can get upstream to solve this, I would be very happy myself, but I doubt it.

@domob1812 If I'm understanding correctly Andrew's explanation at https://github.com/bitcoin/bitcoin/issues/7879#issuecomment-511847862 , it sounds like the high-level issue is that Bitcoin Core expects some metadata to be present for the inputs, which is not available in the network-serialized format that fundrawtransaction accepts. Is this metadata covered by the PSBT specification, or is it not even present there? If it's covered by PSBT, then that seems like a strong hint that we should be using PSBT for this use case rather than the network serialization that Andrew recommended to us. If PSBT doesn't cover it, then that seems like a bug in the PSBT spec.

What one can do instead is use fundrawtransaction as the "buyer" of a name, and then just manually add the name input and output to it. This increases the size of the transaction by less than 50%, so you just need to be "a little" generous in the feerate when calling fundrawtransaction. At least, that's the best solution I was able to come up with so far (and that's what we plan to use for trading in Xaya).

This is an incredibly messy solution since it means that the feerate passed by the user is unpredictably different from the feerate that actually gets used. I would really rather get this resolved properly, even if it takes more effort.

JeremyRand commented 3 years ago

According to https://github.com/bitcoin/bitcoin/blob/master/doc/psbt.md#overall-workflow , PSBT metadata includes:

information about the scripts and public keys involved in each of the inputs (and possibly outputs) of the PSBT

Which sounds like it covers the metadata that's missing according to Andrew's explanation about fundrawtransaction. So it sounds like we should be using PSBT for this. @domob1812 Is my analysis accurate?

domob1812 commented 3 years ago

@JeremyRand Yes indeed, I think the data should be present in PSBTs. I do not know if fundpsbt works with imported addresses in Bitcoin Core - if you got some time, maybe try it out? If it does, then we should just go with PSBTs and all should work well (and I'd be happy also for Xaya usecases). If it doesn't, then indeed that would be a good reason to bring it back up with upstream Bitcoin, and see if we can get that support added.

JeremyRand commented 3 years ago

@JeremyRand Yes indeed, I think the data should be present in PSBTs. I do not know if fundpsbt works with imported addresses in Bitcoin Core - if you got some time, maybe try it out? If it does, then we should just go with PSBTs and all should work well (and I'd be happy also for Xaya usecases). If it doesn't, then indeed that would be a good reason to bring it back up with upstream Bitcoin, and see if we can get that support added.

@domob1812 Unless the Bitcoin Core docs are outdated, there is no direct PSBT equivalent of fundrawtransaction; there is only walletcreatefundedpsbt, which does not accept a PSBT as input. Should we ask Bitcoin Core to add a fundpsbt RPC method that accepts a PSBT as input, and utilizes PSBT metadata to correctly handle inputs that are not solvable by the wallet?

domob1812 commented 3 years ago

@JeremyRand I'm not aware of any other approach either, so yes, let's see what they think about that.

yanmaani commented 3 years ago

@domob1812

What one can do instead is use fundrawtransaction as the "buyer" of a name, and then just manually add the name input and output to it. This increases the size of the transaction by less than 50%, so you just need to be "a little" generous in the feerate when calling fundrawtransaction. At least, that's the best solution I was able to come up with so far (and that's what we plan to use for trading in Xaya).

If you want to do that then it'd make more sense to add a parameter to fundrawtransaction 'bytes', so that the fee is calculated for size+bytes (default value = 0).

domob1812 commented 3 years ago

If you want to do that then it'd make more sense to add a parameter to fundrawtransaction 'bytes', so that the fee is calculated for size+bytes (default value = 0).

Yes, we can do that. I don't think it is the best approach in general - getting upstream Bitcoin to "fix" funding would be ideal. The suggested approach (maybe with your change) is just something we can do instead if they don't agree. Also I think from a practical point of view, very precise fee estimation is at least for now not very important in Namecoin anyway.