Address review feedback on z_shieldcoinbase
This commit is contained in:
parent
993dc0c279
commit
55ebf19c55
|
@ -173,7 +173,7 @@ class WalletSaplingTest(BitcoinTestFramework):
|
|||
taddr1,
|
||||
[{'address': node4_sproutaddr, 'amount': Decimal('2.5')},
|
||||
{'address': node4_saplingaddr, 'amount': Decimal('2.5') - DEFAULT_FEE}],
|
||||
1, DEFAULT_FEE
|
||||
1, DEFAULT_FEE, 'AllowRevealedSenders'
|
||||
)
|
||||
wait_and_assert_operationid_status(self.nodes[1], myopid, "failed", "Sending funds into the Sprout pool is no longer supported.")
|
||||
|
||||
|
|
|
@ -155,6 +155,7 @@ AsyncRPCOperation_sendmany::main_impl(CWallet& wallet) {
|
|||
|
||||
return preparedTx
|
||||
.map([&](const TransactionEffects& effects) {
|
||||
effects.LockSpendable(wallet);
|
||||
try {
|
||||
const auto& spendable = effects.GetSpendable();
|
||||
const auto& payments = effects.GetPayments();
|
||||
|
|
|
@ -60,6 +60,16 @@ AsyncRPCOperation_shieldcoinbase::AsyncRPCOperation_shieldcoinbase(
|
|||
assert(MoneyRange(fee));
|
||||
assert(ztxoSelector.RequireSpendingKeys());
|
||||
|
||||
examine(toAddress_, match {
|
||||
[](const CKeyID&) {
|
||||
throw JSONRPCError(RPC_VERIFY_REJECTED, "Cannot shield coinbase output to a p2pkh address.");
|
||||
},
|
||||
[](const CScriptID&) {
|
||||
throw JSONRPCError(RPC_VERIFY_REJECTED, "Cannot shield coinbase output to a p2sh address.");
|
||||
},
|
||||
[](const auto&) { },
|
||||
});
|
||||
|
||||
// Log the context info
|
||||
if (LogAcceptCategory("zrpcunsafe")) {
|
||||
LogPrint("zrpcunsafe", "%s: z_shieldcoinbase initialized (context=%s)\n", getId(), contextInfo.write());
|
||||
|
@ -81,7 +91,7 @@ void AsyncRPCOperation_shieldcoinbase::main() {
|
|||
|
||||
std::optional<uint256> txid;
|
||||
try {
|
||||
txid = main_impl();
|
||||
txid = main_impl(*pwalletMain);
|
||||
} catch (const UniValue& objError) {
|
||||
int code = find_value(objError, "code").get_int();
|
||||
std::string message = find_value(objError, "message").get_str();
|
||||
|
@ -123,15 +133,14 @@ void AsyncRPCOperation_shieldcoinbase::main() {
|
|||
}
|
||||
|
||||
|
||||
Remaining AsyncRPCOperation_shieldcoinbase::prepare() {
|
||||
auto spendable = builder_.FindAllSpendableInputs(*pwalletMain, ztxoSelector_, COINBASE_MATURITY);
|
||||
Remaining AsyncRPCOperation_shieldcoinbase::prepare(CWallet& wallet) {
|
||||
auto spendable = builder_.FindAllSpendableInputs(wallet, ztxoSelector_, COINBASE_MATURITY);
|
||||
|
||||
// Find unspent coinbase utxos and update estimated size
|
||||
unsigned int max_tx_size = MAX_TX_SIZE_AFTER_SAPLING;
|
||||
CAmount shieldingValue = 0;
|
||||
CAmount remainingValue = 0;
|
||||
// FIXME: bump this up to an appropriate size for Orchard
|
||||
size_t estimatedTxSize = 2000; // 1802 joinsplit description + tx overhead + wiggle room
|
||||
size_t estimatedTxSize = 10000; // per ZIP 401 (https://zips.z.cash/zip-0401#specification)
|
||||
size_t utxoCounter = 0;
|
||||
size_t numUtxos = 0;
|
||||
bool maxedOutFlag = false;
|
||||
|
@ -148,7 +157,7 @@ Remaining AsyncRPCOperation_shieldcoinbase::prepare() {
|
|||
utxoCounter++;
|
||||
if (!maxedOutFlag) {
|
||||
size_t increase =
|
||||
(std::get_if<CScriptID>(&address) != nullptr) ? CTXIN_SPEND_P2SH_SIZE : CTXIN_SPEND_DUST_SIZE;
|
||||
(std::get_if<CScriptID>(&address) != nullptr) ? CTXIN_SPEND_P2SH_SIZE : CTXIN_SPEND_P2PKH_SIZE;
|
||||
|
||||
if (estimatedTxSize + increase >= max_tx_size || (0 < nUTXOLimit_ && nUTXOLimit_ < utxoCounter)) {
|
||||
maxedOutFlag = true;
|
||||
|
@ -179,7 +188,7 @@ Remaining AsyncRPCOperation_shieldcoinbase::prepare() {
|
|||
std::vector<Payment> payments = { Payment(toAddress_, shieldingValue - fee_, std::nullopt) };
|
||||
|
||||
auto preparationResult = builder_.PrepareTransaction(
|
||||
*pwalletMain,
|
||||
wallet,
|
||||
ztxoSelector_,
|
||||
spendable,
|
||||
payments,
|
||||
|
@ -193,50 +202,56 @@ Remaining AsyncRPCOperation_shieldcoinbase::prepare() {
|
|||
ThrowInputSelectionError(err, ztxoSelector_, strategy_);
|
||||
})
|
||||
.map([&](const TransactionEffects& effects) {
|
||||
effects.LockSpendable(wallet);
|
||||
effects_ = effects;
|
||||
});
|
||||
|
||||
return Remaining(utxoCounter, numUtxos, remainingValue, shieldingValue);
|
||||
}
|
||||
|
||||
uint256 AsyncRPCOperation_shieldcoinbase::main_impl() {
|
||||
uint256 AsyncRPCOperation_shieldcoinbase::main_impl(CWallet& wallet) {
|
||||
uint256 txid;
|
||||
|
||||
const auto& spendable = effects_->GetSpendable();
|
||||
const auto& payments = effects_->GetPayments();
|
||||
spendable.LogInputs(getId());
|
||||
try {
|
||||
const auto& spendable = effects_->GetSpendable();
|
||||
const auto& payments = effects_->GetPayments();
|
||||
spendable.LogInputs(getId());
|
||||
|
||||
LogPrint("zrpcunsafe", "%s: spending %s to send %s with fee %s\n", getId(),
|
||||
FormatMoney(payments.Total()),
|
||||
FormatMoney(spendable.Total()),
|
||||
FormatMoney(effects_->GetFee()));
|
||||
LogPrint("zrpc", "%s: total transparent input: %s (to choose from)\n", getId(),
|
||||
FormatMoney(spendable.GetTransparentTotal()));
|
||||
LogPrint("zrpcunsafe", "%s: total shielded input: %s (to choose from)\n", getId(),
|
||||
FormatMoney(spendable.GetSaplingTotal() + spendable.GetOrchardTotal()));
|
||||
LogPrint("zrpc", "%s: total transparent output: %s\n", getId(),
|
||||
FormatMoney(payments.GetTransparentTotal()));
|
||||
LogPrint("zrpcunsafe", "%s: total shielded Sapling output: %s\n", getId(),
|
||||
FormatMoney(payments.GetSaplingTotal()));
|
||||
LogPrint("zrpcunsafe", "%s: total shielded Orchard output: %s\n", getId(),
|
||||
FormatMoney(payments.GetOrchardTotal()));
|
||||
LogPrint("zrpc", "%s: fee: %s\n", getId(), FormatMoney(effects_->GetFee()));
|
||||
LogPrint("zrpcunsafe", "%s: spending %s to send %s with fee %s\n", getId(),
|
||||
FormatMoney(payments.Total()),
|
||||
FormatMoney(spendable.Total()),
|
||||
FormatMoney(effects_->GetFee()));
|
||||
LogPrint("zrpc", "%s: total transparent input: %s (to choose from)\n", getId(),
|
||||
FormatMoney(spendable.GetTransparentTotal()));
|
||||
LogPrint("zrpcunsafe", "%s: total shielded input: %s (to choose from)\n", getId(),
|
||||
FormatMoney(spendable.GetSaplingTotal() + spendable.GetOrchardTotal()));
|
||||
LogPrint("zrpc", "%s: total transparent output: %s\n", getId(),
|
||||
FormatMoney(payments.GetTransparentTotal()));
|
||||
LogPrint("zrpcunsafe", "%s: total shielded Sapling output: %s\n", getId(),
|
||||
FormatMoney(payments.GetSaplingTotal()));
|
||||
LogPrint("zrpcunsafe", "%s: total shielded Orchard output: %s\n", getId(),
|
||||
FormatMoney(payments.GetOrchardTotal()));
|
||||
LogPrint("zrpc", "%s: fee: %s\n", getId(), FormatMoney(effects_->GetFee()));
|
||||
|
||||
auto buildResult = effects_->ApproveAndBuild(
|
||||
Params().GetConsensus(),
|
||||
*pwalletMain,
|
||||
chainActive,
|
||||
strategy_);
|
||||
auto buildResult = effects_->ApproveAndBuild(
|
||||
Params().GetConsensus(),
|
||||
wallet,
|
||||
chainActive,
|
||||
strategy_);
|
||||
|
||||
auto tx = buildResult.GetTxOrThrow();
|
||||
auto tx = buildResult.GetTxOrThrow();
|
||||
|
||||
UniValue sendResult = SendTransaction(tx, payments.GetResolvedPayments(), std::nullopt, testmode);
|
||||
set_result(sendResult);
|
||||
UniValue sendResult = SendTransaction(tx, payments.GetResolvedPayments(), std::nullopt, testmode);
|
||||
set_result(sendResult);
|
||||
|
||||
txid = tx.GetHash();
|
||||
txid = tx.GetHash();
|
||||
|
||||
effects_->UnlockSpendable(*pwalletMain);
|
||||
return txid;
|
||||
effects_->UnlockSpendable(wallet);
|
||||
return txid;
|
||||
} catch (...) {
|
||||
effects_->UnlockSpendable(wallet);
|
||||
throw;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
@ -25,9 +25,9 @@ using namespace libzcash;
|
|||
|
||||
/**
|
||||
When estimating the number of coinbase utxos we can shield in a single transaction:
|
||||
1. Joinsplit description is 1802 bytes.
|
||||
1. An Orchard receiver is 9165 bytes.
|
||||
2. Transaction overhead ~ 100 bytes
|
||||
3. Spending a typical P2PKH is >=148 bytes, as defined in CTXIN_SPEND_DUST_SIZE.
|
||||
3. Spending a typical P2PKH is >=148 bytes, as defined in CTXIN_SPEND_P2PKH_SIZE.
|
||||
4. Spending a multi-sig P2SH address can vary greatly:
|
||||
https://github.com/bitcoin/bitcoin/blob/c3ad56f4e0b587d8d763af03d743fdfc2d180c9b/src/main.cpp#L517
|
||||
In real-world coinbase utxos, we consider a 3-of-3 multisig, where the size is roughly:
|
||||
|
@ -40,7 +40,7 @@ When estimating the number of coinbase utxos we can shield in a single transacti
|
|||
|
||||
// transaction.h comment: spending taddr output requires CTxIn >= 148 bytes and
|
||||
// typical taddr txout is 34 bytes
|
||||
#define CTXIN_SPEND_DUST_SIZE 148
|
||||
#define CTXIN_SPEND_P2PKH_SIZE 148
|
||||
#define CTXOUT_REGULAR_SIZE 34
|
||||
|
||||
class Remaining {
|
||||
|
@ -72,7 +72,7 @@ public:
|
|||
AsyncRPCOperation_shieldcoinbase& operator=(AsyncRPCOperation_shieldcoinbase const&) = delete; // Copy assign
|
||||
AsyncRPCOperation_shieldcoinbase& operator=(AsyncRPCOperation_shieldcoinbase &&) = delete; // Move assign
|
||||
|
||||
Remaining prepare();
|
||||
Remaining prepare(CWallet& wallet);
|
||||
|
||||
virtual void main();
|
||||
|
||||
|
@ -93,7 +93,7 @@ private:
|
|||
|
||||
UniValue contextinfo_; // optional data to include in return value from getStatus()
|
||||
|
||||
uint256 main_impl();
|
||||
uint256 main_impl(CWallet& wallet);
|
||||
};
|
||||
|
||||
// To test private methods, a friend class can act as a proxy
|
||||
|
@ -105,8 +105,8 @@ public:
|
|||
|
||||
// Delegated methods
|
||||
|
||||
uint256 main_impl() {
|
||||
return delegate->main_impl();
|
||||
uint256 main_impl(CWallet& wallet) {
|
||||
return delegate->main_impl(wallet);
|
||||
}
|
||||
|
||||
void set_state(OperationStatus state) {
|
||||
|
|
|
@ -4700,10 +4700,6 @@ UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedO
|
|||
return ret;
|
||||
}
|
||||
|
||||
// transaction.h comment: spending taddr output requires CTxIn >= 148 bytes and typical taddr txout is 34 bytes
|
||||
#define CTXIN_SPEND_DUST_SIZE 148
|
||||
#define CTXOUT_REGULAR_SIZE 34
|
||||
|
||||
size_t EstimateTxSize(
|
||||
const ZTXOSelector& ztxoSelector,
|
||||
const std::vector<Payment>& recipients,
|
||||
|
@ -4762,7 +4758,7 @@ size_t EstimateTxSize(
|
|||
CTransaction tx(mtx);
|
||||
txsize += GetSerializeSize(tx, SER_NETWORK, tx.nVersion);
|
||||
if (fromTaddr) {
|
||||
txsize += CTXIN_SPEND_DUST_SIZE;
|
||||
txsize += CTXIN_SPEND_P2PKH_SIZE;
|
||||
txsize += CTXOUT_REGULAR_SIZE; // There will probably be taddr change
|
||||
}
|
||||
txsize += CTXOUT_REGULAR_SIZE * taddrRecipientCount;
|
||||
|
@ -5231,7 +5227,7 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp)
|
|||
params.size() > 4 ? std::optional(params[4].get_str()) : std::nullopt),
|
||||
// This has identical behavior to `AllowRevealedSenders` for this operation, but
|
||||
// technically, this is what “LegacyCompat” means, so just for consistency.
|
||||
PrivacyPolicy::FullPrivacy);
|
||||
PrivacyPolicy::AllowFullyTransparent);
|
||||
|
||||
// Validate the from address
|
||||
auto fromaddress = params[0].get_str();
|
||||
|
@ -5288,17 +5284,7 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp)
|
|||
// Validate the destination address
|
||||
auto destStr = params[1].get_str();
|
||||
auto destaddress = keyIO.DecodePaymentAddress(destStr);
|
||||
if (destaddress.has_value()) {
|
||||
examine(destaddress.value(), match {
|
||||
[](const CKeyID&) {
|
||||
throw JSONRPCError(RPC_VERIFY_REJECTED, "Cannot shield coinbase output to a p2pkh address.");
|
||||
},
|
||||
[](const CScriptID&) {
|
||||
throw JSONRPCError(RPC_VERIFY_REJECTED, "Cannot shield coinbase output to a p2sh address.");
|
||||
},
|
||||
[](const auto&) { },
|
||||
});
|
||||
} else {
|
||||
if (!destaddress.has_value()) {
|
||||
throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, unknown address format: ") + destStr);
|
||||
}
|
||||
|
||||
|
@ -5328,7 +5314,7 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp)
|
|||
auto async_shieldcoinbase =
|
||||
new AsyncRPCOperation_shieldcoinbase(
|
||||
std::move(builder), ztxoSelector, destaddress.value(), strategy, nUTXOLimit, nFee, contextInfo);
|
||||
auto results = async_shieldcoinbase->prepare();
|
||||
auto results = async_shieldcoinbase->prepare(*pwalletMain);
|
||||
|
||||
// Create operation and add to global queue
|
||||
std::shared_ptr<AsyncRPCQueue> q = getAsyncRPCQueue();
|
||||
|
@ -5622,7 +5608,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
|
|||
CAmount nValue = out.tx->vout[out.i].nValue;
|
||||
|
||||
if (!maxedOutUTXOsFlag) {
|
||||
size_t increase = (std::get_if<CScriptID>(&address) != nullptr) ? CTXIN_SPEND_P2SH_SIZE : CTXIN_SPEND_DUST_SIZE;
|
||||
size_t increase = (std::get_if<CScriptID>(&address) != nullptr) ? CTXIN_SPEND_P2SH_SIZE : CTXIN_SPEND_P2PKH_SIZE;
|
||||
if (estimatedTxSize + increase >= max_tx_size ||
|
||||
(mempoolLimit > 0 && utxoCounter > mempoolLimit))
|
||||
{
|
||||
|
|
|
@ -1490,20 +1490,15 @@ BOOST_AUTO_TEST_CASE(rpc_z_shieldcoinbase_parameters)
|
|||
|
||||
// bad from address
|
||||
BOOST_CHECK_THROW(CallRPC("z_shieldcoinbase "
|
||||
"INVALIDtmRr6yJonqGK23UVhrKuyvTpF8qxQQjKigJ "
|
||||
"tnpoQJVnYBZZqkFadj2bJJLThNCxbADGB5gSGeYTAGGrT5tejsxY9Zc1BtY8nnHmZkB"
|
||||
), runtime_error);
|
||||
"INVALIDtmRr6yJonqGK23UVhrKuyvTpF8qxQQjKigJ tnpoQJVnYBZZqkFadj2bJJLThNCxbADGB5gSGeYTAGGrT5tejsxY9Zc1BtY8nnHmZkB"), runtime_error);
|
||||
|
||||
// bad from address
|
||||
BOOST_CHECK_THROW(CallRPC("z_shieldcoinbase "
|
||||
"** tnpoQJVnYBZZqkFadj2bJJLThNCxbADGB5gSGeYTAGGrT5tejsxY9Zc1BtY8nnHmZkB"
|
||||
), runtime_error);
|
||||
"** tnpoQJVnYBZZqkFadj2bJJLThNCxbADGB5gSGeYTAGGrT5tejsxY9Zc1BtY8nnHmZkB"), runtime_error);
|
||||
|
||||
// bad to address
|
||||
BOOST_CHECK_THROW(CallRPC("z_shieldcoinbase "
|
||||
"tmRr6yJonqGK23UVhrKuyvTpF8qxQQjKigJ "
|
||||
"INVALIDtnpoQJVnYBZZqkFadj2bJJLThNCxbADGB5gSGeYTAGGrT5tejsxY9Zc1BtY8nnHmZkB"
|
||||
), runtime_error);
|
||||
"tmRr6yJonqGK23UVhrKuyvTpF8qxQQjKigJ INVALIDtnpoQJVnYBZZqkFadj2bJJLThNCxbADGB5gSGeYTAGGrT5tejsxY9Zc1BtY8nnHmZkB"), runtime_error);
|
||||
|
||||
// invalid fee amount, cannot be negative
|
||||
BOOST_CHECK_THROW(CallRPC("z_shieldcoinbase "
|
||||
|
@ -1521,10 +1516,32 @@ BOOST_AUTO_TEST_CASE(rpc_z_shieldcoinbase_parameters)
|
|||
|
||||
// invalid limit, must be at least 0
|
||||
BOOST_CHECK_THROW(CallRPC("z_shieldcoinbase "
|
||||
"tmRr6yJonqGK23UVhrKuyvTpF8qxQQjKigJ "
|
||||
"tnpoQJVnYBZZqkFadj2bJJLThNCxbADGB5gSGeYTAGGrT5tejsxY9Zc1BtY8nnHmZkB "
|
||||
"100 -1"
|
||||
), runtime_error);
|
||||
"tmRr6yJonqGK23UVhrKuyvTpF8qxQQjKigJ "
|
||||
"tnpoQJVnYBZZqkFadj2bJJLThNCxbADGB5gSGeYTAGGrT5tejsxY9Zc1BtY8nnHmZkB "
|
||||
"100 -1"
|
||||
), runtime_error);
|
||||
|
||||
// Test constructor of AsyncRPCOperation_shieldcoinbase
|
||||
KeyIO keyIO(Params());
|
||||
UniValue retValue;
|
||||
BOOST_CHECK_NO_THROW(retValue = CallRPC("getnewaddress"));
|
||||
auto taddr2 = std::get<CKeyID>(keyIO.DecodePaymentAddress(retValue.get_str()).value());
|
||||
auto testnetzaddr = std::get<libzcash::SproutPaymentAddress>(keyIO.DecodePaymentAddress("ztjiDe569DPNbyTE6TSdJTaSDhoXEHLGvYoUnBU1wfVNU52TEyT6berYtySkd21njAeEoh8fFJUT42kua9r8EnhBaEKqCpP").value());
|
||||
auto selector = pwalletMain->ZTXOSelectorForAddress(
|
||||
taddr2,
|
||||
true,
|
||||
// In the real transaction builder we use either Require or Disallow, but here we
|
||||
// are checking that there are no UTXOs at all, so we allow either to be selected to
|
||||
// confirm this.
|
||||
TransparentCoinbasePolicy::Allow,
|
||||
false).value();
|
||||
WalletTxBuilder builder(Params(), minRelayTxFee);
|
||||
|
||||
try {
|
||||
std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_shieldcoinbase(std::move(builder), selector, testnetzaddr, PrivacyPolicy::AllowRevealedSenders, SHIELD_COINBASE_DEFAULT_LIMIT, 1));
|
||||
} catch (const UniValue& objError) {
|
||||
BOOST_CHECK( find_error(objError, "Empty inputs"));
|
||||
}
|
||||
}
|
||||
|
||||
BOOST_AUTO_TEST_CASE(rpc_z_mergetoaddress_parameters)
|
||||
|
|
|
@ -7777,10 +7777,8 @@ bool ZTXOSelector::SelectsOrchard() const {
|
|||
|
||||
void SpendableInputs::LimitTransparentUtxos(size_t maxUtxoCount)
|
||||
{
|
||||
if (utxos.size() > maxUtxoCount) {
|
||||
// this operation will always shrink the utxos vector; the dummy is
|
||||
// just here to keep the compiler happy.
|
||||
utxos.resize(maxUtxoCount, COutput::Dummy());
|
||||
while (utxos.size() > maxUtxoCount) {
|
||||
utxos.pop_back();
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
@ -757,9 +757,6 @@ public:
|
|||
|
||||
class COutput
|
||||
{
|
||||
private:
|
||||
COutput() { }
|
||||
|
||||
public:
|
||||
const CWalletTx *tx;
|
||||
int i;
|
||||
|
@ -770,10 +767,6 @@ public:
|
|||
COutput(const CWalletTx *txIn, int iIn, int nDepthIn, bool fSpendableIn, bool fIsCoinbaseIn = false) :
|
||||
tx(txIn), i(iIn), nDepth(nDepthIn), fSpendable(fSpendableIn), fIsCoinbase(fIsCoinbaseIn){ }
|
||||
|
||||
static COutput Dummy() {
|
||||
return COutput();
|
||||
}
|
||||
|
||||
CAmount Value() const { return tx->vout[i].nValue; }
|
||||
std::string ToString() const;
|
||||
};
|
||||
|
|
|
@ -157,7 +157,7 @@ WalletTxBuilder::PrepareTransaction(
|
|||
return selected.map([&](const InputSelection& resolvedSelection) {
|
||||
auto ovks = SelectOVKs(wallet, selector, spendable);
|
||||
|
||||
auto effects = TransactionEffects(
|
||||
return TransactionEffects(
|
||||
anchorConfirmations,
|
||||
resolvedSelection.GetInputs(),
|
||||
resolvedSelection.GetPayments(),
|
||||
|
@ -166,8 +166,6 @@ WalletTxBuilder::PrepareTransaction(
|
|||
ovks.first,
|
||||
ovks.second,
|
||||
anchorHeight);
|
||||
effects.LockSpendable(wallet);
|
||||
return effects;
|
||||
});
|
||||
}
|
||||
|
||||
|
|
Loading…
Reference in New Issue