Skip to content

Commit

Permalink
refactor: add gsl::not_null to get compile time / run time pointer gu…
Browse files Browse the repository at this point in the history
…arantees (dashpay#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 <pasta@dashboost.org>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
  • Loading branch information
PastaPastaPasta and UdjinM6 committed Oct 22, 2023
1 parent 9cfc3a6 commit c51cec6
Show file tree
Hide file tree
Showing 19 changed files with 622 additions and 43 deletions.
3 changes: 3 additions & 0 deletions src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down Expand Up @@ -287,6 +289,7 @@ BITCOIN_CORE_H = \
script/signingprovider.h \
script/standard.h \
shutdown.h \
source_location.h \
spork.h \
stacktraces.h \
streams.h \
Expand Down
6 changes: 3 additions & 3 deletions src/evo/assetlocktx.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
/**
* Common code for Asset Lock and Asset Unlock
*/
bool CheckAssetLockUnlockTx(const CTransaction& tx, const CBlockIndex* pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state)
bool CheckAssetLockUnlockTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state)
{
switch (tx.nType) {
case TRANSACTION_ASSET_LOCK:
Expand Down Expand Up @@ -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<const CBlockIndex*> 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
Expand Down Expand Up @@ -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<CRangesSet>& indexes, TxValidationState& state)
bool CheckAssetUnlockTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& 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
Expand Down
7 changes: 4 additions & 3 deletions src/evo/assetlocktx.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <bls/bls_ies.h>
#include <evo/specialtx.h>
#include <primitives/transaction.h>
#include <gsl/pointers.h>

#include <key_io.h>
#include <serialize.h>
Expand Down Expand Up @@ -128,7 +129,7 @@ class CAssetUnlockPayload
return obj;
}

bool VerifySig(const uint256& msgHash, const CBlockIndex* pindexTip, TxValidationState& state) const;
bool VerifySig(const uint256& msgHash, gsl::not_null<const CBlockIndex*> pindexTip, TxValidationState& state) const;

// getters
uint8_t getVersion() const
Expand Down Expand Up @@ -170,8 +171,8 @@ class CAssetUnlockPayload
};

bool CheckAssetLockTx(const CTransaction& tx, TxValidationState& state);
bool CheckAssetUnlockTx(const CTransaction& tx, const CBlockIndex* pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state);
bool CheckAssetLockUnlockTx(const CTransaction& tx, const CBlockIndex* pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state);
bool CheckAssetUnlockTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state);
bool CheckAssetLockUnlockTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state);
bool GetAssetUnlockFee(const CTransaction& tx, CAmount& txfee, TxValidationState& state);

#endif // BITCOIN_EVO_ASSETLOCKTX_H
144 changes: 144 additions & 0 deletions src/gsl/assert.h
Original file line number Diff line number Diff line change
@@ -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 <source_location.h>
#include <logging.h>

#include <sstream>

//
// 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 <intrin.h>
#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 <exception>

#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<void>(0) : __builtin_unreachable())
#else
#define GSL_ASSUME(cond) static_cast<void>((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<void>(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
Loading

0 comments on commit c51cec6

Please sign in to comment.