diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index b1fbc8e0f253..d035d216360c 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -2,6 +2,8 @@ _Provide a general summary of your changes in the Title above_ +_Do not manually line wrap text; allow the GitHub UI to dynamically wrap lines._ + _Pull requests without a rationale and clear improvement may be closed immediately._ diff --git a/AGENTS.md b/AGENTS.md index 6e5a58e76777..a47070702457 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,37 @@ changes, update the other in the same commit. code; reserve them for things that genuinely need explaining (non-obvious invariants, workaround rationale, non-local side effects). +## Assertions and Checks + +Full guidance lives in `doc/developer-notes.md` under "Assertions and Checks". +Short version, in order of preference: + +- `Assume(cond)` is the default. Use it for "this is how things are supposed to + be": a violation means someone has a bug worth investigating, but execution + stays well-defined. A negative rate-limit counter is the archetype - somebody + decremented twice, we may be more DoS-exposed than intended, but nothing is + corrupt. It aborts in `--enable-debug` and `--enable-fuzz` builds (CI's + `linux64_multiprocess` and fuzz jobs) while a failure is silent in release, + so it must never take down a production node. The expression is always + evaluated. +- `assert(cond)` / `Assert(cond)` is the "we must crash now" case. Use it only + when continuing would be undefined behavior, memory corruption, or corrupt + persisted/consensus state - aborting has to be the safer outcome. It should + be rare and obviously justified, but do use it where it is genuinely needed + to document and enforce a precondition that keeps the code below it safe. + `Assert` returns its argument: `assert(ptr != nullptr); obj = *ptr;` becomes + `obj = *Assert(ptr);` +- `CHECK_NONFATAL(cond)` / `NONFATAL_UNREACHABLE()` for internal logic bugs on + a path with a caller to report to. Required in RPC code, enforced + (best-effort) by `test/lint/lint-assertions.py` for `src/rpc/` and + `src/wallet/rpc*`. + +None of these validate input. Data from peers, RPC arguments, wallet files, or +on-disk state must be checked and rejected through normal error handling - +asserting on it turns a peer-triggered inconsistency into a remote crash. +Environment failures (disk full, corrupt block on disk, failed DB write) are +not checks at all: return an error, `AbortNode()`, or `InitError()`. + ## Repository Map - `src/` - C++ implementation. diff --git a/CLAUDE.md b/CLAUDE.md index 6e5a58e76777..a47070702457 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,6 +24,37 @@ changes, update the other in the same commit. code; reserve them for things that genuinely need explaining (non-obvious invariants, workaround rationale, non-local side effects). +## Assertions and Checks + +Full guidance lives in `doc/developer-notes.md` under "Assertions and Checks". +Short version, in order of preference: + +- `Assume(cond)` is the default. Use it for "this is how things are supposed to + be": a violation means someone has a bug worth investigating, but execution + stays well-defined. A negative rate-limit counter is the archetype - somebody + decremented twice, we may be more DoS-exposed than intended, but nothing is + corrupt. It aborts in `--enable-debug` and `--enable-fuzz` builds (CI's + `linux64_multiprocess` and fuzz jobs) while a failure is silent in release, + so it must never take down a production node. The expression is always + evaluated. +- `assert(cond)` / `Assert(cond)` is the "we must crash now" case. Use it only + when continuing would be undefined behavior, memory corruption, or corrupt + persisted/consensus state - aborting has to be the safer outcome. It should + be rare and obviously justified, but do use it where it is genuinely needed + to document and enforce a precondition that keeps the code below it safe. + `Assert` returns its argument: `assert(ptr != nullptr); obj = *ptr;` becomes + `obj = *Assert(ptr);` +- `CHECK_NONFATAL(cond)` / `NONFATAL_UNREACHABLE()` for internal logic bugs on + a path with a caller to report to. Required in RPC code, enforced + (best-effort) by `test/lint/lint-assertions.py` for `src/rpc/` and + `src/wallet/rpc*`. + +None of these validate input. Data from peers, RPC arguments, wallet files, or +on-disk state must be checked and rejected through normal error handling - +asserting on it turns a peer-triggered inconsistency into a remote crash. +Environment failures (disk full, corrupt block on disk, failed DB write) are +not checks at all: return an error, `AbortNode()`, or `InitError()`. + ## Repository Map - `src/` - C++ implementation. diff --git a/ci/dash/lint-cstyle-casts.py b/ci/dash/lint-cstyle-casts.py new file mode 100755 index 000000000000..eba2d0617d1f --- /dev/null +++ b/ci/dash/lint-cstyle-casts.py @@ -0,0 +1,112 @@ +#!/usr/bin/env python3 +# Copyright (c) 2026 The Dash Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +"""Support C-style cast linting in Dash-specific C++ code.""" + +import argparse +import json +import re +import shlex +import subprocess +import sys +from pathlib import Path +from typing import TextIO + + +CPP_SOURCE_EXTENSIONS = {".cc", ".cpp", ".cxx"} +DIAGNOSTIC_RE = re.compile(r"^(.*?):\d+:\d+: (?:warning|error): .*") +MACRO_EXPANSION_RE = re.compile(r"^(.*?):\d+:\d+: note: expanded from macro .*") +OLD_STYLE_CAST_DIAGNOSTICS = {"clang-diagnostic-old-style-cast", "google-readability-casting"} + + +def get_dash_files(source_root: Path) -> list[str]: + manifest = source_root / "test/util/data/non-backported.txt" + patterns = [line.strip() for line in manifest.read_text(encoding="utf8").splitlines() if line.strip()] + result = subprocess.run( + ["git", "ls-files", "--", *patterns], + cwd=source_root, + check=True, + stdout=subprocess.PIPE, + text=True, + encoding="utf8", + ) + return [line for line in result.stdout.splitlines() if line] + + +def is_dash_file(path: str, dash_files: set[str]) -> bool: + normalized = path.replace("\\", "/") + return any(normalized == dash_file or normalized.endswith(f"/{dash_file}") for dash_file in dash_files) + + +def prepare_compile_database(source_root: Path, input_path: Path, output_dir: Path) -> None: + database = json.loads(input_path.read_text(encoding="utf8")) + + for entry in database: + if "arguments" not in entry: + entry["arguments"] = shlex.split(entry.pop("command")) + entry["arguments"].extend(["-Wold-style-cast", "-Wno-error=old-style-cast"]) + + output_dir.mkdir(parents=True, exist_ok=True) + (output_dir / "compile_commands.json").write_text(json.dumps(database), encoding="utf8") + + +def filter_diagnostics(source_root: Path, input_stream: TextIO, output_stream: TextIO) -> bool: + dash_files = set(get_dash_files(source_root)) + group: list[str] = [] + found_violation = False + + def flush() -> None: + nonlocal found_violation + if not group: + return + diagnostic_match = DIAGNOSTIC_RE.match(group[0]) + is_cast_diagnostic = any(diag in group[0] for diag in OLD_STYLE_CAST_DIAGNOSTICS) + macro_expansions = [ + expansion_match + for line in group + if (expansion_match := MACRO_EXPANSION_RE.match(line)) + ] + target_file = macro_expansions[-1].group(1) if macro_expansions else (diagnostic_match.group(1) if diagnostic_match else "") + is_dash_diagnostic = is_dash_file(target_file, dash_files) if target_file else False + + if not is_cast_diagnostic or is_dash_diagnostic: + output_stream.writelines(group) + found_violation |= is_cast_diagnostic and is_dash_diagnostic + group.clear() + + for line in input_stream: + if DIAGNOSTIC_RE.match(line): + flush() + group.append(line) + elif group: + group.append(line) + else: + output_stream.write(line) + flush() + return found_violation + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + subparsers = parser.add_subparsers(dest="command", required=True) + + prepare = subparsers.add_parser("prepare", help="create a Dash-aware compilation database") + prepare.add_argument("--input", type=Path, required=True) + prepare.add_argument("--output-dir", type=Path, required=True) + prepare.add_argument("--source-root", type=Path, required=True) + + filter_parser = subparsers.add_parser("filter", help="filter clang-tidy diagnostics") + filter_parser.add_argument("--source-root", type=Path, required=True) + + args = parser.parse_args() + source_root = args.source_root.resolve() + if args.command == "prepare": + prepare_compile_database(source_root, args.input.resolve(), args.output_dir.resolve()) + return 0 + return int(filter_diagnostics(source_root, sys.stdin, sys.stdout)) + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/ci/dash/lint-tidy.sh b/ci/dash/lint-tidy.sh index 757009610a9c..e1d736aed287 100755 --- a/ci/dash/lint-tidy.sh +++ b/ci/dash/lint-tidy.sh @@ -66,8 +66,20 @@ python3 "${CLANG_TIDY_CACHE_PY}" --zero-stats 2>&1 || true cd "${BASE_ROOT_DIR}/build-ci/dashcore-${BUILD_TARGET}/src" -if ! ( run-clang-tidy -clang-tidy-binary="${CLANG_TIDY_CACHE}" -quiet "${MAKEJOBS}" | tee tmp.tidy-out.txt ); then - grep -C5 "error: " tmp.tidy-out.txt +CAST_LINT_DB="${PWD}/../cstyle-cast-compile-db" +python3 "${BASE_ROOT_DIR}/ci/dash/lint-cstyle-casts.py" prepare \ + --input "${PWD}/../compile_commands.json" \ + --output-dir "${CAST_LINT_DB}" \ + --source-root "${BASE_ROOT_DIR}" + +if ! ( run-clang-tidy \ + -checks=clang-diagnostic-old-style-cast,google-readability-casting \ + -clang-tidy-binary="${CLANG_TIDY_CACHE}" \ + -p "${CAST_LINT_DB}" \ + -quiet "${MAKEJOBS}" | \ + python3 "${BASE_ROOT_DIR}/ci/dash/lint-cstyle-casts.py" filter --source-root "${BASE_ROOT_DIR}" | \ + tee tmp.tidy-out.txt ); then + grep -E -C5 "error: |warning: use of old-style cast|google-readability-casting" tmp.tidy-out.txt echo "^^^ ⚠️ Failure generated from clang-tidy" false fi diff --git a/ci/dash/matrix.sh b/ci/dash/matrix.sh index 49117d71c82f..e54e279422c4 100755 --- a/ci/dash/matrix.sh +++ b/ci/dash/matrix.sh @@ -12,7 +12,7 @@ source ./ci/test/00_setup_env.sh # Configure sanitizers options export ASAN_OPTIONS="detect_leaks=1:detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1" -export LSAN_OPTIONS="suppressions=${BASE_ROOT_DIR}/test/sanitizer_suppressions/lsan" +export LSAN_OPTIONS="suppressions=${BASE_ROOT_DIR}/test/sanitizer_suppressions/lsan:print_suppressions=0" export TSAN_OPTIONS="suppressions=${BASE_ROOT_DIR}/test/sanitizer_suppressions/tsan:halt_on_error=1:second_deadlock_stack=1" export UBSAN_OPTIONS="suppressions=${BASE_ROOT_DIR}/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" diff --git a/ci/dash/test_integrationtests.sh b/ci/dash/test_integrationtests.sh index d546931d7cae..4b114472693d 100755 --- a/ci/dash/test_integrationtests.sh +++ b/ci/dash/test_integrationtests.sh @@ -28,6 +28,11 @@ fi cd "build-ci/dashcore-$BUILD_TARGET" +if [ -n "${CI_LIMIT_STACK_SIZE}" ]; then + # Upstream uses 512, which segfaults dashd during test framework startup. + ulimit -s 1024 +fi + if [ "$SOCKETEVENTS" = "" ]; then # Let's switch socketevents mode to some random mode R=$((RANDOM%3)) diff --git a/ci/test/00_setup_env_native_asan.sh b/ci/test/00_setup_env_native_asan.sh index c3029cfe68b4..a92cbebf23cd 100755 --- a/ci/test/00_setup_env_native_asan.sh +++ b/ci/test/00_setup_env_native_asan.sh @@ -7,12 +7,13 @@ export LC_ALL=C.UTF-8 export CONTAINER_NAME=ci_native_asan -export PACKAGES="clang-19 llvm-19 libclang-rt-19-dev python3-zmq qtbase5-dev qttools5-dev-tools libevent-dev bsdmainutils libboost-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libqrencode-dev" +export PACKAGES="clang-19 llvm-19 libclang-rt-19-dev python3-zmq qtbase5-dev qttools5-dev-tools libevent-dev bsdmainutils libboost-dev libminiupnpc-dev libzmq3-dev libqrencode-dev" # Reuses the depends built for the linux64 target, which uses the defaults. export DEP_OPTS="" -export TEST_RUNNER_EXTRA="--timeout-factor=4 -j2" # Increase timeout because sanitizers slow down +export TEST_RUNNER_EXTRA="--timeout-factor=4 -j4" # Increase timeout because sanitizers slow down +export CI_LIMIT_STACK_SIZE=1 export GOAL="install" -export BITCOIN_CONFIG="--enable-zmq --enable-crash-hooks --with-gui=qt5 \ +export BITCOIN_CONFIG="--enable-zmq --enable-crash-hooks --with-gui=qt5 --without-bdb --with-sqlite \ --with-sanitizers=address,float-divide-by-zero,integer,undefined \ CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKORDER' \ CC='clang-19 -ftrivial-auto-var-init=pattern' CXX='clang++-19 -ftrivial-auto-var-init=pattern'" diff --git a/ci/test/04_install.sh b/ci/test/04_install.sh index dbe478598b11..992d1ed19b17 100755 --- a/ci/test/04_install.sh +++ b/ci/test/04_install.sh @@ -15,7 +15,7 @@ mkdir -p "${CCACHE_DIR}" mkdir -p "${PREVIOUS_RELEASES_DIR}" export ASAN_OPTIONS="detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1" -export LSAN_OPTIONS="suppressions=${BASE_BUILD_DIR}/test/sanitizer_suppressions/lsan" +export LSAN_OPTIONS="suppressions=${BASE_BUILD_DIR}/test/sanitizer_suppressions/lsan:print_suppressions=0" export TSAN_OPTIONS="suppressions=${BASE_BUILD_DIR}/test/sanitizer_suppressions/tsan:halt_on_error=1" export UBSAN_OPTIONS="suppressions=${BASE_BUILD_DIR}/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" env | grep -E '^(BASE_|QEMU_|CCACHE_|LC_ALL|BOOST_TEST_RANDOM|DEBIAN_FRONTEND|CONFIG_SHELL|(ASAN|LSAN|TSAN|UBSAN)_OPTIONS|PREVIOUS_RELEASES_DIR))' | tee /tmp/env diff --git a/configure.ac b/configure.ac index d36f8046cfa2..daf733fb2224 100644 --- a/configure.ac +++ b/configure.ac @@ -2077,7 +2077,9 @@ CPPFLAGS="$CPPFLAGS_TEMP" if test -n "$use_sanitizers"; then export SECP_CFLAGS="$SECP_CFLAGS $SANITIZER_CFLAGS" fi -ac_configure_args="${ac_configure_args} --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --disable-module-ecdh --disable-openssl-tests" +dnl The ECDH module is required by the wallet's Platform key provider +dnl (DashPay contact request encryption, wallet/platformkeys.cpp). +ac_configure_args="${ac_configure_args} --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-ecdh --disable-openssl-tests" AC_CONFIG_SUBDIRS([src/dashbls src/secp256k1]) AC_OUTPUT diff --git a/contrib/devtools/circular-dependencies.py b/contrib/devtools/circular-dependencies.py index a6cdc343d5cb..6cbbdb189eea 100755 --- a/contrib/devtools/circular-dependencies.py +++ b/contrib/devtools/circular-dependencies.py @@ -11,6 +11,10 @@ MAPPING = { 'core_read.cpp': 'core_io.cpp', 'core_write.cpp': 'core_io.cpp', + 'evo/core_write.cpp': 'core_io.cpp', + 'evo/providertx_util.cpp': 'evo/providertx.cpp', + 'llmq/core_write.cpp': 'core_io.cpp', + 'qt/guiutil_font.cpp': 'qt/guiutil.cpp', } # Directories with header-based modules, where the assumption that .cpp files diff --git a/doc/design/assumeutxo.md b/doc/design/assumeutxo.md index 74aff7d392f8..9846f7f26a0b 100644 --- a/doc/design/assumeutxo.md +++ b/doc/design/assumeutxo.md @@ -3,9 +3,9 @@ Assumeutxo is a feature that allows fast bootstrapping of a validating dashd instance with a very similar security model to assumevalid. -The RPC commands `dumptxoutset` and `loadtxoutset` are used to respectively generate -and load UTXO snapshots. The utility script `./contrib/devtools/utxo_snapshot.sh` may -be of use. +The RPC commands `dumptxoutset` and `loadtxoutset` (yet to be merged) are used to +respectively generate and load UTXO snapshots. The utility script +`./contrib/devtools/utxo_snapshot.sh` may be of use. ## General background @@ -17,14 +17,9 @@ be of use. - A new block index `nStatus` flag is introduced, `BLOCK_ASSUMED_VALID`, to mark block index entries that are required to be assumed-valid by a chainstate created - from a UTXO snapshot. This flag is mostly used as a way to modify certain + from a UTXO snapshot. This flag is used as a way to modify certain CheckBlockIndex() logic to account for index entries that are pending validation by a - chainstate running asynchronously in the background. We also use this flag to control - which index entries are added to setBlockIndexCandidates during LoadBlockIndex(). - -- Indexing implementations via BaseIndex can no longer assume that indexation happens - sequentially, since background validation chainstates can submit BlockConnected - events out of order with the active chain. + chainstate running asynchronously in the background. - The concept of UTXO snapshots is treated as an implementation detail that lives behind the ChainstateManager interface. The external presentation of the changes @@ -76,9 +71,15 @@ original chainstate remains in use as active. Once the snapshot chainstate is loaded and validated, it is promoted to active chainstate and a sync to tip begins. A new chainstate directory is created in the -datadir for the snapshot chainstate called `chainstate_snapshot`. When this directory -is present in the datadir, the snapshot chainstate will be detected and loaded as -active on node startup (via `DetectSnapshotChainstate()`). +datadir for the snapshot chainstate called `chainstate_snapshot`. + +When this directory is present in the datadir, the snapshot chainstate will be detected +and loaded as active on node startup (via `DetectSnapshotChainstate()`). + +A special file is created within that directory, `base_blockhash`, which contains the +serialized `uint256` of the base block of the snapshot. This is used to reinitialize +the snapshot chainstate on subsequent inits. Otherwise, the directory is a normal +leveldb database. | | | | ---------- | ----------- | @@ -88,7 +89,7 @@ active on node startup (via `DetectSnapshotChainstate()`). The snapshot begins to sync to tip from its base block, technically in parallel with the original chainstate, but it is given priority during block download and is allocated most of the cache (see `MaybeRebalanceCaches()` and usages) as our chief -consideration is getting to network tip. +goal is getting to network tip. **Failure consideration:** if shutdown happens at any point during this phase, both chainstates will be detected during the next init and the process will resume. @@ -107,33 +108,36 @@ sequentially. ### Background chainstate hits snapshot base block Once the tip of the background chainstate hits the base block of the snapshot -chainstate, we stop use of the background chainstate by setting `m_stop_use` (not yet -committed - see bitcoin#15606), in `CompleteSnapshotValidation()`, which is checked in -`ActivateBestChain()`). We hash the background chainstate's UTXO set contents and -ensure it matches the compiled value in `CMainParams::m_assumeutxo_data`. - -The background chainstate data lingers on disk until shutdown, when in -`ChainstateManager::Reset()`, the background chainstate is cleaned up with -`ValidatedSnapshotShutdownCleanup()`, which renames the `chainstate_[hash]` datadir as -`chainstate`. +chainstate, we stop use of the background chainstate by setting `m_disabled`, in +`MaybeCompleteSnapshotValidation()`, which is checked in `ActivateBestChain()`. We hash the +background chainstate's UTXO set contents and ensure it matches the compiled value in +`CMainParams::m_assumeutxo_data`. In Dash, completion additionally compares the +deterministic masternode-list hash the background chainstate derived at the base block +against the hash recorded at snapshot activation, and the EvoDB best-block markers +against both chainstates' coins tips; any divergence fails completion with +`EVO_STATE_MISMATCH` and quarantines the snapshot exactly like a UTXO hash mismatch. | | | | ---------- | ----------- | -| number of chainstates | 2 (ibd has `m_stop_use=true`) | +| number of chainstates | 2 (ibd has `m_disabled=true`) | | active chainstate | snapshot | -**Failure consideration:** if dashd unexpectedly halts after `m_stop_use` is set on -the background chainstate but before `CompleteSnapshotValidation()` can finish, the -need to complete snapshot validation will be detected on subsequent init by -`ChainstateManager::CheckForUncleanShutdown()`. +The background chainstate data lingers on disk until the program is restarted. ### Dashd restarts sometime after snapshot validation has completed -When dashd initializes again, what began as the snapshot chainstate is now -indistinguishable from a chainstate that has been built from the traditional IBD -process, and will be initialized as such. +After a shutdown and subsequent restart, `LoadChainstate()` cleans up the background +chainstate with `ValidatedSnapshotCleanup()`, which renames the `chainstate_snapshot` +datadir as `chainstate` and removes the now unnecessary background chainstate data. | | | | ---------- | ----------- | | number of chainstates | 1 | -| active chainstate | ibd | +| active chainstate | ibd (was snapshot, but is now fully validated) | + +What began as the snapshot chainstate is now indistinguishable from a chainstate that +has been built from the traditional IBD process, and will be initialized as such. + +A file will be left in `chainstate/base_blockhash`, which indicates that the +chainstate, even though now fully validated, was originally started from a snapshot +with the corresponding base blockhash. diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 68495d1d10bf..9f26b39d1848 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -17,6 +17,7 @@ Developer Notes - [Devnet, testnet, and regtest modes](#devnet-testnet-and-regtest-modes) - [DEBUG_LOCKORDER](#debug_lockorder) - [DEBUG_LOCKCONTENTION](#debug_lockcontention) + - [Assertions and Checks](#assertions-and-checks) - [Valgrind suppressions file](#valgrind-suppressions-file) - [Compiling for test coverage](#compiling-for-test-coverage) - [Performance profiling with perf](#performance-profiling-with-perf) @@ -108,6 +109,7 @@ code. - `++i` is preferred over `i++`. - `nullptr` is preferred over `NULL` or `(void*)0`. - `static_assert` is preferred over `assert` where possible. Generally; compile-time checking is preferred over run-time checking. + For run-time checks, see [Assertions and Checks](#assertions-and-checks) on choosing between `assert`/`Assert`, `Assume` and `CHECK_NONFATAL`. - Align pointers and references to the left i.e. use `type& var` and not `type &var`. - Use a named cast or functional cast, not a C-Style cast. When casting between integer types, use functional casts such as `int(x)` or `int{x}` @@ -432,30 +434,73 @@ It can be toggled off again with `dash-cli logging [] '["lock"]'`. ### Assertions and Checks -The util file `src/util/check.h` offers helpers to protect against coding and -internal logic bugs. They must never be used to validate user, network or any -other input. - -* `assert` or `Assert` should be used to document assumptions when any - violation would mean that it is not safe to continue program execution. The - code is always compiled with assertions enabled. - - For example, a nullptr dereference or any other logic bug in validation - code means the program code is faulty and must terminate immediately. -* `CHECK_NONFATAL` should be used for recoverable internal logic bugs. On - failure, it will throw an exception, which can be caught to recover from the - error. - - For example, a nullptr dereference or any other logic bug in RPC code - means that the RPC code is faulty and cannot be executed. However, the - logic bug can be shown to the user and the program can continue to run. -* `Assume` should be used to document assumptions when program execution can - safely continue even if the assumption is violated. In debug builds it - behaves like `Assert`/`assert` to notify developers and testers about - nonfatal errors. In production it doesn't warn or log anything, though the - expression is always evaluated. - - For example it can be assumed that a variable is only initialized once, - but a failed assumption does not result in a fatal bug. A failed - assumption may or may not result in a slightly degraded user experience, - but it is safe to continue program execution. +The util file [`src/util/check.h`](../src/util/check.h) offers helpers to +protect against coding and internal logic bugs. They document invariants the +code itself is responsible for maintaining, and must never be used to validate +user, network, RPC, disk or any other input: untrusted data that does not +match expectations is an ordinary error to be handled, not a bug to be +reported. + +Pick the helper by the cost of continuing with the invariant violated: + +| Cost of continuing | Use | +| --- | --- | +| Undefined behavior, memory corruption, or corrupt persisted/consensus state | `assert` / `Assert` | +| A bug worth investigating, but execution stays well-defined | `Assume` | +| Same, but there is a caller who can be told (RPC/CLI) | `CHECK_NONFATAL` / `NONFATAL_UNREACHABLE` | +| Nothing is broken in-process; the environment failed (disk full, failed DB write) | Not a check: return an error, `AbortNode()`, or `InitError()` | + +**`Assume` is the default.** Reach for `Assert`/`assert` only when you can name +the undefined behavior or corruption that continuing would cause. + +* `Assume` documents "this is how things are supposed to be": a violation means + someone has a bug to chase, but execution stays well-defined. The archetype: + + ```cpp + // Somebody decremented twice if this trips. Nothing is corrupt, but the + // rate limiter is not limiting anything, so it is worth finding out about. + Assume(m_request_count >= 0); + ``` + + A bugged rate limiter may expose us to extra DoS pressure; aborting would + turn that into a guaranteed outage for every user running the release. Never + let an `Assume` be the thing that kills a production node. The expression is + always evaluated, in every build; failures abort only where + `-DABORT_ON_FAILED_ASSUME` is defined — `--enable-debug` and `--enable-fuzz` + builds, i.e. CI's `linux64_multiprocess` and fuzz jobs — and are silent in + release. That coverage is partial: an invariant you actually care about also + needs a test, and code downstream must still cope with the violated case. + +* `assert` / `Assert` is for "continuing is unsafe": a nullptr dereference, + out-of-bounds access, or an invariant whose violation would corrupt data on + disk or consensus state. Aborting must be the safer outcome; such cases + should be rare and obvious to a reader. It is still the right tool where a + precondition genuinely keeps the code below it safe: + `Chainstate::ConnectBlock()` dereferences `pindex` unconditionally, so its + `assert(pindex);` just makes a guaranteed crash diagnosable. Plain `assert()` + is always active — the build never defines `NDEBUG` (`check.h` refuses to + compile with it). `Assert` returns its argument, so a check followed by a + use of the checked value collapses into one expression: + `assert(ptr != nullptr); obj = *ptr;` becomes `obj = *Assert(ptr);` + (`Assume` is an identity function too). Prefer `static_assert` when the + condition is known at compile time. + +* `CHECK_NONFATAL` / `NONFATAL_UNREACHABLE` report internal logic bugs to a + caller: they throw `NonFatalCheckError`, which RPC code catches and turns + into an error message asking the user to file a bug report, and the node + keeps running. Mandatory in RPC code, enforced (best-effort) by + `test/lint/lint-assertions.py` for `src/rpc/` and `src/wallet/rpc*`; use + `NONFATAL_UNREACHABLE()` instead of `assert(false)` there. + +An assertion reachable from P2P messages, RPC arguments, wallet files, or +on-disk data is a remote crash. This cuts especially deep in Dash-specific +code: masternode, LLMQ, InstantSend, ChainLocks, governance and CoinJoin paths +process peer-chosen message contents and read state (EvoDB, quorum caches, DKG +sessions) possibly written by an older or buggy version. There, validate and +reject (misbehaving peer, `state.Invalid(...)`, early return) rather than +assert; use `Assume` for our *own* bookkeeping while still handling the +violated case; and reserve `assert` for the narrow spot where continuing would +corrupt EvoDB, the block index, or the wallet. ### Valgrind suppressions file diff --git a/doc/release-notes-25122.md b/doc/release-notes-25122.md new file mode 100644 index 000000000000..5c5f0f911154 --- /dev/null +++ b/doc/release-notes-25122.md @@ -0,0 +1,7 @@ + +Wallet +------ + +- RPC `getreceivedbylabel` now returns an error, "Label not found + in wallet" (-4), if the label is not in the address book. (dash#7550) + diff --git a/doc/release-notes-25504.md b/doc/release-notes-25504.md new file mode 100644 index 000000000000..bf80f180318a --- /dev/null +++ b/doc/release-notes-25504.md @@ -0,0 +1,6 @@ +Updated RPCs +------------ + +- The `listsinceblock`, `listtransactions` and `gettransaction` output now contain a new + `parent_descs` field for every "receive" entry. +- A new optional `include_change` parameter was added to the `listsinceblock` command. 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/doc/release-notes-7052.md b/doc/release-notes-7052.md new file mode 100644 index 000000000000..fd611b543701 --- /dev/null +++ b/doc/release-notes-7052.md @@ -0,0 +1,45 @@ +P2P and network changes +----------------------- + +- The protocol version was bumped to 70241. The `dsa` message gained a + version-gated flags field declaring which mixing direction a + participant intends. A session commits to carrying promotion/demotion + entries only once a participant is admitted that actually declared one, + and it becomes closed to pre-70241 clients only from that point on; + conversely, a session that has already admitted a pre-70241 client + refuses later promotion/demotion participants. Either way the refusal + happens at acceptance time, before any collateral is committed, so a + client doing ordinary 1:1 mixing is never turned away from a session + simply because of who opened it. Unbalanced (promotion/demotion) DSTXes + are only announced as `dstx` to peers at protocol 70241 or newer, since + older peers would reject them as structurally invalid and penalize the + relayer; those peers are sent a plain `tx` announcement instead, so + they still receive the transaction without the mixing metadata they + cannot parse. (#7052) + +- A mixing session only completes once each side of its denomination is + occupied by nobody or by at least two participants, since coins are + only concealed by other coins of the same size on the same side. A + session that attracts a lone promotion or demotion participant and no + counterpart therefore waits, and expires in the queue stage without + charging anyone's collateral, rather than publishing a transaction that + would identify that participant's coins. Because admission relies on + the declared directions, a participant whose entry deviates from what + it declared has its collateral consumed. (#7052) + +Wallet +------ + +- CoinJoin can now promote and demote between adjacent standard + denominations within a mixing session after V24 activation. + Promotion combines 10 inputs of one denomination into 1 output of the + next larger denomination, while demotion splits 1 input into 10 + outputs of the next smaller denomination. Pre-V24 behavior remains + unchanged. (#7052) + +- Conversions only spend fully-mixed coins, and their outputs start + mixing over from zero rounds. The 10:1 shape of a conversion publicly + clusters one participant's coins even inside a mixing transaction, so + a converted coin is not treated as mixed: it re-enters mixing at its + new denomination and disperses normally, while the histories of the + fully-mixed coins that fed the conversion remain protected. (#7052) diff --git a/doc/release-notes-7485.md b/doc/release-notes-7485.md new file mode 100644 index 000000000000..a9d13a78773d --- /dev/null +++ b/doc/release-notes-7485.md @@ -0,0 +1,8 @@ +P2P and network +--------------- + +- Bound in-memory masternode list caches so unauthenticated historical + `GETMNLISTDIFF` requests can no longer grow memory without limit between + blocks. Recent lists are capped by height-aware eviction; stale historical + mini-snapshots are kept in a small LRU cache so repeated requests do not + re-read large on-disk snapshots on every call. (#7485) diff --git a/doc/release-notes-7570.md b/doc/release-notes-7570.md new file mode 100644 index 000000000000..4147781d3abc --- /dev/null +++ b/doc/release-notes-7570.md @@ -0,0 +1,8 @@ +Bug Fixes +--------- + +- Block template creation now checks credit pool limits across complete + transaction packages. Because Asset Unlock limits are cumulative, a package + may exceed the block's limit even though its transactions were accepted + individually. Such packages are now skipped so miners can continue building + a template instead of template creation failing. (#7570) diff --git a/doc/release-notes-7595.md b/doc/release-notes-7595.md new file mode 100644 index 000000000000..deedec190141 --- /dev/null +++ b/doc/release-notes-7595.md @@ -0,0 +1,9 @@ +GUI changes +----------- + +- Dash-Qt now labels masternode registration and update transactions in the + transaction history as **Masternode Registration** and **Masternode Update** + instead of generic "Payment to yourself" rows. A new **Masternode** filter + shows only these operations. The amount shown is the transaction's net effect + on your wallet (for example, the network fee on a self-funded registration). + (#7595) diff --git a/doc/release-notes-7600.md b/doc/release-notes-7600.md new file mode 100644 index 000000000000..323608c15252 --- /dev/null +++ b/doc/release-notes-7600.md @@ -0,0 +1,16 @@ +RPC changes +----------- + +- Normal and Evo `protx` registration and maintenance commands now share a + typed provider-transaction implementation with other wallet frontends. RPC + names and successful result formats are unchanged. Fixed: when a wallet + cannot completely sign the inputs it selected (e.g. `protx register_submit` + run in a different wallet than the one that prepared the registration), + the command now fails with a clear wallet error naming the problem instead + of reporting success with a partially signed transaction or attempting a + broadcast that failed mempool acceptance with a bare `-26` error. The + external-signing workflow (`protx register_prepare` followed by + `protx register_submit`) is unchanged. + `protx update_service` on a masternode whose state does not yield a usable + default fee source now returns an explicit "specify feeSourceAddress" + parameter error instead of an internal error. (#7600) diff --git a/doc/release-notes-7618.md b/doc/release-notes-7618.md new file mode 100644 index 000000000000..02ea16131a6b --- /dev/null +++ b/doc/release-notes-7618.md @@ -0,0 +1,9 @@ +GUI changes +----------- + +- Dash-Qt can now register and maintain regular masternodes and EvoNodes from + the Masternodes tab. The registration wizard supports wallet-funded, + wallet-owned, and externally held collateral, while row actions provide + Update Service, Update Registrar, and Revoke workflows. Generated operator + keys are shown and confirmed before registration; wallet-seed-derived + operator keys are not required. (#7618) diff --git a/src/Makefile.am b/src/Makefile.am index fb1ffde2529a..c09876eca3fc 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -232,6 +232,7 @@ BITCOIN_CORE_H = \ evo/mnhftx.h \ evo/netinfo.h \ evo/providertx.h \ + evo/providertx_service.h \ evo/simplifiedmns.h \ evo/smldiff.h \ evo/specialtx.h \ @@ -281,6 +282,7 @@ BITCOIN_CORE_H = \ interfaces/init.h \ interfaces/ipc.h \ interfaces/node.h \ + interfaces/providertx.h \ interfaces/wallet.h \ kernel/blockmanager_opts.h \ kernel/chain.h \ @@ -370,7 +372,7 @@ BITCOIN_CORE_H = \ rest.h \ rpc/blockchain.h \ rpc/client.h \ - rpc/evo_util.h \ + rpc/json_help.h \ rpc/mempool.h \ rpc/mining.h \ rpc/protocol.h \ @@ -476,6 +478,8 @@ BITCOIN_CORE_H = \ wallet/hdchain.h \ wallet/ismine.h \ wallet/load.h \ + wallet/platformkeys.h \ + wallet/platformtypes.h \ wallet/receive.h \ wallet/rpc/util.h \ wallet/rpc/wallet.h \ @@ -539,6 +543,7 @@ libbitcoin_node_a_SOURCES = \ evo/mnauth.cpp \ evo/mnhftx.cpp \ evo/providertx.cpp \ + evo/providertx_service.cpp \ evo/simplifiedmns.cpp \ evo/smldiff.cpp \ evo/specialtx.cpp \ @@ -635,6 +640,7 @@ libbitcoin_node_a_SOURCES = \ rpc/evo.cpp \ rpc/fees.cpp \ rpc/governance.cpp \ + rpc/json_help.cpp \ rpc/masternode.cpp \ rpc/mempool.cpp \ rpc/mining.cpp \ @@ -706,6 +712,7 @@ libbitcoin_wallet_a_SOURCES = \ wallet/hdchain.cpp \ wallet/interfaces.cpp \ wallet/load.cpp \ + wallet/platformkeys.cpp \ wallet/receive.cpp \ wallet/rpc/addresses.cpp \ wallet/rpc/backup.cpp \ @@ -966,7 +973,6 @@ libbitcoin_common_a_SOURCES = \ evo/providertx_util.cpp \ external_signer.cpp \ governance/common.cpp \ - governance/core_write.cpp \ init/common.cpp \ key.cpp \ key_io.cpp \ @@ -981,7 +987,6 @@ libbitcoin_common_a_SOURCES = \ policy/policy.cpp \ protocol.cpp \ psbt.cpp \ - rpc/evo_util.cpp \ rpc/external_signer.cpp \ rpc/rawtransaction_util.cpp \ rpc/request.cpp \ diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include index 16296207eca1..56ddd8a12156 100644 --- a/src/Makefile.qt.include +++ b/src/Makefile.qt.include @@ -71,8 +71,12 @@ QT_MOC_CPP = \ qt/moc_intro.cpp \ qt/moc_macdockiconhandler.cpp \ qt/moc_macnotificationhandler.cpp \ + qt/moc_masternodedialogs.cpp \ qt/moc_masternodelist.cpp \ qt/moc_masternodemodel.cpp \ + qt/moc_masternodeoperationrunner.cpp \ + qt/moc_masternodewidgets.cpp \ + qt/moc_masternodewizard.cpp \ qt/moc_mnemonicverificationdialog.cpp \ qt/moc_modaloverlay.cpp \ qt/moc_networkwidget.cpp \ @@ -158,8 +162,12 @@ BITCOIN_QT_H = \ qt/macdockiconhandler.h \ qt/macnotificationhandler.h \ qt/macos_appnap.h \ + qt/masternodedialogs.h \ qt/masternodelist.h \ qt/masternodemodel.h \ + qt/masternodeoperationrunner.h \ + qt/masternodewidgets.h \ + qt/masternodewizard.h \ qt/mnemonicverificationdialog.h \ qt/modaloverlay.h \ qt/networkstyle.h \ @@ -302,7 +310,11 @@ BITCOIN_QT_WALLET_CPP = \ qt/createwalletdialog.cpp \ qt/descriptiondialog.cpp \ qt/editaddressdialog.cpp \ + qt/masternodedialogs.cpp \ qt/masternodelist.cpp \ + qt/masternodeoperationrunner.cpp \ + qt/masternodewidgets.cpp \ + qt/masternodewizard.cpp \ qt/mnemonicverificationdialog.cpp \ qt/openuridialog.cpp \ qt/overviewpage.cpp \ diff --git a/src/Makefile.qttest.include b/src/Makefile.qttest.include index 62071d40e5a5..00401c6340d7 100644 --- a/src/Makefile.qttest.include +++ b/src/Makefile.qttest.include @@ -16,13 +16,19 @@ TEST_QT_MOC_CPP = \ if ENABLE_WALLET TEST_QT_MOC_CPP += \ qt/test/moc_addressbooktests.cpp \ + qt/test/moc_providertransactiontests.cpp \ + qt/test/moc_masternodewidgettests.cpp \ + qt/test/moc_masternodemaintenancetests.cpp \ qt/test/moc_wallettests.cpp endif # ENABLE_WALLET TEST_QT_H = \ qt/test/addressbooktests.h \ qt/test/apptests.h \ + qt/test/masternodemaintenancetests.h \ + qt/test/masternodewidgettests.h \ qt/test/optiontests.h \ + qt/test/providertransactiontests.h \ qt/test/rpcnestedtests.h \ qt/test/uritests.h \ qt/test/util.h \ @@ -45,6 +51,9 @@ qt_test_test_dash_qt_SOURCES = \ if ENABLE_WALLET qt_test_test_dash_qt_SOURCES += \ qt/test/addressbooktests.cpp \ + qt/test/providertransactiontests.cpp \ + qt/test/masternodewidgettests.cpp \ + qt/test/masternodemaintenancetests.cpp \ qt/test/wallettests.cpp \ wallet/test/wallet_test_fixture.cpp endif # ENABLE_WALLET diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 427909b8e12f..a34ea4b120a3 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -147,6 +147,7 @@ BITCOIN_TESTS =\ test/llmq_hash_tests.cpp \ test/llmq_invalid_type_tests.cpp \ test/llmq_params_tests.cpp \ + test/llmq_qgetdata_tests.cpp \ test/llmq_snapshot_tests.cpp \ test/llmq_utils_tests.cpp \ test/logging_tests.cpp \ @@ -224,6 +225,7 @@ if ENABLE_WALLET BITCOIN_TESTS += \ wallet/test/bip39_tests.cpp \ wallet/test/coinjoin_tests.cpp \ + wallet/test/platformkeys_tests.cpp \ wallet/test/psbt_wallet_tests.cpp \ wallet/test/spend_tests.cpp \ wallet/test/wallet_tests.cpp \ @@ -235,7 +237,8 @@ BITCOIN_TESTS += \ wallet/test/init_tests.cpp \ wallet/test/ismine_tests.cpp \ wallet/test/rpc_util_tests.cpp \ - wallet/test/scriptpubkeyman_tests.cpp + wallet/test/scriptpubkeyman_tests.cpp \ + wallet/test/walletload_tests.cpp FUZZ_SUITE_LD_COMMON +=\ $(SQLITE_LIBS) \ @@ -321,6 +324,7 @@ test_fuzz_fuzz_SOURCES = \ test/fuzz/decode_tx.cpp \ test/fuzz/descriptor_parse.cpp \ test/fuzz/deserialize.cpp \ + test/fuzz/dkg_message_framing.cpp \ test/fuzz/eval_script.cpp \ test/fuzz/fee_rate.cpp \ test/fuzz/fees.cpp \ diff --git a/src/active/dkgsessionhandler.cpp b/src/active/dkgsessionhandler.cpp index 56cfedf0d42f..a6464a89e712 100644 --- a/src/active/dkgsessionhandler.cpp +++ b/src/active/dkgsessionhandler.cpp @@ -193,7 +193,7 @@ void ActiveDKGSessionHandler::SleepBeforePhase(QuorumPhase curPhase, const uint2 // Don't expect perfect block times and thus reduce the phase time to be on the secure side (caller chooses factor) double adjustedPhaseSleepTimePerMember = phaseSleepTimePerMember * randomSleepFactor; - int64_t sleepTime = (int64_t)(adjustedPhaseSleepTimePerMember * curSession->GetMyMemberIndex().value_or(0)); + int64_t sleepTime = static_cast(adjustedPhaseSleepTimePerMember * curSession->GetMyMemberIndex().value_or(0)); const auto endTime = SteadyClock::now() + std::chrono::milliseconds{sleepTime}; int heightTmp{currentHeight.load()}; int heightStart{heightTmp}; diff --git a/src/bench/block_assemble.cpp b/src/bench/block_assemble.cpp index 70539de36d8d..d5ae9480edd5 100644 --- a/src/bench/block_assemble.cpp +++ b/src/bench/block_assemble.cpp @@ -6,6 +6,7 @@ #include #include #include