Exodus purchases, DEx payments, crowdsales unconfirmed transactions about omnicore

[zathras-crypto]

Few comments from first read (I'm a bit fuzzy, will have another read later :))

• I consider Exodus purchases as a crowdsale. I think it would be silly not to given the usage of crowdsale-esque parameters (early bird bonus, deadline etc etc)
• The issue with the deadline is that JR "allowed" a few late-comers to be included, which does complicate matters a little. I believe this can be handled as a 'special case' to crowdsale logic, rather than completely separate Exodus purchase code as per current.
• We will be allowing BTC crowdsales at some point in future, and I believe Exodus purchases should be a derivation of this (some custom logic will be required, because two properties instead of one were issued and the deadline issue as above, but this should be minimal).
• I'd like to avoid FP math everywhere.
• Allowing DEx payments to also carry a payload and perform a secondary action seems overkill to me at this stage since we are not very well setup to handle multiple actions in the same tx (just look at simple sends + crowdsale participation, it's kind of hacky) - do you have an example use case?
• DEx payments to multiple parties in one transaction I dislike - but I had to build support for it - I can't actually remember why hehe.

I love where you're going with this, and I'm on board - but there are some caveats.

• We discussed 0.0.10 release this morning, it's likely going to be first half of July, with a 4 week period following that for integrators/users to update before 0.0.10 features go live and old versions fall out of consensus. Thus our primary focus should be to make 0.0.10 as solid as it can be before that point.
• Unconfirmed transactions will never be 'usable'. We can provide decoding and notification of unconfirmed transactions for sure, but we can never address the question of validity. With Omni being a state based system it is not possible to know validity before the transaction is actually mined - the best we can do is say "so far we haven't yet seen anything in existing transactions or the mempool that would invalidate this transaction, but until it's mined we can't know for sure".
• I'm a big fan of the idea of ditching txindex, storing our data outside the blockchain and allowing pruning - it will be a big boon to usability (requiring 40 odd gigs can only hurt user uptake) - we've just got to make sure to do it in a safe and secure way.

TL:DR; some very worthwhile points in your post dude, and a lot of them I'm all for. I suggest once 0.0.10 is released and settled in we 'take stock' as it were and plan for 0.0.11 and what our priorities are going to be - a major re-architecture can be on the cards for sure if we can get the support to do it :)

[dexX7]

Allowing DEx payments to also carry a payload and perform a secondary action seems overkill to me at this stage ...

DEx payments to multiple parties in one transaction I dislike ...

Yeah, I'm just trying to find a common basis, so to speak. Supporting multiple DEx purchases, to multiple receivers, in one transaction is what's currently done. And Exodus purchases are not constrained by the fact, that a transaction may also have another action (there are a few mainnet transactions of simple sends, which are Exodus purchases at the same time).

Logic wise it would probably be easier to handle the BTC stuff completely seperated, ideally on an output-for-output basis (after passing some checks such as the marker check). Easier in terms of "not having to go through the whole parsing steps, only to see that a transaction is not a class A transaction, while making sure, it's also not just a RPC call".

Let's ignore for a moment everything that's currently done, and quickly think about a logic flow, which allows to address payload-transactions, Exodus purchases, DEx payments, etc. How would that look like? I'm not really sure.

We discussed 0.0.10 release this morning, it's likely going to be first half of July, with a 4 week period following that for integrators/users to update

Awesome, that's great to hear!

Just some note to clarify: a feature freeze seems reasonable (unless it's low hanging), and it's not really my intention to include BTC logic and unconfirmed transactions in 0.0.10, but to get a better feeling for where it's going, which may have impliciations to the ongoing work.

Speaking of, something very specific we may discuss quickly:

• As far as I can see, a transaction without marker, but only one or more money outputs passes the marker check (omnicore.cpp#L765). I'm wondering: are testnet transactions without marker, but money output as marker replacement, considered as valid right now?
• During the marker checks there are no explicit checks, whether some output type is allowed. What might happen, if there is a class B/multisig transaction on mainnet with "om" OP_RETURN marker, and without Exodus output (before OP_RETURN is enabled)? On the first glimpse it looks like it would pass the marker check (because it has "om" bytes), and would then be classified as class B transaction, because OP_RETURN isn't allowed yet.

[zathras-crypto]

What might happen, if there is a class B/multisig transaction on mainnet with "om" OP_RETURN marker

That's a very good point actually - raises the question "should the marker be class specific?"

but only one or more money outputs passes the marker

Yeah the moneyman thing is an interesting case because it's a deviation from what we do on mainnet to facilitate testing. I'd probably prefer to scrap it altogether and have a Test MSC faucet, but that's probably just me hehe

[dexX7]

"should the marker be class specific?"

In the context of class C it must, at least until the switch-over, to prevent that consensus is broken for old clients, if someone sends a transaction with class C marker, and class B payload.

Yeah the moneyman thing is an interesting case ...

Do you know why this approach was chosen?

[zathras-crypto]

In the context of class C it must, at least until the switch-over, to prevent that consensus is broken for old clients, if someone sends a transaction with class C marker, and class B payload.

Great point, in which case I suggest we make the marker class specific full stop.

Do you know why this approach was chosen?

Probably to get rid of the old mastercoin_balances.txt file you used to have to populate to get a test MSC balance. Since it is a deviation from what we do on mainnet however I think it's not ideal (I get why Michael did it this way though, I just think it's time to move to a less invasive approach)

[dexX7]

Great point, in which case I suggest we make the marker class specific full stop.

Yeah, I agree. This is something I haven't tested though, but I assume, based on the code, the scenario mentioned above is possible, so we definetly should make the marker check isAllowedOutputType-aware.

I just think it's time to move to a less invasive approach

Are you suggesting to drop the money approach, and use Exodus instead? I'd be in favor of it, but it would basically reset our testnet.

[zathras-crypto]

so we definetly should make the marker check isAllowedOutputType-aware.

This would still permit a Class B transaction with a Class C marker once Class C has gone live though right? I fear this introduces complexity we don't need...

but I assume, based on the code

We'd need to test, but I would imagine it would go something like this:

• Existence of Class C marker passes the marker check
• Existence of OP_RETURN data output forces Class selection for parsing to Class C. Class B multisig outputs ignored.
• Class C not enabled yet so parser throws out the transaction,

Are you suggesting to drop the money approach, and use Exodus instead?

I haven't given much thought, just in general terms the moneyman approach means we approach testnet differently to mainnet. As for Exodus, again not sure how we'd work that (perpetual crowdsale is again a deviation from mainnet). A faucet that hands out TestNet BTC & TestNet MSC in one go would be a nice/elegant solution to this, but would require setting something up.

I'd be in favor of it, but it would basically reset our testnet.

Well we're about to reset all the trades on testnet anyway so I'm not really fussed about that - but I don't have a thought out strategy for replacing sends to moneyman, I'm just commenting that having separate things for testnet can confuse things and ideally we'd not do that :)

[dexX7]

This would still permit a Class B transaction with a Class C marker once Class C has gone live though right? I fear this introduces complexity we don't need...

Existence of Class C marker passes the marker check [...]

Well, let's combine the marker check and class selection then?

As for Exodus, again not sure how we'd work that (perpetual crowdsale is again a deviation from mainnet).

I don't think a never-ending Exodus crowdsale is a bad thing, and without one we'd basically have a testnet without Exodus sale, which also differs significantly from mainnet.

A faucet doesn't work for regtest mode.

[zathras-crypto]

Well, let's combine the marker check and class selection then?

That sounds sensible.

I don't think a never-ending Exodus crowdsale is a bad thing, and without one we'd basically have a testnet without Exodus sale, which also differs significantly from mainnet.
A faucet doesn't work for regtest mode.

Fair points :) How would you suggest to handle things like bonus and deadline (or just ignore them as per moneyman and issue 100 TMSC per TBTC?)

[dexX7]

I wouldn't ignore them, but set a very high value for the deadline, an early bird bonus of 0, and a bonus of 0 for the issuer. Using a payout factor of 100 seems fine. In the end, this is probably the same, but handling the Exodus payouts as close to a crowdsale as possible would be great.

Since MSC and TMSC are emitted, maybe we should think of it as two crowdsales at the same time, which are both associated with the same address, and both take BTC etc.

[dexX7]

While looking at parseTransaction, the Exodus logic is as follows:

Take the value of the first Exodus output

This seems to have no mainnet impact, as there were probably only transactions with one Exodus output, but we may take a closer look at this, given that there might be as well more than one Exodus output on testnet/regtest mode.

I'm not necessarily in favor of counting more than one Exodus purchase, if there are more than one Exodus outputs, but I don't really see why we should stick to the "take first" approach. What do you think?

In case of doubt, I tend to count each output on it's own (thus multiple purchases might be possible).

[dexX7]

I started with wip-oc-0.10-parse-transaction-classes to tackle the marker/class issue, and I'm probably going to move some parts here and there.

But a two questions:

• How are we going to handle the Exodus/Money address on testnet? I tend to use only one, but this comes at the cost of resetting testnet.
• How are multiple Exodus purchases in one transaction handled? I tend to allow more than one, and consider each output as "atomic action".

[zathras-crypto]

How are we going to handle the Exodus/Money address on testnet? I tend to use only one, but this comes at the cost of resetting testnet.

How are multiple Exodus purchases in one transaction handled? I tend to allow more than one, and consider each output as "atomic action".

I assume the benefit here is to avoid the extra moneyman code, so wouldn't it be prudent to use the same Exodus code as mainnet? This can be changed to allow multiple Exodus outputs without any consensus issues. As to whether to count each as an atomic action - again I don't feel we're particularly well equipped to handle multiple actions per single transaction in our current codebase. Since these multiple actions are not different actions but the same action, why not just sum multiple outputs to Exodus and process as a single action?

As for the reset testnet thing - I'm not hugely concerned by that but we should probably ask if anyone else has any objections :)

[dexX7]

Regarding the MoneyMan code, I think we could also have the concept of a MoneyAddress, seperately to an ExodusAddress. On mainnet, the ExodusAddress and the MoneyAddress are the same, but on testnet they are not.

Assuming we maintain compability as far as possible (i.e. no testnet reset), then it raises again the question: is it allowed to have a Money output, but no Exodus marker? I think it shouldn't be.

As to whether to count each as an atomic action - again I don't feel we're particularly well equipped to handle multiple actions per single transaction in our current codebase. Since these multiple actions are not different actions but the same action, why not just sum multiple outputs to Exodus and process as a single action?

Code wise, I'd like to get somehow close to something like:

for (unsigned int n = 0; n < tx.vout.size(); ++n) {
    CTxDestination dest;
    if (ExtractDestination(tx.vout[n].scriptPubKey, dest)) {
        if (CBitcoinAddress(dest) == moneyAddress) {
            TXExodusFundraiser(tx, strSender, tx.vout[n].nValue, nBlock, nTime);
            // if we only "take the first", then we may uncomment the break
            // break;
        }
    }
}

Currently Exodus purchases are pretty much unnoticed, and they are at best logged, but don't show up in the UI or on the RPC layer, so I'm not really concerned about "multi actions" at the moment. As said though, I'm also not necessarily in favor of allowing multiple purchases, but it's a bit similar to DEx purchases, and if we allow more than one per transaction, then I think we should also allow more than one Exodus purchase.

[zathras-crypto]

Assuming we maintain compability as far as possible (i.e. no testnet reset), then it raises again the question: is it allowed to have a Money output, but no Exodus marker? I think it shouldn't be.

I don't really get that mate - some (I'd guess a majority) of existing sends to moneyman will not have an Exodus marker, thus I'm not sure how still using moneyman but enforcing an Exodus marker would maintain compatibility - if anything it would give a kind of "half-baked" testnet where some of the previous sends to moneyman were valid and others were not, invalidating some chains of transactions and validating others - I see that as a pretty ugly scenario.

I get what you're saying about code simplicity - but this is not the only place to consider. I do realize we don't currently do anything with Exodus purchases in terms of exposing it but that's only because it's been deprioritized - we should still fix it up at some point (eg gettransaction_MP on an Exodus purchase should work at some point when we have time to do it). If you look at DEx payments there are all sorts of hacky bits to enable multiple actions for the same TXID (storing the same txid in levelDB multiple times with vouts appended, extra code to iterate and work with DEx payments and so on).

In a nutshell:

Scenario A:

* vout 1 = 1 BTC
* vout 2 = 1 BTC
* vout 3 = 2 BTC

* transaction action 1 = return 100 MSC
* transaction action 2 = return 100 MSC
* transaction action 3 = return 200 MSC

Scenario B:

* vout 1 = 1 BTC
* vout 2 = 1 BTC
* vout 3 = 2 BTC

* transaction action 1 = return 400 MSC

Scenario B is simpler in my book and I don't really see a huge value to using

TXExodusFundraiser(tx, strSender, tx.vout[n].nValue, nBlock, nTime);

instead of some derivation of

TXExodusFundraiser(tx, strSender, voutSum, nBlock, nTime);

[dexX7]

I don't really get that mate - some (I'd guess a majority) of existing sends to moneyman will not have an Exodus marker

Ahh, sorry, this changes everything. All my previous posts were based on the assumption those purchases actually have an Exodus marker and Money output.

Scenario B is simpler in my book and I don't really see a huge value to using

I guess my only argument against it is how DEx payments are handled, otherwise I agree.

[zathras-crypto]

Ahh, sorry, this changes everything. All my previous posts were based on the assumption those purchases actually have an Exodus marker and Money output.

I'd need to check to be sure as I'm only going on memory here :)

I guess my only argument against it is how DEx payments are handled, otherwise I agree.

Yeah I do see your point - I just remember when I was doing the DEx payment stuff that it felt ugly and hacky - I'm sure I didn't want to support payments to multiple sellers in the same transaction but I'm sure I had to for a reason (perhaps consensus with msc-tools or something?).

Longer term I absolutely agree that we should support multiple actions per transaction (example send-to-many) but I think this should come as a kind of 'rearchitecture' that we should queue up next - ie get 0.0.10 out and then press 'pause' and take stock of our current position and limitations, and see about a rearchitecture to address. This is where things like no-txindex/pruning, storing our transactions outside the blockchain and so on come in, and we can redesign with a year of experience in mind rather than the current additive model.

[dexX7]

Good points, you're right. I shouldn't necessarily start something new now, so I'm just going to stick to the current behavior, and address the class/marker check issue. And maybe do some non-functional changes on the way. :)