Rework tx creation + implement Class C & missing transactions

[zathras-crypto]

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 200 1430971505 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" 2147483692
95b62d82432a06f3904e4312ed4384a9b57b63f301b0764c893e699ef694107a

# src/bitcoin-cli sendgrant_OMNI "mpZATHupfCLqet5N1YL48ByCM1ZBfddbGJ" mpZATHm5ih33ePoSPxFQ4cvWZWaPkN4G5s 2147483691 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 2147483690
9b831ef60919c42be1ede10fbe4c773a622144669f7dbaa7bb4452574a9263a2

[dexX7]

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

Given all the type checks, it might be worth the redo something mastercoin-MSC#167 (comment) 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.

Edit: in a seperate PR. I'd like to get this one in as soon as possible.

[dexX7]

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

[zathras-crypto]

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

Thanks bud :)

Given all the type checks

Yeah that 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 0.0.9.2-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 :)

[dexX7]

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.

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.

[dexX7]

@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 of 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.

@msgilligan: ping for additional input. :)

[zathras-crypto]

@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?

[dexX7]

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.

[zathras-crypto]

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.

EDIT for clarity : Not sure which way to go re listing sells, your suggested flag is fine for accepts :)

[dexX7]

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.

[dexX7]

What's wrong about no fee?

[zathras-crypto]

What's wrong about no fee?

It's illegal per spec? The field is 'number of coins' which requires a positive value.

[dexX7]

It's not validated, as it seems (regtest mode):

> sendrawtx_MP "mz2kMTWoHnuvapPkHRgCPQt5tZJNFjNwj7" "00010014000000010000000008f0d180000000174876e8000a000000000000000001"
82a313b0de75058054307ff092175772352d2dcae3ad71f98b393a3a80ee65f7

> setgenerate true 1
[
  "6eea2284a15a9d005a320405180c53325470c71e809525e0b7f9dad22acecc08"
]

> gettransaction_MP 82 a313b0de75058054307ff092175772352d2dcae3ad71f98b393a3a80ee65f7
{
  "txid": "82a313b0de75058054307ff092175772352d2dcae3ad71f98b393a3a80ee65f7",
  "sendingaddress": "mz2kMTWoHnuvapPkHRgCPQt5tZJNFjNwj7",
  "ismine": true,
  "confirmations": 1,
  "fee": 0.00009999,
  "blocktime": 1429930699,
  "version": 0,
  "type_int": 20,
  "type": "DEx Sell Offer",
  "propertyid": 1,
  "divisible": true,
  "amount": "1.50000000",
  "feerequired": 0.00000000, <<<
  "timelimit": 10,
  "subaction": "New",
  "bitcoindesired": 1000.00000000,
  "valid": true <<<
}

So let's tackle it this way: do you think it would make sense, to enforce a minimum on the RPC layer? I'm not sure actually, and I'm wondering, what benefit it provides, if zero commitment fee are not disallowed.

[zathras-crypto]

It's not validated, as it seems (regtest mode):

Right right... I don't know what the purpose of a non-zero commitment fee was (I kind of just mostly went off the spec when building those send rpc calls). Given that it's not enforced I think the safest thing is to say zero values are allowed and the info in the spec should be amended.

[dexX7]

I agree, sounds good.

[zathras-crypto]

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.

[dexX7]

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.

Okay, here are a few ideas:

Route 1:

senddexsell: address, property, amount, amount desired, blocks, minfee, action

no default values

Route 2:

createdexsell: address, property, amount, amount desired, blocks, minfee [implies action = 1]
updatedexsell: address, property, amount, amount desired, blocks, minfee [implies action = 2]
canceldexsell: address, property, amount, amount desired, blocks, minfee [implies action = 3]

no default values

Route 3:

createdexsell: address, property, amount, amount desired, (blocks, minfee) [implies action = 1]
updatedexsell: address, property, amount, amount desired, (blocks, minfee) [implies action = 2]
canceldexsell: address, property, amount, amount desired, (blocks, minfee) [implies action = 3]

default value for blocks = 25 blocks to pay
default value for minfee = 0.0001 BTC/25 MSC

...

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.

[zathras-crypto]

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.

[dexX7]

@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 #13 (comment), 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:

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

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.

[zathras-crypto]

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 :)

[dexX7]

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.

[zathras-crypto]

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.

EDIT: confirmed, sorry :)

[dexX7]

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.

[dexX7]

Peew.. :)

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

[zathras-crypto]

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

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