New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support creation of raw transactions with non-wallet inputs #77

Merged
merged 14 commits into from Oct 29, 2015

Conversation

Projects
None yet
2 participants
@dexX7
Copy link
Member

dexX7 commented Jun 8, 2015

Four new RPCs are added to create or modify raw transactions with non-wallet inputs:

  • add a payload with class B encoding (omni_createrawtx_multisig)
  • add a payload with class C encoding (omni_createrawtx_opreturn)
  • add a transaction input (omni_createrawtx_input)
  • add a reference output (omni_createrawtx_reference)
  • add a change output (omni_createrawtx_change)

See the JSON-RPC API documentation.

The core components are the new TxBuilder and OmniTxBuilder classes.

See the class documentation.

Five new parsing helpers are added:

  • ParseTransaction()
  • ParseMutableTransaction()
  • ParsePubKeyOrAddress()
  • ParseOutputIndex()
  • ParsePrevTxs()

The new classes and functions are unit tested (except RPCs).

@dexX7 dexX7 force-pushed the dexX7:wip-oc-0.10-raw-transactions branch to e8955d6 Jun 10, 2015

zathras-crypto added a commit that referenced this pull request Jun 25, 2015

Merge pull request #77 from dexX7/oc-0.10-ui-signal-lock
Don't notify about Omni events, while waiting

@dexX7 dexX7 force-pushed the dexX7:wip-oc-0.10-raw-transactions branch from e8955d6 Jul 16, 2015

@dexX7 dexX7 removed the status: on hold label Jul 22, 2015

@dexX7 dexX7 added the api label Sep 11, 2015

@dexX7 dexX7 force-pushed the dexX7:wip-oc-0.10-raw-transactions branch Oct 12, 2015

@dexX7 dexX7 removed the status: on hold label Oct 12, 2015

@dexX7 dexX7 force-pushed the dexX7:wip-oc-0.10-raw-transactions branch Oct 14, 2015

@dexX7 dexX7 force-pushed the dexX7:wip-oc-0.10-raw-transactions branch 3 times, most recently Oct 16, 2015

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Oct 17, 2015

@zathras-crypto: ready.

Please let me know, if there is anything to improve. :)

@dexX7 dexX7 force-pushed the dexX7:wip-oc-0.10-raw-transactions branch 2 times, most recently Oct 17, 2015

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Oct 22, 2015

I'd also like to add a call to modify inputs, or rather: add an input. This is something that could be used in the context of #11 (to add the payment input).

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Oct 22, 2015

@zathras-crypto: ready.

Sorry dude, missed the ping, will review again this weekend but I did play with this a little while back... Leave it with me :)

I'd also like to add a call to modify inputs, or rather: add an input. This is something that could be used in the context of #11 (to add the payment input).

I think this is fine, but perhaps it should be restricted to Class C only where sender identification is changed to first vin only and adding inputs doesn't risk changing the sender (when using largest input by sum).

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Oct 24, 2015

I think this is fine, but perhaps it should be restricted to Class C only where sender identification is changed to first vin only and adding inputs doesn't risk changing the sender (when using largest input by sum).

Hm.. the builder doesn't have an understanding about Omni transactions, or their senders. Transactions are built in steps, with something like addReference(), addOpReturn(), whereby each step is exposed via the API as stateless call.

It's very low level, but I'm all for better usability, though I currently don't see how. Initially I used a broader approach, where the whole transaction is created in one step, i.e. the user provides all information together, but this was awefully large and unflexible.

dexX7 added some commits Oct 10, 2015

dexX7 added some commits Oct 14, 2015

@dexX7 dexX7 force-pushed the dexX7:wip-oc-0.10-raw-transactions branch Oct 25, 2015

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Oct 25, 2015

I added ParseOutputIndex and omni_createrawtx_input.

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Oct 26, 2015

Yep, that makes sense & I like the way it's done now in 'pieces' rather than a single pass, it's definitely more flexible.

I guess if the user is crafting raw transactions they should be fully aware of what they are doing and protecting them against an unintended change of sender by adding extra inputs might be a bit too much babysitting.

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Oct 26, 2015

Ideally one would build a transaction, and then validate the results with "omni_decodetransaction".

@dexX7

View changes

src/omnicore/rpcrawtx.cpp Outdated
if (!DecodeHexTx(tx, params[0].get_str())) {
throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Transaction deserialization failed");
}
CTransaction tx = ParseMutableTransaction(params[0]);

This comment has been minimized.

@dexX7

dexX7 Oct 26, 2015

Member

Note to self: should be ParseTransaction, not ParseMutableTransaction.

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Oct 26, 2015

Ideally one would build a transaction, and then validate the results with "omni_decodetransaction"

Ahh of course!

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Oct 27, 2015

@zathras-crypto: did you have the chance to review?

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Oct 28, 2015

@zathras-crypto: did you have the chance to review?

I did yep and the code is great, basically what I was attempting to do was recreate existing Omni transactions via the raw interface (of course they'd be unsigned) and compare them.

I was going to loop back around and try and figure out where I went wrong as I ran into some trouble.

Here is an example of one of the transactions I built, but I'm unable to decode it via omni_decodetransaction - I haven't had a chance to dig into why as yet (looked OK from a quick look at it via deocderawtransaction):

010000000163da7c92a0165f68fa280ccdbaaf46d5f61da7640148b40f6c9588c0161e28d10000000000ffffffff033c0f00000000000067514104e6bf7e7f3b8e1b5cbdb7ef22e893583aba36edc63871e265493fd04bd59230da6bdcbd76887863a0294f1114e02328e46b4b48b0fc364aef80f7f831b39eb1f02102c7e8649fc477c0622f2cb0fb5b3dea02a4044958b76809b8e48ecbfaec2299a452aeaa0a0000000000001976a914946cb2e08075bcbaf157e47bcb67eb2b2339d24288acaa0a0000000000001976a914fa0692278afe508514b5ffee8fe5e97732ce066988ac00000000

Oh, I also forgot to mention - the documentation notes the rawtx argument is optional, but it's required. Perhaps this is just my interpretation, but I consider it a 'required' field if we have to include it (even empty). Eg if I don't include the field because it's optional the command fails omni_createrawtx_opreturn "payload", but add the field with an empty value omni_createrawtx_opreturn "" "payload" and it's fine hence (imo) the field is required and supports empty values. I hope that makes sense hehe :)

While on the subject, I did want to say the quality of your work is just really really professional (including commenting and documentation) - it really makes me realize that I need to "up my game" to bring my contributions up to the same standard/level of quality...

EDIT: I just did the same test transactions on another box with a fully synchronized node, I was able to decode successfully. That makes sense now I think about it... OK to merge :)

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Oct 28, 2015

Here is an example of one of the transactions I built, but I'm unable to decode it [...] EDIT: I just did the same test transactions on another box with a fully synchronized node

Ah, I see. Did you explicitly provide the inputs when using "omni_decodetransaction"? If not, I assume this is the cause. The following should work, even on an unsynchronized box (untested edit: tested, works):

omni_decodetransaction "010000000163da7c92a0165f68fa280ccdbaaf46d5f61da7640148b40f6c9588c0161e28d10000000000ffffffff033c0f00000000000067514104e6bf7e7f3b8e1b5cbdb7ef22e893583aba36edc63871e265493fd04bd59230da6bdcbd76887863a0294f1114e02328e46b4b48b0fc364aef80f7f831b39eb1f02102c7e8649fc477c0622f2cb0fb5b3dea02a4044958b76809b8e48ecbfaec2299a452aeaa0a0000000000001976a914946cb2e08075bcbaf157e47bcb67eb2b2339d24288acaa0a0000000000001976a914fa0692278afe508514b5ffee8fe5e97732ce066988ac00000000" '[{"txid":"d1281e16c088956c0fb4480164a71df6d546afbacd0c28fa685f16a0927cda63","vout":0,"scriptPubKey":"76a914ee2953140b43d86027ced62f566c46a7ed495e9e88ac","value":0.0005}]'

Oh, I also forgot to mention - the documentation notes the rawtx argument is optional, but it's required.

Good you noticed this. It's one of the points I was wondering about. Currently the following approach is used:

  • if there is an argument at the very end, which has a default value, and which can be omitted, then the argument is optional
  • if there is an argument at the beginning or inbetween, which has a default value, and which may be set to a value, which isn't strictly matching to the underlying data type, then I call it optional, too

Let me clarify:

The call "omni_createrawtx_opreturn" expects "rawtx" "payload", whereby "rawtx" refers to a transaction. An empty value (either "" or null) isn't a transaction, but accepted, and by labelling it "optional" I wanted to emphasize that providing a transaction is not strictly needed.

A similar approach was used for a few other calls, for example "omni_sendissuancecrowdsale" (see the docs).

But I see your point and I agree, this is indeed confusing, and one may be tempted to omit optional arguments completely, which is worse than calling it "required". I'm going to update and change "optional" to "required" for those calls. :)

While on the subject [...] it really makes me realize that I need to "up my game" to bring my contributions up to the same standard/level of quality...

Thanks, very appreciated! But no worries mate, it's a pleasure to work with you. :) While I believe we should always try to improve, content should be higher valued than style etc., which is only nice to have, but not mandatory.

As a general rule of thumb, I try to tackle it from a perspective of "what can I do to make it easier for a fellow developer, who may not be familiar with the codebase". In an ideal scenario pointing to the code or documentation should be sufficient and self-sustaining, without the need for additional explainations (and if there is need for explanations, then it may as well be documented).

dexX7 added some commits Oct 7, 2015

@dexX7 dexX7 force-pushed the dexX7:wip-oc-0.10-raw-transactions branch to c06c3cd Oct 28, 2015

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Oct 28, 2015

Updated.

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Oct 29, 2015

Did you explicitly provide the inputs when using "omni_decodetransaction"

Nope, I picked up on that afterwards - I thought "of course it doesn't work, how can we identify sender if the inputs are not in the chain?" and remembered you had provided the option to supply the inputs directly to omni_decodetransaction so no problems there :)

A similar approach was used for a few other calls, for example "omni_sendissuancecrowdsale"

Understood, but even though things like URL or DATA may be empty, the field is still required (and noted as 'required' on the API docs).

But I see your point and I agree, this is indeed confusing, and one may be tempted to omit optional arguments completely, which is worse than calling it "required". I'm going to update and change "optional" to "required" for those calls. :)

Thanks :) That was my main concern, that someone would be making the call with just one parameter thinking they didn't need to include the optional parameter and not getting far - I think the change is a good call :)

Thanks, very appreciated! But no worries mate...

You're too kind mate haha - any programming I did previously was to fill a need as part of my work in infrastructure (middleware etc) so the source never saw the light of day - I've got into some bad habits but every time I review your work I think "so polished! man my contributions have rough edges!" so if only for just taking pride in my own work I definitely think a little more effort in commenting and documenting would go a long way :)

Also I'm more than happy if you want to merge this in bud :)

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Oct 29, 2015

(and noted as 'required' on the API docs)

Just to clarify: this was updated with this PR, and it currently still shows "optional" on the main brach.

Also I'm more than happy if you want to merge this in bud :)

Yay, thanks! :)

@dexX7 dexX7 merged commit c06c3cd into OmniLayer:omnicore-0.0.10 Oct 29, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

dexX7 added a commit that referenced this pull request Oct 29, 2015

Merge pull request #77
c06c3cd Update RPC API documentation to underline required values (dexX7)
55d3b01 Update RPC help descriptions to underline required values (dexX7)
b1bcffe Store and restore minrelaytxfee in dust threshold unit tests (dexX7)
7cdc952 Update documentation for raw transaction related API (dexX7)
ff2cf9e Add transaction builder RPCs to RPC server (dexX7)
9ad3e99 Add transaction builder RPCs to RPC client (dexX7)
d993e01 Add RPCs to build raw Omni transactions (dexX7)
1f51b5c Update "omni_decodetransaction", use new helpers and fix example (dexX7)
8cd0c82 Add unit tests tests for raw transaction related parsing (dexX7)
fdc017c Add raw transaction related parsing helpers (dexX7)
e96dfe9 Add unit tests for transaction builder to Makefile (dexX7)
6f037e1 Add unit tests for transaction buidler (dexX7)
b4ffa61 Add transaction builder to Makefile (dexX7)
7595200 Add transaction builder (dexX7)

@dexX7 dexX7 modified the milestone: 0.0.10.0 Dec 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment