From c585d57fec7cf489dca0791e821369c5c4acfe36 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 3 Jun 2020 07:23:10 -0400 Subject: [PATCH 1/9] Merge #18875: fuzz: Stop nodes in process_message* fuzzers fab860aed4878b831dae463e1ee68029b66210f5 fuzz: Stop nodes in process_message* fuzzers (MarcoFalke) 6666c828e072a5e99ea0c16394ca3e5b9de07409 fuzz: Give CNode ownership to ConnmanTestMsg in process_message fuzz harness (MarcoFalke) Pull request description: Background is that I saw an integer overflow in net_processing ``` #30629113 REDUCE cov: 25793 ft: 142917 corp: 3421/2417Kb lim: 4096 exec/s: 89 rss: 614Mb L: 1719/4096 MS: 1 EraseBytes- net_processing.cpp:977:25: runtime error: signed integer overflow: 2147483624 + 100 cannot be represented in type 'int' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior net_processing.cpp:977:25 in net_processing.cpp:985:9: runtime error: signed integer overflow: -2147483572 - 100 cannot be represented in type 'int' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior net_processing.cpp:985:9 in ``` Telling from the line numbers, it looks like `nMisbehavior` wrapped around. Fix that by calling `StopNodes` after each exec, which should clear the node state and thus `nMisbehavior`. ACKs for top commit: practicalswift: ACK fab860aed4878b831dae463e1ee68029b66210f5 Tree-SHA512: 891c081d5843565d891aec028b6c27ef3fa39bc40ae78238e81d8f784b4d4b49cb870998574725a5159dd03aeeb2e0b9bc3d3bb51d57d1231ef42e3394b2d639 --- src/test/fuzz/process_message.cpp | 7 ++++++- src/test/fuzz/process_messages.cpp | 3 ++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index f377fcc360..7e5cb73397 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -59,15 +60,17 @@ void initialize_process_message() void fuzz_target(const std::vector& buffer, const std::string& LIMIT_TO_MESSAGE_TYPE) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + ConnmanTestMsg& connman = *(ConnmanTestMsg*)g_setup->m_node.connman.get(); const std::string random_message_type{fuzzed_data_provider.ConsumeBytesAsString(CMessageHeader::COMMAND_SIZE).c_str()}; if (!LIMIT_TO_MESSAGE_TYPE.empty() && random_message_type != LIMIT_TO_MESSAGE_TYPE) { return; } CDataStream random_bytes_data_stream{fuzzed_data_provider.ConsumeRemainingBytes(), SER_NETWORK, PROTOCOL_VERSION}; - CNode p2p_node{0, ServiceFlags(NODE_NETWORK | NODE_BLOOM), INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, false}; + CNode& p2p_node = *std::make_unique(0, ServiceFlags(NODE_NETWORK | NODE_BLOOM), INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, false).release(); p2p_node.fSuccessfullyConnected = true; p2p_node.nVersion = PROTOCOL_VERSION; p2p_node.SetSendVersion(PROTOCOL_VERSION); + connman.AddTestNode(p2p_node); g_setup->m_node.peerman->InitializeNode(&p2p_node); try { g_setup->m_node.peerman->ProcessMessage(p2p_node, random_message_type, random_bytes_data_stream, GetTimeMillis(), std::atomic{false}); @@ -78,6 +81,8 @@ void fuzz_target(const std::vector& buffer, const std::string& LIMIT_TO g_setup->m_node.peerman->SendMessages(&p2p_node); } SyncWithValidationInterfaceQueue(); + LOCK2(::cs_main, g_cs_orphans); // See init.cpp for rationale for implicit locking order requirement + g_setup->m_node.connman->StopNodes(); } FUZZ_TARGET_INIT(process_message, initialize_process_message) { fuzz_target(buffer, ""); } diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 8d43502be6..83903cf145 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -77,6 +77,7 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages) g_setup->m_node.peerman->SendMessages(&random_node); } } - connman.ClearTestNodes(); SyncWithValidationInterfaceQueue(); + LOCK2(::cs_main, g_cs_orphans); // See init.cpp for rationale for implicit locking order requirement + g_setup->m_node.connman->StopNodes(); } From 2c5cb249be9ab8c048c3fed00811c3b302f7c5e2 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 25 Jun 2020 19:25:46 +0200 Subject: [PATCH 2/9] Merge #11413: [wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option 25dac9fa65243ca8db02df22f484039c08114401 doc: add release notes for explicit fee estimators and bumpfee change (Karl-Johan Alm) 05227a35545d7656450874b3668bf418c73813fb tests for bumpfee / estimate_modes (Karl-Johan Alm) 3404c1b753432c4859a4ca245f01c240610a00cb policy: optional FeeEstimateMode param to CFeeRate::ToString (Karl-Johan Alm) 6fcf4484302d13bd7739b617470d8c8e31974908 rpc/wallet: add two explicit modes to estimate_mode (Karl-Johan Alm) b188d80c2de9ebb114da5ceea78baa46bde7dff6 MOVEONLY: Make FeeEstimateMode available to CFeeRate (Karl-Johan Alm) 5d1a411eb12fc700804ffe5d6e205234d30edd5f fees: add FeeModes doc helper function (Karl-Johan Alm) 91f6d2bc8ff4d4cd1b86daa370ec9d2d9662394d rpc/wallet: add conf_target as alias to confTarget in bumpfee (Karl-Johan Alm) 69158b41fc488e4f220559da17a475eff5923a95 added CURRENCY_ATOM to express minimum indivisible unit (Karl-Johan Alm) Pull request description: This lets users pick their own fees when using `sendtoaddress`/`sendmany` if they prefer this over the estimators. ACKs for top commit: Sjors: re-utACK 25dac9fa65: rebased, more fancy C++, jonatack: ACK 25dac9fa65243ca8db02df2 I think this should be merged after all this time, even though it looks to me like there are needed follow-ups, fixes and test coverage to be added (see further down), which I don't mind helping out with, if wanted. fjahr: Code review ACK 25dac9fa65243ca8db02df22f484039c08114401 Tree-SHA512: f31177e6cabf3187a43cdfe93477144f8e8385c7344613743cbbd16e8490d53ff5144aec7b9de6c9a65eb855b55e0f99d7f164dee4b6bf3cfea4dce51cf11d33 --- doc/release-notes-11413.md | 7 ++ src/policy/feerate.cpp | 9 +-- src/policy/feerate.h | 14 +++- src/policy/fees.h | 7 -- src/util/fees.cpp | 44 ++++++++---- src/util/fees.h | 1 + src/wallet/rpcwallet.cpp | 113 ++++++++++++++++------------- test/functional/wallet_basic.py | 123 +++++++++++++++++++++++++++++++- 8 files changed, 241 insertions(+), 77 deletions(-) create mode 100644 doc/release-notes-11413.md diff --git a/doc/release-notes-11413.md b/doc/release-notes-11413.md new file mode 100644 index 0000000000..165fbc8833 --- /dev/null +++ b/doc/release-notes-11413.md @@ -0,0 +1,7 @@ +Updated or changed RPC +---------------------- + +The `fundrawtransaction`, `sendmany`, `sendtoaddress`, and `walletcreatefundedpsbt` +RPC commands have been updated to include two new fee estimation methods "DASH/kB" and "duff/B". +The target is the fee expressed explicitly in the given form. +In addition, the `estimate_mode` parameter is now case insensitive for all of the above RPC commands. diff --git a/src/policy/feerate.cpp b/src/policy/feerate.cpp index 60894ae8bf..b81d4732ad 100644 --- a/src/policy/feerate.cpp +++ b/src/policy/feerate.cpp @@ -7,8 +7,6 @@ #include -const std::string CURRENCY_UNIT = "DASH"; - CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nBytes_) { assert(nBytes_ <= uint64_t(std::numeric_limits::max())); @@ -37,7 +35,10 @@ CAmount CFeeRate::GetFee(size_t nBytes_) const return nFee; } -std::string CFeeRate::ToString() const +std::string CFeeRate::ToString(const FeeEstimateMode& fee_estimate_mode) const { - return strprintf("%d.%08d %s/kB", nSatoshisPerK / COIN, nSatoshisPerK % COIN, CURRENCY_UNIT); + switch (fee_estimate_mode) { + case FeeEstimateMode::DUFF_B: return strprintf("%d.%03d %s/B", nSatoshisPerK / 1000, nSatoshisPerK % 1000, CURRENCY_ATOM); + default: return strprintf("%d.%08d %s/kB", nSatoshisPerK / COIN, nSatoshisPerK % COIN, CURRENCY_UNIT); + } } diff --git a/src/policy/feerate.h b/src/policy/feerate.h index 386ef05fbd..7db4486560 100644 --- a/src/policy/feerate.h +++ b/src/policy/feerate.h @@ -11,7 +11,17 @@ #include -extern const std::string CURRENCY_UNIT; +const std::string CURRENCY_UNIT = "DASH"; // One formatted unit +const std::string CURRENCY_ATOM = "duff"; // One indivisible minimum value unit + +/* Used to determine type of fee estimation requested */ +enum class FeeEstimateMode { + UNSET, //!< Use default settings based on other criteria + ECONOMICAL, //!< Force estimateSmartFee to use non-conservative estimates + CONSERVATIVE, //!< Force estimateSmartFee to use conservative estimates + DASH_KB, //!< Use explicit DASH/kB fee given in coin control + DUFF_B, //!< Use explicit duff/B fee given in coin control +}; /** * Fee rate in satoshis per kilobyte: CAmount / kB @@ -46,7 +56,7 @@ public: friend bool operator>=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK >= b.nSatoshisPerK; } friend bool operator!=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK != b.nSatoshisPerK; } CFeeRate& operator+=(const CFeeRate& a) { nSatoshisPerK += a.nSatoshisPerK; return *this; } - std::string ToString() const; + std::string ToString(const FeeEstimateMode& fee_estimate_mode = FeeEstimateMode::DASH_KB) const; SERIALIZE_METHODS(CFeeRate, obj) { READWRITE(obj.nSatoshisPerK); } }; diff --git a/src/policy/fees.h b/src/policy/fees.h index aae93a6e4b..674be6d837 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -45,13 +45,6 @@ enum class FeeReason { REQUIRED, }; -/* Used to determine type of fee estimation requested */ -enum class FeeEstimateMode { - UNSET, //!< Use default settings based on other criteria - ECONOMICAL, //!< Force estimateSmartFee to use non-conservative estimates - CONSERVATIVE, //!< Force estimateSmartFee to use conservative estimates -}; - /* Used to return detailed information about a feerate bucket */ struct EstimatorBucket { diff --git a/src/util/fees.cpp b/src/util/fees.cpp index 41149888d7..ae32f79b14 100644 --- a/src/util/fees.cpp +++ b/src/util/fees.cpp @@ -6,11 +6,16 @@ #include #include +#include +#include #include #include +#include +#include -std::string StringForFeeReason(FeeReason reason) { +std::string StringForFeeReason(FeeReason reason) +{ static const std::map fee_reason_strings = { {FeeReason::NONE, "None"}, {FeeReason::HALF_ESTIMATE, "Half Target 60% Threshold"}, @@ -29,16 +34,31 @@ std::string StringForFeeReason(FeeReason reason) { return reason_string->second; } -bool FeeModeFromString(const std::string& mode_string, FeeEstimateMode& fee_estimate_mode) { - static const std::map fee_modes = { - {"UNSET", FeeEstimateMode::UNSET}, - {"ECONOMICAL", FeeEstimateMode::ECONOMICAL}, - {"CONSERVATIVE", FeeEstimateMode::CONSERVATIVE}, +const std::vector>& FeeModeMap() +{ + static const std::vector> FEE_MODES = { + {"unset", FeeEstimateMode::UNSET}, + {"economical", FeeEstimateMode::ECONOMICAL}, + {"conservative", FeeEstimateMode::CONSERVATIVE}, + {(CURRENCY_UNIT + "/kB"), FeeEstimateMode::DASH_KB}, + {(CURRENCY_ATOM + "/B"), FeeEstimateMode::DUFF_B}, }; - auto mode = fee_modes.find(mode_string); - - if (mode == fee_modes.end()) return false; - - fee_estimate_mode = mode->second; - return true; + return FEE_MODES; +} + +std::string FeeModes(const std::string& delimiter) +{ + return Join(FeeModeMap(), delimiter, [&](const std::pair& i) { return i.first; }); +} + +bool FeeModeFromString(const std::string& mode_string, FeeEstimateMode& fee_estimate_mode) +{ + auto searchkey = ToUpper(mode_string); + for (const auto& pair : FeeModeMap()) { + if (ToUpper(pair.first) == searchkey) { + fee_estimate_mode = pair.second; + return true; + } + } + return false; } diff --git a/src/util/fees.h b/src/util/fees.h index fc355ce9c2..0109f4dc39 100644 --- a/src/util/fees.h +++ b/src/util/fees.h @@ -12,5 +12,6 @@ enum class FeeReason; bool FeeModeFromString(const std::string& mode_string, FeeEstimateMode& fee_estimate_mode); std::string StringForFeeReason(FeeReason reason); +std::string FeeModes(const std::string& delimiter); #endif // BITCOIN_UTIL_FEES_H diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 7c3c95c148..4a6fb75be5 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -51,6 +51,8 @@ using interfaces::FoundBlock; static const std::string WALLET_ENDPOINT_BASE = "/wallet/"; +static const uint32_t WALLET_DASH_KB_TO_DUFF_B = COIN / 1000; // 1 duff / B = 0.00001 DASH / kB + static inline bool GetAvoidReuseFlag(const CWallet* const pwallet, const UniValue& param) { bool can_avoid_reuse = pwallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE); bool avoid_reuse = param.isNull() ? can_avoid_reuse : param.get_bool(); @@ -190,6 +192,7 @@ static void WalletTxToJSON(interfaces::Chain& chain, const CWalletTx& wtx, UniVa entry.pushKV(item.first, item.second); } + static std::string LabelFromValue(const UniValue& value) { std::string label = value.get_str(); @@ -198,6 +201,40 @@ static std::string LabelFromValue(const UniValue& value) return label; } +/** + * Update coin control with fee estimation based on the given parameters + * + * @param[in] pwallet Wallet pointer + * @param[in,out] cc Coin control which is to be updated + * @param[in] estimate_mode String value (e.g. "ECONOMICAL") + * @param[in] estimate_param Parameter (blocks to confirm, explicit fee rate, etc) + * @throws a JSONRPCError if estimate_mode is unknown, or if estimate_param is missing when required + */ +static void SetFeeEstimateMode(const CWallet* pwallet, CCoinControl& cc, const UniValue& estimate_mode, const UniValue& estimate_param) +{ + if (!estimate_mode.isNull()) { + if (!FeeModeFromString(estimate_mode.get_str(), cc.m_fee_mode)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter"); + } + } + + if (cc.m_fee_mode == FeeEstimateMode::DASH_KB || cc.m_fee_mode == FeeEstimateMode::DUFF_B) { + if (estimate_param.isNull()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Selected estimate_mode requires a fee rate"); + } + + CAmount fee_rate = AmountFromValue(estimate_param); + if (cc.m_fee_mode == FeeEstimateMode::DUFF_B) { + fee_rate /= WALLET_DASH_KB_TO_DUFF_B; + } + + cc.m_feerate = CFeeRate(fee_rate); + + } else if (!estimate_param.isNull()) { + cc.m_confirm_target = ParseConfirmTarget(estimate_param, pwallet->chain().estimateMaxBlocks()); + } +} + UniValue getnewaddress(const JSONRPCRequest& request) { RPCHelpMan{"getnewaddress", @@ -371,11 +408,9 @@ static UniValue sendtoaddress(const JSONRPCRequest& request) " The recipient will receive less amount of Dash than you enter in the amount field."}, {"use_is", RPCArg::Type::BOOL, /* default */ "false", "Deprecated and ignored"}, {"use_cj", RPCArg::Type::BOOL, /* default */ "false", "Use CoinJoin funds only"}, - {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks)"}, - {"estimate_mode", RPCArg::Type::STR, /* default */ "UNSET", "The fee estimate mode, must be one of:\n" - " \"UNSET\"\n" - " \"ECONOMICAL\"\n" - " \"CONSERVATIVE\""}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"}, + {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" + " \"" + FeeModes("\"\n\"") + "\""}, {"avoid_reuse", RPCArg::Type::BOOL, /* default */ "true", "(only available if avoid_reuse wallet flag is set) Avoid spending from dirty addresses; addresses are considered\n" " dirty if they have previously been used in a transaction."}, }, @@ -386,6 +421,8 @@ static UniValue sendtoaddress(const JSONRPCRequest& request) HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1") + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1 \"donation\" \"seans outpost\"") + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1 \"\" \"\" true") + + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1 \"\" \"\" false true 0.00002 " + (CURRENCY_UNIT + "/kB")) + + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1 \"\" \"\" false true 2 " + (CURRENCY_ATOM + "/B")) + HelpExampleRpc("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\", 0.1, \"donation\", \"seans outpost\"") }, }.Check(request); @@ -424,20 +461,13 @@ static UniValue sendtoaddress(const JSONRPCRequest& request) coin_control.UseCoinJoin(request.params[6].get_bool()); } - if (!request.params[7].isNull()) { - coin_control.m_confirm_target = ParseConfirmTarget(request.params[7], pwallet->chain().estimateMaxBlocks()); - } - - if (!request.params[8].isNull()) { - if (!FeeModeFromString(request.params[8].get_str(), coin_control.m_fee_mode)) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter"); - } - } coin_control.m_avoid_address_reuse = GetAvoidReuseFlag(pwallet, request.params[9]); // We also enable partial spend avoidance if reuse avoidance is set. coin_control.m_avoid_partial_spends |= coin_control.m_avoid_address_reuse; + SetFeeEstimateMode(pwallet, coin_control, request.params[8], request.params[7]); + EnsureWalletIsUnlocked(pwallet); CTransactionRef tx = SendMoney(pwallet, dest, nAmount, fSubtractFeeFromAmount, coin_control, std::move(mapValue)); @@ -852,11 +882,9 @@ static UniValue sendmany(const JSONRPCRequest& request) }, {"use_is", RPCArg::Type::BOOL, /* default */ "false", "Deprecated and ignored"}, {"use_cj", RPCArg::Type::BOOL, /* default */ "false", "Use CoinJoin funds only"}, - {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks)"}, - {"estimate_mode", RPCArg::Type::STR, /* default */ "UNSET", "The fee estimate mode, must be one of:\n" - " \"UNSET\"\n" - " \"ECONOMICAL\"\n" - " \"CONSERVATIVE\""}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"}, + {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" + " \"" + FeeModes("\"\n\"") + "\""}, }, RPCResult{ RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of\n" @@ -902,15 +930,7 @@ static UniValue sendmany(const JSONRPCRequest& request) coin_control.UseCoinJoin(request.params[7].get_bool()); } - if (!request.params[8].isNull()) { - coin_control.m_confirm_target = ParseConfirmTarget(request.params[8], pwallet->chain().estimateMaxBlocks()); - } - - if (!request.params[9].isNull()) { - if (!FeeModeFromString(request.params[9].get_str(), coin_control.m_fee_mode)) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter"); - } - } + SetFeeEstimateMode(pwallet, coin_control, request.params[9], request.params[8]); if (coin_control.IsUsingCoinJoin()) { mapValue["DS"] = "1"; @@ -3254,26 +3274,21 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f if (options.exists("feeRate")) { + if (options.exists("conf_target")) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both conf_target and feeRate"); + } + if (options.exists("estimate_mode")) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both estimate_mode and feeRate"); + } coinControl.m_feerate = CFeeRate(AmountFromValue(options["feeRate"])); coinControl.fOverrideFeeRate = true; } if (options.exists("subtractFeeFromOutputs")) subtractFeeFromOutputs = options["subtractFeeFromOutputs"].get_array(); - if (options.exists("conf_target")) { - if (options.exists("feeRate")) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both conf_target and feeRate"); - } - coinControl.m_confirm_target = ParseConfirmTarget(options["conf_target"], pwallet->chain().estimateMaxBlocks()); - } - if (options.exists("estimate_mode")) { - if (options.exists("feeRate")) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both estimate_mode and feeRate"); - } - if (!FeeModeFromString(options["estimate_mode"].get_str(), coinControl.m_fee_mode)) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter"); - } - } + + SetFeeEstimateMode(pwallet, coinControl, options["estimate_mode"], options["conf_target"]); + } } else { // if options is null and not a bool @@ -3337,11 +3352,9 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request) {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."}, }, }, - {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks)"}, - {"estimate_mode", RPCArg::Type::STR, /* default */ "UNSET", "The fee estimate mode, must be one of:\n" - " \"UNSET\"\n" - " \"ECONOMICAL\"\n" - " \"CONSERVATIVE\""}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"}, + {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" + " \"" + FeeModes("\"\n\"") + "\""}, }, "options"}, }, @@ -4081,10 +4094,8 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request) }, }, {"conf_target", RPCArg::Type::NUM, /* default */ "Fallback to wallet's confirmation target", "Confirmation target (in blocks)"}, - {"estimate_mode", RPCArg::Type::STR, /* default */ "UNSET", "The fee estimate mode, must be one of:\n" - " \"UNSET\"\n" - " \"ECONOMICAL\"\n" - " \"CONSERVATIVE\""}, + {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" + " \"" + FeeModes("\"\n\"") + "\""}, }, "options"}, {"bip32derivs", RPCArg::Type::BOOL, /* default */ "true", "Include BIP 32 derivation paths for public keys if we know them"}, diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index a44483af08..377333cb14 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -223,7 +223,60 @@ class WalletTest(BitcoinTestFramework): self.start_node(3) self.connect_nodes(0, 3) - self.sync_all() + # Sendmany with explicit fee (DASH/kB) + # Throw if no conf_target provided + assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", + self.nodes[2].sendmany, + amounts={ address: 10 }, + estimate_mode='dash/kB') + # Throw if negative feerate + assert_raises_rpc_error(-3, "Amount out of range", + self.nodes[2].sendmany, + amounts={ address: 10 }, + conf_target=-1, + estimate_mode='dash/kB') + fee_per_kb = 0.0002500 + explicit_fee_per_byte = Decimal(fee_per_kb) / 1000 + txid = self.nodes[2].sendmany( + amounts={ address: 10 }, + conf_target=fee_per_kb, + estimate_mode='dash/kB', + ) + self.nodes[2].generate(1) + self.sync_all(self.nodes[0:3]) + node_2_bal = self.check_fee_amount(self.nodes[2].getbalance(), node_2_bal - Decimal('10'), explicit_fee_per_byte, count_bytes(self.nodes[2].gettransaction(txid)['hex'])) + assert_equal(self.nodes[2].getbalance(), node_2_bal) + node_0_bal += Decimal('10') + assert_equal(self.nodes[0].getbalance(), node_0_bal) + + # Sendmany with explicit fee (DUFF/B) + # Throw if no conf_target provided + assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", + self.nodes[2].sendmany, + amounts={ address: 10 }, + estimate_mode='duff/b') + # Throw if negative feerate + assert_raises_rpc_error(-3, "Amount out of range", + self.nodes[2].sendmany, + amounts={ address: 10 }, + conf_target=-1, + estimate_mode='duff/b') + fee_duff_per_b = 2 + fee_per_kb = fee_duff_per_b / 100000.0 + explicit_fee_per_byte = Decimal(fee_per_kb) / 1000 + txid = self.nodes[2].sendmany( + amounts={ address: 10 }, + conf_target=fee_duff_per_b, + estimate_mode='duff/b', + ) + self.nodes[2].generate(1) + self.sync_all(self.nodes[0:3]) + balance = self.nodes[2].getbalance() + node_2_bal = self.check_fee_amount(balance, node_2_bal - Decimal('10'), explicit_fee_per_byte, count_bytes(self.nodes[2].gettransaction(txid)['hex'])) + assert_equal(balance, node_2_bal) + node_0_bal += Decimal('10') + assert_equal(self.nodes[0].getbalance(), node_0_bal) + # check if we can list zero value tx as available coins # 1. create raw_tx @@ -349,6 +402,74 @@ class WalletTest(BitcoinTestFramework): self.nodes[0].generate(1) self.sync_all(self.nodes[0:3]) + # send with explicit dash/kb fee + self.log.info("test explicit fee (sendtoaddress as dash/kb)") + self.nodes[0].generate(1) + self.sync_all(self.nodes[0:3]) + prebalance = self.nodes[2].getbalance() + assert prebalance > 2 + address = self.nodes[1].getnewaddress() + # Throw if no conf_target provided + assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", + self.nodes[2].sendtoaddress, + address=address, + amount=1.0, + estimate_mode='dash/Kb') + # Throw if negative feerate + assert_raises_rpc_error(-3, "Amount out of range", + self.nodes[2].sendtoaddress, + address=address, + amount=1.0, + conf_target=-1, + estimate_mode='dash/kb') + txid = self.nodes[2].sendtoaddress( + address=address, + amount=1.0, + conf_target=0.00002500, + estimate_mode='dash/kb', + ) + tx_size = count_bytes(self.nodes[2].gettransaction(txid)['hex']) + self.sync_all(self.nodes[0:3]) + self.nodes[0].generate(1) + self.sync_all(self.nodes[0:3]) + postbalance = self.nodes[2].getbalance() + fee = prebalance - postbalance - Decimal('1') + assert_fee_amount(fee, tx_size, Decimal('0.00002500')) + + # send with explicit duff/b fee + self.sync_all(self.nodes[0:3]) + self.log.info("test explicit fee (sendtoaddress as duff/b)") + self.nodes[0].generate(1) + prebalance = self.nodes[2].getbalance() + assert prebalance > 2 + address = self.nodes[1].getnewaddress() + # Throw if no conf_target provided + assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", + self.nodes[2].sendtoaddress, + address=address, + amount=1.0, + estimate_mode='duff/b') + # Throw if negative feerate + assert_raises_rpc_error(-3, "Amount out of range", + self.nodes[2].sendtoaddress, + address=address, + amount=1.0, + conf_target=-1, + estimate_mode='duff/b') + txid = self.nodes[2].sendtoaddress( + address=address, + amount=1.0, + conf_target=2, + estimate_mode='duff/B', + ) + tx_size = count_bytes(self.nodes[2].gettransaction(txid)['hex']) + self.sync_all(self.nodes[0:3]) + self.nodes[0].generate(1) + self.sync_all(self.nodes[0:3]) + postbalance = self.nodes[2].getbalance() + fee = prebalance - postbalance - Decimal('1') + assert_fee_amount(fee, tx_size, Decimal('0.00002000')) + # 2. Import address from node2 to node1 self.nodes[1].importaddress(address_to_import) From c0c00f92953d9e3732d5ac7e7ae3b882e61d7fb7 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 21 May 2020 20:36:16 +0200 Subject: [PATCH 3/9] Merge #16946: wallet: include a checksum of encrypted private keys d67055e00dd90f504384e5c3f229fc95306d5aac Upgrade or rewrite encrypted key checksums (Andrew Chow) c9a9ddb4142af0af5f7b1a5ccd13f8e585007089 Set fDecryptionThoroughlyChecked based on whether crypted key checksums are valid (Andrew Chow) a8334f7ac39532528c5f8bd3b0eea05aa63e8794 Read and write a checksum for encrypted keys (Andrew Chow) Pull request description: Adds a checksum to the encrypted key record in the wallet database so that encrypted keys can be checked for corruption on wallet loading, in the same way that unencrypted keys are. This allows for us to skip the full decryption of keys upon the first unlocking of the wallet in that session as any key corruption will have already been detected. The checksum is just the double SHA256 of the encrypted key and it is appended to the record after the encrypted key itself. This is backwards compatible as old wallets will be able to read the encrypted key and ignore that there is more data in the stream. Additionally, old wallets will be upgraded upon their first unlocking (so that key decryption is checked before we commit to a checksum of the encrypted key) and a wallet flag set indicating that. The presence of the wallet flag lets us skip the full decryption as if `fDecryptionThoroughlyChecked` were true. This does mean that the first time an old wallet is unlocked in a new version will take much longer, but subsequent unlocks will be instantaneous. Furthermore, corruption will be detected upon loading rather than on trying to send so wallet corruption will be detected sooner. Fixes #12423 ACKs for top commit: laanwj: code review ACK d67055e00dd90f504384e5c3f229fc95306d5aac jonatack: Code review ACK d67055e00dd90f504384e5c3f229fc95306d5aac meshcollider: Code review ACK d67055e00dd90f504384e5c3f229fc95306d5aac Tree-SHA512: d5c1c10cfcb5db9e10dcf2326423565a9f499290b81f3155ec72254ed5bd7491e2ff5c50e98590eb07842c20d7797b4efa1c3475bae64971d500aad3b4e711d4 --- src/wallet/scriptpubkeyman.cpp | 12 +++++++++++- src/wallet/scriptpubkeyman.h | 4 ++-- src/wallet/walletdb.cpp | 29 ++++++++++++++++++++++++++--- 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 2d962b678e..e66fdcdc2c 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -193,6 +193,7 @@ bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key bool keyPass = mapCryptedKeys.empty(); // Always pass when there are no encrypted keys bool keyFail = false; CryptedKeyMap::const_iterator mi = mapCryptedKeys.begin(); + WalletBatch batch(m_storage.GetDatabase()); for (; mi != mapCryptedKeys.end(); ++mi) { const CPubKey &vchPubKey = (*mi).second.first; @@ -206,6 +207,10 @@ bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key keyPass = true; if (fDecryptionThoroughlyChecked) break; + else { + // Rewrite these encrypted keys with checksums + batch.WriteCryptedKey(vchPubKey, vchCryptedSecret, mapKeyMetadata[vchPubKey.GetID()]); + } } if (keyPass && keyFail) { @@ -923,8 +928,13 @@ bool LegacyScriptPubKeyMan::GetPubKeyInner(const CKeyID &address, CPubKey& vchPu return GetWatchPubKey(address, vchPubKeyOut); } -bool LegacyScriptPubKeyMan::LoadCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret) +bool LegacyScriptPubKeyMan::LoadCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret, bool checksum_valid) { + // Set fDecryptionThoroughlyChecked to false when the checksum is invalid + if (!checksum_valid) { + fDecryptionThoroughlyChecked = false; + } + return AddCryptedKeyInner(vchPubKey, vchCryptedSecret); } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index c8a74b0b3c..79cdb1729a 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -226,7 +226,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv { private: //! keeps track of whether Unlock has run a thorough check before - bool fDecryptionThoroughlyChecked = false; + bool fDecryptionThoroughlyChecked = true; using WatchOnlySet = std::set; using WatchKeyMap = std::map; @@ -371,7 +371,7 @@ public: //! Adds an encrypted key to the store, and saves it to disk. bool AddCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret); //! Adds an encrypted key to the store, without saving it to disk (used by LoadWallet) - bool LoadCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret); + bool LoadCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret, bool checksum_valid); void UpdateTimeFirstKey(int64_t nCreateTime) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore); //! Adds a CScript to the store bool LoadCScript(const CScript& redeemScript); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 362949eb35..85f10c92c8 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -124,8 +124,19 @@ bool WalletBatch::WriteCryptedKey(const CPubKey& vchPubKey, return false; } - if (!WriteIC(std::make_pair(DBKeys::CRYPTED_KEY, vchPubKey), vchCryptedSecret, false)) { - return false; + // Compute a checksum of the encrypted key + uint256 checksum = Hash(vchCryptedSecret.begin(), vchCryptedSecret.end()); + + const auto key = std::make_pair(DBKeys::CRYPTED_KEY, vchPubKey); + if (!WriteIC(key, std::make_pair(vchCryptedSecret, checksum), false)) { + // It may already exist, so try writing just the checksum + std::vector val; + if (!m_batch->Read(key, val)) { + return false; + } + if (!WriteIC(key, std::make_pair(val, checksum), true)) { + return false; + } } EraseIC(std::make_pair(DBKeys::KEY, vchPubKey)); return true; @@ -371,9 +382,21 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, } std::vector vchPrivKey; ssValue >> vchPrivKey; + + // Get the checksum and check it + bool checksum_valid = false; + if (!ssValue.eof()) { + uint256 checksum; + ssValue >> checksum; + if ((checksum_valid = Hash(vchPrivKey.begin(), vchPrivKey.end()) != checksum)) { + strErr = "Error reading wallet database: Crypted key corrupt"; + return false; + } + } + wss.nCKeys++; - if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadCryptedKey(vchPubKey, vchPrivKey)) + if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadCryptedKey(vchPubKey, vchPrivKey, checksum_valid)) { strErr = "Error reading wallet database: LegacyScriptPubKeyMan::LoadCryptedKey failed"; return false; From f83b4bfdb3a05d2d8d0ac540efa591ea51346009 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Fri, 17 Apr 2020 23:05:31 +1200 Subject: [PATCH 4/9] Merge #17824: wallet: Prefer full destination groups in coin selection a2324e4d3f47f084b07a364c9a360a0bf31e86a0 test: Improve naming and logging of avoid_reuse tests (Fabian Jahr) 1abbdac6777bc5396d17a6772c8176a354730997 wallet: Prefer full destination groups in coin selection (Fabian Jahr) Pull request description: Fixes #17603 (together with #17843) In the case of destination groups of >10 outputs existing in a wallet with `avoid_reuse` enabled, the grouping algorithm is adding left-over outputs as an "incomplete" group to the list of groups even when a full group has already been added. This leads to the strange behavior that if there are >10 outputs for a destination the transaction spending from that will effectively use `len(outputs) % 10` as inputs for that transaction. From the original PR and the code comment I understand the correct behavior should be the usage of 10 outputs. I opted for minimal changes in the current code although there maybe optimizations possible for cases with >20 outputs on a destination this sounds like too much of an edge case right now. ACKs for top commit: jonatack: Re-ACK a2324e4 achow101: ACK a2324e4d3f47f084b07a364c9a360a0bf31e86a0 kallewoof: ACK a2324e4d3f47f084b07a364c9a360a0bf31e86a0 meshcollider: Tested ACK a2324e4d3f47f084b07a364c9a360a0bf31e86a0 (verified the new test fails on master without this change) Tree-SHA512: 4743779c5d469fcd16df5baf166024b1d3c8eaca151df1e8281b71df62b29541cf7bfee3f8ab48d83e3b34c9256e53fd38a7b146a54c79f9caa44cce3636971a --- src/wallet/wallet.cpp | 53 +++++++++++------- src/wallet/wallet.h | 2 +- test/functional/wallet_avoidreuse.py | 81 +++++++++++++++++++++++++--- 3 files changed, 109 insertions(+), 27 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d42a82243d..6fd2523152 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2860,6 +2860,13 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm ++it; } + unsigned int limit_ancestor_count = 0; + unsigned int limit_descendant_count = 0; + chain().getPackageLimits(limit_ancestor_count, limit_descendant_count); + size_t max_ancestors = (size_t)std::max(1, limit_ancestor_count); + size_t max_descendants = (size_t)std::max(1, limit_descendant_count); + bool fRejectLongChains = gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS); + // form groups from remaining coins; note that preset coins will not // automatically have their associated (same address) coins included if (coin_control.m_avoid_partial_spends && vCoins.size() > OUTPUT_GROUP_MAX_ENTRIES) { @@ -2868,14 +2875,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm // explicitly shuffling the outputs before processing Shuffle(vCoins.begin(), vCoins.end(), FastRandomContext()); } - std::vector groups = GroupOutputs(vCoins, !coin_control.m_avoid_partial_spends); - - unsigned int limit_ancestor_count; - unsigned int limit_descendant_count; - chain().getPackageLimits(limit_ancestor_count, limit_descendant_count); - size_t max_ancestors = (size_t)std::max(1, limit_ancestor_count); - size_t max_descendants = (size_t)std::max(1, limit_descendant_count); - bool fRejectLongChains = gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS); + std::vector groups = GroupOutputs(vCoins, !coin_control.m_avoid_partial_spends, max_ancestors); bool res = value_to_select <= 0 || SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType) || @@ -5150,32 +5150,49 @@ bool CWalletTx::IsImmatureCoinBase() const return GetBlocksToMaturity() > 0; } -std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin) const { +std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors) const { std::vector groups; std::map gmap; - CTxDestination dst; + std::set full_groups; + for (const auto& output : outputs) { if (output.fSpendable) { + CTxDestination dst; CInputCoin input_coin = output.GetInputCoin(); size_t ancestors, descendants; chain().getTransactionAncestry(output.tx->GetHash(), ancestors, descendants); if (!single_coin && ExtractDestination(output.tx->tx->vout[output.i].scriptPubKey, dst)) { - // Limit output groups to no more than 10 entries, to protect - // against inadvertently creating a too-large transaction - // when using -avoidpartialspends - if (gmap[dst].m_outputs.size() >= OUTPUT_GROUP_MAX_ENTRIES) { - groups.push_back(gmap[dst]); - gmap.erase(dst); + auto it = gmap.find(dst); + if (it != gmap.end()) { + // Limit output groups to no more than OUTPUT_GROUP_MAX_ENTRIES + // number of entries, to protect against inadvertently creating + // a too-large transaction when using -avoidpartialspends to + // prevent breaking consensus or surprising users with a very + // high amount of fees. + if (it->second.m_outputs.size() >= OUTPUT_GROUP_MAX_ENTRIES) { + groups.push_back(it->second); + it->second = OutputGroup{}; + full_groups.insert(dst); + } + it->second.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); + } else { + gmap[dst].Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); } - gmap[dst].Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); } else { groups.emplace_back(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); } } } if (!single_coin) { - for (const auto& it : gmap) groups.push_back(it.second); + for (auto& it : gmap) { + auto& group = it.second; + if (full_groups.count(it.first) > 0) { + // Make this unattractive as we want coin selection to avoid it if possible + group.m_ancestors = max_ancestors - 1; + } + groups.push_back(group); + } } return groups; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index dd0f10bb20..fd167a5e2c 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -920,7 +920,7 @@ public: bool IsSpentKey(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::vector GroupOutputs(const std::vector& outputs, bool single_coin) const; + std::vector GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors) const; bool IsLockedCoin(uint256 hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void LockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/test/functional/wallet_avoidreuse.py b/test/functional/wallet_avoidreuse.py index 87b60ef59d..2f98c89501 100755 --- a/test/functional/wallet_avoidreuse.py +++ b/test/functional/wallet_avoidreuse.py @@ -89,11 +89,15 @@ class AvoidReuseTest(BitcoinTestFramework): self.nodes[0].generate(110) self.sync_all() reset_balance(self.nodes[1], self.nodes[0].getnewaddress()) - self.test_fund_send_fund_senddirty() + self.test_sending_from_reused_address_without_avoid_reuse() reset_balance(self.nodes[1], self.nodes[0].getnewaddress()) - self.test_fund_send_fund_send() + self.test_sending_from_reused_address_fails() reset_balance(self.nodes[1], self.nodes[0].getnewaddress()) self.test_getbalances_used() + reset_balance(self.nodes[1], self.nodes[0].getnewaddress()) + self.test_full_destination_group_is_preferred() + reset_balance(self.nodes[1], self.nodes[0].getnewaddress()) + self.test_all_destination_groups_are_used() def test_persistence(self): '''Test that wallet files persist the avoid_reuse flag.''' @@ -137,13 +141,13 @@ class AvoidReuseTest(BitcoinTestFramework): # Unload temp wallet self.nodes[1].unloadwallet(tempwallet) - def test_fund_send_fund_senddirty(self): + def test_sending_from_reused_address_without_avoid_reuse(self): ''' - Test the same as test_fund_send_fund_send, except send the 10 BTC with + Test the same as test_sending_from_reused_address_fails, except send the 10 BTC with the avoid_reuse flag set to false. This means the 10 BTC send should succeed, - where it fails in test_fund_send_fund_send. + where it fails in test_sending_from_reused_address_fails. ''' - self.log.info("Test fund send fund send dirty") + self.log.info("Test sending from reused address with avoid_reuse=false") fundaddr = self.nodes[1].getnewaddress() retaddr = self.nodes[0].getnewaddress() @@ -188,7 +192,7 @@ class AvoidReuseTest(BitcoinTestFramework): assert_approx(self.nodes[1].getbalance(), 5, 0.001) assert_approx(self.nodes[1].getbalance(avoid_reuse=False), 5, 0.001) - def test_fund_send_fund_send(self): + def test_sending_from_reused_address_fails(self): ''' Test the simple case where [1] generates a new address A, then [0] sends 10 BTC to A. @@ -197,7 +201,7 @@ class AvoidReuseTest(BitcoinTestFramework): [1] tries to spend 10 BTC (fails; dirty). [1] tries to spend 4 BTC (succeeds; change address sufficient) ''' - self.log.info("Test fund send fund send") + self.log.info("Test sending from reused address fails") fundaddr = self.nodes[1].getnewaddress(label="") retaddr = self.nodes[0].getnewaddress() @@ -276,5 +280,66 @@ class AvoidReuseTest(BitcoinTestFramework): assert_unspent(self.nodes[1], total_count=2, total_sum=6, reused_count=1, reused_sum=1) assert_balances(self.nodes[1], mine={"used": 1, "trusted": 5}) + def test_full_destination_group_is_preferred(self): + ''' + Test the case where [1] only has 11 outputs of 1 BTC in the same reused + address and tries to send a small payment of 0.5 BTC. The wallet + should use 10 outputs from the reused address as inputs and not a + single 1 BTC input, in order to join several outputs from the reused + address. + ''' + self.log.info("Test that full destination groups are preferred in coin selection") + + # Node under test should be empty + assert_equal(self.nodes[1].getbalance(avoid_reuse=False), 0) + + new_addr = self.nodes[1].getnewaddress() + ret_addr = self.nodes[0].getnewaddress() + + # Send 11 outputs of 1 BTC to the same, reused address in the wallet + for _ in range(11): + self.nodes[0].sendtoaddress(new_addr, 1) + + self.nodes[0].generate(1) + self.sync_all() + + # Sending a transaction that is smaller than each one of the + # available outputs + txid = self.nodes[1].sendtoaddress(address=ret_addr, amount=0.5) + inputs = self.nodes[1].getrawtransaction(txid, 1)["vin"] + + # The transaction should use 10 inputs exactly + assert_equal(len(inputs), 10) + + def test_all_destination_groups_are_used(self): + ''' + Test the case where [1] only has 22 outputs of 1 BTC in the same reused + address and tries to send a payment of 20.5 BTC. The wallet + should use all 22 outputs from the reused address as inputs. + ''' + self.log.info("Test that all destination groups are used") + + # Node under test should be empty + assert_equal(self.nodes[1].getbalance(avoid_reuse=False), 0) + + new_addr = self.nodes[1].getnewaddress() + ret_addr = self.nodes[0].getnewaddress() + + # Send 22 outputs of 1 BTC to the same, reused address in the wallet + for _ in range(22): + self.nodes[0].sendtoaddress(new_addr, 1) + + self.nodes[0].generate(1) + self.sync_all() + + # Sending a transaction that needs to use the full groups + # of 10 inputs but also the incomplete group of 2 inputs. + txid = self.nodes[1].sendtoaddress(address=ret_addr, amount=20.5) + inputs = self.nodes[1].getrawtransaction(txid, 1)["vin"] + + # The transaction should use 22 inputs exactly + assert_equal(len(inputs), 22) + + if __name__ == '__main__': AvoidReuseTest().main() From 281d2222e115ce0d4b54d660e725656ff7bac139 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 19 Apr 2020 10:32:19 -0400 Subject: [PATCH 5/9] Merge #18601: wallet: Refactor WalletRescanReserver to use wallet reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fc289b7898fb90d4800675b69c0bb9b42df5599f wallet: Refactor WalletRescanReserver to use wallet reference (João Barbosa) Pull request description: Simple refactor to `WalletRescanReserver` to use wallet reference instead of pointer. Complements #18259. ACKs for top commit: MarcoFalke: ACK fc289b7898fb90d4800675b69c0bb9b42df5599f Tree-SHA512: b03e33f2d9df2870436aa3284137fd022dd89ea96a1b170fa27f8685ad4f986e6c4ba5975a84966c30d18430a4014d7d8740a1dff2f985c9ef8226ed18e69db9 --- src/qt/test/wallettests.cpp | 2 +- src/wallet/rpcdump.cpp | 12 ++++++------ src/wallet/rpcwallet.cpp | 6 +++--- src/wallet/test/coinjoin_tests.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 14 +++++++------- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 17 +++++++++-------- 7 files changed, 28 insertions(+), 27 deletions(-) diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index d8e2630a29..2f33564766 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -121,7 +121,7 @@ void TestGUI(interfaces::Node& node) wallet->SetLastBlockProcessed(105, ::ChainActive().Tip()->GetBlockHash()); } { - WalletRescanReserver reserver(wallet.get()); + WalletRescanReserver reserver(*wallet); reserver.reserve(); CWallet::ScanResult result = wallet->ScanForWalletTransactions(Params().GetConsensus().hashGenesisBlock, 0 /* start_height */, {} /* max_height */, reserver, true /* fUpdate */); QCOMPARE(result.status, CWallet::ScanResult::SUCCESS); diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 416f24fb94..c4d227b04f 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -106,7 +106,7 @@ UniValue importprivkey(const JSONRPCRequest& request) EnsureLegacyScriptPubKeyMan(*wallet, true); WalletBatch batch(pwallet->GetDBHandle()); - WalletRescanReserver reserver(pwallet); + WalletRescanReserver reserver(*pwallet); bool fRescan = true; { LOCK(pwallet->cs_wallet); @@ -231,7 +231,7 @@ UniValue importaddress(const JSONRPCRequest& request) throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled when blocks are pruned"); } - WalletRescanReserver reserver(pwallet); + WalletRescanReserver reserver(*pwallet); if (fRescan && !reserver.reserve()) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait."); } @@ -419,7 +419,7 @@ UniValue importpubkey(const JSONRPCRequest& request) throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled when blocks are pruned"); } - WalletRescanReserver reserver(pwallet); + WalletRescanReserver reserver(*pwallet); if (fRescan && !reserver.reserve()) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait."); } @@ -488,7 +488,7 @@ UniValue importwallet(const JSONRPCRequest& request) } WalletBatch batch(pwallet->GetDBHandle()); - WalletRescanReserver reserver(pwallet); + WalletRescanReserver reserver(*pwallet); if (!reserver.reserve()) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait."); } @@ -780,7 +780,7 @@ UniValue importelectrumwallet(const JSONRPCRequest& request) spk_man.UpdateTimeFirstKey(nTimeBegin); pwallet->WalletLogPrintf("Rescanning %i blocks\n", tip_height - nStartHeight + 1); - WalletRescanReserver reserver(pwallet); + WalletRescanReserver reserver(*pwallet); if (!reserver.reserve()) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait."); } @@ -1519,7 +1519,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest) } } - WalletRescanReserver reserver(pwallet); + WalletRescanReserver reserver(*pwallet); if (fRescan && !reserver.reserve()) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait."); } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 4a6fb75be5..3a6609bd60 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2752,7 +2752,7 @@ static UniValue upgradetohd(const JSONRPCRequest& request) // If you are generating new mnemonic it is assumed that the addresses have never gotten a transaction before, so you don't need to rescan for transactions bool rescan = request.params[3].isNull() ? !generate_mnemonic : request.params[3].get_bool(); if (rescan) { - WalletRescanReserver reserver(pwallet); + WalletRescanReserver reserver(*pwallet); if (!reserver.reserve()) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait."); } @@ -3518,7 +3518,7 @@ static UniValue rescanblockchain(const JSONRPCRequest& request) if (!wallet) return NullUniValue; CWallet* const pwallet = wallet.get(); - WalletRescanReserver reserver(pwallet); + WalletRescanReserver reserver(*pwallet); if (!reserver.reserve()) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait."); } @@ -3590,7 +3590,7 @@ static UniValue wipewallettxes(const JSONRPCRequest& request) if (!wallet) return NullUniValue; CWallet* const pwallet = wallet.get(); - WalletRescanReserver reserver(pwallet); + WalletRescanReserver reserver(*pwallet); if (!reserver.reserve()) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort rescan or wait."); } diff --git a/src/wallet/test/coinjoin_tests.cpp b/src/wallet/test/coinjoin_tests.cpp index 53aefdfe05..c1cd9ce6ff 100644 --- a/src/wallet/test/coinjoin_tests.cpp +++ b/src/wallet/test/coinjoin_tests.cpp @@ -138,7 +138,7 @@ public: LOCK2(wallet->cs_wallet, cs_main); wallet->GetLegacyScriptPubKeyMan()->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey()); wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); - WalletRescanReserver reserver(wallet.get()); + WalletRescanReserver reserver(*wallet); reserver.reserve(); CWallet::ScanResult result = wallet->ScanForWalletTransactions(::ChainActive().Genesis()->GetBlockHash(), 0 /* start_height */, {} /* max_height */, reserver, true /* fUpdate */); BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 5e87933424..29cfbd537b 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -101,7 +101,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); } AddKey(wallet, coinbaseKey); - WalletRescanReserver reserver(&wallet); + WalletRescanReserver reserver(wallet); reserver.reserve(); CWallet::ScanResult result = wallet.ScanForWalletTransactions({} /* start_block */, 0 /* start_height */, {} /* max_height */, reserver, false /* update */); BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::FAILURE); @@ -120,7 +120,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); } AddKey(wallet, coinbaseKey); - WalletRescanReserver reserver(&wallet); + WalletRescanReserver reserver(wallet); reserver.reserve(); CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), oldTip->nHeight, {} /* max_height */, reserver, false /* update */); BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS); @@ -146,7 +146,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); } AddKey(wallet, coinbaseKey); - WalletRescanReserver reserver(&wallet); + WalletRescanReserver reserver(wallet); reserver.reserve(); CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), oldTip->nHeight, {} /* max_height */, reserver, false /* update */); BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::FAILURE); @@ -171,7 +171,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); } AddKey(wallet, coinbaseKey); - WalletRescanReserver reserver(&wallet); + WalletRescanReserver reserver(wallet); reserver.reserve(); CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), oldTip->nHeight, {} /* max_height */, reserver, false /* update */); BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::FAILURE); @@ -581,7 +581,7 @@ public: bool firstRun; wallet->LoadWallet(firstRun); AddKey(*wallet, coinbaseKey); - WalletRescanReserver reserver(wallet.get()); + WalletRescanReserver reserver(*wallet); reserver.reserve(); CWallet::ScanResult result = wallet->ScanForWalletTransactions(::ChainActive().Genesis()->GetBlockHash(), 0 /* start_height */, {} /* max_height */, reserver, false /* update */); BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS); @@ -715,7 +715,7 @@ public: wallet->LoadWallet(firstRun); AddWallet(wallet); AddKey(*wallet, coinbaseKey); - WalletRescanReserver reserver(wallet.get()); + WalletRescanReserver reserver(*wallet); reserver.reserve(); { LOCK(wallet->cs_wallet); @@ -1160,7 +1160,7 @@ BOOST_FIXTURE_TEST_CASE(select_coins_grouped_by_addresses, ListCoinsTestingSetup } // Reveal the mined tx, it should conflict with the one we have in the wallet already. - WalletRescanReserver reserver(wallet.get()); + WalletRescanReserver reserver(*wallet); reserver.reserve(); auto result = wallet->ScanForWalletTransactions(::ChainActive().Genesis()->GetBlockHash(), 0 /* start_height */, {} /* max_height */, reserver, false /* update */); BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6fd2523152..c82873ddc4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4791,7 +4791,7 @@ std::shared_ptr CWallet::Create(interfaces::Chain& chain, const std::st } { - WalletRescanReserver reserver(walletInstance.get()); + WalletRescanReserver reserver(*walletInstance); if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(chain.getBlockHash(rescan_height), rescan_height, {} /* max height */, reserver, true /* update */).status)) { error = _("Failed to rescan the wallet during initialization"); return nullptr; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index fd167a5e2c..f8afd8ae95 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1371,35 +1371,36 @@ void MaybeResendWalletTxs(); class WalletRescanReserver { private: - CWallet* m_wallet; + CWallet& m_wallet; bool m_could_reserve; public: - explicit WalletRescanReserver(CWallet* w) : m_wallet(w), m_could_reserve(false) {} + explicit WalletRescanReserver(CWallet& w) : m_wallet(w), m_could_reserve(false) {} bool reserve() { assert(!m_could_reserve); - if (m_wallet->fScanningWallet.exchange(true)) { + if (m_wallet.fScanningWallet.exchange(true)) { return false; } - m_wallet->m_scanning_start = GetTimeMillis(); - m_wallet->m_scanning_progress = 0; - m_wallet->fAbortRescan = false; + m_wallet.m_scanning_start = GetTimeMillis(); + m_wallet.m_scanning_progress = 0; + m_wallet.fAbortRescan = false; m_could_reserve = true; return true; } bool isReserved() const { - return (m_could_reserve && m_wallet->fScanningWallet); + return (m_could_reserve && m_wallet.fScanningWallet); } ~WalletRescanReserver() { if (m_could_reserve) { - m_wallet->fScanningWallet = false; + m_wallet.fScanningWallet = false; } } + }; // Calculate the size of the transaction assuming all signatures are max size From 4e7cb26160134add867cbeffafba8208d1f3b562 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sat, 25 Apr 2020 09:25:26 -0400 Subject: [PATCH 6/9] Merge #18585: test: use zero-argument super() shortcut (Python 3.0+) 0956e46bff7f0b6da65a4de6d4f8261fe9d7055c test: use zero-argument super() shortcut (Python 3.0+) (Sebastian Falbesoner) Pull request description: This mini-PR replaces all calls to `super(...)` with arguments with the zero-argument shortcut `super()` where applicable. See [PEP 3135](https://www.python.org/dev/peps/pep-3135/#specification): > The new syntax: > > super() > > is equivalent to: > > super(__class__, ) > > where __class__ is the class that the method was defined in, and is > the first parameter of the method (normally self for instance methods, and cls > for class methods). ACKs for top commit: fanquake: ACK 0956e46bff7f0b6da65a4de6d4f8261fe9d7055c Tree-SHA512: 4ac66fe7ab2be2e8a514e5fcfc41dbb298f21b23ebb7b7b0310d704b0b3cef8adf287a8d80346d1ea9418998c597b4f0ff1f66148d0d806bb43db6607e0fe1cf --- test/functional/test_framework/messages.py | 6 +++--- test/functional/test_framework/script.py | 8 ++++---- test/functional/wallet_txn_clone.py | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index 0fcdce1248..73027b500b 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -586,16 +586,16 @@ class CBlock(CBlockHeader): __slots__ = ("vtx",) def __init__(self, header=None): - super(CBlock, self).__init__(header) + super().__init__(header) self.vtx = [] def deserialize(self, f): - super(CBlock, self).deserialize(f) + super().deserialize(f) self.vtx = deser_vector(f, CTransaction) def serialize(self): r = b"" - r += super(CBlock, self).serialize() + r += super().serialize() r += ser_vector(self.vtx) return r diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py index 2e395af6c6..872a997808 100644 --- a/test/functional/test_framework/script.py +++ b/test/functional/test_framework/script.py @@ -96,7 +96,7 @@ class CScriptOp(int): return _opcode_instances[n] except IndexError: assert len(_opcode_instances) == n - _opcode_instances.append(super(CScriptOp, cls).__new__(cls, n)) + _opcode_instances.append(super().__new__(cls, n)) return _opcode_instances[n] # Populate opcode instance table @@ -373,7 +373,7 @@ class CScriptTruncatedPushDataError(CScriptInvalidError): """Invalid pushdata due to truncation""" def __init__(self, msg, data): self.data = data - super(CScriptTruncatedPushDataError, self).__init__(msg) + super().__init__(msg) # This is used, eg, for blockchain heights in coinbase scripts (bip34) @@ -459,14 +459,14 @@ class CScript(bytes): def __new__(cls, value=b''): if isinstance(value, bytes) or isinstance(value, bytearray): - return super(CScript, cls).__new__(cls, value) + return super().__new__(cls, value) else: def coerce_iterable(iterable): for instance in iterable: yield cls.__coerce_instance(instance) # Annoyingly on both python2 and python3 bytes.join() always # returns a bytes instance even when subclassed. - return super(CScript, cls).__new__(cls, b''.join(coerce_iterable(value))) + return super().__new__(cls, b''.join(coerce_iterable(value))) def raw_iter(self): """Raw iteration diff --git a/test/functional/wallet_txn_clone.py b/test/functional/wallet_txn_clone.py index e76420bab7..1a88e1464d 100755 --- a/test/functional/wallet_txn_clone.py +++ b/test/functional/wallet_txn_clone.py @@ -25,7 +25,7 @@ class TxnMallTest(BitcoinTestFramework): def setup_network(self): # Start with split network: - super(TxnMallTest, self).setup_network() + super().setup_network() self.disconnect_nodes(1, 2) def run_test(self): From 4f3a1effbf9152d437bebbe1a407e633785e87f6 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Mon, 2 Nov 2020 11:52:00 +1300 Subject: [PATCH 7/9] Merge #20271: doc: Document that wallet salvage is experimental fab94534b64593be1620c989bf69eb02e1be9b1b doc: Document that wallet salvage is experimental (MarcoFalke) Pull request description: See #20151 ACKs for top commit: practicalswift: ACK fab94534b64593be1620c989bf69eb02e1be9b1b: user safety first hebasto: ACK fab94534b64593be1620c989bf69eb02e1be9b1b, maybe capitalize into "WARNING"? meshcollider: Trivial ACK fab94534b64593be1620c989bf69eb02e1be9b1b Tree-SHA512: 94912c491facc485293e4333066057933d706d84c7172f615296e7ba998c583c8bd07e751e6f00cd6576e7791007ace321f959181f7bf6a4e15e10d7ec8a1b7e --- src/bitcoin-wallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp index c2f87e1553..17572464f6 100644 --- a/src/bitcoin-wallet.cpp +++ b/src/bitcoin-wallet.cpp @@ -30,7 +30,7 @@ static void SetupWalletToolArgs(ArgsManager& argsman) argsman.AddArg("create", "Create new wallet file", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS); // Hidden - argsman.AddArg("salvage", "Attempt to recover private keys from a corrupt wallet", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS); + argsman.AddArg("salvage", "Attempt to recover private keys from a corrupt wallet. Warning: 'salvage' is experimental.", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS); argsman.AddArg("wipetxes", "Wipe all transactions from a wallet", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS); } From 7caec1df14c1efe705e9b730b0bd17f543dd806f Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 2 Oct 2020 16:36:57 +0200 Subject: [PATCH 8/9] Merge #19871: doc: Clarify scope of eviction protection of outbound block-relay peers d76925478efd35e6fd835370639f2139b28381e4 [doc] Clarify semantic of peer's m_protect w.r.t to outbound eviction logics (Antoine Riard) ac71fe936da290adf5a3155fe8db5f78b485f1f1 [doc] Clarify scope of eviction protection of outbound block-relay peers (Antoine Riard) Pull request description: Block-relay-only peers were introduced by #15759. According to its author, it was intented to make them only immune to outbound peer rotation-based eviction and not from all eviction as modified comment leans to think of. Clearly indicate that outbound block-relay peers aren't protected from eviction by the bad/lagging chain logic. Fix #19863 ACKs for top commit: naumenkogs: ACK d76925478efd35e6fd835370639f2139b28381e4 jonatack: ACK d76925478efd35e6fd835370639f2139b28381e4 Tree-SHA512: 597fbd62838a6e39276024165b11514cad20a2e9d33cf9202d261cbadcb62b2df427c858e0cb57e585840d4c1d4600104aa53916bb868541f2580e4eed9b4b52 --- src/net_processing.cpp | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f2e23b745d..908966d8a7 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -563,10 +563,17 @@ struct CNodeState { */ bool fSupportsDesiredCmpctVersion; - /** State used to enforce CHAIN_SYNC_TIMEOUT - * Only in effect for outbound, non-manual, full-relay connections, with - * m_protect == false - * Algorithm: if a peer's best known block has less work than our tip, + /** State used to enforce CHAIN_SYNC_TIMEOUT and EXTRA_PEER_CHECK_INTERVAL logic. + * + * Both are only in effect for outbound, non-manual, non-protected connections. + * Any peer protected (m_protect = true) is not chosen for eviction. A peer is + * marked as protected if all of these are true: + * - its connection type is IsBlockOnlyConn() == false + * - it gave us a valid connecting header + * - we haven't reached MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT yet + * - it has a better chain than we have + * + * CHAIN_SYNC_TIMEOUT: if a peer's best known block has less work than our tip, * set a timeout CHAIN_SYNC_TIMEOUT seconds in the future: * - If at timeout their best known block now has more work than our tip * when the timeout was set, then either reset the timeout or clear it @@ -576,6 +583,9 @@ struct CNodeState { * and set a shorter timeout, HEADERS_RESPONSE_TIME seconds in future. * If their best known block is still behind when that new timeout is * reached, disconnect. + * + * EXTRA_PEER_CHECK_INTERVAL: after each interval, if we have too many outbound peers, + * drop the outbound one that least recently announced us a new block. */ struct ChainSyncTimeoutState { //! A timeout used for checking whether our peer has sufficiently synced @@ -2466,12 +2476,12 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const std::vectorpindexBestKnownBlock != nullptr && pfrom.IsAddrRelayPeer()) { - // If this is an outbound full-relay peer, check to see if we should protect - // it from the bad/lagging chain logic. - // Note that block-relay-only peers are already implicitly protected, so we - // only consider setting m_protect for the full-relay peers. if (m_outbound_peers_with_protect_from_disconnect < MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT && nodestate->pindexBestKnownBlock->nChainWork >= m_chainman.ActiveChain().Tip()->nChainWork && !nodestate->m_chain_sync.m_protect) { LogPrint(BCLog::NET, "Protecting outbound peer=%d from eviction\n", pfrom.GetId()); nodestate->m_chain_sync.m_protect = true; From 945aca8b012042b561a8e6f7064c4da264c31803 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 22 Oct 2020 10:13:35 +0200 Subject: [PATCH 9/9] Merge #20039: test: Convert amounts from float to decimal 5aadd4be1883386a04bef6a04e9a1142601ef7a7 Convert amounts from float to decimal (Prayank) Pull request description: > decimal is preferred in accounting applications https://docs.python.org/3.8/library/decimal.html Decimal type saves an exact value so better than using float. ~~3 variables declared with type as 'Decimal' in [test/functional/mempool_accept.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/mempool_accept.py): fee, fee_expected, output_amount~~ ~~Not required to convert to string anymore for using the above variables as decimal~~ + fee, fee_expected, output_amount ~~+ 8 decimal places~~ + Using value of coin['amount'] as decimal and removed 'int' + Removed unnecessary parentheses + Remove str() and use quotes Fixes https://github.com/bitcoin/bitcoin/issues/20011 ACKs for top commit: guggero: ACK 5aadd4be1883386a04bef6a04e9a1142601ef7a7 Tree-SHA512: 5877cf3837e5b65bec0fc8909de141a720bfa02a747513e21d20f3c41ec0cfecc524d2c347a96596b0a1a97900da2acf08b799f26b11d537e4dcddc6ce45f38e --- test/functional/mempool_accept.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index 8f3a34dd28..53140d2e1b 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -4,6 +4,8 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test mempool acceptance of raw transactions.""" + +from decimal import Decimal from io import BytesIO import math @@ -82,10 +84,10 @@ class MempoolAcceptanceTest(BitcoinTestFramework): ) self.log.info('A transaction not in the mempool') - fee = 0.00000700 + fee = Decimal('0.000007') raw_tx_0 = node.signrawtransactionwithwallet(node.createrawtransaction( inputs=[{"txid": txid_in_block, "vout": 0, "sequence": BIP125_SEQUENCE_NUMBER}], # RBF is used later - outputs=[{node.getnewaddress(): 0.3 - fee}], + outputs=[{node.getnewaddress(): Decimal('0.3') - fee}], ))['hex'] tx = CTransaction() tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_0)))