backport: Merge bitcoin/bitcoin#28857 , 29458, (partial) 29236, 29459, 29479, 29615, 29433#7292
backport: Merge bitcoin/bitcoin#28857 , 29458, (partial) 29236, 29459, 29479, 29615, 29433#7292vijaydasmp wants to merge 7 commits into
Conversation
|
vijaydasmp
changed the title
|
✅ Review complete (commit f1121b5) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a -j/--json output mode to rpcauth.py (and documents it), introduces a hex-parse benchmark and wires it into the bench build, centralizes lint excluded-subtree configuration, updates CScript opcode iteration and multisig sigop counting with tests, adds std::array serialization tests and a TryParseHex reserve optimization, and replaces many return error(...) expressions with LogError(...)+explicit returns across multiple subsystems. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Cppcheck (2.20.0)src/netbase.cppChecking src/netbase.cpp ... 🔧 Infer (1.2.0)src/flatfile.cppsrc/flatfile.cpp:8:10: error: 'flatfile.h' file not found with include; use "quotes" instead ... [truncated 2164 characters] ... "-main-file-name" "flatfile.cpp" src/addrdb.cppsrc/addrdb.cpp:6:10: error: 'addrdb.h' file not found with include; use "quotes" instead ... [truncated 2200 characters] ... .CTrans_funct.instruction_scope in file "src/clang/cTrans.ml", line 4850, characters 22-79 src/index/base.cppsrc/index/base.cpp:5:10: fatal error: 'chainparams.h' file not found ... [truncated 1041 characters] ... ns/clang/install/lib/clang/18/include"
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/lint/lint-includes.py (1)
20-26: ⚡ Quick winDeduplicate
EXCLUDED_DIRSnow that shared exclusions are imported.Line 20-23 duplicates paths already provided by
SHARED_EXCLUDED_SUBTREES, leaving two sources of truth.Proposed refactor
-EXCLUDED_DIRS = ["src/leveldb/", - "src/crc32c/", - "src/secp256k1/", - "src/minisketch/", - "src/dashbls/", - "src/immer/", - "src/crypto/x11/"] + SHARED_EXCLUDED_SUBTREES +EXCLUDED_DIRS = [ + "src/dashbls/", + "src/immer/", + "src/crypto/x11/", +] + SHARED_EXCLUDED_SUBTREES🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/lint/lint-includes.py` around lines 20 - 26, The EXCLUDED_DIRS list duplicates paths that are already present in SHARED_EXCLUDED_SUBTREES; update the definition of EXCLUDED_DIRS to avoid redundant entries by removing the hardcoded directory strings and simply compose EXCLUDED_DIRS from any unique local exclusions (if any) plus SHARED_EXCLUDED_SUBTREES, ensuring EXCLUDED_DIRS references SHARED_EXCLUDED_SUBTREES (symbol names: EXCLUDED_DIRS, SHARED_EXCLUDED_SUBTREES) so there is a single source of truth for shared exclusions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/node/blockstorage.cpp`:
- Around line 807-810: In ReadBlockFromDisk (the branch that currently does
`return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match
index for %s at %s", pindex->ToString(), block_pos.ToString()); return false;`)
remove the unreachable `return false;` and instead replace the `return
error(...)` call with a logging call (e.g., LogError or the project's LogPrint
equivalent) that emits the same message using pindex->ToString() and
block_pos.ToString(), then follow it with `return false;` so the branch matches
the LogError(...); return false; pattern used elsewhere.
In `@src/validation.cpp`:
- Around line 2484-2488: The LogError call after Consensus::CheckTxInputs is
logging state.ToString() which is incorrect because the CheckTxInputs failure
details are in tx_state; update the LogError invocation in the ConnectBlock
error branch to include tx_state.ToString() (and/or
tx_state.GetRejectReason()/GetDebugMessage() if you prefer more detail) so the
actual transaction rejection reason from CheckTxInputs is recorded before
returning state.Invalid; keep the existing return that uses tx_state for the
reject reason/debug message.
In `@test/lint/lint-spelling.py`:
- Line 18: The list comprehension uses the loop variable named dir which shadows
the built-in dir() and triggers Ruff A001; update the variable name in the
expression that builds FILES_ARGS (the comprehension referencing
SHARED_EXCLUDED_SUBTREES) to a non-built-in name like subtree or subdir so it no
longer shadows the built-in (e.g., replace occurrences of dir inside the
comprehension with subtree).
---
Nitpick comments:
In `@test/lint/lint-includes.py`:
- Around line 20-26: The EXCLUDED_DIRS list duplicates paths that are already
present in SHARED_EXCLUDED_SUBTREES; update the definition of EXCLUDED_DIRS to
avoid redundant entries by removing the hardcoded directory strings and simply
compose EXCLUDED_DIRS from any unique local exclusions (if any) plus
SHARED_EXCLUDED_SUBTREES, ensuring EXCLUDED_DIRS references
SHARED_EXCLUDED_SUBTREES (symbol names: EXCLUDED_DIRS, SHARED_EXCLUDED_SUBTREES)
so there is a single source of truth for shared exclusions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4c0b72be-d976-4838-b731-76d2d55c7508
📒 Files selected for processing (26)
share/rpcauth/README.mdshare/rpcauth/rpcauth.pysrc/Makefile.bench.includesrc/addrdb.cppsrc/bench/parse_hex.cppsrc/flatfile.cppsrc/index/base.cppsrc/index/blockfilterindex.cppsrc/index/coinstatsindex.cppsrc/index/txindex.cppsrc/kernel/coinstats.cppsrc/logging.hsrc/net.cppsrc/netbase.cppsrc/node/blockstorage.cppsrc/script/signingprovider.cppsrc/test/serialize_tests.cppsrc/util/strencodings.cppsrc/validation.cpptest/functional/mempool_accept.pytest/functional/test_framework/script.pytest/lint/README.mdtest/lint/lint-include-guards.pytest/lint/lint-includes.pytest/lint/lint-spelling.pytest/lint/lint_ignore_dirs.py
| if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) { | ||
| // Any transaction validation failure in ConnectBlock is a block consensus failure | ||
| LogPrintf("ERROR: %s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString()); | ||
| LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString()); | ||
| return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, | ||
| tx_state.GetRejectReason(), tx_state.GetDebugMessage()); |
There was a problem hiding this comment.
Log the failing tx_state instead of state.
Line 2486 logs state.ToString(), but this branch hasn't copied the CheckTxInputs() failure into state yet. That can hide the actual rejection reason in debug.log and make consensus failures harder to diagnose.
Suggested fix
if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) {
// Any transaction validation failure in ConnectBlock is a block consensus failure
- LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString());
+ LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), tx_state.ToString());
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
tx_state.GetRejectReason(), tx_state.GetDebugMessage());
}📝 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.
| if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) { | |
| // Any transaction validation failure in ConnectBlock is a block consensus failure | |
| LogPrintf("ERROR: %s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString()); | |
| LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString()); | |
| return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, | |
| tx_state.GetRejectReason(), tx_state.GetDebugMessage()); | |
| if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) { | |
| // Any transaction validation failure in ConnectBlock is a block consensus failure | |
| LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), tx_state.ToString()); | |
| return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, | |
| tx_state.GetRejectReason(), tx_state.GetDebugMessage()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/validation.cpp` around lines 2484 - 2488, The LogError call after
Consensus::CheckTxInputs is logging state.ToString() which is incorrect because
the CheckTxInputs failure details are in tx_state; update the LogError
invocation in the ConnectBlock error branch to include tx_state.ToString()
(and/or tx_state.GetRejectReason()/GetDebugMessage() if you prefer more detail)
so the actual transaction rejection reason from CheckTxInputs is recorded before
returning state.Invalid; keep the existing return that uses tx_state for the
reject reason/debug message.
| IGNORE_WORDS_FILE = 'test/lint/spelling.ignore-words.txt' | ||
| FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/dashbls/", ":(exclude)src/crc32c/", ":(exclude)src/crypto/", ":(exclude)src/ctpl_stl.h", ":(exclude)src/cxxtimer.hpp", ":(exclude)src/immer/", ":(exclude)src/leveldb/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)src/secp256k1/", ":(exclude)src/minisketch/", ":(exclude)contrib/builder-keys/", ":(exclude)contrib/guix/patches", ":(exclude)src/util/subprocess.hpp", ":(exclude)src/wallet/bip39_english.h"] | ||
| FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/dashbls/", ":(exclude)src/crypto/", ":(exclude)src/ctpl_stl.h", ":(exclude)src/cxxtimer.hpp", ":(exclude)src/immer/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)contrib/builder-keys/", ":(exclude)contrib/guix/patches", ":(exclude)src/util/subprocess.hpp", ":(exclude)src/wallet/bip39_english.h"] | ||
| FILES_ARGS += [f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/lint/lint-spelling.py | head -25Repository: dashpay/dash
Length of output: 1481
🏁 Script executed:
find . -maxdepth 2 -type f \( -name "ruff.toml" -o -name "pyproject.toml" -o -name ".ruff.toml" -o -name "setup.cfg" \) | head -10Repository: dashpay/dash
Length of output: 68
🏁 Script executed:
cat ./ruff.tomlRepository: dashpay/dash
Length of output: 1171
Rename loop variable dir to avoid shadowing Python built-in.
Line 18 uses dir as a loop variable, which shadows the built-in dir() function and triggers Ruff A001. This will fail lint CI since the project's ruff.toml enables the A rule (flake8-builtins) without suppressing this check.
Proposed fix
-FILES_ARGS += [f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES]
+FILES_ARGS += [f":(exclude){excluded_dir}" for excluded_dir in SHARED_EXCLUDED_SUBTREES]📝 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.
| FILES_ARGS += [f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES] | |
| FILES_ARGS += [f":(exclude){excluded_dir}" for excluded_dir in SHARED_EXCLUDED_SUBTREES] |
🧰 Tools
🪛 Ruff (0.15.12)
[error] 18-18: Variable dir is shadowing a Python builtin
(A001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/lint/lint-spelling.py` at line 18, The list comprehension uses the loop
variable named dir which shadows the built-in dir() and triggers Ruff A001;
update the variable name in the expression that builds FILES_ARGS (the
comprehension referencing SHARED_EXCLUDED_SUBTREES) to a non-built-in name like
subtree or subdir so it no longer shadows the built-in (e.g., replace
occurrences of dir inside the comprehension with subtree).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Validated all findings against source. One blocking merge-resolution defect in blockstorage.cpp (stale return error(...) plus unreachable return false) and five LogError calls in validation.cpp that lost their trailing \n during the bitcoin#29236 conversion. Two informational backport-prerequisite notes (partial adaptations of bitcoin#28857 and bitcoin#29236) confirmed against current Dash code.
Reviewed commit: 11dde62
🔴 1 blocking | 🟡 5 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
💬 nitpick: bitcoin#29236 only partially adopted: Dash retains its own error() template
src/util/system.h (lines 52-57)
Verified: bitcoin#29236 upstream removed the error(...) template entirely. Dash applied the scripted-diff converting many error(...) callers to LogError(...) + return false, but kept the error() template at src/util/system.h:53. Several Dash-specific call sites still rely on it (e.g., src/node/blockstorage.cpp:776,784,791,807; src/util/system.cpp:120; src/validation.cpp:2039; src/evo/specialtxman.cpp:593). The retained template wraps LogPrintf rather than LogError, so log severity for those sites is Info rather than Error. Additionally, bitcoin#29633 updated logging.h's leading comment to drop the explicit error reference; this leaves the comment slightly out of sync with Dash's actual state where error() still exists. Cosmetic / informational only — flag for tracking the eventual full removal.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/node/blockstorage.cpp`:
- [BLOCKING] lines 806-810: Stale return error(...) with unreachable return false; bitcoin#29236 conversion missed this site
Confirmed at src/node/blockstorage.cpp:806-810:
```cpp
if (*hash != pindex->GetBlockHash()) {
return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
pindex->ToString(), block_pos.ToString());
return false;
}
Two problems: (1) error(...) already returns false, so the trailing return false; is unreachable — this can trip the configured -Wunreachable-code diagnostics when warnings are promoted to errors. (2) The point of the bitcoin#29236 backport in this PR is precisely to nuke error() from these call sites; every other hunk in the diff converts to LogError(...\n); return false;. This single site was missed during merge resolution.
Functionally the function still returns false on hash mismatch, so consensus is unaffected, but the merge resolution is inconsistent and risks breaking warning-as-error builds.
In src/validation.cpp:
- [SUGGESTION] line 4781: LogError missing trailing \n in RollforwardBlock
Confirmed: line ends with...ToString());and no\nin the format string.LogErrorexpands toLogPrintLevel_→LogPrintf_→tfm::format(fmt, args...)and writes the result verbatim viaLogPrintStr. The Logger uses the trailing\nto drivem_started_new_line(logging.cpp:481), which controls whether the next entry gets its timestamp/source-location/threadname prefix. Without a trailing\nthe next log line gets concatenated onto the same line and loses its prefix. Every other LogError introduced by this PR (e.g., lines 4690, 4755, 4897, 4909, 4916) ends in\n; this one was missed. - [SUGGESTION] line 4924: LogError missing trailing \n on Dash-specific EvoDB inconsistency message
Same root cause as the line 4781 finding: this Dash-specific call (note the(DASH):tag) was converted fromerror(...)but lost the newline. The very next LogError call at line 4936 (also Dash-specific) has the same defect, as do 4942 and 5126. - [SUGGESTION] line 4936: LogError missing trailing \n in ReplayBlocks ReadBlockFromDisk failure
Same missing-newline issue as 4781/4924. Format string ends with...ToString())without\n. - [SUGGESTION] line 4942: LogError missing trailing \n in ReplayBlocks DisconnectBlock failure
Same missing-newline issue. - [SUGGESTION] line 5126: LogError missing trailing \n in LoadGenesisBlock catch handler
Same missing-newline issue in theLoadGenesisBlockexception handler.
</details>
| if (*hash != pindex->GetBlockHash()) { | ||
| return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s", | ||
| pindex->ToString(), block_pos.ToString()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Stale return error(...) with unreachable return false; bitcoin#29236 conversion missed this site
Confirmed at src/node/blockstorage.cpp:806-810:
if (*hash != pindex->GetBlockHash()) {
return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
pindex->ToString(), block_pos.ToString());
return false;
}Two problems: (1) error(...) already returns false, so the trailing return false; is unreachable — this can trip the configured -Wunreachable-code diagnostics when warnings are promoted to errors. (2) The point of the bitcoin#29236 backport in this PR is precisely to nuke error() from these call sites; every other hunk in the diff converts to LogError(...\n); return false;. This single site was missed during merge resolution.
Functionally the function still returns false on hash mismatch, so consensus is unaffected, but the merge resolution is inconsistent and risks breaking warning-as-error builds.
💡 Suggested change
| if (*hash != pindex->GetBlockHash()) { | |
| return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s", | |
| pindex->ToString(), block_pos.ToString()); | |
| return false; | |
| } | |
| if (*hash != pindex->GetBlockHash()) { | |
| LogError("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s\n", | |
| pindex->ToString(), block_pos.ToString()); | |
| return false; | |
| } |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/node/blockstorage.cpp`:
- [BLOCKING] lines 806-810: Stale return error(...) with unreachable return false; bitcoin#29236 conversion missed this site
Confirmed at src/node/blockstorage.cpp:806-810:
```cpp
if (*hash != pindex->GetBlockHash()) {
return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
pindex->ToString(), block_pos.ToString());
return false;
}
Two problems: (1) error(...) already returns false, so the trailing return false; is unreachable — this can trip the configured -Wunreachable-code diagnostics when warnings are promoted to errors. (2) The point of the bitcoin#29236 backport in this PR is precisely to nuke error() from these call sites; every other hunk in the diff converts to LogError(...\n); return false;. This single site was missed during merge resolution.
Functionally the function still returns false on hash mismatch, so consensus is unaffected, but the merge resolution is inconsistent and risks breaking warning-as-error builds.
</details>
| CBlock block; | ||
| if (!ReadBlockFromDisk(block, pindex, m_params.GetConsensus())) { | ||
| return error("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); | ||
| LogError("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); |
There was a problem hiding this comment.
🟡 Suggestion: LogError missing trailing \n in RollforwardBlock
Confirmed: line ends with ...ToString()); and no \n in the format string. LogError expands to LogPrintLevel_ → LogPrintf_ → tfm::format(fmt, args...) and writes the result verbatim via LogPrintStr. The Logger uses the trailing \n to drive m_started_new_line (logging.cpp:481), which controls whether the next entry gets its timestamp/source-location/threadname prefix. Without a trailing \n the next log line gets concatenated onto the same line and loses its prefix. Every other LogError introduced by this PR (e.g., lines 4690, 4755, 4897, 4909, 4916) ends in \n; this one was missed.
💡 Suggested change
| LogError("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); | |
| LogError("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.cpp`:
- [SUGGESTION] line 4781: LogError missing trailing \n in RollforwardBlock
Confirmed: line ends with `...ToString());` and no `\n` in the format string. `LogError` expands to `LogPrintLevel_` → `LogPrintf_` → `tfm::format(fmt, args...)` and writes the result verbatim via `LogPrintStr`. The Logger uses the trailing `\n` to drive `m_started_new_line` (logging.cpp:481), which controls whether the next entry gets its timestamp/source-location/threadname prefix. Without a trailing `\n` the next log line gets concatenated onto the same line and loses its prefix. Every other LogError introduced by this PR (e.g., lines 4690, 4755, 4897, 4909, 4916) ends in `\n`; this one was missed.
| const bool fDIP0003Active = DeploymentActiveAt(*pindexOld, m_params.GetConsensus(), Consensus::DEPLOYMENT_DIP0003); | ||
| if (fDIP0003Active && !m_evoDb.VerifyBestBlock(pindexOld->GetBlockHash())) { | ||
| return error("ReplayBlocks(DASH): Found EvoDB inconsistency"); | ||
| LogError("ReplayBlocks(DASH): Found EvoDB inconsistency"); |
There was a problem hiding this comment.
🟡 Suggestion: LogError missing trailing \n on Dash-specific EvoDB inconsistency message
Same root cause as the line 4781 finding: this Dash-specific call (note the (DASH): tag) was converted from error(...) but lost the newline. The very next LogError call at line 4936 (also Dash-specific) has the same defect, as do 4942 and 5126.
💡 Suggested change
| LogError("ReplayBlocks(DASH): Found EvoDB inconsistency"); | |
| LogError("ReplayBlocks(DASH): Found EvoDB inconsistency\n"); |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.cpp`:
- [SUGGESTION] line 4924: LogError missing trailing \n on Dash-specific EvoDB inconsistency message
Same root cause as the line 4781 finding: this Dash-specific call (note the `(DASH):` tag) was converted from `error(...)` but lost the newline. The very next LogError call at line 4936 (also Dash-specific) has the same defect, as do 4942 and 5126.
| CBlock block; | ||
| if (!ReadBlockFromDisk(block, pindexOld, m_params.GetConsensus())) { | ||
| return error("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); | ||
| LogError("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); |
There was a problem hiding this comment.
🟡 Suggestion: LogError missing trailing \n in ReplayBlocks ReadBlockFromDisk failure
Same missing-newline issue as 4781/4924. Format string ends with ...ToString()) without \n.
💡 Suggested change
| LogError("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); | |
| LogError("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s\n", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.cpp`:
- [SUGGESTION] line 4936: LogError missing trailing \n in ReplayBlocks ReadBlockFromDisk failure
Same missing-newline issue as 4781/4924. Format string ends with `...ToString())` without `\n`.
| DisconnectResult res = DisconnectBlock(block, pindexOld, cache); | ||
| if (res == DISCONNECT_FAILED) { | ||
| return error("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); | ||
| LogError("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); |
There was a problem hiding this comment.
🟡 Suggestion: LogError missing trailing \n in ReplayBlocks DisconnectBlock failure
Same missing-newline issue.
💡 Suggested change
| LogError("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); | |
| LogError("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s\n", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.cpp`:
- [SUGGESTION] line 4942: LogError missing trailing \n in ReplayBlocks DisconnectBlock failure
Same missing-newline issue.
| } | ||
| } catch (const std::runtime_error &e) { | ||
| return error("%s: failed to initialize block database: %s", __func__, e.what()); | ||
| LogError("%s: failed to initialize block database: %s", __func__, e.what()); |
There was a problem hiding this comment.
🟡 Suggestion: LogError missing trailing \n in LoadGenesisBlock catch handler
Same missing-newline issue in the LoadGenesisBlock exception handler.
💡 Suggested change
| LogError("%s: failed to initialize block database: %s", __func__, e.what()); | |
| LogError("%s: failed to initialize block database: %s\n", __func__, e.what()); |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.cpp`:
- [SUGGESTION] line 5126: LogError missing trailing \n in LoadGenesisBlock catch handler
Same missing-newline issue in the `LoadGenesisBlock` exception handler.
| @@ -178,6 +180,16 @@ BOOST_AUTO_TEST_CASE(vector_bool) | |||
| BOOST_CHECK(SerializeHash(vec1) == SerializeHash(vec2)); | |||
| } | |||
|
|
|||
| BOOST_AUTO_TEST_CASE(array) | |||
| { | |||
| std::array<uint8_t, 32> array1{1, 0, 0, 1, 1, 1, 0, 0, 0, 0, 1, 0, 0, 1, 1, 0, 0, 0, 1, 1, 1, 1, 0, 1, 0, 0, 1}; | |||
| DataStream ds; | |||
| ds << array1; | |||
| std::array<uint8_t, 32> array2; | |||
| ds >> array2; | |||
| BOOST_CHECK(array1 == array2); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
💬 Nitpick: bitcoin#28857 cherry-pick is the test commit only; refactor commit deferred until MessageStartChars→std::array conversion is backported
Verified: bitcoin#28857 upstream had a refactor commit (d49d198) initializing m_magic_bytes via the constructor initializer list using std::array<uint8_t, 4>, and a test commit (1e5b861) adding std::array serialization tests. The Dash cherry-pick (1040ad0) only includes the test piece. src/protocol.h:36 still defines typedef unsigned char MessageStartChars[MESSAGE_START_SIZE]; (a C array), so the upstream constructor-initializer form would not compile here. This is non-blocking — the cherry-pick does not introduce broken code — but the refactor portion of bitcoin#28857 is still pending and will need a follow-up after the upstream MessageStartChars→std::array conversion is backported into Dash.
source: ['claude']
There was a problem hiding this comment.
28857: yes, changes in src/net.cpp, src/net.h are missing; please fix it
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative re-review at 11dde62. One blocking merge-resolution defect at src/node/blockstorage.cpp:806-810 (stale return error(...) with unreachable return false; — the only converted site that still uses the old error() helper, inconsistent with the rest of the bitcoin#29236 conversion and risks tripping warning-as-error). Five LogError sites in src/validation.cpp lost their trailing \n during the conversion (4781, 4924, 4936, 4942, 5126); confirmed against logging.cpp:481 where m_started_new_line depends on the terminator. Two informational notes: bitcoin#28857 cherry-pick is test-only because the MessageStartChars→std::array prerequisite (bitcoin#28423) is not yet backported, and bitcoin#29236 is only partially adopted (Dash retains its error() template at src/util/system.h with several call sites still using it).
Reviewed commit: 11dde62
Fresh dispatcher run for this queue item. A same-SHA review already existed, so this records the fresh verification without duplicating inline threads.
Code Review
Reviewed commit: 11dde62
🔴 1 blocking | 🟡 5 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/node/blockstorage.cpp`:
- [BLOCKING] lines 806-810: Missed bitcoin#29236 conversion: stale `return error(...)` with unreachable `return false;`
This site was not adapted during the bitcoin#29236 backport. `error(...)` already returns `false`, so the trailing `return false;` is unreachable and can trip `-Wunreachable-code` on warning-as-error builds. It is also the only call site in this PR's diff that still uses the old `error()` helper instead of the new `LogError(...) + return false` pattern, making the merge resolution inconsistent. Functionality is preserved (the function still returns false on hash mismatch), but the inconsistency should be fixed before merge.
In `src/validation.cpp`:
- [SUGGESTION] line 4781: LogError missing trailing `\n` in RollforwardBlock
`LogError` expands to `LogPrintLevel_` and writes the formatted string verbatim via `LogPrintStr`. The logger uses the trailing `\n` to set `m_started_new_line` (logging.cpp:481), which controls whether the next entry receives its timestamp/source-location/threadname prefix. Without the terminator, the next log line is concatenated onto this one and loses its prefix. Every other LogError introduced by this PR (lines 4690, 4755, 4789, 4860, 4865, 4872, 4879, 4897, 4909, 4916) ends in `\n`; this one was missed during the conversion.
- [SUGGESTION] line 4924: LogError missing trailing `\n` on Dash-specific EvoDB inconsistency log
Same root cause as line 4781: this Dash-specific call (note the `(DASH):` tag) was converted from `error(...)` but lost its newline. The next log entry will be glued onto this one without its usual prefix.
- [SUGGESTION] line 4936: LogError missing trailing `\n` in ReplayBlocks ReadBlockFromDisk failure
Same missing-newline regression as 4781/4924. Format string ends with `...ToString())` with no `\n`, so the next log line loses its prefix.
- [SUGGESTION] line 4942: LogError missing trailing `\n` in ReplayBlocks DisconnectBlock failure
Same missing-newline regression. Without the terminator, subsequent log records are concatenated and lose their normal metadata prefixing — particularly unhelpful when diagnosing replay/recovery failures.
- [SUGGESTION] line 5126: LogError missing trailing `\n` in LoadGenesisBlock catch handler
Same missing-newline regression in the LoadGenesisBlock exception handler. Startup-time failures here are exactly the cases where preserving the prefix on the next log line matters for diagnosis.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/lint/lint-spelling.py (1)
18-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename loop variable
dirto avoid shadowing Python built-in.Line 18 uses
diras a loop variable, which shadows the built-indir()function and triggers Ruff A001. This will fail lint CI checks.🔧 Proposed fix
-FILES_ARGS += [f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES] +FILES_ARGS += [f":(exclude){subtree}" for subtree in SHARED_EXCLUDED_SUBTREES]🤖 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 `@test/lint/lint-spelling.py` at line 18, The comprehension uses the loop variable named dir which shadows the built-in dir() and causes lint A001; rename that variable (for example to subdir or excl) in the expression that builds FILES_ARGS so the line reads FILES_ARGS += [f":(exclude){subdir}" for subdir in SHARED_EXCLUDED_SUBTREES], updating any related references to use the new name and leaving FILES_ARGS and SHARED_EXCLUDED_SUBTREES unchanged.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@test/lint/lint_ignore_dirs.py`:
- Around line 1-5: The list assignment for SHARED_EXCLUDED_SUBTREES has
misaligned continuation lines causing Flake8 E128/E124; fix it by consistently
aligning the items and the closing bracket (either put all items on the same
line or place each string on its own line indented to the same column under the
opening bracket), e.g. align each entry under the first list element and ensure
the closing ] lines up with the start of the assignment so
SHARED_EXCLUDED_SUBTREES is properly formatted.
In `@test/lint/lint-includes.py`:
- Around line 20-26: EXCLUDED_DIRS currently hardcodes directories that are also
present in SHARED_EXCLUDED_SUBTREES causing duplicates; update the EXCLUDED_DIRS
definition to avoid repeating entries by either removing the overlapping
hardcoded entries (leave only the unique ones plus SHARED_EXCLUDED_SUBTREES) or
build EXCLUDED_DIRS by concatenating and then deduplicating while preserving
order (e.g., use an ordered set/dedup logic) so that EXCLUDED_DIRS and
SHARED_EXCLUDED_SUBTREES do not contain duplicated paths; target the
EXCLUDED_DIRS symbol in the diff to implement the change.
---
Duplicate comments:
In `@test/lint/lint-spelling.py`:
- Line 18: The comprehension uses the loop variable named dir which shadows the
built-in dir() and causes lint A001; rename that variable (for example to subdir
or excl) in the expression that builds FILES_ARGS so the line reads FILES_ARGS
+= [f":(exclude){subdir}" for subdir in SHARED_EXCLUDED_SUBTREES], updating any
related references to use the new name and leaving FILES_ARGS and
SHARED_EXCLUDED_SUBTREES unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 977db31c-4dd7-4bd3-808c-c787f898b7a9
📒 Files selected for processing (9)
share/rpcauth/README.mdshare/rpcauth/rpcauth.pytest/functional/mempool_accept.pytest/functional/test_framework/script.pytest/lint/README.mdtest/lint/lint-include-guards.pytest/lint/lint-includes.pytest/lint/lint-spelling.pytest/lint/lint_ignore_dirs.py
✅ Files skipped from review due to trivial changes (2)
- share/rpcauth/README.md
- test/lint/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- share/rpcauth/rpcauth.py
- test/functional/mempool_accept.py
- test/functional/test_framework/script.py
| SHARED_EXCLUDED_SUBTREES = ["src/leveldb/", | ||
| "src/crc32c/", | ||
| "src/secp256k1/", | ||
| "src/minisketch/", | ||
| ] |
There was a problem hiding this comment.
Fix list indentation to satisfy Flake8 checks.
The continuation lines are not properly aligned, triggering E128 and E124 violations that will fail CI lint checks.
📐 Proposed fix
-SHARED_EXCLUDED_SUBTREES = ["src/leveldb/",
- "src/crc32c/",
- "src/secp256k1/",
- "src/minisketch/",
- ]
+SHARED_EXCLUDED_SUBTREES = [
+ "src/leveldb/",
+ "src/crc32c/",
+ "src/secp256k1/",
+ "src/minisketch/",
+]📝 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.
| SHARED_EXCLUDED_SUBTREES = ["src/leveldb/", | |
| "src/crc32c/", | |
| "src/secp256k1/", | |
| "src/minisketch/", | |
| ] | |
| SHARED_EXCLUDED_SUBTREES = [ | |
| "src/leveldb/", | |
| "src/crc32c/", | |
| "src/secp256k1/", | |
| "src/minisketch/", | |
| ] |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 2-2: continuation line under-indented for visual indent
(E128)
[error] 3-3: continuation line under-indented for visual indent
(E128)
[error] 4-4: continuation line under-indented for visual indent
(E128)
[error] 5-5: closing bracket does not match visual indentation
(E124)
🤖 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 `@test/lint/lint_ignore_dirs.py` around lines 1 - 5, The list assignment for
SHARED_EXCLUDED_SUBTREES has misaligned continuation lines causing Flake8
E128/E124; fix it by consistently aligning the items and the closing bracket
(either put all items on the same line or place each string on its own line
indented to the same column under the opening bracket), e.g. align each entry
under the first list element and ensure the closing ] lines up with the start of
the assignment so SHARED_EXCLUDED_SUBTREES is properly formatted.
There was a problem hiding this comment.
29479: let's add dash specific libraries to SHARED_EXCLUDED_SUBTREES too:
- src/immer
- src/crypto/x11/
- src/dashbls
There was a problem hiding this comment.
@knst Great suggestion — adding those Dash-specific third-party subtrees makes sense. Here's the combined fix with proper indentation and the three additional entries (I've added trailing slashes for consistency with the other paths):
-SHARED_EXCLUDED_SUBTREES = ["src/leveldb/",
- "src/crc32c/",
- "src/secp256k1/",
- "src/minisketch/",
- ]
+SHARED_EXCLUDED_SUBTREES = [
+ "src/leveldb/",
+ "src/crc32c/",
+ "src/secp256k1/",
+ "src/minisketch/",
+ "src/immer/",
+ "src/crypto/x11/",
+ "src/dashbls/",
+]✏️ Learnings added
Learnt from: knst
Repo: dashpay/dash PR: 7292
File: test/lint/lint_ignore_dirs.py:1-5
Timestamp: 2026-06-02T08:51:39.050Z
Learning: In the dashpay/dash repository, `SHARED_EXCLUDED_SUBTREES` in `test/lint/lint_ignore_dirs.py` should include both the upstream Bitcoin third-party subtrees (`src/leveldb/`, `src/crc32c/`, `src/secp256k1/`, `src/minisketch/`) and Dash-specific third-party subtrees (`src/immer/`, `src/crypto/x11/`, `src/dashbls/`). These are consumed by lint scripts (lint-include-guards.py, lint-includes.py, lint-spelling.py) to exclude vendored/third-party code from linting.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: knst
Repo: dashpay/dash PR: 7104
File: test/lint/lint-circular-dependencies.py:38-46
Timestamp: 2026-01-15T08:53:09.064Z
Learning: In Python linting for circular dependencies, when refactoring changes module dependencies, update the EXPECTED_CIRCULAR_DEPENDENCIES entries to reflect the new dependency paths rather than removing them. The allowlist encodes the actual circular structure, so reorganizing modules should update the paths accordingly. Apply this guidance to Python test files under test/lint (and subdirectories) to ensure the lint configuration remains accurate and complete.
There was a problem hiding this comment.
added dash specific libraries to SHARED_EXCLUDED_SUBTREES
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
On 16c7f15bcd22f9fa1a71b2d64499b461078734e7, the concrete issues are limited to logging regressions introduced by the error(...) to LogError(...) conversion and one leftover dead-code artifact in ReadBlockFromDisk. The broader backport-prerequisite notes about bitcoin#28857 and bitcoin#29236 are accurate as repository history observations, but they are not actionable defects in this checked-out PR state and should not be posted as review findings.
Correction: this supersedes my immediately previous parser-error review body for this same run; the verified findings below are the intended review output.
Reviewed commit: 16c7f15
🟡 3 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.cpp`:
- [SUGGESTION] line 4781: `RollforwardBlock` leaves the log line unterminated after the `LogError` conversion
`LogError` does not append a newline on its own. In `Logger::LogPrintStr`, `m_started_new_line` is only reset when the format string ends with `\n`, so this message leaves the logger in the middle of a line and the next record is emitted without its normal timestamp/category prefix. That makes block-replay failures materially harder to diagnose.
- [SUGGESTION] lines 4924-4942: `ReplayBlocks` has three unterminated `LogError` messages
All three `LogError` calls added in this `ReplayBlocks` path omit a trailing newline. Because the logger prefixes only the start of a new line, any later log entry after one of these failures will be concatenated onto the same physical line and lose its normal metadata prefix. This affects EvoDB inconsistency reporting, rollback disk-read failures, and disconnect failures.
- [SUGGESTION] line 5126: `LoadGenesisBlock` logs startup failure without terminating the record
The `catch` block was converted from `error(...)` to `LogError(...)`, but the newline was dropped. If genesis-block initialization throws, the next log record is appended to the same line and loses its usual prefix, which degrades exactly the startup diagnostics this path is supposed to preserve.
| CBlock block; | ||
| if (!ReadBlockFromDisk(block, pindex, m_params.GetConsensus())) { | ||
| return error("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); | ||
| LogError("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); |
There was a problem hiding this comment.
🟡 Suggestion: RollforwardBlock leaves the log line unterminated after the LogError conversion
LogError does not append a newline on its own. In Logger::LogPrintStr, m_started_new_line is only reset when the format string ends with \n, so this message leaves the logger in the middle of a line and the next record is emitted without its normal timestamp/category prefix. That makes block-replay failures materially harder to diagnose.
💡 Suggested change
| LogError("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); | |
| LogError("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); |
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.cpp`:
- [SUGGESTION] line 4781: `RollforwardBlock` leaves the log line unterminated after the `LogError` conversion
`LogError` does not append a newline on its own. In `Logger::LogPrintStr`, `m_started_new_line` is only reset when the format string ends with `\n`, so this message leaves the logger in the middle of a line and the next record is emitted without its normal timestamp/category prefix. That makes block-replay failures materially harder to diagnose.
| @@ -4892,12 +4933,14 @@ bool CChainState::ReplayBlocks() | |||
| if (pindexOld->nHeight > 0) { // Never disconnect the genesis block. | |||
| CBlock block; | |||
| if (!ReadBlockFromDisk(block, pindexOld, m_params.GetConsensus())) { | |||
| return error("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); | |||
| LogError("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); | |||
| return false; | |||
| } | |||
| LogPrintf("Rolling back %s (%i)\n", pindexOld->GetBlockHash().ToString(), pindexOld->nHeight); | |||
| DisconnectResult res = DisconnectBlock(block, pindexOld, cache); | |||
| if (res == DISCONNECT_FAILED) { | |||
| return error("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); | |||
| LogError("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); | |||
There was a problem hiding this comment.
🟡 Suggestion: ReplayBlocks has three unterminated LogError messages
All three LogError calls added in this ReplayBlocks path omit a trailing newline. Because the logger prefixes only the start of a new line, any later log entry after one of these failures will be concatenated onto the same physical line and lose its normal metadata prefix. This affects EvoDB inconsistency reporting, rollback disk-read failures, and disconnect failures.
💡 Suggested change
| if (fDIP0003Active && !m_evoDb.VerifyBestBlock(pindexOld->GetBlockHash())) { | |
| LogError("ReplayBlocks(DASH): Found EvoDB inconsistency\n"); | |
| return false; | |
| } | |
| } | |
| auto dbTx = m_evoDb.BeginTransaction(); | |
| // Rollback along the old branch. | |
| while (pindexOld != pindexFork) { | |
| if (pindexOld->nHeight > 0) { // Never disconnect the genesis block. | |
| CBlock block; | |
| if (!ReadBlockFromDisk(block, pindexOld, m_params.GetConsensus())) { | |
| LogError("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s\n", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); | |
| return false; | |
| } | |
| LogPrintf("Rolling back %s (%i)\n", pindexOld->GetBlockHash().ToString(), pindexOld->nHeight); | |
| DisconnectResult res = DisconnectBlock(block, pindexOld, cache); | |
| if (res == DISCONNECT_FAILED) { | |
| LogError("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s\n", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); | |
| return false; | |
| } |
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.cpp`:
- [SUGGESTION] lines 4924-4942: `ReplayBlocks` has three unterminated `LogError` messages
All three `LogError` calls added in this `ReplayBlocks` path omit a trailing newline. Because the logger prefixes only the start of a new line, any later log entry after one of these failures will be concatenated onto the same physical line and lose its normal metadata prefix. This affects EvoDB inconsistency reporting, rollback disk-read failures, and disconnect failures.
| } | ||
| } catch (const std::runtime_error &e) { | ||
| return error("%s: failed to initialize block database: %s", __func__, e.what()); | ||
| LogError("%s: failed to initialize block database: %s", __func__, e.what()); |
There was a problem hiding this comment.
🟡 Suggestion: LoadGenesisBlock logs startup failure without terminating the record
The catch block was converted from error(...) to LogError(...), but the newline was dropped. If genesis-block initialization throws, the next log record is appended to the same line and loses its usual prefix, which degrades exactly the startup diagnostics this path is supposed to preserve.
💡 Suggested change
| LogError("%s: failed to initialize block database: %s", __func__, e.what()); | |
| LogError("%s: failed to initialize block database: %s\n", __func__, e.what()); |
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.cpp`:
- [SUGGESTION] line 5126: `LoadGenesisBlock` logs startup failure without terminating the record
The `catch` block was converted from `error(...)` to `LogError(...)`, but the newline was dropped. If genesis-block initialization throws, the next log record is appended to the same line and loses its usual prefix, which degrades exactly the startup diagnostics this path is supposed to preserve.
| if (*hash != pindex->GetBlockHash()) { | ||
| return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s", | ||
| pindex->ToString(), block_pos.ToString()); | ||
| return false; |
There was a problem hiding this comment.
💬 Nitpick: ReadBlockFromDisk still contains a dead return false; after return error(...)
This branch still uses the old error(...) helper, which already logs and returns false. The following return false; is unreachable dead code. It does not change runtime behavior, but it is a leftover from the partial bitcoin#29236 conversion in a function touched by this PR and should be cleaned up rather than carried forward.
💡 Suggested change
| if (*hash != pindex->GetBlockHash()) { | |
| return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s", | |
| pindex->ToString(), block_pos.ToString()); | |
| return false; | |
| LogError("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s\n", | |
| pindex->ToString(), block_pos.ToString()); | |
| return false; |
source: ['codex']
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
changes required: 28857, 29236, 29479
looks good to me: 29433, 29615, 29459, 29458
| @@ -178,6 +180,16 @@ BOOST_AUTO_TEST_CASE(vector_bool) | |||
| BOOST_CHECK(SerializeHash(vec1) == SerializeHash(vec2)); | |||
| } | |||
|
|
|||
| BOOST_AUTO_TEST_CASE(array) | |||
| { | |||
| std::array<uint8_t, 32> array1{1, 0, 0, 1, 1, 1, 0, 0, 0, 0, 1, 0, 0, 1, 1, 0, 0, 0, 1, 1, 1, 1, 0, 1, 0, 0, 1}; | |||
| DataStream ds; | |||
| ds << array1; | |||
| std::array<uint8_t, 32> array2; | |||
| ds >> array2; | |||
| BOOST_CHECK(array1 == array2); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
28857: yes, changes in src/net.cpp, src/net.h are missing; please fix it
| stream << hashwriter.GetHash(); | ||
| } catch (const std::exception& e) { | ||
| return error("%s: Serialize or I/O error - %s", __func__, e.what()); | ||
| LogError("%s: Serialize or I/O error - %s\n", __func__, e.what()); |
There was a problem hiding this comment.
29236: mark as partial, because instances of error still exist and definition is not removed from logging.h
| } | ||
| } catch (const std::runtime_error &e) { | ||
| return error("%s: failed to initialize block database: %s", __func__, e.what()); | ||
| LogError("%s: failed to initialize block database: %s", __func__, e.what()); |
| DisconnectResult res = DisconnectBlock(block, pindexOld, cache); | ||
| if (res == DISCONNECT_FAILED) { | ||
| return error("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); | ||
| LogError("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString()); |
| SHARED_EXCLUDED_SUBTREES = ["src/leveldb/", | ||
| "src/crc32c/", | ||
| "src/secp256k1/", | ||
| "src/minisketch/", | ||
| ] |
There was a problem hiding this comment.
29479: let's add dash specific libraries to SHARED_EXCLUDED_SUBTREES too:
- src/immer
- src/crypto/x11/
- src/dashbls
|
This pull request has conflicts, please rebase. |
1e5b861 test: Add test for array serialization (TheCharlatan) d49d198 refactor: Initialize magic bytes in constructor initializer (TheCharlatan) Pull request description: This is a followup-PR for bitcoin#28423 * Initialize magic bytes in constructor * Add a small unit test for serializing arrays. ACKs for top commit: sipa: utACK 1e5b861 maflcko: lgtm ACK 1e5b861 Tree-SHA512: 0f58d2332dc501ca9fd419f40ed4f977c83dce0169e9a0eee1ffc9f8daa2d2ef7e7df18205ba076f55d90ae6c4a20d2b51ab303150d38470a962bcc58a66f6e7
…void resizing a19235c Preallocate result in `TryParseHex` to avoid resizing (Lőrinc) b7489ec Add benchmark for TryParseHex (Lőrinc) Pull request description: This pull request introduces optimizations to the `TryParseHex` function, focusing primarily on the ideal case (valid hexadecimal input without spaces). A new benchmark, `HexParse` was introduced in a separate commit. The main optimization preallocates the result vector based on the input string's length. This aims to completely avoid costly dynamic reallocations when no spaces are present. ------------ Before: ``` | ns/base16 | base16/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 1.60 | 623,238,893.11 | 0.3% | 0.01 | `HexParse` | 1.65 | 606,747,566.34 | 0.6% | 0.01 | `HexParse` | 1.60 | 626,149,544.07 | 0.3% | 0.01 | `HexParse` ``` After: ``` | ns/base16 | base16/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 0.68 | 1,465,555,976.27 | 0.8% | 0.01 | `HexParse` | 0.68 | 1,472,962,920.18 | 0.3% | 0.01 | `HexParse` | 0.68 | 1,476,159,423.00 | 0.3% | 0.01 | `HexParse` ``` ACKs for top commit: achow101: ACK a19235c hebasto: ACK a19235c. andrewtoth: Re-ACK a19235c Empact: Re-ACK bitcoin@a19235c Tree-SHA512: e09a59791104be3fd1026862ce98de9efafa1f949626fa01e3b7d58e6a2ef02a11f0de55ddba5c43230a53effd24e6d368c1e12848b17e8ce91d7908a59333f0
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66b5782fe9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "src/crypto/x11/" | ||
| "src/dashbls" |
There was a problem hiding this comment.
Python concatenates these adjacent string literals into src/crypto/x11/src/dashbls, so neither src/crypto/x11/ nor src/dashbls/ is excluded. As a result, test/lint/lint-includes.py scans both vendored subtrees and fails with their existing duplicate and quote-style includes; add a comma between the entries.
Useful? React with 👍 / 👎.
fa39151 refactor: Remove unused error() (MarcoFalke) fad0335 scripted-diff: Replace error() with LogError() (MarcoFalke) fa808fb refactor: Make error() return type void (MarcoFalke) fa1d624 scripted-diff: return error(...); ==> error(...); return false; (MarcoFalke) fa9a5e8 refactor: Add missing {} around error() calls (MarcoFalke) Pull request description: `error(...)` has many issues: * It is often used in the context of `return error(...)`, implying that it has a "fancy" type, creating confusion with `util::Result/Error` * `-logsourcelocations` does not work with it, because it will pretend the error happened inside of `logging.h` * The log line contains `ERROR: `, as opposed to `[error]`, like for other errors logged with `LogError`. Fix all issues by removing it. ACKs for top commit: fjahr: re-utACK fa39151 stickies-v: re-ACK fa39151, no changes since 4a90374 ryanofsky: Code review ACK fa39151. Just rebase since last review Tree-SHA512: ec5bb502ab0d3733fdb14a8a00762638fce0417afd8dd6294ae0d485ce2b7ca5b1efeb50fc2cd7467f6c652e4ed3e99b0f283b08aeca04bbfb7ea4f2c95d283a Rebase and fixing blockstorage.cpp file src/validation.cpp - Fixing linting error
bf264e0 test: check_mempool_result negative feerate (kevkevin) Pull request description: Adds test coverage in `mempool_accept.py` to check if a negative `maxfeerate` is input into `check_mempool_result` Asserts "Amount out of range" error message and `-3` error code Motivated by this [comment](https://github.com/bitcoin/bitcoin/pull/29434/files#r1491112250) ACKs for top commit: maflcko: lgtm ACK bf264e0 brunoerg: nice, utACK bf264e0 davidgumberg: Looks great, ACK bitcoin@bf264e0 Tree-SHA512: 58931b774cc887c616f2fd91af3ee65cc5db55acd8e2875c76de448c80bd4e020b057c5f4f85556431377f0d0e7553771fb285d1ec20cf64f64ec92a47776b78
vijaydasmp
changed the title
80fa7da test: Refactor subtree exclusion in lint tests (Brandon Odiwuor) Pull request description: Fixes bitcoin#17413 Refactor subtree exclusion in lint tests to one place Second attempt after PR: bitcoin#24435 ACKs for top commit: fjahr: re-ACK 80fa7da maflcko: lgtm ACK 80fa7da davidgumberg: ACK bitcoin@80fa7da Tree-SHA512: deff7457dd19ca5ea440d3d53feae047e8863b9ddeb6494a3c94605a5d16edc91db8f99a435b4fab2ef89aedee42439562be006da647fb85bbf3def903a3ce50
… add unit test 3e9c736 test: fix accurate multisig sigop count (BIP16), add unit test (Sebastian Falbesoner) Pull request description: In the course of reviewing bitcoin#29589 I noticed the following buggy call-site of `CScriptOp.decode_op_n` in the CScript's `GetSigOpCount` method: https://github.com/bitcoin/bitcoin/blob/4cc99df44aec4d104590aee46cf18318e22a8568/test/functional/test_framework/script.py#L591-L593 This should be `lastOpcode` rather than `opcode`. The latter is either OP_CHECKMULTISIG or OP_CHECKMULTISIGVERIFY at this point, so `decode_op_n` would result in an error. Also, in `CScript.raw_iter`, we have to return the op as `CScriptOp` type instead of a bare integer, otherwise we can't call the decode method on it. To prevent this in the future, add some simple unit tests for `GetSigOpCount`. Note that this was unnoticed, as the code part was never hit so far in the test framework. ACKs for top commit: achow101: ACK 3e9c736 Christewart: ACK 3e9c736 rkrux: tACK [3e9c736](bitcoin@3e9c736) hernanmarino: tACK 3e9c736 Tree-SHA512: 51647bb6d462fbd101effd851afdbd6ad198c0567888cd4fdcac389a9fb4bd3d7e648095c6944fd8875d36272107ebaabdc62d0e2423289055588c12294d05a7
…output text in json format 9adf949 contrib: rpcauth.py - Add new option (-j/--json) to output text in json format (bstin) Pull request description: This is a simple change to rpcauth.py utility in order to output as json instead raw text. This is beneficial because integrating json output is simpler with multiple different forms of automation and tooling ACKs for top commit: maflcko: ACK 9adf949 achow101: ACK 9adf949 willcl-ark: tACK 9adf949 tdb3: ACK for 9adf949 Tree-SHA512: 2cdc3b2071fbe4fb32a84ce42ee8ad216cff96ed82aaef58daeb3991953ac137ae42d6898a7fdb6cbd1800e1f61ff8d292f0b150eaebdd2a3fd9d37ed7450787
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/lint/lint-include-guards.py (1)
22-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicate entries already in SHARED_EXCLUDED_SUBTREES.
Lines 29-30 hardcode
'src/dashbls'and'src/immer', which are also present inSHARED_EXCLUDED_SUBTREES(once the critical syntax error inlint_ignore_dirs.pyis fixed). This creates redundant entries and undermines the centralization goal of this refactor.♻️ Proposed fix
EXCLUDE_FILES_WITH_PREFIX = ['src/crypto/ctaes', 'src/tinyformat.h', 'src/bench/nanobench.h', 'src/test/fuzz/FuzzedDataProvider.h', 'src/bls', 'src/crypto/x11/sph', 'src/ctpl_stl.h', - 'src/dashbls', 'src/gsl', - 'src/immer'] + SHARED_EXCLUDED_SUBTREES + ] + SHARED_EXCLUDED_SUBTREES🤖 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 `@test/lint/lint-include-guards.py` around lines 22 - 31, EXCLUDE_FILES_WITH_PREFIX currently duplicates entries already defined in SHARED_EXCLUDED_SUBTREES; update the list declared in EXCLUDE_FILES_WITH_PREFIX to remove 'src/dashbls' and 'src/immer' (or any other entries duplicated in SHARED_EXCLUDED_SUBTREES) so the central SHARED_EXCLUDED_SUBTREES remains the single source of truth; locate the EXCLUDE_FILES_WITH_PREFIX definition and delete the redundant string literals while leaving the concatenation with SHARED_EXCLUDED_SUBTREES intact.
♻️ Duplicate comments (2)
test/lint/lint-spelling.py (1)
18-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRename loop variable to avoid shadowing built-in.
The variable name
dirshadows Python's built-indir()function, triggering Ruff A001. This will fail CI since the project'sruff.tomlenables flake8-builtins checks.📝 Proposed fix
-FILES_ARGS += [f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES] +FILES_ARGS += [f":(exclude){excluded_dir}" for excluded_dir in SHARED_EXCLUDED_SUBTREES]🤖 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 `@test/lint/lint-spelling.py` at line 18, In test/lint/lint-spelling.py the list comprehension FILES_ARGS += [f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES] uses the loop variable name dir which shadows the built-in; rename that loop variable (e.g., to subdir or subtree) throughout the comprehension so it reads like [f":(exclude){subdir}" for subdir in SHARED_EXCLUDED_SUBTREES], leaving FILES_ARGS and SHARED_EXCLUDED_SUBTREES unchanged.Source: Linters/SAST tools
test/lint/lint-includes.py (1)
101-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRename loop variable to avoid shadowing built-in.
The variable name
dirshadows Python's built-indir()function, triggering Ruff A001. This will fail CI since the project'sruff.tomlenables flake8-builtins checks.📝 Proposed fix
- exclude_args = [":(exclude)" + dir for dir in SHARED_EXCLUDED_SUBTREES] + exclude_args = [":(exclude)" + excluded_dir for excluded_dir in SHARED_EXCLUDED_SUBTREES]🤖 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 `@test/lint/lint-includes.py` at line 101, The list comprehension uses the loop variable name `dir`, which shadows the built-in dir() and triggers Ruff A001; change the loop variable in the expression that constructs `exclude_args` (the comprehension using `SHARED_EXCLUDED_SUBTREES`) to a non-built-in name like `subtree` or `d` and update any references in that comprehension to use the new name so `exclude_args = [":(exclude)" + <new_name> for <new_name> in SHARED_EXCLUDED_SUBTREES]`.Source: Linters/SAST tools
🤖 Prompt for all review comments with 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.
Inline comments:
In `@test/lint/lint_ignore_dirs.py`:
- Around line 1-8: SHARED_EXCLUDED_SUBTREES contains adjacent string literals
that were accidentally concatenated because trailing commas are missing; add
commas after "src/immer", "src/crypto/x11/" and "src/dashbls" (or wherever the
list entries end) to ensure each entry is a separate string, fix the list
indentation to match surrounding style and normalize trailing slashes
consistently across entries (either include or remove trailing slashes for all
entries) so lint scripts iterating over SHARED_EXCLUDED_SUBTREES see the correct
individual paths.
In `@test/lint/lint-includes.py`:
- Line 43: The list comprehension assigning exclude_args shadows the built-in
dir() by using the loop variable name dir; rename that loop variable (e.g., to
subdir or subtree) in the expression exclude_args = [":(exclude)"+ subdir for
subdir in SHARED_EXCLUDED_SUBTREES] so the built-in is not shadowed and the Ruff
A001/flake8-builtins error is resolved.
---
Outside diff comments:
In `@test/lint/lint-include-guards.py`:
- Around line 22-31: EXCLUDE_FILES_WITH_PREFIX currently duplicates entries
already defined in SHARED_EXCLUDED_SUBTREES; update the list declared in
EXCLUDE_FILES_WITH_PREFIX to remove 'src/dashbls' and 'src/immer' (or any other
entries duplicated in SHARED_EXCLUDED_SUBTREES) so the central
SHARED_EXCLUDED_SUBTREES remains the single source of truth; locate the
EXCLUDE_FILES_WITH_PREFIX definition and delete the redundant string literals
while leaving the concatenation with SHARED_EXCLUDED_SUBTREES intact.
---
Duplicate comments:
In `@test/lint/lint-includes.py`:
- Line 101: The list comprehension uses the loop variable name `dir`, which
shadows the built-in dir() and triggers Ruff A001; change the loop variable in
the expression that constructs `exclude_args` (the comprehension using
`SHARED_EXCLUDED_SUBTREES`) to a non-built-in name like `subtree` or `d` and
update any references in that comprehension to use the new name so `exclude_args
= [":(exclude)" + <new_name> for <new_name> in SHARED_EXCLUDED_SUBTREES]`.
In `@test/lint/lint-spelling.py`:
- Line 18: In test/lint/lint-spelling.py the list comprehension FILES_ARGS +=
[f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES] uses the loop variable
name dir which shadows the built-in; rename that loop variable (e.g., to subdir
or subtree) throughout the comprehension so it reads like [f":(exclude){subdir}"
for subdir in SHARED_EXCLUDED_SUBTREES], leaving FILES_ARGS and
SHARED_EXCLUDED_SUBTREES unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6a12f43c-141b-480e-a898-16dbb2a82157
📒 Files selected for processing (26)
share/rpcauth/README.mdshare/rpcauth/rpcauth.pysrc/Makefile.bench.includesrc/addrdb.cppsrc/bench/parse_hex.cppsrc/flatfile.cppsrc/index/base.cppsrc/index/blockfilterindex.cppsrc/index/coinstatsindex.cppsrc/index/txindex.cppsrc/kernel/coinstats.cppsrc/net.cppsrc/net.hsrc/netbase.cppsrc/node/blockstorage.cppsrc/script/signingprovider.cppsrc/test/serialize_tests.cppsrc/util/strencodings.cppsrc/validation.cpptest/functional/mempool_accept.pytest/functional/test_framework/script.pytest/lint/README.mdtest/lint/lint-include-guards.pytest/lint/lint-includes.pytest/lint/lint-spelling.pytest/lint/lint_ignore_dirs.py
✅ Files skipped from review due to trivial changes (2)
- share/rpcauth/README.md
- src/util/strencodings.cpp
🚧 Files skipped from review as they are similar to previous changes (16)
- src/script/signingprovider.cpp
- test/functional/mempool_accept.py
- src/index/base.cpp
- test/lint/README.md
- src/addrdb.cpp
- src/test/serialize_tests.cpp
- src/Makefile.bench.include
- src/flatfile.cpp
- src/index/txindex.cpp
- src/index/coinstatsindex.cpp
- src/kernel/coinstats.cpp
- src/netbase.cpp
- src/index/blockfilterindex.cpp
- share/rpcauth/rpcauth.py
- test/functional/test_framework/script.py
- src/node/blockstorage.cpp
|
|
||
| def list_files_by_suffix(suffixes): | ||
| exclude_args = [":(exclude)" + dir for dir in EXCLUDED_DIRS] | ||
| exclude_args = [":(exclude)" + dir for dir in SHARED_EXCLUDED_SUBTREES] |
There was a problem hiding this comment.
Rename loop variable to avoid shadowing built-in.
The variable name dir shadows Python's built-in dir() function, triggering Ruff A001. This will fail CI since the project's ruff.toml enables flake8-builtins checks.
📝 Proposed fix
- exclude_args = [":(exclude)" + dir for dir in SHARED_EXCLUDED_SUBTREES]
+ exclude_args = [":(exclude)" + excluded_dir for excluded_dir in SHARED_EXCLUDED_SUBTREES]📝 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.
| exclude_args = [":(exclude)" + dir for dir in SHARED_EXCLUDED_SUBTREES] | |
| exclude_args = [":(exclude)" + excluded_dir for excluded_dir in SHARED_EXCLUDED_SUBTREES] |
🧰 Tools
🪛 Ruff (0.15.15)
[error] 43-43: Variable dir is shadowing a Python builtin
(A001)
🤖 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 `@test/lint/lint-includes.py` at line 43, The list comprehension assigning
exclude_args shadows the built-in dir() by using the loop variable name dir;
rename that loop variable (e.g., to subdir or subtree) in the expression
exclude_args = [":(exclude)"+ subdir for subdir in SHARED_EXCLUDED_SUBTREES] so
the built-in is not shadowed and the Ruff A001/flake8-builtins error is
resolved.
Source: Linters/SAST tools
There was a problem hiding this comment.
♻️ Duplicate comments (3)
test/lint/lint-spelling.py (1)
18-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid shadowing built-in
dir()in exclude-args comprehension.Line 18 triggers Ruff
A001and can fail lint CI.Proposed fix
-FILES_ARGS += [f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES] +FILES_ARGS += [f":(exclude){excluded_dir}" for excluded_dir in SHARED_EXCLUDED_SUBTREES]🤖 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 `@test/lint/lint-spelling.py` at line 18, The list comprehension FILES_ARGS += [f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES] shadows Python's built-in dir(), causing Ruff A001; rename the loop variable (e.g., use "subtree" or "d") and update the f-string to use that new name so the expression becomes something like [f":(exclude){subtree}" for subtree in SHARED_EXCLUDED_SUBTREES], leaving FILES_ARGS and SHARED_EXCLUDED_SUBTREES untouched.Source: Linters/SAST tools
test/lint/lint_ignore_dirs.py (1)
1-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix list indentation so lint passes.
Line 2 through Line 8 still violate Flake8 visual-indentation rules (E128/E124), which can fail lint CI.
Proposed fix
-SHARED_EXCLUDED_SUBTREES = ["src/leveldb/", - "src/crc32c/", - "src/secp256k1/", - "src/minisketch/", - "src/immer/", - "src/crypto/x11/", - "src/dashbls/", - ] +SHARED_EXCLUDED_SUBTREES = [ + "src/leveldb/", + "src/crc32c/", + "src/secp256k1/", + "src/minisketch/", + "src/immer/", + "src/crypto/x11/", + "src/dashbls/", +]🤖 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 `@test/lint/lint_ignore_dirs.py` around lines 1 - 8, The list literal SHARED_EXCLUDED_SUBTREES has inconsistent visual indentation causing Flake8 E124/E128 errors; reformat the list so all continuation lines are aligned either under the opening bracket or properly indented to a 4-space indent level (e.g., align items under the first quote after the = or indent each line by one additional level), ensuring commas remain after each string and the closing bracket lines up with the assignment to satisfy E124/E128 in the SHARED_EXCLUDED_SUBTREES definition.Source: Linters/SAST tools
test/lint/lint-includes.py (1)
43-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRename
dirloop variables to avoid built-in shadowing.Line 43 and Line 101 shadow Python’s built-in
dir()(A001), which is lint-failing.Proposed fix
- exclude_args = [":(exclude)" + dir for dir in SHARED_EXCLUDED_SUBTREES] + exclude_args = [":(exclude)" + excluded_dir for excluded_dir in SHARED_EXCLUDED_SUBTREES] ... - exclude_args = [":(exclude)" + dir for dir in SHARED_EXCLUDED_SUBTREES] + exclude_args = [":(exclude)" + excluded_dir for excluded_dir in SHARED_EXCLUDED_SUBTREES]Also applies to: 101-101
🤖 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 `@test/lint/lint-includes.py` at line 43, The list comprehension uses a loop variable named "dir" which shadows Python's built-in dir(); rename that variable in both occurrences (the comprehension creating exclude_args and the similar occurrence at the other location) to a non-built-in name such as "subtree" or "dirname" (e.g., change "for dir in SHARED_EXCLUDED_SUBTREES" to "for subtree in SHARED_EXCLUDED_SUBTREES") so the variable exclude_args = [":(exclude)"+subtree for subtree in SHARED_EXCLUDED_SUBTREES] (and the matching change at the other occurrence) removes the built-in shadowing.Source: Linters/SAST tools
🤖 Prompt for all review comments with 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.
Duplicate comments:
In `@test/lint/lint_ignore_dirs.py`:
- Around line 1-8: The list literal SHARED_EXCLUDED_SUBTREES has inconsistent
visual indentation causing Flake8 E124/E128 errors; reformat the list so all
continuation lines are aligned either under the opening bracket or properly
indented to a 4-space indent level (e.g., align items under the first quote
after the = or indent each line by one additional level), ensuring commas remain
after each string and the closing bracket lines up with the assignment to
satisfy E124/E128 in the SHARED_EXCLUDED_SUBTREES definition.
In `@test/lint/lint-includes.py`:
- Line 43: The list comprehension uses a loop variable named "dir" which shadows
Python's built-in dir(); rename that variable in both occurrences (the
comprehension creating exclude_args and the similar occurrence at the other
location) to a non-built-in name such as "subtree" or "dirname" (e.g., change
"for dir in SHARED_EXCLUDED_SUBTREES" to "for subtree in
SHARED_EXCLUDED_SUBTREES") so the variable exclude_args = [":(exclude)"+subtree
for subtree in SHARED_EXCLUDED_SUBTREES] (and the matching change at the other
occurrence) removes the built-in shadowing.
In `@test/lint/lint-spelling.py`:
- Line 18: The list comprehension FILES_ARGS += [f":(exclude){dir}" for dir in
SHARED_EXCLUDED_SUBTREES] shadows Python's built-in dir(), causing Ruff A001;
rename the loop variable (e.g., use "subtree" or "d") and update the f-string to
use that new name so the expression becomes something like
[f":(exclude){subtree}" for subtree in SHARED_EXCLUDED_SUBTREES], leaving
FILES_ARGS and SHARED_EXCLUDED_SUBTREES untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0303e601-a6e7-4c30-8856-f868ffd3627f
📒 Files selected for processing (21)
share/rpcauth/README.mdshare/rpcauth/rpcauth.pysrc/addrdb.cppsrc/flatfile.cppsrc/index/base.cppsrc/index/blockfilterindex.cppsrc/index/coinstatsindex.cppsrc/index/txindex.cppsrc/kernel/coinstats.cppsrc/net.cppsrc/netbase.cppsrc/node/blockstorage.cppsrc/script/signingprovider.cppsrc/validation.cpptest/functional/mempool_accept.pytest/functional/test_framework/script.pytest/lint/README.mdtest/lint/lint-include-guards.pytest/lint/lint-includes.pytest/lint/lint-spelling.pytest/lint/lint_ignore_dirs.py
✅ Files skipped from review due to trivial changes (2)
- share/rpcauth/README.md
- test/lint/README.md
🚧 Files skipped from review as they are similar to previous changes (15)
- test/functional/mempool_accept.py
- src/index/base.cpp
- src/kernel/coinstats.cpp
- src/net.cpp
- src/script/signingprovider.cpp
- src/index/blockfilterindex.cpp
- src/flatfile.cpp
- src/addrdb.cpp
- test/functional/test_framework/script.py
- src/node/blockstorage.cpp
- share/rpcauth/rpcauth.py
- src/netbase.cpp
- src/index/txindex.cpp
- src/index/coinstatsindex.cpp
- src/validation.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review at f1121b5. All four prior findings (RollforwardBlock/ReplayBlocks/LoadGenesisBlock LogError missing newlines, and the dead return false in ReadBlockFromDisk) are FIXED in this delta. One new BLOCKING finding from both agents (convergent): the bitcoin#28857 backport applied the upstream V1Transport member-initializer refactor without first applying the bitcoin#28423 prerequisite that converts MessageStartChars from a C array typedef to std::array. The current m_magic_bytes{Params().MessageStart()} brace-initializer is invalid C++ for a C-array member initialized from another C-array reference; reproduced as a hard compile error locally with clang++ -std=c++20.
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/net.cpp`:
- [BLOCKING] src/net.cpp:694-695: V1Transport ctor brace-init breaks build: bitcoin#28857 applied without bitcoin#28423 prerequisite
The bitcoin#28857 cherry-pick includes upstream commit d49d198840 which replaces the V1Transport ctor body with a member-initializer `m_magic_bytes{Params().MessageStart()}`. Upstream this is valid because bitcoin#28423 (commit 9be330b654) had already converted `MessageStartChars` from `typedef unsigned char MessageStartChars[MESSAGE_START_SIZE]` to `using MessageStartChars = std::array<uint8_t, 4>`. Dash has not backported bitcoin#28423: `src/protocol.h:36` still has the C-array typedef, `src/chainparams.h:85` returns `const MessageStartChars&` to that C array, and `src/net.h:410` declares `m_magic_bytes` as `const CMessageHeader::MessageStartChars` (i.e. `const unsigned char[4]`). Brace-initialization of a C-array aggregate from a single array-reference element is invalid C++: it is parsed as a one-element aggregate initializer whose element type `unsigned char` cannot be initialized from `const unsigned char(&)[4]`. Reproduced locally with clang++ -std=c++20: `error: cannot initialize an array element of type 'const unsigned char' with an lvalue of type 'const MessageStartChars' (aka 'const unsigned char[4]')`. The pre-PR Dash code correctly worked around the C-array limitation by using `std::copy(std::begin(Params().MessageStart()), std::end(Params().MessageStart()), m_magic_bytes)` in the constructor body. Fix options: (a) backport bitcoin#28423 first and then keep the refactor, (b) drop upstream commit d49d198840 from this cherry-pick and restore the `std::copy` body (and keep `m_magic_bytes` non-const in `src/net.h`), or (c) expand the brace-init to element-wise initialization as shown in the suggestion (compiles but loses the const intent only if the member type is kept; this works while keeping const).
| V1Transport::V1Transport(const NodeId node_id, int nTypeIn, int nVersionIn) noexcept : | ||
| m_node_id(node_id), hdrbuf(nTypeIn, nVersionIn), vRecv(nTypeIn, nVersionIn) | ||
| m_magic_bytes{Params().MessageStart()}, m_node_id(node_id), hdrbuf(nTypeIn, nVersionIn), vRecv(nTypeIn, nVersionIn) |
There was a problem hiding this comment.
🔴 Blocking: V1Transport ctor brace-init breaks build: bitcoin#28857 applied without bitcoin#28423 prerequisite
The bitcoin#28857 cherry-pick includes upstream commit d49d198 which replaces the V1Transport ctor body with a member-initializer m_magic_bytes{Params().MessageStart()}. Upstream this is valid because bitcoin#28423 (commit 9be330b) had already converted MessageStartChars from typedef unsigned char MessageStartChars[MESSAGE_START_SIZE] to using MessageStartChars = std::array<uint8_t, 4>. Dash has not backported bitcoin#28423: src/protocol.h:36 still has the C-array typedef, src/chainparams.h:85 returns const MessageStartChars& to that C array, and src/net.h:410 declares m_magic_bytes as const CMessageHeader::MessageStartChars (i.e. const unsigned char[4]). Brace-initialization of a C-array aggregate from a single array-reference element is invalid C++: it is parsed as a one-element aggregate initializer whose element type unsigned char cannot be initialized from const unsigned char(&)[4]. Reproduced locally with clang++ -std=c++20: error: cannot initialize an array element of type 'const unsigned char' with an lvalue of type 'const MessageStartChars' (aka 'const unsigned char[4]'). The pre-PR Dash code correctly worked around the C-array limitation by using std::copy(std::begin(Params().MessageStart()), std::end(Params().MessageStart()), m_magic_bytes) in the constructor body. Fix options: (a) backport bitcoin#28423 first and then keep the refactor, (b) drop upstream commit d49d198 from this cherry-pick and restore the std::copy body (and keep m_magic_bytes non-const in src/net.h), or (c) expand the brace-init to element-wise initialization as shown in the suggestion (compiles but loses the const intent only if the member type is kept; this works while keeping const).
| V1Transport::V1Transport(const NodeId node_id, int nTypeIn, int nVersionIn) noexcept : | |
| m_node_id(node_id), hdrbuf(nTypeIn, nVersionIn), vRecv(nTypeIn, nVersionIn) | |
| m_magic_bytes{Params().MessageStart()}, m_node_id(node_id), hdrbuf(nTypeIn, nVersionIn), vRecv(nTypeIn, nVersionIn) | |
| V1Transport::V1Transport(const NodeId node_id, int nTypeIn, int nVersionIn) noexcept : | |
| m_magic_bytes{Params().MessageStart()[0], Params().MessageStart()[1], Params().MessageStart()[2], Params().MessageStart()[3]}, m_node_id(node_id), hdrbuf(nTypeIn, nVersionIn), vRecv(nTypeIn, nVersionIn) |
source: ['claude', 'codex']
6 participants
bitcoin backports