diff --git a/src/core_read.cpp b/src/core_read.cpp index e1f778cab4..6aa270962d 100644 --- a/src/core_read.cpp +++ b/src/core_read.cpp @@ -17,10 +17,10 @@ #include -CScript ParseScript(const std::string& s) -{ - CScript result; +namespace { +opcodetype ParseOpCode(const std::string& s) +{ static std::map mapOpNames; if (mapOpNames.empty()) @@ -42,6 +42,17 @@ CScript ParseScript(const std::string& s) } } } + auto it = mapOpNames.find(s); + if (it == mapOpNames.end()) throw std::runtime_error("script parse error: unknown opcode"); + return it->second; +} + +} // namespace + +CScript ParseScript(const std::string& s) +{ + CScript result; + std::vector words = SplitString(s, " \t\n"); @@ -79,14 +90,10 @@ CScript ParseScript(const std::string& s) std::vector value(w->begin()+1, w->end()-1); result << value; } - else if (mapOpNames.count(*w)) - { - // opcode, e.g. OP_ADD or ADD: - result << mapOpNames[*w]; - } else { - throw std::runtime_error("script parse error"); + // opcode, e.g. OP_ADD or ADD: + result << ParseOpCode(*w); } } diff --git a/src/net.cpp b/src/net.cpp index be3147e160..71843e8ded 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -676,34 +676,21 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) } #undef X -/** - * Receive bytes from the buffer and deserialize them into messages. - * - * @param[in] pch A pointer to the raw data - * @param[in] nBytes Size of the data - * @param[out] complete Set True if at least one message has been - * deserialized and is ready to be processed - * @return True if the peer should stay connected, - * False if the peer should be disconnected from. - */ -bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete) +bool CNode::ReceiveMsgBytes(Span msg_bytes, bool& complete) { complete = false; int64_t nTimeMicros = GetTimeMicros(); LOCK(cs_vRecv); nLastRecv = nTimeMicros / 1000000; - nRecvBytes += nBytes; - while (nBytes > 0) { + nRecvBytes += msg_bytes.size(); + while (msg_bytes.size() > 0) { // absorb network data - int handled = m_deserializer->Read(pch, nBytes); + int handled = m_deserializer->Read(msg_bytes); if (handled < 0) { // Serious header problem, disconnect from the peer. return false; } - pch += handled; - nBytes -= handled; - if (m_deserializer->Complete()) { // decompose a transport agnostic CNetMessage from the deserializer uint32_t out_err_raw_size{0}; @@ -760,13 +747,13 @@ int CNode::GetSendVersion() const return nSendVersion; } -int V1TransportDeserializer::readHeader(const char *pch, unsigned int nBytes) +int V1TransportDeserializer::readHeader(Span msg_bytes) { // copy data to temporary parsing buffer unsigned int nRemaining = 24 - nHdrPos; - unsigned int nCopy = std::min(nRemaining, nBytes); + unsigned int nCopy = std::min(nRemaining, msg_bytes.size()); - memcpy(&hdrbuf[nHdrPos], pch, nCopy); + memcpy(&hdrbuf[nHdrPos], msg_bytes.data(), nCopy); nHdrPos += nCopy; // if header incomplete, exit @@ -800,18 +787,18 @@ int V1TransportDeserializer::readHeader(const char *pch, unsigned int nBytes) return nCopy; } -int V1TransportDeserializer::readData(const char *pch, unsigned int nBytes) +int V1TransportDeserializer::readData(Span msg_bytes) { unsigned int nRemaining = hdr.nMessageSize - nDataPos; - unsigned int nCopy = std::min(nRemaining, nBytes); + unsigned int nCopy = std::min(nRemaining, msg_bytes.size()); if (vRecv.size() < nDataPos + nCopy) { // Allocate up to 256 KiB ahead, but never more than the total message size. vRecv.resize(std::min(hdr.nMessageSize, nDataPos + nCopy + 256 * 1024)); } - hasher.Write({(const unsigned char*)pch, nCopy}); - memcpy(&vRecv[nDataPos], pch, nCopy); + hasher.Write(msg_bytes.first(nCopy)); + memcpy(&vRecv[nDataPos], msg_bytes.data(), nCopy); nDataPos += nCopy; return nCopy; @@ -1925,13 +1912,13 @@ void CConnman::SocketHandler() size_t CConnman::SocketRecvData(CNode *pnode) { // typical socket buffer is 8K-64K - char pchBuf[0x10000]; + uint8_t pchBuf[0x10000]; int nBytes = 0; { LOCK(pnode->cs_hSocket); if (pnode->hSocket == INVALID_SOCKET) return 0; - nBytes = recv(pnode->hSocket, pchBuf, sizeof(pchBuf), MSG_DONTWAIT); + nBytes = recv(pnode->hSocket, (char*)pchBuf, sizeof(pchBuf), MSG_DONTWAIT); if (nBytes < (int)sizeof(pchBuf)) { pnode->fHasRecvData = false; } @@ -1939,7 +1926,7 @@ size_t CConnman::SocketRecvData(CNode *pnode) if (nBytes > 0) { bool notify = false; - if (!pnode->ReceiveMsgBytes(pchBuf, nBytes, notify)) { + if (!pnode->ReceiveMsgBytes(Span(pchBuf, nBytes), notify)) { LOCK(cs_vNodes); pnode->CloseSocketDisconnect(this); } @@ -3080,17 +3067,6 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) { Init(connOptions); - { - LOCK(cs_totalBytesRecv); - nTotalBytesRecv = 0; - } - { - LOCK(cs_totalBytesSent); - nTotalBytesSent = 0; - nMaxOutboundTotalBytesSentInCycle = 0; - nMaxOutboundCycleStartTime = 0s; - } - #ifdef USE_KQUEUE if (socketEventsMode == SOCKETEVENTS_KQUEUE) { kqueuefd = kqueue(); diff --git a/src/net.h b/src/net.h index 5f159a0959..359c00b795 100644 --- a/src/net.h +++ b/src/net.h @@ -591,8 +591,8 @@ private: uint64_t nTotalBytesSent GUARDED_BY(cs_totalBytesSent) {0}; // outbound limit & stats - uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent); - std::chrono::seconds nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent); + uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent) {0}; + std::chrono::seconds nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent) {0}; uint64_t nMaxOutboundLimit GUARDED_BY(cs_totalBytesSent); // P2P timeout in seconds @@ -933,8 +933,8 @@ public: virtual bool Complete() const = 0; // set the serialization context version virtual void SetVersion(int version) = 0; - // read and deserialize data - virtual int Read(const char *data, unsigned int bytes) = 0; + /** read and deserialize data, advances msg_bytes data pointer */ + virtual int Read(Span& msg_bytes) = 0; // decomposes a message from the context virtual std::optional GetMessage(int64_t time, uint32_t& out_err) = 0; virtual ~TransportDeserializer() {} @@ -955,8 +955,8 @@ private: unsigned int nDataPos; const uint256& GetMessageHash() const; - int readHeader(const char *pch, unsigned int nBytes); - int readData(const char *pch, unsigned int nBytes); + int readHeader(Span msg_bytes); + int readData(Span msg_bytes); void Reset() { vRecv.clear(); @@ -990,9 +990,14 @@ public: hdrbuf.SetVersion(nVersionIn); vRecv.SetVersion(nVersionIn); } - int Read(const char *pch, unsigned int nBytes) override { - int ret = in_data ? readData(pch, nBytes) : readHeader(pch, nBytes); - if (ret < 0) Reset(); + int Read(Span& msg_bytes) override + { + int ret = in_data ? readData(msg_bytes) : readHeader(msg_bytes); + if (ret < 0) { + Reset(); + } else { + msg_bytes = msg_bytes.subspan(ret); + } return ret; } std::optional GetMessage(int64_t time, uint32_t& out_err_raw_size) override; @@ -1271,7 +1276,16 @@ public: return nRefCount; } - bool ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete); + /** + * Receive bytes from the buffer and deserialize them into messages. + * + * @param[in] msg_bytes The raw data + * @param[out] complete Set True if at least one message has been + * deserialized and is ready to be processed + * @return True if the peer should stay connected, + * False if the peer should be disconnected from. + */ + bool ReceiveMsgBytes(Span msg_bytes, bool& complete); void SetRecvVersion(int nVersionIn) { diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0c8ae1f490..a53d9ee917 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -589,13 +589,13 @@ struct CNodeState { */ struct ChainSyncTimeoutState { //! A timeout used for checking whether our peer has sufficiently synced - int64_t m_timeout; + int64_t m_timeout{0}; //! A header with the work we require on our peer's chain - const CBlockIndex * m_work_header; + const CBlockIndex* m_work_header{nullptr}; //! After timeout is reached, set to true after sending getheaders - bool m_sent_getheaders; + bool m_sent_getheaders{false}; //! Whether this peer is protected from disconnection due to a bad/slow chain - bool m_protect; + bool m_protect{false}; }; ChainSyncTimeoutState m_chain_sync; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 072d56d6e2..1e0797cd32 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -352,7 +352,7 @@ static UniValue generateblock(const JSONRPCRequest& request) txs.push_back(MakeTransactionRef(std::move(mtx))); } else { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("Transaction decode failed for %s", str)); + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("Transaction decode failed for %s. Make sure the tx has at least one input.", str)); } } diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp index 993368f7a9..6bec4dfec8 100644 --- a/src/rpc/misc.cpp +++ b/src/rpc/misc.cpp @@ -1097,7 +1097,7 @@ static UniValue mockscheduler(const JSONRPCRequest& request) // check params are valid values RPCTypeCheck(request.params, {UniValue::VNUM}); int64_t delta_seconds = request.params[0].get_int64(); - if ((delta_seconds <= 0) || (delta_seconds > 3600)) { + if (delta_seconds <= 0 || delta_seconds > 3600) { throw std::runtime_error("delta_time must be between 1 and 3600 seconds (1 hr)"); } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 0e2b34d60a..5d2f8334a1 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -636,7 +636,7 @@ static UniValue combinerawtransaction(const JSONRPCRequest& request) for (unsigned int idx = 0; idx < txs.size(); idx++) { if (!DecodeHexTx(txVariants[idx], txs[idx].get_str())) { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("TX decode failed for tx %d", idx)); + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("TX decode failed for tx %d. Make sure the tx has at least one input.", idx)); } } @@ -757,7 +757,7 @@ static UniValue signrawtransactionwithkey(const JSONRPCRequest& request) CMutableTransaction mtx; if (!DecodeHexTx(mtx, request.params[0].get_str())) { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); } FillableSigningProvider keystore; @@ -823,10 +823,10 @@ UniValue sendrawtransaction(const JSONRPCRequest& request) UniValue::VBOOL }); - // parse hex string from parameter CMutableTransaction mtx; - if (!DecodeHexTx(mtx, request.params[0].get_str())) - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); + if (!DecodeHexTx(mtx, request.params[0].get_str())) { + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); + } CTransactionRef tx(MakeTransactionRef(std::move(mtx))); const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull() ? @@ -898,7 +898,7 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request) CMutableTransaction mtx; if (!DecodeHexTx(mtx, request.params[0].get_array()[0].get_str())) { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); } CTransactionRef tx(MakeTransactionRef(std::move(mtx))); const uint256& tx_hash = tx->GetHash(); diff --git a/src/scheduler.cpp b/src/scheduler.cpp index 0b08ff2d8b..a3a37e7c15 100644 --- a/src/scheduler.cpp +++ b/src/scheduler.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include @@ -79,7 +80,7 @@ void CScheduler::schedule(CScheduler::Function f, std::chrono::system_clock::tim void CScheduler::MockForward(std::chrono::seconds delta_seconds) { - assert(delta_seconds.count() > 0 && delta_seconds < std::chrono::hours{1}); + assert(delta_seconds > 0s && delta_seconds <= 1h); { LOCK(newTaskMutex); diff --git a/src/sync.cpp b/src/sync.cpp index b9bc05c8ea..d57bd79e7e 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -13,11 +13,12 @@ #include #include - #include +#include #include #include #include +#include #include #include #include @@ -139,16 +140,50 @@ static void potential_deadlock_detected(const LockPair& mismatch, const LockStac throw std::logic_error(strprintf("potential deadlock detected: %s -> %s -> %s", mutex_b, mutex_a, mutex_b)); } -static void push_lock(void* c, const CLockLocation& locklocation) +static void double_lock_detected(const void* mutex, LockStack& lock_stack) { + LogPrintf("DOUBLE LOCK DETECTED\n"); + LogPrintf("Lock order:\n"); + for (const LockStackItem& i : lock_stack) { + if (i.first == mutex) { + LogPrintf(" (*)"); /* Continued */ + } + LogPrintf(" %s\n", i.second.ToString()); + } + if (g_debug_lockorder_abort) { + tfm::format(std::cerr, "Assertion failed: detected double lock at %s:%i, details in debug log.\n", __FILE__, __LINE__); + abort(); + } + throw std::logic_error("double lock detected"); +} + +template +static void push_lock(MutexType* c, const CLockLocation& locklocation) +{ + constexpr bool is_recursive_mutex = + std::is_base_of::value || + std::is_base_of::value; + LockData& lockdata = GetLockData(); std::lock_guard lock(lockdata.dd_mutex); LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; lock_stack.emplace_back(c, locklocation); - for (const LockStackItem& i : lock_stack) { - if (i.first == c) - break; + for (size_t j = 0; j < lock_stack.size() - 1; ++j) { + const LockStackItem& i = lock_stack[j]; + if (i.first == c) { + if (is_recursive_mutex) { + break; + } + // It is not a recursive mutex and it appears in the stack two times: + // at position `j` and at the end (which we added just before this loop). + // Can't allow locking the same (non-recursive) mutex two times from the + // same thread as that results in an undefined behavior. + auto lock_stack_copy = lock_stack; + lock_stack.pop_back(); + double_lock_detected(c, lock_stack_copy); + // double_lock_detected() does not return. + } const LockPair p1 = std::make_pair(i.first, c); if (lockdata.lockorders.count(p1)) @@ -179,10 +214,15 @@ static void pop_lock() } } -void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry) +template +void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry) { push_lock(cs, CLockLocation(pszName, pszFile, nLine, fTry, util::ThreadGetInternalName())); } +template void EnterCritical(const char*, const char*, int, Mutex*, bool); +template void EnterCritical(const char*, const char*, int, RecursiveMutex*, bool); +template void EnterCritical(const char*, const char*, int, std::mutex*, bool); +template void EnterCritical(const char*, const char*, int, std::recursive_mutex*, bool); void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) { diff --git a/src/sync.h b/src/sync.h index a5a583810d..aed8c697b5 100644 --- a/src/sync.h +++ b/src/sync.h @@ -48,7 +48,8 @@ LEAVE_CRITICAL_SECTION(mutex); // no RAII /////////////////////////////// #ifdef DEBUG_LOCKORDER -void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false); +template +void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry = false); void LeaveCritical(); void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line); std::string LocksHeld(); @@ -65,7 +66,8 @@ bool LockStackEmpty(); */ extern bool g_debug_lockorder_abort; #else -inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {} +template +inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry = false) {} inline void LeaveCritical() {} inline void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {} template @@ -133,7 +135,7 @@ class SCOPED_LOCKABLE UniqueLock : public Base private: void Enter(const char* pszName, const char* pszFile, int nLine) { - EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex())); + EnterCritical(pszName, pszFile, nLine, Base::mutex()); #ifdef DEBUG_LOCKCONTENTION if (!Base::try_lock()) { PrintLockContention(pszName, pszFile, nLine); @@ -146,7 +148,7 @@ private: bool TryEnter(const char* pszName, const char* pszFile, int nLine) { - EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex()), true); + EnterCritical(pszName, pszFile, nLine, Base::mutex(), true); Base::try_lock(); if (!Base::owns_lock()) LeaveCritical(); @@ -203,7 +205,7 @@ public: ~reverse_lock() { templock.swap(lock); - EnterCritical(lockname.c_str(), file.c_str(), line, (void*)lock.mutex()); + EnterCritical(lockname.c_str(), file.c_str(), line, lock.mutex()); lock.lock(); } @@ -234,7 +236,7 @@ using DebugLock = UniqueLock b = ConsumeRandomLengthByteVector(fuzzed_data_provider); bool complete; - node.ReceiveMsgBytes((const char*)b.data(), b.size(), complete); + node.ReceiveMsgBytes(b, complete); }); } diff --git a/src/test/fuzz/p2p_transport_deserializer.cpp b/src/test/fuzz/p2p_transport_deserializer.cpp index 0786fdb73c..599dc0d044 100644 --- a/src/test/fuzz/p2p_transport_deserializer.cpp +++ b/src/test/fuzz/p2p_transport_deserializer.cpp @@ -22,15 +22,12 @@ FUZZ_TARGET_INIT(p2p_transport_deserializer, initialize_p2p_transport_deserializ { // Construct deserializer, with a dummy NodeId V1TransportDeserializer deserializer{Params(), (NodeId)0, SER_NETWORK, INIT_PROTO_VERSION}; - const char* pch = (const char*)buffer.data(); - size_t n_bytes = buffer.size(); - while (n_bytes > 0) { - const int handled = deserializer.Read(pch, n_bytes); + Span msg_bytes{buffer}; + while (msg_bytes.size() > 0) { + const int handled = deserializer.Read(msg_bytes); if (handled < 0) { break; } - pch += handled; - n_bytes -= handled; if (deserializer.Complete()) { const int64_t m_time = std::numeric_limits::max(); uint32_t out_err_raw_size{0}; diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp index 04cb96d4ad..25fca556a7 100644 --- a/src/test/sync_tests.cpp +++ b/src/test/sync_tests.cpp @@ -7,6 +7,8 @@ #include +#include + namespace { template void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2) @@ -42,6 +44,38 @@ void TestInconsistentLockOrderDetected(MutexType& mutex1, MutexType& mutex2) NO_ LEAVE_CRITICAL_SECTION(mutex1); BOOST_CHECK(LockStackEmpty()); } + +#ifdef DEBUG_LOCKORDER +template +void TestDoubleLock2(MutexType& m) +{ + ENTER_CRITICAL_SECTION(m); + LEAVE_CRITICAL_SECTION(m); +} + +template +void TestDoubleLock(bool should_throw) +{ + const bool prev = g_debug_lockorder_abort; + g_debug_lockorder_abort = false; + + MutexType m; + ENTER_CRITICAL_SECTION(m); + if (should_throw) { + BOOST_CHECK_EXCEPTION( + TestDoubleLock2(m), std::logic_error, [](const std::logic_error& e) { + return strcmp(e.what(), "double lock detected") == 0; + }); + } else { + BOOST_CHECK_NO_THROW(TestDoubleLock2(m)); + } + LEAVE_CRITICAL_SECTION(m); + + BOOST_CHECK(LockStackEmpty()); + + g_debug_lockorder_abort = prev; +} +#endif /* DEBUG_LOCKORDER */ } // namespace BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup) @@ -92,4 +126,19 @@ BOOST_AUTO_TEST_CASE(inconsistent_lock_order_detected) #endif // DEBUG_LOCKORDER } +/* Double lock would produce an undefined behavior. Thus, we only do that if + * DEBUG_LOCKORDER is activated to detect it. We don't want non-DEBUG_LOCKORDER + * build to produce tests that exhibit known undefined behavior. */ +#ifdef DEBUG_LOCKORDER +BOOST_AUTO_TEST_CASE(double_lock_mutex) +{ + TestDoubleLock(true /* should throw */); +} + +BOOST_AUTO_TEST_CASE(double_lock_recursive_mutex) +{ + TestDoubleLock(false /* should not throw */); +} +#endif /* DEBUG_LOCKORDER */ + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 09f2f1807f..847a490e03 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -7,9 +7,9 @@ #include #include -void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, const char* pch, unsigned int nBytes, bool& complete) const +void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const { - assert(node.ReceiveMsgBytes(pch, nBytes, complete)); + assert(node.ReceiveMsgBytes(msg_bytes, complete)); if (complete) { size_t nSizeAdded = 0; auto it(node.vRecvMsg.begin()); @@ -29,11 +29,11 @@ void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, const char* pch, unsigned bool ConnmanTestMsg::ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const { - std::vector ser_msg_header; + std::vector ser_msg_header; node.m_serializer->prepareForTransport(ser_msg, ser_msg_header); bool complete; - NodeReceiveMsgBytes(node, (const char*)ser_msg_header.data(), ser_msg_header.size(), complete); - NodeReceiveMsgBytes(node, (const char*)ser_msg.data.data(), ser_msg.data.size(), complete); + NodeReceiveMsgBytes(node, ser_msg_header, complete); + NodeReceiveMsgBytes(node, ser_msg.data, complete); return complete; } diff --git a/src/test/util/net.h b/src/test/util/net.h index 8403ea9608..0cdebe4678 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -31,7 +31,7 @@ struct ConnmanTestMsg : public CConnman { void ProcessMessagesOnce(CNode& node) { m_msgproc->ProcessMessages(&node, flagInterruptMsgProc); } - void NodeReceiveMsgBytes(CNode& node, const char* pch, unsigned int nBytes, bool& complete) const; + void NodeReceiveMsgBytes(CNode& node, Span msg_bytes, bool& complete) const; bool ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const; }; diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index c4d227b04f..d6706fd4aa 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -298,8 +298,9 @@ UniValue importprunedfunds(const JSONRPCRequest& request) CWallet* const pwallet = wallet.get(); CMutableTransaction tx; - if (!DecodeHexTx(tx, request.params[0].get_str())) - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); + if (!DecodeHexTx(tx, request.params[0].get_str())) { + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); + } uint256 hashTx = tx.GetHash(); CWalletTx wtx(pwallet, MakeTransactionRef(std::move(tx))); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 3a6609bd60..a19490c298 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2961,7 +2961,7 @@ static UniValue unloadwallet(const JSONRPCRequest& request) "Unloads the wallet referenced by the request endpoint otherwise unloads the wallet specified in the argument.\n" "Specifying the wallet name on a wallet endpoint is invalid.", { - {"wallet_name", RPCArg::Type::STR, /* default */ "the wallet name from the RPC endpoint", "The name of the wallet to unload. Must be provided in the RPC endpoint or this parameter (but not both)."}, + {"wallet_name", RPCArg::Type::STR, /* default */ "the wallet name from the RPC endpoint", "The name of the wallet to unload. If provided both here and in the RPC endpoint, the two must be identical."}, {"load_on_startup", RPCArg::Type::BOOL, /* default */ "null", "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."}, }, RPCResult{RPCResult::Type::OBJ, "", "", { @@ -2975,8 +2975,8 @@ static UniValue unloadwallet(const JSONRPCRequest& request) std::string wallet_name; if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) { - if (!request.params[0].isNull()) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Both the RPC endpoint wallet and wallet_name parameter were provided (only one allowed)"); + if (!(request.params[0].isNull() || request.params[0].get_str() == wallet_name)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "RPC endpoint wallet and wallet_name parameter specify different wallets"); } } else { wallet_name = request.params[0].get_str(); @@ -3464,7 +3464,7 @@ UniValue signrawtransactionwithwallet(const JSONRPCRequest& request) CMutableTransaction mtx; if (!DecodeHexTx(mtx, request.params[0].get_str())) { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); } // Sign the transaction diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index 3752ddf520..f9c98caba5 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -339,13 +339,18 @@ class MultiWalletTest(BitcoinTestFramework): assert_raises_rpc_error(-1, "JSON value is not a string as expected", self.nodes[0].unloadwallet) assert_raises_rpc_error(-18, "Requested wallet does not exist or is not loaded", self.nodes[0].unloadwallet, "dummy") assert_raises_rpc_error(-18, "Requested wallet does not exist or is not loaded", node.get_wallet_rpc("dummy").unloadwallet) - assert_raises_rpc_error(-8, "Both the RPC endpoint wallet and wallet_name parameter were provided (only one allowed)", w1.unloadwallet, "w2"), - assert_raises_rpc_error(-8, "Both the RPC endpoint wallet and wallet_name parameter were provided (only one allowed)", w1.unloadwallet, "w1"), + assert_raises_rpc_error(-8, "RPC endpoint wallet and wallet_name parameter specify different wallets", w1.unloadwallet, "w2"), # Successfully unload the specified wallet name self.nodes[0].unloadwallet("w1") assert 'w1' not in self.nodes[0].listwallets() + # Unload w1 again, this time providing the wallet name twice + self.nodes[0].loadwallet("w1") + assert 'w1' in self.nodes[0].listwallets() + w1.unloadwallet("w1") + assert 'w1' not in self.nodes[0].listwallets() + # Successfully unload the wallet referenced by the request endpoint # Also ensure unload works during walletpassphrase timeout w2.encryptwallet('test') diff --git a/test/util/data/bitcoin-util-test.json b/test/util/data/bitcoin-util-test.json index a84b7c9ea1..db9622d371 100644 --- a/test/util/data/bitcoin-util-test.json +++ b/test/util/data/bitcoin-util-test.json @@ -202,7 +202,7 @@ { "exec": "./dash-tx", "args": ["-create", "outscript=0:123badscript"], "return_code": 1, - "error_txt": "error: script parse error", + "error_txt": "error: script parse error: unknown opcode", "description": "Create a new transaction with an invalid output script" }, { "exec": "./dash-tx",