Skip to content
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

Transaction expiry height #2874

Merged
merged 5 commits into from Feb 24, 2018
Merged

Transaction expiry height #2874

merged 5 commits into from Feb 24, 2018

Conversation

@arcalinea
Copy link
Contributor

arcalinea commented Jan 16, 2018

Implements ZIP 203.

  • Only applies after Overwinter blockheight.

Closes #754.

@arcalinea arcalinea added this to the 1.0.15 milestone Jan 16, 2018
@arcalinea arcalinea self-assigned this Jan 16, 2018
@arcalinea arcalinea added this to Work Queue in Network Upgrade 0 via automation Jan 16, 2018
@arcalinea arcalinea requested review from bitcartel and str4d Jan 16, 2018
@arcalinea arcalinea force-pushed the arcalinea:tx_block_expiry branch from 1daac0b to 0d561c9 Jan 16, 2018
@str4d str4d moved this from Work Queue to In Progress in Network Upgrade 0 Jan 17, 2018
{
if (tx.nBlockExpiry <= nBlockHeight)
return true;
return false;

This comment has been minimized.

Copy link
@str4d

str4d Jan 24, 2018

Contributor

return tx.nBlockExpiry <= nBlockHeight;

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Jan 24, 2018

DEFAULT_TX_EXPIRY should perhaps be DEFAULT_TX_EXPIRY_DELTA to signify it is relative.

@arcalinea arcalinea force-pushed the arcalinea:tx_block_expiry branch from 503281e to 19ba94a Jan 24, 2018
@@ -410,6 +410,7 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += HelpMessageOpt("-salvagewallet", _("Attempt to recover private keys from a corrupt wallet.dat") + " " + _("on startup"));
strUsage += HelpMessageOpt("-sendfreetransactions", strprintf(_("Send transactions as zero-fee transactions if possible (default: %u)"), 0));
strUsage += HelpMessageOpt("-spendzeroconfchange", strprintf(_("Spend unconfirmed change when sending transactions (default: %u)"), 1));
strUsage += HelpMessageOpt("-txblockexpiry", strprintf(_("Set the number of blocks after which an unconfirmed transaction expires from the mempool (default: %u)"), DEFAULT_TX_EXPIRY_DELTA));

This comment has been minimized.

Copy link
@str4d

str4d Jan 25, 2018

Contributor

The current message sounds more like a config option for #2384 than transaction expiry. Maybe something like Set the number of blocks after which a created transaction will become invalid if it has not been mined?

@lindanlee thoughts on user messaging?

This comment has been minimized.

Copy link
@zmanian

zmanian Jan 25, 2018

Set the maximum number of blocks within which this transactions must either confirmed or resent

This comment has been minimized.

Copy link
@arcalinea

arcalinea Jan 25, 2018

Author Contributor

Set the maximum number of blocks within which transactions must be resent if they have not been confirmed ?

@@ -3105,6 +3114,13 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIn
}
}

// Check that all transactions are unexpired
BOOST_FOREACH(const CTransaction& tx, block.vtx) {
if (!IsExpiredTx(tx, nHeight)) {

This comment has been minimized.

Copy link
@str4d

str4d Jan 25, 2018

Contributor

if (IsExpiredTx(tx, nHeight)) {

@@ -356,6 +357,7 @@ class CTransaction
READWRITE(*const_cast<std::vector<CTxIn>*>(&vin));
READWRITE(*const_cast<std::vector<CTxOut>*>(&vout));
READWRITE(*const_cast<uint32_t*>(&nLockTime));
// READWRITE(*const_cast<uint32_t*>(&nBlockExpiry));

This comment has been minimized.

Copy link
@str4d

str4d Jan 25, 2018

Contributor

Needs to match ZIP (ie. both in position within serialization, and being conditional on version).

list<CTransaction> removed;
remove(tx, removed, true);
}
}

This comment has been minimized.

Copy link
@str4d

str4d Jan 25, 2018

Contributor

Indentation of this function should match rest of the file.

@@ -2546,6 +2548,9 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
// all.
txNew.nLockTime = std::max(0, chainActive.Height() - 10);

// Set nBlockExpiry to 20 blocks (default) past current blockheight
txNew.nBlockExpiry = chainActive.Height() + nBlockExpiry;

This comment has been minimized.

Copy link
@str4d

str4d Jan 25, 2018

Contributor

Should rename nBlockExpiry to not be semantically confused with txNew.nBlockExpiry.

This comment has been minimized.

Copy link
@arcalinea

arcalinea Jan 25, 2018

Author Contributor

blockExpiry?

@@ -430,6 +433,7 @@ struct CMutableTransaction
READWRITE(vin);
READWRITE(vout);
READWRITE(nLockTime);
READWRITE(nBlockExpiry);

This comment has been minimized.

Copy link
@str4d

str4d Jan 25, 2018

Contributor

Ditto.

@@ -999,6 +1000,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
mapArgs["-maxtxfee"], ::minRelayTxFee.ToString()));
}
}
nBlockExpiry = GetArg("-txblockexpiry", DEFAULT_TX_EXPIRY_DELTA);

This comment has been minimized.

Copy link
@str4d

str4d Jan 25, 2018

Contributor

Is -txblockexpiry clear enough on its own, or should it indicate somehow that it is a relative value?

This comment has been minimized.

Copy link
@arcalinea

arcalinea Jan 25, 2018

Author Contributor

Thought @ebfull 's suggestion of changing constant's name to include DELTA helps. Can add a comment above

@str4d str4d added this to 1.0.15: Consensus / Node Rules in Release planning Jan 26, 2018
@@ -346,6 +351,8 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
txNew.vout.resize(1);
txNew.vout[0].scriptPubKey = scriptPubKeyIn;
txNew.vout[0].nValue = GetBlockSubsidy(nHeight, chainparams.GetConsensus());
// Set coinbase expiry to current block height
txNew.nBlockExpiry = nHeight;

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

Don't we want to set it to the height after this block? Then nBlockExpiry is semantically accurate for coinbase transactions.

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

Also, this may affect getblocktemplate, and how mining pools update their miners when a new block arrives. With the above, we wouldn't have semantic accuracy if a mining pool reused a coinbase across several block heights, which they are able to do currently IIRC. It won't affect consensus rules, since you are excluding coinbase transactions from the expiry check, but maybe it's something we want to care about? Or maybe not, and we're fine with this information being stored on the blockchain (when the coinbase was generated).

This comment has been minimized.

Copy link
@arcalinea

arcalinea Feb 2, 2018

Author Contributor

I had it set to nBlockExpiry + 1, then changed it back, thinking a coinbase should only be valid for a single block? Was unaware of this behavior of pools. Not sure what the best way to handle Coinbase txs is, did not include this detail in the ZIP for feedback (will go add it now).

This comment has been minimized.

Copy link
@str4d

str4d Feb 5, 2018

Contributor

Ah, I see the confusion. As-defined in your ZIP, nBlockExpiry is the last height at which the transaction is valid. But in your IsExpired() function, nBlockExpiry is used as the first height at which the transaction expires, and thus nBlockExpiry = nHeight semantically would mean that the coinbase transaction is just-expired. We should fix one or the other.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 5, 2018

Contributor

Good point that this could be a hassle for mining pools.

Since coinbase txs are already special, could specify that the expiry height is ignored for coinbase transactions.

We might want to specify that this field is set to no expiry... or say that it is a reserved field for now and should not used be for anything.

That gives us 32 bytes which could be used at a later day... maybe for a merkle root as part of a malleability fix.

This comment has been minimized.

Copy link
@arcalinea

arcalinea Feb 6, 2018

Author Contributor

@str4d Ah I see, I will change the implementation to match the ZIP since that's what has been discussed.

@bitcartel You mean set the expiry height on coinbase txs to a null value?

@@ -50,7 +50,7 @@ double GetDifficultyINTERNAL(const CBlockIndex* blockindex, bool networkDifficul
int nShiftAmount = (powLimit >> 24) & 0xff;

double dDiff =
(double)(powLimit & 0x00ffffff) /
(double)(powLimit & 0x00ffffff) /

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

Exclude this whitespace change from the PR. I'm not gonna block on the rest of them, as I'm guessing your editor is automatically stripping them, but this file is otherwise unaltered.

This comment has been minimized.

Copy link
@arcalinea

arcalinea Feb 2, 2018

Author Contributor

Yeah, think it's my editor.

@@ -2546,6 +2548,9 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
// all.
txNew.nLockTime = std::max(0, chainActive.Height() - 10);

// Set nBlockExpiry to 20 blocks (default) past current blockheight
txNew.nBlockExpiry = chainActive.Height() + blockExpiry;

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

We should set it to the minimum of this and the next activation height, so that transactions drop out of the mempool automatically on the pre-upgrade branch if they aren't mined before activation. I'll add an API to #2898 for fetching the latter.

@@ -300,6 +300,10 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
if (!ContextualCheckInputs(tx, state, view, true, MANDATORY_SCRIPT_VERIFY_FLAGS, true, Params().GetConsensus()))
continue;

// Check to make sure txs have not expired
if (IsExpiredTx(tx, nHeight))

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

Put this check in the first loop, in the same conditional as IsCoinbase() and !IsFinalTx(). That way, we don't go to the trouble of reading into memory all the inputs of expired transactions.

src/main.h Outdated
@@ -350,6 +350,12 @@ bool IsStandardTx(const CTransaction& tx, std::string& reason);
*/
bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime);

/**
* Check if transaction is expired and can be included in a block with the
* specified height and time. Consensus critical.

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

Nice comment 😄

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 20, 2018

Contributor

Remove "and time" - we're only doing height.

// Check that all transactions are unexpired
BOOST_FOREACH(const CTransaction& tx, block.vtx) {
if (!tx.IsCoinBase() && IsExpiredTx(tx, nHeight)) {
return state.DoS(10, error("%s: contains an expired transaction", __func__), REJECT_INVALID, "bad-txns-expired");

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

This should be DoS(100, ...), because we are not defining expiry by time, so miners can be unequivocally held accountable for including an expired transaction.

@@ -3105,6 +3114,13 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIn
}
}

// Check that all transactions are unexpired
BOOST_FOREACH(const CTransaction& tx, block.vtx) {
if (!tx.IsCoinBase() && IsExpiredTx(tx, nHeight)) {

This comment has been minimized.

Copy link
@str4d

str4d Feb 2, 2018

Contributor

If IsExpired() contains the coinbase check, no need to include it here. If we decide it won't, then we'd also need to decide here (and correspondingly handle the fact that coinbase can expire in getblocktemplate).

This comment has been minimized.

Copy link
@arcalinea

arcalinea Feb 2, 2018

Author Contributor

Are there any potential negative consequences if coinbase txs are excluded from tx expiry checks? If not, then I'd say we should keep that check in IsExpired() for clarity.

(I put this redundant check here during my debugging process, and forgot to remove.)

This comment has been minimized.

Copy link
@str4d

str4d Feb 5, 2018

Contributor

None that I can think of...

@arcalinea arcalinea changed the title WIP tx block expiry [WIP] Tx block expiry Feb 6, 2018
@@ -337,6 +341,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)

nLastBlockTx = nBlockTx;
nLastBlockSize = nBlockSize;
LogPrintf("CreateNewBlock(): number of txs in block %u\n", nBlockTx);

This comment has been minimized.

Copy link
@str4d

str4d Feb 10, 2018

Contributor

Non-blocking: Probably doesn't need to be in this PR.

This comment has been minimized.

Copy link
@arcalinea

arcalinea Feb 13, 2018

Author Contributor

Will remove

@@ -3105,6 +3112,13 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIn
}
}

// Check that all transactions are unexpired
BOOST_FOREACH(const CTransaction& tx, block.vtx) {
if (IsExpiredTx(tx, nHeight)) {

This comment has been minimized.

Copy link
@str4d

str4d Feb 10, 2018

Contributor

Does this work for pre-Overwinter transactions? I assume it would add a consensus dependency on the specific default value of nExpiryHeight. Might be useful to gate it behind NetworkUpgradeActive(..., Consensus::UPGRADE_OVERWINTER), in which case you might just want to move it into the new ContextualCheckTransaction function @bitcartel is adding to #2925 (once you rebase on top of it).

This comment has been minimized.

Copy link
@str4d

str4d Feb 10, 2018

Contributor

(Noting that we'd want a lower DoS value when ContextualCheckTransaction is called from AcceptToMemoryPool.)

This comment has been minimized.

Copy link
@arcalinea

arcalinea Feb 13, 2018

Author Contributor

Moving to new ContextualCheckTransaction function

@@ -133,7 +133,7 @@ def run_test(self):
inputs = [{ "txid" : txId, "vout" : vout['n'], "scriptPubKey" : vout['scriptPubKey']['hex']}]
outputs = { self.nodes[0].getnewaddress() : 2.199 }
rawTx = self.nodes[2].createrawtransaction(inputs, outputs)
rawTxPartialSigned = self.nodes[1].signrawtransaction(rawTx, inputs)
rawTxPartialSigned = self.nodes[1].signrawtransaction(rawTx)

This comment has been minimized.

Copy link
@str4d

str4d Feb 10, 2018

Contributor

Revert all changes in this file.

static void MutateTxExpiry(CMutableTransaction& tx, const string& cmdVal)
{
int64_t newExpiry = atoi64(cmdVal);
if (newExpiry > 500000000) {

This comment has been minimized.

Copy link
@str4d

str4d Feb 10, 2018

Contributor

Do we have access to the constant here? If not, we should probably move the constant somewhere it can be accessed.

This comment has been minimized.

Copy link
@arcalinea

arcalinea Feb 13, 2018

Author Contributor

Will use the constant TX_EXPIRY_HEIGHT_THRESHOLD @bitcartel added once rebased

if (tx.nExpiryHeight > CTransaction::MAX_EXPIRY_HEIGHT) {
reason = "expiry-height";
return false;
}

This comment has been minimized.

Copy link
@str4d

str4d Feb 10, 2018

Contributor

Why check the expiry height here? It is being implemented as a consensus rule, not a standard rule.

static const int32_t MIN_CURRENT_VERSION = 1;
static const int32_t MAX_CURRENT_VERSION = 2;
static const int32_t MAX_CURRENT_VERSION = 3;

This comment has been minimized.

Copy link
@str4d

str4d Feb 10, 2018

Contributor

This will conflict with #2906 (as will the rest of the changes in this file).

transactionsToRemove.push_back(tx);
}
}
BOOST_FOREACH(const CTransaction& tx, transactionsToRemove) {

This comment has been minimized.

Copy link
@str4d

str4d Feb 10, 2018

Contributor

for (const CTransaction& tx : transactionsToRemove) { (since we have C++11).

// Set nExpiryHeight to 20 blocks (default) past current blockheight
// nVersion of Overwinter transactions that expire need to be version 3
txNew.nVersion = 3;
if (chainActive.Height() + blockExpiry > 500000000){

This comment has been minimized.

Copy link
@str4d

str4d Feb 10, 2018

Contributor

Use MAX_EXPIRY_HEIGHT.

static const int32_t MAX_CURRENT_VERSION = 3;

// The max threshold for block expiry is 500000000
static const int32_t MAX_EXPIRY_HEIGHT = 500000000;

This comment has been minimized.

Copy link
@str4d

str4d Feb 10, 2018

Contributor

This should probably be in src/consensus/consensus.h.

@@ -2546,6 +2548,16 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
// all.
txNew.nLockTime = std::max(0, chainActive.Height() - 10);

// Set nExpiryHeight to 20 blocks (default) past current blockheight
// nVersion of Overwinter transactions that expire need to be version 3
txNew.nVersion = 3;

This comment has been minimized.

Copy link
@str4d

str4d Feb 10, 2018

Contributor

We can't do this before Overwinter activates, so gate this entire block in a NetworkUpgradeActive check. Also, I expect the txNew.nVersion = 3 here will become redundant, as #2906 itself will need to update the wallet code to set this.

@str4d str4d changed the title [WIP] Tx block expiry [WIP] Transaction expiry height Feb 10, 2018
@arcalinea arcalinea force-pushed the arcalinea:tx_block_expiry branch from 8666ef8 to e6ff12b Feb 13, 2018
@arcalinea arcalinea requested a review from str4d Feb 13, 2018
"t2ECCQPVcxUCSSQopdNquguEPE14HsVfcUn", "t2JabDUkG8TaqVKYfqDJ3rqkVdHKp6hwXvG", "t2FGzW5Zdc8Cy98ZKmRygsVGi6oKcmYir9n", "t2DUD8a21FtEFn42oVLp5NGbogY13uyjy9t",
"t2UjVSd3zheHPgAkuX8WQW2CiC9xHQ8EvWp", "t2TBUAhELyHUn8i6SXYsXz5Lmy7kDzA1uT5", "t2Tz3uCyhP6eizUWDc3bGH7XUC9GQsEyQNc", "t2NysJSZtLwMLWEJ6MH3BsxRh6h27mNcsSy",
"t2KXJVVyyrjVxxSeazbY9ksGyft4qsXUNm9", "t2J9YYtH31cveiLZzjaE4AcuwVho6qjTNzp", "t2QgvW4sP9zaGpPMH1GRzy7cpydmuRfB4AZ", "t2NDTJP9MosKpyFPHJmfjc5pGCvAU58XGa4",
"t29pHDBWq7qN4EjwSEHg8wEqYe9pkmVrtRP", "t2Ez9KM8VJLuArcxuEkNRAkhNvidKkzXcjJ", "t2D5y7J5fpXajLbGrMBQkFg2mFN8fo3n8cX", "t2UV2wr1PTaUiybpkV3FdSdGxUJeZdZztyt",

This comment has been minimized.

Copy link
@str4d

str4d Feb 15, 2018

Contributor

Revert all the whitespace changes in this file.

assert_equal(rawTxPartialSigned['complete'], False) # node1 only has one key, can't comp. sign the tx

assert_equal(rawTxPartialSigned['complete'], False) #node1 only has one key, can't comp. sign the tx

This comment has been minimized.

Copy link
@str4d

str4d Feb 15, 2018

Contributor

The changes in this file all seem to be reversions. What's going on?

@@ -5,6 +5,7 @@
#include "main.h"
#include "primitives/transaction.h"
#include "consensus/validation.h"
#include "consensus/consensus.h"

This comment has been minimized.

Copy link
@str4d

str4d Feb 15, 2018

Contributor

Why is this include here? If it isn't needed, please revert all changes to this file.

This comment has been minimized.

Copy link
@arcalinea

arcalinea Feb 16, 2018

Author Contributor

This must have been something from the rebase, I don't recall changing this file.

@@ -889,6 +897,11 @@ bool ContextualCheckTransaction(const CTransaction& tx, CValidationState &state,
REJECT_INVALID, "tx-overwinter-version-too-high");
}

// Check that all transactions are unexpired

This comment has been minimized.

Copy link
@str4d

str4d Feb 15, 2018

Contributor

Check that transaction is unexpired

if tx1 in txs:
print "Tx mined in block"
else:
print "Tx not in block"

This comment has been minimized.

Copy link
@str4d

str4d Feb 15, 2018

Contributor

I can't tell whether this is expecting the transaction to be mined or dropped.

This comment has been minimized.

Copy link
@arcalinea

arcalinea Feb 15, 2018

Author Contributor

This test has been rewritten

@arcalinea arcalinea force-pushed the arcalinea:tx_block_expiry branch 2 times, most recently from 820e979 to bba48a1 Feb 15, 2018
@arcalinea

This comment has been minimized.

Copy link
Contributor Author

arcalinea commented Feb 16, 2018

mempool_tx_expiry.py test is finally passing correctly for transparent and shielded txs.

Still need to add checks on balances displayed to user after shielded tx expiry, as @bitcartel suggested.

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 16, 2018

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

@arcalinea arcalinea force-pushed the arcalinea:tx_block_expiry branch 4 times, most recently from 68e6801 to e57352d Feb 16, 2018
@arcalinea arcalinea force-pushed the arcalinea:tx_block_expiry branch from d7b59d8 to 5943f22 Feb 23, 2018
@arcalinea

This comment has been minimized.

Copy link
Contributor Author

arcalinea commented Feb 23, 2018

Yeah, think the config flag is fine to set without an error, there's no other way to interact with the expiry height via the RPC

@str4d
str4d approved these changes Feb 23, 2018
Copy link
Contributor

str4d left a comment

utACK

Copy link
Contributor

daira left a comment

The missing return false; needs to be fixed.

The intended behaviour for coinbase needs to be specified in ZIP 203. I'm fine with the current implemented behaviour (ignore nExpiryHeight for coinbase), but there needs to be a positive decision to do that.

// Set nExpiryHeight to expiryDelta (default 20) blocks past current block height
if (NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER)) {
if (nextBlockHeight + expiryDelta >= TX_EXPIRY_HEIGHT_THRESHOLD){
strFailReason = _("nExpiryHeight must be less than TX_EXPIRY_HEIGHT_THRESHOLD.");

This comment has been minimized.

Copy link
@daira

daira Feb 23, 2018

Contributor

Need to return false; here.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 23, 2018

Contributor

Yes, daira is right, need to return false.

This comment has been minimized.

Copy link
@arcalinea

arcalinea Feb 23, 2018

Author Contributor

Added

Jay Graber
@arcalinea arcalinea force-pushed the arcalinea:tx_block_expiry branch from cec8b09 to 7b92f27 Feb 23, 2018
@arcalinea

This comment has been minimized.

Copy link
Contributor Author

arcalinea commented Feb 23, 2018

@daira I added a coinbase rule to the ZIP: zcash/zips@11d243e

Copy link
Contributor

bitcartel left a comment

Ran test, ran my own regtest createrawtransaction tests, and reviewed... Some minor non-blockers, but one fix required as spotted by daira.

@@ -414,6 +414,7 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += HelpMessageOpt("-sendfreetransactions", strprintf(_("Send transactions as zero-fee transactions if possible (default: %u)"), 0));
strUsage += HelpMessageOpt("-spendzeroconfchange", strprintf(_("Spend unconfirmed change when sending transactions (default: %u)"), 1));
strUsage += HelpMessageOpt("-txconfirmtarget=<n>", strprintf(_("If paytxfee is not set, include enough fee so transactions begin confirmation on average within n blocks (default: %u)"), DEFAULT_TX_CONFIRM_TARGET));
strUsage += HelpMessageOpt("-txexpirydelta", strprintf(_("Set the number of blocks after which a transaction that has not been mined will become invalid (default: %u)"), DEFAULT_TX_EXPIRY_DELTA));

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 23, 2018

Contributor

Non-blocking but... but the wording can be confusing. Here's what I think you want to say (but obviously in a less verbose fashion). "After a transaction is created, the number of blocks during when the transaction is expected to be mined, and if not, the transaction expires and becomes invalid"

This comment has been minimized.

Copy link
@tarrenj

tarrenj Jun 2, 2018

"Set the amount of time, in blocks, that a transaction is minable, after which the transaction becomes invalid."?

@@ -22,6 +22,8 @@ static const unsigned int MAX_BLOCK_SIGOPS = 20000;
static const unsigned int MAX_TX_SIZE = 100000;
/** Coinbase transaction outputs can only be spent after this number of new blocks (network rule) */
static const int COINBASE_MATURITY = 100;
/** The minimum value which is invalid for expiry height, used by CTransaction and CMutableTransaction */

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 23, 2018

Contributor

Non-blocking but I had to read this a few times to grok. Maybe something like this is easier to read: " A valid expiry height must be lower than this threshold value, used by ..."

@@ -342,6 +342,8 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
txNew.vout.resize(1);
txNew.vout[0].scriptPubKey = scriptPubKeyIn;
txNew.vout[0].nValue = GetBlockSubsidy(nHeight, chainparams.GetConsensus());
// Set to 0 so expiry height does not apply to coinbase txs
txNew.nExpiryHeight = 0;

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 23, 2018

Contributor

Probably best in the ZIP to say that expiry height does not apply to coinbase transactions, rather than saying the value must be zero, as it then gives us a field we can re-use in a future ZIP e.g. stashing a merkle root in the expiry height field of a coinbase transaction.

// Set nExpiryHeight to expiryDelta (default 20) blocks past current block height
if (NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER)) {
if (nextBlockHeight + expiryDelta >= TX_EXPIRY_HEIGHT_THRESHOLD){
strFailReason = _("nExpiryHeight must be less than TX_EXPIRY_HEIGHT_THRESHOLD.");

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 23, 2018

Contributor

Yes, daira is right, need to return false.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Feb 23, 2018

Regarding the coinbase rule, see my comment #2874 (comment) about not specifying it as zero, but just ignoring it. You could say that it is good practice (i.e. convention) to set to zero. Otherwise, in future, if there is a ZIP for "stash merkle root of some data into expiry height of coinbase tx", it would conflict with the zero rule and the expiry ZIP would have to be updated/superceded.

@str4d
str4d approved these changes Feb 23, 2018
Copy link
Contributor

str4d left a comment

re-utACK

return start_nodes(4, self.options.tmpdir, [["-nuparams=5ba81b19:205", "-txexpirydelta=4"]] * 4)

# Test before, at, and after expiry block
# TODO: Test case of dependent txs in reorgs

This comment has been minimized.

Copy link
@str4d

str4d Feb 23, 2018

Contributor

Do we have a ticket filed for this? If not, please file one.

This comment has been minimized.

Copy link
@arcalinea

arcalinea Feb 24, 2018

Author Contributor

Created #2983

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Feb 23, 2018

@bitcartel it would just mean that to use the field for that we'd need to do a network upgrade, in which case we might just add a real field for it properly. I'm fine with leaving it as a by-convention zero field for now, like hashReserved.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 23, 2018

📌 Commit 7b92f27 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 23, 2018

⌛️ Testing commit 7b92f27 with merge ac290b3...

zkbot added a commit that referenced this pull request Feb 23, 2018
Transaction expiry height

Implements ZIP 203.

- Only applies after Overwinter blockheight.

Closes #754.
@arcalinea

This comment has been minimized.

Copy link
Contributor Author

arcalinea commented Feb 24, 2018

Changed wording of ZIP to set coinbase nExpiryHeight to 0 by convention: zcash/zips@da67f85

@daira
daira approved these changes Feb 24, 2018
Copy link
Contributor

daira left a comment

ut(ACK+cov)

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 24, 2018

💔 Test failed - pr-merge

Jay Graber
@arcalinea

This comment has been minimized.

Copy link
Contributor Author

arcalinea commented Feb 24, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 24, 2018

📌 Commit 59da58c has been approved by arcalinea

@str4d
str4d approved these changes Feb 24, 2018
Copy link
Contributor

str4d left a comment

utACK 59da58c

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 24, 2018

⌛️ Testing commit 59da58c with merge a418756...

zkbot added a commit that referenced this pull request Feb 24, 2018
Transaction expiry height

Implements ZIP 203.

- Only applies after Overwinter blockheight.

Closes #754.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 24, 2018

☀️ Test successful - pr-merge
Approved by: arcalinea
Pushing a418756 to master...

@zkbot zkbot merged commit 59da58c into zcash:master Feb 24, 2018
1 check passed
1 check passed
homu Test successful
Details
Network Upgrade 0 automation moved this from In Progress to Complete Feb 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Network Upgrade 0
  
Complete
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.