Rework tx creation + implement Class C & missing transactions

First off apologies for the size of this PR guys, these things kind of all link together.  Thanks :)
Not final - will need a LOT of testing!  This PR serves three main purposes:
1) Rework transaction creation
Transaction creation has been refactored with this PR.  A number of old functions have been removed, including classB_send and send_INTERNAL_1packet to instead follow a method where the calling interface (eg RPC or UI) will evaluate parameters and perform the necessary checks before calling respective createPayload_type and ClassAgnosticWalletTXBuilder functions to create the transaction.  This facilitates easier adding of new transactions types.
In addition there is also a new raw transaction flag autoCommit (can be set via RPC command setautocommit_OMNI true|false) - when set to false (default true) the RPC interface will ask the wallet to create transactions but will not commit them (or broadcast them), instead returning the raw transaction hex in the response instead of the txid.  For testing use mainly & it seems there may some debate as to whether this should be included so unsure if will stay.
2) Add support for Class C transactions
Class C transactions are supported with this PR.  A Class C transaction moves both the payload and the Omni (Exodus) marker to an OP_RETURN output.  Using a provably unspendable output reduces our impact on the blockchain and further reduces transaction cost.  The client nMaxDatacarrierBytes setting will be used to determine when to send a Class C transaction, falling back to Class B when necessary (if the payload is too large).
3) Add send support for all currently live transaction types
Also known as technical debt, there were numerous transaction types that are currently live but that OmniCore was unable to create/send.  This PR adds support for all transactions that are currently live.  A number of new RPC calls have been added (no UI support just yet) including:
send_OMNI
senddexsell_OMNI
senddexaccept_OMNI
sendissuancecrowdsale_OMNI
sendissuancefixed_OMNI
sendissuancemanaged_OMNI
sendsto_OMNI
sendgrant_OMNI
sendrevoke_OMNI
sendclosecrowdsale_OMNI
sendtrade_OMNI
sendchangeissuer_OMNI

Existing RPC commands sendtoowners_MP and send_MP have been aliased to the new _OMNI calls to provide backwards compatibility.  The original code for them has been removed.
Test txids below (note you will need this branch to see them, as most of them were sent Class C - it was really nice to just be able to pump out transactions of any size and watch ClassAgnostic adapt!):
# src/bitcoin-cli sendissuancefixed_OMNI "mpZATHupfCLqet5N1YL48ByCM1ZBfddbGJ" 2 2 0 "RPC Test Category" "RPC Test Subcategory" "RPC Test Token" "RPC Test URL" "RPC Test Data" 2000
d41b033fe05cff7ca3a491f80c52ae3d5da901cf3108c0d5ee61743415c4efe9

# src/bitcoin-cli sendissuancemanaged_OMNI "mpZATHupfCLqet5N1YL48ByCM1ZBfddbGJ" 2 2 0 "Managed Test Category" "Managed Test Subcategory" "Managed Test Token" "Managed Test URL" "Managed Test Data"
c047ece24168a5c88fddf83f7bb7e79e24a0497d73b0f36d3939262f64bf1e1a

# src/bitcoin-cli sendissuancecrowdsale_OMNI "mpZATHupfCLqet5N1YL48ByCM1ZBfddbGJ" 2 2 0 "Crowdsale Test Category" "Crowdsale Test Subcategory" "Crowdsale Test Token" "Crowdsale Test URL" "Crowdsale Test Data" 1<PHONE_NUMBER>505 10 15
09585f480072c95d262f5481a484f354f66c28c9c010bff59b4fae6f6a2b9779

# src/bitcoin-cli senddexsell_OMNI "mpZATHupfCLqet5N1YL48ByCM1ZBfddbGJ" 1 0.01 1 10 0.0001 1
0de2c53a5754b0477b9b33f908dbb7ddd561ffb91c2806d56d061cf8907a19b7

# src/bitcoin-cli senddexaccept_OMNI "mpZATHupfCLqet5N1YL48ByCM1ZBfddbGJ" "mvayzbj425X55kRLLPQiuCXWUED6LMP65C" 2 1.5
c821101fb455428ad48597f28b672c505260cfe854c94f22a0e09a84f0b7b75a

# src/bitcoin-cli sendclosecrowdsale_OMNI "mpZATHupfCLqet5N1YL48ByCM1ZBfddbGJ"<PHONE_NUMBER>
95b62d82432a06f3904e4312ed4384a9b57b63f301b0764c893e699ef694107a

# src/bitcoin-cli sendgrant_OMNI "mpZATHupfCLqet5N1YL48ByCM1ZBfddbGJ" mpZATHm5ih33ePoSPxFQ4cvWZWaPkN4G5s<PHONE_NUMBER> 100 "Test Grant Memo"
c8644fe15cb0f9d61c44c465365dc25e0f07f2b5ad768602b9a46e8b3bd75806

# src/bitcoin-cli send_MP "mpZATHupfCLqet5N1YL48ByCM1ZBfddbGJ" "mpZATHm5ih33ePoSPxFQ4cvWZWaPkN4G5s" 1 0.01
2af942fe0a3c4795c29f087f40701c50152fae83aea9c3f52839d1c51cec7aaf

# src/bitcoin-cli send_OMNI "mpZATHupfCLqet5N1YL48ByCM1ZBfddbGJ" "mpZATHm5ih33ePoSPxFQ4cvWZWaPkN4G5s" 1 0.02
a708f6352de14db63597a97851ed37900b42a9d6f6f5851cd6d688fd23bd2ef3

# src/bitcoin-cli sendtoowners_MP "mpZATHupfCLqet5N1YL48ByCM1ZBfddbGJ" 57 100
3476ad202dd2a8f3b666f514ad5e15b2dd89eff1b35e44028ad9b5eb780a0914

# src/bitcoin-cli sendsto_OMNI "mpZATHupfCLqet5N1YL48ByCM1ZBfddbGJ" 57 200
94fea1a1deaa41eaf5c5b2907bff76df8c0cd4834037d03a45924e37f21f81d0

# src/bitcoin-cli sendtrade_OMNI "mpZATHupfCLqet5N1YL48ByCM1ZBfddbGJ" 57 10 1 20 1
78894c5a6f1a874142fdd0b7e07edc741105972e43706a9828604ed9e64bdfaa

# src/bitcoin-cli sendchangeissuer_OMNI "mpZATHupfCLqet5N1YL48ByCM1ZBfddbGJ" mpZATHm5ih33ePoSPxFQ4cvWZWaPkN4G5s<PHONE_NUMBER>
9b831ef60919c42be1ede10fbe4c773a622144669f7dbaa7bb4452574a9263a2

Awesome. I'll take a look at this later.
Given all the type checks, it might be worth the redo something https://github.com/mastercoin-MSC/mastercore/pull/167#issuecomment-68625798 and get some "parse-to-that-type-or-throw-json-exception" functions. But this would just serve readability, on the very first glimpse it looks like you covered the checks.

Also, re: merge conflict. Didn't test it locally, nor really looked through, but IIRC you started with the <IP_ADDRESS>-no-history branch, which might have caused some incompabilities? In the worst case the commits could be easily cherry-picked.

Awesome. I'll take a look at this later.

Thanks bud :)

Given all the type checks

Yeah that looks good looks good - at some point I really need to get to looking at all those outstanding PR's in the old repo!

started with the <IP_ADDRESS>-no-history branch

Could very well be the case - but I had thought it was from omnicore-0.0.10 (exposing my lack of GitHub skills here lol):
On branch 0.0.10-Z-ClassC
Your branch is ahead of 'origin/omnicore-0.0.10' by 25 commits.

merge conflict

Yep I noticed that too, now need to see what the issues are - hopefully be something small :)

Your branch is ahead of 'origin/omnicore-0.0.10' by 25 commits.

Oh.. I pull it locally in a few minutes, but I fear this will ovewrite the history in an odd way, sort of like with similar problems I faced, when I tried to hack the history back into 0.0.10. But let's see.
I have already a few ideas as follow up, including an idea how to tackle the send_ and create_ topic, and I would like to merge this ASAP, because it's such a huge change and it's sort of stalls anything else until it's in.

at some point I really need to get to looking at all those outstanding PR's

No need to. I'll resubmit them, here, when it's time. The upcoming ones are probably going to focus on moving some parts (say for example the send and sto logic, which is wrongly in mastercore.cpp right now), starting with the logger.

The merge conflicts are fixed mate, it was just that I hadn't pulled from origin for a while.  Should be sorted now.

I have already a few ideas as follow up

Sounds good, looking forward to hearing about them :)

I would like to merge this ASAP

Whilst I'm pretty confident in the Class C stuff, the transaction creation stuff has only had very limited testing so there may still be bugs left there we need to find?

No need to. I'll resubmit them, here, when it's time.

Are you sure?  That would help me tremendously (by saving me a bunch of work reviewing all the old PRs) but I don't want to see past contributions go to waste as it were - up to you bud :)

Edit: if you have time to spare (which I doubt hehe), you could add a comment to each function though, and roughly give it a category. In particular this would be useful to locale "used by UI only" code blocks.

Happy to. FYI there is very minimal stuff for the UI that's not in the qt/ subdirectory.  There are a few concepts that are currently only exposed in the UI, but in future could also be used elsewhere (for example set_wallet_totals and associated balance arrays - we only display a total wallet balance for property X in the UI currently, but that info could also be quite useful on the RPC just the same ("how much X do I have in the wallet" is a question that currently can't be easily asked of the RPC layer - surprisingly when I think about it).

Should be sorted now.

Actually it's the case I feared. If you take a look at: https://github.com/zathras-crypto/mastercore/compare/9f3aa31623...98ba242f95
When merging this, the the old commits will be overwritten, or rather: commited a second time. I assume, when you use git log --show-signature there is no GPG signature for my commits, right? If you don't mind (should take a minute hopefully), could you start with the 0.0.10 branch, then use cherry-pick xxxx for the commits of this PR, and at the very end git checkout -b 0.0.10-Z-ClassC (might have to use git branch -D 0.0.10-Z-ClassC to remove it locally first). Then, push and overwrite with git push origin 0.0.10-Z-ClassC -f. Opening a new PR is not required, but the -f just overwrites the commits from this one.
If this takes more than a minute, it's probably not worth the effort, but would be nice to have. :)

Are you sure? That would help me tremendously

Yes of course. :) So regarding the pending PRs: I would like to get https://github.com/mastercoin-MSC/mastercore/pull/265 in, in a slightly different form, because the debug.log handling of Bitcoin Core 0.10 was slightly improved. The rationale is that the log is used basically everywhere, but creates nasty cross dependencies, as well as those annoying extern int xxxx that are required in the files that use the logger. From there, it looks like the next step would probably be to work on getting some  type checks in, as well as moving and grouping code blocks.

Happy to. FYI there is very minimal stuff for the UI that's not in the qt/ subdirectory.

Exactly those parts I'm referring to - those which are not actually part of the QT code, but helper (and still only used by the UI). I would like to move them into omnicore_qt.cpp/.h or omnicore_ui.cpp/.h.

Whilst I'm pretty confident in the Class C stuff, the transaction creation stuff has only had very limited testing so there may still be bugs left there we need to find?

I will begin with the local review very soon, but for the sake of testing it makes actually sense to me to pull it in, because that would provide a basis everyone could work with, instead of having some scattered mix of local/remote changes.

Visual code check: passed
bitcoind unit tests: passed
bitcoin-qt unit tests: passed
Test results of bitcoin-qt, datacarrier=1, datacarriersize=40: passed (two trivial, one unrelated error)
Report: http://bitwatch.co/uploads/omnicore-pr-13-qt40/
Test results of bitcoin-qt, datacarrier=1, datacarriersize=800: passed (two trivial, one unrelated error)
Report: http://bitwatch.co/uploads/omnicore-pr-13-qt800/
Test results of bitcoin-qt, datacarrier=1, datacarriersize=0: failure (several errors)
Report: http://bitwatch.co/uploads/omnicore-pr-13-qt0/
Debug log: http://bitwatch.co/uploads/omnicore-pr-13-qt0/mastercore.log
Test results of bitcoind, datacarrier=1, datacarriersize=0: failure (several errors)
Report: http://bitwatch.co/uploads/omnicore-pr-13-d0/

Observation 1:
The transactions, constructed with datacarriersize=0 still use OP_RETURN.
Observation 2:
When requesting transaction data with getrawtransaction, the OP_RETURN output is considered as non-standard:
{
"value" : 0.00000000,
"n" : 0,
"scriptPubKey" : {
"asm" : "OP_RETURN 6f6d000000320200010000000000005350000000000000011f018be6",
"hex" : "6a1c6f6d000000320200010000000000005350000000000000011f018be6",
"type" : "nonstandard"
}

A fix is likely not difficult, but I noticed something, very important:
Transaction parsing and processing must not be dependent on the client configuration.
While it's certainly a bug that the created transactions use OP_RETURN, while they should not, it's an entirely different issue, if certain transactions are considered as non-standard, because this triggers a failure due to the transaction output type rules used in parseTransaction() (via ExtractDestination() and isTransactionTypeAllowed()).

The transactions, constructed with datacarriersize=0 still use OP_RETURN.

OK that was unexpected hehe - the class selection code couldn't be any simpler:
  // Determine the class to send the transaction via - default is Class C
  int omniTxClass = OMNI_CLASS_C;
  if(data.size()>nMaxDatacarrierBytes-2) omniTxClass = OMNI_CLASS_B;

so perhaps nMaxDatacarrierBytes is not being set when OP_RETURN is disabled entirely with datacarriersize=0, let me investigate further mate.

When requesting transaction data with getrawtransaction, the OP_RETURN output is considered as non-standard:

Yeah that is a pretty significant issue.  Off the top of my head the only thing I can think of is to trap any changes to permitbaremultisig or datacarriersize and inform the user that changing these parameters away from default is not allowed.  Seems pretty rough though :(

Issue 1 (example fix):
static bool UseEncodingClassC(size_t nDataSize)
{
    size_t nTotalSize = nDataSize + 2; // Marker "om"
    bool fDataEnabled = GetBoolArg("-datacarrier", true);

    return nTotalSize < nMaxDatacarrierBytes && fDataEnabled;
}
int mastercore::ClassAgnosticWalletTXBuilder(const ...)
{
    int omniTxClass = OMNI_CLASS_C;
    if (!UseEncodingClassC(data.size())) omniTxClass = OMNI_CLASS_B;
    // ...
}

Issue 2:
This could turn into something more complex, because any use of ExtractDestination(), ExtractDestinations() and getOutputType() should be evaluated very carefully.
The upside: it looks like there are not many, at least in parseTransaction. but interpretPacket should probably be checked as well.
The underlying function used in all three is Solver(), which is influenced by the user settings -datacarrier here and -datacarriersize here.

I think enforcing datacarriersize with a fixed value is a no-go, for several reasons and new issues that would be introduced by this, and the restrictions that cause the trouble should be removed instead.

The easy route:
Copy ExtractDestination(), ExtractDestinations() and Solver(), but remove the restrictions, which are introduced and influenced by those settings.

The maybe more complex route:
Provide customized alternatives to those functions.

Issue 3: (new)
The restructuring of parseTransaction may have introduced another edge case issue, which I'm currently looking into.

Issue 1 (example fix)

Thanks - that should do the trick :)

Issue 2
I think enforcing datacarriersize with a fixed value is a no-go, for several reasons and new issues that would be introduced by this, and the restrictions that cause the trouble should be removed instead.

Let's flip this round, why do we need to support custom datacarriersize?  Class handling could be easily handled by for example a --disableclassc startup param or setclasscenabled true|false RPC call.  I'm really interested in what new issues you think will be introduced here, because (at first glance) it seems far safer than using customized bitcoin functions ExtractDestination()/ ExtractDestinations()/Solver() - using copied/customized bitcoin functions surely leads us down the rabbit hole where the Omni side of things thinks the transaction is fine but the Bitcoin side of things thinks it's a non-standard transaction and the unexplored consequences of that?
Interesting topic for sure - we just need to find the best way to handle :)

Issue 3
Great bud, thanks - let me know what you find :)

P.S. the question "why do we need to support custom datacarriersize?" essentially comes from the fact that transactions larger than the default will never be mined and settings >0 and <default I can't think of a use for.

Ah, so the issue with a fixed datacarriersize is not about creating transactions, but parsing transactions. Was this part clear?

P.S. the question "why do we need to support custom datacarriersize?" essentially comes from the fact that transactions larger than the default will never be mined and nMaxDatacarrierBytes settings >0 and <default I can't think of a use for.

That's not true. There are quite a few, currently considered as non-standard, OP_RETURN transactions in the wild, and it's really not that difficult to find a pool which accepts exotic scripts and oversized OP_RETURN transactions. Further, the "standard" behavior on testnet differs from mainnet.
But my actual concern is about something else: it's consensus affecting, due to it's impact on parseTransaction.
Let's play it through, maybe I miss something:
Say a size of 40 byte is enforced. What happens, once more miners accept 80 byte? This isn't a black-or-white event, but going to be a step-by-step progress. Would you schedule a consensus breaking update, and switch to 80 byte, if, at some point, a certain level of adoption is given? And let's say such a 80 byte update is done, and for some very odd reason some miners decide to switch to a lower value. As first consequence Omni Core users would end up with slowly confirming transactions, as second consequence, another consensus breaking update might be scheduled, to match that new "network default size", and then the cycle repeates.. See where I'm going? :)

That's not true. There are quite a few, currently considered as non-standard, OP_RETURN transactions in the wild

OK that's a fair point

But my actual concern is about something else: it's consensus affecting

We're on the same page - I just think we have different ways of viewing it.  I had thought the safest model was to work in absolutes.  Say you do create a 105 byte payload for example and get it mined in a non-standard OP_RETURN output.  We cannot allow one client to think the transaction is valid while another client thinks it's not valid due to differing datacarrier settings.  Thus if we enforce 40 (or in later versions 80) bytes all clients apply the same rules.  I'm not sure I believe mining policy changes faster than our release cycle (especially at the mo).
I think I get where you are going - you'd like to be able to accept any size payload inside the OP_RETURN output if it makes it into a block, regardless of client settings - would that be correct?

Would you schedule a consensus breaking update

Not directly - I'm a proponent of 'bundling' multiple consensus affecting changes into each version bump.  So since 80 bytes is in master and should make it into Bitcoin 0.10.1, once that was out and mining policy started to adapt we would 'queue' the change to 80 bytes to be included with our next consensus affecting release.  So yes I'd include it in a consensus breaking update, but no I wouldn't schedule one just for this change solely.
Note, 40 byte Class C covers more than 99.5% of our transactions last time I did the numbers (issuances are the only transaction that does not fit), thus moving to 80 bytes isn't critical for me.

And because tone is hard to convey, I'm absolutely open to changes - hopefully this discussion will flesh out the best way to go :)

We cannot allow one client to think the transaction is valid while another client thinks it's not valid due to differing datacarrier settings.

Yes, agreed. Preventing this, one way or the other, is the goal.

Say you do create a 105 byte payload for example and get it mined in a non-standard OP_RETURN output.

This is an issue, because we depend on the interpretation of "standardness", and this is the first time we notice it, due to the user settings of datacarriersize and datacarrier.
Let's not think about how to get to that point for a moment: would you agree that removing this dependency would resolve the issue?

I had thought the safest model was to work in absolutes.

I agree that a fixed value can resolve the issue, but aside from the above, it restricts the actions of an user, so - assuming both routes were equally secure - I would consider this route as less convenient.
But convenience also aside: I'm not yet convinced this is side effect free.
Maybe it helps for me to get a better feeling for the situation, if you could specify/describe the behavior of a 40 byte restriction informally, in the context of transaction validity? Very roughly something, that could be put into the spec.

This is an issue, because we depend on the interpretation of "standardness", and this is the first time we notice it, due to the user settings of datacarriersize and datacarrier.

This in a nutshell is my concern.  Put simply, even if we 'override' behaviour on the Omni side with custom solver() and friends, on the bitcoin side "standardness" is still affected by the users datacarriersize setting.  There could be unintended consequences of this.

would you agree that removing this dependency would resolve the issue?

Could you rephrase the question mate, not sure I'm 100% clear

Maybe it helps for me to get a better feeling for the situation, if you could specify/describe the behavior of a 40 byte restriction informally, in the context of transaction validity?

We have free reign here given that we are working in a different way (ie we previously had to build an implementation from a given set of rules in the spec, this time we're building the implementation first and will add the appropriate rules to the spec second) so we can decide whatever we like.
Long story short I'd viewed the path of least resistance to define a Class C (first iteration) transaction with a maximum packet size just as we do with the other classes and then hardcode datacarriersize and datacarrier parameters accordingly.  I completely agree that it's more restrictive - but it is also non-invasive (code wise) and I'm kind of following @m21 's lead here and putting safety/stability first.

assuming both routes were equally secure

I would take the less restrictive route absolutely, assuming both routes were equally secure/safe/stable etc :)

Standardness has no impact on Bitcoin transaction validity, but I think we may have talked past each other:
I did not suggest to replace Solver() globally, but only for the Omni side. Instead of hardcoding a value of 40 byte, I'd remove the size restriction for OP_RETURN, which then mirrors the behavior of Bitcoin Core on testnet in that context.

By the way, this got sort of lost: issue 3 was acutally not an issue and I'm more or less convinced the behavior of parseTransaction old vs. new is similar. (well, except the OP_RETURN part of course.. :)

I did not suggest to replace Solver() globally, but only for the Omni side

Yeah I know, did you see my edit?

EDIT: Perhaps I can clarify here. Let's say we do take your approach of using our own copies of solver() and friends:
Client A: omnicore v0.0.10 with datacarriersize=100
Client B: omnicore v0.0.10 with datacarriersize=40
Transaction is sent by Client A with a payload of say 60 bytes and this is mined OK
Client A: omni code sees transaction is valid, bitcoin code sees transaction is standard
Client B: omni code sees transaction is valid, bitcoin code sees transaction as not standard
That difference between what the bitcoin side sees is what concerns me on two clients using same version of OmniCore - potential unintended things as a result of that. Hope that helps clarify :)

I get what you're saying, I'm just pointing out that we can have two Omniclients that think differently about the standardness of a transaction on the bitcoin side, precisely because we are only modifying the Omni side.  And any unintended side effects of that.

By the way, this got sort of lost: issue 3 was acutally not an issue and I'm more or less convinced the behavior of parseTransaction old vs. new is similar. (well, except the OP_RETURN part of course.. :)

That's awesome thanks :)

Okay, just to be clear, and sorry to repeat this, but it's crucial: standardness has no impact on Bitcoin validity. It's entirely fine to screw it up, remove standard checks (as done for testnet), or to create some funny policies to allow 3x 150 byte OP_RETURNs in one transaction, or 7-of-9 bare multisig scripts, or to make pay-to-pubkey-hash non-standard. From a Bitcoin POV, this does not impact consensus, nor the user's point of view, and if there is a miner, who has a similar standard policy, then even 3x OP_RETURNs within one transaction can become part of the blockchain. Bitcoin Core 0.11 is heavly leaning towards more options to customize standardness, and this would probably be the latest point, where this turns into a bigger issue.
The point I'm trying to make: because of this, well let's call it non-eternal-and-likely-going-to-change-rather-than-sooner-in-one-way-or-the-other behavior, and because consensus critical parts of Omni Core use standard related functions, any change can have a direct impact on the consensus on the meta layer, which should not be the case.
I converted what I mentioned above into code, starting by copying the standard functions Solver(), ExtractDestination() and ExtractDestinations(), then renamed any use of those to SafeSolver(), SafeExtractDestination() and SafeExtractDestinations() in all mastercore_ and omnicore_ files, except selectCoins() and finally I adjust policy. Altogether there were only 8 occurrences:

https://github.com/bitcoin/bitcoin/compare/96a42ad69861211a2badaa9fd4d5dd4945bf684e...dexX7:oc-0.10-guard-against-standard-changes

This does not touch any original Bitcoin Core file, and should basically have no impact of any kind, except ensuring Omni consensus isn't affected by standard changes anymore. As bonus: the user can use any datacarriersize value or disable datacarrier altogether - so it's even possible to enforce class B encoding for outgoing transactions.
Currently I'm waiting for the Travis results as final confirmation, and I'm going to provide test reports, once they are ready.
I totally feel bad for probably sounding too insisting, but I'm convinced this is more secure than enforcing a standard size of 40 byte by overwriting datacarriersize and datacarrier. :)

Okay, just to be clear

You actually made a very good point there - what if there are 3 OP_RETURN outputs in a transaction inside a mined block?  I didn't even consider this case or code for it - the code follows the 'rule' that only one OP_RETURN output is allowed per transaction but I absolutely do see your point - that 'rule' is not an absolute and it's perfectly possible to see this type of thing occur.
Looking from that perspective there may be other places where we need to 'toughen up' a little bit also (ie where assumptions on mining policy have translated into code).

standardness has no impact on Bitcoin validity

Sorry my wording was awful, what I'm referring to is our code - does standardness have any impact on our code - where do we use it, how do we use it, where have we assumed an output is standard and so on.  Most importantly, does anything (else) break when an output is non-standard etc etc.

I converted what I mentioned above into code

I'm surprised at the small scope, I hadn't looked yet but thought it would be more involved :)

I totally feel bad for probably sounding too insisting

Not at all, discussion/debate is how the best results are obtained :)  Given scope is far less than I expected and seems perfectly safe, happy to go with your direction instead bud.

Not at all, ...

Awesome! :)

I'm surprised at the small scope

I figured it's probably easier to show the changes instead of talking about different scenarios, but I was surprised, too!
The only other functions of script/standard we use are GetScriptForMultisig() and GetScriptForDestination(), and they don't embed any rules, but we could clone them (+ txnouttype enum) as well, and remove all references to script/standard. Could be done as follow up optimization at some point.

... what if there are 3 OP_RETURN outputs in a transaction inside a mined block?

Right now only the first one would be processed, and an extension requires a tweak of the "class C specific parsing" section. The data is already available in op_return_script_data, because GetScriptPushes() simply pushes new data at the end of the vector, but doesn't replace or remove any.

Looking from that perspective there may be other places where we need to 'toughen up' a little bit also ...

I think we're covered mostly, and I didn't find other critical areas, or can think of others at the moment. But in the long run it would probably make sense to isolate all consensus relevant code, which might be a bit tricky, at least when it comes to drawing a line where consensus relevant code starts and ends, and then try to minimize the dependencies or potential impact of external code.
For what it's worth: I also can't imagine any scenario where a malicous miner could somehow mess with the consensus state. :)

Travis CI tests (Bitcoin Core tests, cross plattform building, ...):
"SafeSolver, SafeExtractDestination, SafeExtractDestinations":

Commits: 92f8a41...oc-0.10-guard-against-standard-changes
Build and test results: https://travis-ci.org/dexX7/bitcoin/builds/57589319 (6/6 passed)

"Fix class selection size check":

Commits: a4d8540...oc-0.10-pr-13-should-use-class-c
Build and test results: https://travis-ci.org/dexX7/bitcoin/builds/57594213 (6/6 passed)

RPC tests via bitcoin-spock with both merged together:

-datacarrier
-datacarriersize
test report

0
0
110 of 112 passed, 2 failures, 22 ignored

0
40
110 of 112 passed, 2 failures, 22 ignored

1
0
110 of 112 passed, 2 failures, 22 ignored

1
18
109 of 112 passed, 3 failures, 22 ignored

1
40
109 of 112 passed, 3 failures, 22 ignored

1
800
109 of 112 passed, 3 failures, 22 ignored

All failures are unrelated and the ignored tests are not yet supported (meta DEx).
Once I get my hands on a fully synced chain (have only my unsynced notebook here for the next 1-2 days), mainnet consensus comparison tests are up next.
@zathras-crypto: I suggest we let this settle for a moment, think once again about the approach and then consolidate and merge everything within the next days. The spock tests currently don't have explicit OP_RETURN checks, and it's only tested indirectly, but I'd assume it doesn't take very long to cover. :) Given the awesome raw transaction support, we finally can also start with more unit tests directly in Omni Core.

@zathras-crypto: do you consider this as finalized at this point, and ready?
I still hadn't the chance to do mainnet tests, but it would be good to know, whether this can be merged, once consensus is confirmed. Further, what's your opinion on the standard issue at this point? If you agree this is the way to go, I suggest you either adopt the related changes, or I could do a follow up later.

As I say mate I'm fairly confident in the Class C stuff, but I think there probably needs to be some testing written around all these new RPC calls I put in.  I mean I tested myself with simple test transactions but we couldn't in good conscience allow them out into the wild without doing the tests for fringe cases/invalid values and so on - hopefully some of the testing infrastructure that's been built up over the last months can help here?
Re standard, you made the case well there - I'm happy to incorporate your suggested changes from here https://github.com/dexX7/bitcoin/compare/92f8a41230586e01e9c3e78a3f161920f058f35b...dexX7:oc-0.10-guard-against-standard-changes
As to merging - I'm of split minds - I thought the concept was to only merge changes to main when they are ready and properly tested, but I also see your point that merging could enable a greater testing audience for those changes in the first place(so long as no one uses it for real!) - so unsure if I'm honest lol :)

I thought the concept was to only merge changes to main when they are ready and properly tested.

Yeah, you're completely right! I pretty much focused on class C, and neglected the transaction creation. Over the last days bitcoin-spock got it's own raw transaction creator for raw Omni data of all transaction types, which should help a lot. I'll take a stab at it tomorrow and build some regtests. :)
Speaking of: given that raw transactions can now be created via Omni Core, I actually think get_payload should be available as well. Not as full blown decoding of course, but the payload extraction used for OmniChest.

TIL: Omni Core happily parses 1-of-6 bare multisig scripts.
This is no new behavior, and mostly unrelated to the Solver() discussion, but yet another point not 100 % matching the spec.
I don't consider it as bug, and neither as an issue, and just in case: this was tested on regtestnet, it can occure on testnet at any time, and may happen on mainnet, if someone really wants to.
But given that context, I actually think multiple OP_RETURNs should be processed as well.

@zathras-crypto: please correct me if I'm wrong, but I think this introduces a consensus break, which relates to the DEx commitment fee:

Case 1 (old):
// quickly go through the outputs [...]
for (unsigned int i = 0; i < wtx.vout.size(); i++)
{
    CTxDestination dest;
    string strAddress;
    outAll += wtx.vout[i].nValue; // <----------------------------------------- !!
    if (ExtractDestination(wtx.vout[i].scriptPubKey, dest)) {
        // ...

Any output, and it doesn't matter of which type contributes to outAll.

Case 2 (new):
// ### EXODUS MARKER IDENTIFICATION  [...]
for (unsigned int i = 0; i < wtx.vout.size(); i++)
{
    txnouttype outType;
    if (!getOutputType(wtx.vout[i].scriptPubKey, outType)) continue;
    if (outType == TX_PUBKEYHASH) {
        CTxDestination dest;
        string strAddress;
        outAll += wtx.vout[i].nValue; // <------------------------------------- !!
        if (ExtractDestination(wtx.vout[i].scriptPubKey, dest)) {
            // ...

The contribution to outAll is no longer based on all outputs, but only based on those, which pass the getOutputType check, and further are of type TX_PUBKEYHASH.

Consider the transaction:
Input  1: 0.00021092 BTC

Output 1: 0.00000546 BTC - Exodus marker
Output 2: 0.00000546 BTC - Reference output
Output 3: 0.00000684 BTC - Payload "accept DEx offer" (embedded via multisig)

And calculating the transaction fee:
Fee case 1: 0.00021092 - (0.00000546 + 0.00000546 + 0.00000684) = 0.00019316 BTC
Fee case 2: 0.00021092 - (0.00000546 + 0.00000546             ) = 0.0002     BTC

Case 2 doesn't count the 0.00000684 BTC, because the output is not of type TX_PUBKEYHASH.
This can break consensus, if there were a DEx offer with a commitment fee of 0.0002 BTC, resulting in a valid accept in case 2, but an invalid accept in case 1.

please correct me if I'm wrong

You're not wrong - great spot - what would we do without you!!!???!!!
Thanks again :)

Haha, yeah, this was really close. ;)
Something else I was wondering about, but related to the transaction creation: say you'd actually want to accept a DEx offer, and use senddexaccept_OMNI to do so: how is the commitment fee handled? As far as I can see, the fee is applied via the internal wallet transaction creation, and there doesn't seem to be a really straight forward way to set a fee for a specific transaction.

Something else I was wondering about, senddexaccept_OMNI

Shooooot!  You picked that up yourself - I should have guessed :p  That was the one issue that I did know about (sadly the outAll one I missed completely :( yikes) and wanted to see if the spock/travis testing infrastructure was effective enough to pick it up.  Guess you saw it first hehe (if that's bad form on my part for not reporting it I apologize, but I wanted to "test the testing" if that makes sense).
Re the issue itself, I looked at a few options - the simplest was to inflate the client mining fee before calling accept creation and then drop it back afterwards - but I didn't "like" it because the fee is per KB and whilst most accepts would be sub 1KB it's another of those 'assumptions' I didn't want to put in code (eg imagine we needed a lot of inputs - could end up paying n multiples of the accept fee).
TL:DR; I spent more time on it than I care to admit without a great solution - so I did what I usually do in those cases hehe (bounce off to some other topic hoping that coming back to it 'fresh' gives me some inspiration)...  Of course if you have any suggestions I'm all ears too :)
Thanks
Z

There are unfortunally not yet spock tests for every aspect, and the ones for accepting offers are still missing. :(
How about.. creating the transaction, but without commiting it, then checking the fee, and if it's not sufficient, rejecting the accept, and otherwise broadcasting it? Out of the blue, this is probably the easiest route I can think of.

// Actual run, continue ...
int result = ClassAgnosticWalletTXBuilder(fromAddress, toAddress, "", 0, payload, txid, rawHex, autoCommit);

In the long run this could be addressed more properly, and it's probably not too far fetched to have a deeper access to the wallet and transaction creation at some point, but right now it's more about prevening some unexpected, bad outcome. :)

Yeah that would work without touching the bitcoin code, do you think it's too restrictive?
FYI the bitcoin changes are small, but I figured it would be better to find a non-invasive way to do it since we're trying to keep bitcoin changes to a min.  Not sure how to show you that from my scratch stuff without committing, erm - in a nutshell the bitcoin changes needed were:

Add an optional omniMinAcceptFee = 0 param to CWallet::CreateTransaction
Do same to GetMinimumFee
Then you can pass the accept Fee through to GetMinimumFee which has the extra case:

    // Handle Omni accepts with minimum mining fee
    if (nFeeNeeded < minAcceptFee)
        nFeeNeeded = minAcceptFee);

added to it 2nd to last (before the final max check:)
    // But always obey the maximum
    if (nFeeNeeded > maxTxFee)
        nFeeNeeded = maxTxFee;

TL:DR; I couldn't find a way to do it purely in Omni code & but was hoping when I took a fresh look I might see a different avenue :)

Oh that's super cool :)

I'm quite surprised and GetMinimumFee() actually looks pretty interessting, especially the first entry:
// payTxFee is user-set "I want to pay this much"
CAmount nFeeNeeded = payTxFee.GetFee(nTxBytes);
// user selected total at least (default=true)
if (fPayAtLeastCustomFee && nFeeNeeded > 0 && nFeeNeeded < payTxFee.GetFeePerK())
    nFeeNeeded = payTxFee.GetFeePerK();

... isn't this, exactly what we want?
The tooltip sounds also great:

I looked at a few options - the simplest was to inflate the client mining fee before calling accept creation and then drop it back afterwards - but I didn't "like" it because the fee is per KB and whilst most accepts would be sub 1KB it's another of those 'assumptions'

What would happen, if the transaction were indeed larger than 1 KB?
Just thinking about something like (which is probably what you tried):
// Outer scope:
extern CFeeRate payTxFee;
extern bool fPayAtLeastCustomFee;

// In senddexaccept_OMNI:
CFeeRate payTxFeeOriginal = payTxFee;
bool fPayAtLeastCustomFeeOriginal = fPayAtLeastCustomFee;

payTxFee = CFeeRate(nMinimumDexFee, 1000);
fPayAtLeastCustomFee = true;
//
// ... send
//
payTxFee = payTxFeeOriginal;
fPayAtLeastCustomFee = fPayAtLeastCustomFeeOriginal;

Heh.

isn't this, exactly what we want?

I completely missed that - forgot all about the new transaction fee bit that came in with 0.10 :anguished: and just went straight to adding our own case.  That approach should be perfect :)

Yeah, I didn't notice until now either. :)
I can give it a try, probably on monday, and I plan to work further towards transaction creation and encoding tests in general. Not really sure how to tacke it though, because there are many combination and sequences, to cover the requirements.
What would you consider as highest priority, maybe even unrelated to the transaction creation, at this point?

What would you consider as highest priority, maybe even unrelated to the transaction creation, at this point?

I think at the moment MetaDEx will always get the priorities because of it's value but TBH I think it's all important - it's probably smart to finish the Class C & encoding piece if only so we can merge it and put all the focus on MetaDEx.

I think at the moment MetaDEx will always get the priorities because of it's value but TBH I think it's all important - it's probably smart to finish the Class C & encoding piece if only so we can merge it and put all the focus on MetaDEx.

Yes, I agree. This PR should get in, before anything else, mainly due to it's huge size and the several parts it touches. So let me rephrase: what would you consider as highest priority or must-have, to consider this one as merge ready?

what would you consider as highest priority or must-have, to consider this one as merge ready?

Well, I guess I'd consider these prerequisites:

We sort the accept fee issue
Our testing team gives Class C the OK
Our testing team gives encoding the OK

Note when I say testing team I believe that's still you, Marv & Sean?  Craig's not told me about any changes so I'm just assuming we still have a testing team :grimacing:

@zathras-crypto: I guess I'd consider these prerequisites ...

Alright. In this case I should probably focus on creating additional tests.
I actually haven't heard from Marv in regards of tests or so for a while, and I started to wonder, given the context of the meta DEx test plan, and no further updates.

While testing something with bitcoin-spock, I stumbed upon a dust-ish value, which was off by 2 satoshi.
It turned out, the IsDust() check slightly changed it's behavior from Bitcoin Core 0.9 to Bitcoin Core 0.10 (probably unintended) due to integer rounding effects, and this has an impact on GetDustThreshold(), because IsDust() is basically used as reference for the limit/threshold caluclation.
Example with nValue = 927 (result of dust threshold calculation), nSize = 182 (size for a pay-to-pubkey-hash output) and nRelayTxFee = 1700 following:
IsDust() in Bitcoin Core 0.9:
  nValue * 1000 / (3 * nSize) < nRelayTxFee

= 927    * 1000 / (3 * 182)   < 1700
= 927000        / 546         < 1700
= 1697                        < 1700

= true

IsDust() in Bitcoin Core 0.10:
  nValue < 3 * (nRelayTxFee * nSize / 1000)

= 927    < 3 * (1700        * 182   / 1000)
= 927    < 3 * (309400              / 1000)
= 927    < 3 * 309
= 927    < 927

= false

So what does this mean in theory?
If one uses the default -minrelaytxfee=0.00001, then the results are similar, but when using for example -minrelaytxfee=0.000017, then the old GetDustLimit() returns 929, while the new GetDustThreshold() returns 927.

The value 929 is no dust in Bitcoin Core 0.9, and also not in Bitcoin Core 0.10
The value 927 is considered as dust in Bitcoin Core 0.9, but not in Bitcoin Core 0.10*

*If the same minrelaytxfee would be used!
In practise this isn't relevant, because the minimum relaytxfee default value 0.00001 (or 1000 satoshi) is not affected by rounding issues, and going lower than that is considered as dust by both clients, and increasing that number is considered as no dust by both clients, so the outcome is similar.
Nevertheless: I tend to stick to the old calculation, just in case, and I'm going to push it as part of some other PR, e.g. the unfinished encoding and transaction creation PR.

Nevertheless: I tend to stick to the old calculation, just in case

What's the concern here - 0.0.10 is consensus breaking so no-one would be able to use Bitcoin 0.9 based clients anyway.

What's the concern here - 0.0.10 is consensus breaking so no-one would be able to use Bitcoin 0.9 based clients anyway.

Hm.. true. Yeah, there is basically no issue.

I actually haven't heard from Marv in regards of tests or so for a while, and I started to wonder, given the context of the meta DEx test plan, and no further updates.

I'll check in with @CraigSellars at the next meeting (about 2 days away) and find out what's happening with testing resources etc and update you mate...

@dexX7 FYI I raised the issue of further developing the metadex test plan to catch more of thee fail cases at the meeting today - there is another meeting to discuss MetaDEx testing scheduled in a couple of days, you should have been on the invite...  If you can make it great, if not let me know anything you'd like me to raise :)

I received the invitation and I'm going to participate. Thanks!

FYI started about 40 mins ago mate if you still want to join :)

@zathras-crypto: if you have some time, please resolve the merge conflicts.
I'm going to add the creation methods to OmniJ, so it would be great, if this PR could be merged - at least in theory.
To give you an overview of the unit tests, which are implemented:

CreatePayload_XXX: comparison to known reference
CreatePayload_XXX: comparison to known reference, but empty text fields
CreatePayload_XXX: comparison to known reference, but 700 char texts to test trimming
OmniCore_Encode_ClassC: confirm prepending of marker

@zathras-crypto: if you have some time, please resolve the merge conflicts.
I'm going to add the creation methods to OmniJ, so it would be great, if this PR could be merged - at least in theory.
To give you an overview of the tests, which are implemented:

CreatePayload_XXX: comparison to known reference
CreatePayload_XXX: comparison to known reference, but 700 char texts, to test trimming
CreatePayload_XXX: comparison to known reference, but empty text fields
GetDustThreshold: confirm expected amounts for allowed output types
GetDustThreshold: confirm the amounts are minimal (= dust + 1 satoshi)
GetOutputType: confirm identification of allowed, and other output types
GetScriptPushes: confirm extraction of data for allowed, and non-standard output types
OmniCore_Encode_ClassC: confirm embedding and prepending of marker
OmniCore_Encode_ClassC: confirm embedding and prepending of marker, with empty payload
PrepareObfuscatedHashes: comparison to known reference hashes
SafeSolver: confirm OP_RETURN is not influenced by user settings

I haven't touched it since the last update, because I'm sort of out of ideas. Please let me know, if you have something else in mind, which could or should be tested.

@zathras-crypto: if you have some time, please resolve the merge conflicts.

Done.

Please let me know, if you have something else in mind, which could or should be tested.

I just dropped in some code for the accept fee handling as per our discussion earlier on, but I'm questioning whether the automatic retrieval and application of the required fee might be a bad idea...  Thoughts?

Hmm.. well, let me say it this way: the DEx is very user unfriendly, and it's up the clever UI and wallet designers to deal with it. This isn't only the fee, but also the payment window..
I have an idea though: how about an extra flag, which defaults to false, to explicitly confirm paying a fee, which is higher than the client fee, and further, to explicitly accept offers with a payment window of less than 3 blocks?
If this flag is false, and it's an "unsafe" situation, a warning could be returned with information about the fee to pay and the payment window.
"Power" users can always create transactions with enabled flag right from the beginning, so this wouldn't be very restrictive.

That's not a bad idea.  Also 3 blocks is a little on the low side IMO - IIRC Masterchest Wallet & OmniWallet both agreed to apply a minimum 10 block accept to wallet functions.  This isn't per spec, but too many people were losing money to 'shady' offers with low payment windows (especially with the incorrect assumption of some users that an unconfirmed transaction will always be mined in the next block) so we felt it was the responsible thing to do as developers of those respective softwares.
I would say being the reference client I don't like to enfore things that aren't in the spec, but if we as you note allow "power" users to bypass it then it's not a concern.
I'd like to do same and probably change the sell RPC call respectively to restrict listing sell offers with shady payment windows, but again I don't like adding stuff to a reference client that isn't per spec.
Hmm...  Not sure which way to go, but thanks for bringing it up - we definitely want to do something.

Also 3 blocks is a little on the low side IMO

Well, then let's use 10 for accepting offers.

Not sure which way to go re listing sells

Imho there should be no restriction for creating offers, because buyers are already protected via the accepting flag.
I would have suggested to provide sane default values though (like 10 or 20 blocks or so), but this requires to restructure the order of the parameters.

I would have suggested to provide sane default values though (like 10 or 20 blocks or so), but this requires to restructure the order of the parameters.

Since it's a new call you can suggest changes mate, parameter order for those new calls is not set in stone until we release.

Since it's a new call you can suggest changes mate, parameter order for those new calls is not set in stone until we release.

We could make it more user friendly this way, and provide sane default values, where the user safely can create and accept offers, without caring very much about all those "confusing" details. Most offers would end up with those default values, and if someone wants to use something else, he or she can do so of course. When accepting a shady offer, then buyer warning protection would be triggered.
So the question really is: how far do we want to go here? I'm all in for easier interfaces, but at the same time one could argue, all this could be done on another layer, i.e. the UI or in some external wallet, such as Omni Wallet.

I kind of like route 3 but then I think default parameters benefit end users, and how many end users are going to be making RPC calls to send transactions?

So the question really is: how far do we want to go here? I'm all in for easier interfaces, but at the same time one could argue, all this could be done on another layer.

Yeah, I think you hit the nail on the head here.  Making the API 'user friendly' should not receive as much attention as making say the UI user friendly.

@zathras-crypto: I'd like to get some kind of consolidation going at some point (especially Class C stuff)

The class C part seems solid, also the data embedding and the raw transaction construction. What's missing are tests for the new creation commands via RPC.
I completely agree re: consolidation, especially given that this is a rather large PR, and I would actually prefer to get this in as fast as possible. One of the reasons is to "use" the new code, in addition to the testing of some isolated parts, which, well, let's say, provides much more exposure and chances to spot defects, and it's sort of lost time, if it's unused, at least in my opinion.
On the other hand, there is also the chance of introducing sneaky, and hard to spot issues, such as the almost-consensus-break related to the DEx commitment fee, as outlined in https://github.com/OmniLayer/omnicore/pull/13#issuecomment-91921821, and handling it too loose probably increases the chance of hidden issues due to the increased overall complexity of several new things mixed together.
So let's evaluate the following options:

Merge (when?)
Remove the RPC creation commands and add them later seperately (would you prefer this?)
Wait (how long, and which requirements have to be met?)
???

I favor 1, and as soon as possible, once you agree. 2 might be an option, but this creates additional work, and I wouldn't consider the RPC interface as critical enough to justify it, mostly because it doesn't have much impact on the rest.

What's missing are tests for the new creation commands via RPC.

Perhaps we should open an issue and have a bunch of 'todo' on there?

such as the almost-consensus-break related to the DEx commitment fee

This is true, and was a nice spot - thanks :) - but aren't we breaking consensus anyway the moment we live either Class C or MetaDEx?  Obviously we need to stay in consensus for that period of 0.0.10 release to feature live while people upgrade, but I don't see that being an extended period of time.

handling it too loose probably increases the chance of hidden issues

Agreed,

So let's evaluate the following options:

Merge (when?)

IMO asap.

Remove the RPC creation...

I don't think that's necessary - anyone can build busted up/incorrect transactions and broadcast them, if there is something wrong with the new RPC creation commands I don't see that as introducing any extra risk right now - but they have to be tested/solid before release.

Wait (how long, and which requirements have to be met?)

Golden question :)

Perhaps we should open an issue and have a bunch of 'todo' on there?

Good idea, though I'd prefer to have one issue for each task, issue or todo point, which is easier to clear and focus on imho.

but aren't we breaking consensus anyway the moment we live either Class C or MetaDEx?

Yes, but there is a significant difference: the consensus breaks for class C and the meta DEx are scheduled and synchronized for all participants, whereby this bug would have been live right from the beginning, and 0.0.9 users could be fooled into a wrong (according to 0.0.10 logic) state.

IMO asap.

Then we agree.

Golden question :)

There is a small change required:
diff --git a/src/mastercore_sp.h b/src/mastercore_sp.h
index 3c710ce..ed64ee9 100644
--- a/src/mastercore_sp.h
+++ b/src/mastercore_sp.h
@@ -7,6 +7,7 @@

 class CBlockIndex;

+#include "base58.h"
 #include "tinyformat.h"
 #include "uint256.h"
 #include "utiltime.h"

I'm going to run mainnet consensus checks in a few minutes, on top of 28, as final confirmation for both.

Ah, ok - that's interesting - I had a merge conflict and checked the current branch 0.0.10 (https://github.com/OmniLayer/omnicore/blob/omnicore-0.0.10/src/mastercore_sp.h) which didn't have the base58.h in it.

Thanks for the patch - I did it slightly differently to maintain the 'flow' of approach but if this isn't suitable just let me know and I'll drop it out and replace with your patch.

Looks fine on the first look. Build ongoing.

Peew.. :)
Great work @zathras-crypto, one very significant milestone accomplished! :)

Great work @zathras-crypto, one very significant milestone accomplished! :)

Thanks dude, your help was invaluable as always :) :) :)