Merge bitcoin/bitcoin#27039: blockstorage: do not flush block to disk if it is already there #1123
Conversation
…s already there dfcef53 blockstorage: do not flush block to disk if it is already there (Matthew Zipkin) Pull request description: Closes bitcoin#2039 When reindexing from flat-file block storage there is no need to write anything back to disk, since the block data is already there. This PR skips flushing to disk those blocks that already have a known position in the datastore. Skipping this means that users can write-protect the `blk` files on disk which may be useful for security or even safely sharing that data between multiple bitcoind instances. `FindBlockPos()` may also flush the undo data file, but again this is skipped if the corresponding block position is known, like during the initial stage of a reindex when block data is being indexed. Once the block index is complete the validation mechanism will call `ConnectBlock()` which will save undo data at that time. The call stack looks like this: ``` init() ThreadImport() <-- process fReindex flag LoadExternalBlockFile() AcceptBlock() SaveBlockToDisk() FindBlockPos() FlushBlockFile() <-- unnecessary if block is already on disk ``` A larger refactor of this part of the code was started by mzumsande here: https://github.com/mzumsande/bitcoin/tree/202207_refactor_findblockpos including this fix, reviewers can let me know if the changes should be combined. ACKs for top commit: sipa: utACK dfcef53 mzumsande: re-ACK dfcef53 achow101: ACK dfcef53 furszy: Rebase diff ACK dfcef53. Tree-SHA512: 385c5ac1288b325135398d0ddd3ab788fa98cc0ca19bd2474c74039f2ce70d5088c1d1c9d4dd10aefcbd4c757767ec5805d07ba8cee9289a66f96e6f9eaa5279
WalkthroughUpdates block file switch flushing in BlockManager::FindBlockPos to explicitly flush the previous file with its index and log failures without propagating errors. Adds a functional test that simulates reindexing with a read-only/immutable blk00000.dat under -fastprune, applying platform-specific immutability flags and verifying behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Backport Verification SuccessfulNo issues found! This backport is complete and faithful to the original Bitcoin change. Original Bitcoin commit: Verification Details:
Core Changes Analysis:Bitcoin Intent: Skip flushing block files to disk during reindex when block position is already known, enabling read-only reindexing. Dash Implementation: ✅ Correctly implemented
Size Analysis:
Size Difference Explained: ✅ Acceptable
Reviewer Feedback Analysis:PastaPastaPasta comments reviewed: 0 No human reviewer feedback - automation-generated backport with clean implementation. This PR correctly implements Bitcoin's optimization to avoid unnecessary disk writes during reindexing, allowing users to write-protect block files for security or sharing between multiple dashd instances. The core logic changes are minimal and faithful to Bitcoin's intent. This PR is ready for merge. ✅ |
DashCoreAutoGuix
added
the
verified
PastaPastaPasta
added
ci-failed
Backport Validation Report for PR #1123Original Bitcoin Commit: bitcoin/bitcoin#27039 by Matthew Zipkin SummaryThis PR backports Bitcoin Core's fix to prevent unnecessary flushing of block data to disk when the blocks are already present, particularly during reindex operations. This enables safer read-only block file operations. Files Modified
Issues Found and Fixed1. Lint Error - Missing /* Continued */ CommentIssue: LogPrintLevel call was split across multiple lines without the required 2. File Operation ValidationAnalysis: The backport correctly creates a new test file ( Core Changes Validatedsrc/node/blockstorage.cpp✅ Correctly implemented: The logic now only calls Key behavioral change:
test/functional/feature_reindex_readonly.py✅ Correctly adapted: The test now expects reindex to succeed with read-only block files (previous behavior was to fail). This reflects the fix working correctly. Size Analysis
The size difference is justified because:
Validation ConclusionThis backport faithfully implements Bitcoin's blockstorage optimization. The lint issue has been resolved and the changes correctly prevent unnecessary disk I/O during reindex operations, enabling read-only block file scenarios. Recommendation: ✅ APPROVED - Backport is functionally correct and ready for merge after lint fix. Validation performed by automated backport validator |
This resolves the lint error by following Dash's coding standard for multiline logging statements. The LogPrintLevel call was split across multiple lines and needed the /* Continued */ comment to pass lint checks. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/node/blockstorage.cpp(1 hunks)test/functional/feature_reindex_readonly.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be placed in test/functional/ and written in Python
Files:
test/functional/feature_reindex_readonly.py
**
⚙️ CodeRabbit configuration file
**: # CodeRabbit AI Review Instructions for Dash BackportsYour Role
You are reviewing Bitcoin Core backports to Dash Core. Your ONLY job is to validate that the Dash commit faithfully represents the original Bitcoin commit with minimal, necessary adaptations.
Critical Validation Rules
1. File Operations Must Match (AUTO-REJECT if violated)
- If Bitcoin modifies an existing file → Dash MUST modify (not create new)
- If Bitcoin creates a new file → Dash creates
- If Bitcoin deletes a file → Dash deletes
- Common failure: Bitcoin modifies keys.txt, Dash creates new file with 58 keys
2. Size Ratio Check (80-150% of Bitcoin)
- Count functional lines changed (exclude comments/whitespace)
- Dash changes should be 80-150% of Bitcoin's size
- Red flag: 2-line Bitcoin fix becoming 150+ lines in Dash
3. No Scope Creep
- Reject if you see: "TODO:", "FIXME:", "while we're here", "also fix"
- No unrelated refactoring or style changes
- Only Bitcoin's intended changes + minimal Dash adaptations
4. Bitcoin-Specific Code Detection
- Auto-reject witness/segwit code:
msg_wtxidrelay,MSG_WTX, witness imports- Auto-reject RBF (replace-by-fee) functionality
- Note: PSBT is supported in Dash (don't flag)
5. Mandatory Adaptations Only
bitcoin→dashin strings/pathsBitcoin→Dashin user-facing text- Port numbers: 8332→9998 (RPC), 8333→9999 (P2P)
- Hardcoded test values specific to Dash
- No other changes unless absolutely required
6. Completeness Check
- All files changed in Bitcoin must be present
- Extra files need clear justification (Dash-specific compatibility)
- Missing files = incomplete backport
Review Process
- First: Check file operations match exactly
- Second: Calculate size ratio
- Third: Scan for scope creep patterns
- Fourth: Detect Bitcoin-specific code
- Fifth: Verify all changes are minimal adaptations
Output Format
VALIDATION: [PASS/FAIL] File Operatio...
Files:
test/functional/feature_reindex_readonly.pysrc/node/blockstorage.cpp
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
C++20 codebase should be placed under src/
Files:
src/node/blockstorage.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.
🧬 Code graph analysis (1)
test/functional/feature_reindex_readonly.py (2)
test/functional/test_framework/test_framework.py (1)
start_node(646-655)test/functional/test_framework/test_node.py (2)
chain_path(429-430)wait_for_debug_log(473-506)
🪛 Ruff (0.12.2)
test/functional/feature_reindex_readonly.py
16-16: Missing return type annotation for public function set_test_params
Add return type annotation: None
(ANN201)
21-21: reindex_readonly is too complex (12 > 10)
(C901)
21-21: Missing return type annotation for public function reindex_readonly
Add return type annotation: None
(ANN201)
37-37: Do not assign a lambda expression, use a def
Rewrite undo_immutable as a def
(E731)
40-40: Starting a process with a partial executable path
(S607)
42-42: subprocess call: check for execution of untrusted input
(S603)
42-42: Starting a process with a partial executable path
(S607)
43-43: Do not assign a lambda expression, use a def
Rewrite undo_immutable as a def
(E731)
43-43: subprocess call: check for execution of untrusted input
(S603)
43-43: Starting a process with a partial executable path
(S607)
48-48: Logging statement uses f-string
(G004)
50-50: Logging statement uses f-string
(G004)
56-56: Do not catch blind exception: Exception
(BLE001)
59-59: Starting a process with a partial executable path
(S607)
61-61: subprocess call: check for execution of untrusted input
(S603)
61-61: Starting a process with a partial executable path
(S607)
62-62: Do not assign a lambda expression, use a def
Rewrite undo_immutable as a def
(E731)
62-62: subprocess call: check for execution of untrusted input
(S603)
62-62: Starting a process with a partial executable path
(S607)
67-67: Logging statement uses f-string
(G004)
69-69: Logging statement uses f-string
(G004)
73-74: try-except-pass detected, consider logging the exception
(S110)
73-73: Do not catch blind exception: Exception
(BLE001)
85-85: Missing return type annotation for public function run_test
Add return type annotation: None
(ANN201)
🪛 Flake8 (7.2.0)
test/functional/feature_reindex_readonly.py
[error] 37-37: do not assign a lambda expression, use a def
(E731)
[error] 43-43: do not assign a lambda expression, use a def
(E731)
[error] 62-62: do not assign a lambda expression, use a def
(E731)
🔇 Additional comments (3)
src/node/blockstorage.cpp (1)
645-651: LGTM: flush moved under !fKnown with rationale preservedPlacing the flush behind
if (!fKnown)matches the upstream intent (don’t write when the block position is already known) and the added comment documents why failures shouldn’t be propagated.test/functional/feature_reindex_readonly.py (2)
76-82: Run/skip logic depending on immutability: intentional, but clarify conditionUsing the truthiness of
undo_immutableto gate the test is fine. Given we now default toNoneand only set a callable when immutable mode is successfully applied (or toFalseto skip on root), the behavior is explicit and avoids running on unsupported platforms.
21-27: generateblock helper confirmed in test frameworkThe
generateblockhelper is defined and in active use within Dash’s functional test suite, supporting theoutput="raw(...)"andtransactions=[]parameters as expected.•
test/functional/test_framework/test_node.py:349–351defines a pass-throughgenerateblockmethod
•test/functional/test_framework/test_framework.py:808–811implementsgenerateblock(generator, …)that invokes the node’s helper
• Multiple existing tests (e.g.rpc_generateblock.py,feature_utxo_set_hash.py) callself.generateblock(node, output="raw(...)", transactions=[])without issueNo adaptation is necessary.
|
|
||
| // Do not propagate the return code. The flush concerns a previous block | ||
| // and undo file that has already been written to. If a flush fails | ||
| // here, and we crash, there is no expected additional block data | ||
| // inconsistency arising from the flush failure here. However, the undo | ||
| // data may be inconsistent after a crash if the flush is called during | ||
| // a reindex. A flush error might also leave some of the data files | ||
| // untrimmed. | ||
| if (!FlushBlockFile(m_last_blockfile, !fKnown, finalize_undo)) { | ||
| LogPrintLevel(BCLog::BLOCKSTORE, BCLog::Level::Warning, /* Continued */ | ||
| "Failed to flush previous block file %05i (finalize=%i, finalize_undo=%i) before opening new block file %05i\n", | ||
| m_last_blockfile, !fKnown, finalize_undo, nFile); | ||
| } |
There was a problem hiding this comment.
Compilation break: calling non-existent FlushBlockFile overload; adapt backport to Dash’s BlockManager API
The new call FlushBlockFile(m_last_blockfile, !fKnown, finalize_undo) assumes an overload that accepts a file index and returns a boolean. In this file, only void BlockManager::FlushBlockFile(bool fFinalize, bool finalize_undo) exists. This will not compile (wrong arity and return type), and the if (!FlushBlockFile(...)) check is invalid.
To faithfully mirror Bitcoin’s behavior (log-and-continue instead of aborting) and to keep existing callers intact, add a new overload:
- Declare a new overload in
src/node/blockstorage.h:
+// Flush block file for a specific file index. Returns false on failure (no AbortNode).
+bool FlushBlockFile(int block_file, bool fFinalize, bool finalize_undo);- Define it in
src/node/blockstorage.cppnear the existing FlushBlockFile():
+bool BlockManager::FlushBlockFile(int block_file, bool fFinalize, bool finalize_undo)
+{
+ LOCK(cs_LastBlockFile);
+ FlatFilePos block_pos_old(block_file, m_blockfile_info[block_file].nSize);
+ if (!BlockFileSeq().Flush(block_pos_old, fFinalize)) {
+ return false;
+ }
+ // We do not always flush the undo file, as the chain tip may be lagging behind.
+ if (!fFinalize || finalize_undo) {
+ FlushUndoFile(block_file, finalize_undo);
+ }
+ return true;
+}Optionally, re-implement the legacy two-arg version in terms of the new one to preserve its current AbortNode semantics without duplicating logic:
void BlockManager::FlushBlockFile(bool fFinalize, bool finalize_undo)
{
- LOCK(cs_LastBlockFile);
- FlatFilePos block_pos_old(m_last_blockfile, m_blockfile_info[m_last_blockfile].nSize);
- if (!BlockFileSeq().Flush(block_pos_old, fFinalize)) {
- AbortNode("Flushing block file to disk failed. This is likely the result of an I/O error.");
- }
- // we do not always flush the undo file, as the chain tip may be lagging behind the incoming blocks,
- // e.g. during IBD or a sync after a node going offline
- if (!fFinalize || finalize_undo) FlushUndoFile(m_last_blockfile, finalize_undo);
+ if (!FlushBlockFile(m_last_blockfile, fFinalize, finalize_undo)) {
+ AbortNode("Flushing block file to disk failed. This is likely the result of an I/O error.");
+ }
}With this addition, the new call inside FindBlockPos compiles and behaves like upstream (only log a warning on failure).
📝 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.
| // Do not propagate the return code. The flush concerns a previous block | |
| // and undo file that has already been written to. If a flush fails | |
| // here, and we crash, there is no expected additional block data | |
| // inconsistency arising from the flush failure here. However, the undo | |
| // data may be inconsistent after a crash if the flush is called during | |
| // a reindex. A flush error might also leave some of the data files | |
| // untrimmed. | |
| if (!FlushBlockFile(m_last_blockfile, !fKnown, finalize_undo)) { | |
| LogPrintLevel(BCLog::BLOCKSTORE, BCLog::Level::Warning, /* Continued */ | |
| "Failed to flush previous block file %05i (finalize=%i, finalize_undo=%i) before opening new block file %05i\n", | |
| m_last_blockfile, !fKnown, finalize_undo, nFile); | |
| } | |
| ++ b/src/node/blockstorage.h | |
| @@ class BlockManager { | |
| /** | |
| * Flush block file for a specific file index. | |
| * Returns false on failure (no AbortNode). | |
| */ | |
| bool FlushBlockFile(int block_file, bool fFinalize, bool finalize_undo); | |
| /** | |
| * Flush the last block file. Aborts on failure. | |
| */ | |
| void FlushBlockFile(bool fFinalize, bool finalize_undo); | |
| ++ b/src/node/blockstorage.cpp | |
| @@ | |
| bool BlockManager::FlushBlockFile(int block_file, bool fFinalize, bool finalize_undo) | |
| { | |
| LOCK(cs_LastBlockFile); | |
| FlatFilePos block_pos_old(block_file, m_blockfile_info[block_file].nSize); | |
| if (!BlockFileSeq().Flush(block_pos_old, fFinalize)) { | |
| return false; | |
| } | |
| // We do not always flush the undo file, as the chain tip may be lagging behind. | |
| if (!fFinalize || finalize_undo) { | |
| FlushUndoFile(block_file, finalize_undo); | |
| } | |
| return true; | |
| } | |
| @@ | |
| void BlockManager::FlushBlockFile(bool fFinalize, bool finalize_undo) | |
| { | |
| - LOCK(cs_LastBlockFile); | |
| - FlatFilePos block_pos_old(m_last_blockfile, m_blockfile_info[m_last_blockfile].nSize); | |
| - if (!BlockFileSeq().Flush(block_pos_old, fFinalize)) { | |
| - AbortNode("Flushing block file to disk failed. This is likely the result of an I/O error."); | |
| - } | |
| - // we do not always flush the undo file... | |
| // Delegate to the new overload; abort on failure. | |
| if (!FlushBlockFile(m_last_blockfile, fFinalize, finalize_undo)) { | |
| AbortNode("Flushing block file to disk failed. This is likely the result of an I/O error."); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/node/blockstorage.cpp around lines 644-656, the code calls a non-existent
FlushBlockFile overload; declare a new overload in src/node/blockstorage.h that
accepts a file index and returns bool, then implement that function in
src/node/blockstorage.cpp near the existing FlushBlockFile() code so it flushes
the specified block file and returns success/failure (without aborting), and
update the legacy two-arg void FlushBlockFile(bool fFinalize, bool
finalize_undo) to call or forward to the new overload (or vice‑versa) to
preserve existing AbortNode semantics where required; this ensures the if
(!FlushBlockFile(m_last_blockfile, ...)) check compiles and behaves like
upstream (log-and-continue on failure).
| self.log.debug("Make the first block file read-only") | ||
| filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat" | ||
| filename.chmod(stat.S_IREAD) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Preserve original file mode; avoid hard-coding 0o400
Capture and restore the original mode so the test doesn’t leave altered permissions on failure paths.
Apply:
- filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat"
- filename.chmod(stat.S_IREAD)
+ filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat"
+ orig_mode = stat.S_IMODE(filename.stat().st_mode)
+ filename.chmod(0o444) # read-only for allAnd at cleanup (see Lines 83-84 change below) restore orig_mode.
📝 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.
| self.log.debug("Make the first block file read-only") | |
| filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat" | |
| filename.chmod(stat.S_IREAD) | |
| self.log.debug("Make the first block file read-only") | |
| filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat" | |
| orig_mode = stat.S_IMODE(filename.stat().st_mode) | |
| filename.chmod(0o444) # read-only for all |
🤖 Prompt for AI Agents
In test/functional/feature_reindex_readonly.py around lines 33 to 36, the test
currently hard-codes a readonly mode (stat.S_IREAD) and doesn't preserve the
file's original permissions; change it to capture the file's current mode
(orig_mode = filename.stat().st_mode) before calling chmod, set the readonly
mode for the test, and then ensure the cleanup code (around lines 83-84)
restores the original mode by calling filename.chmod(orig_mode) so the test does
not leave altered permissions on failures.
| undo_immutable = lambda: None | ||
| # Linux | ||
| try: | ||
| subprocess.run(['chattr'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) | ||
| try: | ||
| subprocess.run(['chattr', '+i', filename], capture_output=True, check=True) | ||
| undo_immutable = lambda: subprocess.check_call(['chattr', '-i', filename]) | ||
| self.log.info("Made file immutable with chattr") | ||
| except subprocess.CalledProcessError as e: |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Replace lambda sentinel with explicit callable and check tools via shutil.which
Avoid assigning lambdas (style E731) and prefer checking tool availability with shutil.which to skip meaningless subprocess calls. Also pass text=True so logged stdout/stderr are strings.
- undo_immutable = lambda: None
+ undo_immutable = None
# Linux
try:
- subprocess.run(['chattr'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
+ from shutil import which
+ if which('chattr') is None:
+ raise FileNotFoundError('chattr not found')
try:
- subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)
- undo_immutable = lambda: subprocess.check_call(['chattr', '-i', filename])
+ subprocess.run(['chattr', '+i', str(filename)], capture_output=True, check=True, text=True)
+ def _undo():
+ subprocess.check_call(['chattr', '-i', str(filename)])
+ undo_immutable = _undo
self.log.info("Made file immutable with chattr")
except subprocess.CalledProcessError as e:
self.log.warning(str(e))
if e.stdout:
self.log.warning(f"stdout: {e.stdout}")
if e.stderr:
self.log.warning(f"stderr: {e.stderr}")
if os.getuid() == 0:
self.log.warning("Return early on Linux under root, because chattr failed.")
self.log.warning("This should only happen due to missing capabilities in a container.")
self.log.warning("Make sure to --cap-add LINUX_IMMUTABLE if you want to run this test.")
- undo_immutable = False
+ undo_immutable = FalseCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.12.2)
37-37: Do not assign a lambda expression, use a def
Rewrite undo_immutable as a def
(E731)
40-40: Starting a process with a partial executable path
(S607)
42-42: subprocess call: check for execution of untrusted input
(S603)
42-42: Starting a process with a partial executable path
(S607)
43-43: Do not assign a lambda expression, use a def
Rewrite undo_immutable as a def
(E731)
43-43: subprocess call: check for execution of untrusted input
(S603)
43-43: Starting a process with a partial executable path
(S607)
🪛 Flake8 (7.2.0)
[error] 37-37: do not assign a lambda expression, use a def
(E731)
[error] 43-43: do not assign a lambda expression, use a def
(E731)
🤖 Prompt for AI Agents
In test/functional/feature_reindex_readonly.py around lines 37 to 45, replace
the lambda sentinel and blind subprocess calls with an explicit callable and a
proper existence check: use shutil.which('chattr') to skip the chattr block when
the tool is not present, define undo_immutable as a named function (or
functools.partial) that calls subprocess.check_call(['chattr', '-i', filename])
instead of lambda: None, and when invoking subprocess.run use text=True (or
encoding='utf-8') so stdout/stderr are strings and capture_output=True before
logging them; also catch subprocess.CalledProcessError and log e.stdout and
e.stderr (now strings) for diagnostics.
| except Exception: | ||
| # macOS, and *BSD | ||
| try: | ||
| subprocess.run(['chflags'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) | ||
| try: | ||
| subprocess.run(['chflags', 'uchg', filename], capture_output=True, check=True) | ||
| undo_immutable = lambda: subprocess.check_call(['chflags', 'nouchg', filename]) | ||
| self.log.info("Made file immutable with chflags") | ||
| except subprocess.CalledProcessError as e: | ||
| self.log.warning(str(e)) | ||
| if e.stdout: | ||
| self.log.warning(f"stdout: {e.stdout}") | ||
| if e.stderr: | ||
| self.log.warning(f"stderr: {e.stderr}") | ||
| if os.getuid() == 0: | ||
| self.log.warning("Return early on BSD under root, because chflags failed.") | ||
| undo_immutable = False | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
BSD/macOS path: mirror subprocess safety and avoid broad except
Same improvements as above: which('chflags'), pass text=True, and avoid catching bare Exception unless you log at least the message.
- except Exception:
+ except Exception:
# macOS, and *BSD
try:
- subprocess.run(['chflags'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
+ from shutil import which
+ if which('chflags') is None:
+ raise FileNotFoundError('chflags not found')
try:
- subprocess.run(['chflags', 'uchg', filename], capture_output=True, check=True)
- undo_immutable = lambda: subprocess.check_call(['chflags', 'nouchg', filename])
+ subprocess.run(['chflags', 'uchg', str(filename)], capture_output=True, check=True, text=True)
+ def _undo():
+ subprocess.check_call(['chflags', 'nouchg', str(filename)])
+ undo_immutable = _undo
self.log.info("Made file immutable with chflags")
except subprocess.CalledProcessError as e:
self.log.warning(str(e))
if e.stdout:
self.log.warning(f"stdout: {e.stdout}")
if e.stderr:
self.log.warning(f"stderr: {e.stderr}")
if os.getuid() == 0:
self.log.warning("Return early on BSD under root, because chflags failed.")
undo_immutable = False
- except Exception:
- pass
+ except Exception as e:
+ self.log.debug(f"Skipping immutability on this platform: {e}")📝 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.
| except Exception: | |
| # macOS, and *BSD | |
| try: | |
| subprocess.run(['chflags'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) | |
| try: | |
| subprocess.run(['chflags', 'uchg', filename], capture_output=True, check=True) | |
| undo_immutable = lambda: subprocess.check_call(['chflags', 'nouchg', filename]) | |
| self.log.info("Made file immutable with chflags") | |
| except subprocess.CalledProcessError as e: | |
| self.log.warning(str(e)) | |
| if e.stdout: | |
| self.log.warning(f"stdout: {e.stdout}") | |
| if e.stderr: | |
| self.log.warning(f"stderr: {e.stderr}") | |
| if os.getuid() == 0: | |
| self.log.warning("Return early on BSD under root, because chflags failed.") | |
| undo_immutable = False | |
| except Exception: | |
| pass | |
| except Exception: | |
| # macOS, and *BSD | |
| try: | |
| from shutil import which | |
| if which('chflags') is None: | |
| raise FileNotFoundError('chflags not found') | |
| try: | |
| subprocess.run(['chflags', 'uchg', str(filename)], capture_output=True, check=True, text=True) | |
| def _undo(): | |
| subprocess.check_call(['chflags', 'nouchg', str(filename)]) | |
| undo_immutable = _undo | |
| self.log.info("Made file immutable with chflags") | |
| except subprocess.CalledProcessError as e: | |
| self.log.warning(str(e)) | |
| if e.stdout: | |
| self.log.warning(f"stdout: {e.stdout}") | |
| if e.stderr: | |
| self.log.warning(f"stderr: {e.stderr}") | |
| if os.getuid() == 0: | |
| self.log.warning("Return early on BSD under root, because chflags failed.") | |
| undo_immutable = False | |
| except Exception as e: | |
| self.log.debug(f"Skipping immutability on this platform: {e}") |
🧰 Tools
🪛 Ruff (0.12.2)
56-56: Do not catch blind exception: Exception
(BLE001)
59-59: Starting a process with a partial executable path
(S607)
61-61: subprocess call: check for execution of untrusted input
(S603)
61-61: Starting a process with a partial executable path
(S607)
62-62: Do not assign a lambda expression, use a def
Rewrite undo_immutable as a def
(E731)
62-62: subprocess call: check for execution of untrusted input
(S603)
62-62: Starting a process with a partial executable path
(S607)
67-67: Logging statement uses f-string
(G004)
69-69: Logging statement uses f-string
(G004)
73-74: try-except-pass detected, consider logging the exception
(S110)
73-73: Do not catch blind exception: Exception
(BLE001)
🪛 Flake8 (7.2.0)
[error] 62-62: do not assign a lambda expression, use a def
(E731)
| if undo_immutable: | ||
| self.log.debug("Attempt to restart and reindex the node with the unwritable block file") | ||
| with self.nodes[0].wait_for_debug_log([b"Reindexing finished"]): | ||
| self.start_node(0, extra_args=['-reindex', '-fastprune']) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid duplicating -fastprune on restart
-fastprune is already in self.extra_args. Passing it again is harmless but noisy.
- self.start_node(0, extra_args=['-reindex', '-fastprune'])
+ self.start_node(0, extra_args=['-reindex'])📝 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.
| self.start_node(0, extra_args=['-reindex', '-fastprune']) | |
| self.start_node(0, extra_args=['-reindex']) |
🤖 Prompt for AI Agents
In test/functional/feature_reindex_readonly.py around line 79, the call to
self.start_node(0, extra_args=['-reindex', '-fastprune']) duplicates
'-fastprune' because it is already present in self.extra_args; remove the
redundant '-fastprune' from the restart call and pass only the necessary
'-reindex' (or merge with self.extra_args so duplicates are not re-specified) to
keep logs clean.
| filename.chmod(0o777) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restore original file mode instead of 0o777
Restoring to 0o777 is overly permissive and may pollute subsequent tests. Use previously captured mode.
- filename.chmod(0o777)
+ filename.chmod(orig_mode)📝 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.
| filename.chmod(0o777) | |
| filename.chmod(orig_mode) |
🤖 Prompt for AI Agents
In test/functional/feature_reindex_readonly.py around lines 83-84, the test
currently resets the file mode to a hardcoded 0o777 which is too permissive;
instead capture the original file mode before changing permissions and restore
that captured mode at the end of the test (use the stored mode value rather than
0o777) so the test returns the file to its original permissions.
|
Issues Found:
Reason for Manual Intervention:
Recommendation: 🚫 CI Check: 6 jobs failing (threshold: max 1 allowed) |
DashCoreAutoGuix
added
the
needs-fixes
Backports bitcoin#27039
Original commit: 69ddee6
Backported from Bitcoin Core v0.28
Summary by CodeRabbit