Class C support for 0.0.10

[zathras-crypto]

Note: Not yet final.
Also pending outcome of OmniLayer/spec#281.

This PR adds Class C support for both encoding and decoding. Class C in the current context (but open to change) is:

• Data packet moved to OP_RETURN output
• Data packet is not obfuscated
• Exodus marker removed and replaced with omni bytes at beginning of OP_RETURN output

A singular send function (ClassAgnostic_send) is now used which will default to Class C if the packet is small enough, otherwise falling back to class B.

[dexX7]

I think it would be useful to provide an option to disable OP_RETURN, e.g. via CLI command, but this would only be nice to have. I shared my opinion on the topic in general in the other thread, and this PR reflects the discussion pretty well, but I insist on a short marker.. ;)

The spock tests require a minimal change, which I'm going to address tomorrow.

What I'm wondering about though: what exactly is MIN_PACKET_SIZE_CLASS_C and why is it required?

Quick side note: as I know have merge rights, I think we should discuss and define a simple merge/contribution process over the next days.

[zathras-crypto]

I think it would be useful to provide an option to disable OP_RETURN

You can disable OP_RETURN simply by setting datacarrierbytes to 0 :)

What I'm wondering about though: what exactly is MIN_PACKET_SIZE_CLASS_C and why is it required?

Supposed to be the smallest supported cleartext payload, to avoid passing in knowingly too short data payloads for further processing.

Quick side note: as I know have merge rights, I think we should discuss and define a simple merge/contribution process over the next days.

Oh - congrats! Re merge/contribution process um I'm not sure to be honest mate no-one has talked to me about this so I'm not sure - to date @m21 has always been the merge guy (though he's been trying to encourage me to take over which I try to avoid hehe) but I guess a more community driven model is appropriate now he's focused on Factom...

[dexX7]

Supposed to be the smallest supported cleartext payload, to avoid passing in knowingly too short data payloads for further processing.

Ah gotcha, so this is basically a range/length check to prevent other parsing steps try to process what's not there. I was just wondering, because step1() only parses version and type and I couldn't find the relation to the actual data embedding mechanism. Maybe rename it to MIN_PAYLOAD_SIZE or so?

[zathras-crypto]

Maybe rename it to MIN_PAYLOAD_SIZE or so?

Good suggestion :) FYI the value is also wrong, I forgot about the 'close crowdsale' transaction which is even smaller.

[dexX7]

@zathras-crypto:

1. Can you think of a way where parseTransaction() is transformed into something that doesn't use GetTransaction() to fetch the previous transactions, but instead, where the previous transactions are provided or so? I'm thinking about unit tests here, and in it's current form it seems impossible to create any, because without a history, it seems impossible to grap previous transactions via GetTransaction(), unless there is another way, I'm currently not seeing.
2. For the first tests, it would be sufficient to test, whether parseTransaction() is able to extract payloads and doesn't fail. For the future, it would be awesome, if parseTransaction() were side effect free, in particular I'm referring to the calls of TXExodusFundraiser() and DEx_payment().

We could create a bunch of RPC/spock tests to simulate all this, and test parseTransaction() indirectly, but unit tests are much faster and much easier to execute (./test_bitcoin), and would also allow to just create a bunch fo raw transactions (probably including previous transactions) and really test every aspect. Do you roughly understand where I'm going here..? :)

Edit: ping @msgilligan as grand test master as well.. :)

[zathras-crypto]

@dexX7 not off the top of my head mate, initial thoughts would be breaking down parseTransaction() into smaller chunks (checkMarker, parseSender, parsePayload, and so on).

FYI on the topic of tests, I'm nearly through with my work on missing transactions which is quite a change to sending and will require a bunch of testing (it removes send_INTERNAL_1packet entirely for example) - I should be pushing something close-to-complete over the next day or so :)

Thanks
Z

[dexX7]

initial thoughts would be breaking down parseTransaction() into smaller chunks (checkMarker, parseSender, parsePayload, and so on)

Yeah, that's probably the way to go, long term at least, and I'm testing some parts of that already (+ there is a Java implementation with a similar approach), but this seems too complex to quickly add it into the class C pull request.. ;) I was just wondering, if you may have had some other idea, which might have been easier.

FYI on the topic of tests, I'm nearly through with my work on missing transactions which is quite a change to sending and will require a bunch of testing

Please push! :) I already have your 0.0.10-Z-TechDebt_MissingTransactions locally, and it's just waiting to be used. :)

Did you already consider RPC commands?

The RPC interface should probably be "checked" sooner or later, and this is something, I have in mind for some days, actually a bit similar to what you mentioned with "let's map all events for the UI", and in particular: it would be helpful, if there were a clean list for each command / transaction type with requirements to pass to finalize the transaction creation or broadcat. For example: "does the user have enough balance?", "does this or that property exist?", ...

[zathras-crypto]

Yeah I did already consider the RPC interface, basically class*_send has been morphed into classAgnosticWalletTXBuilder and the process is largely to simply do some checks, call CreatePayload_type and pass that into classAgnosticWalletTXBuilder from each RPC call (or UI element) that wants to create a transaction.

There are bits I put in too like for example you can start with ---autocommit=false and then all send RPC commands will return the raw hex for the created tx instead of committing and broadcasting and returning a txid (figured that would help with testing after talking with @msgilligan).

I'm fleshing it all out and it'll be ready to push soon, though it's all merged with Class C stuff now so I may close this PR and open another (sadly larger in scope) PR to reflect what I've changed properly.

Speaking of RPC, pinging everyone here for commentary on whether the nomenclature is OK?

zathras@zlappy:~/github/build/mastercore$ grep _OMNI src/rpcserver.cpp 
    { "omni layer",   "send_OMNI",                    &send_OMNI,                     false,  false,  true },
    { "omni layer",   "senddexsell_OMNI",             &senddexsell_OMNI,              false,  false,  true },
    { "omni layer",   "senddexaccept_OMNI",           &senddexaccept_OMNI,            false,  false,  true },
    { "omni layer",   "sendissuancecrowdsale_OMNI",   &sendissuancecrowdsale_OMNI,    false,  false,  true },
    { "omni layer",   "sendissuancefixed_OMNI",       &sendissuancefixed_OMNI,        false,  false,  true },
    { "omni layer",   "sendissuancemanual_OMNI",      &sendissuancemanual_OMNI,       false,  false,  true },
    { "omni layer",   "sendsto_OMNI",                 &sendsto_OMNI,                  false,  false,  true },
    { "omni layer",   "sendgrant_OMNI",               &sendgrant_OMNI,                false,  false,  true },
    { "omni layer",   "sendrevoke_OMNI",              &sendrevoke_OMNI,               false,  false,  true },
    { "omni layer",   "sendclosecrowdsale_OMNI",      &sendclosecrowdsale_OMNI,       false,  false,  true },
    { "omni layer",   "sendtrade_OMNI",               &sendtrade_OMNI,                false,  false,  true },
    { "omni layer",   "sendchangeissuer_OMNI",        &sendchangeissuer_OMNI,         false,  false,  true },

Note aliases for send_MP > send_OMNI and sendtoowners_MP > sendsto_OMNI and so on will be employed for backwards compatibility.

[dexX7]

There are bits I put in too like for example you can start with ---autocommit=false and then all send RPC commands will return the raw hex ...

Interesting approach. I'm very curious about the code, because this is really, really an awesome feature, but requiring a restart is pretty harsh. On the other hand, duplicated commands, one with createXX, and one with broadcastXX might create a significant overhead. Adding more flags, such as broadcast, which defaults to true could be an alternative. But let's see.. :)

Speaking of RPC

We should get a working document for all transaction types. This document should list RPC commands and parameters, a short name, and a long, descriptive name. This is something I stumbled upon so many times, and I always ended up with slightly different results, whether it's during tests, the spec, or some documentation. And there is also the Omniwallet API. Maybe it's time to normalize it once and for all? :)

Regarding the actual commands:

1. I still hate the case-sensitive (!) suffix... ._." ... whether it's _MP or _OMNI. The commands are plain strings, so it would even be fine to use something like "abc::de->f--=send@" as command (just an example). Is there really no other way? :/
2. Replacing send_MP with send_OMNI breaks all integrations. There is a nice feature since 0.10: the category can be set to "hidden", to hide the commands from help. Maybe the old commands could still be available, but hidden, at least for one version, which nevertheless call send_OMNI in the end?

[dexX7]

Oh, nevermind, you actually mentioned aliasing.

sadly larger in scope

Given that omnicore-0.0.10 is a WIP branch anyway, we should probably get changes in more often, and in smaller pieces, instead of complete features in one larger chuck, even if this might end up with some back-and-forth or code moving.

[zathras-crypto]

I still hate the case-sensitive (!) suffix... ._." ... whether it's _MP or _OMNI

I'm fully open to changing the naming convention :) I just felt MP was inappropriate/outdated so changed in line with our existing convention. Feel free to suggest alternatives and see if we can get a consensus on alternate nomenclature (though I would insist on keeping the old _MP commands around (aliased) for a while at least).

Given that omnicore-0.0.10 is a WIP branch anyway, we should probably get changes in more often, and in smaller pieces, instead of complete features in one larger chuck

Yeah, I don't disagree mate and I'm probably being a bit of a PITA with these large chunks. The problem is a lot of my work is experimental - for example I spent about 6 hours on an alternate approach before deciding it was worse than what we have now and abandoning it in favour of this other approach I'm now working on. I always felt committing (crap) like that was bad form which is why I usually only commit work I intend to ask to be put into the project (I have all kinds of rubbish local that never made it up onto GitHub hehe). Perhaps I need to adjust my workflow a bit.

[zathras-crypto]

Interesting approach. I'm very curious about the code, because this is really, really an awesome feature, but requiring a restart is pretty harsh

That can be avoided very easily just by allowing that flag (default bool autoCommit = true;) to be set via the RPC interface as well as (instead of?) a startup param.

I started with multiple cloned commands but it blew out with huge amounts of duplicate code very quickly. I also looked at adding a parameter to the sendX_OMNI calls but some of these have optional parameters which need to be last and it would break backwards compat to put other params in earlier in the call (plug it seemed somewhat 'ugly' to have to specificially say for every transaction that you want to commit it). Having a (it's a global) flag that we can just flip on/off for the commit stage of transaction creation makes things much simpler from a code PoV.

[dexX7]

though I would insist on keeping the old _MP commands around (aliased) for a while at least)

Definitely, otherwise it would probably break some integrations.

alternate nomenclature

It's tricky, and I have no good solution in mind, unfortunally. A lower case prefix with underscore feels more intuitive, but this is just a feeling. Alternatively, commands could be grouped (e.g. dex_xxx, mdex_xxx, sto_xxx, ...), or there might be no explicit part at all, but instead the names are changes to something conflict free (e.g. instead of getbalance_MP, something like gettokenbalance or get_token_balance).

'ugly' to have to specificially say for every transaction that you want to commit it

I see your point. Let me think of this.. I could imagine there might be a way where a flag (hidden or visible) is used at very end, even with default parameters, combined with some alias/forwarding calls. For example: create_xxx internally calls send_xxx ... ... false.

That can be avoided very easily just by allowing that flag (default bool autoCommit = true;) to be set via the RPC interface as well as (instead of?) a startup param.

This is a good idea.

[zathras-crypto]

I could imagine there might be a way where a flag (hidden or visible) is used at very end, even with default parameters, combined with some alias/forwarding calls.

Great minds and all that hehe, yep I played with this too - the problem is the way the RPC code calls the method, it seemed to be built around always having the same structure and I couldn't find workable way to have two different RPC commands call the same Value blahblah_OMNI with a third param eg (const json_spirit::Array& params, bool fHelp, bool shouldICommit) without butchering the RPC code sadly.

[zathras-crypto]

For example: create_xxx internally calls send_xxx ... ... false

FYI that is kind of how it works now - the last parameter to ClassAgnosticWalletTXBuilder is a bool on whether to commit it or not. I'm bouncing between some family stuff at the mo (Easter and all, the kids love it - probably all that chocolate!) but I'll get the final pieces finished and push it up - probably tomorrow - for you to take a look at bud :)

EDIT: sorry should clarify, when I say works now I just mean the internal function to create transactions takes a bool on whether to commit - there are no 'create_xxx' RPC calls (I did actually clone a few in this manner (eg send_OMNI and createsend_OMNI) but the code was entirely duplicated every line bar one - it seemed awful!)

[dexX7]

it seemed to be built around always having the same structure

Yup, it's very strict.

What I had in mind was something like: https://gist.github.com/dexX7/273529eebd07967f484c (edit: the example is probably a bit lame, because HandleTxCreation could as well just have a bool shouldICommit, but it hopefully serves as example for "adding some flags" ... :)

But ... actually, when I think about it: to broadcast a transaction, the transaction has to be created first anyway, right? So "broadcast_xxx" might just call "create_xxx" and then broadcast it's result?

Edit: happy eastern and have fun! :)

[msgilligan]

I don't like the idea of a configuration flag that requires a restart, nor do I like the idea of an RPC that creates global state. Since the "don't commit" flag would probably be used mostly for testing, why not just make it the very last parameter and require that the preceding optional parameters be explicitly specified?

[zathras-crypto]

@msgilligan can you please be more specific & elaborate? "I don't like it" isn't super helpful :)

Since the "don't commit" flag would probably be used mostly for testing

FYI the reason there is a global is to put OmniCore into a sort of testing mode (to return raw transactions). I cannot think of any reason it would be used outside of that context (eg this functionality cannot be used by end users to create unsigned raw transactions etc or non-wallet raw transactions etc).

why not just make it the very last parameter and require that the preceding optional parameters be explicitly specified?

Let's assume we are using the raw signed hex as part of our testing and further let's assume we need to specify all optional parameters and lastly a bool for whether to commit - now we've effectively restricted any ability to test commands without the full suite of optional parameters - eg out of the following two test scenarios, you can only test one of them:

send_OMNI fromAddress toAddress 1 0.01 true (error - cannot test without optional refAmount parameter)
send_OMNI fromAddress toAddress 1 0.01 0 true (ok - can test with optional parameter)

I object to not being able to test common syntax (common syntax eg for send does not include a reference amount).

TL:DR; the whole point of pumping raw hex out instead of committing was to assist you guys with testing, so of course I'm very interested in what is best for you guys - if you can tell me what you don't like/object to about my original method perhaps we can work out a better way to come at this.

Thanks
Z

[msgilligan]

Sorry for not being clear.

I don't like the idea of a configuration option that requires a restart because the tests as currently written run against a single launch of the server and managing the configuration and restarts would be inconvenient. On that issue, I was pretty much echoing Dexx's point.

And my concern with and RPC that sets/toggles the return hex vs commit transaction is that it adds critical state to the server that could cause problems in production and that requires any tests to manage that state.

The reason I asked for hex output of the various transactions was as a reference for creating/debugging a Java implementation of transaction encoding. There are probably better ways of doing that -- such as using the data from the C++ unit tests.

I'm not sure that getting the hex transactions is that useful in what are supposed to be integration/functional tests anyway.

TL;DR I think we can leave the "return hex" mode entirely out of the RPCs -- unless and until we find a real need for it.

[zathras-crypto]

Thanks for the clarification :)

I don't like the idea of a configuration option that requires a restart because the tests as currently written run against a single launch of the server and managing the configuration and restarts would be inconvenient. On that issue, I was pretty much echoing Dexx's point.

Agreed & that had been dealt with via a new RPC command I added yesterday (setautocommit true|false) to enable flipping the flag during runtime.

And my concern with and RPC that sets/toggles the return hex vs commit transaction is that it adds critical state to the server that could cause problems in production and that requires any tests to manage that state.

Not sure I 100% understand your meaning here but that's OK - the idea was to lower risk (by opening up testing avenues) but if the consensus is that it introduces risk I'm comfortable leaving it out.

The reason I asked for hex output of the various transactions was as a reference for creating/debugging a Java implementation of transaction encoding. There are probably better ways of doing that -- such as using the data from the C++ unit tests.

Understood, thanks.

TL;DR I think we can leave the "return hex" mode entirely out of the RPCs -- unless and until we find a real need for it.

Noted. If it's no value in testing I'm not aware of any other uses for it.

[zathras-crypto]

@dexX7 sorry missed this bit:

We should get a working document for all transaction types. This document should list RPC commands and parameters, a short name, and a long, descriptive name. This is something I stumbled upon so many times, and I always ended up with slightly different results, whether it's during tests, the spec, or some documentation. And there is also the Omniwallet API. Maybe it's time to normalize it once and for all? :)

I would say for OmniCore at least the API documentation should be the primary doc? It'll need updating if my work gets merged in that's for sure :)

https://github.com/OmniLayer/omnicore/blob/omnicore-0.0.10/doc/apidocumentation.md

[zathras-crypto]

Closing pull request in favour of consolidated code. Will link new PR here when ready.