Skip to content
This repository has been archived by the owner on May 13, 2022. It is now read-only.

use maker input pubkey instead of coinjoin pubkey for authorization #90

Closed
wants to merge 3 commits into from

Conversation

AdamISZ
Copy link
Member

@AdamISZ AdamISZ commented May 25, 2015

Elementary testnet testing (1 maker) passed OK.
Obviously this PR, being a breaking change, will have to wait until an opportune moment.
Also, it will need more testing.

@AdamISZ
Copy link
Member Author

AdamISZ commented May 25, 2015

Addresses #88

@chris-belcher
Copy link
Collaborator

Good job.

The abstract functions in message_channel.py should be changed too.

Now for actually merging this.
One way is to merge it right now while the project is small so it's less hassle.
Another way is to wait until more features like #45 especially minimum maker count, and then ask everyone to update.
Another way is to create another command different from !ioauth and code takers to understand both, a bit like a soft fork from bitcoin. It would take much more code for this, which would have to stay in the codebase for a long time after.

Would appreciate opinions on the best way to do this. I'm leaning towards code the minimum-maker-count feature and release ASAP.

@chris-belcher
Copy link
Collaborator

Further point, if we're changing the authentication it might be worth fixing this issue at the same time #83

@AdamISZ
Copy link
Member Author

AdamISZ commented May 26, 2015

Thanks for the abstract method catch. Fixed.

Re: #83 , it seems like one solution might be: replace

auth input_utxo_pubkey btc_sig

with

auth [input_pubkey1,input_pubkey2,..,M] btc_sig

where it's understood that if there is more than 1 pubkey, it's a multisig, and the maker should read the last entry in the list as M in M of N. Then he can verify the multisig address from these pubkeys and also verify that btc_sig is signed by the (first?) of the pubkeys in the list. A list of only one input_pubkey is read as meaning an ordinary non-multisig.

@chris-belcher
Copy link
Collaborator

That sounds good.

btc_sig should be checked against any pubkey in the list because the taker's pubkey may happen to not be the first one.

Only issue I can think is there may be other kinds of p2sh scripts that are not multisig. Like some exotic contract.
I can't think of any situation where the script doesn't contain some public key and signature, otherwise the miner could take the same input and modify the output to steal the money. So what if the auth command has the entire scriptPubKey in it, the maker parses it for any pubkeys and the signature has to verify with one of them.
We have to ask whether this is worth it, all p2sh usage today is some kind of multisig. Almost everything on the bitcoin wiki contracts page is based on multisig and for the stuff that isn't it's not clear whether it actually requires coinjoin privacy.

@AdamISZ
Copy link
Member Author

AdamISZ commented May 26, 2015

I can do it now but I think it's worth looking at support-multisig-input as a "soft fork", because when it's introduced it will not break older clients/bots (according to the syntax above, let's say).

EDIT: sorry, I wasn't thinking clearly. This isn't true, if a taker sends a new-style auth it breaks the current bots. Hmm.

Whereas the changes currently in this PR are a "hard fork", breaking everyone's auth mechanism. So combine with #29 let's say (but even then, just make separate PRs but merge at the same time), plus any other breaking changes you want to do.

As for more exotic contracts, I don't think it's pressing. Re: check all pubkeys, definitely, yes, we already do something similar to that anyway.

@chris-belcher
Copy link
Collaborator

I think any "hard fork" should happen sooner rather than later. It gets more disruptive as time goes on because more users need to update.

It's nowhere near as disruptive or dangerous as a bitcoin hard fork. No value is at risk, it just means the two kinds of bots cannot communicate. The liquidity is split in two but there's so little of that right now it's not too much of a loss. The community is small and interested. So I'd say do a hard fork soon, I'll work on the minimum maker count feature.

@AdamISZ
Copy link
Member Author

AdamISZ commented May 26, 2015

OK, seems like a good point of view. I can do the multisig thing and include it in this PR if you like.

@chris-belcher
Copy link
Collaborator

Yes please.

@chris-belcher
Copy link
Collaborator

At the same time as this change, we could add a field to later implement this idea by theymos

In exchange for a (comparatively large) extra fee, takers could require that the unspent-outputs provided by makers be x blocks deep (I'm thinking ~1500). This reduces the Sybil risk because an attacker will only have so many bitcoins, and this requirement ties up a lot of their bitcoins for a while if a lot of takers are routinely requiring that at least some of their partners provide old bitcoins.

The extra field announces a block height of the UTXOs, with zero meaning don't know/don't care.

EDIT: actually announcing block height incentivizes bots to announce new and higher fees at each new -blocknotify. That would be quite spammish. Maybe the added parameter could be extra-fee-per-block-from-blockheight. Which is an extra fee added on top for every new confirmation of a utxo.

That assumes the market price goes up linearly which might not be true. Maybe the parameter should be a simple mathematical expression. For example 200_h meaning 200 extra satoshi for every block height. e^(h/200) or 200_h**2 for a fee which goes up faster than linearly by block height.

@AdamISZ
Copy link
Member Author

AdamISZ commented May 28, 2015

fd76aa6 allows the maker to receive multisig address inputs in the !auth message and verifies the signature provided matches one of the given pubkeys. It also sanity checks the multisig parameters (M and N) and checks (as before) that the claimed multisig appears in the transaction.

What I haven't done yet is address what is going on on the taker side (it seems still a bit of an open question how the taker is going to create the multisig signatures as you described in #83). I was toying with the idea of putting it in crudely, via entries on the command line, but it seemed there's still too many open questions there.

All I have tested is a normal transaction in joinmarket-pit-test to make sure that standard transactions still work correctly.

@chris-belcher
Copy link
Collaborator

I'll make a git fork which will have all the commits of the new protocol. Ready to be merged into master one day after everything works.

For the taker side we'll have a new script called raw-tx.py or something, which returns a partially unsigned transaction hex that the user has to copypaste to whoever has the private keys to get them signed. It will also work for normal p2pubkeyhash, although the the user has to feed in a pubkey for the encryption

@chris-belcher
Copy link
Collaborator

created branch newprotocol and merged this in

@adlai
Copy link
Contributor

adlai commented Jun 1, 2015

How about using a different order type for the new protocol? That lets people "soft fork" while outdated clients will simply ignore the new type.

@chris-belcher
Copy link
Collaborator

What are the advantages of that? With this way the clients also ignore each other.

@chris-belcher
Copy link
Collaborator

I see what you mean.

Yeah I guess the main reason is I don't feel like programming that up. Seems easier to just ask everyone to update

@chris-belcher
Copy link
Collaborator

Clarification:
https://github.com/chris-belcher/joinmarket/blob/newprotocol/lib/maker.py#L69 This should be range(2, 17) to allow M-of-2 multisig

Or even range(1, 17) to allow 1-of-1 multisig in a future where we all use p2sh for everything

@AdamISZ
Copy link
Member Author

AdamISZ commented Jun 3, 2015

No, len(pubkeys) is 1 longer due to format [pubkey1,pubkey2, .. M] as I wrote in the changes to the *txt file.

As for 1 of 1 I also briefly considered it but decided against it - but sure, OK.

@chris-belcher
Copy link
Collaborator

I've become increasingly aware that spending from hardware wallets and cold storage, as in http://www.reddit.com/r/joinmarket/comments/36xyw5/any_way_of_doing_multisig_or_hw_wallets_like_the/ cannot be done with the current auth protocol because the private key is needed. People who keep private keys offline won't want to transfer them to an online computer only to use them to authenticate the encrypted stream.

What do you think of optionally allowing takers to not authenticate their side? What are the cryptographic implications of only the maker authenticating himself? Of course most of the time the taker will authenticate, only in the multisig or hardware wallet spending they wont.

Could be implemented by the taker sending Null for the signature, which means the maker skips the ecdsa verifying stuff.

@AdamISZ
Copy link
Member Author

AdamISZ commented Jun 4, 2015

Given this perspective, I am struggling to find a very good argument for keeping the authentication to btc signatures, at least on the taker side. One other possibility (apart from just removing it) is to replace the signature to the input_utxo_pubkey with a signature to some chosen as-persistent-as-you-like bitcoin address. In this I am remembering theymos' comment:

  • Instead of requiring NickServ registration, makers could generate an identity separately (maybe just a Bitcoin address) and communicate it on the IRC channel using public-key crypto. This is more convenient and will work across multiple IRC servers.

On the maker side, it feels like a stronger argument to insist on some sort of verification of holding the claimed funds (although, as currently coded, the maker only attests to one small chunk of coin). Then again, this works against the request we're getting to be able to have non-hot-wallet maker functionality. I wonder if that is just asking too much.

This all seems quite difficult.

@chris-belcher
Copy link
Collaborator

If one side authenticates it makes a MITM much more difficult. Since by design the maker always has immediate access to his private key, it makes sense to use it for authenticating as well.
So from the point of view of takers, they are always talking to the owner of the UTXOs they are about to join with.

I think theymos' idea was meant to be related to #57 Using an identity related to a bitcoin address is equivalent to trust-on-first-use, which is slightly worse than using the UTXO pubkey.

If there's no serious implications of taker authentication being optional, I'll code it up later.

@AdamISZ
Copy link
Member Author

AdamISZ commented Jun 4, 2015

But it's a bit more complicated than that because, don't forget, we are using libnacl for authenticated encryption to an ephemeral pubkey per transaction. So we do have MITM protection during any particular transaction with all counterparties.

TOFU - well, not necessarily, in as much as bitcoin keys could be used in a WoT a la bitcoin-otc. Using just a random bitcoin key without any persistence would be a pointless add-on to the authentication we already have in libnacl.

@chris-belcher
Copy link
Collaborator

Sorry, a bit more complicated than what?

So libnacl already has authentication? How can it do that without distributing keys somehow? I thought in TLS the certificate authorities are used for distributing the correct keys, and in JoinMarket so far we use the UTXO pubkey instead.

OK yes agreed, TOFU is not the only way to use bitcoin-address-as-identity.

@AdamISZ
Copy link
Member Author

AdamISZ commented Jun 4, 2015

We generate keypairs on the fly for each new counterparty, for each new transaction. Then a session key is generated using DH key exchange. Part of that session key is used to create MACs for authentication, using Poly1305. This verifies that only one entity is conducting the entire exchange, as well as ensures the integrity of the messages. That's why I qualified with "during any particular transaction with all counterparties. " Obviously it does nothing to address the question of who you're starting your conversation with, only that it stays the same. But it's still classed as "authenticated encryption".

Upshot of all this, I see no reason that optionally removing the btc sig on taker side would cause any problems right now. Maybe on IRC we can discuss and come to a decision about where we see the whole "persistent identity" thing going.

@chris-belcher
Copy link
Collaborator

Okay, understood. Interesting subtle differences. :)

I'll come on IRC a little later tonight.

@chris-belcher chris-belcher mentioned this pull request Aug 3, 2015
@AdamISZ AdamISZ closed this Nov 20, 2015
@AdamISZ AdamISZ deleted the encryption2 branch November 20, 2015 13:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants