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

Take expiryheight as param to createrawtransaction #3336

Merged
merged 3 commits into from Jul 30, 2018

Conversation

@arcalinea
Copy link
Contributor

arcalinea commented Jun 14, 2018

Addresses #3333

@arcalinea arcalinea self-assigned this Jun 14, 2018

@arcalinea arcalinea added this to the v2.0.0 milestone Jul 3, 2018

@arcalinea arcalinea requested review from bitcartel and str4d Jul 3, 2018

@daira
Copy link
Contributor

daira left a comment

Some minor changes needed.

@@ -410,7 +410,7 @@ UniValue verifytxoutproof(const UniValue& params, bool fHelp)

UniValue createrawtransaction(const UniValue& params, bool fHelp)
{
if (fHelp || params.size() < 2 || params.size() > 3)
if (fHelp || params.size() < 2 || params.size() > 4)
throw runtime_error(
"createrawtransaction [{\"txid\":\"id\",\"vout\":n},...] {\"address\":amount,...} ( locktime )\n"

This comment has been minimized.

@daira

daira Jul 12, 2018

Contributor

Add expiryheight to the synopsis.

if (params.size() > 3 && !params[3].isNull()) {
int64_t nExpiryHeight = params[3].get_int64();
if (nExpiryHeight < 0 || nExpiryHeight >= TX_EXPIRY_HEIGHT_THRESHOLD) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, nExpiryHeight must be less than TX_EXPIRY_HEIGHT_THRESHOLD.");

This comment has been minimized.

@daira

daira Jul 12, 2018

Contributor

s/nExpiryHeight/expiryheight/ (to match the help text).

This comment has been minimized.

@daira

daira Jul 12, 2018

Contributor

"nonnegative and less than TX_EXPIRY_HEIGHT_THRESHOLD"

@@ -434,6 +434,7 @@ UniValue createrawtransaction(const UniValue& params, bool fHelp)
" ,...\n"
" }\n"
"3. locktime (numeric, optional, default=0) Raw locktime. Non-0 value also locktime-activates inputs\n"
"4. expiryheight (numeric, optional, default=0) Expiry height of transaction\n"

This comment has been minimized.

@daira

daira Jul 12, 2018

Contributor

If I were being pedantic I would suggest adding "(if Overwinter is active)", but since it already is on mainnet, that isn't needed.


if (params.size() > 2 && !params[2].isNull()) {
int64_t nLockTime = params[2].get_int64();
if (nLockTime < 0 || nLockTime > std::numeric_limits<uint32_t>::max())
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, locktime out of range");
rawTx.nLockTime = nLockTime;
}

if (NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER)) {

This comment has been minimized.

@daira

daira Jul 12, 2018

Contributor

If Overwinter is not active then expiryheight will be ignored. I guess that doesn't matter.

This comment has been minimized.

@bitcartel

bitcartel Jul 21, 2018

Contributor

If Overwinter is not active and an expiry height is passed in, let's throw a JSONRPCError so the caller knows that the expiry height is an invalid parameter.

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 18, 2018

☔️ The latest upstream changes (presumably #3350) made this pull request unmergeable. Please resolve the merge conflicts.


if (params.size() > 2 && !params[2].isNull()) {
int64_t nLockTime = params[2].get_int64();
if (nLockTime < 0 || nLockTime > std::numeric_limits<uint32_t>::max())
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, locktime out of range");
rawTx.nLockTime = nLockTime;
}

if (NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER)) {

This comment has been minimized.

@bitcartel

bitcartel Jul 21, 2018

Contributor

If Overwinter is not active and an expiry height is passed in, let's throw a JSONRPCError so the caller knows that the expiry height is an invalid parameter.

Jay Graber and others added some commits Jun 14, 2018

@bitcartel bitcartel force-pushed the arcalinea:expiryheight-createrawtransaction branch from 93f48aa to 3006784 Jul 30, 2018

I have usurped this PR so cannot self-ACK

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jul 30, 2018

@daira @arcalinea I have force pushed to update this PR.

@str4d str4d added this to In Review in Zcashd Team Jul 30, 2018

@str4d

str4d approved these changes Jul 30, 2018

Copy link
Contributor

str4d left a comment

ut(ACK+cov). Comments are non-blocking.

throw runtime_error(
"createrawtransaction [{\"txid\":\"id\",\"vout\":n},...] {\"address\":amount,...} ( locktime )\n"
"createrawtransaction [{\"txid\":\"id\",\"vout\":n},...] {\"address\":amount,...} ( locktime ) ( expiryheight )\n"

This comment has been minimized.

@str4d

str4d Jul 30, 2018

Contributor

In Bitcoin Core, this positional argument is the boolean replaceable, allowing BIP 125 to be configured (which enables a transaction to be later replaced by one with higher fees). Given that we are highly unlikely to ever bring in BIP 125 (both because it creates a distinguisher wrt fees, and we have transaction expiry which we are configuring with the same positional), I'm content with our addition here.

@@ -453,19 +454,25 @@ UniValue createrawtransaction(const UniValue& params, bool fHelp)
int nextBlockHeight = chainActive.Height() + 1;
CMutableTransaction rawTx = CreateNewContextualCMutableTransaction(
Params().GetConsensus(), nextBlockHeight);

if (NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER)) {
if (rawTx.nExpiryHeight >= TX_EXPIRY_HEIGHT_THRESHOLD){

This comment has been minimized.

@str4d

str4d Jul 30, 2018

Contributor

Technically this means we are no longer checking here that the output of CreateNewContextualCMutableTransaction() is inside the threshold, but that's probably fine (we should be checking this in a unit test anyway).

try:
self.nodes[0].createrawtransaction([], {}, 0, 499999999)
except JSONRPCException,e:
assert(False)

This comment has been minimized.

@str4d

str4d Jul 30, 2018

Contributor

If we aren't expecting a failure, the we shouldn't catch and rethrow, otherwise we won't see the error message in the output (we would just see the assertion failure).

This comment has been minimized.

@bitcartel

bitcartel Jul 30, 2018

Contributor

The line number of the assertion will be shown in the output so should be ok. Anyway, I've force-pushed an update so that the error message will appear.

@bitcartel bitcartel force-pushed the arcalinea:expiryheight-createrawtransaction branch from 3006784 to f01c11b Jul 30, 2018

@ebfull

ebfull approved these changes Jul 30, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jul 30, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 30, 2018

📌 Commit f01c11b has been approved by bitcartel

zkbot added a commit that referenced this pull request Jul 30, 2018

Auto merge of #3336 - arcalinea:expiryheight-createrawtransaction, r=…
…bitcartel

Take expiryheight as param to createrawtransaction

Addresses #3333
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 30, 2018

⌛️ Testing commit f01c11b with merge a11e6aa...

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 30, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing a11e6aa to master...

@zkbot zkbot merged commit f01c11b into zcash:master Jul 30, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from In Review to Released (Merged in Master) Jul 30, 2018

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