Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ $(COV_TOOL_WRAPPER):
@chmod +x $(COV_TOOL_WRAPPER)

baseline.info: $(COV_TOOL_WRAPPER)
$(LCOV) -c -i -d $(abs_builddir)/src -o $@
$(LCOV) $(LCOV_OPTS) -c -i -d $(abs_builddir)/src -o $@

baseline_filtered.info: baseline.info
$(abs_builddir)/contrib/filter-lcov.py $(LCOV_FILTER_PATTERN) $< $@
Expand Down Expand Up @@ -221,13 +221,13 @@ functional_test_filtered.info: functional_test.info
$(LCOV) -a $@ $(LCOV_OPTS) -o $@

fuzz_coverage.info: fuzz_filtered.info
$(LCOV) -a $(LCOV_OPTS) baseline_filtered.info -a fuzz_filtered.info -o $@ | $(GREP) "\%" | $(AWK) '{ print substr($$3,2,50) "/" $$5 }' > coverage_percent.txt
$(LCOV) $(LCOV_OPTS) -a baseline_filtered.info -a fuzz_filtered.info -o $@ | $(GREP) "\%" | $(AWK) '{ print substr($$3,2,50) "/" $$5 }' > coverage_percent.txt

test_dash_coverage.info: baseline_filtered.info test_dash_filtered.info
$(LCOV) -a $(LCOV_OPTS) baseline_filtered.info -a test_dash_filtered.info -o $@
$(LCOV) $(LCOV_OPTS) -a baseline_filtered.info -a test_dash_filtered.info -o $@

total_coverage.info: test_dash_filtered.info functional_test_filtered.info
$(LCOV) -a $(LCOV_OPTS) baseline_filtered.info -a test_dash_filtered.info -a functional_test_filtered.info -o $@ | $(GREP) "\%" | $(AWK) '{ print substr($$3,2,50) "/" $$5 }' > coverage_percent.txt
$(LCOV) $(LCOV_OPTS) -a baseline_filtered.info -a test_dash_filtered.info -a functional_test_filtered.info -o $@ | $(GREP) "\%" | $(AWK) '{ print substr($$3,2,50) "/" $$5 }' > coverage_percent.txt

fuzz.coverage/.dirstamp: fuzz_coverage.info
$(GENHTML) -s $(LCOV_OPTS) $< -o $(@D)
Expand Down
1 change: 1 addition & 0 deletions ci/test/00_setup_env_native_fuzz.sh
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ export RUN_FUZZ_TESTS=true
export GOAL="install"
export BITCOIN_CONFIG="--enable-zmq --enable-fuzz --with-sanitizers=fuzzer,address,undefined,float-divide-by-zero,integer \
CC='clang-19 -ftrivial-auto-var-init=pattern' CXX='clang++-19 -ftrivial-auto-var-init=pattern'"
export LLVM_SYMBOLIZER_PATH="/usr/bin/llvm-symbolizer-17"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect compiler and symbolizer pins in the changed CI setup script.
rg -n 'BITCOIN_CONFIG|LLVM_SYMBOLIZER_PATH|clang-[0-9]+|llvm-symbolizer-[0-9]+' ci/test/00_setup_env_native_fuzz.sh -C1

# Extract and compare major versions (read-only check).
python - <<'PY'
import re
from pathlib import Path

p = Path("ci/test/00_setup_env_native_fuzz.sh").read_text()
clang = re.search(r"clang-(\d+)", p)
symbolizer = re.search(r"llvm-symbolizer-(\d+)", p)

print("clang_major =", clang.group(1) if clang else "not found")
print("symbolizer_major =", symbolizer.group(1) if symbolizer else "not found")

if clang and symbolizer and clang.group(1) != symbolizer.group(1):
    print("RESULT: version mismatch detected (needs explicit justification).")
else:
    print("RESULT: no major-version mismatch detected in this file.")
PY

Repository: dashpay/dash

Length of output: 446


Address LLVM version mismatch and verify CI-scope approval.

Line 19 pins llvm-symbolizer-17 while line 18 compiles with clang-19. This major-version mismatch between the compiler and symbolizer can degrade stack trace quality during fuzzing and sanitizer reporting. Additionally, this change modifies the ci/** directory, which per coding guidelines should not be changed unless explicitly prompted. Confirm that this CI modification is in scope and that the symbolizer version mismatch is intentional.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ci/test/00_setup_env_native_fuzz.sh` at line 19, The export of
LLVM_SYMBOLIZER_PATH points to llvm-symbolizer-17 while the build uses clang-19
(mismatched major versions), and changes to ci/** require explicit scope
approval; update the symbolizer to match the compiler (e.g., set
LLVM_SYMBOLIZER_PATH to llvm-symbolizer-19) or make the clang invocation use
clang-17 so versions align, and add a brief CI note or PR description confirming
that modifying files under ci/** is authorized by the team/CI owners; locate the
assignment to LLVM_SYMBOLIZER_PATH and the clang-19 invocation in the script and
adjust one of them (or add an approval comment) to resolve the mismatch and
policy concern.

Comment thread
knst marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use the installed LLVM 19 symbolizer

The active CI image sets LLVM_VERSION=19 and installs /usr/bin/llvm-symbolizer-19 (with the unversioned alternative), but this fuzz preset exports the nonexistent version-17 path. test/fuzz/test_runner.py:get_fuzz_env passes that value to both ASan and UBSan; ASan rejects an invalid external symbolizer during its initial fuzz -help=1 probe, so the linux64_fuzz job aborts before running any fuzz target.

Useful? React with 👍 / 👎.

5 changes: 2 additions & 3 deletions contrib/devtools/circular-dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import sys
import re
import multiprocessing
from typing import Dict, List, Set

MAPPING = {
'core_read.cpp': 'core_io.cpp',
Expand Down Expand Up @@ -38,13 +37,13 @@ def module_name(path):
return None

files = dict()
deps: Dict[str, Set[str]] = dict()
deps: dict[str, Set[str]] = dict()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore Set before evaluating annotations

After removing Set from the typing import, this module-level annotation is still evaluated at load time under the supported Python 3.10 runtime, so any environment where the import above succeeds will next fail with NameError: name 'Set' is not defined. That still prevents the circular-dependency linter from processing files.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Set is undefined — typing import was removed.

Line 37 still uses Set[str], but the typing import was replaced with multiprocessing on Line 8. This will raise NameError: name 'Set' is not defined at module load time (annotations on module-level variables are evaluated eagerly). Confirmed by both Ruff and Flake8 (F821 undefined name Set). This would break the downstream test/lint/lint-circular-dependencies.py check, which invokes this script via subprocess.

🐛 Proposed fix
-deps: dict[str, Set[str]] = dict()
+deps: dict[str, set[str]] = dict()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
deps: dict[str, Set[str]] = dict()
deps: dict[str, set[str]] = dict()
🧰 Tools
🪛 Flake8 (7.3.0)

[error] 37-37: undefined name 'Set'

(F821)

🪛 Ruff (0.15.20)

[error] 37-37: Undefined name Set

(F821)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@contrib/devtools/circular-dependencies.py` at line 37, The module-level
annotation in circular-dependencies.py still uses Set[str] after the typing
import was removed, causing a NameError at import time. Update the deps
declaration to use a type that is actually imported or remove the annotation
dependency entirely, and make sure any required symbol is brought in near the
top of the file so the script can still run when invoked by the lint check.

Source: Linters/SAST tools


# Defined at module level (reading the global `deps`) so it pickles by reference
# for multiprocessing.Pool; forked workers inherit the populated `deps`.
def handle_module2(module):
# Build the transitive closure of dependencies of module
closure: Dict[str, List[str]] = dict()
closure: dict[str, list[str]] = dict()
for dep in deps[module]:
closure[dep] = []
while True:
Expand Down
3 changes: 1 addition & 2 deletions contrib/devtools/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,9 @@
import shutil
import sys
import os
from typing import List


def determine_wellknown_cmd(envvar, progname) -> List[str]:
def determine_wellknown_cmd(envvar, progname) -> list[str]:
Comment thread
coderabbitai[bot] marked this conversation as resolved.
maybe_env = os.getenv(envvar)
maybe_which = shutil.which(progname)
if maybe_env:
Expand Down
3 changes: 1 addition & 2 deletions contrib/guix/security-check.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
'''
import re
import sys
from typing import List

import lief

Expand Down Expand Up @@ -277,7 +276,7 @@ def check_MACHO_BRANCH_PROTECTION(binary) -> bool:
arch = binary.abstract.header.architecture
binary.concrete

failed: List[str] = []
failed: list[str] = []
for (name, func) in CHECKS[etype][arch]:
if not func(binary):
failed.append(name)
Expand Down
7 changes: 3 additions & 4 deletions contrib/guix/symbol-check.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
find ../path/to/guix/binaries -type f -executable | xargs python3 contrib/guix/symbol-check.py
'''
import sys
from typing import Dict, List

import lief

Expand Down Expand Up @@ -50,7 +49,7 @@

# Expected linker-loader names can be found here:
# https://sourceware.org/glibc/wiki/ABIList?action=recall&rev=16
ELF_INTERPRETER_NAMES: Dict[lief.ELF.ARCH, Dict[lief.ENDIANNESS, str]] = {
ELF_INTERPRETER_NAMES: dict[lief.ELF.ARCH, dict[lief.ENDIANNESS, str]] = {
lief.ELF.ARCH.x86_64: {
lief.ENDIANNESS.LITTLE: "/lib64/ld-linux-x86-64.so.2",
},
Expand All @@ -69,7 +68,7 @@
},
}

ELF_ABIS: Dict[lief.ELF.ARCH, Dict[lief.ENDIANNESS, List[int]]] = {
ELF_ABIS: dict[lief.ELF.ARCH, dict[lief.ENDIANNESS, list[int]]] = {
lief.ELF.ARCH.x86_64: {
lief.ENDIANNESS.LITTLE: [3,2,0],
},
Expand Down Expand Up @@ -303,7 +302,7 @@ def check_ELF_ABI(binary) -> bool:
binary = lief.parse(filename)
etype = binary.format

failed: List[str] = []
failed: list[str] = []
for (name, func) in CHECKS[etype]:
if not func(binary):
failed.append(name)
Expand Down
6 changes: 3 additions & 3 deletions contrib/macdeploy/macdeployqtplus
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import sys, re, os, platform, shutil, stat, subprocess, os.path
from argparse import ArgumentParser
from pathlib import Path
from subprocess import PIPE, run
from typing import List, Optional
from typing import Optional

# This is ported from the original macdeployqt with modifications

Expand Down Expand Up @@ -181,7 +181,7 @@ class DeploymentInfo(object):
return True
return False

def getFrameworks(binaryPath: str, verbose: int) -> List[FrameworkInfo]:
def getFrameworks(binaryPath: str, verbose: int) -> list[FrameworkInfo]:
objdump = os.getenv("OBJDUMP", "objdump")
if verbose:
print(f"Inspecting with {objdump}: {binaryPath}")
Expand Down Expand Up @@ -285,7 +285,7 @@ def copyFramework(framework: FrameworkInfo, path: str, verbose: int) -> Optional

return toPath

def deployFrameworks(frameworks: List[FrameworkInfo], bundlePath: str, binaryPath: str, strip: bool, verbose: int, deploymentInfo: Optional[DeploymentInfo] = None) -> DeploymentInfo:
def deployFrameworks(frameworks: list[FrameworkInfo], bundlePath: str, binaryPath: str, strip: bool, verbose: int, deploymentInfo: Optional[DeploymentInfo] = None) -> DeploymentInfo:
if deploymentInfo is None:
deploymentInfo = DeploymentInfo()

Expand Down
6 changes: 3 additions & 3 deletions contrib/message-capture/message-capture-parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from io import BytesIO
import json
from pathlib import Path
from typing import Any, List, Optional
from typing import Any, Optional
Comment thread
coderabbitai[bot] marked this conversation as resolved.

sys.path.append(os.path.join(os.path.dirname(__file__), '../../test/functional'))

Expand Down Expand Up @@ -92,7 +92,7 @@ def to_jsonable(obj: Any) -> Any:
return obj


def process_file(path: str, messages: List[Any], recv: bool, progress_bar: Optional[ProgressBar]) -> None:
def process_file(path: str, messages: list[Any], recv: bool, progress_bar: Optional[ProgressBar]) -> None:
with open(path, 'rb') as f_in:
if progress_bar:
bytes_read = 0
Expand Down Expand Up @@ -189,7 +189,7 @@ def main():
output = Path.cwd() / Path(args.output) if args.output else False
use_progress_bar = (not args.no_progress_bar) and sys.stdout.isatty()

messages = [] # type: List[Any]
messages = [] # type: list[Any]
if use_progress_bar:
total_size = sum(capture.stat().st_size for capture in capturepaths)
progress_bar = ProgressBar(total_size)
Expand Down
20 changes: 10 additions & 10 deletions contrib/seeds/makeseeds.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import collections
import json
import multiprocessing
from typing import List, Dict, Union
from typing import Union

NSEEDS=512

Expand Down Expand Up @@ -76,28 +76,28 @@ def parseip(ip_in: str) -> Union[dict, None]:
"sortkey": sortkey
}

def filtermulticollateralhash(mns : List[Dict]) -> List[Dict]:
def filtermulticollateralhash(mns : list[dict]) -> list[dict]:
'''Filter out MNs sharing the same collateral hash'''
hist = collections.defaultdict(list)
for mn in mns:
hist[mn['collateralHash']].append(mn)
return [mn for mn in mns if len(hist[mn['collateralHash']]) == 1]

def filtermulticollateraladdress(mns : List[Dict]) -> List[Dict]:
def filtermulticollateraladdress(mns : list[dict]) -> list[dict]:
'''Filter out MNs sharing the same collateral address'''
hist = collections.defaultdict(list)
for mn in mns:
hist[mn['collateralAddress']].append(mn)
return [mn for mn in mns if len(hist[mn['collateralAddress']]) == 1]

def filtermultipayoutaddress(mns : List[Dict]) -> List[Dict]:
def filtermultipayoutaddress(mns : list[dict]) -> list[dict]:
'''Filter out MNs sharing the same payout address'''
hist = collections.defaultdict(list)
for mn in mns:
hist[mn['state']['payoutAddress']].append(mn)
return [mn for mn in mns if len(hist[mn['state']['payoutAddress']]) == 1]

def resolveasn(resolver, ip : Dict) -> Union[int, None]:
def resolveasn(resolver, ip : dict) -> Union[int, None]:
""" Look up the asn for an `ip` address by querying cymru.com
on network `net` (e.g. ipv4 or ipv6).

Expand All @@ -124,7 +124,7 @@ def resolveasn(resolver, ip : Dict) -> Union[int, None]:
return None

# Based on Greg Maxwell's seed_filter.py
def filterbyasn(ips: List[Dict], max_per_asn: Dict, max_per_net: int) -> List[Dict]:
def filterbyasn(ips: list[dict], max_per_asn: dict, max_per_net: int) -> list[dict]:
""" Prunes `ips` by
(a) trimming ips to have at most `max_per_net` ips from each net (e.g. ipv4, ipv6); and
(b) trimming ips to have at most `max_per_asn` ips from each asn in each net.
Expand All @@ -144,8 +144,8 @@ def filterbyasn(ips: List[Dict], max_per_asn: Dict, max_per_net: int) -> List[Di

# Filter IPv46 by ASN, and limit to max_per_net per network
result = []
net_count: Dict[str, int] = collections.defaultdict(int)
asn_count: Dict[int, int] = collections.defaultdict(int)
net_count: dict[str, int] = collections.defaultdict(int)
asn_count: dict[int, int] = collections.defaultdict(int)

for i, ip in enumerate(ips_ipv46):
if i % 10 == 0:
Expand All @@ -169,9 +169,9 @@ def filterbyasn(ips: List[Dict], max_per_asn: Dict, max_per_net: int) -> List[Di
result.extend(ips_onion[0:max_per_net])
return result

def ip_stats(ips: List[Dict]) -> str:
def ip_stats(ips: list[dict]) -> str:
""" Format and return pretty string from `ips`. """
hist: Dict[str, int] = collections.defaultdict(int)
hist: dict[str, int] = collections.defaultdict(int)
for ip in ips:
if ip is not None:
hist[ip['net']] += 1
Expand Down
1 change: 1 addition & 0 deletions src/.clang-tidy
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
Checks: '
-*,
bugprone-argument-comment,
bugprone-string-constructor,
bugprone-use-after-move,
misc-unused-using-decls,
modernize-use-default-member-init,
Expand Down
7 changes: 5 additions & 2 deletions src/Makefile.qt.include
Original file line number Diff line number Diff line change
Expand Up @@ -500,11 +500,14 @@ SECONDARY: $(QT_QM)

$(srcdir)/qt/dashstrings.cpp: FORCE
@test -n $(XGETTEXT) || echo "xgettext is required for updating translations"
$(AM_V_GEN) cd $(srcdir); XGETTEXT=$(XGETTEXT) COPYRIGHT_HOLDERS="$(COPYRIGHT_HOLDERS)" $(PYTHON) ../share/qt/extract_strings_qt.py $(libbitcoin_node_a_SOURCES) $(libbitcoin_wallet_a_SOURCES) $(libbitcoin_common_a_SOURCES) $(libbitcoin_zmq_a_SOURCES) $(libbitcoin_consensus_a_SOURCES) $(libbitcoin_util_a_SOURCES)
$(AM_V_GEN) cd $(srcdir); XGETTEXT=$(XGETTEXT) COPYRIGHT_HOLDERS="$(COPYRIGHT_HOLDERS)" $(PYTHON) ../share/qt/extract_strings_qt.py \
$(libbitcoin_node_a_SOURCES) $(libbitcoin_wallet_a_SOURCES) $(libbitcoin_common_a_SOURCES) \
$(libbitcoin_zmq_a_SOURCES) $(libbitcoin_consensus_a_SOURCES) $(libbitcoin_util_a_SOURCES) \
$(BITCOIN_QT_BASE_CPP) $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)

# The resulted dash_en.xlf source file should follow Transifex requirements.
# See: https://docs.transifex.com/formats/xliff#how-to-distinguish-between-a-source-file-and-a-translation-file
translate: $(srcdir)/qt/dashstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_BASE_CPP) qt/bitcoin.cpp $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
translate: $(srcdir)/qt/dashstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_BASE_CPP) $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
@test -n $(LUPDATE) || echo "lupdate is required for updating translations"
$(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(LUPDATE) -no-obsolete -I $(srcdir) -locations relative $^ -ts $(srcdir)/qt/locale/dash_en.ts
@test -n $(LCONVERT) || echo "lconvert is required for updating translations"
Expand Down
4 changes: 2 additions & 2 deletions src/wallet/db.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,9 @@ bool IsSQLiteFile(const fs::path& path)

file.close();

// Check the magic, see https://sqlite.org/fileformat2.html
// Check the magic, see https://sqlite.org/fileformat.html
std::string magic_str(magic, 16);
if (magic_str != std::string("SQLite format 3", 16)) {
if (magic_str != std::string{"SQLite format 3\000", 16}) {
return false;
}

Expand Down
25 changes: 25 additions & 0 deletions src/wallet/transaction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@

#include <wallet/transaction.h>

#include <interfaces/chain.h>

using interfaces::FoundBlock;

namespace wallet {
bool CWalletTx::IsEquivalentTo(const CWalletTx& _tx) const
{
Expand All @@ -24,4 +28,25 @@ int64_t CWalletTx::GetTxTime() const
int64_t n = nTimeSmart;
return n ? n : nTimeReceived;
}

void CWalletTx::updateState(interfaces::Chain& chain)
{
bool active;
auto lookup_block = [&](const uint256& hash, int& height, TxState& state) {
// If tx block (or conflicting block) was reorged out of chain
// while the wallet was shutdown, change tx status to UNCONFIRMED
// and reset block height, hash, and index. ABANDONED tx don't have
// associated blocks and don't need to be updated. The case where a
// transaction was reorged out while online and then reconfirmed
// while offline is covered by the rescan logic.
if (!chain.findBlock(hash, FoundBlock().inActiveChain(active).height(height)) || !active) {
state = TxStateInactive{};
}
};
if (auto* conf = state<TxStateConfirmed>()) {
lookup_block(conf->confirmed_block_hash, conf->confirmed_block_height, m_state);
} else if (auto* conf = state<TxStateConflicted>()) {
lookup_block(conf->conflicting_block_hash, conf->conflicting_block_height, m_state);
}
}
} // namespace wallet
8 changes: 8 additions & 0 deletions src/wallet/transaction.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@
#include <variant>
#include <vector>

namespace interfaces {
class Chain;
} // namespace interfaces

namespace wallet {
//! State of transaction confirmed in a block.
struct TxStateConfirmed {
Expand Down Expand Up @@ -309,6 +313,10 @@ class CWalletTx
template<typename T> const T* state() const { return std::get_if<T>(&m_state); }
template<typename T> T* state() { return std::get_if<T>(&m_state); }

//! Update transaction state when attaching to a chain, filling in heights
//! of conflicted and confirmed blocks
void updateState(interfaces::Chain& chain);

bool isAbandoned() const { return state<TxStateInactive>() && state<TxStateInactive>()->abandoned; }
bool isConflicted() const { return state<TxStateConflicted>(); }
bool isUnconfirmed() const { return !isAbandoned() && !isConflicted() && !isConfirmed(); }
Expand Down
20 changes: 3 additions & 17 deletions src/wallet/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1157,23 +1157,7 @@ bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx
// If wallet doesn't have a chain (e.g when using dash-wallet tool),
// don't bother to update txn.
if (HaveChain()) {
bool active;
auto lookup_block = [&](const uint256& hash, int& height, TxState& state) {
// If tx block (or conflicting block) was reorged out of chain
// while the wallet was shutdown, change tx status to UNCONFIRMED
// and reset block height, hash, and index. ABANDONED tx don't have
// associated blocks and don't need to be updated. The case where a
// transaction was reorged out while online and then reconfirmed
// while offline is covered by the rescan logic.
if (!chain().findBlock(hash, FoundBlock().inActiveChain(active).height(height)) || !active) {
state = TxStateInactive{};
}
};
if (auto* conf = wtx.state<TxStateConfirmed>()) {
lookup_block(conf->confirmed_block_hash, conf->confirmed_block_height, wtx.m_state);
} else if (auto* conf = wtx.state<TxStateConflicted>()) {
lookup_block(conf->conflicting_block_hash, conf->conflicting_block_height, wtx.m_state);
}
wtx.updateState(chain());
}
if (/* insertion took place */ ins.second) {
wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx));
Expand Down Expand Up @@ -4044,8 +4028,10 @@ int CWallet::GetTxDepthInMainChain(const CWalletTx& wtx) const
{
AssertLockHeld(cs_wallet);
if (auto* conf = wtx.state<TxStateConfirmed>()) {
assert(conf->confirmed_block_height >= 0);
return GetLastBlockHeight() - conf->confirmed_block_height + 1;
} else if (auto* conf = wtx.state<TxStateConflicted>()) {
assert(conf->conflicting_block_height >= 0);
return -1 * (GetLastBlockHeight() - conf->conflicting_block_height + 1);
} else {
return 0;
Expand Down
7 changes: 7 additions & 0 deletions src/wallet/wallet.h
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,13 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
* <0 : conflicts with a transaction this deep in the blockchain
* 0 : in memory pool, waiting to be included in a block
* >=1 : this many blocks deep in the main chain
*
* Preconditions: it is only valid to call this function when the wallet is
* online and the block index is loaded. So this cannot be called by
* bitcoin-wallet tool code or by wallet migration code. If this is called
* without the wallet being online, it won't be able able to determine the
* the height of the last block processed, or the heights of blocks
* referenced in transaction, and might cause assert failures.
Comment on lines +596 to +602

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix wording typos in precondition comment.

Line 535 has a duplicated word (“able able”), and the phrasing around block references is awkward. Small cleanup will avoid confusion in a precondition that now matters more.

✏️ Proposed comment fix
-     * without the wallet being online, it won't be able able to determine the
-     * the height of the last block processed, or the heights of blocks
-     * referenced in transaction, and might cause assert failures.
+     * without the wallet being online, it won't be able to determine the
+     * height of the last block processed, or the heights of blocks
+     * referenced by transactions, and might cause assert failures.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
*
* Preconditions: it is only valid to call this function when the wallet is
* online and the block index is loaded. So this cannot be called by
* bitcoin-wallet tool code or by wallet migration code. If this is called
* without the wallet being online, it won't be able able to determine the
* the height of the last block processed, or the heights of blocks
* referenced in transaction, and might cause assert failures.
*
* Preconditions: it is only valid to call this function when the wallet is
* online and the block index is loaded. So this cannot be called by
* bitcoin-wallet tool code or by wallet migration code. If this is called
* without the wallet being online, it won't be able to determine the
* height of the last block processed, or the heights of blocks
* referenced by transactions, and might cause assert failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/wallet/wallet.h` around lines 531 - 537, Edit the precondition comment in
src/wallet/wallet.h to remove the duplicated word "able" and clarify the awkward
sentence about block references: change "it won't be able able to determine the
the height of the last block processed, or the heights of blocks referenced in
transaction, and might cause assert failures." to a clean phrasing such as "it
won't be able to determine the height of the last processed block or the heights
of blocks referenced by transactions, which may cause assert failures." Update
the comment block near the wallet online/block index precondition so it reads
clearly and contains no duplicated words.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is non-functional upstream comment wording in a backport. Upstream cosmetic style is explicitly excluded from general backport findings.

*/
int GetTxDepthInMainChain(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
bool IsTxInMainChain(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)
Expand Down
Loading
Loading