backport: Merge bitcoin#30170, 30026, 30017, 29961, 29910, 29849, 29865 - #7257
backport: Merge bitcoin#30170, 30026, 30017, 29961, 29910, 29849, 29865#7257vijaydasmp wants to merge 7 commits into
Conversation
|
a8df81b to
3673fe0
Compare
vijaydasmp
changed the title
|
🕓 Queued for automated review — 28th in line, estimated start in ~39 h (commit 52b8033)
|
WalkthroughThis pull request changes URL decoding to a new Estimated code review effort: 3 (Moderate) | ~35 minutes Suggested reviewers: Related PRs: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/fuzz/poolresource.cpp (1)
68-68: Optional: Considersize_t{1}instead of1Ufor consistency.For complete consistency with the other changes in this line, you could replace
- 1Uwith- size_t{1}. This is purely a style preference and doesn't affect correctness, as1Uwill be promoted tosize_tduring the subtraction.✨ Optional consistency fix
- size_t size = m_provider.ConsumeIntegralInRange<size_t>(size_t{1} << size_bits, (size_t{1} << (size_bits + 1)) - 1U) << alignment_bits; + size_t size = m_provider.ConsumeIntegralInRange<size_t>(size_t{1} << size_bits, (size_t{1} << (size_bits + 1)) - size_t{1}) << alignment_bits;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/poolresource.cpp` at line 68, Replace the literal unsigned int used in the upper bound with a size_t brace-init for consistency: in the call to m_provider.ConsumeIntegralInRange<size_t> change the upper bound expression from ((size_t{1} << (size_bits + 1)) - 1U) to ((size_t{1} << (size_bits + 1)) - size_t{1}) so both operands are size_t; this edit is in the expression passed to m_provider.ConsumeIntegralInRange<size_t>.src/test/validation_chainstate_tests.cpp (1)
112-119: ReplaceAssert(...)withBOOST_REQUIREfor test-scoped failure handling.In test code,
Assert(...)callsabort()and terminates the entire test process immediately. UsingBOOST_REQUIRE/BOOST_REQUIRE_MESSAGEinstead aborts only the current test case, allowing other test cases in the suite to continue—critical for test-run completeness. This also aligns with the project's requirement that unit tests insrc/test/use Boost::Test.Suggested refactor
- CChainState& background_cs{*Assert([&]() -> CChainState* { + CChainState* background_cs_ptr{[&]() -> CChainState* { for (CChainState* cs : chainman.GetAll()) { if (cs != &chainman.ActiveChainstate()) { return cs; } } return nullptr; - }())}; + }()}; + BOOST_REQUIRE_MESSAGE(background_cs_ptr != nullptr, "Background chainstate not found"); + CChainState& background_cs{*background_cs_ptr};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/validation_chainstate_tests.cpp` around lines 112 - 119, The test uses Assert(...) which aborts the whole process; replace it with Boost checks: iterate over chainman.GetAll() to find the non-active CChainState* (same logic as the lambda), store it in a CChainState* variable, then use BOOST_REQUIRE_MESSAGE(found != nullptr, "background chainstate not found") to fail only this test if null; finally bind CChainState& background_cs to *found (preserving the original variable name background_cs and using chainman.ActiveChainstate() to identify the active one).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/fuzz/poolresource.cpp`:
- Line 68: Replace the literal unsigned int used in the upper bound with a
size_t brace-init for consistency: in the call to
m_provider.ConsumeIntegralInRange<size_t> change the upper bound expression from
((size_t{1} << (size_bits + 1)) - 1U) to ((size_t{1} << (size_bits + 1)) -
size_t{1}) so both operands are size_t; this edit is in the expression passed to
m_provider.ConsumeIntegralInRange<size_t>.
In `@src/test/validation_chainstate_tests.cpp`:
- Around line 112-119: The test uses Assert(...) which aborts the whole process;
replace it with Boost checks: iterate over chainman.GetAll() to find the
non-active CChainState* (same logic as the lambda), store it in a CChainState*
variable, then use BOOST_REQUIRE_MESSAGE(found != nullptr, "background
chainstate not found") to fail only this test if null; finally bind CChainState&
background_cs to *found (preserving the original variable name background_cs and
using chainman.ActiveChainstate() to identify the active one).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf1c9f39-4bdd-4412-b32b-ce4ffdc697a7
📥 Commits
Reviewing files that changed from the base of the PR and between 31c8464 and 9fb5593d467374770847be704149ad3cfbdac7f9.
📒 Files selected for processing (21)
configure.acsrc/Makefile.amsrc/Makefile.test.includesrc/bitcoin-cli.cppsrc/bitcoin-wallet.cppsrc/bitcoind.cppsrc/common/run_command.cppsrc/common/url.cppsrc/common/url.hsrc/qt/main.cppsrc/test/common_url_tests.cppsrc/test/fuzz/poolresource.cppsrc/test/fuzz/string.cppsrc/test/system_tests.cppsrc/test/util/setup_common.cppsrc/test/validation_chainstate_tests.cppsrc/txorphanage.cppsrc/txorphanage.hsrc/util/subprocess.hsrc/wallet/rpc/util.cpptest/lint/lint-spelling.py
💤 Files with no reviewable changes (6)
- src/bitcoin-wallet.cpp
- src/qt/main.cpp
- src/test/util/setup_common.cpp
- src/bitcoin-cli.cpp
- src/bitcoind.cpp
- configure.ac
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the exact patch scope for PR #7257 against the checked-out source at 9fb5593d467374770847be704149ad3cfbdac7f9. The successful agent section (codex-general) reported no findings, and direct inspection of the touched URL decoding, orphanage time-type cleanup, and subprocess header rename did not reveal a concrete correctness, Dash-specific interaction, or build-integration issue within this diff.
Reviewed commit: 9fb5593d
| @@ -921,6 +921,7 @@ libbitcoin_common_a_SOURCES = \ | |||
| coins.cpp \ | |||
| common/bloom.cpp \ | |||
| common/run_command.cpp \ | |||
| common/url.cpp \ | |||
There was a problem hiding this comment.
consider to do bitcoin#29967 to addition for bitcoin#29904
| throw OSError("TerminateProcess", 0); | ||
| } | ||
| #else | ||
| if (session_leader_) killpg(child_pid_, sig_num); |
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
Looks good for me overall; but bitcoin#29865 should be included to this PR too
| @@ -160,11 +160,11 @@ class OSError: public std::runtime_error | |||
|
|
|||
| //Environment Variable types | |||
| #ifndef _MSC_VER | |||
| using env_string_t = std::string; | |||
| using env_char_t = char; | |||
| using env_string_t = std::string; | |||
There was a problem hiding this comment.
replace Adddressing linter errors for using tabs instead of spaces by backporting bitcoin#29865 which just removes most of this code ; to prevent conflicts in the future
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Backport bundle (bitcoin#29849, bitcoin#29904, bitcoin#29910, bitcoin#29961, bitcoin#30017, bitcoin#30026, bitcoin#30170) is clean overall; merge resolutions are faithful and Dash-specific subsystems are untouched. One real gap: the bitcoin#29961 cherry-pick left behind the now-dead param_pack/has_type template metaprogram in src/util/subprocess.h that upstream deleted alongside check_output. Non-blocking, but worth fixing to keep the file aligned with upstream and remove the lingering check_ouput/ouput typos that bitcoin#29849 implicitly relied on being gone.
Reviewed commit: 9fb5593d
🟡 1 suggestion(s)
1 additional finding
🟡 suggestion: Incomplete backport of bitcoin#29961: dead `param_pack`/`has_type` metaprogram remains
src/util/subprocess.h (lines 947-975)
Upstream bitcoin#29961 removed four pieces of dead code from subprocess.hpp: (1) util::is_ready, (2) Buffer's #if 0 copy/move debug ctors, (3) Popen::kill and its docstring entries, and (4) the param_pack / has_type<> template metaprogram in namespace detail. The Dash backport (commit 4518a34698) applied (1)-(3) but left (4) untouched at src/util/subprocess.h:947-975.
These templates existed solely to support compile-time validation of check_output / ArgumentDeducer / OptionDeducer — all of which the same upstream PR removed. There are no remaining users in the file. The cherry-pick is therefore semantically incomplete.
Leaving the block in place also preserves the misspellings check_ouput / ouput (lines 952-953), which partially defeats the purpose of bitcoin#29849 dropping subprocess.h from the codespell exclude list — a future spelling-lint run can surface them. Removing this block aligns Dash with upstream after bitcoin#29961 and removes the typo footgun in one shot.
🤖 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/util/subprocess.h`:
- [SUGGESTION] lines 947-975: Incomplete backport of bitcoin#29961: dead `param_pack`/`has_type` metaprogram remains
Upstream bitcoin#29961 removed four pieces of dead code from `subprocess.hpp`: (1) `util::is_ready`, (2) `Buffer`'s `#if 0` copy/move debug ctors, (3) `Popen::kill` and its docstring entries, and (4) the `param_pack` / `has_type<>` template metaprogram in `namespace detail`. The Dash backport (commit 4518a34698) applied (1)-(3) but left (4) untouched at `src/util/subprocess.h:947-975`.
These templates existed solely to support compile-time validation of `check_output` / `ArgumentDeducer` / `OptionDeducer` — all of which the same upstream PR removed. There are no remaining users in the file. The cherry-pick is therefore semantically incomplete.
Leaving the block in place also preserves the misspellings `check_ouput` / `ouput` (lines 952-953), which partially defeats the purpose of bitcoin#29849 dropping `subprocess.h` from the codespell exclude list — a future spelling-lint run can surface them. Removing this block aligns Dash with upstream after #29961 and removes the typo footgun in one shot.
13f5391 Fix typos in `subprocess.hpp` (Hennadii Stepanov) Pull request description: Resolves one item in the bitcoin#28981 (review): > - Remove linter exclusions and fix all issues. Based on upstream arun11299/cpp-subprocess#101. ACKs for top commit: fanquake: ACK 13f5391 Tree-SHA512: 2ee27a5b7d1ba6f47a5148add155c918eadaaffb94a4b5dd3edea00e63440b87291c559361bf25a8db1567debff78cf7e9466dc34f14331ca1d426994837df93
…header name conventions 08f756b Replace locale-dependent `std::strerror` with `SysErrorString` (Hennadii Stepanov) d8e4ba4 refactor: Rename `subprocess.hpp` to follow our header name conventions (Hennadii Stepanov) Pull request description: This PR renames the header `*.hpp` --> `*.h` and adjusts the header guard name, which makes it available for processing by linters. Fixed the following linter warning: ``` The locale dependent function strerror(...) appears to be used: src/util/subprocess.h: std::runtime_error( err_msg + ": " + std::strerror(err_code) ) Unnecessary locale dependence can cause bugs that are very tricky to isolate and fix. Please avoid using locale-dependent functions if possible. Advice not applicable in this specific case? Add an exception by updating the ignore list in /bitcoin/test/lint/lint-locale-dependence.py ^---- failure generated from lint-locale-dependence.py ``` ACKs for top commit: TheCharlatan: ACK 08f756b Tree-SHA512: 57a2f01c20eb9552481e428a4969bd59e9ada9f784fe1a45cb62aa9c9152c8e950d336854f45af0e2e5dc7c7b2a1fb216c8f832e3d6ccfb457ad71b6e423231e
vijaydasmp
changed the title
|
This pull request has conflicts, please rebase. |
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): Cumulative review at head 8c931802. The bitcoin#29865 backport requested by reviewer knst is FIXED (commit 8c931802 cleanly removes unused Popen options in src/util/subprocess.h with no smuggled changes). The prior bitcoin#29961 incomplete-backport finding is STILL VALID: upstream commit ff79adb also removed the unused param_pack/has_type metaprogram and fixed the check_ouput/ouput misspellings, but those hunks were never applied to the Dash tree and bitcoin#29865 did not touch them. No new blocking issues. Codex's secondary txorphanage prerequisite-chain finding is dropped as a false positive: the bitcoin#30170 NodeSeconds conversion was adapted cleanly onto Dash's older txorphanage baseline and the PR does not claim to backport bitcoin#28364/bitcoin#29031/bitcoin#30000.
🔴 3 blocking | 🟡 1 suggestion(s)
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 `src/util/subprocess.h`:
- [SUGGESTION] src/util/subprocess.h:706-737: Incomplete bitcoin#29961 backport: leftover param_pack/has_type templates and check_ouput/ouput misspellings
Carried forward from prior review and re-validated at HEAD 8c931802f3 — still valid.
Upstream bitcoin#29961 commit ff79adbe05 ('remove unused templates from cpp-subprocess') removed two pieces of dead code that the Dash squashed merge (26fae74374) did not apply: (1) the `param_pack` / `has_type` metaprogram in `namespace detail` and (2) the surrounding comments containing the upstream misspellings `check_ouput` / `ouput`. Both are still present at src/util/subprocess.h:706-737 (`template <typename... T> struct param_pack{};`, the three `has_type` specializations, and the comment ending `// or in check_ouput arguments.`).
Grep confirms `param_pack`, `has_type`, `check_output`, and `check_ouput` are not referenced anywhere else in the tree, so this is pure dead code with no compile or runtime impact. The newly added bitcoin#29865 (commit 8c931802) only removed unused Popen options (shell, preexec, defer_spawn, close_fds, session_leader, bufsize, cwd, environment) and does not touch this block.
Not a merge-blocker, but the cherry-pick is semantically incomplete relative to its commit message 'Merge bitcoin/bitcoin#29961: refactor: remove remaining unused code from cpp-subprocess'. Either complete the 29961 backport by also dropping the param_pack/has_type templates and fixing the comments, or document the intentional deviation.
- [BLOCKING] src/util/subprocess.h:705-737: Incomplete backport of bitcoin#29961: leftover param_pack/has_type metaprogram and 'check_ouput'/'ouput' misspellings
Carried forward from prior review and re-validated at HEAD 8c931802f3.
Upstream bitcoin#29961 (merge 81174d8a9b) is composed of 4 commits; one of them — ff79adbe05 'remove unused templates from cpp-subprocess' — removes three pieces of dead code from src/util/subprocess.h: (1) util::is_ready, (2) the param_pack / has_type metaprogram in namespace detail, and (3) the misspelled comment block that references 'check_ouput' / 'ouput'.
Dash's squashed backport (26fae74374) only applied (1). The current head still contains the param_pack/has_type templates at lines 712–730 and the misspelled comments at lines 709–710 and 736–737, e.g.:
// checking of the arguments provided to 'check_ouput' function
// wherein the user is not expected to provide an 'ouput' option.
template <typename... T> struct param_pack{};
template <typename F, typename T> struct has_type;
...
* or in check_ouput arguments.
Evidence: `diff <(git show 81174d8a9b:src/util/subprocess.h) <(git show 8c931802f3:src/util/subprocess.h)` shows precisely the param_pack/has_type block as present-in-Dash / absent-upstream, plus the trailing misspelled comment.
The newly added bitcoin#29865 (commit 8c931802) does not touch this block and therefore does not fix the leftover. The cherry-pick is semantically incomplete with respect to its stated goal ('Merge bitcoin/bitcoin#29961: refactor: remove remaining unused code from cpp-subprocess').
Impact: dead code only — no compile or runtime effect. Classify as suggestion: either complete the 29961 backport by also dropping the param_pack/has_type block and the 'check_ouput'/'ouput' misspelled comments, or document the intentional deviation. Not a merge-blocker.
---
**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/util/subprocess.h:706-737: Incomplete backport: bitcoin#29961
Prior finding STILL VALID. Upstream bitcoin#29961 removed the unused detail::param_pack / has_type metaprogram and updated the ArgumentDeducer comment to only reference Popen constructor options. Dash's bitcoin#29961 backport removed other dead cpp-subprocess pieces, but current head still retains this block, including stale check_ouput/ouput comments. The newly added bitcoin#29865 backport removed the unused Popen options but does not address this bitcoin#29961 hunk, so the prior incomplete-backport root cause remains. This is dead code and appears non-blocking per the agent (restored to blocking by policy), but it is still an incomplete upstream cherry-pick.
---
**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/txorphanage.cpp`:
- [BLOCKING] src/txorphanage.cpp:43: Missing prerequisites for bitcoin#30170: bitcoin#28364, bitcoin#29031, bitcoin#30000
New cumulative finding. Upstream bitcoin#30170 was applied on a newer TxOrphanage baseline: its pre-PR state indexed m_orphans by Wtxid, erased by Wtxid, had a TXPACKAGES elapsed-time removal log, and used LimitOrphans(unsigned int max_orphans, FastRandomContext& rng). Those upstream baseline pieces trace to bitcoin#28364, bitcoin#29031, and bitcoin#30000. Dash current head adapts the time-type conversion onto the older txid-keyed orphanage with internal RNG creation and no elapsed-time TXPACKAGES removal log. The adaptation appears clean for the narrow NodeSeconds conversion, so this is not blocking, but under the origin-based prerequisite rule the upstream dependency chain for the claimed bitcoin#30170 backport is incomplete.
---
**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.
| @@ -39,7 +40,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) | |||
| return false; | |||
| } | |||
|
|
|||
| auto ret = m_orphans.emplace(hash, OrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME, m_orphan_list.size(), sz}); | |||
| auto ret = m_orphans.emplace(hash, OrphanTx{tx, peer, Now<NodeSeconds>() + ORPHAN_TX_EXPIRE_TIME, m_orphan_list.size(), sz}); | |||
There was a problem hiding this comment.
🔴 Blocking: Missing prerequisites for bitcoin#30170: bitcoin#28364, bitcoin#29031, bitcoin#30000
New cumulative finding. Upstream bitcoin#30170 was applied on a newer TxOrphanage baseline: its pre-PR state indexed m_orphans by Wtxid, erased by Wtxid, had a TXPACKAGES elapsed-time removal log, and used LimitOrphans(unsigned int max_orphans, FastRandomContext& rng). Those upstream baseline pieces trace to bitcoin#28364, bitcoin#29031, and bitcoin#30000. Dash current head adapts the time-type conversion onto the older txid-keyed orphanage with internal RNG creation and no elapsed-time TXPACKAGES removal log. The adaptation appears clean for the narrow NodeSeconds conversion, so this is not blocking, but under the origin-based prerequisite rule the upstream dependency chain for the claimed bitcoin#30170 backport is incomplete.
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.
source: ['codex-backport-reviewer']
There was a problem hiding this comment.
Resolved in this update — Missing prerequisites for bitcoin#30170: bitcoin#28364, bitcoin#29031, bitcoin#30000 no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Code Review
Source: reviewer lanes: claude-general=opus (completed); codex-general=gpt-5.5[high] (failed); claude-dash-core-commit-history=opus (completed); codex-dash-core-commit-history=gpt-5.5[high] (failed); claude-backport-reviewer=opus (completed); codex-backport-reviewer=gpt-5.5[high] (failed); verifier=claude opus (completed).
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): Clean Bitcoin Core backport of 8 upstream PRs (bitcoin#29849, bitcoin#29910, bitcoin#29904, bitcoin#29961, bitcoin#30017, bitcoin#30026, bitcoin#30170, bitcoin#29865). Merge resolutions correct: URL_DECODE indirection removal is consistent across dashd/dash-cli/qt/bitcoin-wallet/setup_common, wallet/rpc/util.cpp is properly retargeted to UrlDecode(string_view), subprocess.hpp→.h rename fully propagated, USE_LIBEVENT AM_CONDITIONAL removal safe, and txorphanage NodeSeconds refactor compiles cleanly against Dash's callers. No Dash subsystem (LLMQ, evo, governance, CoinJoin) is touched. One minor completeness gap in the bitcoin#29961 squash: dead-code templates left behind.
🔴 2 blocking
The verifier also emitted a duplicate nitpick for the subprocess.h completeness gap; the policy-gated blocking finding below supersedes it.
🤖 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/util/subprocess.h`:
- [BLOCKING] src/util/subprocess.h:712-729: Incomplete backport of bitcoin#29961: unused has_type/param_pack templates not removed
Upstream bitcoin#29961 commit ff79adbe ('remove unused templates from cpp-subprocess') deletes the `param_pack` struct and `has_type` template metaprograms (upstream diff removes lines ~702-724 of subprocess.h). Dash's squashed backport commit 26fae74374 only removed `is_ready` and does not include the removal of these templates — they remain at `src/util/subprocess.h:712-729`. Shortstat confirms the divergence: upstream #29961 = 18 insertions, 96 deletions; Dash #29961 = 18 insertions, 66 deletions (30-line delta matches the template block). Non-functional (templates are dead code — no references remain in Dash's tree after the cleanup), but the '' merge is incomplete relative to the upstream PR title.
---
**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/txorphanage.cpp`:
- [BLOCKING] src/txorphanage.cpp:1-200: bitcoin#30170: EraseTxNoLock LogPrint hunk absent (unrelated upstream PR not backported)
Upstream bitcoin#30170 also converts `GetTime() + ORPHAN_TX_EXPIRE_TIME - it->second.nTimeExpire` to `Ticks<std::chrono::seconds>(NodeClock::now() + ...)` inside `EraseTxNoLock` (a LogPrint of ' removed orphan tx ... after %ds'). Dash's `src/txorphanage.cpp` has neither an `EraseTxNoLock` helper nor that LogPrint statement — they were introduced by earlier upstream PRs (refactor to split EraseTx / add debug log) that have not been backported. This is a pre-existing gap, not a semantic issue: the cherry-pick correctly omits the missing hunk. non-blocking per the agent (restored to blocking by policy), but flagging so the missing intermediate PRs can be considered when they get backported (they will need to re-add the type-safe conversion at that time).
---
**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.
|
This pull request has conflicts, please rebase. |
…subprocess 8b52e7f update comments in cpp-subprocess (check_output references) (Sebastian Falbesoner) 97f1597 remove unused method `Popen::kill` from cpp-subprocess (Sebastian Falbesoner) 908c51f remove commented out code in cpp-subprocess (Sebastian Falbesoner) ff79adb remove unused templates from cpp-subprocess (Sebastian Falbesoner) Pull request description: This PR removes remaining code that is unused within the cpp-subprocess module (templates and commented out code). Happy to add more removals if anyone finds more unused parts. Note that there are some API functions of the `Popen` class that we don't use, e.g. `wait()`, `pid()`, `poll()`, `kill()`, but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there. ACKs for top commit: fjahr: Code review ACK 8b52e7f achow101: ACK 8b52e7f hebasto: ACK 8b52e7f. Tree-SHA512: 14c1cd2216185d941923f06fdc7acbeed66cd87e2691d9a352f7309b3e07fe4877b580f598a2e4106f9c48395ed6de00a0bfb5d3c3af9c4624d1956a0f543e99
b50d127 refactor: Make 64-bit shift explicit (Hennadii Stepanov) Pull request description: This PR fixes MSVC warning [C4334](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4334) in the fuzzing code. Similar to bitcoin#26252. All `DisableSpecificWarnings` dropped from `fuzz.vcxproj` as all remained are inherited from `common.init.vcxproj`. Required to simplify warning suppression porting to the CMake-based build system. ACKs for top commit: maflcko: utACK b50d127 sipsorcery: utACK b50d127 Tree-SHA512: 18f6082b4234506ad2f9df54e577031b97cdf9f7ef64cad4162f275660716ab73587a97d3af0f778dfd48d2751d8676b5d3381d0aa837fcc60a09704473a9209
bd2de7a refactor, test: Always initialize pointer (Hennadii Stepanov) Pull request description: This change fixes MSVC warning [C4703](https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4703). All `DisableSpecificWarnings` dropped from `test_bitcoin.vcxproj` as all remained are inherited from `common.init.vcxproj`. Required to simplify warning suppression porting to the CMake-based build system. ACKs for top commit: maflcko: utACK bd2de7a sipsorcery: utACK bd2de7a. ryanofsky: Code review ACK bd2de7a Tree-SHA512: 006db041d3c3697a77d9df14de86cf7c8a10804b45789df01268b2236cf6452e77dc57e89f5d5a6bc26d4b5cd483f0722d6035649c8a523b57954bb1fc810d0c
fa6d489 refactor: Use type-safe time in txorphanage (MarcoFalke) Pull request description: This allows to remove manual conversions like multiplication by `60`, and uses a type-safe type instead of a raw `int64_t`. ACKs for top commit: epiccurious: utACK fa6d489. dergoegge: Code review ACK fa6d489 brunoerg: utACK fa6d489 Tree-SHA512: c187d0e579b1131afcef8c901f5662c18ab867fa2a99fbb13b67bb1e10b2047128194bfef8329cde0d51e1c35d6227ae292b823968f37ea9422975e46e01846a
13adbf7 remove unneeded environment option from cpp-subprocess (Sebastian Falbesoner) 2088777 remove unneeded cwd option from cpp-subprocess (Sebastian Falbesoner) 03ffb09 remove unneeded bufsize option from cpp-subprocess (Sebastian Falbesoner) 79c3036 remove unneeded close_fds option from cpp-subprocess (Sebastian Falbesoner) 62db8f8 remove unneeded session_leader option from cpp-subprocess (Sebastian Falbesoner) 80d008c remove unneeded defer_spawn option from cpp-subprocess (Sebastian Falbesoner) cececad remove unneeded preexec function option from cpp-subprocess (Sebastian Falbesoner) 633e45b remove unneeded shell option from cpp-subprocess (Sebastian Falbesoner) Pull request description: The newly introduced cpp-subprocess library provides a good number of options for the `Popen` class: https://github.com/bitcoin/bitcoin/blob/0de63b8b46eff5cda85b4950062703324ba65a80/src/util/subprocess.hpp#L1009-L1020 Some of them are either not fully implemented (`shell`, missing an implementation on Windows), implemented in an ugly way (e.g. using "Impoverished, meager, needy, truly needy version of type erasure" for `preexec_func` according to the author's own words) or simply unlikely to be ever needed for our external signer use-case (`defer_spawn`). Instead of maintaining incomplete and/or unneeded code, I'd suggest to get rid of it and only keep support for options if there is a strong reason for it. ACKs for top commit: achow101: ACK 13adbf7 hebasto: re-ACK 13adbf7. Tree-SHA512: 8270da27891cb659da2ef6062a23f4b86331859b15ac27b79ae7433b14f5bd7efaba621f2b3ba1953708d0f38377a8bd23ef1cc0f28b9c152ac8958dd9eec6b0
63328b0 to
52b8033
Compare
vijaydasmp
changed the title
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
6 participants
bitcoin backport