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

Get rid of Exodus output #281

Closed
dexX7 opened this issue Oct 29, 2014 · 56 comments
Closed

Get rid of Exodus output #281

dexX7 opened this issue Oct 29, 2014 · 56 comments

Comments

@dexX7
Copy link
Member

dexX7 commented Oct 29, 2014

@zathras-crypto suggested the removal of the Exodus address quite some time ago.

Is this feasible? What's your opinion? What are alternatives?

@dexX7
Copy link
Member Author

dexX7 commented Mar 18, 2015

When considering the whole topic, it's important to understand there is no perfect solution, and there are two conflicting goals:

  1. For fast transaction processing, meta transactions should be easy to identify.
  2. Meta transactions should not be easy to identify, to increase privacy and censorship resistance.

Ideally, the mechanism and parameters are chosen such that the cost of identification is minimal, but high enough to make it a difficult decision, whether it's worth, in terms of additional cost of computation power invested, to identify transactions.

For the discussion of finding a new machanism, let's assume a) payloads were fully obfuscated or compressed and appear to be just random data, indistinguishable from other transaction data on the chain, and b) there were no other telling factors, which indicate whether a transaction might be a meta transaction. When mentioning the later, I'm mostly thinking about address reuse, and since #62 is on the table, it's perfectly feasible to avoid address reuse in the future.

We can distinguish between two general directions:

1) Stateless transaction identification:

Statelessness is used in the context of being able to determine whether a transaction qualifies as potential meta transaction, without the need of any information other than the transaction itself.

It basically boils down to some distinct characteristics to identify a transaction, for example:

  • a dedicated output, such as the Exodus output
  • some clearly visible magic bytes, such as a prefix of a payload
  • some magic bytes that are visible after doing a transformation

The later could be "take the hash of the first transaction input, xor it with the data to test, and if the result contains some magic bytes, the test is passed", or similar.

This is straight forward and very solid, as there is no room for any misinterpretation.

2) Stateful transaction identification:

This is an extremely interesting topic in my opinion, but likewise complex. The goals mentioned above can actually be refined to "transactions should be easy to identify by active participants in the eco system, but difficult to find by anyone else".

There would be distinct characteristics of some kind, but instead of using static magic bytes as identifier, such characteristica would be dynamic and dependent on the state of the system.

For the sake of an example let's say "meta transactions have an output which mirrors one of the coinbase transaction output scripts of one of the last 1000 blocks, but at minimum 100 blocks, with an output value of at least 0.0001 BTC", or "meta transaction payloads are prefixed with a timestamp, which differs no more than 48 hours from the timestamp of the block the transaction is part of".

Those examples provide no gain and fail to fulfil the refined goal, and were chosen rather arbitrary.

3) Stateful transaction identification without identifier:

There is actually no strict requirement of a distinct characteristica at all, nor does any of the above mentioned identifiers provide any guarantee that an identified transaction is valid.

It is thinkable to handle any potential payload as payload and continue with transaction processing, namely decoding, deobfuscation or decompression and further validation and interpretation of the data. This process fails for any invalid transaction at some point, whether it's because the payload doesn't match a known transaction type, or ultimately due to a violation of consensus rules, for example an attempt to transfer tokens of an unknown property. This process happens either way, because meta transaction are not validated by Bitcoin miners.

It is crucial to understand that common identification mechanisms, such as the use of some magic bytes, are merely hints, which serve as first filter to speedup transaction processing, expressing "this transaction has some distinct characteristics, therefore it may be a meta transaction, and it's safe to skip other transactions without such characteristics".

On the first glimpse such a "no identifier" approach looks like a huge performance blocker, but I'm not entirely sure, if this is true, and I'm going to do some tests later.

4) Consensus state dependent identification:

Let's recall the refined goal of transactions that are "easy to identify by active participants, but difficult to find by anyone else", and this is an idea I had some time ago, but still haven't put much thoughts into, so I'm going to stick to the general concept:

Mastercoin is a stateful protocol and might be be considered as deterministic state transition system, where transactions can trigger state transitions, and where valid transactions definitely trigger state transitions. The current state is the result of a sequence of transitions, and therefore transactions, involving properties such as balances, known properties, crowdsales, offers, [...].

Where I'm getting: what if a compact representation of a previous state would be used as identifier..?

I'm going to stop here, as this is a rather complex topic and there are quite a few things to consider, and at this point I don't want to shift the focus on consensus state dependent transaction markers, but rather to fuel the discussion about getting rid of the Exodus output.

4) TL;TR:

At this point I think a feasible and reasonable alternative to the Exodus output is either the use of magic bytes within some obfuscated chuck of data, or no distinct identifier at all. I'll update, once I collected some data regarding the "no identifier" approach.

If it turns out that the "no identifier" approach comes with unbearable slowdowns, and hiding identifiers within obfuscated data chucks is still too far, then clearly visible identifiers that could become part of the payload of OP_RETURN or "Class C" encoded transactions might be used as intermediate solution.

I'd like to invite @lukejr, not because of any blacklists used by Eligius, but because the reasons for blacklisting Master/Omni transactions are reasonable to some degree, and input would be very appreciated. :)

@dexX7
Copy link
Member Author

dexX7 commented Mar 18, 2015

Ouch.

Even though this is based on only a few hundred thousand transactions tested, it currently looks like a "no identifier" approach is.. well let's say.. significantly slower (approx. 5-30x).

The path usually ends with "no matching DEx offer found". Though it's worth to mention that I disabled any logging for obvious reasons while testing.

There is some room for optimization, but the main reason for the slowdown is related to the fetching of input transactions to determine a sender of each transaction vs. fetching a whole block and processing it's transactions sequentially (with an early exit, if no Exodus output was found).

@zathras-crypto: you asked why I'd prefer to select the sender solely based on the first vin: here is another reason why.. ;) But that aside, this is merely the tip of the iceberg.

There is another interesting aspect: DEx payments are currently pure Bitcoin transactions, with or without Exodus marker, but more importantly: those transactions are without an explicit data payload, which indicates "this is a meta transaction, and in particular a BTC payment". It actually seems reasonable to me to attach one.

@zathras-crypto
Copy link
Contributor

Well, sadly the expected consolidation of outputs at the Exodus address doesn't seem to be occurring by those with keys for it, so I do agree the Exodus marker is undesirable. It's another output that gets left lying around the UTXO set. The whole reason for my work to move us to OP_RETURN is too many multsigs don't get redeemed hence polluting, so Exodus is kind of more of the same issue.

I would feedback that I believe any change to this must apply to Class C only since that's a new and default tx class. I don't want to break any other implementations that may exist so suggest that current transaction classes (A/B) remain as is.

From a code PoV the path of least resistance is to swap out the Exodus marker with something that serves the same purpose but carries less weight (bytes) - an entire output is excessive when considered in this context, but removing said output also needs to be weighed against the loss of the ability to pull all Omni transactions simply by watching the Exodus address.

Re obfuscation, as I've mentioned on other threads I see zero value to this while there is an Exodus output (since that's the easiest method to detect our transactions), but even on removing it - do we still want to obfuscate? I personally see the Bitcoin mining future as one of mining policies and so the question is whether we want to get into an Omni transaction obfuscation vs detection race. My own belief is that mining policies will likely focus on the script type & size rather than script content. Don't get me wrong, I'm not an authority on this and I guess it is feasible to have a policy against cleartext encoded data, it just would seem an unusual direction to say "OP_RETURN 1ab2randomdata is valid" but "OP_RETURN 0001cleardata" is not valid.

Long story short the Exodus marker is a big piece of optimization, it allows us to drop processing of a transaction immediately without doing any further work. Ideally a lighter weight replacement would be used - how many bytes are necessary to avoid the chance of a collision I guess would be up for debate, but if it's small enough it could form the first bytes in the OP_RETURN output and serve as a 'drop' in replacement (parseTransaction can simply have the Exodus marker identification code extended to check the first n bytes of a NULL_DATA output.)

There is another interesting aspect: DEx payments are currently pure Bitcoin transactions, with or without Exodus marker,

FYI DEx payments do need to include an Exodus marker in order to be valid.

@dexX7
Copy link
Member Author

dexX7 commented Mar 19, 2015

Ideally a lighter weight replacement would be used - how many bytes are necessary to avoid the chance of a collision I guess would be up for debate, ...

In my opinion it seems even more desirable to have no marker at all, because collision doesn't seem like an issue, given other constraints and requirements for transaction validity.

I claim that there is basically no way to create a transaction with "effect" accidently, because payloads have a very specific format, starting with type and version prefix, which already act as sort-of identifier. The chance that such a payload is found and the sender has sufficient funds and this was not intended appears to be non existing.

FYI DEx payments do need to include an Exodus marker in order to be valid.

Yes, I mentioned it in the sense that it would be nice to have a meta payload here as well, unrelated to a marker, as this would basically resolve the issues around "no marker" or a "lighter weight marker" within the payload.

but removing said output also needs to be weighed against the loss of the ability to pull all Omni transactions simply by watching the Exodus address.

Do you have a specific use-case in mind? I have thought about a few, for example the ability to pull transactions via some data provider such as blockchain.info, but I'm not sure, if this is desirable, given the additional risk.

This affects thin-clients in general: even if transactions can be pulled externally (web API, Electrum like, via bloomfilters, ...), or it can be shown that transactions are valid Bitcoin transactions, it appears much more difficult or impossible to determine, if some data is missing, which seems rather crucial.

Edit: this doesn't mean I see no options for thin-clients, and something similar to Electrum is probably the way to go, but I believe such systems would be specialized either way.

And for what it's worth: chain.com can be used to pull OP_RETURN data.

@zathras-crypto
Copy link
Contributor

I claim that there is basically no way to create a transaction with "effect" accidently

This I have to disagree with mate. It assumes the address will never be used with any other future service that may encode data in OP_RETURN. A couple of bytes and a couple of integers is not what I'd call unique data types and it is entirely feasible in my opinion that some future service may encode data similarly to us. The most obvious example is if someone forks us into a new project.

The alternative is to say to our users "you should not use your address for anything but Omni" but I think that is overkill and unnecessary. A unique marker serves not only identification purposes but is also (just IMO) a directive from the sender to "take the Omni action described in the payload". I don't really buy that accidents here are impossible (I do agree they're fundamentally and grossly unlikely, but that's not the same thing as not possible).

Do you have a specific use-case in mind?

Not really, just maximal flexibility. Ability to watch Exodus for txs is a comment that was put to me (I'd have to go hunting to cite) when I first raised getting rid of Exodus so wanted to include in the discussion for completeness.

@zathras-crypto
Copy link
Contributor

Following on from my previous comment, even just prefixing four bytes OMNI at the beginning of the OP_RETURN output costs us only 5% of our potential packet space (not enough to make a difference, everything still fits Class C with the exception of issuances).

That swaps what maybe, 35? bytes of Exodus output for 4, but still gives all the benefits of an identification marker with the primary loss being the "watch Exodus for transactions" process (how much or little benefit that is I'm unsure).

@dexX7
Copy link
Member Author

dexX7 commented Mar 19, 2015

Sorry, I hit the wrong button and didn't want to edit, so:

OMNI

I see your point about an explicit directive, but I think a 2 byte marker should be sufficient. Maybe it's more appealing, if transaction version and type are considered as implicit markers, e.g. with "om" (note: there is "OA", so lower case seems more reasonable):

0x6f6d 0000 0000
0x6f6d 0000 0003
0x6f6d 0000 0014
0x6f6d 0001 0014
0x6f6d 0000 0015
0x6f6d 0000 0032
0x6f6d 0000 0033
0x6f6d 0000 0035
0x6f6d 0000 0036
0x6f6d 0000 0037
0x6f6d 0000 0046

But let's think about alternatives. Maybe there are some. I liked the idea of sequence numbers in theory, and maybe there is a way to turn a plain marker into something more functional, such as tagging outputs as reference, or other parts of the transaction.

Alternatively, what might look more random on the first glimpse: the use of some kind of pointers, for example: two offsets, where the bytes pointed to equal X, when doing a specific operation, say byte1 * byte2 = X. This would be easy to verify, and when considering more than the payload, there is plenty of bytes to choose from. But this just as an idea.

Very important in my opinion is to use in any case at most 4 byte, but preferably less. As posted in another thread, this is how a simple send with "virtual" reference could look like:

6f6d|0000|0000|00000003|00000000000001f4|dd84a40ba1d6efd7d303e2c3ca80ced2a9b3d45f|____
    |ver |type|  curr  |  num-of-coins  | ref: 1MCHESTxYkPSLoJ57WBQot7vz3xkNahkcb|2 byte unused

With a 4 byte marker the payload would have a size of exactly 40 byte, which fits nicely into one small sized OP_RETURN, but when thinking about embedding multiple transactions, there might be the need of some kind of seperator, terminator, flag or pointer, to indicate some other data is not junk, but indeed another payload.

@zathras-crypto
Copy link
Contributor

Just wanted to mention that I don't support virtual references for basic transactions like simple send any time soon (though I do support them for advanced functions involving multiple recipients). Long story short we're really ramping up with more and more companies integrating with the Omni Layer and I know that the reference output is functional in many of these integrations. From notification of an inbound transaction (the BTC output to reference address) through to sending additional BTC in the reference output to enable the recipient to then fund their next transaction etc there are various uses in play already so it is (obviously just IMO) just too substantial of a shift at this stage. It'll cause a bunch of grief for our partners/integrators and doesn't enable any new (or improve any existing) functionality - so can't see it getting much traction to be honest.

Reference outputs are redeemed and spent along with new transactions - I have no objection to using these - my 'beef' as it were was with UTXOs not being redeemed (Exodus, multisigs etc) and hence Class C work and considering dropping Exodus :)

Really hoping some others can weigh in with views here!! The last txindex change didn't go down too well (even though the discussion was open here, just no one participated) so for something like this we really need some more input!!!

Thanks
Z

@zathras-crypto
Copy link
Contributor

Note, not to say that I don't agree with your reasoning - marker should be kept to 4 bytes or less to allow such a virtual reference simple send to occur in future - just wanted to be upfront that it'll be a while before we can adopt such a practice.

@zathras-crypto
Copy link
Contributor

Oh sorry, one more thing - I'm sure I mentioned this on another discussion relating to reference, but I always thought we would end up simply storing the vout for the reference in the data packet. It costs us a single byte and removes any ambiguity as to which vout is the reference. Don't think I ever took it anywhere though...

@dexX7
Copy link
Member Author

dexX7 commented Mar 19, 2015

Note, not to say that I don't agree with your reasoning - marker should be kept to 4 bytes or less to allow such a virtual reference simple send to occur in future - just wanted to be upfront that it'll be a while before we can adopt such a practice.

Perfect. My intend was to find reasons to justify a short marker, not to push virtual references. :)

@dexX7
Copy link
Member Author

dexX7 commented Mar 20, 2015

@zathras-crypto: I'm nervertheless curious and would love to hear more about the actual use-cases of integrators, in the context of:

I know that the reference output is functional in many of these integrations.

Where the following is straight forward:

... through to sending additional BTC in the reference output to enable the recipient to then fund their next transaction

But I'm interested in:

From notification of an inbound transaction (the BTC output to reference address) ...

I assume you are referring to something like "the receiver gets a "ping"" or "the user can see a transaction on blockchain.info" for example. Is that correct? I'm asking, because this sounds related to the issues that STO transactions caused in the UI, because there are only Bitcoin signals (whether on a code level, or in form of user notifications). And I further assume you are not referring to -walletnotify=cmd in particular? I'm just trying to understand the issue, not trying to make a point, and based on those assumptions, it looks like the underlying issues are:

  • missing signals for meta transactions on several layers
  • (the need of) a more widespread use of meta transaction explorers

At this point it's not all that difficult to handle, because transactions are usually bound to "pings" of Bitcoin transactions, with a very few exceptions, but this is going to change very drastically with the upcoming meta DEx integration, which sort of sounds concerning, after reading your statements?

@zathras-crypto
Copy link
Contributor

Sadly I couldn't advise on all the various integration scenarios - I just don't have the visibility. Perhaps @achamely @CraigSellars could advise? But regarding "receiver gets a ping" that's essentially it yep. Let's take an exchange, many of our exchange partners offer unique deposit addresses to each user, and thus need to know when they have received an inbound transaction. The simplest way I know of to do that would be via walletnotify but I assume there are various methods they could be using for this.

If we look at say MetaDEx, I don't see deposits to webwallets/exchanges/services coming in the form of a MetaDEx trade. Same thing with STO - there is no reference output there but it's not going to be used as the method of transfer for sending tokens to one of our integrators or partners.

That does open an interesting question though - let's say I have deposit addresses with DexXExchange for ZathToken and then issue an STO on ZathToken - would the exchange register funds received in that STO as a deposit from the sender? Probably not - reflecting on this perhaps our partners should be displaying a notification that only simple sends are supported for depositing funds? I don't really see this as an issue for MetaDEx as unless the integration partner actually opens a trade from a deposit address, it's not going to be an issue.

Even if we say trigger walletnotify ourselves we still need to change everything that works on the principles of having a transaction in the wallet (from listtransactions_MP to history in UI). TL:DR; I just see it as too big a scope of change for not enough benefit.

Hope that makes sense :)

@dexX7
Copy link
Member Author

dexX7 commented Mar 20, 2015

Sadly I couldn't advise on all the various integration scenarios - I just don't have the visibility. Perhaps @achamely @CraigSellars could advise?

That would be really interesting. :)

Even if we say trigger walletnotify ...

I wasn't thinking about this in particular, but more about the general topic. And you're right, it's likely that exchanges won't be affected by meta DEx trades, which seems more an issue for the UI. Just trying to grasp the bigger picture, so to speak, since all this seems related.

Which sort of brings us back to the original topic: having an explicit Exodus output with "ping" vs. a marker of some other kind, as you suggested with an "OMNI" (or "om") prefix in the OP_RETURN payload.

@zathras-crypto
Copy link
Contributor

Oh bleep, sorry mate this was sitting in a tab without clicking the comment button :( d'oh

Which sort of brings us back to the original topic: having an explicit Exodus output with "ping" vs. a marker of some other kind, as you suggested with an "OMNI" (or "om") prefix in the OP_RETURN payload.

The reason I'm open to this is because the Exodus outputs are not being redeemed, nor are multisigs - so that's my primary driver for getting rid of them. I don't have this concern on reference addresses. A 'ping' to the Exodus address serves very little benefit (that I know of), where-as the 'ping' to the reference address provides many benefits (incoming tx notification, a tx in the wallet to work with for history and so on).

Long story short:

Exodus: I'd like to hear others comments on getting rid of the Exodus marker and replacing it with either a 2 or 4 byte om/OMNI in the payload for Class C - I'm comfortable with it myself, have already toyed with code for it locally and it's not much of a difference from current. If there is a reason for it to stay, we need to know about it ASAP. Otherwise I see no reason to spawn a new polluting UTXO every transaction (the exodus marker), the whole point of Class C is to remove polluting elements from our transactions.

Reference: I support this staying as is for now.

@dexX7
Copy link
Member Author

dexX7 commented Mar 22, 2015

To further clarify and discuss: the change of marker is only going to affect class C encoding, while, say for example bare multisig transactions, must nevertheless be constructed with Exodus output, which is also verified and such a transaction otherwise considered as invalid?

@dexX7
Copy link
Member Author

dexX7 commented Mar 22, 2015

Actually, even when data is embedded in bare multisig, there could be an OP_RETURN output, which solely holds a marker - assuming there no cross-encoding-scheme-data-embedding applied, where half of the data is in an OP_RETURN output and the other half in multisig pubkeys. But I guess that's a bit too far?

@zathras-crypto
Copy link
Contributor

To further clarify and discuss: the change of marker is only going to affect class C encoding

Yes (imo). Retroactively applying a change of marker to existing transaction classes would invalidate existing tools made to create those transactions (the advisor Class A script, the python scripts - even your bitwatch tool :P) unless we made the change optional.

Actually, even when data is embedded in bare multisig, there could be an OP_RETURN output, which solely holds a marker

In theory yes, you could treat 'the' marker as being an either/or - eg say that a paytopubkeyhash output to Exodus or a OP_RETURN output that begins with om/OMNI constitutes a marker regardless of transaction class, but then we need to return to the whole class fall back question (eg if an OP_RETURN packet is present, we don't know until later in interpretPacket whether that's valid Omni messaging and it's a departure to go back and reprocess as Class B). I thus question the usefulness of introducing this kind of extra complexity. I believe that 99%+ of our transactions will go out Class C (issuances are the only tx so far that doesn't fit, and at last check issuances were a fraction of one percent of our txs).

where half of the data is in an OP_RETURN output and the other half in multisig pubkeys. But I guess that's a bit too far?

I don't object on principle - I just don't think the edge cases that would be satisfied by this capability are worthy of resourcing - eg in the context of "mixed Class B & Class C encoding" or say "saving address feature" personally I'd be pushing to do the savings address thing. Just one opinion though and just because I don't personally see very much advantage in mixed encoding, doesn't mean that the project as a group thinks the same. Come on get involved guys, someone must have an opinion, there are at least 10 odd people at the weekly meetings!!!

Thanks
Z

@dexX7
Copy link
Member Author

dexX7 commented Mar 22, 2015

Retroactively applying a change of marker.

Yeah, just wanted to clarify, and I was more thinking about something like class B v2, but I agree regarding the logic overhead and I'd rather prefer to get class C out ASAP and then do another switch at some point in the future, which covers everything, from bare multisig to OP_RETURN, markers, cross-encodings, maybe compression, etc., to avoid ending up with A, B, C, D, E, F...

Well, and at some point, but this is probably also stuff for a different topic, old encoding schemes should be depreciated, say "class A is no longer accepted after block n".

Edit:

bitwatch tool

Hm..? Are you referring to the private API for Alvin with the transaction deserialization and creation patch or the web builder? The later constructs raw transactions, but doesn't tackle encoding or how they end up in actual transaction, and the results can simply be used as input for sendrawtx_MP. :)

This is actually an important topic which you mentioned here: ideally third party tools should not be affected by changes, if they are used in combination Omni Core, e.g. by pushing transactions via sendrawtx_MP (to name one example). The whole bitcoin-spock test framework is probably a more relevant example, which should mostly be fine despite the actual encoding that is used. And as it turned out, it works very well with your class C branch - except the one test that fails, because a simple send has no class B characteristics. :)

I'm not up-to-date on this, but at some point I noticed OmniWallet has it's own implementation of transaction decoding, which might be affected though.

@zathras-crypto
Copy link
Contributor

I'd rather prefer to get class C out ASAP

I would also, but I also think there is a lot of testing and work for MetaDEx before the next release and that's going to take time, so if there are good ideas that don't take much work I'm definitely open to including them in Class C. For example moving Exodus marker to the data packet in the OP_RETURN output just for Class C transactions is a relatively simple change from a code perspective - of course if we decide we want to do that.

old encoding schemes should be depreciated, say "class A is no longer accepted after block n".

JR was very specific about always being able to fall back to Class A - IIRC it was to support a full range of bitcoin wallets being able to do basic transfers of tokens even if they were not Omni aware. I'm not sure how relevant that is today.

@dexX7
Copy link
Member Author

dexX7 commented Mar 24, 2015

IIRC it was to support a full range of bitcoin wallets being able to do basic transfers of tokens even if they were not Omni aware.

The more I think about it.. we actually already soft-deprechiated class A by not providing any way to create class A encoded transactions. I guess that's fine and ideally we open up on the decoding side as much as possible, i.e. support, but not encourage, pub-key-hash data embedding.

So if there are good ideas that don't take much work I'm definitely open to including them in Class C.

I'm making progress on the modularization branch, and I'm pretty close to "embed or extract any data in or from any output". I think and hope some parts of that can be adopted at some point, which could provide a base for mixed encodings and related.

What I'm wondering about the most though is how to make sense of the data, so to speak. Not in the sense of data interpretation, but rather payload identification across multiple outputs and script types. Or let me rephrase: some of the current processing rules in that context are "if not class B, then class A", "class B payloads are ordered", "the first public key of multisig outputs is skipped and the others considered as payload", and similar rules would have to be defined for a broader model, or there would be some other mechanism to explicitly tag payloads. Once the basic decoding works, I'm going to pull the byte serialization in and take a stab at multi-transactions within transactions. At least on that level (data embedding, extraction and transformation in and from plain transaction objects) it shouldn't be too difficult to get a POC up.

@zathras-crypto
Copy link
Contributor

As discussed in meeting guys, Class C has three options:

  1. Retain original Exodus marker
  2. Drop Exodus marker and use a 2 byte marker om at the beginning of the OP_RETURN output
  3. Drop Exodus marker and use a 4 byte marker omni at the beginning of the OP_RETURN output

Class C as written currently adopts 3 as that is my preference (4 bytes, minimal chance of collision, virtual ref still possible etc etc).

Please comment :)

@achamely
Copy link
Contributor

I like the idea of 3, its clean and simple. would the omni marker in op_return be the only addition or would we include/leverage the space for additional functionality

@zathras-crypto
Copy link
Contributor

FYI Class C performs the following:

  • Moves the 'marker' from an Exodus output to a byte stream at the beginning of OP_RETURN pushdata (no Exodus cost anymore)
  • Moves the data packet from multisig outputs to the OP_RETURN output (no multisig cost anymore)

This has two key benefits I was shooting for:

  • Get rid of these crappy outputs that are not being redeemed and thus bloating the UTXO set (both Exodus and multisigs are culprits)
  • Reduce transaction cost further (cost should now simply be miningfee + reference output only)

FYI internally I've refactored it all into a classAgnostic_send so basically programmatically now all we need to do is pump in the payload we want - we don't need to worry about which class or encoding etc, it's all handled automatically.

@CraigSellars
Copy link
Member

I also agree with option 3 as the correct direction. I assume that Class B of course will continue to use exodus?

@CraigSellars
Copy link
Member

Something to keep in mind - I’ve been toying with the idea of leveraging ANYONE_CAN_PAY transactions at some point, but that of course will break our rules of which sending address is the correct one to debit… in terms of ordering, etc., just something to consider.

@zathras-crypto
Copy link
Contributor

I guess it's a long thread - few posts up :P hehe but yep Class B will continue to use Exodus.

To further clarify and discuss: the change of marker is only going to affect class C encoding

Yes (imo). Retroactively applying a change of marker to existing transaction classes would invalidate existing tools made to create those transactions (the advisor Class A script, the python scripts - even your bitwatch tool :P) unless we made the change optional.

@CraigSellars
Copy link
Member

I guess it's a long thread - few posts up :P hehe

Doh.

@zathras-crypto
Copy link
Contributor

I’ve been toying with the idea of leveraging ANYONE_CAN_PAY

That is managable I believe - the simplest model is to put an exception into the input invalidation rules and say that ANYONE_CAN_PAY outputs are simply ignored during sender identification.

That way as long as the sender could contribute at least something, another party could fund the rest of the transaction.

Though to note, Class C transactions are much cheaper :)

@CraigSellars
Copy link
Member

I'm not up-to-date on this, but at some point I noticed OmniWallet has it's own implementation of transaction decoding, which might be affected though.

Curoius what’s missing in Core that Omniwallet needed to do it on its own…

@CraigSellars
Copy link
Member

That way as long as the sender could contribute at least something, another party could fund the rest of the transaction.

Is that true? I suppose if it werent, there would be no protection on the p2p network to prevent spam from ANYONE_CAN_SPEND where the signer has provided nothing. Pity. I’m secretly hoping even an empty address could broadcast something and let another pick up the tab. Wholle separate topic though.

@achamely
Copy link
Contributor

Omniwallet has its own decoder for pending tx support (i.e. to decode the tx's its creating sending). Eventually we're going to want to roll tx decoding and (if possible) tx creation into omnicore entirely. i.e. omniwallet calls omnicore and says i want to send tx type X with info Y and core returns the unsigned tx for us to give the user to sign.

@zathras-crypto
Copy link
Contributor

@CraigSellars
The sender has to contribute an input so we know who the sender is :)

ANYONE_CAN_SPEND != ANYONE_CAN_PAY

What I think (from previous conversations) you're looking for is for a 3rd party to be able to fund the transaction costs for a send. If that's correct, I'd posit we need at least one input from the sender to identify who the sender is and to sighash the outputs (otherwise they could be changed). The rest of the cost could be covered by an ANYONECANPAY input potentially.

@achamely
I'd agree I'd like to see all this in OmniCore but we hit the dreaded UTXO issue again for senders not in the wallet (and you don't want to add every user to some huge watch-only wallet hehe). I would like to extend decoding to raw and pending, we just have a large amount of other priorities sadly :(

@CraigSellars
Copy link
Member

I'd posit we need at least one input from the sender to identify who the sender is and to sighash the outputs (otherwise they could be changed).

I was thinking a combo of ANYONECANPAY and another address providing some input (likely from the same wallet), but without an unspent output, you couldn’t identify the sender. :-/ Let’s table this side discussion.

@dexX7
Copy link
Member Author

dexX7 commented Mar 31, 2015

Vote for 2 byte or less.

I’ve been toying with the idea of leveraging ANYONE_CAN_PAY transactions at some point, but that of course will break our rules of which sending address is the correct one to debit… in terms of ordering, etc., just something to consider.

This has nothing to do with OP_RETURN, but actually one of the reasons why I was pushing for a new input selection where the first input = sender, to allow funding by a third party (namely the "anyone" in this scheme, which is currently difficult, because the outcome of "contribution by sum" is not very "foreseeable"), ultimately to work towards atomic BTC/token trading.

But speaking of anyone-can-spend: follow property id 2147483670 on testnet.. ;)

@dexX7
Copy link
Member Author

dexX7 commented Mar 31, 2015

To get an idea for the SINGLE|ANYONECANPAY scheme:

goldcoins

This was based on @petertodd's Decentralized digital asset exchange with honest pricing and market depth post.

Edit: this scheme, as it is, would be vulnerable to double-spending of tokens due to the balance based nature of MSC, but there are two ways around: one involving third-party trust, where the party cannot steal per se, or by introducing a new transaction type to bind tokens to an output.

@zathras-crypto
Copy link
Contributor

Vote for 2 byte or less

Or less?!? :P My reason for 4 bytes is simply one of mitigating risk of collision - 2 bytes is a 1-in-65,000 risk, 4 bytes is a 1-in-4,228,250,000 risk (much more palatable).

There is no Omni message we can do in 38 bytes that we cannot do in 36 (as far as my research has gone - even including your virtual reference idea), so for me it came down to: there are positives to using 4 bytes (collision avoidance) but no (obvious) negatives.

EDIT: Even less of an issue with the move to 80 bytes.

But actually one of the reasons why I was pushing for a new input selection where the first input = sender,

I can't say I agree a new input selection routine is required for ANYONECANPAY, the input can simply be excluded. I just can't see the benefit to this for @CraigSellars use case (I'll get to token trading in a mo). The simple fact is we need at least one input from the sender for multiple reasons (most importantly to prove the transaction is authorized) and since we have the ability to send transactions that cost what, under a thousand satoshis with Class C I disagree it's a use case worthy of resourcing (it would only be of use in those specific scenarios where the sender had some BTC (not zero) but that BTC balance was under 0.00001 BTC.

TL:DR; assisting senders that have >0 but <0.00001 BTC is of such limited scope I see many, many things in our pipeline more worthy of attention.

Again though that's just one persons opinion :)

ultimately to work towards atomic BTC/token trading.

Now, that's a use case worth resourcing, but you're fighting an uphill battle there because by taking MSC out of the equation for trading tokens (it is currently required as the entry and exit point to the ecosystem) you effectively remove the value proposition for MSC that is supposed to be introduced with the MetaDEx. I understand that a lot of our funding comes from sponsors who may hold large MSC positions, so I think you may find some objection to that.

@dexX7
Copy link
Member Author

dexX7 commented Apr 1, 2015

Or less?!? :P My reason for 4 bytes is simply one of mitigating risk of collision - 2 bytes is a 1-in-65,000 risk, 4 bytes is a 1-in-4,228,250,000 risk (much more palatable).

I could argue the chance of a collision is astronomically smaller, if the whole message is considered, and even more, if state (in terms of balances) is factored in, but I already did so earlier.

There is no Omni message we can do in 38 bytes that we cannot do in 36 (as far as my research has gone ...

There is also no Omni message that couldn't be split into several class A encoded packages, but ...

Now, that's a use case worth resourcing, but you're fighting an uphill battle there because by taking MSC out of the equation for trading tokens ...

Hehe sorry. In this context I usually refer to MSC. :)

There are two common arguments "why systems like Mastercoin are inferior [to alternatives]": 1. no atomic trades and 2. no SPV. Well, there are ways for point one, and I have a very rough idea to tackle point two, with a VerSum like approach (thanks @CraigSellars for explicitly asking, if I read the paper). Pushing either one of those directions would be a game changer and make a real difference.

This is not because atomic trades are funky, but because it allows to move trading off-chain, e.g. in the realm of much faster trade execution.

I can't say I agree a new input selection routine is required for ANYONECANPAY, the input can simply be excluded.

I'm unsure, if I can follow. What do you mean by "can be excluded"? Consider a transaction where:

  • Input 0: 500 MSC, 0.00000546 BTC from the seller
  • Input 1: 100 BTC from the buyer
  • Output 0: payload "transfer 500 MSC"
  • Output 1: dust-ish reference output to the seller buyer
  • Output 2: 100 BTC payment output to the buyer seller

You probably see why "contribution by sum" wouldn't work here. :)

@zathras-crypto
Copy link
Contributor

I could argue the chance of a collision is astronomically smaller, if the whole message is considered, and even more, if state (in terms of balances) is factored in, but I already did so earlier.

Yeah, I'm just not sure I subscribe to that. Firstly my main concern with this has always been forks - you only have to look at many of the altcoin clones out in the crypto community to realize most are what I call "copy paste" devs where very, very little attention is given and not much is changed. Back in the day when I was trying to make money from altcoins (a failed endeavour big time!!!) I even contacted several coin devs and gave a kick up the ass for not even bothering to change the alert key from the parent they'd forked from. Long story short, if we're forked so is our encoding scheme with it, and thus it's a fair assumption to say only the marker will be unique. As for state, I just have a problem in my gut with using state as a metric for whether the message is Omni or not (because we then add an transaction that's invalid due to insufficent funds to the stack that wasn't supposed to be there).

There is also no Omni message that couldn't be split into several class A encoded packages, but ...

Lol, not quite sure what you mean there mate, but the point I was trying to get across is I see positives (collision avoidance) but no negatives (we don't restrict any messages or functionality) to a 4 byte marker :)

I'm unsure, if I can follow. What do you mean by "can be excluded"?

Oh, just musing out loud - literally that we could (rather than invalidate a transaction) just drop the ANYONECANPAY input - but that's in the context of the "using a 3rd party to cover transaction costs" rather than atomic trading.

There are two common arguments "why systems like Mastercoin are inferior [to alternatives]": 1. no atomic trades and 2. no SPV.

As I say - "that's a use case worth resourcing" - you don't need to convince me of the benefit :) but I'm also a realist and understand that investors want return and pushing for things that will reduce that return are usually unsuccessful. That's not to say don't try, or that it's a bad idea - call it expectations management if you will (perhaps I was in mgmt too long hehe). Long story short, if you can make the case that atomic trades will increase the value of Omni as a technology AND increase the value of MSC for those investors I think you may get some traction, but my point was essentially just trying to convey the concept that MetaDEx was supposed to be the real value of the MSC token and I think you'll face a hard time getting large holders of MSC to keep funding a direction that will devalue their investment (MSC). Let's just be frank about it, with BTC to token trades/BTC crowdsales/lack of MSC fees and so on there isn't much to bring value to the MSC token and the (let's call it social contract) between JR and contributors during the crowdsale was that we would never take actions to devalue MSC - I don't know how relevant that is today though.

@zathras-crypto
Copy link
Contributor

BTW If you're wondering why I bother with trying to manage your expectations it's because I highly (very highly) value your contributions and would rather you saw debate with me than get disappointed by a negative response from leadership. Perhaps that is overstepping my remit though - I shouldn't try to speak for others.

@zathras-crypto
Copy link
Contributor

Hmm - actually reflecting on this a bit more I kind of invalidate my own point above there - if we're forked and the marker is changed, that doesn't constitute a risk of collision. I was too focused on the payload - the unique marker would obviously mitigate that.

@dexX7
Copy link
Member Author

dexX7 commented Apr 1, 2015

Long story short, if we're forked so is our encoding scheme with it, and thus it's a fair assumption to say only the marker will be unique.

I see where you're going, but a) this sort of sounds, as if you assume there could be more than 65 thousand forks, and b) the underlying issue applies to any fork as well: it's probably in no one's interest to re-use a marker or to build a system that collides with another one.

Edit:

I was too focused on the payload - the unique marker would obviously mitigate that.

I guess that clarifies the situation. :)

@zathras-crypto
Copy link
Contributor

Debate aside though, the good news is there seems to be universal support for dropping Exodus and moving the marker to the OP_RETURN output, the question now is just that of the size :)

Just thinking out loud here, but you proposed using the transaction version/type to potentially form part of the marker - perhaps that's a suitable scenario. I think type is too variable, but version is usable. We could setup marker detection to look for the two om bytes and a second two bytes within an allowed range (currently 0000 and 0001).

EDIT:
Teeny tiny code change, but I wonder about scalability - how likely are we to have huge numbers of versions though - probably not likely - and if so we could always move to ranges rather than direct comparison.

if (outType == TX_NULL_DATA) { // look for 'omni' bytes
    vector<string>temp_op_return_script_data;
    GetScriptPushes(wtx.vout[i].scriptPubKey, temp_op_return_script_data);
    if (temp_op_return_script_data.size() > 0) {
        if (temp_op_return_script_data[0].size() > 8) {
            if (("6f6d0000" == temp_op_return_script_data[0].substr(0,8)) ||
                ("6f6d0001" == temp_op_return_script_data[0].substr(0,8)) ||
                ("6f6d0002" == temp_op_return_script_data[0].substr(0,8))) {
                hasOmniBytes = true;
            }
        }
    }
}

@dexX7
Copy link
Member Author

dexX7 commented Apr 1, 2015

Just thinking out loud here, but you proposed using the transaction version/type to potentially form part of the marker - perhaps that's a suitable scenario.

Sorry, again not sure, if I follow correctly: this basically already established implicitly, because type and version checks are done at some point? Sure, it would be possible to match the payload (with marker for OP_RETURN and without marker for bare-multisig) against some templates (version, type, ...) as early filter, but I'm not sure, if an early filter vs. doing those checks twice provides a performance gain, or maybe I'm missing something?

@zathras-crypto
Copy link
Contributor

Sorry, again not sure, if I follow correctly: this basically already established implicitly, because type and version checks are done at some point?

They are indeed yep, but it's just keeping the logic clean. parseTransaction is where we identify if it's an Omni transaction and thus if we want to include say the version bytes in forming part of this identification process then parseTransaction is where we should do it. Otherwise we call mp_tx->Set for a non-Omni transaction and continue to pass it through the logic (where it eventually gets dropped for invalid version, rather than not being an Omni transaction - perhaps an academic distinction heh)

Admittedly we're talking about a 1-in-65,000 chance of colliding those first two bytes and passing such a transaction through to be interpreted and dropped based on version bytes. Perhaps not much of an issue?

Re early filter - that's the general principle yep (hence why we do marker ID at the beginning of parseTransaction to avoid wasting work on heavy tasks like looking up input transactions on non Omni transactions) but if we're talking about early filtering of transactions that collide on those first 2 bytes, not something we need to optimize for I don't think in that context agreed.

String comparisons are fast and I don't think we'd be looking at a performance penalty to check those 3rd and 4th bytes in the marker identification code (note we are checking the first two bytes anyway already) so my feeling is it's a bit "cleaner" that way - ie if we do collide those first two bytes unless the next two are also a supported version the transaction does not leave parseTransaction as an Omni transaction (later to be invalidated in interpretation) but instead is correctly dropped as a non-Omni transaction.

@dexX7
Copy link
Member Author

dexX7 commented Apr 1, 2015

so my feeling is it's a bit "cleaner" that way

Hmm.. hmm. I'm split. I see your point, but until now I considered parseTransaction for "general payload extraction, which mostly doesn't care about the content".

I'm not against it, because early filters actually provide a performance boost, which was very notable, when I benchmarked the parsing speed without the Exodus marker check (it works!.. ;), but I'm more concerned about bloating parseTransaction and introducing additional logic. For example, this would be something to be aware of, when using version numbers > 1, or if payloads were obfuscated at some point.

What about payloads, which were extracted from bare multisig?

It would probably make sense, unrelated to this in particular, to have a function such as hasMarker (or: hasExodusMarker, hasPayloadMarker, isClassB, isClassC, ...), to have a seperated section for those checks. This would simplify parseTransaction and appears to be easier to maintain, e.g. if/when the identification mechanism changes, and it's further probably also easier to read/understand.

@zathras-crypto
Copy link
Contributor

What about payloads, which were extracted from bare multisig?

This is at the beginning of parseTransaction, where we are not yet working with a payload. I'm just proposing a minor change to the current Class C marker identification code (multisig unaffected):

// ### EXODUS MARKER IDENTIFICATION ### - quickly go through the outputs & ensure there is a marker (exodus and/or omni bytes)
  for (unsigned int i = 0; i < wtx.vout.size(); i++) {
      txnouttype outType;
      if (!getOutputType(wtx.vout[i].scriptPubKey, outType)) continue; //unable to get an output type, ignore
      if (outType == TX_PUBKEYHASH) { // look for exodus marker
          CTxDestination dest;
          string strAddress;
          outAll += wtx.vout[i].nValue;
          if (ExtractDestination(wtx.vout[i].scriptPubKey, dest)) {
              strAddress = CBitcoinAddress(dest).ToString();
              if (exodus_address == strAddress) {
                  ExodusValues[marker_count++] = wtx.vout[i].nValue;
              } else if (isNonMainNet() && (getmoney_testnet == strAddress)) {
                  TestNetMoneyValues[getmoney_count++] = wtx.vout[i].nValue;
              }
          }
      }
      if (outType == TX_NULL_DATA) { // look for 'omni' bytes
          vector<string>temp_op_return_script_data;
          GetScriptPushes(wtx.vout[i].scriptPubKey, temp_op_return_script_data);
          if (temp_op_return_script_data.size() > 0) {
              if (temp_op_return_script_data[0].size() > 8) {
                  if ("6f6d6e69" == temp_op_return_script_data[0].substr(0,8)) {
                      hasOmniBytes = true;
                  }
              }
          }
      }
  }

All I'm doing is changing the 4 byte marker detection specifically for Class C:

if ("6f6d6e69" == temp_op_return_script_data[0].substr(0,8)) {

to

if (("6f6d0000" == temp_op_return_script_data[0].substr(0,8)) ||
    ("6f6d0001" == temp_op_return_script_data[0].substr(0,8)) ||
    ("6f6d0002" == temp_op_return_script_data[0].substr(0,8))) {

Hope that makes sense bud :) But long story short, Class B detection is completely unaffected by this.

It would probably make sense, unrelated to this in particular, to have a function such as hasMarker

Whilst I do agree in principle, to avoid duplication marker identification currently serves other purposes too because we populate Exodus amounts to assist with Class A reference identification (where falling back to all outputs same amount) and amount sent to crowdsale etc in that same code.

TL:DR; the only change I'm proposing is the line checking the first 4 bytes of an OP_RETURN output and instead of checking for omni we check for om00/om01/om02

@zathras-crypto
Copy link
Contributor

What I saw as the easy switch to a 2 byte marker FYI - zathras-crypto/omnicore@fd881a3

Am I overlooking something?

@dexX7
Copy link
Member Author

dexX7 commented Apr 1, 2015

I'm trying to figure out what the issue is (i.e. my setup, bitcoin-spock or your branch), but currently I see tons of errors, all pointing to failures in interpretPacket(). Did you forget to remove the marker from the payload by any chance? :)

Edit: OmniLayer/omnicore/tree/omnicore-0.0.10 runs without issues, so I assume this is indeed the case.

@zathras-crypto
Copy link
Contributor

Shoot - that'll be it - sorry I shouldn't bounce around too many things at once (currently rewriting balances tab). In this section https://github.com/zathras-crypto/mastercore/blob/0.0.10-Z-ClassC/src/mastercore.cpp#L1478-L1488 if you change:

std::string payload = op_return_script_data[0].substr(8,op_return_script_data[0].size()-8); // strip out marker

to

std::string payload = op_return_script_data[0].substr(4,op_return_script_data[0].size()-4); // strip out marker

That should sort it, I'll commit a fix soon but I have a whole bunch of uncommitted balances tab stuff outstanding at the mo so not sure how to checkout to ClassC without committing that stuff first...

@dexX7
Copy link
Member Author

dexX7 commented Apr 1, 2015

Shoot - that'll be it - sorry I shouldn't bounce around too many things at once ...

Hehe. Works fine, tests passed. :)

@zathras-crypto
Copy link
Contributor

Nice, thanks :)

@CraigSellars
Copy link
Member

Omniwallet has its own decoder for pending tx support (i.e. to decode the tx's its creating sending). Eventually we're going to want to roll tx decoding and (if possible) tx creation into omnicore entirely. i.e. omniwallet calls omnicore and says i want to send tx type X with info Y and core returns the unsigned tx for us to give the user to sign.

This is something I would very much want to see.

@CraigSellars
Copy link
Member

it's probably in no one's interest to re-use a marker or to build a system that collides with another one.

I’m not certain this is true.

@dexX7
Copy link
Member Author

dexX7 commented Apr 3, 2015

@CraigSellars: This is something I would very much want to see.

  1. OP_RETURN 80-byte limit restored, enable prunable outputs mastercoin-MSC/mastercore#283 (comment) Create or fetch raw Omni data
  2. WIP: modularization, minimal encoding mastercoin-MSC/mastercore#302 (comment) Wrap it into transaction outputs
  3. Construct a transaction

Now all this was to test some approaches, and is only partially related, but @zathras-crypto published a new branch for payload creation yesterday (or so) and his class C encoding branch looks pretty solid as well, basically closing the link between raw transaction data and class B/C transactions.

I'd say Omni Core 0.10 (0.0.10 :p) will be huge, and I'm confident creating raw transactions is going to a part of it.. ;)

@dexX7
Copy link
Member Author

dexX7 commented Jun 30, 2015

Class C doesn't use the Exodus marker anymore, but the bytes "omni" instead. /close

@dexX7 dexX7 closed this as completed Jun 30, 2015
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

No branches or pull requests

4 participants