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

Implemented micropayment channels for counterparty assets. #944

Open
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
4 participants
@F483
Copy link
Member

F483 commented Nov 15, 2016

All commits needed have been squashed, see micropayment_channels branch for an unsquashed version and added micropayment code has 100% coverage. Note that this may contain some changes from @rubensayshi, if he has not already merged them.

Api documentation can here and protocol documentation can be found here, a PR for the documentation has been opened.

New api calls start with mpc_ and micropayment code is in counterpartylib.lib.micropayments

Additionally I added an api call "sendrawtransaction" which pipes through to bitcoind, this helps a lot with mitigating race conditions in clients.

I also moved much of the signing/script code to its own python libray (github, pypi) so client that need it will not have to depend on counterpartylib and its extensive dependencies. This should belong to counterparty and I will gladly transfer ownership.

I will of course maintain this code and make any changes needed so this patches is accepted.

@F483

This comment has been minimized.

Copy link
Member Author

F483 commented Nov 15, 2016

@rubensayshi this PR is now against develop and I also created a PR for the documentation as well.

@rubensayshi

This comment has been minimized.

Copy link
Member

rubensayshi commented Nov 20, 2016

I'm gonna keep all discussion in this PR and avoid commenting in the documentation PR except for inline nits to avoid having a discussion split amongst multiple PRs.

first off; damn nice job, everything looks very well thought through and coded up.


A thing that took me a moment to realize is that these payment channels differ from a "traditional" bitcoin payment channel because of the lack of change in the counterparty protocol.
I think you should add a bit of explanation about this in the documentation to make it easier / quicker to understand for other readers.

The commit TX only contains the send to the payee, the remaining "change" is left behind in the deposit and is eventually redeemed by the payer in the Change TX using the spend secret.
This is afaik in contrary to most traditional bitcoin payment channels where the "pay output" and change output are both part of the "Commit TX".


Another thing that I think would be good to clarify is that reusing a payment channel is impossible due to how Counterparty works (if you can do 1 spend from an P2SH address you can spend all assets in that address) and that the code has a safeguard in there.
Considering the spend secret of a new payment channel should be a new (randomly generated) secret this should problem should never occur though even when the payee and payer use the same pubkeys again.
But I think it would just be good to clarify this "limitation" in the docs ;)


It's unclear to me what the purpose is of the revoke TX.
When does is "Return Funds" scenario played? is this for when for w/e (external) reason the the payee decides the payer doesn't owe him anything?


Can you elaborate on the default of 2 blocks delay time that allows the payer to use the revoke secret? is 2 blocks a sensible delay?


I think the docs should elaborate a bit more on choosing the expire_time of the deposit TX and how a margin of error is necessary even when the time of how long the channel will stay open is known to keep a safe margin for blocks being found quickly etc. considering as soon as the expire is hit the payer to undo the whole transaction.


Again something should be documented is the fee that is required for the various TXs that can follow the deposit TX.
You have the payer provide enough BTC for 3x send TXs (of 500 bytes int(fee_per_kb / 2)).

... continueing on this submit later ;-) ...

@F483

This comment has been minimized.

Copy link
Member Author

F483 commented Nov 21, 2016

It's unclear to me what the purpose is of the revoke TX.

The commit tx will determine the funds transferred, to transfer additional funds a new commit tx with a higher amount is created. Funds can also be returned by the payee if they reveal the revoke secret. This effectively makes it impossible for the payee to publish revoked commits, as the payer can recover all funds using the revoke secret. This is accomplished by the commit delay time (default two block) that the payee has to wait until they can recover the funds using a payout tx.

Hope this clears things up a bit, documentation never was my strong point. I will go through the documentation and try to clear up what is going on.

@F483

This comment has been minimized.

Copy link
Member Author

F483 commented Nov 29, 2016

Added optional 'spend_secret' option to 'mpc_recoverables'. This enables the payer to recover the change in the case where no funds were transferred and the payee kindly provided the spend secret so the payer does not have to wait for the channel to expire.

@rubensayshi rubensayshi force-pushed the CounterpartyXCP:develop branch from 7d6424b to daa7ec7 Dec 9, 2016

@rubensayshi

This comment has been minimized.

Copy link
Member

rubensayshi commented Dec 9, 2016

hmm I think a check should be added for the payee to make sure the commit TX fee is enough and the extra BTC provided to pay for the fee of the payout TX is enough.

a low fee on the commit TX could allow the payer to use his expiration TX if the commit TX won't get confirmed.

and the 2nd check is mostly just to avoid having to go though the hassle of construction a TX with additional inputs to pay for the fee.

validate.is_hex(secret)

# update state
list(map(lambda s: _revoke(state, s), secrets))

This comment has been minimized.

@rubensayshi

rubensayshi Dec 9, 2016

Member

NIT; please change to normal loop instead

This comment has been minimized.

@F483

F483 Jan 9, 2017

Author Member

Done, not sure why I wrote it that way in the first place :/

@rubensayshi

This comment has been minimized.

Copy link
Member

rubensayshi commented Dec 9, 2016

code looks good, we'll need to adopt that separate package into the Counterparty org when we're ready to merge it

@F483

This comment has been minimized.

Copy link
Member Author

F483 commented Jan 9, 2017

@rubensayshi I'm guessing the seperate package you mean is micropayment-core. If you want to adopt into counterparty org now is the best time to make any renaming or such. I will continue to support it and transfer the pypi package ownership if possible/desired/needed.

The reason the code is in a separate package, is so a client doesn't need all the counterparty lib dependencies.

@F483

This comment has been minimized.

Copy link
Member Author

F483 commented Jan 10, 2017

@rubensayshi request feedback

I think a check should be added for the payee to make sure the commit TX fee is enough and the extra BTC provided to pay for the fee of the payout TX is enough.

Generally a good idea I think, but I'm not sure if we need a changed API for this. It seems that this is a higher level client issue. Checking the fee is simple in most bitcoin libs, as not to need an extra call. I will try change my client to check tx fees and likely prefer using pycoin calls over any cp API call anyway.

a low fee on the commit TX could allow the payer to use his expiration TX if the commit TX won't get confirmed.
and the 2nd check is mostly just to avoid having to go though the hassle of construction a TX with extra inputs to pay for the fee.

Adding more inputs doesn't help with the commit tx, it may take a long time for the channel to settle and the fee may be to low by that point.

This is different for the payout. Constructing the payout tx with more inputs may be a good extension to the API (so far as counterparty allows for extra btc inputs/outputs).

@unsystemizer

This comment has been minimized.

Copy link
Contributor

unsystemizer commented Jan 10, 2017

Checking the fee is simple in most bitcoin libs, as not to need an extra call.

#952 can ensure that good estimates can be obtained "natively", as long as Bitcoin Core estimates work properly.
I'm running one counterparty-lib instance (and picopayments) with this patch, but I haven't used this particular enhancement yet.

@F483 F483 force-pushed the F483:micropayments branch 3 times, most recently from 3f048b5 to f822b7c Feb 24, 2017

app = flask.Flask(__name__)
auth = HTTPBasicAuth()

@dispatcher.add_method
def shutdown(secret):
assert self.shutdown_secret and secret == self.shutdown_secret

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

if the wrong secret is provided this will crash the server

This comment has been minimized.

@F483

F483 Mar 22, 2017

Author Member

Good catch!! This likely exists in another branch as well!!

I think this may have come from another branch I depended upon that has not been merged into dev yet. @rubensayshi do you know more about this?

def stop(self):
util.api('shutdown', {'secret': self.shutdown_secret})

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

can you just call self.shutdown directly here, or does the dispatch decorator prevent that?

This comment has been minimized.

@F483

F483 Mar 22, 2017

Author Member

I cant say. I think this may have come from another branch I depended upon that has not been merged into dev yet. @rubensayshi do you know more about this?

@@ -382,21 +384,34 @@ def run(self):

class APIServer(threading.Thread):
"""Handle JSON-RPC API calls."""
def __init__(self):
def __init__(self, db=None):
self.db = db

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

can you explain the reason for adding self.db vs the way it was done before? (I will probably find out as I review, but any pointers here are helpful...e.g. if it is a design decision, vs something you had to do due to a runtime issue with the picopayments enhancements)

This comment has been minimized.

@F483

F483 Mar 22, 2017

Author Member

I cant say. I think this may have come from another branch I depended upon that has not been merged into dev yet. @rubensayshi do you know more about this?

This comment has been minimized.

@F483
app = flask.Flask(__name__)
auth = HTTPBasicAuth()

@dispatcher.add_method
def shutdown(secret):

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

also, what is the use-case for the new shutdown method?

This comment has been minimized.

@F483

F483 Mar 22, 2017

Author Member

I cant say. I think this may have come from another branch I depended upon that has not been merged into dev yet. @rubensayshi do you know more about this?

This comment has been minimized.

@F483
import pycoin
from . import validate
from . import exceptions
from micropayment_core import util

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

is this file part of the PR? I don't see it (was looking for util.load_tx() specifically and couldn't find the file)

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

you might also want to import this file as mpc_util instead of util as there is another util.py and whenever I see util. I am automatically thinking it's something in util.py....

@@ -764,6 +779,255 @@ def handle_root(args_path):
# Not found
return flask.Response(None, 404, mimetype='application/json')

@dispatcher.add_method
def sendrawtransaction(tx_hex):

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

given the call to validate.tx_signature, is this function specific to micropayments? if so, I'd prefix it with mpc_?

This comment has been minimized.

@F483

F483 Mar 23, 2017

Author Member

I only added the mpc_ prefix for the added api calls. Internally all micropayment code resides in the counterpartylib.lib.micropayments and is imported explicitly.

This comment has been minimized.

@F483

F483 Mar 23, 2017

Author Member

This call specifically will validate any bitcoin transaction, so it may be better to move it out of the micropayments module.

@dispatcher.add_method
def mpc_make_deposit(asset, payer_pubkey, payee_pubkey,
spend_secret_hash, expire_time, quantity):
""" Create deposit and setup initial payer state.

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

Probably remove these docstrings, as the other API methods don't have them, and it's redundant with what's inAPI.md.

Also, please check your docs PR at https://github.com/CounterpartyXCP/Documentation/pull/123/files and make sure it is up to date, thanks!

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

(alternatively, we could add docstrings to all API methods in this file, but I'm not convinced of that path, as it's redundant with API.md and I'd rather keep things simple...unless we added the docstrings and made a script that created API docs from them dynamically)

"topublish": str # Unsigned deposit rawtx.
}
"""
netcode = "XTN" if config.TESTNET else "BTC"

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

due to the fact this line is being repeated numerous times, this might be better as a constant either in this file (APIServer?) or in a micropayments_core.config or micropayments_core.util...

}
"""
netcode = "XTN" if config.TESTNET else "BTC"
return micropayments.create_commit(

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

may be an inconsistency of how you are doing your spacing with these function calls.... is your IDE doing this? (above you have the first func parameter on the same line, and the closing parenthesis on the same line as the last function parameter...thought it might be the column widths, but the docstrings exceed those)

import binascii
from Crypto.Cipher import ARC4

def init_arc4(seed):

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

if this is all this file will ever contain, I'd say to throw it in util.py

This comment has been minimized.

@F483

F483 Mar 22, 2017

Author Member

That's odd I don't remember adding or needing this. I must have picked it up with some merge against another branch I needed. My guess is that the other branch has not been merged into dev yet. @rubensayshi do you know more about this code?

@@ -166,7 +166,7 @@ def is_scriptpubkey_spendable(scriptpubkey_hex, source, multisig_inputs=False):
class MempoolError(Exception):
pass

def get_unspent_txouts(source, unconfirmed=False, multisig_inputs=False, unspent_tx_hash=None):
def get_unspent_txouts(source, unconfirmed=False, unspent_tx_hash=None):

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

good catch...

return extract_addresses_from_txlist(tx_hashes_tx, getrawtransaction_batch)


def extract_addresses_from_txlist(tx_hashes_tx, _getrawtransaction_batch):

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

can we put this helper in util_test.py? I'd rather not clutter non-test parts of the code with test helpers

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

or rather, just have one method here that takes a default arg, e.g. _getrawtransaction_batch=getrawtransaction_batch ...still a bit warty but better than two separate methods I think... keeping the docstring you have

This comment has been minimized.

@F483
750000: {'ledger_hash': '955fb9d714eb6c05e3d64f05ddb2f2ff18423e1e16c05dfc6aea8838ce71b807', 'txlist_hash': '54f06faab3e94d30fda98df2a95f3f0a7ad7527b7301bfc824ba847c1b06bd17'},
800000: {'ledger_hash': '0a29c190507ba4cfbeb028e79060352788317143bb907532b0c202c25bf9b2b8', 'txlist_hash': 'd251bb3f1b7be7d6e912bbcb83e3bd554ce82e2e5fd9e725114107a4e2864602'},
850000: {'ledger_hash': '681aa15524dc39d864787362d1eac60a48b10a1d8a689fed4ef33e6f2780ba4d', 'txlist_hash': '94a37d05251ab4749722976929b888853b1b7662595955f94dd8dc97c50cb032'},
# 650000: {'ledger_hash': '451a04386454fe0f9f1ff81770f70c762be675b88d1cec4fc1ec3d1fbe74d08c', 'txlist_hash': '7ac0d7fa2ec4553ca83c3687b9e16367f468c315df838aef55ae4fd1135adae9'},

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

were these checkpoints broken for you before picopayments, or do you have tests that invalidate them?

This comment has been minimized.

@F483

F483 Mar 22, 2017

Author Member

These checkpoints are broken and be removed or fixed in another PR. Since it is testnet I doen't think fixing them is of much importance.

# License: MIT (see LICENSE file)


from .control import make_deposit # NOQA

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

what does NOQA mean?

This comment has been minimized.

@F483

F483 Mar 22, 2017

Author Member

The #NOQA comment is for automated pep8 tests (I fail my builds if they are not pep8 compliant). In thes case I import the function to make it available in the module and pep8 would complain I imported something I didn't use.

@@ -0,0 +1,18 @@
# coding: utf-8

This comment has been minimized.

@robby-dermody

robby-dermody Mar 21, 2017

Member

hmm, probably remove these headers... or at least the copyright line and the license line, because if this is becoming part of the counterparty-lib source, the copyright line is listed in our LICENSE file, and the License line is not really necessary

looking into it further, ...obviously I am not a lawyer, but it appears there are two main ways to do this. first is you grant CP a irrevocable license to use the code and you keep the copyright, the 2nd is that the copyright for this code is assigned to CP... see http://producingoss.com/en/copyright-assignment.html

I'd prefer the second (as the code is becoming part of counterparty-lib, it keeps things tighter) but am open to discussion here

This comment has been minimized.

@F483

F483 Mar 22, 2017

Author Member

Removed the headers as requested. I am also no lawer, but as far as I can see we are both using the same MIT license. So we should have no issues there.

@F483 F483 force-pushed the F483:micropayments branch 2 times, most recently from bd7eca5 to da57a1d Mar 22, 2017

@F483 F483 force-pushed the F483:micropayments branch from da57a1d to 9613df3 Mar 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.