zecfoundation
/
zcashd
mirror of https://github.com/ZcashFoundation/zcashd.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Address z_mergetoaddress review feedback 

This removes a btest case (can’t mix Sprout and Sapling inputs in
z_mergetoaddress) that is no longer testable outside the RPC interface (and
there is an rpc-test for it already).

It also adds a new failure case in `WalletTxBuilder::PrepareTransaction` – we
now check that the fee is in `MoneyRange`. This is generally protected by
`AmountFromValue` in RPC calls, but we have tests that check it at this level,
and better safe than sorry.
This commit is contained in:
Greg Pfeil 2023-04-20 09:58:44 -06:00
parent ac218b697a
commit 3bf966eef2
No known key found for this signature in database
GPG Key ID: 1193ACD196ED61F2
8 changed files with 94 additions and 70 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
src/wallet/asyncrpcoperation_common.cpp
View File

@ -167,6 +167,15 @@ void ThrowInputSelectionError(
"The proposed transaction would result in %s in change.",
FormatMoney(err.available - err.required)));
},
[](const InvalidFeeError& err) {
throw JSONRPCError(
RPC_INVALID_PARAMETER,
strprintf(
"The provided fee, %s, is invalid. Fees must be non-negative, and no greater "
"than the total amount of ZEC that will ever be available.",
DisplayMoney(err.fixedFee),
DisplayMoney(MAX_MONEY)));
},
[](const AbsurdFeeError& err) {
throw JSONRPCError(
RPC_INVALID_PARAMETER,

15
src/wallet/asyncrpcoperation_mergetoaddress.cpp
View File

@ -73,11 +73,14 @@ main_impl(
void AsyncRPCOperation_mergetoaddress::main()
{
if (isCancelled()) {
effects_.UnlockSpendable(*pwalletMain);
return;
}
set_state(OperationStatus::EXECUTING);
start_execution_clock();
bool success = false;
#ifdef ENABLE_MINING
GenerateBitcoins(false, 0, Params());
#endif
@ -145,10 +148,10 @@ main_impl(
bool sendsToShielded = false;
for (const auto& resolvedPayment : payments.GetResolvedPayments()) {
sendsToShielded = sendsToShielded || examine(resolvedPayment.address, match {
[](const CKeyID&) { return true; },
[](const CScriptID&) { return true; },
[](const libzcash::SaplingPaymentAddress&) { return false; },
[](const libzcash::OrchardRawAddress&) { return false; },
[](const CKeyID&) { return false; },
[](const CScriptID&) { return false; },
[](const libzcash::SaplingPaymentAddress&) { return true; },
[](const libzcash::OrchardRawAddress&) { return true; },
});
}
if (spendable.sproutNoteEntries.empty()

5
src/wallet/asyncrpcoperation_shieldcoinbase.cpp
View File

@ -82,6 +82,11 @@ AsyncRPCOperation_shieldcoinbase::~AsyncRPCOperation_shieldcoinbase() {
}
void AsyncRPCOperation_shieldcoinbase::main() {
if (isCancelled()) {
effects_.value().UnlockSpendable(*pwalletMain);
return;
}
set_state(OperationStatus::EXECUTING);
start_execution_clock();

72
src/wallet/gtest/test_rpc_wallet.cpp
View File

@ -57,8 +57,11 @@ TEST(WalletRPCTests, RPCZMergeToAddressInternals)
std::string taddr_string = keyIO.EncodeDestination(taddr);
NetAmountRecipient taddr1(keyIO.DecodePaymentAddress(taddr_string).value(), Memo());
auto pa = pwalletMain->GenerateNewSproutZKey();
NetAmountRecipient zaddr1(pa, Memo());
auto sproutKey = pwalletMain->GenerateNewSproutZKey();
NetAmountRecipient zaddr1(sproutKey, Memo());
auto saplingKey = pwalletMain->GenerateNewLegacySaplingZKey();
NetAmountRecipient zaddr2(saplingKey, Memo());
WalletTxBuilder builder(Params(), minRelayTxFee);
auto selector = CWallet::LegacyTransparentZTXOSelector(
@ -66,30 +69,49 @@ TEST(WalletRPCTests, RPCZMergeToAddressInternals)
TransparentCoinbasePolicy::Disallow);
TransactionStrategy strategy(PrivacyPolicy::AllowRevealedSenders);
SpendableInputs inputs;
auto wtx = FakeWalletTx();
inputs.utxos.emplace_back(COutput(&wtx, 0, 100, true));
// Can’t send to Sprout
builder.PrepareTransaction(
*pwalletMain,
selector,
inputs,
zaddr1,
chainActive,
strategy,
0,
1)
.map_error([](const auto& err) {
EXPECT_TRUE(examine(err, match {
[](const AddressResolutionError& are) {
return are == AddressResolutionError::SproutRecipientsNotSupported;
},
[](const auto&) { return false; },
}));
})
.map([](const auto&) { EXPECT_TRUE(false); });
// Insufficient funds
{
SpendableInputs inputs;
auto wtx = FakeWalletTx();
inputs.utxos.emplace_back(COutput(&wtx, 0, 100, true));
builder.PrepareTransaction(
*pwalletMain,
selector,
inputs,
zaddr1,
chainActive,
strategy,
0,
1)
.map_error([](const auto& err) {
EXPECT_TRUE(examine(err, match {
[](const AddressResolutionError& are) {
return are == AddressResolutionError::SproutRecipientsNotSupported;
},
[](const auto&) { return false; },
}));
})
.map([](const auto&) { EXPECT_TRUE(false); });
}
builder.PrepareTransaction(
*pwalletMain,
selector,
inputs,
zaddr2,
chainActive,
strategy,
std::nullopt,
1)
.map_error([](const auto& err) {
EXPECT_TRUE(examine(err, match {
[](const InvalidFundsError& ife) {
return std::holds_alternative<InsufficientFundsError>(ife.reason);
},
[](const auto&) { return false; },
}));
})
.map([](const auto&) { EXPECT_TRUE(false); });
}
UnloadGlobalWallet();
}

10
src/wallet/rpcwallet.cpp
View File

@ -5544,13 +5544,13 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
int sproutNoteLimit = MERGE_TO_ADDRESS_DEFAULT_SPROUT_LIMIT;
int saplingNoteLimit = MERGE_TO_ADDRESS_DEFAULT_SAPLING_LIMIT;
int shieldedNoteLimit = 0;
if (params.size() > 4) {
int nNoteLimit = params[4].get_int();
if (nNoteLimit < 0) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Limit on maximum number of notes cannot be negative");
}
shieldedNoteLimit = nNoteLimit;
sproutNoteLimit = nNoteLimit;
saplingNoteLimit = nNoteLimit;
}
std::optional<Memo> memo;
@ -5675,8 +5675,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
size_t increase = allInputs.sproutNoteEntries.empty() || allInputs.sproutNoteEntries.size() % 2 == 0 ?
JOINSPLIT_SIZE(SAPLING_TX_VERSION) : 0;
if (estimatedTxSize + increase >= max_tx_size ||
(sproutNoteLimit > 0 && noteCounter > sproutNoteLimit) ||
(shieldedNoteLimit > 0 && noteCounter > shieldedNoteLimit))
(sproutNoteLimit > 0 && noteCounter > sproutNoteLimit))
{
maxedOutNotesFlag = true;
} else {
@ -5700,8 +5699,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
if (!maxedOutNotesFlag) {
size_t increase = SPENDDESCRIPTION_SIZE;
if (estimatedTxSize + increase >= max_tx_size ||
(saplingNoteLimit > 0 && noteCounter > saplingNoteLimit) ||
(shieldedNoteLimit > 0 && noteCounter > shieldedNoteLimit))
(saplingNoteLimit > 0 && noteCounter > saplingNoteLimit))
{
maxedOutNotesFlag = true;
} else {

34
src/wallet/test/rpc_wallet_tests.cpp
View File

@ -1634,9 +1634,8 @@ BOOST_AUTO_TEST_CASE(rpc_z_mergetoaddress_parameters)
KeyIO keyIO(Params());
WalletTxBuilder builder(Params(), minRelayTxFee);
NetAmountRecipient testnetzaddr(
keyIO.DecodePaymentAddress("ztjiDe569DPNbyTE6TSdJTaSDhoXEHLGvYoUnBU1wfVNU52TEyT6berYtySkd21njAeEoh8fFJUT42kua9r8EnhBaEKqCpP").value(),
Memo());
auto saplingKey = pwalletMain->GenerateNewLegacySaplingZKey();
NetAmountRecipient testnetzaddr(saplingKey, Memo());
auto selector = CWallet::LegacyTransparentZTXOSelector(
true,
TransparentCoinbasePolicy::Disallow);
@ -1645,13 +1644,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_mergetoaddress_parameters)
builder.PrepareTransaction(*pwalletMain, selector, {}, testnetzaddr, chainActive, strategy, -1, 1)
.map_error([&](const auto& err) {
// TODO: Provide `operator==` on `InputSelectionError` and use that here.
BOOST_CHECK(examine(err, match {
[](AddressResolutionError are) {
// FIXME: Should be failing because of negative fee – change recipient address to not be Sprout.
return are == AddressResolutionError::SproutRecipientsNotSupported;
},
[](const auto&) { return false; },
}));
BOOST_CHECK(std::holds_alternative<InvalidFeeError>(err));
})
.map([](const auto&) {
BOOST_FAIL("Fee value of -1 expected to be out of the valid range of values.");
@ -1670,27 +1663,6 @@ BOOST_AUTO_TEST_CASE(rpc_z_mergetoaddress_parameters)
.map([](const auto&) {
BOOST_FAIL("Expected an error.");
});
auto wtx = FakeWalletTx();
SpendableInputs inputs;
inputs.utxos.emplace_back(COutput(&wtx, 0, 100, true));
inputs.sproutNoteEntries.emplace_back(SproutNoteEntry {JSOutPoint(), SproutPaymentAddress(), SproutNote(), "", 0});
inputs.saplingNoteEntries.emplace_back(SaplingNoteEntry {SaplingOutPoint(), SaplingPaymentAddress(), SaplingNote({}, uint256(), 0, uint256(), Zip212Enabled::BeforeZip212), "", 0});
builder.PrepareTransaction(*pwalletMain, selector, inputs, testnetzaddr, chainActive, strategy, 1, 1)
.map_error([&](const auto& err) {
// TODO: Provide `operator==` on `InputSelectionError` and use that here.
BOOST_CHECK(examine(err, match {
[](AddressResolutionError are) {
// FIXME: Should be failing because of insufficient funds – change recipient address to not be Sprout.
return are == AddressResolutionError::SproutRecipientsNotSupported;
},
[](const auto&) { return false; },
}));
})
.map([](const auto&) {
BOOST_FAIL("Expected an error.");
});
}
void TestWTxStatus(const Consensus::Params consensusParams, const int delta) {

9
src/wallet/wallet_tx_builder.cpp
View File

@ -392,8 +392,13 @@ WalletTxBuilder::PrepareTransaction(
const std::optional<CAmount>& fee,
uint32_t anchorConfirmations) const
{
if (fee.has_value() && maxTxFee < fee.value()) {
return tl::make_unexpected(MaxFeeError(fee.value()));
if (fee.has_value()) {
if (maxTxFee < fee.value()) {
return tl::make_unexpected(MaxFeeError(fee.value()));
} else if (!MoneyRange(fee.value())) {
// TODO: This check will be obviated by #6579.
return tl::make_unexpected(InvalidFeeError(fee.value()));
}
}
int anchorHeight = GetAnchorHeight(chain, anchorConfirmations);

10
src/wallet/wallet_tx_builder.h
View File

@ -300,6 +300,15 @@ public:
available(available), required(required) { }
};
/// Error when a fee is outside `MoneyRange`
class InvalidFeeError {
public:
CAmount fixedFee;
InvalidFeeError(CAmount fixedFee):
fixedFee(fixedFee) { }
};
/// Error when a fee is higher than can be useful. This reduces the chance of accidentally
/// overpaying with explicit fees.
class AbsurdFeeError {
@ -340,6 +349,7 @@ typedef std::variant<
AddressResolutionError,
InvalidFundsError,
ChangeNotAllowedError,
InvalidFeeError,
AbsurdFeeError,
MaxFeeError,
ExcessOrchardActionsError> InputSelectionError;