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

tx=20: add Action byte for New, Update, Cancel #41

Closed
wants to merge 11 commits into from

Conversation

marv-engine
Copy link

To simplify updating or cancelling a Sell offer, and to prevent an update from generating an unintended new Sell offer if the previous one had already been accepted.

Version # and Doc History need to be updated.

To simplify updating or cancelling a Sell offer, and to prevent an update from generating an unintended new Sell offer if the previous one had already been accepted.
@dacoinminster
Copy link
Contributor

Good idea adding a byte to the offer request. What about backwards compatibility with the test transactions we have done so far?

@dacoinminster
Copy link
Contributor

I need the devs to weigh in on this one, especially in regards to the effect of adding this byte for backwards compatibility. I'm pretty sure this is the best solution from a user-experience perspective.

@zathras-crypto
Copy link
Contributor

Quick clarification - what would be envisaged for action = 2 (update) with a saleamount of 0 - same behaviour as action=3 (cancel?)

@marv-engine
Copy link
Author

It seems reasonable that a 0 saleamount means cancel. That begs the
question: Is 0 a valid value for fields such as

  • Amount of bitcoins desired
  • Time limit
  • Minimum bitcoin transaction fee

and what does 0 mean in each case?

We need to answer this type of question for all the transactions, e.g. is
it valid to transfer 0 MSC in tx 0 or to purchase 0 MSC in tx 22, and what
does it mean?

Marv Schneider
VP, User Experience/Product Usability
Engine, Inc.
marv@engine.co
240-462-6123 cell

On Tue, Jan 21, 2014 at 6:17 PM, zathras-crypto notifications@github.comwrote:

Quick clarification - what would be envisaged for action = 2 (update) with
a saleamount of 0 - same behaviour as action=3 (cancel?)


Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-32975687
.

@maran
Copy link
Contributor

maran commented Jan 22, 2014

I would prefer to invalidate 0 amount transactions / time limit and perhaps even transaction fee.

  • We don't need it anymore for "amount" since we have a cancel option now.
  • An unlimited time limit could result in a 'stuck' Selling Offer since you could broadcast a purchase intent that never completes
  • This would only encourage spam.

This pull would invalidate all existing selling offers, which I think won't matter anyway since they are only in test. Adding legacy support (build defaulting to action=0) is probably not worth it.

My 2 cents :)

@B1tMaster
Copy link

Perhaps another approach to this would be to

  1. Include a transaction ID into every message that is related to the same transaction. It would span ALL messages including the first one, update amount or price changes or cancels. This way all messages on the block-chain could be easily related by that "transaction-ID" . This way one can update the price, amount of MSC for sale etc without having to cancel. A transaction ID will united all these messages and an application parsing this can always take latest part of the transaction as valid offer.
  2. In addition, do not consider any transaction as DONE until prices match and user (buyer or seller) issue a final "commit" message formalising end of transaction. A user who finally matches the other "price" is the one to issue a commit message. this would remove all issues of automatically matching transactions. The update on price could be issued by a seller (reduce his ask) or by a buyer (increase his bid). A transaction can also end in a cancel.

Commit could probably also be incorporated into any message making it final. (but maybe it is better to have a separate commit message as well)

So if I issue as sale offer of 1 MSC and say I make this offer valid for 10 blocks, then anybody can respond to this offer by issuing a buy bid (using the same transaction ID) that was posted in the offer.

Any such reply received within the 10 blocks would be presented to the seller in his GUI. In fact, a seller may be presented with multiple offers to chose from. He will obviously chose the one with highest bid.

the price of the "bid" and his "ask" most likely will not match. So he can adjust his offer to cross with the highest bidder and issue an updated sell order with updated price (using the same transaction ID) and also issue a commit message as well to close the transaction.

A seller should also be able to specifically allocate his sell to multiple buyers. Or even increase the amount he is selling if amount of the bids exceeds the amount he offered for sale and he happens to have more coins to sell.

If prices for bid and ask do not cross within the specified time frame , then transaction expire. Absence of a commit message also invalidate any started transaction.

The negative to this approach is that an extra "commit" message is needed which further increase the cost of the transaction.

@zathras-crypto
Copy link
Contributor

Bitmaster - thanks & FYI your suggested process is a significant deviation from current DEx process and would require we re-evaluate our approach from a holistic PoV - it should also be noted that including a reference transaction ID adds significantly to the payload size (it would triple it from 33 bytes to 97 bytes).

As discussed in the last sync I think this topic is complex and deserves a real-time discussion. IRC is fine or Skype perhaps. Taking a stab at timezones how does 20:00 GMT 25/01 suit?

@zathras-crypto
Copy link
Contributor

Also +1 re Tachikoma's comments on 0 value timelimits (for payment) - I concur that could be open to abuse (perpetually locked in 'pending payment') - continuing with that logic an upper bound would also be appropriate - a timelimit of 999999999 is just as problematic.

@maran
Copy link
Contributor

maran commented Jan 23, 2014

I can make time on 25/01 around 20:00 GMT to have a discussion about this. I think it's important @marv-engine is there as well since he has been very vocal about this issue so far. Could that time work for you?

@B1tMaster
Copy link

Speaking of FIX. The protocol has so called: Cancel-and-Replace message which is both : cancel of the previous order and updating the order with new information. Could be relevant here as a model. http://www.onixs.biz/fix-dictionary/4.2/msgType_G_71.html

@B1tMaster
Copy link

I am away during the weekend on 25th. Also 20 GMT is 4 am for me. :-)

One more question on the protocol encoding in response to tathras: I may be mis-understanding some basics on bitcoin chain encoding reqs maybe? But otherwise, I don't understand why are we using so many bytes for everything.
For example: 4 bytes for order type? One byte in general can encode 255 order types. I am not sure why are we using 4 bytes for just a couple of order types? And if we could use smaller space message sizes would be dramatically smaller.

Also, could we use something like Google Buffers to encode the entire message into a compact binary form which then could be stored onto a block-chain?

@zathras-crypto
Copy link
Contributor

Why are we using 32 bit integers instead of 16 (or 8) bit? That'd be one for J.R. to answer, I wouldn't want to speak for him :)

@zathras-crypto
Copy link
Contributor

Additionally since the concept of bids has been raised, my thoughts on how this would be achieved without deviating from the current model too much:

  1. Buyer broadcasts Master protocol message from buying address containing bid offer (amount, currency ID, price etc)
  2. Seller broadcasts Master protocol message from selling address accepting bid offer (or part thereof) and a timelimit for payment
  3. Buyer sends BTC from buying address to selling address within timelimit (else expires)
  4. Protocol transfers reserved funds to buying address

Note: Assuming appropriate confirmations between messages
Note: Wallet software could check the buying address for sufficient BTC to cover the bid and provide an option to hide unfunded bids
Note: The accept bid message is invalid if the sending address does not have >= amount. Protocol will reserve amount from the selling address for timelimit

It's rough - been a long day. May be a glaring hole there so feel free to tear it apart, it's just brain dump :)

@dacoinminster
Copy link
Contributor

I didn't notice that the pull request specified a 32-bit integer (sorry).
Definitely one byte should be sufficient. Marv, can you update it?

Interesting idea about posting bids in the block chain. My main worry would
be fake bids. With a MSC sell offer, you know for sure that it is real. I'd
at least require that the address must actually contain the BTC for sale,
and probably add an anti-spam fee of some sort, preferably destroying a
small amount of MSC to place a bid.

Anti-spam fees are a whole different conversation. We need them for smart
property, and we need to figure out a way to make them float so that they
are always the appropriate size, but I don't want to make things too
complicated. I'll hopefully have some time to work on something in that
direction once I'm full time in a couple weeks.

I'd suggest that bids in the order book are probably out-of-scope for now,
but it would be cool to add them once we've got a good idea of how we'll do
a floating anti-spam fee.

I'm probably not going to be able to join the conference call (I am VERY
busy trying to finish strong here at Cozi before I leave). As per our
procedure I previously specified, Maran should make the final call here
once he has heard everyone's opinions.

Thanks!

On Thu, Jan 23, 2014 at 1:11 AM, zathras-crypto notifications@github.comwrote:

Additionally since the concept of bids has been raised, my thoughts on how
this would be achieved without deviating from the current model too much:

  1. Buyer broadcasts Master protocol message from buying address containing
    bid offer (amount, currency ID, price etc)
  2. Seller broadcasts Master protocol message from selling address
    accepting bid offer (or part thereof) and a timelimit for payment
  3. Buyer sends BTC from buying address to selling address within timelimit
    (else expires)
  4. Protocol transfers reserved funds to buying address

Note: Assuming appropriate confirmations between messages
Note: Wallet software could check the buying address for sufficient BTC to
cover the bid and provide an option to hide unfunded bids
Note: The accept bid message is invalid if the sending address does not
have >= amount. Protocol will reserve amount from the selling address for
timelimit

It's rough - been a long day. May be a glaring hole there so feel free to
tear it apart, it's just brain dump :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-33107470
.

@marv-engine
Copy link
Author

I did make the action 1 byte:

  1. Action = 1 for New offer (2 for Update offer, 3 for Cancel offer)
    (8-bit unsigned integer, 1 byte)

I think the question about a 32-bit integer may refer to the transaction
type:

  1. Transaction type = 20 for currency trade offer for bitcoins (32-bit
    unsigned integer, 4 bytes)

I, too, have wondered why it's so big.

Marv Schneider
VP, User Experience/Product Usability
Engine, Inc.
marv@engine.co
240-462-6123 cell

On Thu, Jan 23, 2014 at 12:53 PM, dacoinminster notifications@github.comwrote:

I didn't notice that the pull request specified a 32-bit integer (sorry).
Definitely one byte should be sufficient. Marv, can you update it?

Interesting idea about posting bids in the block chain. My main worry
would
be fake bids. With a MSC sell offer, you know for sure that it is real.
I'd
at least require that the address must actually contain the BTC for sale,
and probably add an anti-spam fee of some sort, preferably destroying a
small amount of MSC to place a bid.

Anti-spam fees are a whole different conversation. We need them for smart
property, and we need to figure out a way to make them float so that they
are always the appropriate size, but I don't want to make things too
complicated. I'll hopefully have some time to work on something in that
direction once I'm full time in a couple weeks.

I'd suggest that bids in the order book are probably out-of-scope for now,
but it would be cool to add them once we've got a good idea of how we'll
do
a floating anti-spam fee.

I'm probably not going to be able to join the conference call (I am VERY
busy trying to finish strong here at Cozi before I leave). As per our
procedure I previously specified, Maran should make the final call here
once he has heard everyone's opinions.

Thanks!

On Thu, Jan 23, 2014 at 1:11 AM, zathras-crypto notifications@github.comwrote:

Additionally since the concept of bids has been raised, my thoughts on
how
this would be achieved without deviating from the current model too
much:

  1. Buyer broadcasts Master protocol message from buying address
    containing
    bid offer (amount, currency ID, price etc)
  2. Seller broadcasts Master protocol message from selling address
    accepting bid offer (or part thereof) and a timelimit for payment
  3. Buyer sends BTC from buying address to selling address within
    timelimit
    (else expires)
  4. Protocol transfers reserved funds to buying address

Note: Assuming appropriate confirmations between messages
Note: Wallet software could check the buying address for sufficient BTC
to
cover the bid and provide an option to hide unfunded bids
Note: The accept bid message is invalid if the sending address does not
have >= amount. Protocol will reserve amount from the selling address
for
timelimit

It's rough - been a long day. May be a glaring hole there so feel free
to
tear it apart, it's just brain dump :)


Reply to this email directly or view it on GitHub<
https://github.com/mastercoin-MSC/spec/pull/41#issuecomment-33107470>
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-33149515
.

@dacoinminster
Copy link
Contributor

That's what I thought. Sorry for the confusion. I used 32-bit integers
anywhere I didn't want to limit future growth. Probably we shouldn't mess
with those unless we have good reason.

On Thu, Jan 23, 2014 at 10:03 AM, Marv Schneider
notifications@github.comwrote:

I did make the action 1 byte:

  1. Action = 1 for New offer (2 for Update offer, 3 for Cancel offer)
    (8-bit unsigned integer, 1 byte)

I think the question about a 32-bit integer may refer to the transaction
type:

  1. Transaction type = 20 for currency trade offer for bitcoins (32-bit
    unsigned integer, 4 bytes)

I, too, have wondered why it's so big.

Marv Schneider
VP, User Experience/Product Usability
Engine, Inc.
marv@engine.co
240-462-6123 cell

On Thu, Jan 23, 2014 at 12:53 PM, dacoinminster notifications@github.comwrote:

I didn't notice that the pull request specified a 32-bit integer
(sorry).
Definitely one byte should be sufficient. Marv, can you update it?

Interesting idea about posting bids in the block chain. My main worry
would
be fake bids. With a MSC sell offer, you know for sure that it is real.
I'd
at least require that the address must actually contain the BTC for
sale,
and probably add an anti-spam fee of some sort, preferably destroying a
small amount of MSC to place a bid.

Anti-spam fees are a whole different conversation. We need them for
smart
property, and we need to figure out a way to make them float so that
they
are always the appropriate size, but I don't want to make things too
complicated. I'll hopefully have some time to work on something in that
direction once I'm full time in a couple weeks.

I'd suggest that bids in the order book are probably out-of-scope for
now,
but it would be cool to add them once we've got a good idea of how we'll
do
a floating anti-spam fee.

I'm probably not going to be able to join the conference call (I am VERY
busy trying to finish strong here at Cozi before I leave). As per our
procedure I previously specified, Maran should make the final call here
once he has heard everyone's opinions.

Thanks!

On Thu, Jan 23, 2014 at 1:11 AM, zathras-crypto <
notifications@github.com>wrote:

Additionally since the concept of bids has been raised, my thoughts on
how
this would be achieved without deviating from the current model too
much:

  1. Buyer broadcasts Master protocol message from buying address
    containing
    bid offer (amount, currency ID, price etc)
  2. Seller broadcasts Master protocol message from selling address
    accepting bid offer (or part thereof) and a timelimit for payment
  3. Buyer sends BTC from buying address to selling address within
    timelimit
    (else expires)
  4. Protocol transfers reserved funds to buying address

Note: Assuming appropriate confirmations between messages
Note: Wallet software could check the buying address for sufficient
BTC
to
cover the bid and provide an option to hide unfunded bids
Note: The accept bid message is invalid if the sending address does
not
have >= amount. Protocol will reserve amount from the selling address
for
timelimit

It's rough - been a long day. May be a glaring hole there so feel free
to
tear it apart, it's just brain dump :)


Reply to this email directly or view it on GitHub<
https://github.com/mastercoin-MSC/spec/pull/41#issuecomment-33107470>
.


Reply to this email directly or view it on GitHub<
https://github.com/mastercoin-MSC/spec/pull/41#issuecomment-33149515>
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-33150552
.

@zathras-crypto
Copy link
Contributor

So @marv-engine are we on for discussing this 20:00GMT 25/01 or would you prefer to try and arrange a time where @B1tMaster can join?

@marv-engine
Copy link
Author

Sorry, we should schedule this for a time that works for everyone who's interested, but ASAP. I'm generally available. If you want to be in the live mtg, let us know when you can make it. If you're comfortable just stating your position, please do that.

This discussion has grown beyond just the Action byte (New, Update, Cancel) I proposed. Can we make a decision on just that part now and address the other suggested enhancements afterwards?

Also we need to be clear about:

  • attempted updates/cancels after partial acceptance (I think those have to be allowed)
  • min and max values for payment time limit
  • is 0 a valid amount for sale, if so what does it mean?
  • is 0 a valid amount for bitcoins desired (would anyone just give currency away to the first taker??)
  • Minimum bitcoin transaction fee
  • valid values for other fields?

@marv-engine
Copy link
Author

I replied by adding a comment to the PR. It says:

Sorry, we should schedule this for a time that works for everyone who's
interested, but ASAP. I'm generally available. If you want to be in the
live mtg, let us know when you can make it. If you're comfortable just
stating your position, please do that.

This discussion has grown beyond just the Action byte (New, Update, Cancel)
I proposed. Can we make a decision on just that part now and address the
other suggested enhancements afterwards?

Also we need to be clear about:

  • attempted updates/cancels after partial acceptance (I think those have
    to be allowed)
  • min and max values for payment time limit
  • is 0 a valid amount for sale, if so what does it mean?
  • is 0 a valid amount for bitcoins desired (would anyone just give
    currency away to the first taker??)
  • Minimum bitcoin transaction fee
  • valid values for other fields?

Marv Schneider
VP, User Experience/Product Usability
Engine, Inc.
marv@engine.co
240-462-6123 cell

On Sat, Jan 25, 2014 at 12:36 AM, zathras-crypto
notifications@github.comwrote:

So @marv-engine https://github.com/marv-engine are we on for discussing
this 20:00GMT 25/01 or would you prefer to try and arrange a time where
@B1tMaster https://github.com/B1tMaster can join?


Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-33281765
.

@zathras-crypto
Copy link
Contributor

Hey guys,

Can we have a an example series of transactions that triggers this race condition? I can't seem to find one anywhere. Ie user A does X, then user B does Y, then Z condition occurs etc. This helps with clarity.

Re the additional byte, I'm in split minds & supportive if it's fundamentally necessary. Tachikoma has raised the possibility of invalidating all previous DEx messages. Since it's only test we can get away with this but what is the impact on branding (if any) - does it give the impression we don't have a clear direction/strategy if we state we're invalidating all DEx messages done so far? Or is this a non-issue?

If we did go ahead with this method:

  • Min and max values (timelimit), perhaps 1 & 1000 respectively. If a user wants to request immediate payment (next block) then that's up to them, and 1000 blocks is on average a week (holding a fixed price allowing payment a week later in crypto markets could be financial suicide if someone got unlucky, but we should be giving the users the choice). Much longer than this and we start looking at stale 'pending payment' (as Tachikoma referred to - 'stuck') sells/accepts.
  • 0 for sale should either be a) invalidated or b) = same behaviour as cancel
  • 0 bitcoins desired should be invalidated
  • Min transaction fee should be 0.0001. We could go as low as 0.00005430 but I don't see why we'd need to.

Thanks
Zathras

@marv-engine
Copy link
Author

Can we have a an example series of transactions that triggers this race
condition? I can't seem to find one anywhere. Ie user A does X, then user B
does Y, then Z condition occurs etc. This helps with clarity.

Current behavior:

  1. User A has coins to sell, so he posts a Sell Offer.
  2. These 2 happen at the same time (at least User B's action takes
    effect before User A):
    • User B wants to buy them all so he posts a Purchase offer for the
      whole amount offered.
    • User A wants to change his asking price so he posts a Sell Offer
      for the same coins with a modified price. If User B has already accepted
      the existing Sell Offer, User A has unintentionally created a 2nd Sell
      Offer (the one with the modified price) which could get accepted before
      User A is able to cancel it.

Proposed behavior:

  1. User A has coins to sell, so he posts a Sell Offer (Action=New).
  2. These 2 happen at the same time:
    • User B wants to buy them all so he posts a Purchase offer for the
      whole amount offered.
    • User A wants to change his asking price so he posts a Sell Offer
      (Action=Update) for the same coins with a modified price. If User B has
      already accepted the existing Sell Offer, this Update will be rejected
      because there is no outstanding Sell Offer. If any amount of the existing
      Sell Offer has not been accepted, the Update will apply to the remaining
      amount. If User A posts a Sell Offer (Action=Cancel) but there's
      no active
      Sell Offer, that tx should be rejected.

Re: invalidating previous DEx messages - that's one reason why I proposed
having bytes in each message reserved for future use. It allows for this
type of change, and it should convey that we have a strategy
for inevitable change.

Marv Schneider
VP, User Experience/Product Usability
Engine, Inc.
marv@engine.co
240-462-6123 cell

On Mon, Jan 27, 2014 at 5:44 PM, zathras-crypto notifications@github.comwrote:

Hey guys,

Can we have a an example series of transactions that triggers this race
condition? I can't seem to find one anywhere. Ie user A does X, then user B
does Y, then Z condition occurs etc. This helps with clarity.

Re the additional byte, I'm in split minds & supportive if it's
fundamentally necessary. Tachikoma has raised the possibility of
invalidating all previous DEx messages. Since it's only test we can get
away with this but what is the impact on branding (if any) - does it give
the impression we don't have a clear direction/strategy if we state we're
invalidating all DEx messages done so far? Or is this a non-issue?

If we did go ahead with this method:

  • Min and max values, perhaps 1 & 1000 respectively. If a user wants
    to request immediate payment (next block) then that's up to them, and 1000
    blocks in on average a week (holding a fixed price allowing payment a week
    later in crypto markets could be financial suicide if someone got unlucky,
    but we should be giving the users the choice). Much longer than this and we
    start looking at stale 'pending payment' (as Tachikoma referred to -
    'stuck') sells/accepts.
  • 0 for sale should either be a) invalidated or b) = same behaviour as
    cancel
  • 0 bitcoins desired should be invalidated
  • Min transaction fee should be 0.0001. We could go as low as
    0.00005430 but I don't see why we'd need to.

Thanks
Zathras


Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-33432275
.

@zathras-crypto
Copy link
Contributor

Thanks Marv & this is exactly what I was looking for. This really helps - I could never quite understand what the problem was to begin with & that's probably because what you've described I don't see as a problem at all :)

Current behaviour:

  1. User A has coins to sell, so he posts a Sell Offer.
  2. These 2 happen at the same time (at least User B's action takes
    effect before User A):
    • User B wants to buy them all so he posts a Purchase offer for the
      whole amount offered.
    • User A wants to change his asking price so he posts a Sell Offer
      for the same coins with a modified price. If User B has already accepted
      the existing Sell Offer, User A has unintentionally created a 2nd Sell
      Offer (the one with the modified price) which could get accepted before
      User A is able to cancel it.

Feedback:

  • There is no such thing as 'at the same time'. Even if multiple master protocol messages appear in the same block, they are processed by their order in the block. Thus there is always a sequence to processing master protocol messages (every message is either before or after another in the chain, never at the same time).
  • Sellers can only have one active sell offer per address at one time
  • If User B's accept is processed before User A's sell change, then the sell change is simply invalid. User A cannot create a second sell offer unintentionally as described because the original sell is not yet closed and only one active sell is allowed (sells are not closed when funds are reserved by an acceptoffer (also called purchaseoffer), they're closed when funds are paid, the trade is complete and there are no more coins remaining).

TL:DR; I don't see a race condition. Am I the only one in this mindset? Tachikoma/Bitoy/Graz what do you guys think?

Thanks :)
Zathras

@maran
Copy link
Contributor

maran commented Jan 28, 2014

There still is a race condition, it's just moved to the actual purchase.

  1. User A has coins to sell so he posts a Sell Offer.
  2. User B did a broadcast for a Purchase offer which was accepted.
  3. User B sends the Bitcoin funds while User A changes the price of the
    original Sell Offer. (I'm going to argue he did not see his offer was
    accepted yet)
  4. We now have the situation Marv was trying to describe where we have a
    change that now is a new Purchase offer.

On Tue, Jan 28, 2014 at 3:27 AM, zathras-crypto notifications@github.comwrote:

Thanks Marv & this is exactly what I was looking for. This really helps -
I could never quite understand what the problem was to begin with & that's
probably because what you've described I don't see as a problem at all :)

Current behaviour:

  1. User A has coins to sell, so he posts a Sell Offer.
  2. These 2 happen at the same time (at least User B's action takes
    effect before User A):
  3. User B wants to buy them all so he posts a Purchase offer for the
    whole amount offered.
    • User A wants to change his asking price so he posts a Sell Offer
      for the same coins with a modified price. If User B has already accepted
      the existing Sell Offer, User A has unintentionally created a 2nd Sell
      Offer (the one with the modified price) which could get accepted before
      User A is able to cancel it.

Feedback:

  • There is no such thing as 'at the same time'. Even if multiple
    master protocol messages appear in the same block, they are processed by
    their order in the block. Thus there is always a sequence to processing
    master protocol messages and the concept of 'at the same time' doesn't
    exist.
  • Sellers can only have one active sell offer per address at one time
  • If User B's accept is processed before User A's sell change, then
    the sell change is simply invalid. User A cannot create a second sell offer
    unintentionally as described as the original sell is not yet closed and
    only one active sell is allowed (sells are not closed when funds are
    reserved, they're closed when funds are paid and the trade is complete).

TL:DR; I don't see a race condition. Am I the only one in this mindset?
Tachikoma/Bitoy/Graz what do you guys think?

Thanks :)
Zathras

Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-33445876
.

@zathras-crypto
Copy link
Contributor

Apologies to labor this but I'm still not seeing the issue, inline:

  1. User B did a broadcast for a Purchase offer which was accepted.
    So the terms of the trade are now locked into the blockchain.
  2. User B sends the Bitcoin funds while User A changes the price of the original Sell Offer. (I'm going to argue he did not see his offer was accepted yet).
    Regardless of what end user software displays/what the user sees, the terms of the trade as per the protocol are already locked into the blockchain - User A cannot change the price of the reserved portion of the original sell offer once the terms have been agreed to and that acceptance is confirmed on the blockchain (via the accept offer) - thus User B is free
    to send the originally agreed BTC amount without fearing the price may be changed mid trade.
  3. We now have the situation Marv was trying to describe where we have a change that now is a new Purchase offer.
    I'm not 100% I have your meaning - but can I flip this around for a second - and forgive me if this is what you meant - I would have thought a bigger risk is the inverse - a seller has a sell at X price, buyer attempts to send an accept offer at X price, while in the same block the seller changes the price to Y. If the sellers transaction is first in the block, the accept offer is then applied to the new sell & locked in at price Y. Wallet software needs to detect this condition and highlight that the terms the buyer accepted have changed. How would adding a byte for transaction intent help this example?

For clarity, I'm not against an extra byte to explicitly state transaction intent, so if I'm the only one posing this position and everyone else doesn't see what I'm talking about, don't let me hold you up. With that said, I always prefer to understand the reasoning for something and preferably would like to identify the exact circumstances that we need to protect against

Thanks :)
Zathras

On Tue, Jan 28, 2014 at 7:41 PM, Maran notifications@github.com wrote:

There still is a race condition, it's just moved to the actual purchase.

  1. User A has coins to sell so he posts a Sell Offer.
  2. User B did a broadcast for a Purchase offer which was accepted.
  3. User B sends the Bitcoin funds while User A changes the price of the
    original Sell Offer. (I'm going to argue he did not see his offer was
    accepted yet)
  4. We now have the situation Marv was trying to describe where we have a
    change that now is a new Purchase offer.

On Tue, Jan 28, 2014 at 3:27 AM, zathras-crypto <notifications@github.com

wrote:

Thanks Marv & this is exactly what I was looking for. This really helps -
I could never quite understand what the problem was to begin with &
that's
probably because what you've described I don't see as a problem at all :)

Current behaviour:

  1. User A has coins to sell, so he posts a Sell Offer.
  2. These 2 happen at the same time (at least User B's action takes
    effect before User A):
  3. User B wants to buy them all so he posts a Purchase offer for the
    whole amount offered.
  4. User A wants to change his asking price so he posts a Sell Offer
    for the same coins with a modified price. If User B has already accepted
    the existing Sell Offer, User A has unintentionally created a 2nd Sell
    Offer (the one with the modified price) which could get accepted before
    User A is able to cancel it.

Feedback:

  • There is no such thing as 'at the same time'. Even if multiple
    master protocol messages appear in the same block, they are processed by
    their order in the block. Thus there is always a sequence to processing
    master protocol messages and the concept of 'at the same time' doesn't
    exist.
  • Sellers can only have one active sell offer per address at one time
  • If User B's accept is processed before User A's sell change, then
    the sell change is simply invalid. User A cannot create a second sell
    offer
    unintentionally as described as the original sell is not yet closed and
    only one active sell is allowed (sells are not closed when funds are
    reserved, they're closed when funds are paid and the trade is complete).

TL:DR; I don't see a race condition. Am I the only one in this mindset?
Tachikoma/Bitoy/Graz what do you guys think?

Thanks :)
Zathras

Reply to this email directly or view it on GitHub<
https://github.com/mastercoin-MSC/spec/pull/41#issuecomment-33445876>

.

Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-33459878
.

@maran
Copy link
Contributor

maran commented Jan 28, 2014

Just to be clear on this:

The problem we are trying to solve is that User A created a new Purchase
Offer instead of changing the (sold out) one.

So if you wanted to sell 10MSC, you now might end up selling 10MSC twice
since the change PO you intended is now a new one for the full amount again.

Right?

On Tue, Jan 28, 2014 at 10:00 AM, zathras-crypto
notifications@github.comwrote:

Apologies to labor this but I'm still not seeing the issue, inline:

  1. User B did a broadcast for a Purchase offer which was accepted.
    So the terms of the trade are now locked into the blockchain.
  2. User B sends the Bitcoin funds while User A changes the price of
    the original Sell Offer. (I'm going to argue he did not see his offer
    was accepted yet).
    Regardless of what end user software displays/what the user sees, the
    terms of the trade as per the protocol are already locked into the
    blockchain - User A cannot change the price of the reserved portion of the
    original sell offer once the terms have been agreed to and that acceptance
    is confirmed on the blockchain (via the accept offer) - thus User B is free
    to send the originally agreed BTC amount without fearing the price may be
    changed mid trade.
  3. We now have the situation Marv was trying to describe where we have
    a change that now is a new Purchase offer.
    *I'm not 100% I have your meaning - but can I flip this around for a second
  4. and forgive me if this is what you meant - I would have thought a bigger
    risk is the inverse - a seller has a sell at X price, buyer attempts to
    send an accept offer at X price, while in the same block the seller changes
    the price to Y. If the sellers transaction is first in the block, the
    accept offer is then locked in at price Y. Wallet software needs to detect
    this condition and highlight that the terms the buyer accepted have
    changed. How would adding a byte for transaction intent help this example?*

For clarity, I'm not against an extra byte to explicitly state transaction
intent, so if I'm the only one posing this position and everyone else
doesn't see what I'm talking about, don't let me hold you up. With that
said, I always prefer to understand the reasoning for something and
preferably would like to identify the exact circumstances that we need to
protect against

Thanks :)
Zathras

On Tue, Jan 28, 2014 at 7:41 PM, Maran notifications@github.com wrote:

There still is a race condition, it's just moved to the actual purchase.

  1. User A has coins to sell so he posts a Sell Offer.
  2. User B did a broadcast for a Purchase offer which was accepted.
  3. User B sends the Bitcoin funds while User A changes the price of the
    original Sell Offer. (I'm going to argue he did not see his offer was
    accepted yet)
  4. We now have the situation Marv was trying to describe where we have a
    change that now is a new Purchase offer.

On Tue, Jan 28, 2014 at 3:27 AM, zathras-crypto <
notifications@github.com

wrote:

Thanks Marv & this is exactly what I was looking for. This really
helps -
I could never quite understand what the problem was to begin with &
that's
probably because what you've described I don't see as a problem at all
:)

Current behaviour:

  1. User A has coins to sell, so he posts a Sell Offer.
  2. These 2 happen at the same time (at least User B's action takes
    effect before User A):
  3. User B wants to buy them all so he posts a Purchase offer for the
    whole amount offered.
  4. User A wants to change his asking price so he posts a Sell Offer

for the same coins with a modified price. If User B has already
accepted
the existing Sell Offer, User A has unintentionally created a 2nd Sell
Offer (the one with the modified price) which could get accepted before
User A is able to cancel it.

Feedback:

  • There is no such thing as 'at the same time'. Even if multiple

master protocol messages appear in the same block, they are processed
by
their order in the block. Thus there is always a sequence to processing
master protocol messages and the concept of 'at the same time' doesn't
exist.

  • Sellers can only have one active sell offer per address at one time
  • If User B's accept is processed before User A's sell change, then

the sell change is simply invalid. User A cannot create a second sell
offer
unintentionally as described as the original sell is not yet closed and
only one active sell is allowed (sells are not closed when funds are
reserved, they're closed when funds are paid and the trade is
complete).

TL:DR; I don't see a race condition. Am I the only one in this mindset?
Tachikoma/Bitoy/Graz what do you guys think?

Thanks :)
Zathras

Reply to this email directly or view it on GitHub<
https://github.com/mastercoin-MSC/spec/pull/41#issuecomment-33445876>

.

Reply to this email directly or view it on GitHub<
https://github.com/mastercoin-MSC/spec/pull/41#issuecomment-33459878>

.

Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-33460899
.

@zathras-crypto
Copy link
Contributor

Yeah I think I've got you - thanks - where you say purchase offer you're meaning sell offer right?

So let's just break this down with example block numbers (again if I'm being pedantic feel free to curse :P)

BLOCK1 - Confirmed tx where seller offers 10MSC for 1BTC
BLOCK2 - Confirmed tx where buyer accepts 10MSC for 1BTC
BLOCK3 - Confirmed tx where buyer sends 1BTC, protocol transfers 10MSC from seller to buyer

If the seller broadcasts a update-sell between 2 and 3, it's invalid because the funds are already reserved and the sell is still open. No new sell is created.
If the seller broadcasts a update-sell after 3, this is a new sell offer. Is this what we're trying to protect against? A user not seeing that their previous trade has completed and mistakenly starting a new sell instead of changing the old one? If that's the case then wouldn't that be a presentation layer issue, not protocol? Why would client software allow you to modify a completed sell? Also I can't imagine we'd want client software broadcasting unnecessary transactions (w/ update-sell byte) if the prior sell is already complete and thus that transaction is invalid before it's even broadcast. TL:DR; it's the responsibility of client software to guide the users actions, not the protocol.

Just my view :)

@maran
Copy link
Contributor

maran commented Jan 28, 2014

Yeah that's exactly right.

It would be easier to let the client be responsible for this. I'm just not
sure it's a responsible move spec wise. Bad clients could broadcast these
problems and create problems for the end-users. Not sure what is the right
move here.

On Tue, Jan 28, 2014 at 10:23 AM, zathras-crypto
notifications@github.comwrote:

Yeah I think I've got you - thanks - where you say purchase offer you're
meaning sell offer right?

So let's just break this down with example block numbers (again if I'm
being pedantic feel free to curse :P)

BLOCK1 - Confirmed tx where seller offers 10MSC for 1BTC
BLOCK2 - Confirmed tx where buyer accepts 10MSC for 1BTC
BLOCK3 - Confirmed tx where buyer sends 1BTC, protocol transfers 10MSC
from seller to buyer

If the seller broadcasts a update-sell between 2 and 3, it's invalid
because the funds are already reserved and the sell is still open. No new
sell is created.
If the seller broadcasts a update-sell after 3, this is a new sell offer.
Is this what we're trying to protect against? A user not seeing that their
previous trade has completed and mistakenly starting a new sell instead of
changing the old one? If that's the case then wouldn't that be a
presentation layer issue, not protocol? Why would client software allow you
to modify a completed sell? Also I can't imagine we'd want client software
broadcasting unnecessary transactions (update-sell) if the prior sell is
already complete and thus that transaction is invalid before it's even
broadcast. TL:DR; it's the responsibility of client software to guide the
users actions, not the protocol.

Just my view :)

Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-33462338
.

@zathras-crypto
Copy link
Contributor

Scratch that. My apologies.

If the seller broadcasts an update-sell between block 2 and 3, AND it makes it into block 3, AND it is ordered after the BTC payment - boom, update becomes new sell (from the perspective of a sell message broadcast prior to the previous sell closing).

Again should be handled client side - ie clients shouldn't let you broadcast a change sell if all the funds are already reserved - but do concur we can't rely on that (trust wise) - people can broadcast whatever they like - and we should have a set way of dealing with whatever is broadcast.

So we rely (trust) on the behaviour that is dictated by the protocol:

  • If the updated sell transaction is before the BTC payment, the original sell is still reserved and the updated sell is invalid
  • If the updated sell transaction is after the BTC payment, a new sell is created

The protocol will always behave the same way & it's up to clients to interpret that and allow users to interact with it (and potentially stop users broadcasting transactions they didn't mean to if that's a concern). Other than a bad client tricking a user into selling twice is there anything else in play here?

Thanks for taking time to discuss mate :)

@maran
Copy link
Contributor

maran commented Jan 28, 2014

I don't think so but I let's see what Marv has to say about our recent
discussion. :)

On Tue, Jan 28, 2014 at 10:43 AM, zathras-crypto
notifications@github.comwrote:

Scratch that. My apologies.

If the seller broadcasts an update-sell between block 2 and 3, AND it
makes it into block 3, AND it is ordered after the BTC payment - boom,
update becomes new sell.

Again should be handled client side - ie clients shouldn't let you
broadcast a change sell if all the funds are already reserved - but do
concur we can't rely on that (trust wise) - people can broadcast whatever
they like - and we should have a set way of dealing with whatever is
broadcast.

So we rely (trust) on the behaviour that is dictated by the protocol:

  • If the updated sell transaction is before the BTC payment, the
    original sell is still reserved and the updated sell is invalid
  • If the updated sell transaction is after the BTC payment, a new sell
    is created

The protocol will always behave the same way & it's up to clients to
interpret that and allow users to interact with it (and potentially stop
users broadcasting transactions they didn't mean to if that's a concern).
Other than a bad client tricking a user into selling twice is there
anything else in play here?

Thanks for taking time to discuss mate :)

Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-33463649
.

@zathras-crypto
Copy link
Contributor

Cool - I'll just add my PoV on 'bad clients'. If a client is bad, you're (to steal a quote) attached to another object by an inclined plane, wrapped helically around an axis.

ie - screwed. You're giving it the authority to sign transactions on your behalf, there's no need for trickery it could just send your balance anywhere it wanted.

For broadcasted transactions, as long as the protocol handles these messages in a defined manner, it'll handle them this way regardless of user intent.

EDIT: for clarity

@marv-engine
Copy link
Author

Zathras, Maran,
Thanks for taking the time to walk through the logic. That clarified
several things for me. I've come away with 4 general conclusions:

  1. The spec has to address all potential sequences and not leave
    anything to someone's (mis)interpretation. In this case, there
    are several variations for the Sell Offer/Purchase Offer interactions that
    can happen asynchronously in addition to: 1) sell offer, then 2)
    purchase full amount, then 3) purchaser pays on time - the seller can
    change/cancel the offer, one or more purchasers can accept a partial
    amount, purchasers can choose to not pay. In all cases, we want all
    participants to be protected from performing unintended actions, e.g. the
    seller creating a new sell offer, or the buyer accepting a sell offer not
    knowing the seller changed the offer terms. Including an offer id that's
    unique to the latest sell offer terms would protect the buyer. If the sell
    offer has changed, the buyer client would need the new offer id (and
    therefore have its terms) in order to accept it.
  2. Enforcement can be done by the protocol or the client or both,
    wherever it makes sense. The spec has to be clear about whose
    responsibility it is. In any case, the client must present the most
    up-to-date information available about the offer so the participant can
    make an informed decision to proceed.
  3. To deal with client misbehavior, there's a business opportunity here.
    An independent organization can offer to do client testing and provide a
    badge to clients that pass. This will give users an additional level of
    confidence that the client is doing the right thing for the user.
  4. There should be a way for transaction definitions to change (because
    they will) without invalidating old versions. I don't mean that old
    versions can be created beyond a certain point in time (block number) if
    that's the right rule. What I mean is that clients should be able to parse
    old versions. I have suggested 2 approaches - 1) use a byte in the
    transaction type for the version number (255 should be more than enough),
    or 2) include bytes reserved for future use. The version number is better
    because it's explicit so the client can easily detect it and parse the
    message, and it allows more flexibility to change the transaction content.

Marv Schneider
VP, User Experience/Product Usability
Engine, Inc.
marv@engine.co
240-462-6123 cell

On Tue, Jan 28, 2014 at 5:07 AM, zathras-crypto notifications@github.comwrote:

Cool - I'll just add my PoV on 'bad clients'. If a client is bad, you're
(to steal a quote) attached to another object by an inclined plane, wrapped
helically around an axis.

ie - screwed. You're giving it the authority to sign transactions on your
behalf, there's no need for trickery it could just send your balance
anywhere it wanted. As long as the protocol handles these messages in a
defined manner, it'll handle them this way regardless of user intent.

Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-33465255
.

@dacoinminster
Copy link
Contributor

I'd like to settle this in our conference call in 45 minutes. I hope you
all can join in for that.

There is no way for implementations to smoothly handle this problem without
changing the spec in some way. Marv's way looks good, but it would be
annoying if it breaks all our old test MSC sales on the dEX.

On Tue, Jan 28, 2014 at 8:23 AM, Marv Schneider notifications@github.comwrote:

Zathras, Maran,
Thanks for taking the time to walk through the logic. That clarified
several things for me. I've come away with 4 general conclusions:

  1. The spec has to address all potential sequences and not leave
    anything to someone's (mis)interpretation. In this case, there
    are several variations for the Sell Offer/Purchase Offer interactions that
    can happen asynchronously in addition to: 1) sell offer, then 2)
    purchase full amount, then 3) purchaser pays on time - the seller can
    change/cancel the offer, one or more purchasers can accept a partial
    amount, purchasers can choose to not pay. In all cases, we want all
    participants to be protected from performing unintended actions, e.g. the
    seller creating a new sell offer, or the buyer accepting a sell offer not
    knowing the seller changed the offer terms. Including an offer id that's
    unique to the latest sell offer terms would protect the buyer. If the sell
    offer has changed, the buyer client would need the new offer id (and
    therefore have its terms) in order to accept it.
  2. Enforcement can be done by the protocol or the client or both,
    wherever it makes sense. The spec has to be clear about whose
    responsibility it is. In any case, the client must present the most
    up-to-date information available about the offer so the participant can
    make an informed decision to proceed.
  3. To deal with client misbehavior, there's a business opportunity here.
    An independent organization can offer to do client testing and provide a
    badge to clients that pass. This will give users an additional level of
    confidence that the client is doing the right thing for the user.
  4. There should be a way for transaction definitions to change (because
    they will) without invalidating old versions. I don't mean that old
    versions can be created beyond a certain point in time (block number) if
    that's the right rule. What I mean is that clients should be able to parse
    old versions. I have suggested 2 approaches - 1) use a byte in the
    transaction type for the version number (255 should be more than enough),
    or 2) include bytes reserved for future use. The version number is better
    because it's explicit so the client can easily detect it and parse the
    message, and it allows more flexibility to change the transaction content.

Marv Schneider
VP, User Experience/Product Usability
Engine, Inc.
marv@engine.co
240-462-6123 cell

On Tue, Jan 28, 2014 at 5:07 AM, zathras-crypto <notifications@github.com

wrote:

Cool - I'll just add my PoV on 'bad clients'. If a client is bad, you're
(to steal a quote) attached to another object by an inclined plane,
wrapped
helically around an axis.

ie - screwed. You're giving it the authority to sign transactions on your
behalf, there's no need for trickery it could just send your balance
anywhere it wanted. As long as the protocol handles these messages in a
defined manner, it'll handle them this way regardless of user intent.

Reply to this email directly or view it on GitHub<
https://github.com/mastercoin-MSC/spec/pull/41#issuecomment-33465255>

.

Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-33494653
.

@DavidAJohnston
Copy link
Contributor

Sounds like this was settled on the call. Time to merge?

@zathras-crypto
Copy link
Contributor

Oh no - did that discussion take place while I was buying the skypeout stuff to dial in to the second number? Could one of the devs just briefly summarize the conclusion for me?

@marv-engine
Copy link
Author

Zathras,
We're going with PR 41 - adding the Action byte. Also, I will submit
another PR to use one of the bytes in the transaction id as a version
number. We'll take advantage of the fact that 3 of the bytes are 0, and
we'll use one of those bytes to indicate the current definition is version
0. This will apply to all the transaction types.

Marv Schneider
VP, User Experience/Product Usability
Engine, Inc.
marv@engine.co
240-462-6123 cell

On Tue, Jan 28, 2014 at 4:55 PM, zathras-crypto notifications@github.comwrote:

Oh no - did that discussion take place while I was buying the skypeout
stuff to dial in to the second number? Could one of the devs just briefly
summarize the conclusion for me?

Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-33531215
.

@zathras-crypto
Copy link
Contributor

Fair enough. Plan for existing transactions?

Marv Schneider added 10 commits January 29, 2014 19:22
Every transaction definition now includes a backward compatible version number, to accommodate changes to the definitions.
Added explanation about reading 1st byte to determine message version number. Various other spelling corrections, clean up.

Still more tx definitions to complete.
several questions embedded in the text, in bold.
e.g. for updating/canceling sell & buy offers
@ABISprotocol
Copy link

Curious about how this treats transactions that are tiny (or, perhaps, bundled microdonations that are confirmed by an entity who is the receiver).

@dacoinminster
Copy link
Contributor

Zathras: I believe these changes are backward-compatible with old
transactions.

Marv: This can't be auto-merged because of conflicts. Can you pull from the
repository and resolve conflicts?

All: We do need to get rid of the inline questions. I hope to look at them
in detail tomorrow, but if you guys can get rid of them without me, that
would be great. Overall, I'd prefer not to make any changes that break
backwards compatibility unless we have to for some reason, and then only on
Test MSC.

On Wed, Feb 5, 2014 at 1:11 PM, ABIS notifications@github.com wrote:

Curious about how this treats transactions that are tiny (or, perhaps,
bundled microdonations that are confirmed by an entity who is the receiver).

Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-34245936
.

@dacoinminster
Copy link
Contributor

These changes were included via a different pull request

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

Successfully merging this pull request may close these issues.

None yet

7 participants