fix: repair dash-chainstate dist sources#7341
Conversation
|
|
✅ Review complete (commit a42b90b) |
|
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: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR updates src/Makefile.am: replaces Sequence Diagram(s)sequenceDiagram
participant BitcoinChainstate as bitcoin-chainstate.cpp
participant CSporkManager as CSporkManager
participant Chainlocks as chainlock::Chainlocks
participant Node as node::LoadChainstate
BitcoinChainstate->>CSporkManager: construct CSporkManager
BitcoinChainstate->>Chainlocks: construct Chainlocks
BitcoinChainstate->>Node: call LoadChainstate(..., mn_sync, ...)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. 🔧 Infer (1.2.0)src/bitcoin-chainstate.cppsrc/bitcoin-chainstate.cpp:14:10: error: 'chainlock/chainlock.h' file not found with include; use "quotes" instead ... [truncated 2200 characters] ... ine 18, characters 15-63 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
There was a problem hiding this comment.
Code Review
Convergent finding from both reviewers: the repaired dash_chainstate_SOURCES list correctly updates moved/renamed governance and address-index paths so make distdir passes, but it still omits index/addressindex_util.cpp — the file that defines AddressBytesFromScript, which is referenced by both index/addressindex.cpp (6 call sites) and the already-listed txmempool.cpp (3 call sites). The dash-chainstate binary will therefore fail to link with an undefined reference when --enable-experimental-util-chainstate is set. Since this PR's explicit goal is to repair dash-chainstate sources, adding the helper is in scope.
🔴 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/Makefile.am`:
- [BLOCKING] src/Makefile.am:1168: dash_chainstate is missing index/addressindex_util.cpp — link will fail with undefined AddressBytesFromScript
The updated source list points at the real `index/addressindex.cpp`, but does not include its companion `index/addressindex_util.cpp`, which is the sole definition of `AddressBytesFromScript` (src/index/addressindex_util.cpp:12). That symbol is used 6 times in `index/addressindex.cpp` (lines 211, 232, 254, 313, 341, 368) and 3 times in `txmempool.cpp` (lines 555, 571, 626), both of which are compiled into `dash_chainstate`. Because `dash-chainstate` is linked from this explicit source list rather than against the server library where the helper is normally pulled in, building with `--enable-experimental-util-chainstate` will fail at link time with an unresolved `AddressBytesFromScript` symbol. The PR fixes `make distdir` but leaves the actual binary unbuildable, so the repair is incomplete. Add `index/addressindex_util.cpp` next to the address index source — it is already listed in `libbitcoin_common_a_SOURCES` (Makefile.am:540), confirming it is a standalone translation unit. Note: if `spentindex.cpp` / `timestampindex.cpp` end up transitively required by chainstate code paths they would need similar treatment, but `addressindex_util.cpp` is the unambiguous immediate gap.
| dash_chainstate_SOURCES = \ | ||
| bitcoin-chainstate.cpp \ | ||
| addressindex.cpp \ | ||
| index/addressindex.cpp \ |
There was a problem hiding this comment.
🔴 Blocking: dash_chainstate is missing index/addressindex_util.cpp — link will fail with undefined AddressBytesFromScript
The updated source list points at the real index/addressindex.cpp, but does not include its companion index/addressindex_util.cpp, which is the sole definition of AddressBytesFromScript (src/index/addressindex_util.cpp:12). That symbol is used 6 times in index/addressindex.cpp (lines 211, 232, 254, 313, 341, 368) and 3 times in txmempool.cpp (lines 555, 571, 626), both of which are compiled into dash_chainstate. Because dash-chainstate is linked from this explicit source list rather than against the server library where the helper is normally pulled in, building with --enable-experimental-util-chainstate will fail at link time with an unresolved AddressBytesFromScript symbol. The PR fixes make distdir but leaves the actual binary unbuildable, so the repair is incomplete. Add index/addressindex_util.cpp next to the address index source — it is already listed in libbitcoin_common_a_SOURCES (Makefile.am:540), confirming it is a standalone translation unit. Note: if spentindex.cpp / timestampindex.cpp end up transitively required by chainstate code paths they would need similar treatment, but addressindex_util.cpp is the unambiguous immediate gap.
| index/addressindex.cpp \ | |
| bitcoin-chainstate.cpp \ | |
| index/addressindex.cpp \ | |
| index/addressindex_util.cpp \ |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved by 7a87bacc5d4cb1d9b5bb3ebf68a9e41ddb7c4fc8 — _dash_chainstate now includes index/addressindex_util.cpp, so AddressBytesFromScript is provided.
Review-system reconciliation based on the latest commit diff.
0670788 to
80f75aa
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior blocking finding (missing index/addressindex_util.cpp in dash_chainstate_SOURCES) is FIXED at src/Makefile.am:1169 — the util file is listed immediately after index/addressindex.cpp, providing AddressBytesFromScript. Latest delta also adds index/spentindex.cpp (line 1223) and rewires bitcoin-chainstate.cpp to the new LoadChainstate signature. Both Claude and Codex independently verified the fix; no new findings; CodeRabbit reported 0 inline findings.
80f75aa to
7a87bac
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 7a87bac confirms PR is a narrowly scoped repair of the experimental dash-chainstate target after upstream refactors of the address-index and governance code. dash_chainstate_SOURCES correctly references the relocated index/addressindex.cpp, index/addressindex_util.cpp, index/spentindex.cpp, and governance/superblock.cpp, and drops files that no longer exist (governance/classes.cpp, governance/exceptions.cpp, governance/validators.cpp) plus files unnecessary for this binary in the latest delta (governance/net_governance.cpp, governance/signing.cpp). The LoadChainstate call in bitcoin-chainstate.cpp matches the signature in src/node/chainstate.h:75-100 (govman dropped, mn_sync added, index booleans removed). Carried-forward prior findings: none. New findings in the latest delta: none.
PastaPastaPasta
left a comment
There was a problem hiding this comment.
self reviewed; I believe this is proper; and fixes CI; @knst please review post merge
7a87bac to
4709e99
Compare
| governance/governance.cpp \ | ||
| governance/object.cpp \ | ||
| governance/validators.cpp \ | ||
| governance/superblock.cpp \ |
There was a problem hiding this comment.
governance/superblock.cpp is a replacement for governance/governance.cpp for chainstate, not an addition:
- governance/governance.cpp \4709e99 to
a42b90b
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at a42b90b confirms the dash-chainstate dist source list and bitcoin-chainstate.cpp call site are consistent with the current tree after the address-index split, governance/superblock manager separation, and LoadChainstate signature change. Carried-forward prior findings: none (prior review at 7a87bac was clean). New findings in the latest delta (removal of governance/governance.cpp from dash_chainstate_SOURCES): none — consistent with LoadChainstate no longer constructing CGovernanceManager. Both agents and CodeRabbit reported zero inline findings.
Issue being fixed or feature implemented
The
make distdiranddash-chainstatepackaging/build paths referenced stale source paths after recent address index and governance refactors. This caused CI packaging/build jobs using--enable-experimental-util-chainstateto fail.What was done?
dash_chainstate_SOURCESto useindex/addressindex.cppand include its standalone helper/source dependencies.bitcoin-chainstate.cppto match the currentLoadChainstatesetup flow after the governance/superblock manager refactor.How Has This Been Tested?
./configure --enable-experimental-util-chainstatemake -C src -j4 dash-chainstategit diff --checkrm -rf dashcore-distcheck-test && mkdir dashcore-distcheck-test && (cd src && make top_distdir=../dashcore-distcheck-test distdir=../dashcore-distcheck-test/src am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)Breaking Changes
None.
Checklist:
This pull request was created by Codex.