From 44620de508fab004b7c4c5b03367d52e50d67172 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 15 Dec 2020 10:27:16 +0100 Subject: [PATCH 1/5] [Wallet] Do not perform ECDSA signing in the fee calculation inner loop >>> based on bitcoin/bitcoin@b3d7b1cbe7afdc6a63bbcbe938e8639deedb04a1 Performing signing in the inner loop has terrible performance when many passes through are needed to complete the selection. Signing before the algorithm is complete also gets in the way of correctly setting the fee (e.g. preventing over-payment when the fee required goes down on the final selection.) Use of the dummy might overpay on the signatures by a couple bytes in uncommon cases where the signatures' DER encoding is smaller than the dummy: Who cares? --- src/wallet/wallet.cpp | 80 +++++++++++++++++++++++-------------------- 1 file changed, 43 insertions(+), 37 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 416504017d36..4d16178d5fcf 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2697,6 +2697,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, coinFilter.nCoinType = coin_type; { + std::set > setCoins; LOCK2(cs_main, cs_wallet); { std::vector vAvailableCoins; @@ -2722,8 +2723,8 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, } // Choose coins to use - std::set > setCoins; CAmount nValueIn = 0; + setCoins.clear(); if (!SelectCoinsToSpend(vAvailableCoins, nTotalValue, setCoins, nValueIn, coinControl)) { if (coin_type == ALL_COINS) { @@ -2809,29 +2810,12 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, txNew.vin.emplace_back(coin.first->GetHash(), coin.second); } - // Sign + // Fill in dummy signatures for fee calculation. int nIn = 0; - CTransaction txNewConst(txNew); - SigVersion sigversion = txNewConst.GetRequiredSigVersion(); - for (const std::pair & coin : setCoins) { - bool signSuccess; + for (const auto & coin : setCoins) { const CScript& scriptPubKey = coin.first->vout[coin.second].scriptPubKey; SignatureData sigdata; - bool haveKey = coin.first->GetStakeDelegationCredit() > 0; - if (sign) { - signSuccess = ProduceSignature( - TransactionSignatureCreator(this, &txNewConst, nIn, coin.first->vout[coin.second].nValue, SIGHASH_ALL), - scriptPubKey, - sigdata, - sigversion, - !haveKey // fColdStake = false - ); - } else { - signSuccess = ProduceSignature( - DummySignatureCreator(this), scriptPubKey, sigdata, sigversion, false); - } - - if (!signSuccess) { + if (!ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata, txNew.GetRequiredSigVersion(), false)) { strFailReason = _("Signing transaction failed"); return false; } else { @@ -2840,25 +2824,14 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, nIn++; } - unsigned int nBytes = ::GetSerializeSize(txNew, SER_NETWORK, PROTOCOL_VERSION); - - // Remove scriptSigs if we used dummy signatures for fee calculation - if (!sign) { - for (CTxIn& vin : txNew.vin) - vin.scriptSig = CScript(); - } - - // Embed the constructed transaction data in wtxNew. - *static_cast(wtxNew) = CTransaction(txNew); + const unsigned int nBytes = ::GetSerializeSize(txNew, SER_NETWORK, PROTOCOL_VERSION); + CAmount nFeeNeeded = std::max(nFeePay, GetMinimumFee(nBytes, nTxConfirmTarget, mempool)); - // Limit size - if (nBytes >= MAX_STANDARD_TX_SIZE) { - strFailReason = _("Transaction too large"); - return false; + // Remove scriptSigs to eliminate the fee calculation dummy signatures + for (CTxIn& vin : txNew.vin) { + vin.scriptSig = CScript(); } - CAmount nFeeNeeded = std::max(nFeePay, GetMinimumFee(nBytes, nTxConfirmTarget, mempool)); - if (coinControl && nFeeNeeded > 0 && coinControl->nMinimumTotalFee > nFeeNeeded) { nFeeNeeded = coinControl->nMinimumTotalFee; } @@ -2885,6 +2858,39 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, return false; } } + + if (sign) { + CTransaction txNewConst(txNew); + int nIn = 0; + for (const auto& coin : setCoins) { + const CScript& scriptPubKey = coin.first->vout[coin.second].scriptPubKey; + SignatureData sigdata; + bool haveKey = coin.first->GetStakeDelegationCredit() > 0; + + if (!ProduceSignature( + TransactionSignatureCreator(this, &txNewConst, nIn, coin.first->vout[coin.second].nValue, SIGHASH_ALL), + scriptPubKey, + sigdata, + txNewConst.GetRequiredSigVersion(), + !haveKey // fColdStake + )) { + strFailReason = _("Signing transaction failed"); + return false; + } else { + UpdateTransaction(txNew, nIn, sigdata); + } + nIn++; + } + } + + // Limit size + if (::GetSerializeSize(txNew, SER_NETWORK, PROTOCOL_VERSION) >= MAX_STANDARD_TX_SIZE) { + strFailReason = _("Transaction too large"); + return false; + } + + // Embed the constructed transaction data in wtxNew. + *static_cast(wtxNew) = CTransaction(txNew); } return true; } From efd7139fe48e8e0003788d7df6db958a3f269185 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 15 Dec 2020 11:27:41 +0100 Subject: [PATCH 2/5] Sapling: Decouple ProveAndSign from Build --- src/sapling/transaction_builder.cpp | 143 ++++++++++++++-------------- src/sapling/transaction_builder.h | 2 + 2 files changed, 76 insertions(+), 69 deletions(-) diff --git a/src/sapling/transaction_builder.cpp b/src/sapling/transaction_builder.cpp index 97203338d56f..41bd0530dfc3 100644 --- a/src/sapling/transaction_builder.cpp +++ b/src/sapling/transaction_builder.cpp @@ -209,66 +209,31 @@ void TransactionBuilder::SendChangeTo(CTxDestination& changeAddr) saplingChangeAddr = nullopt; } -TransactionBuilderResult TransactionBuilder::Build() +TransactionBuilderResult TransactionBuilder::ProveAndSign() { // - // Consistency checks + // Sapling spend descriptions // - // Valid fee - if (fee < 0) { - return TransactionBuilderResult("Fee cannot be negative"); - } - - // Valid change - CAmount change = mtx.sapData->valueBalance - fee; - for (auto& tIn : tIns) { - change += tIn.value; - } - for (auto& tOut : mtx.vout) { - change -= tOut.nValue; - } - if (change < 0) { - return TransactionBuilderResult("Change cannot be negative"); - } + if (!spends.empty() || !outputs.empty()) { - // - // Change output - // + auto ctx = librustzcash_sapling_proving_ctx_init(); - if (change > 0) { - // If we get here and the change is dust, add it to the fee - CAmount dustThreshold = (spends.empty() && outputs.empty()) ? GetDustThreshold(minRelayTxFee) : - GetShieldedDustThreshold(minRelayTxFee); - if (change > dustThreshold) { - // Send change to the specified change address. If no change address - // was set, send change to the first Sapling address given as input - // (A t-address can only be used as the change address if explicitly set.) - if (saplingChangeAddr) { - AddSaplingOutput(saplingChangeAddr->first, saplingChangeAddr->second, change); - } else if (tChangeAddr) { - // tChangeAddr has already been validated. - AddTransparentOutput(*tChangeAddr, change); - } else if (!spends.empty()) { - auto fvk = spends[0].expsk.full_viewing_key(); - auto note = spends[0].note; - libzcash::SaplingPaymentAddress changeAddr(note.d, note.pk_d); - AddSaplingOutput(fvk.ovk, changeAddr, change); - } else { - return TransactionBuilderResult("Could not determine change address"); + // Create Sapling OutputDescriptions + for (auto output : outputs) { + // Check this out here as well to provide better logging. + if (!output.note.cmu()) { + librustzcash_sapling_proving_ctx_free(ctx); + return TransactionBuilderResult("Output is invalid"); } - } else { - // Not used after, but update for consistency - fee += change; - change = 0; - } - } - // - // Sapling spends and outputs - // - if (!spends.empty() || !outputs.empty()) { + auto odesc = output.Build(ctx); + if (!odesc) { + librustzcash_sapling_proving_ctx_free(ctx); + return TransactionBuilderResult("Failed to create output description"); + } - auto ctx = librustzcash_sapling_proving_ctx_init(); + mtx.sapData->vShieldedOutput.push_back(odesc.get()); + } // Create Sapling SpendDescriptions for (auto spend : spends) { @@ -307,23 +272,6 @@ TransactionBuilderResult TransactionBuilder::Build() mtx.sapData->vShieldedSpend.push_back(sdesc); } - // Create Sapling OutputDescriptions - for (auto output : outputs) { - // Check this out here as well to provide better logging. - if (!output.note.cmu()) { - librustzcash_sapling_proving_ctx_free(ctx); - return TransactionBuilderResult("Output is invalid"); - } - - auto odesc = output.Build(ctx); - if (!odesc) { - librustzcash_sapling_proving_ctx_free(ctx); - return TransactionBuilderResult("Failed to create output description"); - } - - mtx.sapData->vShieldedOutput.push_back(odesc.get()); - } - // // Signatures // @@ -375,3 +323,60 @@ TransactionBuilderResult TransactionBuilder::Build() return TransactionBuilderResult(CTransaction(mtx)); } + +TransactionBuilderResult TransactionBuilder::Build() +{ + // + // Consistency checks + // + // Valid fee + if (fee < 0) { + return TransactionBuilderResult("Fee cannot be negative"); + } + + // Valid change + CAmount change = mtx.sapData->valueBalance - fee; + for (auto& tIn : tIns) { + change += tIn.value; + } + for (auto& tOut : mtx.vout) { + change -= tOut.nValue; + } + if (change < 0) { + return TransactionBuilderResult("Change cannot be negative"); + } + + // + // Change output + // + + if (change > 0) { + // If we get here and the change is dust, add it to the fee + CAmount dustThreshold = (spends.empty() && outputs.empty()) ? GetDustThreshold(minRelayTxFee) : + GetShieldedDustThreshold(minRelayTxFee); + if (change > dustThreshold) { + // Send change to the specified change address. If no change address + // was set, send change to the first Sapling address given as input + // (A t-address can only be used as the change address if explicitly set.) + if (saplingChangeAddr) { + AddSaplingOutput(saplingChangeAddr->first, saplingChangeAddr->second, change); + } else if (tChangeAddr) { + // tChangeAddr has already been validated. + AddTransparentOutput(*tChangeAddr, change); + } else if (!spends.empty()) { + auto fvk = spends[0].expsk.full_viewing_key(); + auto note = spends[0].note; + libzcash::SaplingPaymentAddress changeAddr(note.d, note.pk_d); + AddSaplingOutput(fvk.ovk, changeAddr, change); + } else { + return TransactionBuilderResult("Could not determine change address"); + } + } else { + // Not used after, but update for consistency + fee += change; + change = 0; + } + } + + return ProveAndSign(); +} diff --git a/src/sapling/transaction_builder.h b/src/sapling/transaction_builder.h index 164a3ed05d10..257baa34823b 100644 --- a/src/sapling/transaction_builder.h +++ b/src/sapling/transaction_builder.h @@ -121,6 +121,8 @@ class TransactionBuilder void SendChangeTo(CTxDestination& changeAddr); TransactionBuilderResult Build(); + // Add Sapling Spend/Output descriptions, binding sig, and transparent signatures + TransactionBuilderResult ProveAndSign(); }; #endif /* TRANSACTION_BUILDER_H */ From 5d8ba3d7008ffa2cb45c98a346e46fcf9b00469f Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 15 Dec 2020 12:29:31 +0100 Subject: [PATCH 3/5] Sapling: add option to skip ProveAndSing in TransactionBuilder::Build adding dummy proofs and signatures instead --- src/sapling/transaction_builder.cpp | 63 ++++++++++++++++++++++++++++- src/sapling/transaction_builder.h | 6 ++- src/uint256.h | 1 + 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/src/sapling/transaction_builder.cpp b/src/sapling/transaction_builder.cpp index 41bd0530dfc3..71e5b362b8b1 100644 --- a/src/sapling/transaction_builder.cpp +++ b/src/sapling/transaction_builder.cpp @@ -324,7 +324,66 @@ TransactionBuilderResult TransactionBuilder::ProveAndSign() return TransactionBuilderResult(CTransaction(mtx)); } -TransactionBuilderResult TransactionBuilder::Build() +TransactionBuilderResult TransactionBuilder::AddDummySignatures() +{ + if (!spends.empty() || !outputs.empty()) { + // Add Dummy Sapling OutputDescriptions + OutputDescription dummyOD; + dummyOD.cv = UINT256_MAX; + dummyOD.cmu = UINT256_MAX; + dummyOD.ephemeralKey = UINT256_MAX; + dummyOD.encCiphertext = {{0xff}}; + dummyOD.outCiphertext = {{0xff}}; + dummyOD.zkproof = {{0xff}}; + for (unsigned int i = 0; i < outputs.size(); i++) { + mtx.sapData->vShieldedOutput.push_back(dummyOD); + } + // Add Dummy Sapling SpendDescriptions + SpendDescription dummySD; + dummySD.cv = UINT256_MAX; + dummySD.anchor = UINT256_MAX; + dummySD.nullifier = UINT256_MAX; + dummySD.rk = UINT256_MAX; + dummySD.zkproof = {{0xff}}; + dummySD.spendAuthSig = {{0xff}}; + for (unsigned int i = 0; i < spends.size(); i++) { + mtx.sapData->vShieldedSpend.push_back(dummySD); + } + // Add Dummy Binding sig + mtx.sapData->bindingSig = {{0xff}}; + } + + // Add Dummmy Transparent signatures + CTransaction txNewConst(mtx); + for (int nIn = 0; nIn < (int) mtx.vin.size(); nIn++) { + auto tIn = tIns[nIn]; + SignatureData sigdata; + if (!ProduceSignature(DummySignatureCreator(keystore), tIn.scriptPubKey, sigdata, SIGVERSION_SAPLING, false)) { + return TransactionBuilderResult("Failed to sign transaction"); + } else { + UpdateTransaction(mtx, nIn, sigdata); + } + } + + return TransactionBuilderResult(CTransaction(mtx)); +} + +void TransactionBuilder::ClearProofsAndSignatures() +{ + // Clear Sapling output descriptions + mtx.sapData->vShieldedOutput.clear(); + + // Clear Sapling spend descriptions + mtx.sapData->vShieldedSpend.clear(); + + // Clear Binding sig + mtx.sapData->bindingSig = {{0}}; + + // Clear Transparent signatures + for (CTxIn& in : mtx.vin) in.scriptSig = CScript(); +} + +TransactionBuilderResult TransactionBuilder::Build(bool fDummySig) { // // Consistency checks @@ -378,5 +437,5 @@ TransactionBuilderResult TransactionBuilder::Build() } } - return ProveAndSign(); + return fDummySig ? AddDummySignatures() : ProveAndSign(); } diff --git a/src/sapling/transaction_builder.h b/src/sapling/transaction_builder.h index 257baa34823b..0073fb3ca7b3 100644 --- a/src/sapling/transaction_builder.h +++ b/src/sapling/transaction_builder.h @@ -120,9 +120,13 @@ class TransactionBuilder void SendChangeTo(CTxDestination& changeAddr); - TransactionBuilderResult Build(); + TransactionBuilderResult Build(bool fDummySig = false); // Add Sapling Spend/Output descriptions, binding sig, and transparent signatures TransactionBuilderResult ProveAndSign(); + // Add dummy Sapling Spend/Output descriptions, binding sig, and transparent signatures + TransactionBuilderResult AddDummySignatures(); + // Remove Sapling Spend/Output descriptions, binding sig, and transparent signatures + void ClearProofsAndSignatures(); }; #endif /* TRANSACTION_BUILDER_H */ diff --git a/src/uint256.h b/src/uint256.h index 85a58c687835..5c690c79fd6b 100644 --- a/src/uint256.h +++ b/src/uint256.h @@ -139,5 +139,6 @@ arith_uint512 UintToArith512(const uint512 &); /** constant uint256 instances */ const uint256 UINT256_ZERO = uint256(); const uint256 UINT256_ONE = uint256("0000000000000000000000000000000000000000000000000000000000000001"); +const uint256 UINT256_MAX = uint256("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"); #endif // PIVX_UINT256_H From 6099737168b3769c65d999dc3fb5b38a44eed909 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 15 Dec 2020 12:35:01 +0100 Subject: [PATCH 4/5] Sapling: Skip proofs and signatures during fee calculation loop --- src/sapling/sapling_operation.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/sapling/sapling_operation.cpp b/src/sapling/sapling_operation.cpp index 4315fcc90207..8754bc4e9940 100644 --- a/src/sapling/sapling_operation.cpp +++ b/src/sapling/sapling_operation.cpp @@ -170,7 +170,7 @@ OperationResult SaplingOperation::build() // Build the transaction txBuilder.SetFee(nFeeRet); - TransactionBuilderResult txResult = txBuilder.Build(); + TransactionBuilderResult txResult = txBuilder.Build(true); auto opTx = txResult.GetTx(); // Check existent tx @@ -213,6 +213,16 @@ OperationResult SaplingOperation::build() } // Done fee = nFeeRet; + + // Clear dummy signatures/proofs and add real ones + txBuilder.ClearProofsAndSignatures(); + TransactionBuilderResult txResult = txBuilder.ProveAndSign(); + auto opTx = txResult.GetTx(); + // Check existent tx + if (!opTx) { + return errorOut("Failed to build transaction: " + txResult.GetError()); + } + finalTx = *opTx; return OperationResult(true); } From e5aab5364bab8448c9b67b33477b080143686bbd Mon Sep 17 00:00:00 2001 From: random-zebra Date: Fri, 18 Dec 2020 17:28:43 +0100 Subject: [PATCH 5/5] Refactor: initialize dummy Spend/Output descriptions only once --- src/sapling/transaction_builder.cpp | 49 +++++++++++++++++++---------- src/sapling/transaction_builder.h | 5 +++ 2 files changed, 37 insertions(+), 17 deletions(-) diff --git a/src/sapling/transaction_builder.cpp b/src/sapling/transaction_builder.cpp index 71e5b362b8b1..8cb3addd398e 100644 --- a/src/sapling/transaction_builder.cpp +++ b/src/sapling/transaction_builder.cpp @@ -68,6 +68,35 @@ Optional OutputDescriptionInfo::Build(void* ctx) { return odesc; } +// Dummy constants used during fee-calculation loop +static OutputDescription CreateDummyOD() +{ + OutputDescription dummyOD; + dummyOD.cv = UINT256_MAX; + dummyOD.cmu = UINT256_MAX; + dummyOD.ephemeralKey = UINT256_MAX; + dummyOD.encCiphertext = {{0xff}}; + dummyOD.outCiphertext = {{0xff}}; + dummyOD.zkproof = {{0xff}}; + return dummyOD; +} +static SpendDescription CreateDummySD() +{ + SpendDescription dummySD; + dummySD.cv = UINT256_MAX; + dummySD.anchor = UINT256_MAX; + dummySD.nullifier = UINT256_MAX; + dummySD.rk = UINT256_MAX; + dummySD.zkproof = {{0xff}}; + dummySD.spendAuthSig = {{0xff}}; + return dummySD; +} + +const OutputDescription DUMMY_SHIELD_OUT = CreateDummyOD(); +const SpendDescription DUMMY_SHIELD_SPEND = CreateDummySD(); +const SaplingTxData::binding_sig_t DUMMY_SHIELD_BINDSIG = {{0xff}}; + + TransactionBuilderResult::TransactionBuilderResult(const CTransaction& tx) : maybeTx(tx) {} TransactionBuilderResult::TransactionBuilderResult(const std::string& error) : maybeError(error) {} @@ -328,29 +357,15 @@ TransactionBuilderResult TransactionBuilder::AddDummySignatures() { if (!spends.empty() || !outputs.empty()) { // Add Dummy Sapling OutputDescriptions - OutputDescription dummyOD; - dummyOD.cv = UINT256_MAX; - dummyOD.cmu = UINT256_MAX; - dummyOD.ephemeralKey = UINT256_MAX; - dummyOD.encCiphertext = {{0xff}}; - dummyOD.outCiphertext = {{0xff}}; - dummyOD.zkproof = {{0xff}}; for (unsigned int i = 0; i < outputs.size(); i++) { - mtx.sapData->vShieldedOutput.push_back(dummyOD); + mtx.sapData->vShieldedOutput.push_back(DUMMY_SHIELD_OUT); } // Add Dummy Sapling SpendDescriptions - SpendDescription dummySD; - dummySD.cv = UINT256_MAX; - dummySD.anchor = UINT256_MAX; - dummySD.nullifier = UINT256_MAX; - dummySD.rk = UINT256_MAX; - dummySD.zkproof = {{0xff}}; - dummySD.spendAuthSig = {{0xff}}; for (unsigned int i = 0; i < spends.size(); i++) { - mtx.sapData->vShieldedSpend.push_back(dummySD); + mtx.sapData->vShieldedSpend.push_back(DUMMY_SHIELD_SPEND); } // Add Dummy Binding sig - mtx.sapData->bindingSig = {{0xff}}; + mtx.sapData->bindingSig = DUMMY_SHIELD_BINDSIG; } // Add Dummmy Transparent signatures diff --git a/src/sapling/transaction_builder.h b/src/sapling/transaction_builder.h index 0073fb3ca7b3..c4557ff963a3 100644 --- a/src/sapling/transaction_builder.h +++ b/src/sapling/transaction_builder.h @@ -55,6 +55,11 @@ struct TransparentInputInfo { CAmount value) : scriptPubKey(scriptPubKey), value(value) {} }; +// Dummy constants used during fee-calculation loop +extern const OutputDescription DUMMY_SHIELD_OUT; +extern const SpendDescription DUMMY_SHIELD_SPEND; +extern const SaplingTxData::binding_sig_t DUMMY_SHIELD_BINDSIG; + class TransactionBuilderResult { private: Optional maybeTx;