diff --git a/doc/release-notes-28414.md b/doc/release-notes-28414.md new file mode 100644 index 000000000000..7fca11f8220b --- /dev/null +++ b/doc/release-notes-28414.md @@ -0,0 +1,5 @@ +RPC Wallet +---------- + +- RPC `walletprocesspsbt` return object now includes field `hex` (if the transaction +is complete) containing the serialized transaction suitable for RPC `sendrawtransaction`. (#28414) diff --git a/src/init.cpp b/src/init.cpp index 8d516f2cdb9a..c99f384abfbd 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -184,6 +184,11 @@ static const char* DEFAULT_ASMAP_FILENAME="ip_asn.map"; * The PID file facilities. */ static const char* BITCOIN_PID_FILENAME = "dashd.pid"; +/** + * True if this process has created a PID file. + * Used to determine whether we should remove the PID file on shutdown. + */ +static bool g_generated_pid{false}; static fs::path GetPidFile(const ArgsManager& args) { @@ -199,12 +204,24 @@ static fs::path GetPidFile(const ArgsManager& args) #else tfm::format(file, "%d\n", getpid()); #endif + g_generated_pid = true; return true; } else { return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno))); } } +static void RemovePidFile(const ArgsManager& args) +{ + if (!g_generated_pid) return; + const auto pid_path{GetPidFile(args)}; + if (std::error_code error; !fs::remove(pid_path, error)) { + std::string msg{error ? error.message() : "File does not exist"}; + LogPrintf("Unable to remove PID file (%s): %s\n", fs::PathToString(pid_path), msg); + } +} + + ////////////////////////////////////////////////////////////////////////////// // // Shutdown @@ -476,13 +493,7 @@ void Shutdown(NodeContext& node) node.chainman.reset(); node.scheduler.reset(); - try { - if (!fs::remove(GetPidFile(*node.args))) { - LogPrintf("%s: Unable to remove PID file: File does not exist\n", __func__); - } - } catch (const fs::filesystem_error& e) { - LogPrintf("%s: Unable to remove PID file: %s\n", __func__, fsbridge::get_filesystem_error_message(e)); - } + RemovePidFile(*node.args); LogPrintf("%s: done\n", __func__); } diff --git a/src/qt/test/optiontests.cpp b/src/qt/test/optiontests.cpp index 17ffeb220b69..067cfaba04b3 100644 --- a/src/qt/test/optiontests.cpp +++ b/src/qt/test/optiontests.cpp @@ -61,7 +61,11 @@ void OptionTests::migrateSettings() QVERIFY(!settings.contains("addrSeparateProxyTor")); std::ifstream file(gArgs.GetDataDirNet() / "settings.json"); + std::string default_warning = strprintf("This file is automatically generated and updated by %s. Please do not edit this file while the node " + "is running, as any changes might be ignored or overwritten.", + PACKAGE_NAME); QCOMPARE(std::string(std::istreambuf_iterator(file), std::istreambuf_iterator()).c_str(), "{\n" + " \"_warning_\": \""+ default_warning+"\",\n" " \"dbcache\": \"600\",\n" " \"listen\": false,\n" " \"onion\": \"onion:234\",\n" diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 24b63fe83eec..aab76c8131f2 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -54,13 +54,22 @@ using node::UpdateTime; /** * Return average network hashes per second based on the last 'lookup' blocks, - * or from the last difficulty change if 'lookup' is nonpositive. - * If 'height' is nonnegative, compute the estimate at the time when a given block was found. + * or from the last difficulty change if 'lookup' is -1. + * If 'height' is -1, compute the estimate from current chain tip. + * If 'height' is a valid block height, compute the estimate at the time when a given block was found. */ static UniValue GetNetworkHashPS(int lookup, int height, const CChain& active_chain) { + if (lookup < -1 || lookup == 0) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid nblocks. Must be a positive number or -1."); + } + + if (height < -1 || height > active_chain.Height()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Block does not exist at specified height"); + } + const CBlockIndex* pb = active_chain.Tip(); - if (height >= 0 && height < active_chain.Height()) { + if (height >= 0) { pb = active_chain[height]; } @@ -68,7 +77,7 @@ static UniValue GetNetworkHashPS(int lookup, int height, const CChain& active_ch return 0; // If lookup is -1, then use blocks since last difficulty change. - if (lookup <= 0) + if (lookup == -1) lookup = pb->nHeight % Params().GetConsensus().DifficultyAdjustmentInterval() + 1; // If lookup is larger than chain, then set it to chain length. @@ -102,7 +111,7 @@ static RPCHelpMan getnetworkhashps() "Pass in [blocks] to override # of blocks, -1 specifies since last difficulty change.\n" "Pass in [height] to estimate the network speed at the time when a certain block was found.\n", { - {"nblocks", RPCArg::Type::NUM, RPCArg::Default{120}, "The number of blocks, or -1 for blocks since last difficulty change."}, + {"nblocks", RPCArg::Type::NUM, RPCArg::Default{120}, "The number of previous blocks to calculate estimate from, or -1 for blocks since last difficulty change."}, {"height", RPCArg::Type::NUM, RPCArg::Default{-1}, "To estimate at the time of the given height."}, }, RPCResult{ diff --git a/src/test/settings_tests.cpp b/src/test/settings_tests.cpp index 9bd9c7942043..0f57b3c9e2c6 100644 --- a/src/test/settings_tests.cpp +++ b/src/test/settings_tests.cpp @@ -99,7 +99,9 @@ BOOST_AUTO_TEST_CASE(ReadWrite) // Check invalid json not allowed WriteText(path, R"(invalid json)"); BOOST_CHECK(!util::ReadSettings(path, values, errors)); - std::vector fail_parse = {strprintf("Unable to parse settings file %s", fs::PathToString(path))}; + std::vector fail_parse = {strprintf("Settings file %s does not contain valid JSON. This is probably caused by disk corruption or a crash, " + "and can be fixed by removing the file, which will reset settings to default values.", + fs::PathToString(path))}; BOOST_CHECK_EQUAL_COLLECTIONS(errors.begin(), errors.end(), fail_parse.begin(), fail_parse.end()); } diff --git a/src/util/settings.cpp b/src/util/settings.cpp index 421b2be4623c..c0423ea023ff 100644 --- a/src/util/settings.cpp +++ b/src/util/settings.cpp @@ -5,6 +5,10 @@ #include #include +#if defined(HAVE_CONFIG_H) +#include +#endif + #include #include @@ -90,7 +94,9 @@ bool ReadSettings(const fs::path& path, std::map& va SettingsValue in; if (!in.read(std::string{std::istreambuf_iterator(file), std::istreambuf_iterator()})) { - errors.emplace_back(strprintf("Unable to parse settings file %s", fs::PathToString(path))); + errors.emplace_back(strprintf("Settings file %s does not contain valid JSON. This is probably caused by disk corruption or a crash, " + "and can be fixed by removing the file, which will reset settings to default values.", + fs::PathToString(path))); return false; } @@ -123,6 +129,13 @@ bool WriteSettings(const fs::path& path, std::vector& errors) { SettingsValue out(SettingsValue::VOBJ); + // Add auto-generated warning comment only if it does not exist + if (!values.contains("_warning_")) { + out.pushKV("_warning_", strprintf("This file is automatically generated and updated by %s. Please do not edit this file while the node " + "is running, as any changes might be ignored or overwritten.", + PACKAGE_NAME)); + } + // Push settings values for (const auto& value : values) { out.__pushKV(value.first, value.second); } diff --git a/src/wallet/rpc/encrypt.cpp b/src/wallet/rpc/encrypt.cpp index 9a0b6896a152..8ea9ed030bff 100644 --- a/src/wallet/rpc/encrypt.cpp +++ b/src/wallet/rpc/encrypt.cpp @@ -220,7 +220,11 @@ RPCHelpMan encryptwallet() "After this, any calls that interact with private keys such as sending or signing \n" "will require the passphrase to be set prior the making these calls.\n" "Use the walletpassphrase call for this, and then walletlock call.\n" - "If the wallet is already encrypted, use the walletpassphrasechange call.\n", + "If the wallet is already encrypted, use the walletpassphrasechange call.\n" + "** IMPORTANT **\n" + "For security reasons, the encryption process will generate a new HD seed, resulting\n" + "in the creation of a fresh set of active descriptors. Therefore, it is crucial to\n" + "securely back up the newly generated wallet file using the backupwallet RPC.\n", { {"passphrase", RPCArg::Type::STR, RPCArg::Optional::NO, "The pass phrase to encrypt the wallet with. It must be at least 1 character, but should be long."}, }, @@ -266,7 +270,7 @@ RPCHelpMan encryptwallet() if (pwallet->IsHDEnabled()) { return "wallet encrypted; If you forget the passphrase, you will lose access to your funds. Make sure that you have backup of your seed or mnemonic."; } - return "wallet encrypted; The keypool has been flushed. You need to make a new backup."; + return "wallet encrypted; The keypool has been flushed. You need to make a new backup with the backupwallet RPC."; }, }; } diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index f44eedff2bc8..c1b040a0e8cd 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -1278,6 +1278,7 @@ RPCHelpMan walletprocesspsbt() { {RPCResult::Type::STR, "psbt", "The base64-encoded partially signed transaction"}, {RPCResult::Type::BOOL, "complete", "If the transaction has a complete set of signatures"}, + {RPCResult::Type::STR_HEX, "hex", /*optional=*/true, "The hex-encoded network transaction if complete"}, } }, RPCExamples{ @@ -1327,6 +1328,14 @@ RPCHelpMan walletprocesspsbt() ssTx << psbtx; result.pushKV("psbt", EncodeBase64(ssTx.str())); result.pushKV("complete", complete); + if (complete) { + CMutableTransaction mtx; + // Returns true if complete, which we already think it is. + CHECK_NONFATAL(FinalizeAndExtractPSBT(psbtx, mtx)); + CDataStream ssTx_final(SER_NETWORK, PROTOCOL_VERSION); + ssTx_final << mtx; + result.pushKV("hex", HexStr(ssTx_final)); + } return result; }, diff --git a/test/functional/feature_filelock.py b/test/functional/feature_filelock.py index 31f5225ae19c..2aa384ced7f5 100755 --- a/test/functional/feature_filelock.py +++ b/test/functional/feature_filelock.py @@ -33,8 +33,11 @@ def run_test(self): expected_msg = f"Error: Cannot obtain a lock on data directory {datadir}. {self.config['environment']['PACKAGE_NAME']} is probably already running." self.nodes[1].assert_start_raises_init_error(extra_args=[f'-datadir={self.nodes[0].datadir}', '-noserver'], expected_msg=expected_msg) + self.log.info("Check that cookie and PID file are not deleted when attempting to start a second dashd using the same datadir") cookie_file = Path(datadir) / ".cookie" - assert cookie_file.exists() # should not be deleted during the second bitcoind instance shutdown + assert cookie_file.exists() # should not be deleted during the second dashd instance shutdown + pid_file = Path(datadir) / "dashd.pid" + assert pid_file.exists() if self.is_wallet_compiled(): def check_wallet_filelock(descriptors): diff --git a/test/functional/feature_settings.py b/test/functional/feature_settings.py index 7b295e854d6a..835cb5df5f58 100755 --- a/test/functional/feature_settings.py +++ b/test/functional/feature_settings.py @@ -24,10 +24,11 @@ def run_test(self): settings = Path(node.datadir, self.chain, "settings.json") conf = Path(node.datadir, "dash.conf") - # Assert empty settings file was created + # Assert default settings file was created self.stop_node(0) + default_settings = {"_warning_": "This file is automatically generated and updated by Dash Core. Please do not edit this file while the node is running, as any changes might be ignored or overwritten."} with settings.open() as fp: - assert_equal(json.load(fp), {}) + assert_equal(json.load(fp), default_settings) # Assert settings are parsed and logged with settings.open("w") as fp: @@ -49,12 +50,12 @@ def run_test(self): # Assert settings are unchanged after shutdown with settings.open() as fp: - assert_equal(json.load(fp), {"string": "string", "num": 5, "bool": True, "null": None, "list": [6, 7]}) + assert_equal(json.load(fp), {**default_settings, **{"string": "string", "num": 5, "bool": True, "null": None, "list": [6, 7]}}) # Test invalid json with settings.open("w") as fp: fp.write("invalid json") - node.assert_start_raises_init_error(expected_msg='Unable to parse settings file', match=ErrorMatch.PARTIAL_REGEX) + node.assert_start_raises_init_error(expected_msg='does not contain valid JSON. This is probably caused by disk corruption or a crash', match=ErrorMatch.PARTIAL_REGEX) # Test invalid json object with settings.open("w") as fp: diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index 2f74a60003ba..f436cccbe1d3 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -428,18 +428,46 @@ def _test_getdifficulty(self): def _test_getnetworkhashps(self): self.log.info("Test getnetworkhashps") - hashes_per_second = self.nodes[0].getnetworkhashps() + assert_raises_rpc_error( + -8, + "Block does not exist at specified height", + lambda: self.nodes[0].getnetworkhashps(100, self.nodes[0].getblockcount() + 1), + ) + assert_raises_rpc_error( + -8, + "Block does not exist at specified height", + lambda: self.nodes[0].getnetworkhashps(100, -10), + ) + assert_raises_rpc_error( + -8, + "Invalid nblocks. Must be a positive number or -1.", + lambda: self.nodes[0].getnetworkhashps(-100), + ) + assert_raises_rpc_error( + -8, + "Invalid nblocks. Must be a positive number or -1.", + lambda: self.nodes[0].getnetworkhashps(0), + ) + + # Genesis block height estimate should return 0 + hashes_per_second = self.nodes[0].getnetworkhashps(100, 0) + assert_equal(hashes_per_second, 0) + # This should be 2 hashes every 2.6 minutes (156 seconds) or 1/78 + hashes_per_second = self.nodes[0].getnetworkhashps() assert abs(hashes_per_second * 78 - 1) < 0.0001 - # Test setting the first param of getnetworkhashps to negative value returns the average network + # Test setting the first param of getnetworkhashps to -1 returns the average network # hashes per second from the last difficulty change. current_block_height = self.nodes[0].getmininginfo()['blocks'] blocks_since_last_diff_change = current_block_height % DIFFICULTY_ADJUSTMENT_INTERVAL + 1 expected_hashes_per_second_since_diff_change = self.nodes[0].getnetworkhashps(blocks_since_last_diff_change) assert_equal(self.nodes[0].getnetworkhashps(-1), expected_hashes_per_second_since_diff_change) - assert_equal(self.nodes[0].getnetworkhashps(-2), expected_hashes_per_second_since_diff_change) + + # Ensure long lookups get truncated to chain length + hashes_per_second = self.nodes[0].getnetworkhashps(self.nodes[0].getblockcount() + 1000) + assert hashes_per_second > 0.003 def _test_stopatheight(self): assert_equal(self.nodes[0].getblockcount(), HEIGHT) diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 5788ff158336..688f4a117de7 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -77,12 +77,21 @@ def run_test(self): self.nodes[0].walletpassphrase(passphrase="password", timeout=1000000) - # Sign the transaction and send - signed_tx = self.nodes[0].walletprocesspsbt(psbt=psbtx, finalize=False)['psbt'] - finalized_tx = self.nodes[0].walletprocesspsbt(psbt=psbtx, finalize=True)['psbt'] - assert signed_tx != finalized_tx - final_tx = self.nodes[0].finalizepsbt(signed_tx)['hex'] - self.nodes[0].sendrawtransaction(final_tx) + # Sign the transaction but don't finalize + processed_psbt = self.nodes[0].walletprocesspsbt(psbt=psbtx, finalize=False) + assert "hex" not in processed_psbt + signed_psbt = processed_psbt['psbt'] + + # Finalize and send + finalized_hex = self.nodes[0].finalizepsbt(signed_psbt)['hex'] + self.nodes[0].sendrawtransaction(finalized_hex) + + # Alternative method: sign AND finalize in one command + processed_finalized_psbt = self.nodes[0].walletprocesspsbt(psbt=psbtx, finalize=True) + finalized_psbt = processed_finalized_psbt['psbt'] + finalized_psbt_hex = processed_finalized_psbt['hex'] + assert signed_psbt != finalized_psbt + assert finalized_psbt_hex == finalized_hex # Manually selected inputs can be locked: assert_equal(len(self.nodes[0].listlockunspent()), 0) @@ -139,7 +148,7 @@ def run_test(self): # Check decodepsbt fee calculation (input values shall only be counted once per UTXO) assert_equal(decoded['fee'], created_psbt['fee']) assert_equal(walletprocesspsbt_out['complete'], True) - self.nodes[1].sendrawtransaction(self.nodes[1].finalizepsbt(walletprocesspsbt_out['psbt'])['hex']) + self.nodes[1].sendrawtransaction(walletprocesspsbt_out['hex']) self.log.info("Test walletcreatefundedpsbt fee rate of 10000 sat/vB and 0.1 BTC/kvB produces a total fee at or slightly below -maxtxfee (~0.05290000)") res1 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"fee_rate": 10000, "add_inputs": True}) @@ -230,7 +239,7 @@ def run_test(self): # partially sign with node 2. This should be complete and sendable walletprocesspsbt_out = self.nodes[2].walletprocesspsbt(psbtx) assert_equal(walletprocesspsbt_out['complete'], True) - self.nodes[2].sendrawtransaction(self.nodes[2].finalizepsbt(walletprocesspsbt_out['psbt'])['hex']) + self.nodes[2].sendrawtransaction(walletprocesspsbt_out['hex']) # check that walletprocesspsbt fails to decode a non-psbt rawtx = self.nodes[1].createrawtransaction([{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():9.99}) @@ -514,14 +523,13 @@ def test_psbt_input_keys(psbt_input, keys): assert not signed['complete'] signed = self.nodes[0].walletprocesspsbt(signed['psbt']) assert signed['complete'] - self.nodes[0].finalizepsbt(signed['psbt']) psbt = wallet.walletcreatefundedpsbt([ext_utxo], {self.nodes[0].getnewaddress(): 15}, 0, {"add_inputs": True, "solving_data":{"descriptors": [desc]}}) signed = wallet.walletprocesspsbt(psbt['psbt']) assert not signed['complete'] signed = self.nodes[0].walletprocesspsbt(signed['psbt']) assert signed['complete'] - final = self.nodes[0].finalizepsbt(signed['psbt'], False) + final = signed['hex'] dec = self.nodes[0].decodepsbt(signed["psbt"]) for i, txin in enumerate(dec["tx"]["vin"]): @@ -555,8 +563,8 @@ def test_psbt_input_keys(psbt_input, keys): ) signed = wallet.walletprocesspsbt(psbt["psbt"]) signed = self.nodes[0].walletprocesspsbt(signed["psbt"]) - final = self.nodes[0].finalizepsbt(signed["psbt"]) - assert self.nodes[0].testmempoolaccept([final["hex"]])[0]["allowed"] + final = signed["hex"] + assert self.nodes[0].testmempoolaccept([final])[0]["allowed"] # Reducing the weight should have a lower fee psbt2 = wallet.walletcreatefundedpsbt( inputs=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": low_input_weight}], diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index 2d54742337c4..a1c0aed0b1e3 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/test/functional/wallet_fundrawtransaction.py @@ -530,8 +530,7 @@ def test_spend_2of2(self): funded_psbt = wmulti.walletcreatefundedpsbt(inputs=inputs, outputs=outputs, options={'changeAddress': w2.getrawchangeaddress()})['psbt'] signed_psbt = w2.walletprocesspsbt(funded_psbt) - final_psbt = w2.finalizepsbt(signed_psbt['psbt']) - self.nodes[2].sendrawtransaction(final_psbt['hex']) + self.nodes[2].sendrawtransaction(signed_psbt['hex']) self.generate(self.nodes[2], 1) # Make sure funds are received at node1. diff --git a/test/functional/wallet_multisig_descriptor_psbt.py b/test/functional/wallet_multisig_descriptor_psbt.py index ccd89379a6de..40b291212e3d 100755 --- a/test/functional/wallet_multisig_descriptor_psbt.py +++ b/test/functional/wallet_multisig_descriptor_psbt.py @@ -150,8 +150,7 @@ def run_test(self): signing_wallet = participants["signers"][m] psbt = signing_wallet.walletprocesspsbt(psbt["psbt"]) assert_equal(psbt["complete"], m == self.M - 1) - finalized = coordinator_wallet.finalizepsbt(psbt["psbt"]) - coordinator_wallet.sendrawtransaction(finalized["hex"]) + coordinator_wallet.sendrawtransaction(psbt["hex"]) self.log.info("Check that balances are correct after the transaction has been included in a block.") self.generate(self.nodes[0], 1) diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index 289fa5468fe1..d7479267c26a 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -498,13 +498,11 @@ def run_test(self): signed = ext_wallet.walletprocesspsbt(res["psbt"]) signed = ext_fund.walletprocesspsbt(res["psbt"]) assert signed["complete"] - self.nodes[0].finalizepsbt(signed["psbt"]) res = self.test_send(from_wallet=ext_wallet, to_wallet=self.nodes[0], amount=15, inputs=[ext_utxo], add_inputs=True, psbt=True, include_watching=True, solving_data={"descriptors": [desc]}) signed = ext_wallet.walletprocesspsbt(res["psbt"]) signed = ext_fund.walletprocesspsbt(res["psbt"]) assert signed["complete"] - self.nodes[0].finalizepsbt(signed["psbt"]) dec = self.nodes[0].decodepsbt(signed["psbt"]) for i, txin in enumerate(dec["tx"]["vin"]): @@ -539,8 +537,7 @@ def run_test(self): signed = ext_wallet.walletprocesspsbt(res["psbt"]) signed = ext_fund.walletprocesspsbt(res["psbt"]) assert signed["complete"] - tx = self.nodes[0].finalizepsbt(signed["psbt"]) - testres = self.nodes[0].testmempoolaccept([tx["hex"]])[0] + testres = self.nodes[0].testmempoolaccept([signed["hex"]])[0] assert_equal(testres["allowed"], True) assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal(0.0001)) diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index eb0610cb256c..b77ab9097c7a 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -148,8 +148,7 @@ def test_valid_signer(self): dest = self.nodes[0].getnewaddress() mock_psbt = mock_wallet.walletcreatefundedpsbt([], {dest:0.5}, 0, {}, True)['psbt'] mock_psbt_signed = mock_wallet.walletprocesspsbt(psbt=mock_psbt, sign=True, sighashtype="ALL", bip32derivs=True) - mock_psbt_final = mock_wallet.finalizepsbt(mock_psbt_signed["psbt"]) - mock_tx = mock_psbt_final["hex"] + mock_tx = mock_psbt_signed["hex"] assert mock_wallet.testmempoolaccept([mock_tx])[0]["allowed"] # # Create a new wallet and populate with specific public keys, in order