backport: Merge bitcoin#29041, 29034#7190
Open
vijaydasmp wants to merge 1 commit into
Open
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
March_2026_3
branch
4 times, most recently
from
32d86aa to
dbe3516
Compare
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
March_2026_3
branch
4 times, most recently
from
4c7ddba to
7e96446
Compare
vijaydasmp
changed the title
|
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 (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughAcross the functional test suite, all platform detection calls are standardized to use Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/miner.cpp`:
- Line 24: Add an explicit include for timedata.h in miner.cpp to stop relying
on transitive includes: include <timedata.h> near the top of the file so that
GetAdjustedTime() (used in miner.cpp at the calls around GetAdjustedTime()
lines) is declared directly rather than via governance/governance.h →
coinjoin/coinjoin.h; update the include block in miner.cpp to add <timedata.h>
alongside the existing <util/time.h>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fa03ef5f-d178-4efc-9a1e-f0a6be8c6193
📒 Files selected for processing (15)
src/chain.hsrc/net_processing.cppsrc/node/miner.cppsrc/rpc/mining.cppsrc/timedata.htest/functional/README.mdtest/functional/feature_bind_extra.pytest/functional/feature_init.pytest/functional/feature_notifications.pytest/functional/rpc_bind.pytest/functional/rpc_net.pytest/functional/test_framework/p2p.pytest/functional/test_framework/test_node.pytest/functional/test_runner.pytest/functional/wallet_multiwallet.py
💤 Files with no reviewable changes (1)
- src/timedata.h
| #include <timedata.h> | ||
| #include <util/moneystr.h> | ||
| #include <util/system.h> | ||
| #include <util/time.h> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
import re
repo = Path('.')
miner = repo / 'src/node/miner.cpp'
inc_re = re.compile(r'#include <([^>]+)>')
includes = []
for line in miner.read_text().splitlines():
m = inc_re.match(line.strip())
if m:
includes.append(m.group(1))
if line.strip() == 'namespace node {':
break
print("Direct includes in src/node/miner.cpp:")
for inc in includes:
print(f" {inc}")
print("\nDeclaration/definition sites for GetAdjustedTime():")
for path in sorted(repo.glob('src/**/*')):
if path.suffix not in {'.h', '.hpp', '.hh', '.c', '.cc', '.cpp'}:
continue
text = path.read_text(errors='ignore')
for lineno, line in enumerate(text.splitlines(), 1):
if 'GetAdjustedTime(' in line:
print(f" {path}:{lineno}: {line.strip()}")
print("\nDirectly included headers that reference GetAdjustedTime() or timedata.h:")
for inc in includes:
for path in sorted(repo.glob(f'**/{Path(inc).name}')):
if not path.is_file():
continue
text = path.read_text(errors='ignore')
hits = [
(lineno, line.strip())
for lineno, line in enumerate(text.splitlines(), 1)
if 'GetAdjustedTime(' in line or 'timedata.h' in line
]
if hits:
print(f" {path}")
for lineno, line in hits:
print(f" {lineno}: {line}")
PYRepository: dashpay/dash
Length of output: 4490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check which directly included headers transitively include timedata.h
direct_includes=(
"node/miner.h"
"chain.h"
"chainparams.h"
"consensus/amount.h"
"consensus/consensus.h"
"consensus/merkle.h"
"consensus/tx_verify.h"
"consensus/validation.h"
"deploymentstatus.h"
"node/context.h"
"policy/feerate.h"
"policy/policy.h"
"pow.h"
"primitives/transaction.h"
"util/moneystr.h"
"util/system.h"
"util/time.h"
"validation.h"
"chainlock/chainlock.h"
"chainlock/handler.h"
"evo/specialtx.h"
"evo/cbtx.h"
"evo/chainhelper.h"
"evo/creditpool.h"
"evo/mnhftx.h"
"evo/deterministicmns.h"
"evo/simplifiedmns.h"
"evo/specialtxman.h"
"governance/governance.h"
"instantsend/instantsend.h"
"llmq/blockprocessor.h"
"llmq/context.h"
"llmq/options.h"
"llmq/snapshot.h"
"masternode/payments.h"
)
echo "Checking which direct includes transitively include timedata.h:"
for inc in "${direct_includes[@]}"; do
# Find the file
file_path=$(find src -name "$(basename "$inc")" -path "*${inc}" 2>/dev/null | head -1)
if [ -f "$file_path" ]; then
if grep -q "timedata\.h" "$file_path"; then
echo " ✓ $inc includes timedata.h"
grep -n "timedata\.h" "$file_path"
fi
fi
done
echo ""
echo "Checking src/node/miner.h specifically:"
if [ -f "src/node/miner.h" ]; then
cat -n src/node/miner.h | head -50
fiRepository: dashpay/dash
Length of output: 1660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check full miner.h
echo "=== Full src/node/miner.h ==="
cat src/node/miner.h
echo ""
echo "=== Checking validation.h for timedata.h inclusion ==="
grep -n "timedata.h" src/validation.h || echo "Not found in validation.h"
echo ""
echo "=== Checking node/context.h for timedata.h inclusion ==="
grep -n "timedata.h" src/node/context.h || echo "Not found in node/context.h"
echo ""
echo "=== Checking all .h files in src/node/ for timedata.h ==="
grep -r "timedata.h" src/node/ || echo "Not found in src/node/"
echo ""
echo "=== Checking governance/governance.h (uses GetAdjustedTime) ==="
grep -n "timedata.h" src/governance/governance.h || echo "Not found in governance.h"
grep -n "#include" src/governance/governance.h | head -20Repository: dashpay/dash
Length of output: 8763
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's trace the include chain more carefully
echo "=== Searching for timedata.h in all header includes ==="
python3 << 'PYSCRIPT'
import os
import re
from pathlib import Path
def find_transitive_includes(start_file, max_depth=5):
"""Recursively find all includes from a file."""
visited = set()
includes = {}
def traverse(filepath, depth=0):
if depth > max_depth or filepath in visited:
return
visited.add(filepath)
if not filepath.exists():
return
try:
content = filepath.read_text(errors='ignore')
except:
return
includes[str(filepath)] = []
for line in content.split('\n'):
match = re.match(r'#include\s*[<"]([^>"]+)[>"]', line.strip())
if match:
inc = match.group(1)
includes[str(filepath)].append(inc)
# Try to resolve the include
inc_path = filepath.parent / inc
if inc_path.exists():
traverse(inc_path, depth + 1)
traverse(start_file)
return includes
# Check validation.h (one of the major includes)
validation_h = Path('src/validation.h')
print("Checking validation.h includes for timedata.h...")
includes = find_transitive_includes(validation_h)
found_timedata = False
for file, incs in includes.items():
if 'timedata.h' in incs:
print(f" Found in {file}: {[i for i in incs if 'timedata' in i]}")
found_timedata = True
if not found_timedata:
print(" timedata.h not found in validation.h include chain")
# Check governance/governance.h (uses GetAdjustedTime)
print("\nChecking governance/governance.h includes...")
gov_h = Path('src/governance/governance.h')
if gov_h.exists():
includes = find_transitive_includes(gov_h)
found_timedata = False
for file, incs in includes.items():
if 'timedata.h' in incs:
print(f" Found in {file}: {[i for i in incs if 'timedata' in i]}")
found_timedata = True
if not found_timedata:
print(" timedata.h not found in governance/governance.h include chain")
PYSCRIPT
echo ""
echo "=== Direct search for which header includes timedata.h ==="
grep -r "^\s*#include.*timedata\.h" src/ | head -20Repository: dashpay/dash
Length of output: 1238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's check if this can actually compile by looking at what governance.h includes
echo "=== Full governance/governance.h include chain ==="
head -30 src/governance/governance.h
echo ""
echo "=== Check if any major included header has timedata in its transitive chain ==="
# Let's check a few key headers
for header in "src/util/system.h" "src/validation.h" "src/governance/governance.h" "src/coinjoin/coinjoin.h"; do
if [ -f "$header" ]; then
echo "=== $header (first 50 lines) ==="
head -50 "$header"
echo ""
fi
done
echo ""
echo "=== Check if governance/governance.h is included in miner.cpp ==="
grep -n "governance/governance.h" src/node/miner.cppRepository: dashpay/dash
Length of output: 5938
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking governance/signing.h for timedata.h ==="
head -40 src/governance/signing.h | grep -E "^#include"
echo ""
echo "=== Full governance/signing.h includes ==="
grep "^#include" src/governance/signing.h
echo ""
echo "=== Checking governance/governance.cpp (impl) for timedata ==="
grep -n "timedata" src/governance/governance.cpp | head -5
echo ""
echo "=== Now let's check if protocol.h or sync.h include timedata ==="
grep "timedata.h" src/protocol.h src/sync.h 2>/dev/null || echo "Not found in protocol.h or sync.h"
echo ""
echo "=== Check all files directly included by miner.cpp for timedata chain ==="
python3 << 'PY'
import re
from pathlib import Path
miner_cpp = Path('src/node/miner.cpp')
includes = []
# Extract direct includes from miner.cpp
for line in miner_cpp.read_text().split('\n'):
m = re.match(r'#include <([^>]+)>', line.strip())
if m:
includes.append(m.group(1))
if 'namespace node {' in line:
break
# For each include, check if it (or its transitive includes) reference timedata
print("Checking which direct includes transitively lead to timedata.h definition/declaration:\n")
def check_include(inc, visited=None, depth=0):
if visited is None:
visited = set()
if depth > 3 or inc in visited:
return False
visited.add(inc)
# Try to find the file
candidates = list(Path('src').glob(f'**/{Path(inc).name}'))
for cand in candidates:
if str(cand).endswith(inc) or cand.name == Path(inc).name:
try:
content = cand.read_text(errors='ignore')
# Check if this file declares or includes timedata
if 'GetAdjustedTime' in content:
return True
# Check includes in this file
for line in content.split('\n'):
m = re.match(r'#include <([^>]+)>', line.strip())
if m:
if check_include(m.group(1), visited, depth + 1):
return True
except:
pass
return False
for inc in includes[:10]: # Check first 10
if check_include(inc):
print(f" ✓ {inc} -> leads to GetAdjustedTime definition")
PYRepository: dashpay/dash
Length of output: 784
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check governance/object.h for timedata inclusion ==="
grep "timedata\|GetAdjustedTime" src/governance/object.h
echo ""
echo "=== Check governance/classes.h for timedata inclusion ==="
grep "timedata\|GetAdjustedTime" src/governance/classes.h
echo ""
echo "=== Let's trace what includes governance/classes.h ==="
head -30 src/governance/classes.h | grep "#include"
echo ""
echo "=== Now check if miner.cpp would actually compile as-is ==="
echo "Looking at actual usage in miner.cpp..."
grep -B2 -A2 "GetAdjustedTime" src/node/miner.cpp
echo ""
echo "=== Final check: Verify coinjoin.h is in miner.cpp includes ==="
grep "coinjoin" src/node/miner.cppRepository: dashpay/dash
Length of output: 112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check what files exist in governance/ ==="
ls -la src/governance/ | grep -E "\.(h|cpp)$"
echo ""
echo "=== Check if governance/object.h exists ==="
if [ -f src/governance/object.h ]; then
echo "Found - checking includes..."
head -30 src/governance/object.h | grep "#include"
else
echo "Not found, checking governance/ directory..."
find src/governance -name "*.h" | head -10
fi
echo ""
echo "=== Direct check: does miner.cpp compile without timedata.h? ==="
echo "Looking at miner.cpp lines 50-55 (around first GetAdjustedTime call):"
sed -n '50,55p' src/node/miner.cpp
echo ""
echo "=== Looking at miner.cpp lines 218-225 (around second GetAdjustedTime call): ==="
sed -n '218,225p' src/node/miner.cpp
echo ""
echo "=== Trace evo/chainhelper.h since it's included in miner.cpp ==="
head -40 src/evo/chainhelper.h | grep "#include"Repository: dashpay/dash
Length of output: 2565
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check governance/vote.h ==="
head -30 src/governance/vote.h | grep "#include"
echo ""
echo "=== Check governance/votedb.h ==="
head -30 src/governance/votedb.h | grep "#include"
echo ""
echo "=== Comprehensive trace: which header in the chain includes timedata.h ==="
python3 << 'PY'
import re
from pathlib import Path
def get_direct_includes(filepath):
"""Get direct includes from a file."""
try:
content = Path(filepath).read_text(errors='ignore')
includes = []
for line in content.split('\n'):
m = re.match(r'#include <([^>]+)>', line.strip())
if m:
includes.append(m.group(1))
return includes
except:
return []
# Start from governance.h and trace through
chain = ['governance/governance.h']
visited = set()
depth = 0
print("Tracing include chain from governance/governance.h:")
print(f"0: governance/governance.h")
while chain and depth < 5:
current = chain.pop(0)
if current in visited:
continue
visited.add(current)
incs = get_direct_includes(f'src/{current}')
for inc in incs:
print(f" → {inc}")
if 'timedata.h' in inc:
print(f"\n✓ FOUND: {current} includes timedata.h!")
# Follow governance/* includes
if 'governance/' in inc or 'vote' in inc:
chain.append(inc)
depth += 1
PY
echo ""
echo "=== Now verify: check if governance/vote.h includes timedata ==="
grep "timedata" src/governance/vote.h || echo "No timedata in vote.h"Repository: dashpay/dash
Length of output: 1150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check util/time.h fully ==="
cat src/util/time.h
echo ""
echo "=== Check util/system.h for timedata ==="
grep "timedata" src/util/system.h
echo ""
echo "=== Final check: Does coinjoin/coinjoin.h (which includes timedata.h) get included transitively? ==="
python3 << 'PY'
import re
from pathlib import Path
def find_include_path(start_file, target_include, visited=None, path=None):
"""Find if target_include is reachable from start_file."""
if visited is None:
visited = set()
if path is None:
path = []
if start_file in visited:
return None
visited.add(start_file)
path = path + [start_file]
try:
content = Path(f'src/{start_file}').read_text(errors='ignore')
except:
return None
for line in content.split('\n'):
m = re.match(r'#include <([^>]+)>', line.strip())
if m:
inc = m.group(1)
if inc == target_include:
return path + [inc]
# Try to follow the include
result = find_include_path(inc, target_include, visited, path)
if result:
return result
return None
# Check if timedata.h is reachable from any of miner.cpp's direct includes
miner_includes = [
'node/miner.h',
'chain.h',
'validation.h',
'governance/governance.h',
'evo/chainhelper.h'
]
print("Checking if timedata.h is reachable from miner.cpp's includes:\n")
for inc in miner_includes:
path = find_include_path(inc, 'timedata.h')
if path:
print(f"✓ {' → '.join(path)}")
PYRepository: dashpay/dash
Length of output: 5448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== One final definitive check: Search for any other GetAdjustedTime declaration ==="
grep -r "GetAdjustedTime" src/ --include="*.h" | grep -E "declare|inline|extern"
echo ""
echo "=== Check if timedata.h is the ONLY place GetAdjustedTime is declared ==="
grep -r "int64_t GetAdjustedTime" src/ --include="*.h"
echo ""
echo "=== Verify the actual include structure being built in miner.cpp ==="
# Let's read the actual miner.cpp to see the full include section
head -45 src/node/miner.cppRepository: dashpay/dash
Length of output: 140
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's be very explicit and simple
echo "=== Is GetAdjustedTime declared in timedata.h? ==="
grep -n "GetAdjustedTime" src/timedata.h
echo ""
echo "=== Is timedata.h included in miner.cpp? ==="
grep -n "timedata.h" src/node/miner.cpp || echo "(not found)"
echo ""
echo "=== Full include block of miner.cpp ==="
sed -n '1,45p' src/node/miner.cpp
echo ""
echo "=== Summary: Which headers declare GetAdjustedTime() ==="
grep -l "GetAdjustedTime" src/*.hRepository: dashpay/dash
Length of output: 1708
🏁 Script executed:
#!/bin/bash
set -euoripefail
echo "=== Check governance/classes.h includes ==="
grep "#include" src/governance/classes.h
echo ""
echo "=== Check if any Dash subsystem header (evo, llmq, etc) includes timedata transitively ==="
for header in src/evo/*.h src/llmq/*.h; do
if grep -q "timedata" "$header" 2>/dev/null; then
echo "Found timedata ref in: $header"
grep -n "timedata" "$header"
fi
done
echo ""
echo "=== Most important: Check if coinjoin/coinjoin.h is included transitively ==="
python3 << 'PY'
import re
from pathlib import Path
# Check if coinjoin.h is included transitively from miner.cpp's direct includes
def find_all_includes(filepath, visited=None, depth=0):
"""Recursively find ALL includes."""
if visited is None:
visited = set()
if depth > 4:
return set()
if filepath in visited:
return set()
visited.add(filepath)
try:
content = Path(filepath).read_text(errors='ignore')
except:
return set()
all_incs = set()
for line in content.split('\n'):
m = re.match(r'#include <([^>]+)>', line.strip())
if m:
inc = m.group(1)
all_incs.add(inc)
# Recursively find includes from this include
for candidate in Path('src').glob(f'**/{Path(inc).name}'):
if candidate.is_file():
all_incs.update(find_all_includes(str(candidate), visited, depth+1))
return all_incs
# Check a few key direct includes from miner.cpp
miner_direct = [
'governance/governance.h',
'chain.h',
'validation.h',
'evo/chainhelper.h'
]
print("Checking if coinjoin/coinjoin.h is transitively included:")
for inc in miner_direct:
filepath = f'src/{inc}'
if Path(filepath).exists():
all_incs = find_all_includes(filepath)
if 'coinjoin/coinjoin.h' in all_incs or 'coinjoin.h' in all_incs:
print(f" ✓ {inc} -> includes coinjoin/coinjoin.h")
if 'timedata.h' in all_incs:
print(f" ✓ {inc} -> includes timedata.h")
PYRepository: dashpay/dash
Length of output: 730
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check governance/classes.h includes ==="
grep "#include" src/governance/classes.h
echo ""
echo "=== Check if any Dash subsystem header (evo, llmq, etc) includes timedata transitively ==="
for header in src/evo/*.h src/llmq/*.h; do
if grep -q "timedata" "$header" 2>/dev/null; then
echo "Found timedata ref in: $header"
grep -n "timedata" "$header"
fi
done
echo ""
echo "=== Most important: Check if coinjoin/coinjoin.h is included transitively ==="
python3 << 'PY'
import re
from pathlib import Path
# Check if coinjoin.h is included transitively from miner.cpp's direct includes
def find_all_includes(filepath, visited=None, depth=0):
"""Recursively find ALL includes."""
if visited is None:
visited = set()
if depth > 4:
return set()
if filepath in visited:
return set()
visited.add(filepath)
try:
content = Path(filepath).read_text(errors='ignore')
except:
return set()
all_incs = set()
for line in content.split('\n'):
m = re.match(r'#include <([^>]+)>', line.strip())
if m:
inc = m.group(1)
all_incs.add(inc)
# Recursively find includes from this include
for candidate in Path('src').glob(f'**/{Path(inc).name}'):
if candidate.is_file():
all_incs.update(find_all_includes(str(candidate), visited, depth+1))
return all_incs
# Check a few key direct includes from miner.cpp
miner_direct = [
'governance/governance.h',
'chain.h',
'validation.h',
'evo/chainhelper.h'
]
print("Checking if coinjoin/coinjoin.h is transitively included:")
for inc in miner_direct:
filepath = f'src/{inc}'
if Path(filepath).exists():
all_incs = find_all_includes(filepath)
if 'coinjoin/coinjoin.h' in all_incs or 'coinjoin.h' in all_incs:
print(f" ✓ {inc} -> includes coinjoin/coinjoin.h")
if 'timedata.h' in all_incs:
print(f" ✓ {inc} -> includes timedata.h")
PYRepository: dashpay/dash
Length of output: 580
Add explicit #include <timedata.h> to avoid relying on transitive includes.
GetAdjustedTime() is declared only in timedata.h and used at lines 52 and 221, but this TU currently depends on a transitive include chain (governance/governance.h → coinjoin/coinjoin.h → timedata.h). While this compiles, it's fragile: any changes to coinjoin/coinjoin.h's includes would silently break this compilation unit.
Suggested fix
`#include` <node/miner.h>
`#include` <chain.h>
+#include <timedata.h>
`#include` <chainparams.h>🧰 Tools
🪛 Cppcheck (2.19.0)
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/node/miner.cpp` at line 24, Add an explicit include for timedata.h in
miner.cpp to stop relying on transitive includes: include <timedata.h> near the
top of the file so that GetAdjustedTime() (used in miner.cpp at the calls around
GetAdjustedTime() lines) is declared directly rather than via
governance/governance.h → coinjoin/coinjoin.h; update the include block in
miner.cpp to add <timedata.h> alongside the existing <util/time.h>.
There was a problem hiding this comment.
Good catch! We need at least 28956 first.
There was a problem hiding this comment.
@UdjinM6, understood — thank you for the context!
bitcoin#28956 ("Nuke adjusted time from validation") is the PR that removes timedata.h entirely (including its GetAdjustedTime() declaration) and moves the network-adjusted time logic into net_processing, with the src/node/miner.cpp include swap (timedata.h → util/time.h) being part of that same PR.
So the include change in this backport is essentially a cherry-picked fragment of bitcoin#28956 — and without the rest of that PR (which moves/removes GetAdjustedTime() itself), miner.cpp would still be relying on a transitive include for the declaration. The right path is to land bitcoin#28956 first so the full refactoring is in place, making this include change coherent. I'll withdraw my suggestion to add <timedata.h> back since the intended resolution is the broader upstream backport.
✏️ Learnings added
Learnt from: UdjinM6
Repo: dashpay/dash PR: 7190
File: src/node/miner.cpp:24-24
Timestamp: 2026-04-09T12:25:27.445Z
Learning: In dashpay/dash, the include change in `src/node/miner.cpp` that swaps `#include <timedata.h>` for `#include <util/time.h>` (as seen in PR `#7190`) is a fragment of upstream bitcoin/bitcoin#28956 ("Nuke adjusted time from validation"). This change is only valid after bitcoin#28956 is fully backported, as that PR is what actually moves/removes `GetAdjustedTime()` out of `timedata.h` and into `util/time.h`. Do not flag the missing `timedata.h` include in `miner.cpp` as a standalone issue; it requires bitcoin#28956 to be backported first.
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: UdjinM6
Repo: dashpay/dash PR: 7265
File: src/wallet/wallet.cpp:3381-3382
Timestamp: 2026-04-03T14:17:17.656Z
Learning: In Dash Core, `strprintf()` uses `tinyformat` (type-safe printf). Do not flag format-specifier mismatches for integer types when using `strprintf()`: `%d` is correct for any integer type, including `CAmount`/`int64_t` arguments. Only treat `%d`/integer combinations as a formatting issue if they occur in a non-`strprintf()` (e.g., libc `printf`) context or there is evidence the format string is not being handled by `tinyformat`.
Author
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified 1 finding: the backported include cleanup from bitcoin#29361 removes timedata.h from src/node/miner.cpp, but the file still calls GetAdjustedTime() which is declared only in that header. This should break compilation unless another transitive path exists.
Reviewed commit: 7e96446
🟡 1 suggestion(s)
1 additional finding
🟡 suggestion: `src/node/miner.cpp` loses the declaration of `GetAdjustedTime()` when `timedata.h` is removed
src/node/miner.cpp (lines 1-10)
Verified in the PR diff against upstream/v23.1.x: src/node/miner.cpp drops #include <timedata.h> and adds #include <util/time.h>. The file still calls GetAdjustedTime() at src/node/miner.cpp:52 and src/node/miner.cpp:221. GetAdjustedTime() is declared in src/timedata.h:75, and src/util/time.h does not declare it. A header search shows only src/coinjoin/coinjoin.h and src/addrman_impl.h include timedata.h; miner.cpp does not include either directly or through its obvious remaining direct headers (node/miner.h, validation.h, chain.h, node/context.h, pow.h, util/system.h, util/time.h). So the transitive-include explanation is not supported by the source, and this change should break compilation of the miner.cpp translation unit unless #include <timedata.h> is restored.
🤖 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/miner.cpp`:
- [SUGGESTION] lines 1-10: `src/node/miner.cpp` loses the declaration of `GetAdjustedTime()` when `timedata.h` is removed
Verified in the PR diff against `upstream/v23.1.x`: `src/node/miner.cpp` drops `#include <timedata.h>` and adds `#include <util/time.h>`. The file still calls `GetAdjustedTime()` at `src/node/miner.cpp:52` and `src/node/miner.cpp:221`. `GetAdjustedTime()` is declared in `src/timedata.h:75`, and `src/util/time.h` does not declare it. A header search shows only `src/coinjoin/coinjoin.h` and `src/addrman_impl.h` include `timedata.h`; `miner.cpp` does not include either directly or through its obvious remaining direct headers (`node/miner.h`, `validation.h`, `chain.h`, `node/context.h`, `pow.h`, `util/system.h`, `util/time.h`). So the transitive-include explanation is not supported by the source, and this change should break compilation of the `miner.cpp` translation unit unless `#include <timedata.h>` is restored.
|
✅ Review complete (commit 517c643) |
Author
|
Hi @PastaPastaPasta , requesting review |
thepastaclaw
reviewed
May 9, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Single convergent finding from both agents: src/node/miner.cpp dropped the direct #include <timedata.h> as part of the bitcoin#29361 backport but still calls GetAdjustedTime() (declared only in timedata.h), so the file now compiles only via a transitive include. This is brittle but non-blocking. Test-only backports (bitcoin#29041, bitcoin#29034) look correctly adapted.
Reviewed commit: 7e96446
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: 7e96446
🟡 1 suggestion(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/miner.cpp`:
- [SUGGESTION] line 24: miner.cpp uses GetAdjustedTime() but no longer directly includes <timedata.h>
The bitcoin#29361 backport replaced `#include <timedata.h>` with `#include <util/time.h>` in this file, but `GetAdjustedTime()` is still called at src/node/miner.cpp:52 and src/node/miner.cpp:221 and is declared only in src/timedata.h:75 (verified: no declaration in util/time.h). Upstream could safely drop the include because Bitcoin removed `GetAdjustedTime()` itself in commit ff9039f6ea (#25908) and replaced it with `ChainstateManager::m_adjusted_time_callback`; Dash has not backported that removal. The translation unit therefore compiles only by way of an incidental transitive include, which an unrelated future header cleanup could turn into a hard build break. Other files affected by the same backport are unaffected (src/net_processing.cpp keeps a direct `#include <timedata.h>`; src/rpc/mining.cpp does not call `GetAdjustedTime()`). The minimal, safe Dash adaptation is to keep the direct include alongside the new `<util/time.h>`.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Thanks for the ping. This PR is still at the exact head I already covered with the automated review (7e96446e). There have been no code changes since that review, so I’m not going to duplicate the full agent/verifier output.
Existing automated review: #7190 (review)
The remaining item is the same backport-adaptation suggestion from that review: keep Dash’s direct #include <timedata.h> in src/node/miner.cpp because Dash still calls GetAdjustedTime() there, while upstream Bitcoin could drop the include only after removing that call in bitcoin#25908.
|
This pull request has conflicts, please rebase. |
…oin#29030) ea00f98 test: fix intermittent error in rpc_net.py (bitcoin#29030) (Sebastian Falbesoner) Pull request description: Asserting for the debug log message "Added connection peer=" is insufficient for ensuring that this new connection will show up in a following getpeerinfo() call, as the debug message is written in the CNode ctor, which means it hasn't necessarily been added to CConnman.m_nodes at this point. Solve this by using the recently introduced `wait_for_new_peer` helper (see bitcoin#29006, commit 00e0658), which is more robust. Fixes bitcoin#29030. ACKs for top commit: maflcko: lgtm ACK ea00f98 Tree-SHA512: dda307949a466fb3b24408a8c213d307e0af2155f2e8b4e52c836a22397f9d218bf9d8c54ca55bae62a96d7566f27167db9311dd8801785c327234783af5ed00
vijaydasmp
changed the title
thepastaclaw
suggested changes
Jun 21, 2026
There was a problem hiding this comment.
Code Review
Policy gate: agent-reported missing upstream prerequisite findings were restored as blocking findings. This PR is a full Bitcoin backport, and omitted upstream hunks require their prerequisite PRs unless the finding is explicitly allowlisted (intentional_exclusion / policy_override). The agent evidence is preserved below.
Prior finding reconciliation: the previous src/node/miner.cpp <timedata.h> finding is FIXED at 7ff83d0. The file now keeps #include <timedata.h> while also including <util/time.h>, so there is no carry-forward action for that item.
🔴 3 blocking
3 additional finding(s) omitted (not in diff).
🤖 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.
Prior finding reconciliation:
- [FIXED] src/node/miner.cpp retains #include <timedata.h>
Prior status: STILL VALID at 7e96446e — there, the bitcoin#29361 backport removed #include <timedata.h> from src/node/miner.cpp while GetAdjustedTime() was still called locally. Current status (head 7ff83d09): FIXED. The re-pushed bitcoin#29361 backport commit adds <util/time.h> without removing <timedata.h>. Verified at src/node/miner.cpp:22 the include is present, and GetAdjustedTime() is still invoked at src/node/miner.cpp:53 and src/node/miner.cpp:222. No carry-forward action required.
In `src/headerssync.cpp`:
- [BLOCKING] src/headerssync.cpp:1: Missing prerequisite: bitcoin#25717
bitcoin#29361 upstream changes src/headerssync.cpp by replacing its <timedata.h> include with <util/time.h>. This Dash head has no src/headerssync.cpp at all. Upstream introduced that file as part of bitcoin#25717 (p2p: Implement anti-DoS headers sync). This is a missing prerequisite by origin, not a Dash-specific adaptation. It is non-blocking per the agent (restored to blocking by policy) for this PR because Dash has no corresponding file to compile and the include cleanup was cleanly omitted.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
In `test/functional/test_framework/util.py`:
- [BLOCKING] test/functional/test_framework/util.py:431-435: Missing prerequisite: bitcoin#27302
bitcoin#29034 upstream changes get_temp_default_datadir() in test_framework/util.py from sys.platform checks to platform.system(), and also updates the feature_config_args.py tests that call it. In this Dash head, util.py jumps from get_datadir_path() at line 431 directly to append_config() at line 435; get_temp_default_datadir() is absent, and feature_config_args.py also lacks the bitcoin#27302 test_config_file_log/test_ignored_default_conf sections. Upstream introduced these sections in bitcoin#27302. This is a soft prerequisite gap because the current Dash cherry-pick does not call the missing helper.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
In `test/functional/feature_remove_pruned_files_on_startup.py`:
- [BLOCKING] test/functional/feature_remove_pruned_files_on_startup.py:1: Missing prerequisite: bitcoin#26533
bitcoin#29034 upstream changes feature_remove_pruned_files_on_startup.py by replacing the Windows check os.name != 'nt' with platform.system() != 'Windows'. This Dash head does not have test/functional/feature_remove_pruned_files_on_startup.py. Upstream introduced that test in bitcoin#26533 (prune: scan and unlink already pruned block files on startup). This is a missing test prerequisite by origin, but it is non-blocking per the agent (restored to blocking by policy) for this PR because no Dash code references the absent test.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
thepastaclaw
suggested changes
Jun 21, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Policy gate: an agent-reported missing upstream prerequisite was restored as a blocking finding. This PR is a full Bitcoin backport, and omitted upstream hunks require their prerequisite PRs unless the finding is explicitly allowlisted (intentional_exclusion / policy_override). The agent's original evidence is preserved in the finding(s) below.
Prior verifier summary (overridden by policy gate): Current head reduces this PR to two test-only backport commits (bitcoin#29041, bitcoin#29034); the previously-included bitcoin#29361 timedata-include cleanup has been reverted out of scope, resolving prior findings #1 and #2. The bitcoin#29034 cherry-pick still silently drops upstream hunks that target code introduced by bitcoin#27302 and bitcoin#26533, neither of which is in this branch's history, so prior findings #3 and #4 remain valid as backport-completeness gaps for a PR advertised as a full upstream merge.
🔴 14 blocking
14 additional finding(s) omitted (not in diff).
🤖 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 `test/functional/test_framework/util.py`:
- [BLOCKING] test/functional/test_framework/util.py:431-435: bitcoin#29034 backport omits hunks dependent on missing prerequisite bitcoin#27302
The merge commit advertises a full backport of bitcoin/bitcoin#29034, but two of its hunks are silently dropped because the prerequisite bitcoin#27302 has not been backported into this branch's history (`git merge-base --is-ancestor 66b08e7822 f9fda6856` returns false). Upstream #29034 (commit 4c65ac96f8) converts `get_temp_default_datadir()` in `test/functional/test_framework/util.py` from `sys.platform` to `platform.system()`, and updates `test_config_file_log()` / `test_ignored_default_conf()` in `test/functional/feature_config_args.py`. None of these symbols exist in this tree (grep returns no matches). Per the backport rubric, an omitted upstream hunk due to absent prerequisite code is a missing prerequisite, not a harmless adaptation. Either backport bitcoin#27302 first and then apply the #29034 update to those sections, or document the intentional exclusion in the PR description / non-backported tracker so future readers know the merge subject overstates what was applied.
- [BLOCKING] test/functional/test_framework/util.py:431-435: bitcoin#29034 is missing the default-datadir prerequisite coverage
Upstream bitcoin#29034 updates get_temp_default_datadir() from sys.platform to platform.system(), and also updates the feature_config_args.py tests that exercise ignored default configuration files. In current head, util.py goes directly from get_datadir_path() to append_config(), with no get_temp_default_datadir() helper, and feature_config_args.py lacks the test_config_file_log/test_ignored_default_conf blocks introduced by bitcoin#27302. Because this PR claims to merge bitcoin#29034, omitting the prerequisite means part of the upstream OS-detection cleanup and its regression coverage is silently not backported.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] test/functional/test_framework/util.py:1: Prior finding #3 (bitcoin#27302 prerequisite) — out of scope for commit-history specialist
Prior review's blocking finding about the missing bitcoin#27302 prerequisite (get_temp_default_datadir helper / feature_config_args coverage) is a backport-completeness concern, not commit-history hygiene. Defer to backport-reviewer specialist.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] test/functional/test_framework/util.py:1: Backport 29034 still omits prerequisite util helper coverage
Carried forward from the prior review: commit f9fda68564 claims to merge bitcoin/bitcoin#29034, but the current head still has no get_temp_default_datadir() helper in test/functional/test_framework/util.py and does not include the related feature_config_args.py coverage that upstream #29034 updates after bitcoin#27302. Because Dash merges the backport commit as permanent history, this leaves the commit message claiming a complete upstream OS-detection cleanup while prerequisite-backed portions are absent. Backport the Dash-adapted bitcoin#27302 pieces and apply the #29034 changes there, or explicitly document that those upstream hunks are intentionally excluded.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] test/functional/test_framework/util.py:1: Missing prerequisite: bitcoin#27302 (still valid for the #29034 backport)
Upstream bitcoin#29034 (commit 4c65ac96f8) modifies `test/functional/test_framework/util.py` to convert `get_temp_default_datadir()` from `sys.platform` checks to `platform.system()`. Dash's util.py has no `get_temp_default_datadir` helper (`Grep get_temp_default_datadir test/functional` returns no matches), so the Dash cherry-pick silently omits that hunk. Upstream also modifies `test/functional/feature_config_args.py` to convert `test_config_file_log` / `test_ignored_default_conf`'s `sys.platform == 'win32'` checks; Dash lacks both methods (search confirms no matches). `git log -S test_config_file_log` traces the missing helpers to bitcoin#27302 (`Merge bitcoin/bitcoin#27302: init: Error if ignored bitcoin.conf file is found`). The PR title claims a full backport of bitcoin#29034, but two of its source-file hunks are dropped because the prerequisite bitcoin#27302 was never backported. Per the skill rubric, this is a missing prereq, not a Dash adaptation — there is no Dash-specific reason these helpers are absent. Either backport bitcoin#27302 first then re-apply the #29034 hunks to those files, or document an intentional exclusion of those sections. Classification: STILL VALID.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] test/functional/test_framework/util.py:435: Missing prerequisite: bitcoin#27302
STILL VALID. Upstream bitcoin#29034 changes get_temp_default_datadir() in test_framework/util.py from sys.platform checks to platform.system(), and also updates the related feature_config_args.py tests test_config_file_log() and test_ignored_default_conf(). Current Dash head has no get_temp_default_datadir() between get_datadir_path() and append_config(), and feature_config_args.py lacks those two default-datadir tests. Upstream introduced this helper and coverage in bitcoin#27302 (merge 66b08e7822 / commit 3746f00be1). This means the bitcoin#29034 backport is still missing prerequisite test framework content rather than carrying the full upstream OS-detection cleanup.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
In `test/functional/feature_remove_pruned_files_on_startup.py`:
- [BLOCKING] test/functional/feature_remove_pruned_files_on_startup.py:1: bitcoin#29034 backport omits feature_remove_pruned_files_on_startup.py hunk (prerequisite bitcoin#26533 missing)
Upstream bitcoin#29034 replaces `if os.name != 'nt':` with `if platform.system() != 'Windows':` in `test/functional/feature_remove_pruned_files_on_startup.py`. The file does not exist in this branch (`ls` returns No such file or directory), and the introducing PR bitcoin#26533 (merge bb136aaf2c) is not an ancestor of f9fda6856 (`git merge-base --is-ancestor` returns false), even though it exists on other branches. The cherry-pick therefore drops this file's hunk while still labelling the commit as a complete `Merge bitcoin/bitcoin#29034`. Backport bitcoin#26533 first and apply the OS-detection update, or document this intentional exclusion (e.g. in the PR description or the backport tracker) so the divergence from the advertised upstream merge is explicit.
- [BLOCKING] test/functional/feature_remove_pruned_files_on_startup.py:1: Prior finding #4 (bitcoin#26533 prerequisite) — out of scope for commit-history specialist
Prior review's blocking finding about the missing bitcoin#26533 prerequisite (feature_remove_pruned_files_on_startup.py) is a backport-completeness concern, not commit-history hygiene. Defer to backport-reviewer specialist.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] test/functional/feature_remove_pruned_files_on_startup.py:1: Backport 29034 still omits prerequisite pruned-files test
Carried forward from the prior review: commit f9fda68564 still does not include test/functional/feature_remove_pruned_files_on_startup.py, the test introduced upstream by bitcoin#26533 and later changed by bitcoin#29034 from os.name to platform.system(). As written, the permanent backport history says bitcoin#29034 was merged but leaves this prerequisite-dependent upstream hunk absent without explanation. Backport the Dash-adapted bitcoin#26533 test and then apply the #29034 OS-detection update, or document the intentional exclusion in the backport commit/PR rationale.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] test/functional/feature_remove_pruned_files_on_startup.py:1: Missing prerequisite: bitcoin#26533 (still valid for the #29034 backport)
Upstream bitcoin#29034 (commit 4c65ac96f8) modifies `test/functional/feature_remove_pruned_files_on_startup.py` by replacing `if os.name != 'nt':` with `if platform.system() != 'Windows':`. The file does not exist in Dash (`ls test/functional/feature_remove_pruned_files_on_startup.py` → No such file or directory; `git ls-tree f9fda68564^ ...` confirms absence). `git log --diff-filter=A` traces the file's introduction to upstream commit 3141eab9c6 / merge bb136aaf2c, which is bitcoin#26533 (`Merge bitcoin/bitcoin#26533: prune: scan and unlink already pruned block files on startup`). The PR claims a full backport of bitcoin#29034 but this entire file hunk is silently omitted because the prerequisite bitcoin#26533 was never backported. Per the rubric, 'Dash does not have that test yet' is evidence of a missing prerequisite, not intentional divergence, when the PR is presented as a full upstream merge. Either backport bitcoin#26533 first and apply the #29034 hunk on top, or document the intentional exclusion. Classification: STILL VALID.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] test/functional/feature_remove_pruned_files_on_startup.py:1: Missing prerequisite: bitcoin#26533
STILL VALID. Upstream bitcoin#29034 modifies feature_remove_pruned_files_on_startup.py by replacing the Windows check os.name != 'nt' with platform.system() != 'Windows'. Current Dash head does not contain this file at all, so that upstream hunk is omitted because the prerequisite test is absent. The file was introduced upstream by bitcoin#26533 (merge bb136aaf2c, commit 3141eab9c6). Backporting bitcoin#29034 as a complete upstream merge still requires either the Dash-adapted bitcoin#26533 test or explicit documentation that this upstream test is intentionally excluded.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
In `src/headerssync.cpp`:
- [BLOCKING] src/headerssync.cpp:1: Prior finding #2 (bitcoin#25717 prerequisite) — out of scope for commit-history specialist
Prior review's blocking finding about the missing bitcoin#25717 prerequisite (headerssync.cpp) is a backport-completeness concern handled by the backport-reviewer specialist, not commit-history hygiene. Not re-classified here; defer to that specialist.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
- [BLOCKING] src/headerssync.cpp:1: Prior finding OUTDATED: bitcoin#25717 prereq tied to dropped #29361 backport
This prior finding flagged that bitcoin#29361 upstream rewrites `src/headerssync.cpp` includes, but Dash lacks the file (it would require backporting bitcoin#25717 first). Since the current head drops the bitcoin#29361 backport entirely (see analysis above), this dependency chain is no longer in scope. Classification: OUTDATED.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
In `src/node/miner.cpp`:
- [BLOCKING] src/node/miner.cpp:22: Prior finding OUTDATED: bitcoin#29361 backport removed from PR scope
At 7ff83d09 this finding was already marked FIXED (the `#include <timedata.h>` was retained). At the current head f9fda685, the bitcoin#29361 backport commit (`Merge bitcoin/bitcoin#29361: refactor: Fix timedata includes`, prior bc6cc34421) is no longer present in this branch. The latest delta against 7ff83d09 (`src/chain.h`, `src/net_processing.cpp`, `src/node/miner.cpp`, `src/rpc/mining.cpp`, `src/timedata.h`) is the reversal of that backport. Current `src/node/miner.cpp:22` still contains `#include <timedata.h>`, and `GetAdjustedTime()` resolves through it. Classification: OUTDATED — no longer in scope for this PR.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
thepastaclaw
suggested changes
Jun 21, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review — correction
Correction to my immediately prior automated review for this same SHA: the deterministic policy gate over-promoted duplicate/outdated reconciliation notes, which inflated the review to 14 blocking entries. Please ignore that noisy review body and use this consolidated result instead.
Prior-finding reconciliation:
- Prior #1 (
src/node/miner.cpp/ timedata include): OUTDATED/FIXED. The current PR no longer includes the bitcoin#29361 backport, andminer.cppstill retains#include <timedata.h>. - Prior #2 (
src/headerssync.cpp/ bitcoin#25717): OUTDATED. It was tied to the dropped bitcoin#29361 backport, so it is no longer in scope for this head. - Prior #3 (
test/functional/test_framework/util.py/ bitcoin#27302): STILL VALID. - Prior #4 (
test/functional/feature_remove_pruned_files_on_startup.py/ bitcoin#26533): STILL VALID.
🔴 2 blocking
-
test/functional/test_framework/util.py: bitcoin#29034 still omits hunks that depend on prerequisite bitcoin#27302. Upstream bitcoin#29034 convertsget_temp_default_datadir()and relatedfeature_config_args.pycoverage fromsys.platformtoplatform.system(), but current Dash head has neither the helper nor those tests. Either backport the Dash-adapted bitcoin#27302 pieces and apply the bitcoin#29034 updates, or explicitly document that those upstream hunks are intentionally excluded. -
test/functional/feature_remove_pruned_files_on_startup.py: bitcoin#29034 still omits the hunk for the pruned-files startup test because prerequisite bitcoin#26533 is absent. Upstream bitcoin#29034 changes that test's Windows guard fromos.nametoplatform.system(), but the file does not exist in this head. Either backport the Dash-adapted bitcoin#26533 test and apply the bitcoin#29034 update, or explicitly document the intentional exclusion.
No new latest-delta-only findings beyond those carried-forward bitcoin#29034 prerequisite gaps.
thepastaclaw
suggested changes
Jun 22, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Policy gate: an agent-reported missing upstream prerequisite was restored as a blocking finding. This PR is a full Bitcoin backport, and omitted upstream hunks require their prerequisite PRs unless the finding is explicitly allowlisted (intentional_exclusion / policy_override). The agent's original evidence is preserved in the finding(s) below.
Prior verifier summary (overridden by policy gate): The current head 517c643 contains only the bitcoin#29041 rpc_net.py backport, and its wait_for_new_peer prerequisite is already present. The PR metadata still advertises bitcoin#29034, but none of bitcoin#29034's upstream 12-file diff is present at the assigned head, so the PR is incomplete unless bitcoin#29034 is explicitly removed from scope. CodeRabbit provided no inline findings to validate.
🔴 3 blocking
3 additional finding(s) omitted (not in diff).
🤖 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 `test/functional/README.md`:
- [BLOCKING] test/functional/README.md:39: Missing advertised backport: bitcoin#29034
The PR title and GitHub metadata still say this backports bitcoin#29041 and bitcoin#29034, but the current reviewed stack against develop contains only 517c643c6f for bitcoin#29041 and the base-to-head diff changes only test/functional/rpc_net.py. Upstream bitcoin#29034 changes 12 files, including this README guideline, platform.system() conversions in functional tests, and prerequisite-dependent hunks in feature_config_args.py, test_framework/util.py, and feature_remove_pruned_files_on_startup.py; none of those changes are present at this head. Restore/rebase the bitcoin#29034 backport or update the PR title/body to state that #29034 was intentionally dropped.
In `test/functional/test_framework/util.py`:
- [BLOCKING] test/functional/test_framework/util.py:431-435: bitcoin#29034 prerequisite default-datadir coverage is still missing
The PR still advertises bitcoin#29034, but the current head has dropped the 29034 merge commit and still lacks the bitcoin#27302 prerequisite pieces that 29034 modifies upstream. In this file, the helper area goes directly from get_datadir_path() to append_config(), with no get_temp_default_datadir(), and feature_config_args.py still lacks the related default-datadir tests. If bitcoin#29034 remains in scope, the backport is incomplete; restore the prerequisite coverage or explicitly remove 29034 from the PR scope.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
In `test/functional/feature_remove_pruned_files_on_startup.py`:
- [BLOCKING] test/functional/feature_remove_pruned_files_on_startup.py:1: bitcoin#29034 still omits the pruned-files startup test prerequisite
The current head still does not contain test/functional/feature_remove_pruned_files_on_startup.py. Upstream bitcoin#29034 updates this test after it was introduced by bitcoin#26533, so with the PR title still advertising bitcoin#29034 and no documented exclusion, the backport remains incomplete. Restore/adapt the prerequisite test or update the PR scope so it no longer claims to merge bitcoin#29034.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
|
Coordinator note for the exact-head review at |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
bitcoin backporting