From 0cfc4b7d2699a544e7c5b7531420c147a56aa8a6 Mon Sep 17 00:00:00 2001 From: Malcolm MacLeod Date: Tue, 31 Mar 2020 19:18:21 +0200 Subject: [PATCH] CORE: Ensure renewal is working properly There were some regression/issues with renewal of accounts that expired in phase 3 when trying to renew in phase4 --- src/Gulden/rpcgulden.cpp | 9 ++++++--- src/policy/policy.cpp | 2 +- src/qt/_Gulden/witnessdialog.cpp | 5 +++-- src/script/sign.cpp | 9 +++++++++ src/script/sign.h | 3 ++- src/wallet/wallet.h | 4 ++-- src/wallet/wallet_transaction.cpp | 22 ++++++++++++++++------ src/wallet/witness_operations.cpp | 18 +++++++++--------- 8 files changed, 48 insertions(+), 24 deletions(-) diff --git a/src/Gulden/rpcgulden.cpp b/src/Gulden/rpcgulden.cpp index 5beb14444e..46bea6b6e5 100644 --- a/src/Gulden/rpcgulden.cpp +++ b/src/Gulden/rpcgulden.cpp @@ -2664,7 +2664,7 @@ static UniValue renewwitnessaccount(const JSONRPCRequest& request) if (request.fHelp || request.params.size() != 2) throw std::runtime_error( - "renewwitnessaccount \"witness_account\" \n" + "renewwitnessaccount \"funding_account\" \"witness_account\" \n" "\nRenew an expired witness account. \n" "1. \"funding_account\" (required) The unique UUID or label for the account.\n" "2. \"witness_account\" (required) The unique UUID or label for the account.\n" @@ -2700,20 +2700,23 @@ static UniValue renewwitnessaccount(const JSONRPCRequest& request) throw JSONRPCError(RPC_MISC_ERROR, "Cannot split a witness-only account as spend key is required to do this."); } + bool shouldUpgrade = false; + //fixme: (PHASE5) - Share common code with GUI::requestRenewWitness std::string strError; CMutableTransaction tx(CURRENT_TX_VERSION_POW2); CReserveKeyOrScript changeReserveKey(pactiveWallet, fundingAccount, KEYCHAIN_EXTERNAL); CAmount transactionFee; - if (!pactiveWallet->PrepareRenewWitnessAccountTransaction(fundingAccount, witnessAccount, changeReserveKey, tx, transactionFee, strError)) + if (!pactiveWallet->PrepareRenewWitnessAccountTransaction(fundingAccount, witnessAccount, changeReserveKey, tx, transactionFee, strError, nullptr, nullptr, &shouldUpgrade)) { throw std::runtime_error(strprintf("Failed to create renew transaction [%s]", strError.c_str())); } + uint256 finalTransactionHash; { LOCK2(cs_main, pactiveWallet->cs_wallet); - if (!pactiveWallet->SignAndSubmitTransaction(changeReserveKey, tx, strError, &finalTransactionHash)) + if (!pactiveWallet->SignAndSubmitTransaction(changeReserveKey, tx, strError, &finalTransactionHash, shouldUpgrade?SignType::WitnessUpdate:SignType::Spend)) { throw std::runtime_error(strprintf("Failed to sign renew transaction [%s]", strError.c_str())); } diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp index 92cb0945c1..baba26d765 100644 --- a/src/policy/policy.cpp +++ b/src/policy/policy.cpp @@ -294,7 +294,7 @@ bool IsSegregatedSignatureDataStandard(const CTransaction& tx, const CCoinsViewC for (uint64_t i=0; i MAX_STANDARD_SEGREGATED_SIGNATURE_STACK_ITEMS) return false; diff --git a/src/qt/_Gulden/witnessdialog.cpp b/src/qt/_Gulden/witnessdialog.cpp index ef32a65f28..b89c9cb979 100644 --- a/src/qt/_Gulden/witnessdialog.cpp +++ b/src/qt/_Gulden/witnessdialog.cpp @@ -708,8 +708,9 @@ bool WitnessDialog::doUpdate(bool forceUpdate, WitnessStatus* pWitnessStatus) case WitnessStatus::Expired: { computedWidgetIndex = WitnessDialogStates::EXPIRED; - stateRenewWitnessButton = !accountStatus.hasUnconfirmedWittnessTx; - stateExtendButton = IsSegSigEnabled(chainActive.TipPrev()) && accountStatus.hasUnconfirmedWittnessTx; + stateUpgradeButton = IsSegSigEnabled(chainActive.TipPrev()) && accountStatus.hasScriptLegacyOutput && !accountStatus.hasUnconfirmedWittnessTx; + stateRenewWitnessButton = !stateUpgradeButton && !accountStatus.hasUnconfirmedWittnessTx; + stateExtendButton = !stateUpgradeButton && IsSegSigEnabled(chainActive.TipPrev()) && accountStatus.hasUnconfirmedWittnessTx; break; } case WitnessStatus::Emptying: diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 9afc3cfeda..075309e1b4 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -184,6 +184,11 @@ static bool SignStep(const BaseSignatureCreator& creator, const CScript& scriptP } break; } + case WitnessUpdate: + { + return true; + break; + } case Witness: { if (!Sign1(witnessKeyID, creator, scriptPubKey, ret, sigversion)) @@ -232,6 +237,10 @@ static bool SignStep(const BaseSignatureCreator& creator, const CTxOutPoW2Witnes return false; return true; } + case WitnessUpdate: + { + return false; + } case Witness: { if (!Sign1(pow2Witness.witnessKeyID, creator, scriptSignatureDataPlaceholder, ret, SIGVERSION_SEGSIG)) diff --git a/src/script/sign.h b/src/script/sign.h index 7591f7dfef..d2dd2ec9da 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -77,7 +77,8 @@ struct SignatureData { enum SignType { Spend, - Witness + Witness, + WitnessUpdate }; /** Get the CKeyID of the pubkey for the key that should be used to sign an output */ diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index e5cb2bec13..a036ea79f7 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -771,7 +771,7 @@ class CWallet : public CGuldenWallet bool SignTransaction(CAccount* fromAccount, CMutableTransaction& tx, SignType type); //! Create a transaction that renews an expired witness account - bool PrepareRenewWitnessAccountTransaction(CAccount* funderAccount, CAccount* targetWitnessAccount, CReserveKeyOrScript& changeReserveKey, CMutableTransaction& tx, CAmount& nFeeOut, std::string& strError, uint64_t* skipPastTransaction=nullptr, CCoinControl* coinControl=nullptr); + bool PrepareRenewWitnessAccountTransaction(CAccount* funderAccount, CAccount* targetWitnessAccount, CReserveKeyOrScript& changeReserveKey, CMutableTransaction& tx, CAmount& nFeeOut, std::string& strError, uint64_t* skipPastTransaction=nullptr, CCoinControl* coinControl=nullptr, bool* shouldUpgrade=nullptr); //! Create a transaction upgrades an old ScriptLegacyOutput witness to a new PoW2WitnessOutput void PrepareUpgradeWitnessAccountTransaction(CAccount* funderAccount, CAccount* targetWitnessAccount, CReserveKeyOrScript& changeReserveKey, CMutableTransaction& tx, CAmount& nFeeOut); @@ -799,7 +799,7 @@ class CWallet : public CGuldenWallet /** * Sign and submit a transaction (that has not yet been signed) to the network, add to wallet as appropriate etc. */ - bool SignAndSubmitTransaction(CReserveKeyOrScript& changeReserveKey, CMutableTransaction& tx, std::string& strError, uint256* pTransactionHashOut=nullptr); + bool SignAndSubmitTransaction(CReserveKeyOrScript& changeReserveKey, CMutableTransaction& tx, std::string& strError, uint256* pTransactionHashOut=nullptr, SignType type=SignType::Spend); bool CommitTransaction(CWalletTx& wtxNew, CReserveKeyOrScript& reservekey, CConnman* connman, CValidationState& state); diff --git a/src/wallet/wallet_transaction.cpp b/src/wallet/wallet_transaction.cpp index 3881fb615f..eb9d01ee45 100644 --- a/src/wallet/wallet_transaction.cpp +++ b/src/wallet/wallet_transaction.cpp @@ -776,9 +776,16 @@ bool CWallet::AddFeeForTransaction(CAccount* forAccount, CMutableTransaction& tx CTransaction txNewConst(txNew); // Remove scriptSigs to eliminate the fee calculation dummy signatures - for (auto& vin : txNew.vin) { - vin.scriptSig = CScript(); - vin.segregatedSignatureData.SetNull(); + { + LOCK2(cs_main, cs_wallet); + for (auto& txin : txNew.vin) + { + txin.scriptSig = CScript(); + txin.segregatedSignatureData.SetNull(); + + // Prevent input being spent twice + LockCoin(txin.prevout); + } } // Allow to override the default confirmation target over the CoinControl instance @@ -888,7 +895,7 @@ bool CWallet::AddFeeForTransaction(CAccount* forAccount, CMutableTransaction& tx } -bool CWallet::PrepareRenewWitnessAccountTransaction(CAccount* funderAccount, CAccount* targetWitnessAccount, CReserveKeyOrScript& changeReserveKey, CMutableTransaction& tx, CAmount& nFeeOut, std::string& strError, uint64_t* skipPastTransaction, CCoinControl* coinControl) +bool CWallet::PrepareRenewWitnessAccountTransaction(CAccount* funderAccount, CAccount* targetWitnessAccount, CReserveKeyOrScript& changeReserveKey, CMutableTransaction& tx, CAmount& nFeeOut, std::string& strError, uint64_t* skipPastTransaction, CCoinControl* coinControl, bool* shouldUpgrade) { LOCK2(cs_main, cs_wallet); // cs_main required for ReadBlockFromDisk. @@ -920,6 +927,9 @@ bool CWallet::PrepareRenewWitnessAccountTransaction(CAccount* funderAccount, CAc addedAny = true; + if (shouldUpgrade && witCoin.coin.out.GetType() == CTxOutType::ScriptLegacyOutput) + *shouldUpgrade = true; + // Add witness input AddTxInput(tx, CInputCoin(witCoin.outpoint, witCoin.coin.out, allowIndexBased, witCoin.coin.nHeight, witCoin.coin.nTxIndex), false); @@ -1050,9 +1060,9 @@ void CWallet::PrepareUpgradeWitnessAccountTransaction(CAccount* funderAccount, C throw std::runtime_error("Could not find suitable witness to upgrade"); } -bool CWallet::SignAndSubmitTransaction(CReserveKeyOrScript& changeReserveKey, CMutableTransaction& tx, std::string& strError, uint256* pTransactionHashOut) +bool CWallet::SignAndSubmitTransaction(CReserveKeyOrScript& changeReserveKey, CMutableTransaction& tx, std::string& strError, uint256* pTransactionHashOut, SignType type) { - if (!SignTransaction(nullptr, tx, SignType::Spend)) + if (!SignTransaction(nullptr, tx, type)) { strError = "Unable to sign transaction"; return false; diff --git a/src/wallet/witness_operations.cpp b/src/wallet/witness_operations.cpp index a45bb6912a..04c81f200c 100644 --- a/src/wallet/witness_operations.cpp +++ b/src/wallet/witness_operations.cpp @@ -146,10 +146,10 @@ void extendwitnessaddresshelper(CAccount* fundingAccount, witnessOutputsInfoVect } } - uint256 finalTransactionHash; + uint256 extendTransactionHash; { LOCK2(cs_main, pwallet->cs_wallet); - if (!pwallet->SignAndSubmitTransaction(reservekey, extendWitnessTransaction, reasonForFail, &finalTransactionHash)) + if (!pwallet->SignAndSubmitTransaction(reservekey, extendWitnessTransaction, reasonForFail, &extendTransactionHash)) { throw witness_error(witness::RPC_MISC_ERROR, strprintf("Failed to commit transaction [%s]", reasonForFail.c_str())); } @@ -157,7 +157,7 @@ void extendwitnessaddresshelper(CAccount* fundingAccount, witnessOutputsInfoVect // Set result parameters if (pTxid != nullptr) - *pTxid = finalTransactionHash.GetHex(); + *pTxid = extendTransactionHash.GetHex(); if (pFee != nullptr) *pFee = transactionFee; } @@ -324,15 +324,15 @@ void upgradewitnessaccount(CWallet* pwallet, CAccount* fundingAccount, CAccount* CAmount transactionFee; pwallet->PrepareUpgradeWitnessAccountTransaction(fundingAccount, witnessAccount, changeReserveKey, tx, transactionFee); - uint256 finalTransactionHash; - if (!pwallet->SignAndSubmitTransaction(changeReserveKey, tx, strError, &finalTransactionHash)) + uint256 upgradeTransactionHash; + if (!pwallet->SignAndSubmitTransaction(changeReserveKey, tx, strError, &upgradeTransactionHash, SignType::WitnessUpdate)) { throw std::runtime_error(strprintf("Failed to sign transaction [%s]", strError.c_str())); } // Set result parameters if (pTxid != nullptr) - *pTxid = finalTransactionHash.GetHex(); + *pTxid = upgradeTransactionHash.GetHex(); if (pFee != nullptr) *pFee = transactionFee; } @@ -442,10 +442,10 @@ void rotatewitnessaddresshelper(CAccount* fundingAccount, witnessOutputsInfoVect throw witness_error(witness::RPC_MISC_ERROR, strprintf("Failed to fund transaction [%s]", reasonForFail.c_str())); } - uint256 finalTransactionHash; + uint256 rotateTransactionHash; { LOCK2(cs_main, pactiveWallet->cs_wallet); - if (!pwallet->SignAndSubmitTransaction(reservekey, rotateWitnessTransaction, reasonForFail, &finalTransactionHash)) + if (!pwallet->SignAndSubmitTransaction(reservekey, rotateWitnessTransaction, reasonForFail, &rotateTransactionHash)) { throw witness_error(witness::RPC_MISC_ERROR, strprintf("Failed to commit transaction [%s]", reasonForFail.c_str())); } @@ -453,7 +453,7 @@ void rotatewitnessaddresshelper(CAccount* fundingAccount, witnessOutputsInfoVect // Set result parameters if (pTxid != nullptr) - *pTxid = finalTransactionHash.GetHex(); + *pTxid = rotateTransactionHash.GetHex(); if (pFee != nullptr) *pFee = transactionFee; }