backport: Merge bitcoin#30703, 30024, 29707, 28336, 29335, 30063, 30161, 30160 - #7308
backport: Merge bitcoin#30703, 30024, 29707, 28336, 29335, 30063, 30161, 30160#7308vijaydasmp wants to merge 8 commits into
Conversation
vijaydasmp
changed the title
|
vijaydasmp
changed the title
|
@codex review pr |
💡 Codex Reviewhttps://github.com/dashpay/dash/blob/a8e58ba2206f2dd9b15e43639e04cf36c51e07ce/test/functional/test_runner.py#L684-L686 The no-space error branch calls ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
29d7630 to
664a58e
Compare
vijaydasmp
changed the title
|
🕓 Ready for review — 6 ahead in queue (commit d558eae) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 664a58e9f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
| // Deallocate old buffer and update housekeeping. | ||
| std::allocator<T>().deallocate(m_buffer, m_capacity); |
There was a problem hiding this comment.
Reallocate() unconditionally calls std::allocator<T>().deallocate(m_buffer, m_capacity), but a default-constructed VecDeque has m_buffer == nullptr and m_capacity == 0, and ~VecDeque() always invokes Reallocate(0). That means destruction of an untouched deque performs a deallocate call with a pointer/size pair that was never returned by allocate, which violates allocator preconditions and is undefined behavior (it can break under hardened allocators or sanitizer-enabled builds).
Useful? React with 👍 / 👎.
| @@ -659,6 +680,11 @@ def run_tests(*, test_list, src_dir, build_dir, tmpdir, jobs=1, attempts=1, enab | |||
| logging.debug("Early exiting after test failure") | |||
| break | |||
|
|
|||
| if "[Errno 28] No space left on device" in stdout: | |||
There was a problem hiding this comment.
The new ENOSPC fast-fail path only scans stdout, but Python tracebacks and many subprocess failures report "[Errno 28] No space left on device" on stderr. In those common cases the runner will miss the disk-full condition and continue scheduling tests, defeating the intended early-stop behavior and often producing cascading failures.
Useful? React with 👍 / 👎.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR updates miniupnpc to 2.2.7 with CMake-based packaging and new patches, adds BitSet and VecDeque utility headers with fuzz targets, centralizes public-key parsing through HexToPubKey and updates call sites and tests, adds functional test runner disk-space checks and a nocleanup option, renames a debug-log wait method, and adjusts comments plus a few tooling commands. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 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.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc/dependencies.md`:
- Line 42: Update the MiniUPnPc entry in the Markdown table: locate the row
containing "MiniUPnPc" and the current "2.2.2" Version used value (and the link
to depends/packages/miniupnpc.mk) and change the Version used cell to "2.2.7" so
the doc matches the package bump.
In `@src/util/bitset.h`:
- Around line 198-205: The Doxygen comments for IntBitSet's bitwise assignment
operators are wrong: swap the descriptions for operator|= and operator&= so
operator|= is documented as OR and operator&= as AND, correct the operator^=
comment to read "from this and a" instead of "from this as a", and ensure the
operator-= comment still reads "AND NOT"; update the comment lines associated
with the methods named operator|=(const IntBitSet&), operator&=(const
IntBitSet&), and operator^=(const IntBitSet&) accordingly.
In `@test/functional/test_runner.py`:
- Around line 683-686: Fix the typo in the early-exit message string inside the
conditional that checks for "[Errno 28] No space left on device" (the sys.exit
call that references tmpdir and stdout); change "insuffient" to "insufficient"
so the message reads "...due to insufficient free space in {tmpdir}." Ensure the
rest of the sys.exit string remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c1d7857f-4cdd-4a99-afca-9c81182b50de
📥 Commits
Reviewing files that changed from the base of the PR and between 5fd84aa and 664a58e9f88a95413e49885573dea1827cf0f9d9.
📒 Files selected for processing (29)
Makefile.amdepends/packages/miniupnpc.mkdepends/patches/miniupnpc/cmake_get_src_addr.patchdepends/patches/miniupnpc/dont_leak_info.patchdepends/patches/miniupnpc/fix_windows_snprintf.patchdepends/patches/miniupnpc/no_libtool.patchdepends/patches/miniupnpc/respect_mingw_cflags.patchdoc/dependencies.mdsrc/Makefile.amsrc/Makefile.test.includesrc/net_processing.cppsrc/outputtype.cppsrc/policy/policy.cppsrc/rpc/output_script.cppsrc/rpc/util.cppsrc/test/fuzz/bitset.cppsrc/test/fuzz/vecdeque.cppsrc/util/bitset.hsrc/util/vecdeque.hsrc/wallet/rpc/backup.cppsrc/wallet/rpc/spend.cpptest/functional/feature_init.pytest/functional/rpc_rawtransaction.pytest/functional/test_framework/test_node.pytest/functional/test_runner.pytest/functional/wallet_basic.pytest/functional/wallet_fundrawtransaction.pytest/get_previous_releases.pytest/sanitizer_suppressions/ubsan
💤 Files with no reviewable changes (2)
- depends/patches/miniupnpc/no_libtool.patch
- depends/patches/miniupnpc/respect_mingw_cflags.patch
| /** Set this object's bits to be the binary AND between respective bits from this and a. */ | ||
| constexpr IntBitSet& operator|=(const IntBitSet& a) noexcept { m_val |= a.m_val; return *this; } | ||
| /** Set this object's bits to be the binary OR between respective bits from this and a. */ | ||
| constexpr IntBitSet& operator&=(const IntBitSet& a) noexcept { m_val &= a.m_val; return *this; } | ||
| /** Set this object's bits to be the binary AND NOT between respective bits from this and a. */ | ||
| constexpr IntBitSet& operator-=(const IntBitSet& a) noexcept { m_val &= ~a.m_val; return *this; } | ||
| /** Set this object's bits to be the binary XOR between respective bits from this as a. */ | ||
| constexpr IntBitSet& operator^=(const IntBitSet& a) noexcept { m_val ^= a.m_val; return *this; } |
There was a problem hiding this comment.
Docstring labels for operator|= and operator&= are swapped (plus a typo on line 204).
The implementation is correct, but the Doxygen-style comments mislabel the bitwise operation. The |= operator is documented as AND and &= as OR. Also "from this as a" should read "from this and a" on the XOR comment.
Note: the corresponding MultiIntBitSet docstrings (lines 419/427) are already correct.
📝 Proposed fix
- /** Set this object's bits to be the binary AND between respective bits from this and a. */
+ /** Set this object's bits to be the binary OR between respective bits from this and a. */
constexpr IntBitSet& operator|=(const IntBitSet& a) noexcept { m_val |= a.m_val; return *this; }
- /** Set this object's bits to be the binary OR between respective bits from this and a. */
+ /** Set this object's bits to be the binary AND between respective bits from this and a. */
constexpr IntBitSet& operator&=(const IntBitSet& a) noexcept { m_val &= a.m_val; return *this; }
/** Set this object's bits to be the binary AND NOT between respective bits from this and a. */
constexpr IntBitSet& operator-=(const IntBitSet& a) noexcept { m_val &= ~a.m_val; return *this; }
- /** Set this object's bits to be the binary XOR between respective bits from this as a. */
+ /** Set this object's bits to be the binary XOR between respective bits from this and a. */
constexpr IntBitSet& operator^=(const IntBitSet& a) noexcept { m_val ^= a.m_val; return *this; }📝 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.
| /** Set this object's bits to be the binary AND between respective bits from this and a. */ | |
| constexpr IntBitSet& operator|=(const IntBitSet& a) noexcept { m_val |= a.m_val; return *this; } | |
| /** Set this object's bits to be the binary OR between respective bits from this and a. */ | |
| constexpr IntBitSet& operator&=(const IntBitSet& a) noexcept { m_val &= a.m_val; return *this; } | |
| /** Set this object's bits to be the binary AND NOT between respective bits from this and a. */ | |
| constexpr IntBitSet& operator-=(const IntBitSet& a) noexcept { m_val &= ~a.m_val; return *this; } | |
| /** Set this object's bits to be the binary XOR between respective bits from this as a. */ | |
| constexpr IntBitSet& operator^=(const IntBitSet& a) noexcept { m_val ^= a.m_val; return *this; } | |
| /** Set this object's bits to be the binary OR between respective bits from this and a. */ | |
| constexpr IntBitSet& operator|=(const IntBitSet& a) noexcept { m_val |= a.m_val; return *this; } | |
| /** Set this object's bits to be the binary AND between respective bits from this and a. */ | |
| constexpr IntBitSet& operator&=(const IntBitSet& a) noexcept { m_val &= a.m_val; return *this; } | |
| /** Set this object's bits to be the binary AND NOT between respective bits from this and a. */ | |
| constexpr IntBitSet& operator-=(const IntBitSet& a) noexcept { m_val &= ~a.m_val; return *this; } | |
| /** Set this object's bits to be the binary XOR between respective bits from this and a. */ | |
| constexpr IntBitSet& operator^=(const IntBitSet& a) noexcept { m_val ^= a.m_val; return *this; } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/util/bitset.h` around lines 198 - 205, The Doxygen comments for
IntBitSet's bitwise assignment operators are wrong: swap the descriptions for
operator|= and operator&= so operator|= is documented as OR and operator&= as
AND, correct the operator^= comment to read "from this and a" instead of "from
this as a", and ensure the operator-= comment still reads "AND NOT"; update the
comment lines associated with the methods named operator|=(const IntBitSet&),
operator&=(const IntBitSet&), and operator^=(const IntBitSet&) accordingly.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the reported findings against head 664a58e9f88a95413e49885573dea1827cf0f9d9. The MiniUPnPc documentation mismatch is real and actionable; the claimed missing read-only reindex test hunk is factual upstream context but not a concrete issue in Dash's current tree because the referenced file is absent and no broken call remains.
Reviewed commit: 664a58e9
💬 1 nitpick(s)
| @@ -39,7 +39,7 @@ You can find installation instructions in the `build-*.md` file for your platfor | |||
| | Dependency | Releases | Version used | Minimum required | Runtime | | |||
| | --- | --- | --- | --- | --- | | |||
| | [libnatpmp](../depends/packages/libnatpmp.mk) | [link](https://github.com/miniupnp/libnatpmp/) | commit [07004b9...](https://github.com/miniupnp/libnatpmp/tree/07004b97cf691774efebe70404cf22201e4d330d) | | No | | |||
| | [MiniUPnPc](../depends/packages/miniupnpc.mk) | [link](https://miniupnp.tuxfamily.org/) | [2.2.2](https://github.com/bitcoin/bitcoin/pull/20421) | 2.1 | No | | |||
| | [MiniUPnPc](../depends/packages/miniupnpc.mk) | [link](https://miniupnp.tuxfamily.org/) | [2.2.2](https://github.com/bitcoin/bitcoin/pull/29707) | 2.1 | No | | |||
There was a problem hiding this comment.
💬 Nitpick: MiniUPnPc documented version still says 2.2.2 after the 2.2.7 bump
The depends package now builds MiniUPnPc 2.2.7, but the dependency table still lists the version as 2.2.2 while pointing at bitcoin#29707. This leaves the documentation inconsistent with the actual package version selected by depends/packages/miniupnpc.mk.
source: ['claude', 'claude-backport-reviewer']
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18b5096736
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (size_t new_pos = 0; new_pos < m_size; ++new_pos) { | ||
| std::construct_at(new_buffer + new_pos, std::move(*(m_buffer + old_pos))); |
There was a problem hiding this comment.
When T has a potentially throwing move constructor, a growth operation such as reserve() or emplace_back() can throw here after earlier elements have already been destroyed in the old buffer. Because m_size, m_offset, and m_buffer are not updated and the partially populated new buffer is leaked, subsequent access or destruction treats destroyed old slots as live objects, causing undefined behavior. Either require nothrow moves or add rollback/copy-based relocation so an exception leaves the deque valid.
Useful? React with 👍 / 👎.
| if "[Errno 28] No space left on device" in stdout: | ||
| sys.exit(f"Early exiting after test failure due to insuffient free space in {tmpdir}\n" | ||
| f"Test execution data left in {tmpdir}.\n" | ||
| f"Additional storage is needed to execute testing.") |
There was a problem hiding this comment.
When ENOSPC is detected while multiple tests are running, this immediate sys.exit() bypasses the dangling-process cleanup at the end of run_tests(). The sibling test processes and any nodes they started therefore remain alive after the runner exits and may continue consuming the already exhausted filesystem; terminate the active job_queue.jobs before exiting or route this condition through the normal cleanup path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 550f4c1aa4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (m_size == m_capacity) Reallocate((m_size + 1) * 2); | ||
| std::construct_at(m_buffer + BufferIndex(m_size), std::forward<Args>(args)...); |
There was a problem hiding this comment.
When a full deque is passed one of its own elements, such as deque.push_back(deque.front()), this reallocation moves or copies the buffer and then deallocates it before the forwarded argument is used on the next line. The argument consequently refers to destroyed storage, causing a use-after-free or a copy from a moved-from object; construct a temporary before reallocating or otherwise account for arguments that alias the deque. The equivalent growth path in emplace_front has the same issue.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 550f4c1aa4. The 9 backports (BitSet/VecDeque + fuzz harnesses, MiniUPnPc 2.2.7/CMake, HexToPubKey consistency, MAX_SCRIPT_ELEMENT_SIZE cleanup, test_runner disk-full handling, busy_wait_for_debug_log rename, get_previous_releases.py simplification, TIMEOUT removal) are cleanly integrated: Makefile.am and Makefile.test.include register the new sources, the ubsan suppression for bitset_detail::PopCount is in place, and RPC pubkey error strings in tests match the new C++ messages. The prior nitpick about the stale MiniUPnPc 2.2.2 version in doc/dependencies.md has been resolved. No new in-scope findings.
|
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): Verified PR #7308 at exact SHA b1304f0ceacd3d106eb65602a5b2bc0a3722c916; backport-reviewer ran. The only surviving issue is a confirmed incomplete bitcoin#30006 backport: the upstream hunk for feature_reindex_readonly.py was omitted because Dash is missing the bitcoin#27850 prerequisite test, and the PR does not document an intentional exclusion.
🔴 3 blocking
Findings not posted inline (1)
These findings could not be anchored to the current diff, but they are still part of this review.
- [BLOCKING]
test/functional/feature_reindex_readonly.py:78: Missing prerequisite: bitcoin#27850 — The bitcoin#30006 backport is partial because upstream also updates test/functional/feature_reindex_readonly.py, replacing the old busy wait_for_debug_log([b"Reindexing finished"]) call with assert_debug_log(["Reindexing finished"], timeout=60). Dash only backported the helper rename in test_node...
🤖 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/test_node.py`:
- [BLOCKING] test/functional/test_framework/test_node.py:493: Backport omits bitcoin#30006 prerequisite test
The upstream bitcoin#30006 commit modifies three files: feature_init.py, test_node.py, and test/functional/feature_reindex_readonly.py. This Dash merge only includes the first two because feature_reindex_readonly.py is absent; that file was introduced upstream by bitcoin#27850 and registered in test_runner.py. The current Dash tree has no feature_reindex_readonly.py, reindex_readonly, or BlockstoreReindex equivalent, and the PR claims a full bitcoin#30006 backport without an explicit exclusion. Add or adapt the bitcoin#27850 read-only blockstore test prerequisite before applying the bitcoin#30006 helper rename so the backport is complete.
In `test/functional/feature_reindex_readonly.py`:
- [BLOCKING] test/functional/feature_reindex_readonly.py:78: Missing prerequisite: bitcoin#27850
The bitcoin#30006 backport is partial because upstream also updates test/functional/feature_reindex_readonly.py, replacing the old busy wait_for_debug_log([b"Reindexing finished"]) call with assert_debug_log(["Reindexing finished"], timeout=60). Dash only backported the helper rename in test_node.py and the feature_init.py call-site because feature_reindex_readonly.py is absent. That test was introduced upstream by de8f9123af ("test: cover read-only blockstore") in bitcoin#27850, with later followups before bitcoin#30006. I do not see a Dash-specific reason to exclude this generic blockstore/reindex coverage. This is non-blocking per the agent (restored to blocking by policy) because production code is unaffected and the present call-sites were adapted cleanly, but the claimed full backport of bitcoin#30006 is missing the upstream test dependency chain.
---
**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_reindex_readonly.py:78: Missing prerequisite: bitcoin#27850
The bitcoin#30006 backport is partial because upstream also updates test/functional/feature_reindex_readonly.py, replacing the old busy wait_for_debug_log([b"Reindexing finished"]) call with assert_debug_log(["Reindexing finished"], timeout=60). Dash only backported the helper rename in test_node.py and the feature_init.py call-site because feature_reindex_readonly.py is absent. That test was introduced upstream by de8f9123af ("test: cover read-only blockstore") in bitcoin#27850, with later followups before bitcoin#30006. I do not see a Dash-specific reason to exclude this generic blockstore/reindex coverage. This is non-blocking per the agent (restored to blocking by policy) because production code is unaffected and the present call-sites were adapted cleanly, but the claimed full backport of bitcoin#30006 is missing the upstream test dependency chain.
---
**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.
| @@ -490,7 +490,7 @@ def assert_debug_log(self, expected_msgs, unexpected_msgs=None, timeout=2): | |||
| self._raise_assertion_error('Expected messages "{}" does not partially match log:\n\n{}\n\n'.format(str(expected_msgs), print_log)) | |||
|
|
|||
| @contextlib.contextmanager | |||
| def wait_for_debug_log(self, expected_msgs, timeout=60): | |||
| def busy_wait_for_debug_log(self, expected_msgs, timeout=60): | |||
There was a problem hiding this comment.
🔴 Blocking: Backport omits bitcoin#30006 prerequisite test
The upstream bitcoin#30006 commit modifies three files: feature_init.py, test_node.py, and test/functional/feature_reindex_readonly.py. This Dash merge only includes the first two because feature_reindex_readonly.py is absent; that file was introduced upstream by bitcoin#27850 and registered in test_runner.py. The current Dash tree has no feature_reindex_readonly.py, reindex_readonly, or BlockstoreReindex equivalent, and the PR claims a full bitcoin#30006 backport without an explicit exclusion. Add or adapt the bitcoin#27850 read-only blockstore test prerequisite before applying the bitcoin#30006 helper rename so the backport is complete.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Backport omits bitcoin#30006 prerequisite test 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.
|
This pull request has conflicts, please rebase. |
0d90a61 to
1929417
Compare
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:
If these PRs merge firstThis PR will likely need a rebase:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1929417102
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for test_result in test_results: | ||
| all_passed = all_passed and test_result.was_successful | ||
| results_writer.writerow([test_result.name, test_result.status, str(test_result.time)]) | ||
| results_writer.writerow(['ALL', ("Passed" if all_passed else "Failed"), str(total_runtime)]) |
There was a problem hiding this comment.
When --coverage and --resultsfile are used together, this writes ALL,Passed whenever every test script passes, but coverage.report_rpc_coverage() runs afterward and can make the runner exit nonzero for uncovered RPCs. Consumers of the CSV will therefore record a successful run despite the command failing; generate the aggregate after coverage is evaluated or include the coverage result in it.
Useful? React with 👍 / 👎.
1929417 to
28bc3e8
Compare
vijaydasmp
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28bc3e8acb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| I m_val; /**< The remaining bits of (*m_ptr)[m_idx]. */ | ||
| unsigned m_pos; /**< The last reported position. */ | ||
| unsigned m_idx; /**< The index in *m_ptr currently being iterated over. */ | ||
| constexpr Iterator(const std::array<I, N>& ref) noexcept : m_ptr(&ref), m_idx(0) |
There was a problem hiding this comment.
When a MultiIntBitSet has no bits set, this constructor finishes with m_idx == N without ever assigning m_pos. Iterator explicitly permits copying, so code that saves and then copies begin() before comparing it with end() copies an indeterminate unsigned member, which is undefined behavior even though no dereference occurs. Initialize the iterator position for the empty/end state.
Useful? React with 👍 / 👎.
28bc3e8 to
e92b726
Compare
vijaydasmp
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — GLM Flash + Sol
The eight-backport stack is generally integrated cleanly, and the prior bitcoin#30006 findings no longer apply to this exact range. Two blocking issues remain: bitcoin#30024 silently omits an upstream test-file hunk without documenting the exclusion, and the disk-full handling from bitcoin#29335 is bypassed by Dash CI's retry/fail-fast configuration and can skip parallel-job cleanup.
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: dash-core-commit-history); reviewer 3: glm-5.3-flash (agent: phase1-reviewer, role: backport-reviewer); reviewer 4: gpt-5.6-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-5.6-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 6: gpt-5.6-sol (agent: phase2-reviewer, role: backport-reviewer); final verifier: gpt-5.6-sol (agent: sol-verifier, role: final-verifier)
Review provenance
- Phase 1 reviewers (GLM Flash):
glm-5.3-flash— general (completed); agentphase1-reviewer,glm-5.3-flash— dash-core-commit-history (completed); agentphase1-reviewer,glm-5.3-flash— backport-reviewer (completed); agentphase1-reviewer - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier; agentsol-verifier - Phase 2 reviewers (Sol):
gpt-5.6-sol— general (completed); agentphase2-reviewer,gpt-5.6-sol— dash-core-commit-history (completed); agentphase2-reviewer,gpt-5.6-sol— backport-reviewer (completed); agentphase2-reviewer
🔴 2 blocking
1 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_runner.py`:
- [BLOCKING] test/functional/test_runner.py:686-693: Handle ENOSPC before Dash retry, fail-fast, and cleanup branches
Dash CI invokes the runner with `--attempts=3 --failfast`. `TestHandler.get_next()` retries a failed process at lines 807-834 before yielding its captured output, so an ENOSPC failure is retried twice. When the final attempt is yielded, the `failfast` branch at lines 686-688 runs before this check, making the new diagnostic unreachable in the standard CI configuration. In non-fail-fast parallel runs, the direct `sys.exit()` also bypasses the dangling-job cleanup at lines 711-715 and can leave sibling tests and their nodes running while the filesystem is exhausted. Detect ENOSPC in the captured stdout or stderr before retrying, propagate that condition ahead of fail-fast handling, and terminate through the common job-cleanup path.
In `test/functional/p2p_segwit.py`:
- [BLOCKING] test/functional/p2p_segwit.py: Document the omitted SegWit test hunk from bitcoin#30024
Upstream commit ffc674595cb for bitcoin#30024 changes four files, including the `test_max_witness_push_length()` docstring in `test/functional/p2p_segwit.py`. Dash commit 8109f3c53de lands only the three C++ comment replacements. The missing file reflects Dash's lack of the upstream SegWit test subsystem, but neither the PR body nor the backport commit records that exclusion. Because the commit advertises a full bitcoin#30024 merge, permanently document that this test-file hunk was intentionally omitted due to Dash's no-SegWit divergence, or backport the prerequisite test area.
| logging.debug("Early exiting after test failure") | ||
| break | ||
|
|
||
| if "[Errno 28] No space left on device" in stdout: | ||
| sys.exit(f"Early exiting after test failure due to insuffient free space in {tmpdir}\n" | ||
| f"Test execution data left in {tmpdir}.\n" | ||
| f"Additional storage is needed to execute testing.") |
There was a problem hiding this comment.
🔴 Blocking: Handle ENOSPC before Dash retry, fail-fast, and cleanup branches
Dash CI invokes the runner with --attempts=3 --failfast. TestHandler.get_next() retries a failed process at lines 807-834 before yielding its captured output, so an ENOSPC failure is retried twice. When the final attempt is yielded, the failfast branch at lines 686-688 runs before this check, making the new diagnostic unreachable in the standard CI configuration. In non-fail-fast parallel runs, the direct sys.exit() also bypasses the dangling-job cleanup at lines 711-715 and can leave sibling tests and their nodes running while the filesystem is exhausted. Detect ENOSPC in the captured stdout or stderr before retrying, propagate that condition ahead of fail-fast handling, and terminate through the common job-cleanup path.
source: ['claude', 'codex']
…releases.py fa5aeab test: Avoid duplicate curl call in get_previous_releases.py (MarcoFalke) Pull request description: Seems odd having to translate `404` to "Binary tag was not found". Also, it seems odd to write a for-loop over a list with one item. Fix both issues by just using a single call to `curl --fail ...`. Can be tested with: `test/get_previous_releases.py -b v99.99.99` Before: ``` Releases directory: releases Fetching: https://bitcoincore.org/bin/bitcoin-core-99.99.99/bitcoin-99.99.99-x86_64-linux-gnu.tar.gz % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 0 286k 0 0 0 0 0 0 --:--:-- 0:00:01 --:--:-- 0 Binary tag was not found ``` After: ``` Releases directory: releases Fetching: https://bitcoincore.org/bin/bitcoin-core-99.99.99/bitcoin-99.99.99-x86_64-linux-gnu.tar.gz % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 0 286k 0 0 0 0 0 0 --:--:-- 0:00:01 --:--:-- 0 curl: (22) The requested URL returned error: 404 ACKs for top commit: fanquake: ACK fa5aeab brunoerg: utACK fa5aeab tdb3: tested ACK fa5aeab Tree-SHA512: d5d31e0bccdd9de9b4a8ecf2e69348f4e8cee773050c8259b61db1ce5de73f6fbfffbe8c4d2571f7bef2de29cb42fd244573deebfbec614e487e76ef41681b9c
…_SCRIPT_ELEMENT_SIZE ffc6745 Replace remaining "520" magic numbers with MAX_SCRIPT_ELEMENT_SIZE (Jon Atack) Pull request description: Noticed these while reviewing BIPs yesterday. It would be clearer and more future-proof to refer to their constant name. ACKs for top commit: instagibbs: ACK ffc6745 sipa: ACK ffc6745 achow101: ACK ffc6745 glozow: ACK ffc6745, agree it's clearer for these comments to refer to the greppable name of the limit rather than the number Tree-SHA512: 462afc1c64543877ac58cb3acdb01d42c6d08abfb362802f29f3482d75401a2a8adadbc2facd222a9a9fefcaab6854865ea400f50ad60bec17831d29f7798afe
5195baa depends: fix miniupnpc snprintf usage on Windows (fanquake) 3c2d440 depends: switch miniupnpc to CMake (Cory Fields) f5618c7 depends: add upstream CMake patch to miniupnpc (fanquake) 6866b57 depends: miniupnpc 2.2.7 (fanquake) Pull request description: This picks up one of the changes from bitcoin#29232, which is a switch to building miniupnpc with CMake. It includes an update to the most recent version of miniupnpc (2.2.7), which means we can drop one patch from that commit, and includes a new patch for a change I've upstreamed miniupnp/miniupnp#721, as well as some suggestions from the previous PR. ACKs for top commit: theuni: ACK 5195baa. TheCharlatan: utACK 5195baa Tree-SHA512: 5b27e132cd5eed285e9be34c8b96893417d92a1ae55c99345c9a89e1c1c5e40e4bc840bc061b879758b2b11fcb520cd98c3da985c1e153f2e5380cf63efe2d69
…ific error messages 98570fe test: add coverage for parsing cryptographically invalid pubkeys (Sebastian Falbesoner) c740b15 rpc: use `HexToPubKey` helper for all legacy pubkey-parsing RPCs (Sebastian Falbesoner) 100e8a7 rpc: check and throw specific pubkey parsing errors in `HexToPubKey` (Sebastian Falbesoner) Pull request description: Parsing legacy public keys can fail for three reasons (in this order): - pubkey is not in hex - pubkey has an invalid length (not 33 or 65 bytes for compressed/uncompressed, respectively) - pubkey is crytographically invalid, i.e. is not on curve (`CPubKey.IsFullyValid()` check) Many RPCs currently perform these checks manually with different error messages, even though we already have a `HexToPubKey` helper. This PR puts all three checks in this helper (the length check was done on the call-sites before), adds specific error messages for each case, and consequently uses it for all RPCs that parse legacy pubkeys. This leads to deduplicated code and also to more consistent and detailed error messages for the user. Affected RPC calls are `createmultisig`, `addmultisigaddress`, `importpubkey`, `importmulti`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send` and `sendall`. Note that the error code (-5 a.k.a. `RPC_INVALID_ADDRESS_OR_KEY`) doesn't change in any of the causes, so the changes are not breaking RPC API compatibility. Only the messages are more specific. The last commits adds test coverage for the cryptographically invalid (not-on-curve) pubkey case which wasn't exercised before. ACKs for top commit: stratospher: tested ACK 98570fe. davidgumberg: ACK bitcoin@98570fe Eunovo: Tested ACK bitcoin@98570fe achow101: ACK 98570fe Tree-SHA512: cfa474176e95b5b18f3a9da28fdd9e87195cd58994c1331198f2840925fff322fd323a6371feab74a1b32e4b9ea58a6dc732fa751b4cdd45402c1029af609ece
357ad11 test: Handle functional test disk-full error (Brandon Odiwuor) Pull request description: Fixes: bitcoin#23099 Handle disk-full more gracefully in functional tests ACKs for top commit: itornaza: re-ACK 357ad11 achow101: reACK 357ad11 cbergqvist: reACK 357ad11. Looks good! tdb3: re ACK for 357ad11 Tree-SHA512: 9bb0d3fbe84600c88873b9f55d4b5d1443f79ec303467680c301be2b4879201387f203d9d1984169461f321037189b5e10a6a4b9d61750de638f072d2f95d77e
… variable 189d0da build, test: Remove unused `TIMEOUT` environment variable (Hennadii Stepanov) Pull request description: Setting the `TIMEOUT` environment variable has been a noop in both cases since its introduction. It seems to have been inadvertently copy-pasted from existed code. For example, in commit d80e3cb, it was needlessly copied from a valid case a few lines above for the `qa/pull-tester/run-bitcoind-for-test.sh` script. ACKs for top commit: maflcko: utACK 189d0da edilmedeiros: ACK 189d0da Tree-SHA512: 61111eba30e0c82a0220bea48eba451cd9caa68785b48ec8a91059ca5aadfaff2f6d2ccdc5aa737c5cefa33579cb735431bb9e94bda8fa047825d7bd28d542fb
7b8eea0 tests: add fuzz tests for VecDeque (Pieter Wuille) 62fd24a util: add VecDeque (Pieter Wuille) Pull request description: Extracted from bitcoin#30126. This adds a `VecDeque` data type, inspired by `std::deque`, but backed by a single allocated memory region used as a ring buffer instead of a linked list of arrays. This gives better memory locality and less allocation overhead, plus better guarantees (some C++ standard library implementations, though not libstdc++ and libc++, use a separate allocation per element in a deque). It is intended for the candidate set search queue in bitcoin#30126, but may be useful as a replacement for `std::deque` in other places too. It's not a full drop-in replacement, as I did not add iteration support which is unnecessary for the intended use case, but nothing prevents adding that if needed. Everything is tested through a simulation-based fuzz test that compares the behavior with normal `std::deque` equivalent operations, both for trivially-copyable/destructible types and others. ACKs for top commit: instagibbs: reACK bitcoin@7b8eea0 cbergqvist: re-ACK 7b8eea0 hebasto: re-ACK 7b8eea0, I've verified changes since my recent [review](bitcoin#30161 (review)) with glozow: ACK 7b8eea0 Tree-SHA512: 1b62f3ba1a43a1293d8c9de047e2399442e74c46de2df81406151fe27538716ce265f35fb6779ee56d77a39cddf8fb4b4e15bda8f04ebf3b149e2f05fa55cb21
47f705b tests: add fuzz tests for BitSet (Pieter Wuille) 59a6df6 util: add BitSet (Pieter Wuille) Pull request description: Extracted from bitcoin#30126. This introduces the `BitSet` data structure, inspired by `std::bitset`, but with a few features that cannot be implemented on top without efficiency loss: * Finding the first set bit (`First`) * Finding the last set bit (`Last`) * Iterating over all set bits (`begin` and `end`). And a few other operators/member functions that help readability for bitcoin#30126: * `operator-` for set subtraction * `Overlaps()` for testing whether intersection is non-empty * `IsSupersetOf()` for testing (non-strict) supersetness * `IsSubsetOf()` for testing (non-strict) subsetness * `Fill()` to construct a set with all numbers from 0 to n-1, inclusive * `Singleton()` to construct a set with one specific element. Everything is tested through a simulation-based fuzz test that compares the behavior with normal `std::bitset` equivalent operations. ACKs for top commit: instagibbs: ACK bitcoin@47f705b achow101: ACK 47f705b cbergqvist: re-ACK 47f705b theStack: Code-review ACK 47f705b Tree-SHA512: e451bf4b801f193239ee434b6b614f5a2ac7bb49c70af5aba24c2ac0c54acbef4672556800e4ac799ae835632bdba716209c5ca8c37433a6883dab4eb7cd67c1
e92b726 to
d558eae
Compare
5 participants
bitcoin backports