Add atomic name trades

[JeremyRand]

No description provided.

[JeremyRand]

@domob1812 @yanmaani Feel free to review. Functional tests aren't added yet; I will do so before merging, but it's currently working well enough that it should be reviewable. Workflow is to use name_buy or name_sell with the amount parameter to create an offer, and then use the inverse command with both amount and offer parameters to accept the offer, then use the broadcast command with the completed trade transaction.

[JeremyRand]

Functional tests are added now; this is a candidate for merging.

[JeremyRand]

@ryancdotorg FYI this PR implements your NameTrade non-interactive scheme.

[ryancdotorg]

@ryancdotorg FYI this PR implements your NameTrade non-interactive scheme.

neat

[JeremyRand]

@domob1812 @yanmaani note that I'd like Namecoin Core to wind up with the same RPC API as Electrum-NMC for this (modulo inherent differences between Core and Electrum), so if there are any issues with this API that might block it from being merged to Namecoin Core, please bring those up before this is merged.

[JeremyRand]

Note that if this PR is approved, it will be applied to 4.0.4 4.0.6 (which branched after this PR was created), not 4.0.2.

[yanmaani]

Notes on review:

• I did not attempt to run the code or the tests, whether to check if it gave an error or to check that it gave correct results.
• I did not review anything in transaction.py, because I do not have the competence for doing so.

First of all, as for the implementation:

1. Amount comparison shouldn't be equality; if someone offers to buy my name for $100, selling it for $99 should be OK (as long as I get the extra $1), but obviously not the other way around. Nobody would complain about free money, and matching it down to the satoshi while inputting it as a float seems unpleasant.
2. How do you handle coin locking? If I offer to buy two names for 1 NMC each, will the offers take the same coin input (mutually exclusive), or will it pick two separate inputs (locking coins in wallet)? (I think that the best would be the second)
3. Is the money calculation correct for all cases? In particular, is it net or gross of fees? (Selling a name for $100 to lose $99 in fees isn't expected behaviour, to me.) Doesn't Electrum have a method like pwallet->GetCredit?
4. While checking an offer we're accepting, why check outputs of buy offers or inputs of sell offers? That's strictly the counterparty's problem, and it makes the logic harder to follow. (As regards to checking that they're not enriching themselves on the change, I would feel much better if it were possible to check the balance of the transaction instead by such a method as described above.)
5. I don't see anything strictly wrong with the implementation, but it's very long and difficult to review. Would it be possible to make it shorter somehow, or to factor out some redundant code into helper functions? (I understand if that's difficult, since it's basically a long series of imperative operations)
6. The integration tests are lacking. In particular, it would be good with some tests to check that it won't accept invalid inputs etc. (In Namecoin Core, I think tests are supposed to trigger all possible error messages and failure modes? Do we have this for Electrum-NMC too?). I don't want to be too pedantic here, but I'm kind of concerned is because reviewing all of this by hand and reasoning about all the possible failure modes in your head is not exactly trivial. (Not that it's anything wrong with your code, but it sort of has to be a very long series of imperative business logic mixed in with piles and piles of boilerplate.)

Now on to the higher-level stuff.

As you say, it's of great importance that Namecoin Core and Electrum-NMC are consistent here, both in respect of:

1. Having similar enough interfaces
2. Having compatible offers

As for the interface, the API that I've written some (unfinished) code¹ for is:

{"name", RPCArg::Type::STR, RPCArg::Optional::NO, "The name to buy"},
{"amount", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "How much they'd get for it"},
{"offer", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED_NAMED_ARG, "An existing offer to accept (leave blank to create new)"},

In this respect, the code is very similar but not fully the same. In particular, why do you do the following?

     async def name_buy(self, identifier, ..., amount=0.0):
     ...
     if amount == 0.0:
         raise Exception("Must specify amount")

Why not just make amount a required param? Is there some internal Electrum-NMC API thing going on here?

I've also thought it'd be very useful to be able to specify extra options, in particular a locktime. Either you'd have one option for each such "gadget", or there could perhaps be a singular option that added transaction flags. What do you reckon would be best? (This is not a blocker, but would be very nice to have soon-ish so you can do auctions etc)

As concerns the second point, would it be possible to have unit tests that load specific private keys and ensure that the offers generated are bit-for-bit identical? If not, I think it would at least be reasonable to throw in some test vector offers and make sure that they can be accepted.

Footnotes

1. If memory serves me right, it's blocked because of some cursed issue about the coin selection, same as the second point in the review comments above, but I'm not entirely sure. ↩

[domob1812]

From a quick glance (I don't have time for more at the moment and in the forseeable future) the API looks fine to me (and pretty straight-forward). Have you thought about using PSBTs instead of raw transactions, though?

[JeremyRand]

Good review, thanks.

Amount comparison shouldn't be equality; if someone offers to buy my name for $100, selling it for $99 should be OK (as long as I get the extra $1), but obviously not the other way around. Nobody would complain about free money, and matching it down to the satoshi while inputting it as a float seems unpleasant.

@yanmaani Electrum commands use Decimals, not floats. So there shouldn't be any issues with rounding. The intent of doing the equality check is to avoid surprises, even if they are surprises that we expect the user to find pleasant. (I can imagine scenarios in which the two parties are friends or otherwise have an existing relationship, in which case one party might want to notify the other party if they accidentally fat-fingered a price and are selling a name for much less than intended.) That said, I think it would be reasonable to modify the Exception messages to explicitly say what the Offer amount is and what the user-entered amount is, so that users have a little more context if there's a mismatch.

How do you handle coin locking? If I offer to buy two names for 1 NMC each, will the offers take the same coin input (mutually exclusive), or will it pick two separate inputs (locking coins in wallet)? (I think that the best would be the second)

The commands don't handle coin locking, whether by marking coins as spent or frozen. This is consistent with all other Electrum commands. E.g. if you run name_update (or payto) twice in a row, it will double-spend as well. Electrum's API expects the user to freeze coins, broadcast the transaction, or add the transaction to the wallet as needed. The GUI (which is written but not in this PR; it will be present in the next version of this PR) instructs users that they can revoke a Buy Offer by double-spending it, so to some extent we want it to be possible to invalidate these Offers. I think it would be reasonable for the GUI to offer to automatically freeze the user-contribued inputs to an Offer (and add a string to the input's label so that the GUI makes it clear why it's frozen), since other GUI elements (unlike the RPC commands) typically do not expect the user to manually avoid double-spending.

Is the money calculation correct for all cases? In particular, is it net or gross of fees? (Selling a name for $100 to lose $99 in fees isn't expected behaviour, to me.) Doesn't Electrum have a method like pwallet->GetCredit?

Standard convention in Electrum's RPC API is that the user specifies fees separately from the amount, via the fee and feerate parameters. If the user's default fee rate is sane, they won't incur massive fees unless the transaction is massive. Since the Offer only contributes one input and one output, this is most likely not going to be caused by a malicious Offer, and in any event the user can glance at the transaction size before they broadcast it. If the user wants to specify a custom fee rate or a custom absolute fee, they can do so; this is not considered part of the amount (just like in the rest of the Electrum RPC API).

While checking an offer we're accepting, why check outputs of buy offers or inputs of sell offers? That's strictly the counterparty's problem, and it makes the logic harder to follow. (As regards to checking that they're not enriching themselves on the change, I would feel much better if it were possible to check the balance of the transaction instead by such a method as described above.)

I don't want to try to process an Offer that's in an unexpected form, so I do as much validation as possible before I process it. It's not clear to me that all of these are security-critical, but it seems safer this way.

I don't see anything strictly wrong with the implementation, but it's very long and difficult to review. Would it be possible to make it shorter somehow, or to factor out some redundant code into helper functions? (I understand if that's difficult, since it's basically a long series of imperative operations)

I tried briefly to abstract the buy and sell commands into a metacommand, but it turned out to be harder to read. I think the code that parses the amount of the Offer might be able to use some helper functions that Electrum already has; last I looked most of those helper functions don't support name operations properly, but maybe that can be improved.

The integration tests are lacking. In particular, it would be good with some tests to check that it won't accept invalid inputs etc. (In Namecoin Core, I think tests are supposed to trigger all possible error messages and failure modes? Do we have this for Electrum-NMC too?). I don't want to be too pedantic here, but I'm kind of concerned is because reviewing all of this by hand and reasoning about all the possible failure modes in your head is not exactly trivial. (Not that it's anything wrong with your code, but it sort of has to be a very long series of imperative business logic mixed in with piles and piles of boilerplate.)

Namecoin Core has an atomic trade functional test; the tests in this PR follow a similar workflow (though it's simpler because this API is higher-level than what Namecoin Core has). I'm not opposed to adding extra checks that Namecoin Core's test doesn't have.

Why not just make amount a required param? Is there some internal Electrum-NMC API thing going on here?

amount is an optional parameter because I copied the function declaration from another name command and neglected to change it at the time, and then forgot I could change it when I added the check. I will make it required. Good catch. I will also maybe change amount to requested_amount for name_sell so that the help text is correct (amount's help text says "Amount to be sent", which is wrong for a command that receives that amount).

I've also thought it'd be very useful to be able to specify extra options, in particular a locktime. Either you'd have one option for each such "gadget", or there could perhaps be a singular option that added transaction flags. What do you reckon would be best? (This is not a blocker, but would be very nice to have soon-ish so you can do auctions etc)

Standard Electrum convention is to use separate parameters for things like locktime, while I think Bitcoin Core tends to use an options dict (I think they might be transitioning to Electrum's approach though?). I'm following Electrum's convention here; Namecoin Core can follow Bitcoin Core's convention. locktime is already supported in this PR (though I didn't test it), so you could theoretically rig a script to do auctions with it already.

As concerns the second point, would it be possible to have unit tests that load specific private keys and ensure that the offers generated are bit-for-bit identical? If not, I think it would at least be reasonable to throw in some test vector offers and make sure that they can be accepted.

I know the unit tests can load a specific seed and check that transactions match exactly. I'm not sure how easy it is to do that with RPC commands. It's probably not very hard; I will look into it.

From a quick glance (I don't have time for more at the moment and in the forseeable future) the API looks fine to me (and pretty straight-forward). Have you thought about using PSBTs instead of raw transactions, though?

@domob1812 I asked the Bitcoin Core guys about this, and they said that PSBT is the wrong format for this use case. PSBT exists for use cases where a given scriptSig needs collaboration from multiple users in order to construct (e.g. a multisig scriptSig); the network serialization format of transactions doesn't have any defined way to include empty or partially-constructed scriptSigs. The use case in this PR doesn't have that issue; each scriptSig is fully defined by one user, so there is no circumstance where empty or partial scriptSigs have to hit the wire. The fact that different scriptSigs are written by different users doesn't change that. Given this, the Bitcoin Core guys told me that the standard network serialization format used by the raw transaction API is the correct way to do this.

Given that @domob1812 is mostly occupied, I'll assume that code review from @yanmaani is sufficient, unless Daniel registers an objection to something.

[yanmaani]

@yanmaani Electrum commands use Decimals, not floats. So there shouldn't be any issues with rounding.

That's true if you enter it as a Decimal, as opposed to calling it directly with a float.

The intent of doing the equality check is to avoid surprises, even if they are surprises that we expect the user to find pleasant. (I can imagine scenarios in which the two parties are friends or otherwise have an existing relationship, in which case one party might want to notify the other party if they accidentally fat-fingered a price and are selling a name for much less than intended.) That said, I think it would be reasonable to modify the Exception messages to explicitly say what the Offer amount is and what the user-entered amount is, so that users have a little more context if there's a mismatch.

I am not convinced that this is a good idea, and it will lead to an inconsistent API between the two versions. At the very least, the error messages should say whether it's too much or too little, and these should be two separate error conditions/types. If the price is too much, I would strongly suggest, if the established practice for error messages permits it, that the message explicitly suggests to lower your price to the asking price. (Also, see below)

The commands don't handle coin locking, whether by marking coins as spent or frozen. This is consistent with all other Electrum commands. E.g. if you run name_update (or payto) twice in a row, it will double-spend as well. Electrum's API expects the user to freeze coins, broadcast the transaction, or add the transaction to the wallet as needed. The GUI (which is written but not in this PR; it will be present in the next version of this PR) instructs users that they can revoke a Buy Offer by double-spending it, so to some extent we want it to be possible to invalidate these Offers. I think it would be reasonable for the GUI to offer to automatically freeze the user-contribued inputs to an Offer (and add a string to the input's label so that the GUI makes it clear why it's frozen), since other GUI elements (unlike the RPC commands) typically do not expect the user to manually avoid double-spending.

OK, I think this is fine. Maybe it should be an option - we might want some offers to be mutually exclusive - but that's for a future GUI patch.

Anyway, you should probably document these decisions explicitly in the code.

Standard convention in Electrum's RPC API is that the user specifies fees separately from the amount, via the fee and feerate parameters.

Bitcoin Core does that too, no?

If the user's default fee rate is sane, they won't incur massive fees unless the transaction is massive.

Not true. Consider Bitcoin - they had $50/txn fees at some points in time. If such fees happen on Namecoin, users will have to pay very big sums compared to their potential gain, even if the tx is small.

Since the Offer only contributes one input and one output, this is most likely not going to be caused by a malicious Offer, and in any event the user can glance at the transaction size before they broadcast it. If the user wants to specify a custom fee rate or a custom absolute fee, they can do so; this is not considered part of the amount (just like in the rest of the Electrum RPC API).

If so, why would you have the "exact amount" API? If users are trading with their friends and they fat-finger the amount, then surely by the same measure they should just be able to review before sending?

IMO, including footguns is always a bad idea, even if people have the option of double-checking. It's still not great if people can get that badly punished on fees.

Especially for the case of selling names, I also don't see how it's consistent with current practice. When you send a payment request for 1 mBTC, you're implicitly asking the payer to pay 1.01 mBTC or whatever the fees are, no? So "fees are on top of the amount" can only logically apply for the buyer. (That said, if he is the maker, then the taker=seller will set the fees anyway, and so since this only applies for taker=buyer, it seems better to go "all the way" to be consistent on both sides and across implementations.)

I don't want to try to process an Offer that's in an unexpected form, so I do as much validation as possible before I process it. It's not clear to me that all of these are security-critical, but it seems safer this way.

I'm not sure that this is a great idea, since it makes the function harder to read. At the very least, why not put the not-strictly-necessary checks into a separate function?

I tried briefly to abstract the buy and sell commands into a metacommand, but it turned out to be harder to read. I think the code that parses the amount of the Offer might be able to use some helper functions that Electrum already has; last I looked most of those helper functions don't support name operations properly, but maybe that can be improved.

What about writing a tiny helper function that calculates net loss/gain from a tx, and using it only for this code?

Namecoin Core has an atomic trade functional test; the tests in this PR follow a similar workflow (though it's simpler because this API is higher-level than what Namecoin Core has). I'm not opposed to adding extra checks that Namecoin Core's test doesn't have.

Yes, but Namecoin Core doesn't have a high-level API for atomic name trades (yet!). So the test should be much more advanced, since the Core test just does basic smoke testing to ensure this is a supported feature.

As far as I can see, these are the possible cases to test:

• Pecuniary issues:
  • Fails if you try to overpay for a name
  • Fails if you try to underpay for a name
  • Fails if you try to sell a name for too little
  • Fails if you try to sell a name for too much
• Transaction issues:
  • Fails if there's too many inputs/outputs
  • Fails if (non-)name outputs/inputs in the wrong places
  • Fails if there's the wrong SIGHASH flags
  • Fails if wrong name identifier
• Electrum-specific issues:
  • Fails if name was updated too recently
• API:
  • Fails if extra outputs while creating trade offer
  • Fails if amount not spec'd (not needed if non-optional arg)
• Normative behaviour:
  • Does not fail while accepting valid (fresh) transaction
  • Does not fail while accepting hardcoded transaction
    • Unit test to check that fresh transactions equal known good value for test vector key

Are Electrum tests also supposed to be 100%?

amount is an optional parameter because I copied the function declaration from another name command and neglected to change it at the time, and then forgot I could change it when I added the check. I will make it required. Good catch.

Great!

I will also maybe change amount to requested_amount for name_sell so that the help text is correct (amount's help text says "Amount to be sent", which is wrong for a command that receives that amount).

It's not possible for amount to have different help text for the two commands? If not, then I think it'd make more sense to change them both (e.g. paid_amount vs got_amount or sent_amount vs received_amount or offered_amount vs requested_amount)

Standard Electrum convention is to use separate parameters for things like locktime, while I think Bitcoin Core tends to use an options dict (I think they might be transitioning to Electrum's approach though?). I'm following Electrum's convention here; Namecoin Core can follow Bitcoin Core's convention. locktime is already supported in this PR (though I didn't test it), so you could theoretically rig a script to do auctions with it already.

Sounds good.

I know the unit tests can load a specific seed and check that transactions match exactly. I'm not sure how easy it is to do that with RPC commands. It's probably not very hard; I will look into it.

Great!

[domob1812]

Ok, makes sense - in this case, the transaction is collaborative, but not the individual signatures. Concept ACK from my side, but yeah, I won't be able to review the Python code (and even if I were, I'm not an expert in Python and/or Electrum anyway).

[JeremyRand]

Merged to 4.0.6 branch per consensus with @yanmaani. Leaving this PR open to track subsequent improvements from his review.