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

rawreissuance rpc #475

Merged
merged 8 commits into from Dec 17, 2018

Conversation

Projects
None yet
2 participants
@instagibbs
Collaborator

instagibbs commented Dec 3, 2018

Follows a similar transaction creation flow as #474 . Tests demonstrate a multi-sig spend of a reissuance token to issue additional assets.

Also includes a number of fixes I discovered along the way.

self.nodes[0].sendtoaddress(blind_addr, total_amount, "", "", True)
self.nodes[1].generate(1)

# Next we used convinience function for transaction flows that will work

This comment has been minimized.

@stevenroose

stevenroose Dec 3, 2018

Member

convenience*?

This comment has been minimized.

@instagibbs

instagibbs Dec 3, 2018

Collaborator

removed in other pr

# Next we used convinience function for transaction flows that will work

# Start with single issuance input, unblinded (makes 5 outputs for later larger issuances)
# NOTE: the "blind" argument must should for all entries for `blindrawtransaction` step

This comment has been minimized.

@stevenroose

stevenroose Dec 3, 2018

Member

This sentence doesn't seem to make sense to me.

This comment has been minimized.

@instagibbs

instagibbs Dec 3, 2018

Collaborator

removed in other PR

assert_equal(len(id_set), 1)

# Random contract string should again differ
issued_tx = self.nodes[2].rawissueasset(funded_tx, [{"asset_amount":1, "asset_address":nonblind_addr, "contract":"deadbeef"}])[0]["hex"]

This comment has been minimized.

@stevenroose

stevenroose Dec 3, 2018

Member

Reminder: contract value 32-bytes.

This comment has been minimized.

@instagibbs

instagibbs Dec 3, 2018

Collaborator

fixed in other PR, will rebased post-merge

self.nodes[0].generate(1)
assert_equal(self.nodes[0].gettransaction(tx_id)["confirmations"], 1)

# TODO test raw issuance for non-empty contract hash

This comment has been minimized.

@stevenroose

stevenroose Dec 3, 2018

Member

I think you did that.. Or do you mean raw REissuance with non-empty contract hash?

@instagibbs

This comment has been minimized.

Collaborator

instagibbs commented Dec 3, 2018

I'll rebase this PR post-merge of parent PR.

}

uint256 asset_blinder;
asset_blinder.SetHex(asset_blinder_str);

This comment has been minimized.

@instagibbs

instagibbs Dec 3, 2018

Collaborator

Can use ParseHashV for error handling and parsing of bitcoin hash-like objects.

}

uint256 entropy;
entropy.SetHex(entropy_str);

This comment has been minimized.

@instagibbs

instagibbs Dec 3, 2018

Collaborator

Can use ParseHashV for error handling and parsing of bitcoin hash-like objects.

if (!IsHex(contract_hash_str) || contract_hash_str.size() != 64) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Contract hash must be 64 character hex strings.");
}
contract_hash = uint256(ParseHex(contract_hash_str));

This comment has been minimized.

@instagibbs

instagibbs Dec 3, 2018

Collaborator

We should be using ParseHashV to make sure we're parsing it like a hash like everywhere else in Bitcoin Core and to do argument checking.

mtx.vin[issuance_input_index].assetIssuance.nAmount = asset_amount;

// Place assets into randomly placed output slots, before change output, inserted in place
assert(mtx.vout.size() >= 1);

This comment has been minimized.

@instagibbs

instagibbs Dec 4, 2018

Collaborator

need to validate user input in callee, or just return a useful error

@instagibbs instagibbs force-pushed the instagibbs:rawreissuance_rpc branch from b4ece6e to a1d6a91 Dec 12, 2018

@instagibbs

This comment has been minimized.

Collaborator

instagibbs commented Dec 12, 2018

rebased onto tip post #474 merge

@instagibbs

This comment has been minimized.

Collaborator

instagibbs commented Dec 12, 2018

READY FOR REVIEW

@stevenroose

Doc nit. API is a bit awkward with the amount of data that needs to be kept from the first issuance to the second, but I guess that's just how it is. I don't think it makes sense to try and improve the UX by bundling all that into a single blob. This is for advanced users only, I guess.

tACK a1d6a91

"rawreissueasset transaction {\"vin\":\"n\", \"asset_amount\":x.xxx, \"asset_address\":\"address\", \"asset_blinder\":<hex>, \"entropy\":<hex>, ( \"contract_hash\":<hex> )}\n"
"\nRe-issue an asset by attaching pseudo-inputs to transaction inputs, revealing the underlying reissuance token of the input. Returns the transaction hex.\n"
"\nArguments:\n"
"1. \"transaction\" (string, required) Transaction in hex in which to include a peg-in input.\n"

This comment has been minimized.

@stevenroose

stevenroose Dec 12, 2018

Member

You mean issuance here instead of pegin?

This comment has been minimized.

@instagibbs

instagibbs Dec 12, 2018

Collaborator

oops yes

@instagibbs

This comment has been minimized.

Collaborator

instagibbs commented Dec 12, 2018

It's pretty much impossible to carry around a blob, since as soon as you spend the initial output, you'll have to update it. Would be nice to have something more straight forward, but indeed this is for power users.

@instagibbs instagibbs force-pushed the instagibbs:rawreissuance_rpc branch from a1d6a91 to bf19b1a Dec 12, 2018

@instagibbs

This comment has been minimized.

Collaborator

instagibbs commented Dec 12, 2018

While adding the final TODO test, I ran into:

  1. A couple blinding bugs in the wallet where it was looking at the wrong field, or didn't push the right values when amounts were 0 in certain initial issuance cases. I had forgotten how some of this issuance consensus logic worked and wrote it wrong, and current tests weren't quite catching it.
  2. contract_hash for reissuance was unused. Heh. entropy field is enough to re-calculate everything in a reissuance. So the final test has a non-null contract test, though it ended up mostly testing (1) instead.

@instagibbs instagibbs force-pushed the instagibbs:rawreissuance_rpc branch from bf19b1a to 181d099 Dec 12, 2018

decode_tx = self.nodes[0].decoderawtransaction(issued_tx)
id_set.add(decode_tx["vin"][1]["issuance"]["asset"])
non_null_contract_token = decode_tx["vin"][1]["issuance"]["token"]

This comment has been minimized.

@stevenroose

stevenroose Dec 17, 2018

Member

I got confused you weren't using this variable and you got through the python linter. But forgot this is 0.14. In 0.17 the linter doesn't allow you to not use variables.

@stevenroose

This comment has been minimized.

Member

stevenroose commented Dec 17, 2018

retACK 181d099

That was a subtle change, good catch!

-    // Don't issue stuff or set values unless non-zero (both are against consensus)
+    // Explicit 0 is represented by a null value, don't set to non-null in that case
+    if (blind_issuance || asset_amount != 0) {
+        mtx.vin[issuance_input_index].assetIssuance.nAmount = asset_amount;
+    }
+    // Don't make zero value output(impossible by consensus)
     if (asset_amount > 0) {
         mtx.vout.insert(mtx.vout.begin()+asset_place, asset_out);
-        mtx.vin[issuance_input_index].assetIssuance.nAmount = asset_amount;
     }
@instagibbs

This comment has been minimized.

Collaborator

instagibbs commented Dec 17, 2018

removed the unused variable in the test

@instagibbs instagibbs merged commit bc257db into ElementsProject:elements-0.14.1 Dec 17, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

instagibbs added a commit that referenced this pull request Dec 17, 2018

Merge #475: rawreissuance rpc
bc257db f'rawreissueasset functional tests' (Gregory Sanders)
181d099 rawreissueasset functional tests (Gregory Sanders)
21cb8aa rawreissuance rpc (Gregory Sanders)
1251ecd fixup rawissueasset help comment (Gregory Sanders)
23371af add confidential_key explanation in validateaddress help (Gregory Sanders)
c579f4b Correct how wallet blinding logic detects which token id to use based on blinding keys (Gregory Sanders)
fff2dcc blindrawtransaction: Have blind_issuances parse correctly from command line (Gregory Sanders)
756bdc0 issueasset_base: Push 0-value value to issuance field if blinding (Gregory Sanders)

@instagibbs instagibbs referenced this pull request Dec 18, 2018

Open

raw issuance RPC #12

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