From c51cec606d31c1400c4a04e216d343989c934d26 Mon Sep 17 00:00:00 2001 From: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Date: Sun, 22 Oct 2023 09:14:30 -0500 Subject: [PATCH] refactor: add gsl::not_null to get compile time / run time pointer guarantees (#5595) ## Issue being fixed or feature implemented Current implementation relies either on asserts or sometimes checks then returning a special value; In the case of asserts (or no assert where we use the value without checks) it'd be better to make it explicit to function caller that the ptr must be not_null; otherwise gsl::not_null will call terminate. See https://github.com/microsoft/GSL/blob/main/docs/headers.md#user-content-H-pointers-not_null and https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-nullptr I'm interested in a conceptual review; specifically on if this is beneficial over just converting these ptrs to be a reference? ## What was done? *Partial* implementation on using gsl::not_null in dash code ## How Has This Been Tested? Building ## Breaking Changes None ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ --------- Signed-off-by: pasta Co-authored-by: UdjinM6 --- src/Makefile.am | 3 + src/evo/assetlocktx.cpp | 6 +- src/evo/assetlocktx.h | 7 +- src/gsl/assert.h | 144 +++++++++++++ src/gsl/pointers.h | 348 +++++++++++++++++++++++++++++++ src/llmq/blockprocessor.cpp | 10 +- src/llmq/blockprocessor.h | 12 +- src/llmq/chainlocks.cpp | 6 +- src/llmq/chainlocks.h | 8 +- src/llmq/commitment.cpp | 4 +- src/llmq/commitment.h | 6 +- src/llmq/dkgsession.cpp | 2 +- src/llmq/dkgsession.h | 2 +- src/llmq/instantsend.cpp | 7 +- src/llmq/instantsend.h | 8 +- src/llmq/quorums.cpp | 8 +- src/llmq/quorums.h | 6 +- src/source_location.h | 76 +++++++ test/lint/lint-include-guards.sh | 2 +- 19 files changed, 622 insertions(+), 43 deletions(-) create mode 100644 src/gsl/assert.h create mode 100644 src/gsl/pointers.h create mode 100644 src/source_location.h diff --git a/src/Makefile.am b/src/Makefile.am index 2f223b6c79..1121842d85 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -191,6 +191,8 @@ BITCOIN_CORE_H = \ governance/validators.h \ governance/vote.h \ governance/votedb.h \ + gsl/assert.h \ + gsl/pointers.h \ flat-database.h \ hdchain.h \ flatfile.h \ @@ -287,6 +289,7 @@ BITCOIN_CORE_H = \ script/signingprovider.h \ script/standard.h \ shutdown.h \ + source_location.h \ spork.h \ stacktraces.h \ streams.h \ diff --git a/src/evo/assetlocktx.cpp b/src/evo/assetlocktx.cpp index 3a094b7c0e..2476de6a07 100644 --- a/src/evo/assetlocktx.cpp +++ b/src/evo/assetlocktx.cpp @@ -23,7 +23,7 @@ /** * Common code for Asset Lock and Asset Unlock */ -bool CheckAssetLockUnlockTx(const CTransaction& tx, const CBlockIndex* pindexPrev, const std::optional& indexes, TxValidationState& state) +bool CheckAssetLockUnlockTx(const CTransaction& tx, gsl::not_null pindexPrev, const std::optional& indexes, TxValidationState& state) { switch (tx.nType) { case TRANSACTION_ASSET_LOCK: @@ -108,7 +108,7 @@ std::string CAssetLockPayload::ToString() const const std::string ASSETUNLOCK_REQUESTID_PREFIX = "plwdtx"; -bool CAssetUnlockPayload::VerifySig(const uint256& msgHash, const CBlockIndex* pindexTip, TxValidationState& state) const +bool CAssetUnlockPayload::VerifySig(const uint256& msgHash, gsl::not_null pindexTip, TxValidationState& state) const { // That quourm hash must be active at `requestHeight`, // and at the quorumHash must be active in either the current or previous quorum cycle @@ -143,7 +143,7 @@ bool CAssetUnlockPayload::VerifySig(const uint256& msgHash, const CBlockIndex* p return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-not-verified"); } -bool CheckAssetUnlockTx(const CTransaction& tx, const CBlockIndex* pindexPrev, const std::optional& indexes, TxValidationState& state) +bool CheckAssetUnlockTx(const CTransaction& tx, gsl::not_null pindexPrev, const std::optional& indexes, TxValidationState& state) { // Some checks depends from blockchain status also, such as `known indexes` and `withdrawal limits` // They are omitted here and done by CCreditPool diff --git a/src/evo/assetlocktx.h b/src/evo/assetlocktx.h index ebf9c175b4..b105dfd320 100644 --- a/src/evo/assetlocktx.h +++ b/src/evo/assetlocktx.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -128,7 +129,7 @@ public: return obj; } - bool VerifySig(const uint256& msgHash, const CBlockIndex* pindexTip, TxValidationState& state) const; + bool VerifySig(const uint256& msgHash, gsl::not_null pindexTip, TxValidationState& state) const; // getters uint8_t getVersion() const @@ -170,8 +171,8 @@ public: }; bool CheckAssetLockTx(const CTransaction& tx, TxValidationState& state); -bool CheckAssetUnlockTx(const CTransaction& tx, const CBlockIndex* pindexPrev, const std::optional& indexes, TxValidationState& state); -bool CheckAssetLockUnlockTx(const CTransaction& tx, const CBlockIndex* pindexPrev, const std::optional& indexes, TxValidationState& state); +bool CheckAssetUnlockTx(const CTransaction& tx, gsl::not_null pindexPrev, const std::optional& indexes, TxValidationState& state); +bool CheckAssetLockUnlockTx(const CTransaction& tx, gsl::not_null pindexPrev, const std::optional& indexes, TxValidationState& state); bool GetAssetUnlockFee(const CTransaction& tx, CAmount& txfee, TxValidationState& state); #endif // BITCOIN_EVO_ASSETLOCKTX_H diff --git a/src/gsl/assert.h b/src/gsl/assert.h new file mode 100644 index 0000000000..9cfad78755 --- /dev/null +++ b/src/gsl/assert.h @@ -0,0 +1,144 @@ +/////////////////////////////////////////////////////////////////////////////// +// +// Copyright (c) 2015 Microsoft Corporation. All rights reserved. +// +// This code is licensed under the MIT License (MIT). +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +// +/////////////////////////////////////////////////////////////////////////////// + +#ifndef GSL_ASSERT_H +#define GSL_ASSERT_H + +#include +#include + +#include + +// +// Temporary until MSVC STL supports no-exceptions mode. +// Currently terminate is a no-op in this mode, so we add termination behavior back +// +#if defined(_MSC_VER) && (defined(_KERNEL_MODE) || (defined(_HAS_EXCEPTIONS) && !_HAS_EXCEPTIONS)) +#define GSL_KERNEL_MODE + +#define GSL_MSVC_USE_STL_NOEXCEPTION_WORKAROUND +#include +#define RANGE_CHECKS_FAILURE 0 + +#if defined(__clang__) +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Winvalid-noreturn" +#endif // defined(__clang__) + +#else // defined(_MSC_VER) && (defined(_KERNEL_MODE) || (defined(_HAS_EXCEPTIONS) && +// !_HAS_EXCEPTIONS)) + +#include + +#endif // defined(_MSC_VER) && (defined(_KERNEL_MODE) || (defined(_HAS_EXCEPTIONS) && +// !_HAS_EXCEPTIONS)) + +// +// make suppress attributes parse for some compilers +// Hopefully temporary until suppression standardization occurs +// +#if defined(__clang__) +#define GSL_SUPPRESS(x) [[gsl::suppress(#x)]] +#else +#if defined(_MSC_VER) && !defined(__INTEL_COMPILER) && !defined(__NVCC__) +#define GSL_SUPPRESS(x) [[gsl::suppress(x)]] +#else +#define GSL_SUPPRESS(x) +#endif // _MSC_VER +#endif // __clang__ + +#if defined(__clang__) || defined(__GNUC__) +#define GSL_LIKELY(x) __builtin_expect(!!(x), 1) +#define GSL_UNLIKELY(x) __builtin_expect(!!(x), 0) + +#else + +#define GSL_LIKELY(x) (!!(x)) +#define GSL_UNLIKELY(x) (!!(x)) +#endif // defined(__clang__) || defined(__GNUC__) + +// +// GSL_ASSUME(cond) +// +// Tell the optimizer that the predicate cond must hold. It is unspecified +// whether or not cond is actually evaluated. +// +#ifdef _MSC_VER +#define GSL_ASSUME(cond) __assume(cond) +#elif defined(__GNUC__) +#define GSL_ASSUME(cond) ((cond) ? static_cast(0) : __builtin_unreachable()) +#else +#define GSL_ASSUME(cond) static_cast((cond) ? 0 : 0) +#endif + +// +// GSL.assert: assertions +// + +namespace gsl +{ + + namespace details + { +#if defined(GSL_MSVC_USE_STL_NOEXCEPTION_WORKAROUND) + + typedef void(__cdecl* terminate_handler)(); + + // clang-format off + GSL_SUPPRESS(f.6) // NO-FORMAT: attribute + // clang-format on + [[noreturn]] inline void __cdecl default_terminate_handler() + { + __fastfail(RANGE_CHECKS_FAILURE); + } + + inline gsl::details::terminate_handler& get_terminate_handler() noexcept + { + static terminate_handler handler = &default_terminate_handler; + return handler; + } + +#endif // defined(GSL_MSVC_USE_STL_NOEXCEPTION_WORKAROUND) + + [[noreturn]] inline void terminate(nostd::source_location loc) noexcept + { +#if defined(GSL_MSVC_USE_STL_NOEXCEPTION_WORKAROUND) + (*gsl::details::get_terminate_handler())(); +#else + std::ostringstream s; + s << "ERROR: error detected null not_null detected at " << loc.file_name() << ":" << loc.line() << ":" + << loc.column() << ":" << loc.function_name() + << std::endl; + std::cerr << s.str() << std::flush; + LogPrintf("%s", s.str()); /* Continued */ + std::terminate(); +#endif // defined(GSL_MSVC_USE_STL_NOEXCEPTION_WORKAROUND) + } + + } // namespace details +} // namespace gsl + +#define GSL_CONTRACT_CHECK(type, cond, loc) \ + (GSL_LIKELY(cond) ? static_cast(0) : gsl::details::terminate(loc)) + +#define Expects(cond, loc) GSL_CONTRACT_CHECK("Precondition", cond, loc) +#define Ensures(cond, loc) GSL_CONTRACT_CHECK("Postcondition", cond, loc) + +#if defined(GSL_MSVC_USE_STL_NOEXCEPTION_WORKAROUND) && defined(__clang__) +#pragma clang diagnostic pop +#endif + +#endif // GSL_ASSERT_H diff --git a/src/gsl/pointers.h b/src/gsl/pointers.h new file mode 100644 index 0000000000..bf444ef5f2 --- /dev/null +++ b/src/gsl/pointers.h @@ -0,0 +1,348 @@ +/////////////////////////////////////////////////////////////////////////////// +// +// Copyright (c) 2015 Microsoft Corporation. All rights reserved. +// +// This code is licensed under the MIT License (MIT). +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +// +/////////////////////////////////////////////////////////////////////////////// + +#ifndef GSL_POINTERS_H +#define GSL_POINTERS_H + +#include // for Ensures, Expects +#include + +#include // for forward +#include // for ptrdiff_t, nullptr_t, size_t +#include // for shared_ptr, unique_ptr +#include // for hash +#include // for enable_if_t, is_convertible, is_assignable +#include // for declval + +#if !defined(GSL_NO_IOSTREAMS) +#include // for ostream +#endif // !defined(GSL_NO_IOSTREAMS) + +namespace gsl +{ + + namespace details + { + template + struct is_comparable_to_nullptr : std::false_type + { + }; + + template + struct is_comparable_to_nullptr< + T, + std::enable_if_t() != nullptr), bool>::value>> + : std::true_type + { + }; + + // Resolves to the more efficient of `const T` or `const T&`, in the context of returning a const-qualified value + // of type T. + // + // Copied from cppfront's implementation of the CppCoreGuidelines F.16 (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in) + template + using value_or_reference_return_t = std::conditional_t< + sizeof(T) < 2*sizeof(void*) && std::is_trivially_copy_constructible::value, + const T, + const T&>; + + } // namespace details + +// +// GSL.owner: ownership pointers +// + using std::shared_ptr; + using std::unique_ptr; + +// +// owner +// +// `gsl::owner` is designed as a safety mechanism for code that must deal directly with raw pointers that own memory. +// Ideally such code should be restricted to the implementation of low-level abstractions. `gsl::owner` can also be used +// as a stepping point in converting legacy code to use more modern RAII constructs, such as smart pointers. +// +// T must be a pointer type +// - disallow construction from any type other than pointer type +// + template ::value>> + using owner = T; + +// +// not_null +// +// Restricts a pointer or smart pointer to only hold non-null values. +// +// Has zero size overhead over T. +// +// If T is a pointer (i.e. T == U*) then +// - allow construction from U* +// - disallow construction from nullptr_t +// - disallow default construction +// - ensure construction from null U* fails +// - allow implicit conversion to U* +// + template + class not_null + { + public: + static_assert(details::is_comparable_to_nullptr::value, "T cannot be compared to nullptr."); + + template ::value>> + constexpr not_null(U&& u, nostd::source_location loc = nostd::source_location::current()) : ptr_(std::forward(u)) + { + Expects(ptr_ != nullptr, loc); + } + + template ::value>> + constexpr not_null(T u, nostd::source_location loc = nostd::source_location::current()) : ptr_(std::move(u)) + { + Expects(ptr_ != nullptr, loc); + } + + template ::value>> + constexpr not_null(const not_null& other) : not_null(other.get()) + {} + + not_null(const not_null& other) = default; + not_null& operator=(const not_null& other) = default; + constexpr details::value_or_reference_return_t get() const + noexcept(noexcept(details::value_or_reference_return_t{std::declval()})) + { + return ptr_; + } + + constexpr operator T() const { return get(); } + constexpr decltype(auto) operator->() const { return get(); } + constexpr decltype(auto) operator*() const { return *get(); } + + // prevents compilation when someone attempts to assign a null pointer constant + not_null(std::nullptr_t) = delete; + not_null& operator=(std::nullptr_t) = delete; + + // unwanted operators...pointers only point to single objects! + not_null& operator++() = delete; + not_null& operator--() = delete; + not_null operator++(int) = delete; + not_null operator--(int) = delete; + not_null& operator+=(std::ptrdiff_t) = delete; + not_null& operator-=(std::ptrdiff_t) = delete; + void operator[](std::ptrdiff_t) const = delete; + + private: + T ptr_; + }; + + template + auto make_not_null(T&& t) noexcept + { + return not_null>>{std::forward(t)}; + } + +#if !defined(GSL_NO_IOSTREAMS) + template + std::ostream& operator<<(std::ostream& os, const not_null& val) + { + os << val.get(); + return os; + } +#endif // !defined(GSL_NO_IOSTREAMS) + + template + auto operator==(const not_null& lhs, + const not_null& rhs) noexcept(noexcept(lhs.get() == rhs.get())) + -> decltype(lhs.get() == rhs.get()) + { + return lhs.get() == rhs.get(); + } + + template + auto operator!=(const not_null& lhs, + const not_null& rhs) noexcept(noexcept(lhs.get() != rhs.get())) + -> decltype(lhs.get() != rhs.get()) + { + return lhs.get() != rhs.get(); + } + + template + auto operator<(const not_null& lhs, + const not_null& rhs) noexcept(noexcept(std::less<>{}(lhs.get(), rhs.get()))) + -> decltype(std::less<>{}(lhs.get(), rhs.get())) + { + return std::less<>{}(lhs.get(), rhs.get()); + } + + template + auto operator<=(const not_null& lhs, + const not_null& rhs) noexcept(noexcept(std::less_equal<>{}(lhs.get(), rhs.get()))) + -> decltype(std::less_equal<>{}(lhs.get(), rhs.get())) + { + return std::less_equal<>{}(lhs.get(), rhs.get()); + } + + template + auto operator>(const not_null& lhs, + const not_null& rhs) noexcept(noexcept(std::greater<>{}(lhs.get(), rhs.get()))) + -> decltype(std::greater<>{}(lhs.get(), rhs.get())) + { + return std::greater<>{}(lhs.get(), rhs.get()); + } + + template + auto operator>=(const not_null& lhs, + const not_null& rhs) noexcept(noexcept(std::greater_equal<>{}(lhs.get(), rhs.get()))) + -> decltype(std::greater_equal<>{}(lhs.get(), rhs.get())) + { + return std::greater_equal<>{}(lhs.get(), rhs.get()); + } + +// more unwanted operators + template + std::ptrdiff_t operator-(const not_null&, const not_null&) = delete; + template + not_null operator-(const not_null&, std::ptrdiff_t) = delete; + template + not_null operator+(const not_null&, std::ptrdiff_t) = delete; + template + not_null operator+(std::ptrdiff_t, const not_null&) = delete; + + + template ().get()), bool = std::is_default_constructible>::value> + struct not_null_hash + { + std::size_t operator()(const T& value) const { return std::hash{}(value.get()); } + }; + + template + struct not_null_hash + { + not_null_hash() = delete; + not_null_hash(const not_null_hash&) = delete; + not_null_hash& operator=(const not_null_hash&) = delete; + }; + +} // namespace gsl + +namespace std +{ + template + struct hash> : gsl::not_null_hash> +{ +}; + +} // namespace std + +namespace gsl +{ + +// +// strict_not_null +// +// Restricts a pointer or smart pointer to only hold non-null values, +// +// - provides a strict (i.e. explicit constructor from T) wrapper of not_null +// - to be used for new code that wishes the design to be cleaner and make not_null +// checks intentional, or in old code that would like to make the transition. +// +// To make the transition from not_null, incrementally replace not_null +// by strict_not_null and fix compilation errors +// +// Expect to +// - remove all unneeded conversions from raw pointer to not_null and back +// - make API clear by specifying not_null in parameters where needed +// - remove unnecessary asserts +// + template + class strict_not_null : public not_null + { + public: + template ::value>> + constexpr explicit strict_not_null(U&& u) : not_null(std::forward(u)) + {} + + template ::value>> + constexpr explicit strict_not_null(T u) : not_null(u) + {} + + template ::value>> + constexpr strict_not_null(const not_null& other) : not_null(other) + {} + + template ::value>> + constexpr strict_not_null(const strict_not_null& other) : not_null(other) + {} + + // To avoid invalidating the "not null" invariant, the contained pointer is actually copied + // instead of moved. If it is a custom pointer, its constructor could in theory throw exceptions. + strict_not_null(strict_not_null&& other) noexcept(std::is_nothrow_copy_constructible::value) = default; + strict_not_null(const strict_not_null& other) = default; + strict_not_null& operator=(const strict_not_null& other) = default; + strict_not_null& operator=(const not_null& other) + { + not_null::operator=(other); + return *this; + } + + // prevents compilation when someone attempts to assign a null pointer constant + strict_not_null(std::nullptr_t) = delete; + strict_not_null& operator=(std::nullptr_t) = delete; + + // unwanted operators...pointers only point to single objects! + strict_not_null& operator++() = delete; + strict_not_null& operator--() = delete; + strict_not_null operator++(int) = delete; + strict_not_null operator--(int) = delete; + strict_not_null& operator+=(std::ptrdiff_t) = delete; + strict_not_null& operator-=(std::ptrdiff_t) = delete; + void operator[](std::ptrdiff_t) const = delete; + }; + +// more unwanted operators + template + std::ptrdiff_t operator-(const strict_not_null&, const strict_not_null&) = delete; + template + strict_not_null operator-(const strict_not_null&, std::ptrdiff_t) = delete; + template + strict_not_null operator+(const strict_not_null&, std::ptrdiff_t) = delete; + template + strict_not_null operator+(std::ptrdiff_t, const strict_not_null&) = delete; + + template + auto make_strict_not_null(T&& t) noexcept + { + return strict_not_null>>{std::forward(t)}; + } + +#if (defined(__cpp_deduction_guides) && (__cpp_deduction_guides >= 201611L)) + +// deduction guides to prevent the ctad-maybe-unsupported warning + template + not_null(T) -> not_null; + template + strict_not_null(T) -> strict_not_null; + +#endif // ( defined(__cpp_deduction_guides) && (__cpp_deduction_guides >= 201611L) ) +} // namespace gsl + +namespace std +{ + template + struct hash> : gsl::not_null_hash> +{ +}; + +} // namespace std + +#endif // GSL_POINTERS_H diff --git a/src/llmq/blockprocessor.cpp b/src/llmq/blockprocessor.cpp index 509c8445d6..8057402ca0 100644 --- a/src/llmq/blockprocessor.cpp +++ b/src/llmq/blockprocessor.cpp @@ -146,7 +146,7 @@ void CQuorumBlockProcessor::ProcessMessage(const CNode& peer, std::string_view m AddMineableCommitment(qc); } -bool CQuorumBlockProcessor::ProcessBlock(const CBlock& block, const CBlockIndex* pindex, BlockValidationState& state, bool fJustCheck, bool fBLSChecks) +bool CQuorumBlockProcessor::ProcessBlock(const CBlock& block, gsl::not_null pindex, BlockValidationState& state, bool fJustCheck, bool fBLSChecks) { AssertLockHeld(cs_main); @@ -310,7 +310,7 @@ bool CQuorumBlockProcessor::ProcessCommitment(int nHeight, const uint256& blockH return true; } -bool CQuorumBlockProcessor::UndoBlock(const CBlock& block, const CBlockIndex* pindex) +bool CQuorumBlockProcessor::UndoBlock(const CBlock& block, gsl::not_null pindex) { AssertLockHeld(cs_main); @@ -411,7 +411,7 @@ bool CQuorumBlockProcessor::UpgradeDB() return true; } -bool CQuorumBlockProcessor::GetCommitmentsFromBlock(const CBlock& block, const CBlockIndex* pindex, std::multimap& ret, BlockValidationState& state) +bool CQuorumBlockProcessor::GetCommitmentsFromBlock(const CBlock& block, gsl::not_null pindex, std::multimap& ret, BlockValidationState& state) { AssertLockHeld(cs_main); @@ -541,7 +541,7 @@ CFinalCommitmentPtr CQuorumBlockProcessor::GetMinedCommitment(Consensus::LLMQTyp } // The returned quorums are in reversed order, so the most recent one is at index 0 -std::vector CQuorumBlockProcessor::GetMinedCommitmentsUntilBlock(Consensus::LLMQType llmqType, const CBlockIndex* pindex, size_t maxCount) const +std::vector CQuorumBlockProcessor::GetMinedCommitmentsUntilBlock(Consensus::LLMQType llmqType, gsl::not_null pindex, size_t maxCount) const { AssertLockNotHeld(m_evoDb.cs); LOCK(m_evoDb.cs); @@ -680,7 +680,7 @@ std::vector CQuorumBlockProcessor::GetMinedCommitmentsIndexe } // The returned quorums are in reversed order, so the most recent one is at index 0 -std::map> CQuorumBlockProcessor::GetMinedAndActiveCommitmentsUntilBlock(const CBlockIndex* pindex) const +std::map> CQuorumBlockProcessor::GetMinedAndActiveCommitmentsUntilBlock(gsl::not_null pindex) const { std::map> ret; diff --git a/src/llmq/blockprocessor.h b/src/llmq/blockprocessor.h index 95d88641fd..bc9bd4fd54 100644 --- a/src/llmq/blockprocessor.h +++ b/src/llmq/blockprocessor.h @@ -14,6 +14,8 @@ #include #include +#include + #include class BlockValidationState; @@ -53,8 +55,8 @@ public: void ProcessMessage(const CNode& peer, std::string_view msg_type, CDataStream& vRecv); - bool ProcessBlock(const CBlock& block, const CBlockIndex* pindex, BlockValidationState& state, bool fJustCheck, bool fBLSChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - bool UndoBlock(const CBlock& block, const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool ProcessBlock(const CBlock& block, gsl::not_null pindex, BlockValidationState& state, bool fJustCheck, bool fBLSChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool UndoBlock(const CBlock& block, gsl::not_null pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); void AddMineableCommitment(const CFinalCommitment& fqc); bool HasMineableCommitment(const uint256& hash) const; @@ -64,14 +66,14 @@ public: bool HasMinedCommitment(Consensus::LLMQType llmqType, const uint256& quorumHash) const; CFinalCommitmentPtr GetMinedCommitment(Consensus::LLMQType llmqType, const uint256& quorumHash, uint256& retMinedBlockHash) const; - std::vector GetMinedCommitmentsUntilBlock(Consensus::LLMQType llmqType, const CBlockIndex* pindex, size_t maxCount) const; - std::map> GetMinedAndActiveCommitmentsUntilBlock(const CBlockIndex* pindex) const; + std::vector GetMinedCommitmentsUntilBlock(Consensus::LLMQType llmqType, gsl::not_null pindex, size_t maxCount) const; + std::map> GetMinedAndActiveCommitmentsUntilBlock(gsl::not_null pindex) const; std::vector GetMinedCommitmentsIndexedUntilBlock(Consensus::LLMQType llmqType, const CBlockIndex* pindex, size_t maxCount) const; std::vector> GetLastMinedCommitmentsPerQuorumIndexUntilBlock(Consensus::LLMQType llmqType, const CBlockIndex* pindex, size_t cycle) const; std::optional GetLastMinedCommitmentsByQuorumIndexUntilBlock(Consensus::LLMQType llmqType, const CBlockIndex* pindex, int quorumIndex, size_t cycle) const; private: - static bool GetCommitmentsFromBlock(const CBlock& block, const CBlockIndex* pindex, std::multimap& ret, BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + static bool GetCommitmentsFromBlock(const CBlock& block, gsl::not_null pindex, std::multimap& ret, BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool ProcessCommitment(int nHeight, const uint256& blockHash, const CFinalCommitment& qc, BlockValidationState& state, bool fJustCheck, bool fBLSChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main); static bool IsMiningPhase(const Consensus::LLMQParams& llmqParams, int nHeight) EXCLUSIVE_LOCKS_REQUIRED(cs_main); size_t GetNumCommitmentsRequired(const Consensus::LLMQParams& llmqParams, int nHeight) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); diff --git a/src/llmq/chainlocks.cpp b/src/llmq/chainlocks.cpp index 977824a0e4..7258660613 100644 --- a/src/llmq/chainlocks.cpp +++ b/src/llmq/chainlocks.cpp @@ -178,7 +178,7 @@ void CChainLocksHandler::ProcessNewChainLock(const NodeId from, const llmq::CCha __func__, clsig.ToString(), from); } -void CChainLocksHandler::AcceptedBlockHeader(const CBlockIndex* pindexNew) +void CChainLocksHandler::AcceptedBlockHeader(gsl::not_null pindexNew) { LOCK(cs); @@ -361,7 +361,7 @@ void CChainLocksHandler::TransactionAddedToMempool(const CTransactionRef& tx, in txFirstSeenTime.emplace(tx->GetHash(), nAcceptTime); } -void CChainLocksHandler::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) +void CChainLocksHandler::BlockConnected(const std::shared_ptr& pblock, gsl::not_null pindex) { if (!m_mn_sync.IsBlockchainSynced()) { return; @@ -394,7 +394,7 @@ void CChainLocksHandler::BlockConnected(const std::shared_ptr& pbl } -void CChainLocksHandler::BlockDisconnected(const std::shared_ptr& pblock, const CBlockIndex* pindexDisconnected) +void CChainLocksHandler::BlockDisconnected(const std::shared_ptr& pblock, gsl::not_null pindexDisconnected) { LOCK(cs); blockTxs.erase(pindexDisconnected->GetBlockHash()); diff --git a/src/llmq/chainlocks.h b/src/llmq/chainlocks.h index 97454aa769..64f7501ec3 100644 --- a/src/llmq/chainlocks.h +++ b/src/llmq/chainlocks.h @@ -16,6 +16,8 @@ #include #include +#include + #include #include #include @@ -100,11 +102,11 @@ public: void ProcessMessage(const CNode& pfrom, const std::string& msg_type, CDataStream& vRecv); void ProcessNewChainLock(NodeId from, const CChainLockSig& clsig, const uint256& hash) LOCKS_EXCLUDED(cs); - void AcceptedBlockHeader(const CBlockIndex* pindexNew) LOCKS_EXCLUDED(cs); + void AcceptedBlockHeader(gsl::not_null pindexNew) LOCKS_EXCLUDED(cs); void UpdatedBlockTip(); void TransactionAddedToMempool(const CTransactionRef& tx, int64_t nAcceptTime) LOCKS_EXCLUDED(cs); - void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) LOCKS_EXCLUDED(cs); - void BlockDisconnected(const std::shared_ptr& pblock, const CBlockIndex* pindexDisconnected) LOCKS_EXCLUDED(cs); + void BlockConnected(const std::shared_ptr& pblock, gsl::not_null pindex) LOCKS_EXCLUDED(cs); + void BlockDisconnected(const std::shared_ptr& pblock, gsl::not_null pindexDisconnected) LOCKS_EXCLUDED(cs); void CheckActiveState() LOCKS_EXCLUDED(cs); void TrySignChainTip() LOCKS_EXCLUDED(cs); void EnforceBestChainLock() LOCKS_EXCLUDED(cs); diff --git a/src/llmq/commitment.cpp b/src/llmq/commitment.cpp index 78ff897e10..967b4a4927 100644 --- a/src/llmq/commitment.cpp +++ b/src/llmq/commitment.cpp @@ -32,7 +32,7 @@ void LogPrintfFinalCommitment(Types... out) { } } -bool CFinalCommitment::Verify(const CBlockIndex* pQuorumBaseBlockIndex, bool checkSigs) const +bool CFinalCommitment::Verify(gsl::not_null pQuorumBaseBlockIndex, bool checkSigs) const { const auto& llmq_params_opt = GetLLMQParams(llmqType); if (!llmq_params_opt.has_value()) { @@ -176,7 +176,7 @@ bool CFinalCommitment::VerifySizes(const Consensus::LLMQParams& params) const return true; } -bool CheckLLMQCommitment(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state) +bool CheckLLMQCommitment(const CTransaction& tx, gsl::not_null pindexPrev, TxValidationState& state) { CFinalCommitmentTxPayload qcTx; if (!GetTxPayload(tx, qcTx)) { diff --git a/src/llmq/commitment.h b/src/llmq/commitment.h index 39a2a38318..15a066ed53 100644 --- a/src/llmq/commitment.h +++ b/src/llmq/commitment.h @@ -12,6 +12,8 @@ #include #include +#include + #include class CBlockIndex; @@ -59,7 +61,7 @@ public: return int(std::count(validMembers.begin(), validMembers.end(), true)); } - bool Verify(const CBlockIndex* pQuorumBaseBlockIndex, bool checkSigs) const; + bool Verify(gsl::not_null pQuorumBaseBlockIndex, bool checkSigs) const; bool VerifyNull() const; bool VerifySizes(const Consensus::LLMQParams& params) const; @@ -169,7 +171,7 @@ public: } }; -bool CheckLLMQCommitment(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state); +bool CheckLLMQCommitment(const CTransaction& tx, gsl::not_null pindexPrev, TxValidationState& state); } // namespace llmq diff --git a/src/llmq/dkgsession.cpp b/src/llmq/dkgsession.cpp index 88b11ef3a4..052df49d7a 100644 --- a/src/llmq/dkgsession.cpp +++ b/src/llmq/dkgsession.cpp @@ -57,7 +57,7 @@ CDKGMember::CDKGMember(const CDeterministicMNCPtr& _dmn, size_t _idx) : } -bool CDKGSession::Init(const CBlockIndex* _pQuorumBaseBlockIndex, Span mns, const uint256& _myProTxHash, int _quorumIndex) +bool CDKGSession::Init(gsl::not_null _pQuorumBaseBlockIndex, Span mns, const uint256& _myProTxHash, int _quorumIndex) { m_quorum_base_block_index = _pQuorumBaseBlockIndex; quorumIndex = _quorumIndex; diff --git a/src/llmq/dkgsession.h b/src/llmq/dkgsession.h index 6989b8e2ae..7312f17464 100644 --- a/src/llmq/dkgsession.h +++ b/src/llmq/dkgsession.h @@ -307,7 +307,7 @@ public: CDKGSession(const Consensus::LLMQParams& _params, CBLSWorker& _blsWorker, CDKGSessionManager& _dkgManager, CDKGDebugManager& _dkgDebugManager, CConnman& _connman) : params(_params), blsWorker(_blsWorker), cache(_blsWorker), dkgManager(_dkgManager), dkgDebugManager(_dkgDebugManager), connman(_connman) {} - bool Init(const CBlockIndex* pQuorumBaseBlockIndex, Span mns, const uint256& _myProTxHash, int _quorumIndex); + bool Init(gsl::not_null pQuorumBaseBlockIndex, Span mns, const uint256& _myProTxHash, int _quorumIndex); [[nodiscard]] std::optional GetMyMemberIndex() const { return myIdx; } diff --git a/src/llmq/instantsend.cpp b/src/llmq/instantsend.cpp index 94fb2c7851..9f00f54c13 100644 --- a/src/llmq/instantsend.cpp +++ b/src/llmq/instantsend.cpp @@ -250,7 +250,8 @@ void CInstantSendDb::RemoveArchivedInstantSendLocks(int nUntilHeight) db->WriteBatch(batch); } -void CInstantSendDb::WriteBlockInstantSendLocks(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) +void CInstantSendDb::WriteBlockInstantSendLocks(const gsl::not_null>& pblock, + gsl::not_null pindexConnected) { LOCK(cs_db); CDBBatch batch(*db); @@ -268,7 +269,7 @@ void CInstantSendDb::WriteBlockInstantSendLocks(const std::shared_ptrWriteBatch(batch); } -void CInstantSendDb::RemoveBlockInstantSendLocks(const std::shared_ptr& pblock, const CBlockIndex* pindexDisconnected) +void CInstantSendDb::RemoveBlockInstantSendLocks(const gsl::not_null>& pblock, gsl::not_null pindexDisconnected) { LOCK(cs_db); CDBBatch batch(*db); @@ -440,7 +441,7 @@ std::vector CInstantSendDb::RemoveChainedInstantSendLocks(const uint256 return result; } -void CInstantSendDb::RemoveAndArchiveInstantSendLock(const CInstantSendLockPtr& islock, int nHeight) +void CInstantSendDb::RemoveAndArchiveInstantSendLock(const gsl::not_null& islock, int nHeight) { LOCK(cs_db); diff --git a/src/llmq/instantsend.h b/src/llmq/instantsend.h index cf4c88d0a3..777af30f24 100644 --- a/src/llmq/instantsend.h +++ b/src/llmq/instantsend.h @@ -15,6 +15,8 @@ #include #include +#include + #include #include #include @@ -148,8 +150,8 @@ public: * @param nUntilHeight the height from which to base the remove of archive IS Locks */ void RemoveArchivedInstantSendLocks(int nUntilHeight) LOCKS_EXCLUDED(cs_db); - void WriteBlockInstantSendLocks(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) LOCKS_EXCLUDED(cs_db); - void RemoveBlockInstantSendLocks(const std::shared_ptr& pblock, const CBlockIndex* pindexDisconnected) LOCKS_EXCLUDED(cs_db); + void WriteBlockInstantSendLocks(const gsl::not_null>& pblock, gsl::not_null pindexConnected) LOCKS_EXCLUDED(cs_db); + void RemoveBlockInstantSendLocks(const gsl::not_null>& pblock, gsl::not_null pindexDisconnected) LOCKS_EXCLUDED(cs_db); bool KnownInstantSendLock(const uint256& islockHash) const LOCKS_EXCLUDED(cs_db); /** * Gets the number of IS Locks which have not been confirmed by a block @@ -198,7 +200,7 @@ public: */ std::vector RemoveChainedInstantSendLocks(const uint256& islockHash, const uint256& txid, int nHeight) LOCKS_EXCLUDED(cs_db); - void RemoveAndArchiveInstantSendLock(const CInstantSendLockPtr& islock, int nHeight) LOCKS_EXCLUDED(cs_db); + void RemoveAndArchiveInstantSendLock(const gsl::not_null& islock, int nHeight) LOCKS_EXCLUDED(cs_db); }; class CInstantSendManager : public CRecoveredSigsListener diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index 23553aca95..3dd23fdbb8 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -363,10 +363,8 @@ void CQuorumManager::CheckQuorumConnections(const Consensus::LLMQParams& llmqPar } } -CQuorumPtr CQuorumManager::BuildQuorumFromCommitment(const Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex) const +CQuorumPtr CQuorumManager::BuildQuorumFromCommitment(const Consensus::LLMQType llmqType, gsl::not_null pQuorumBaseBlockIndex) const { - assert(pQuorumBaseBlockIndex); - const uint256& quorumHash{pQuorumBaseBlockIndex->GetBlockHash()}; uint256 minedBlockHash; CFinalCommitmentPtr qc = quorumBlockProcessor.GetMinedCommitment(llmqType, quorumHash, minedBlockHash); @@ -572,10 +570,8 @@ CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const uint25 return GetQuorum(llmqType, pQuorumBaseBlockIndex); } -CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex) const +CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, gsl::not_null pQuorumBaseBlockIndex) const { - assert(pQuorumBaseBlockIndex); - auto quorumHash = pQuorumBaseBlockIndex->GetBlockHash(); // we must check this before we look into the cache. Reorgs might have happened which would mean we might have diff --git a/src/llmq/quorums.h b/src/llmq/quorums.h index 6be46ef765..bc2eba6ae4 100644 --- a/src/llmq/quorums.h +++ b/src/llmq/quorums.h @@ -16,6 +16,8 @@ #include +#include + #include #include @@ -263,10 +265,10 @@ private: // all private methods here are cs_main-free void CheckQuorumConnections(const Consensus::LLMQParams& llmqParams, const CBlockIndex *pindexNew) const; - CQuorumPtr BuildQuorumFromCommitment(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex) const; + CQuorumPtr BuildQuorumFromCommitment(Consensus::LLMQType llmqType, gsl::not_null pQuorumBaseBlockIndex) const; bool BuildQuorumContributions(const CFinalCommitmentPtr& fqc, const std::shared_ptr& quorum) const; - CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, const CBlockIndex* pindex) const; + CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, gsl::not_null pindex) const; /// Returns the start offset for the masternode with the given proTxHash. This offset is applied when picking data recovery members of a quorum's /// memberlist and is calculated based on a list of all member of all active quorums for the given llmqType in a way that each member /// should receive the same number of request if all active llmqType members requests data from one llmqType quorum. diff --git a/src/source_location.h b/src/source_location.h new file mode 100644 index 0000000000..efbc99b829 --- /dev/null +++ b/src/source_location.h @@ -0,0 +1,76 @@ +// Copyright (c) 2023 The Dash Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. +// Originally from https://github.com/paweldac/source_location + +#ifndef BITCOIN_SOURCE_LOCATION_H +#define BITCOIN_SOURCE_LOCATION_H + +#pragma once + +#include + +namespace nostd { + struct source_location { + public: +#if defined(__clang__) and (__clang_major__ >= 9) + static constexpr source_location current(const char* fileName = __builtin_FILE(), + const char* functionName = __builtin_FUNCTION(), + const uint_least32_t lineNumber = __builtin_LINE(), + const uint_least32_t columnOffset = __builtin_COLUMN()) noexcept +#elif defined(__GNUC__) and (__GNUC__ > 4 or (__GNUC__ == 4 and __GNUC_MINOR__ >= 8)) + static constexpr source_location current(const char* fileName = __builtin_FILE(), + const char* functionName = __builtin_FUNCTION(), + const uint_least32_t lineNumber = __builtin_LINE(), + const uint_least32_t columnOffset = 0) noexcept +#else + static constexpr source_location current(const char* fileName = "unsupported", + const char* functionName = "unsupported", + const uint_least32_t lineNumber = 0, + const uint_least32_t columnOffset = 0) noexcept +#endif + { + return source_location(fileName, functionName, lineNumber, columnOffset); + } + + source_location(const source_location&) = default; + source_location(source_location&&) = default; + + constexpr const char* file_name() const noexcept + { + return fileName; + } + + constexpr const char* function_name() const noexcept + { + return functionName; + } + + constexpr uint_least32_t line() const noexcept + { + return lineNumber; + } + + constexpr std::uint_least32_t column() const noexcept + { + return columnOffset; + } + + private: + constexpr source_location(const char* fileName, const char* functionName, const uint_least32_t lineNumber, + const uint_least32_t columnOffset) noexcept + : fileName(fileName) + , functionName(functionName) + , lineNumber(lineNumber) + , columnOffset(columnOffset) + { + } + + const char* fileName; + const char* functionName; + const std::uint_least32_t lineNumber; + const std::uint_least32_t columnOffset; + }; +} // namespace nostd + +#endif // BITCOIN_SOURCE_LOCATION_H diff --git a/test/lint/lint-include-guards.sh b/test/lint/lint-include-guards.sh index 0015d0e24c..4d4c14f29a 100755 --- a/test/lint/lint-include-guards.sh +++ b/test/lint/lint-include-guards.sh @@ -10,7 +10,7 @@ export LC_ALL=C HEADER_ID_PREFIX="BITCOIN_" HEADER_ID_SUFFIX="_H" -REGEXP_EXCLUDE_FILES_WITH_PREFIX="src/(crypto/ctaes/|dashbls/|immer/|leveldb/|crc32c/|secp256k1/|test/fuzz/FuzzedDataProvider.h|tinyformat.h|bench/nanobench.h|univalue/|ctpl_stl.h|bls/|crypto/sph)" +REGEXP_EXCLUDE_FILES_WITH_PREFIX="src/(crypto/ctaes/|dashbls/|immer/|leveldb/|crc32c/|secp256k1/|test/fuzz/FuzzedDataProvider.h|tinyformat.h|bench/nanobench.h|univalue/|ctpl_stl.h|bls/|crypto/sph|gsl)" EXIT_CODE=0 for HEADER_FILE in $(git ls-files -- "*.h" | grep -vE "^${REGEXP_EXCLUDE_FILES_WITH_PREFIX}")