refactor: complete removal of error logging helper - #7683
PastaPastaPasta merged 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
|
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughThe legacy Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Phase 1 blocker gate
The refactor introduces one release-build undefined-behavior bug: FindDevNetGenesisBlock can reach the end of a non-void function after the assertion is compiled out. The transaction validation log also reports the wrong validation state, and CFlatDB::Read retains the old split ERROR-prefixed logging pattern, undermining the stated logging cleanup.
Validated blockers were found by the Phase-1 review and confirmed by a fresh verifier. Phase 2 is deferred until a fresh same-head revalidation clears the blocker gate.
🔴 1 blocking | 🟡 2 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: CheckTxInputs failure logs the wrong validation state
src/validation.cpp:2429-2433
This branch validates into tx_state, but the LogError call prints state.ToString(), where state is the outer block validation state and has not been invalidated yet. Consequently, the log omits the transaction's actual reject reason and debug message even though those values are returned from tx_state immediately afterward. Log tx_state.ToString() instead.
LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), tx_state.ToString());
source: muse-spark-1.3-contributor (phase1-reviewer: general, dash-core-commit-history)
🟡 Suggestion: CFlatDB::Read retains split ERROR-prefixed logging
src/flat-database.h:160-173
Read() still emits LogPrintf("ERROR: ...") followed by a second LogPrintf to complete the message. This preserves the legacy manual severity prefix and fragmented record even though the PR's stated goal is to convert remaining error logging to LogError with consistent severity and source locations. Convert each error branch to one LogError call, preserving the existing recreate-versus-fail behavior.
else if (readResult != ReadResult::Ok) {
if (readResult == ReadResult::IncorrectFormat) {
LogError("%s: Magic is ok but data has invalid format, will try to recreate (%s)\n", __func__, strFilename);
} else {
LogError("%s: File format is unknown or invalid, please fix it manually (%s)\n", __func__, strFilename);
// program should exit with an error
return false;
}
}
source: muse-spark-1.3-contributor (phase1-reviewer: general, dash-core-commit-history)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-gate-verifier, role: verifier)
- Triage:
normalbygpt-6-astra(effort low) — This is a cross-cutting refactor across database, indexing, validation, chain-parameter, special-transaction, and logging-test code that changes error-handling return paths, but it does not itself alter consensus, funds movement, cryptography, networking deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— dash-core-commit-history (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 11% left, 5h 100% left),glm-5.3-flash(zai below 15% reserve: 5h 0% left, weekly 53% left) - Fresh verifier:
gpt-6-astra— verifier; agentastra-gate-verifier - Phase 2 reviewers: not run (deferred by blocker gate)
🤖 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/chainparams.cpp`:
- [BLOCKING] src/chainparams.cpp:103-107: FindDevNetGenesisBlock falls off a non-void function
FindDevNetGenesisBlock returns CBlock by value, but after the nonce loop it only logs an error and calls assert(false). In release builds, assert is compiled out, so control reaches the end of the function without returning a CBlock, which is undefined behavior. The failure path needs a terminating operation that is effective in release builds, such as throwing an exception after the assertion.
In `src/validation.cpp`:
- [SUGGESTION] src/validation.cpp:2429-2433: CheckTxInputs failure logs the wrong validation state
This branch validates into tx_state, but the LogError call prints state.ToString(), where state is the outer block validation state and has not been invalidated yet. Consequently, the log omits the transaction's actual reject reason and debug message even though those values are returned from tx_state immediately afterward. Log tx_state.ToString() instead.
In `src/flat-database.h`:
- [SUGGESTION] src/flat-database.h:160-173: CFlatDB::Read retains split ERROR-prefixed logging
Read() still emits LogPrintf("ERROR: ...") followed by a second LogPrintf to complete the message. This preserves the legacy manual severity prefix and fragmented record even though the PR's stated goal is to convert remaining error logging to LogError with consistent severity and source locations. Convert each error branch to one LogError call, preserving the existing recreate-versus-fail behavior.
| // This is very unlikely to happen as we start the devnet with a very low difficulty. In many cases even the first | ||
| // iteration of the above loop will give a result already | ||
| error("FindDevNetGenesisBlock: could not find devnet genesis block for %s", devNetName); | ||
| LogError("FindDevNetGenesisBlock: could not find devnet genesis block for %s\n", devNetName); | ||
| assert(false); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: FindDevNetGenesisBlock falls off a non-void function
FindDevNetGenesisBlock returns CBlock by value, but after the nonce loop it only logs an error and calls assert(false). In release builds, assert is compiled out, so control reaches the end of the function without returning a CBlock, which is undefined behavior. The failure path needs a terminating operation that is effective in release builds, such as throwing an exception after the assertion.
| // This is very unlikely to happen as we start the devnet with a very low difficulty. In many cases even the first | |
| // iteration of the above loop will give a result already | |
| error("FindDevNetGenesisBlock: could not find devnet genesis block for %s", devNetName); | |
| LogError("FindDevNetGenesisBlock: could not find devnet genesis block for %s\n", devNetName); | |
| assert(false); | |
| } | |
| LogError("FindDevNetGenesisBlock: could not find devnet genesis block for %s\n", devNetName); | |
| assert(false); | |
| throw std::runtime_error(strprintf("FindDevNetGenesisBlock: could not find devnet genesis block for %s", devNetName)); |
source: muse-spark-1.3-contributor (phase1-reviewer: general, dash-core-commit-history)
There was a problem hiding this comment.
Resolved (re-reviewed at 907354cf): FindDevNetGenesisBlock now throws a std::runtime_error after logging if nonce exhaustion occurs, so the non-void function no longer falls through without returning.
There was a problem hiding this comment.
Withdrawn (re-reviewed at a03a9c5c): Your explanation is correct: Dash rejects NDEBUG builds, and the retained assert(false) terminates this path. The removed throw was unreachable; the original fall-through finding was incorrect and does not require a separate hardening change.
thepastaclaw
added
the
pastaclaw:changes-requested
{
"worktree": "/Users/claw/Projects/dash-pr37/worktrees/tracker-3153",
"model": "grok-4.5",
"thinking": "high",
"task_brief": "Implement three verified findings on dashpay/dash PR #7683: add a release-effective terminating operation after the assert in FindDevNetGenesisBlock; log tx_state.ToString() in the CheckTxInputs failure branch; replace split manual ERROR-prefixed LogPrintf records in CFlatDB::Read with single LogError calls while preserving recreate-versus-fail behavior. Inspect instructions, surrounding code/history, make only scoped edits, commit incrementally, run focused tests, formatting and git diff --check, and run configured Claude review/quality hooks. Do not push, publish, or open a PR.",
"allowed_writes": ["/Users/claw/Projects/dash-pr37/worktrees/tracker-3153/src/chainparams.cpp", "/Users/claw/Projects/dash-pr37/worktrees/tracker-3153/src/validation.cpp", "/Users/claw/Projects/dash-pr37/worktrees/tracker-3153/src/flat-database.h", "tests only if required by the verified fixes"],
"validation": ["focused relevant unit tests", "format/lint checks relevant to changed files", "git diff --check", "configured Claude Code review and quality hooks"]
}Managed Claude ACP cannot be spawned in this worker because the configured ACP agent allowlist excludes |
thepastaclaw
removed
the
pastaclaw:changes-requested
![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: 907354cfe4
ℹ️ 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".
| 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()); |
There was a problem hiding this comment.
When Consensus::CheckTxInputs fails, this logs the outer BlockValidationState instead of the TxValidationState that contains the actual reject reason and debug message. The function returns the transaction state's details immediately afterward, so the error log can be empty or misleading precisely when operators need it to diagnose a rejected block; log tx_state.ToString() here.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
The implementation correctly completes removal of the obsolete error logging helper, and the previously reported logging and non-void fall-through issues are fixed at the exact head. No new correctness or Dash-specific interaction issues were found. The branch still contains two small corrective commits that would be cleaner folded into the main refactor before merge.
🟡 2 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Squash review-fix commit into the refactor
<commit:2eefd9cc89>:1
Commit 2eefd9c (fix: address error logging review feedback) changes only call sites introduced by 5a06c55, correcting the flat-database logging split and the validation-state argument. Since this is direct corrective work for the preceding refactor and has no independent durable purpose, squash it into 5a06c55 so the permanent history contains one coherent change.
source: gpt-6-astra (phase2-reviewer: dash-core-commit-history)
🟡 Suggestion: Squash the devnet termination fix into the refactor
<commit:907354cfe4>:1
Commit 907354c (fix: terminate devnet genesis search failure) adds the missing termination path to FindDevNetGenesisBlock, a function modified by 5a06c55. This is another direct corrective follow-up to the same refactor; fold it into 5a06c55 along with 2eefd9c so the permanent history avoids review-fix seams.
source: gpt-6-astra (phase2-reviewer: dash-core-commit-history)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — This is a cross-cutting logging and error-handling refactor spanning database, indexing, validation, chain-parameter, special-transaction, and utility code with a regression test, but it does not directly change consensus, funds movement, cryptography, networking deserialization, or storage migrations. - Phase 1 reviewers: not run (skipped for throughput: 12 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 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 `<commit:2eefd9cc89>`:
- [SUGGESTION] <commit:2eefd9cc89>:1: Squash review-fix commit into the refactor
Commit 2eefd9cc89 (`fix: address error logging review feedback`) changes only call sites introduced by 5a06c55641, correcting the flat-database logging split and the validation-state argument. Since this is direct corrective work for the preceding refactor and has no independent durable purpose, squash it into 5a06c55641 so the permanent history contains one coherent change.
In `<commit:907354cfe4>`:
- [SUGGESTION] <commit:907354cfe4>:1: Squash the devnet termination fix into the refactor
Commit 907354cfe4 (`fix: terminate devnet genesis search failure`) adds the missing termination path to `FindDevNetGenesisBlock`, a function modified by 5a06c55641. This is another direct corrective follow-up to the same refactor; fold it into 5a06c55641 along with 2eefd9cc89 so the permanent history avoids review-fix seams.
907354c to
414b457
Compare
thepastaclaw
removed
the
pastaclaw:commented
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
The complete 11-file diff at 414b457 contains no actionable in-scope defects, and all five prior findings are fixed. The logging conversions preserve failure propagation and Dash-specific state handling while correcting severity, source locations, and record separation. The local unit-test target built successfully, all 8 logging tests passed with 67 assertions, and format-string lint and diff whitespace checks passed.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — This is a cross-cutting but contained refactor across database, indexing, validation, chain-parameter, and special-transaction error paths, with explicit return-flow changes and a regression test, but it does not itself alter consensus, funds movement, cryptography, networking, or storage formats. - Phase 1 reviewers: not run (skipped for throughput: 31 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
| error("FindDevNetGenesisBlock: could not find devnet genesis block for %s", devNetName); | ||
| LogError("FindDevNetGenesisBlock: could not find devnet genesis block for %s\n", devNetName); | ||
| assert(false); | ||
| throw std::runtime_error(strprintf("FindDevNetGenesisBlock: could not find devnet genesis block for %s", devNetName)); |
There was a problem hiding this comment.
Why adding this? Isn't it a dead code?
There was a problem hiding this comment.
You're right, it's dead code — src/util/check.h #errors out if NDEBUG is defined, so assert(false) always aborts and nothing after it is reachable. I added it while converting the error() call (which returned bool) and over-corrected for the non-void return; the pre-existing error(...); assert(false); already compiled fine, and -Werror=return-type is happy without the throw.
Dropped the throw and the now-unneeded <stdexcept> include in a03a9c5, so this hunk is just the error() → LogError() swap, matching AddLLMQ() right below it.
🤖 Posted autonomously by Claude on behalf of pasta.
414b457 to
a03a9c5
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
thepastaclaw
removed
the
pastaclaw:approved
| BOOST_CHECK_EQUAL_COLLECTIONS(log_lines.begin(), log_lines.end(), expected.begin(), expected.end()); | ||
| } | ||
|
|
||
| BOOST_FIXTURE_TEST_CASE(logging_flat_database_failure, LogSetup) |
There was a problem hiding this comment.
why is that?
Seems as completely irrelevant, isn't it?
File is backported. Log fixes are trivial. This PR is simple refactoring.
This test is useless.
There was a problem hiding this comment.
Agreed — the test was dropped in 4d52849, so src/test/logging_tests.cpp is no longer part of this PR.
The branch has since been rebuilt again (now at 3d27bac) to keep it strictly mechanical: the src/validation.cpp state → tx_state change was a behavior fix rather than an error(...) conversion and has been split out into #7713, and a CFlatDB::Read() hunk that converted LogPrintf calls (not error(...)) and reworded their messages was reverted. The only addition is dropping SafeStringFormat() from src/logging.h, which becomes dead code once error(...) is deleted — it was its only caller.
🤖 Posted autonomously by Claude on behalf of pasta.
|
extra comments about "how has this been testsed" Instead adding a new test, just run and validate that it works! This test is useless. Instead checking for |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Independently reviewed the complete diff at a03a9c5 and found no actionable in-scope defects. Four prior findings are fixed; the devnet fall-through finding is withdrawn because the retained assert(false) terminates execution in supported builds. Format-string lint and diff whitespace checks passed; compilation and runtime tests were not performed during this verification.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: dash-core-commit-history); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
lowbygpt-6-astra(effort low) — The diff is a straightforward logging-helper replacement with preserved failure returns, a diagnostic correction, and a focused regression test, without changing consensus, storage formats, or other critical behavior. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— dash-core-commit-history (completed, effort high); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(zai below 15% reserve: 5h 100% left, weekly 13% left) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort medium); agentphase2-reviewer,gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
a03a9c5 to
4d52849
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
thepastaclaw
removed
the
pastaclaw:approved
4d52849 to
d03c5df
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d03c5df to
3d27bac
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Rebuilt this branch to keep it strictly mechanical. Two changes were not
One change was added, because this PR creates it: The diff is now a pure 🤖 Posted autonomously by Claude on behalf of pasta. |
4 participants
Issue being fixed or feature implemented
The backport of Bitcoin Core bitcoin#29236 merged in Dash #7292 converted the upstream call sites but intentionally left Dash-specific
error(...)users in place. This leaves inconsistent error severity and source locations, and prevents completing the logging cleanup in Dash Core.What was done?
error(...)call sites in flat database, index, transaction database, validation, chain parameters, and special transaction code toLogError(...);with explicit failure returns.error(...)helper fromsrc/util/system.h.SafeStringFormat()fromsrc/logging.h. Theerror(...)helper was its only remaining caller, so it is dead code once that helper is gone. Nothing is lost:LogPrintf_()already wrapstfm::formatin its owntinyformat::format_errorhandler, soLogError(...)has the same no-throw propertySafeStringFormat()provided.Every hunk is a mechanical
error(fmt, ...)→LogError(fmt "\n", ...); return false;conversion —LogErrordoes not append a newline, hence the added\n. One conversion is a small hardening as a side effect:The old call passed a runtime-built string as the format string, so a
%insidee.what()was a format-string hazard; the new call passes it as an argument.No behavior other than log severity/source-location changes, and no log message text changes.
How Has This Been Tested?
make -j. Since theerror(...)helper is deleted fromsrc/util/system.h, a successful build is itself proof that no call sites remain../src/test/test_dash(969 test cases, no errors).test/lint/lint-logs.pyandtest/lint/lint-format-strings.py.Breaking Changes
None.
Checklist:
This pull request was created by Codex.