backport: Merge bitcoin#28831, 29657, 29189, 30633, 27905, 28093, 30026, 27780, 28789#7378
backport: Merge bitcoin#28831, 29657, 29189, 30633, 27905, 28093, 30026, 27780, 28789#7378vijaydasmp wants to merge 9 commits into
Conversation
44445ae test: Avoid intermittent failures in feature_init (MarcoFalke) Pull request description: The code not only modifies block dat files, but also leveldb files, which may be of smaller size. Such corruption may not force leveldb to abort, according to the intermittent test failures. Fix the intermittent test failures by reverting bitcoin@5ab6419 . ACKs for top commit: kevkevinpal: lgtm ACK [44445ae](bitcoin@44445ae) fjahr: ACK 44445ae theStack: ACK 44445ae Tree-SHA512: 8084e4aeb8a976c1706a1898d7854c55d0c4ec7b5a08f65f97ffc173c935f9b0e0c1caef7be1538a458e4c018f7bd1948173349ec76ca48bc4013a63f284bb0f
|
This pull request has conflicts, please rebase. |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 8bf7f8d) |
![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: 4a4f775ff3
ℹ️ 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".
| ConnectionTypeForNetinfo(peer.conn_type), | ||
| peer.network, | ||
| peer.transport_protocol_type.starts_with('v') == 0 ? peer.transport_protocol_type[1] : ' ', | ||
| (peer.transport_protocol_type.size() == 2 && peer.transport_protocol_type[0] == 'v') == 0 ? peer.transport_protocol_type[1] : ' ', |
There was a problem hiding this comment.
This keeps the old == 0 negation, so the new length check is applied to the wrong branch: documented v1/v2 transports print a blank version, while any non-v? value still indexes transport_protocol_type[1]. If the daemon ever returns a short or malformed transport string, dash-cli -netinfo can still read past the string instead of falling back to a space.
Useful? React with 👍 / 👎.
WalkthroughThe PR updates core validation, fuzzing, functional tests, and release-note text. It also changes secp256k1 public headers and build/CI wiring for static-library use, refactors internal field and RNG helpers, and adds EllSwift exhaustive test support. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/secp256k1/.cirrus.yml (3)
67-94: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a single
matrixkey in this task.YAML does not merge duplicate keys here. The second
matrixat Line 86 overwrites the feature/config matrix declared at Line 70, so this task keeps only the compiler axis and silently drops the widened coverage you just added.🤖 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/secp256k1/.cirrus.yml` around lines 67 - 94, The x86_64 Linux task currently defines `matrix` twice, so the later compiler-only `matrix` in this task overwrites the earlier feature/config matrix and drops coverage. Merge both axes into a single `matrix` under the same task definition, keeping the existing env combinations and the `CC` variants together so all intended builds run. Use the task block with the `name: "x86_64: Linux (Debian stable)"` as the place to fix this.Source: Linters/SAST tools
282-320: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe sanitizer task has duplicate
matrixkeys.Only the last
matrixblock is preserved here, so the Valgrind/UBSan-ASan-LSan variants and the ASM/ECMULT axis above it are dropped instead of combined. This effectively rewrites the sanitizer coverage to a much smaller set of jobs.🤖 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/secp256k1/.cirrus.yml` around lines 282 - 320, The task definition in the Cirrus config has multiple `matrix` keys under the same `task`, so only the last one is applied and the Valgrind/UBSan-ASan-LSan and ASM/ECMULT variants are lost. Merge the `matrix` entries into a single combined `matrix` structure for this task, keeping the existing axes from the sanitizer job and the `CC`/`HOST` variants together so all intended job combinations are generated.
120-138: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe second
envblock overwrites the first one.The
envmapping at Line 125 replaces the Homebrew/MAKEFLAGSsettings from Line 120 instead of extending them. That changes the macOS task setup in a way that is easy to miss and makes the matrix results differ from the intended configuration.🤖 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/secp256k1/.cirrus.yml` around lines 120 - 138, The Cirrus configuration has two separate env mappings, and the second one in the main task setup overwrites the Homebrew and MAKEFLAGS settings instead of extending them. Merge the environment entries into a single env block so the macOS task keeps HOMEBREW_NO_AUTO_UPDATE, HOMEBREW_NO_INSTALL_CLEANUP, and MAKEFLAGS while still setting ASM, WITH_VALGRIND, CTIMETESTS, and CC before the matrix runs.
🤖 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 `@src/bitcoin-cli.cpp`:
- Line 556: The transport marker selection in the `-netinfo` output has an
inverted guard because the safety check is negated with `== 0`, causing
unexpected strings to fall into the indexing branch. Update the conditional in
the `bitcoin-cli` netinfo formatting logic so the `peer.transport_protocol_type`
length-and-prefix check is used directly, and only index `[1]` when the
transport string is actually one of the expected versioned values; otherwise
fall back to the placeholder character.
In `@src/secp256k1/ci/linux-debian.Dockerfile`:
- Around line 29-30: The Dockerfile is still defaulting the GCC snapshot to
version 14, so the CI image won’t validate GCC 15 compatibility. Update the
`GCC_SNAPSHOT_MAJOR` argument in `linux-debian.Dockerfile` to 15 so the
`gcc-snapshot` build uses the intended compiler version. Keep the change
confined to the Docker build argument used for the GCC snapshot setup.
In `@src/secp256k1/src/ecmult_gen_compute_table_impl.h`:
- Around line 25-26: The invariant checks in ecmult_gen_compute_table_impl
should run before any allocation-size computation so they guard the first n * g
overflow path. Move the VERIFY_CHECK(g > 0) and VERIFY_CHECK(n > 0) statements
to immediately after g and n are computed, before the checked_malloc size
expression in this function, so the early size calculation cannot happen with
invalid values.
---
Outside diff comments:
In `@src/secp256k1/.cirrus.yml`:
- Around line 67-94: The x86_64 Linux task currently defines `matrix` twice, so
the later compiler-only `matrix` in this task overwrites the earlier
feature/config matrix and drops coverage. Merge both axes into a single `matrix`
under the same task definition, keeping the existing env combinations and the
`CC` variants together so all intended builds run. Use the task block with the
`name: "x86_64: Linux (Debian stable)"` as the place to fix this.
- Around line 282-320: The task definition in the Cirrus config has multiple
`matrix` keys under the same `task`, so only the last one is applied and the
Valgrind/UBSan-ASan-LSan and ASM/ECMULT variants are lost. Merge the `matrix`
entries into a single combined `matrix` structure for this task, keeping the
existing axes from the sanitizer job and the `CC`/`HOST` variants together so
all intended job combinations are generated.
- Around line 120-138: The Cirrus configuration has two separate env mappings,
and the second one in the main task setup overwrites the Homebrew and MAKEFLAGS
settings instead of extending them. Merge the environment entries into a single
env block so the macOS task keeps HOMEBREW_NO_AUTO_UPDATE,
HOMEBREW_NO_INSTALL_CLEANUP, and MAKEFLAGS while still setting ASM,
WITH_VALGRIND, CTIMETESTS, and CC before the matrix runs.
🪄 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: e901107f-afa6-422a-b41b-3eb8033f4ae4
📒 Files selected for processing (47)
configure.acdoc/release-notes-29189.mddoc/shared-libraries.mdsrc/bitcoin-cli.cppsrc/chainparamsbase.hsrc/node/interface_ui.hsrc/policy/feerate.hsrc/secp256k1/.cirrus.ymlsrc/secp256k1/CHANGELOG.mdsrc/secp256k1/Makefile.amsrc/secp256k1/ci/cirrus.shsrc/secp256k1/ci/linux-debian.Dockerfilesrc/secp256k1/configure.acsrc/secp256k1/doc/ellswift.mdsrc/secp256k1/examples/CMakeLists.txtsrc/secp256k1/examples/examples_util.hsrc/secp256k1/include/secp256k1.hsrc/secp256k1/include/secp256k1_ecdh.hsrc/secp256k1/include/secp256k1_ellswift.hsrc/secp256k1/include/secp256k1_extrakeys.hsrc/secp256k1/include/secp256k1_schnorrsig.hsrc/secp256k1/sage/group_prover.sagesrc/secp256k1/src/CMakeLists.txtsrc/secp256k1/src/ecdsa_impl.hsrc/secp256k1/src/ecmult.hsrc/secp256k1/src/ecmult_const_impl.hsrc/secp256k1/src/ecmult_gen_compute_table_impl.hsrc/secp256k1/src/ecmult_impl.hsrc/secp256k1/src/field.hsrc/secp256k1/src/field_10x26_impl.hsrc/secp256k1/src/field_5x52_impl.hsrc/secp256k1/src/field_impl.hsrc/secp256k1/src/group.hsrc/secp256k1/src/group_impl.hsrc/secp256k1/src/int128_struct_impl.hsrc/secp256k1/src/modules/ellswift/Makefile.am.includesrc/secp256k1/src/modules/ellswift/tests_exhaustive_impl.hsrc/secp256k1/src/modules/ellswift/tests_impl.hsrc/secp256k1/src/precompute_ecmult.csrc/secp256k1/src/testrand_impl.hsrc/secp256k1/src/tests.csrc/secp256k1/src/tests_exhaustive.csrc/secp256k1/src/util.hsrc/test/fuzz/utxo_total_supply.cppsrc/test/validation_chainstate_tests.cppsrc/validation.cpptest/functional/feature_init.py
💤 Files with no reviewable changes (2)
- src/secp256k1/examples/CMakeLists.txt
- src/secp256k1/configure.ac
| # Build and install gcc snapshot | ||
| ARG GCC_SNAPSHOT_MAJOR=14 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
git ls-files src/secp256k1Repository: dashpay/dash
Length of output: 5771
🏁 Script executed:
head -n 50 src/secp256k1/ci/linux-debian.DockerfileRepository: dashpay/dash
Length of output: 2606
🏁 Script executed:
find . -name "*.md" -o -name "CONTRIBUTING*" -o -name "README*" | head -20Repository: dashpay/dash
Length of output: 685
Update GCC_SNAPSHOT_MAJOR to 15 to align with GCC 15 compatibility goals.
The ARG GCC_SNAPSHOT_MAJOR defaults to 14 in src/secp256k1/ci/linux-debian.Dockerfile, causing the built gcc-snapshot to use GCC 14. This contradicts the PR objective of validating GCC 15 compatibility fixes.
- ARG GCC_SNAPSHOT_MAJOR=14
+ ARG GCC_SNAPSHOT_MAJOR=15Updating this ensures the CI environment actually tests against the intended compiler version.
🤖 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/secp256k1/ci/linux-debian.Dockerfile` around lines 29 - 30, The
Dockerfile is still defaulting the GCC snapshot to version 14, so the CI image
won’t validate GCC 15 compatibility. Update the `GCC_SNAPSHOT_MAJOR` argument in
`linux-debian.Dockerfile` to 15 so the `gcc-snapshot` build uses the intended
compiler version. Keep the change confined to the Docker build argument used for
the GCC snapshot setup.
There was a problem hiding this comment.
Code Review
Backport batch review found a merge-resolution defect in the bitcoin#29657 cherry-pick: Dash kept the inverted == 0 predicate, so normal v2 peers render blank while malformed values can still index out of bounds. The deterministic backport-prereq policy gate also restored the bitcoin#27780 prerequisite gap as blocking for the bitcoin#28789 fuzz timeout reduction. One release-note wording nit remains.
🔴 2 blocking | 💬 1 nitpick(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/bitcoin-cli.cpp`:
- [BLOCKING] src/bitcoin-cli.cpp:556: bitcoin#29657 backport inverts the netinfo transport-version check
The cherry-pick of upstream c3e632b441 brings in the new `(size() == 2 && [0] == 'v')` predicate but keeps the trailing `== 0` from Dash's pre-existing `starts_with('v') == 0` (introduced by 1269ac2e1d, which mechanically substituted `starts_with` for `rfind('v', 0)` without realizing the return type changed from `size_type` to `bool`). With `== 0` left in place, the new condition is the negation of upstream's:
- `"v2"` (the normal case) → `(true && true) == 0` is false → ternary returns `' '`, so the version column is blank for ordinary v2 transport peers.
- `"v"` → `(false && _) == 0` is true → returns `peer.transport_protocol_type[1]`, the exact out-of-bounds dereference upstream #29657 was fixing.
- `"v10"` → returns `[1] == '1'`, the other case the upstream fix targets.
- `""` → reads `[0]` on an empty string (UB).
Dropping the `== 0` restores upstream semantics and simultaneously repairs the carried-forward Dash inversion. Display-only, no consensus impact, but the cherry-pick as committed does the opposite of what it advertises.
In `src/test/fuzz/utxo_total_supply.cpp`:
- [BLOCKING] src/test/fuzz/utxo_total_supply.cpp:125: Missing prerequisite: bitcoin#27780
bitcoin#28789 is the second timeout reduction and upstream applies it on top of bitcoin#27780. Upstream's pre-PR state already had the bitcoin#27780 cap and comments: `LIMITED_WHILE(fuzzed_data_provider.remaining_bytes(), 2'000)`, which bitcoin#28789 reduces to `2'00`. Dash's preimage still had `LIMITED_WHILE(..., 100'000)`, so this PR changed it to `100'00` instead of upstream's `2'00`. That is a workaround for the missing prerequisite, not a clean adaptation, and leaves the timeout guard 50x looser than the upstream backport target.
---
**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.
| ConnectionTypeForNetinfo(peer.conn_type), | ||
| peer.network, | ||
| peer.transport_protocol_type.starts_with('v') == 0 ? peer.transport_protocol_type[1] : ' ', | ||
| (peer.transport_protocol_type.size() == 2 && peer.transport_protocol_type[0] == 'v') == 0 ? peer.transport_protocol_type[1] : ' ', |
There was a problem hiding this comment.
🔴 Blocking: bitcoin#29657 backport inverts the netinfo transport-version check
The cherry-pick of upstream c3e632b brings in the new (size() == 2 && [0] == 'v') predicate but keeps the trailing == 0 from Dash's pre-existing starts_with('v') == 0 (introduced by 1269ac2, which mechanically substituted starts_with for rfind('v', 0) without realizing the return type changed from size_type to bool). With == 0 left in place, the new condition is the negation of upstream's:
"v2"(the normal case) →(true && true) == 0is false → ternary returns' ', so the version column is blank for ordinary v2 transport peers."v"→(false && _) == 0is true → returnspeer.transport_protocol_type[1], the exact out-of-bounds dereference upstream Bugfix: bitcoin-cli: Check length of peer.transport_protocol_type bitcoin/bitcoin#29657 was fixing."v10"→ returns[1] == '1', the other case the upstream fix targets.""→ reads[0]on an empty string (UB).
Dropping the == 0 restores upstream semantics and simultaneously repairs the carried-forward Dash inversion. Display-only, no consensus impact, but the cherry-pick as committed does the opposite of what it advertises.
| (peer.transport_protocol_type.size() == 2 && peer.transport_protocol_type[0] == 'v') == 0 ? peer.transport_protocol_type[1] : ' ', | |
| (peer.transport_protocol_type.size() == 2 && peer.transport_protocol_type[0] == 'v') ? peer.transport_protocol_type[1] : ' ', |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — bitcoin#29657 backport inverts the netinfo transport-version check 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.
| libbitcoinconsensus | ||
| ======================== | ||
|
|
||
| This library is deprecated and will be removed for v28. | ||
|
|
||
| It has existed for nearly 10 years with very little known uptake or impact. It | ||
| has become a maintenance burden. | ||
|
|
||
| The underlying functionality does not change between versions, so any users of | ||
| the library can continue to use the final release indefinitely, with the | ||
| understanding that Taproot is its final consensus update. | ||
|
|
||
| In the future, libbitcoinkernel will provide a much more useful API that is | ||
| aware of the UTXO set, and therefore be able to fully validate transactions and | ||
| blocks. |
There was a problem hiding this comment.
💬 Nitpick: Adapt libconsensus release note wording for Dash
This new release-note file still refers to libbitcoinconsensus, libbitcoinkernel, and Taproot as the final consensus update. The companion doc/shared-libraries.md hunk in the same PR correctly says dashconsensus. For consistency with the rest of the Dash tree (which exposes dashconsensus, not libbitcoinconsensus, and where Taproot is not a Dash consensus milestone), this note should be reworded to match. Dropping the Taproot sentence avoids documenting a Bitcoin-specific milestone in Dash release notes.
| libbitcoinconsensus | |
| ======================== | |
| This library is deprecated and will be removed for v28. | |
| It has existed for nearly 10 years with very little known uptake or impact. It | |
| has become a maintenance burden. | |
| The underlying functionality does not change between versions, so any users of | |
| the library can continue to use the final release indefinitely, with the | |
| understanding that Taproot is its final consensus update. | |
| In the future, libbitcoinkernel will provide a much more useful API that is | |
| aware of the UTXO set, and therefore be able to fully validate transactions and | |
| blocks. | |
| dashconsensus | |
| ============= | |
| This library is deprecated and will be removed in v28. | |
| It has existed for nearly 10 years with very little known uptake or impact. It | |
| has become a maintenance burden. | |
| The underlying functionality does not change between versions, so any users of | |
| the library can continue to use the final release indefinitely, with the | |
| understanding that it will not receive future consensus updates. | |
| In the future, a kernel library will provide a much more useful API that is | |
| aware of the UTXO set, and therefore be able to fully validate transactions and | |
| blocks. |
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Adapt libconsensus release note wording for Dash 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.
| StoreLastTxo(); | ||
|
|
||
| LIMITED_WHILE(fuzzed_data_provider.remaining_bytes(), 100'000) | ||
| LIMITED_WHILE(fuzzed_data_provider.remaining_bytes(), 100'00) |
There was a problem hiding this comment.
🔴 Blocking: Missing prerequisite: bitcoin#27780
bitcoin#28789 is the second timeout reduction and upstream applies it on top of bitcoin#27780. Upstream's pre-PR state already had the bitcoin#27780 cap and comments: LIMITED_WHILE(fuzzed_data_provider.remaining_bytes(), 2'000), which bitcoin#28789 reduces to 2'00. Dash's preimage still had LIMITED_WHILE(..., 100'000), so this PR changed it to 100'00 instead of upstream's 2'00. That is a workaround for the missing prerequisite, not a clean adaptation, and leaves the timeout guard 50x looser than the upstream backport target.
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 prerequisite: bitcoin#27780 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.
…ort_protocol_type c3e632b Bugfix: bitcoin-cli: Check length of peer.transport_protocol_type (Luke Dashjr) Pull request description: "v" would dereference beyond the string length, and "v10" would show as '1' Turn both of these cases into a blank, like anything else unexpected currently is. ACKs for top commit: sipa: utACK c3e632b. hernanmarino: utACK c3e632b alfonsoromanz: ACK c3e632b achow101: ACK c3e632b Tree-SHA512: f641e4412521adae7c8c8e1f268bdaaa223d9048d8286e3df4b13905faaa0d601155ce581cd649f760cab2acc4122356fa94a44714f1f190845552100105eda0
25dc87e libconsensus: deprecate (Cory Fields) Pull request description: This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib). Several discussions have arisen wrt migrating it to CMake and it has become difficult to justify adding more complexity for a library that is virtually unused anyway. See for example the discussions: hebasto#41 bitcoin#29123 And here: bitcoin#29180 Where it is pointed out that the libbitcoinconsensus functions are slower than those the internal bitcoind equivalents due to the missing sha2 implementations. Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not migrating it to CMake and letting it end with v27. Any remaining use-cases could be handled in the future by libbitcoinkernel. If there are any users currently using libbitcoinconsensus, please chime in with your use-case! Edit: Corrected final release to be v27. ACKs for top commit: TheCharlatan: ACK 25dc87e fanquake: ACK 25dc87e - this library has very little, if any impactful real world usage. It has been entirely broken (on various platforms) for long periods of its existence, where nobody even noticed. Pruning this out to save porting, and starting anew with the kernel, is the sane thing to do. Tree-SHA512: baff2b3c4f76f520c96021035f751fdcb51bedf00e767660249e92a7bc7c5c176786bcf2c4cfe2d2351c200f932b39eb886bcfb22fbec824a41617590d6a1638
055bc05 policy/feerate.h: avoid constraint self-dependency (Matt Whitlock) 138f867 add missing #include <cstdint> for GCC 15 (Matt Whitlock) Pull request description: bitcoin#30612 with changes made. GCC 15 introduces three build failures: * Two are related to missing includes. You can't use `uint16_t` et al. without including `<cstdint>`. * The third is harder to understand but easy to fix. GCC changed something about the way templates are instantiated when checking type constraints, and now there is a dependency loop while checking `std::optional<CFeeRate>`. This manifests as the following compile-time mess: ``` In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/format:48, from /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/bits/chrono_io.h:39, from /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/chrono:3362, from ./util/time.h:9, from ./primitives/block.h:12, from ./blockencodings.h:8, from blockencodings.cpp:5: /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits: In substitution of 'template<class _Up> requires !(is_same_v<std::optional<_Tp>, typename std::remove_cvref<_It2>::type>) && (is_constructible_v<_Tp, const _Up&>) && (__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type>) constexpr std::optional<CFeeRate>::optional(const std::optional<_Tp>&) [with _Up = CFeeRate]': /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:1140:25: required by substitution of 'template<class _Tp, class ... _Args> using std::__is_constructible_impl = std::__bool_constant<__is_constructible(_Tp, _Args ...)> [with _Tp = CFeeRate; _Args = {std::optional<CFeeRate>&}]' 1140 | = __bool_constant<__is_constructible(_Tp, _Args...)>; | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:1145:12: required from 'struct std::is_constructible<CFeeRate, std::optional<CFeeRate>&>' 1145 | struct is_constructible | ^~~~~~~~~~~~~~~~ /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:178:35: required by substitution of 'template<class ... _Bn> std::__detail::__first_t<std::integral_constant<bool, false>, typename std::enable_if<(!(bool)(_Bn::value)), void>::type ...> std::__detail::__or_fn(int) [with _Bn = {std::is_constructible<CFeeRate, std::optional<CFeeRate>&>, std::is_convertible<std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, std::optional<CFeeRate> >, std::is_convertible<std::optional<CFeeRate>, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate>&>, std::is_convertible<const std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate> >, std::is_convertible<const std::optional<CFeeRate>, CFeeRate>}]' 178 | __enable_if_t<!bool(_Bn::value)>...>; | ^~~~~ /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:196:41: required from 'struct std::__or_<std::is_constructible<CFeeRate, std::optional<CFeeRate>&>, std::is_convertible<std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, std::optional<CFeeRate> >, std::is_convertible<std::optional<CFeeRate>, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate>&>, std::is_convertible<const std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate> >, std::is_convertible<const std::optional<CFeeRate>, CFeeRate> >' 196 | : decltype(__detail::__or_fn<_Bn...>(0)) | ~~~~~~~~~~~~~~~~~~~~~~~~~^~~ /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:824:45: required from 'constexpr const bool std::optional<CFeeRate>::__construct_from_contained_value<CFeeRate, CFeeRate>' 824 | = !__converts_from_optional<_Tp, _From>::value; | ^~~~~ /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:884:7: required by substitution of 'template<class _Up> requires !(is_same_v<std::optional<_Tp>, typename std::remove_cvref<_It2>::type>) && (is_constructible_v<_Tp, const _Up&>) && (__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type>) constexpr std::optional<CFeeRate>::optional(const std::optional<_Tp>&) [with _Up = CFeeRate]' 884 | && __construct_from_contained_value<_Up> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./validation.h:164:41: required from here 164 | return MempoolAcceptResult(state); | ^ /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:886:2: required by the constraints of 'template<class _Tp> template<class _Up> requires !(is_same_v<std::optional<_Tp>, typename std::remove_cvref<_It2>::type>) && (is_constructible_v<_Tp, const _Up&>) && (__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type>) constexpr std::optional<_Tp>::optional(const std::optional<_From>&)' /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:884:14: error: satisfaction of atomic constraint '__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type> [with _Tp = _Tp; _Up = _Up]' depends on itself 884 | && __construct_from_contained_value<_Up> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``` It is easiest to solve this by changing the `static_assert` in the explicit `CFeeRate` constructor to a SFINAE by using a type constraint on the function template parameter. We already [downstreamed](gentoo/gentoo#38015) these fixes in Gentoo. ACKs for top commit: stickies-v: ACK 055bc05 Tree-SHA512: ce9cb27bcd9b0f4bbc80951e45cf7127112dcb7f9937bcb0167b362026d35beecb1255354746de0aac82e03c41eaccbe26acbfe0ddff2ee1e5a8634673f4f4ba
…ndex e639364 validation: add missing insert to m_dirty_blockindex (Martin Zumsande) Pull request description: When the status of a block index is changed, we must add it to `m_dirty_blockindex` or the change might not get persisted to disk. This is missing from one spot in `FindMostWorkChain()`, where `BLOCK_FAILED_CHILD` is set. Since we have [code](https://github.com/bitcoin/bitcoin/blob/f0758d8a6696657269d9c057e7aa079ffa9e1c16/src/node/blockstorage.cpp#L284-L287) that later sets missing `BLOCK_FAILED_CHILD` during the next startup, I don't think that this can lead to bad block indexes in practice, but I still think it's worth fixing. ACKs for top commit: TheCharlatan: ACK e639364 stickies-v: ACK e639364 Tree-SHA512: a97af9c173e31b90b677a1f95de822e08078d78013de5fa5fe4c3bec06f45d6e1823b7694cdacb887d031329e4b4afc6a2003916e0ae131279dee71f43e1f478
5080c9c build: adapt Windows builds for libsecp256k1 build changes (fanquake) ff061fd Squashed 'src/secp256k1/' changes from 705ce7e..c545fdc (fanquake) Pull request description: Includes bitcoin-core/secp256k1#1378. Which fixes bitcoin#28079. Adapts Windows build for bitcoin-core/secp256k1#1367. ACKs for top commit: hebasto: ACK 5080c9c, I've made the `src/secp256k1` subtree update locally and got zero diff with this PR branch. jonasnick: ACK 5080c9c Tree-SHA512: 37915d420ebacefc6bc82c2511bff3d6884e01d5c92795f19cd61862f96b30aa1fe768aeabec128c9d25c1d8bc62b46b46969626067266074b39566ad9e2f5ba
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
fafb4da fuzz: Avoid timeout in utxo_total_supply (MarcoFalke) Pull request description: Looks like for high block counts it may be better to mock the chain, otherwise a high limit will lead to fuzz input bloat and timeouts, see bitcoin#17860 (comment). It can be checked that the fuzz target can still find the CVE, see bitcoin#17860 (review) with a diff of: ```diff diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index f949655..6f4cfb5f51 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -39,8 +39,6 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state) // the underlying coins database. std::set<COutPoint> vInOutPoints; for (const auto& txin : tx.vin) { - if (!vInOutPoints.insert(txin.prevout).second) - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); } if (tx.IsCoinBase()) ``` Also, fix a nit, see bitcoin#17860 (comment) ACKs for top commit: dergoegge: ACK fafb4da Tree-SHA512: a28fe9cd6ebb4c9bed5a5b35be76c1c436a87586c8fc3b3c4c8559a4a77ac08098324370da421d794c99579882c0872b6b29415de47ade6a05a08504a3d494c4
fa7ba92 fuzz: Avoid utxo_total_supply timeout (MarcoFalke) Pull request description: Looks like this still may take a long time to run large fuzz inputs. Thus, reduce it further, but still allow it to catch the regression, if re-introduced: ```diff diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index f949655..4bdd15c5ee 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -40,7 +40,7 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state) std::set<COutPoint> vInOutPoints; for (const auto& txin : tx.vin) { if (!vInOutPoints.insert(txin.prevout).second) - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); + {}//return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); } if (tx.IsCoinBase()) ``` This is the second take, see bitcoin#27780. If in the future it still times out, I think the fuzz test can just be removed. Example input: ``` JREROy5pcnAgQyw7IC4ODg4ODg4ODg4O0dEODg4ODg4ZDg4ODg4ODg4ODg7RDg4ODg4ODg4O0dEODg4ODg4ODg4ODg7R0Q4ODg4ODg4ODtHRDg4ODtHR0dEODg4O0dEODg7R0Q4ODg4ODg4ODtHRDg4ODg4ODg4ODg4O0dEODg4ODg4ODg7R0Q4ODg7R0Q4O0dEODg4ODg4ODg4ODg7R0Q4ODg4ODtHRDg4ODtHR ACKs for top commit: dergoegge: ACK fa7ba92 brunoerg: utACK fa7ba92 Tree-SHA512: 154a4895834babede6ce7b775562a7026637af1097e53e55676e92f6cf966ae0c092300ebf7e51a397eebd11f7b41d020586663e781f70d084efda1c0fe851b4
vijaydasmp
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/secp256k1/.cirrus.yml (1)
65-404: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAvoid editing the vendored
src/secp256k1subtree directly.This PR changes files under
src/secp256k1/**, but the repo guidance marks that subtree as vendored and no-touch. If this is an intentional vendor sync, please route it through the project’s vendor-update process instead of landing piecemeal edits here. As per coding guidelines, "Do not make changes to vendored dependencies: src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue} or src/crypto/{ctaes,x11}".🤖 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/secp256k1/.cirrus.yml` around lines 65 - 404, The change is in the vendored secp256k1 CI config, which should not be edited directly. Revert the manual edits under src/secp256k1 and, if this update is intentional, apply it via the project’s vendor-update flow instead of patching .cirrus.yml in place. Use the vendored subtree boundary around the .cirrus.yml configuration to locate and remove these changes.Source: Coding guidelines
🤖 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 `@src/secp256k1/.cirrus.yml`:
- Around line 125-129: The macOS task’s env settings are split across multiple
env mappings, and the later one in .cirrus.yml overrides the earlier block so
HOMEBREW_NO_AUTO_UPDATE, HOMEBREW_NO_INSTALL_CLEANUP, and MAKEFLAGS get dropped.
Merge all of the task’s env entries into a single env block for that macOS task,
keeping ASM, WITH_VALGRIND, CTIMETESTS, CC, and the Homebrew/MAKEFLAGS settings
together.
- Around line 83-94: The Cirrus Linux job currently defines two separate matrix
blocks, so the later one overwrites the earlier CFLAGS/WIDEMUL entries and those
combinations never run. Update the .cirrus.yml job definition to use a single
matrix under the Linux config and include both the existing feature/build env
variants and the compiler variants in that same matrix, preserving the intended
entries from the job’s matrix list.
---
Outside diff comments:
In `@src/secp256k1/.cirrus.yml`:
- Around line 65-404: The change is in the vendored secp256k1 CI config, which
should not be edited directly. Revert the manual edits under src/secp256k1 and,
if this update is intentional, apply it via the project’s vendor-update flow
instead of patching .cirrus.yml in place. Use the vendored subtree boundary
around the .cirrus.yml configuration to locate and remove these changes.
🪄 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: 6d4a6a66-0b0e-4336-b9a4-2925d5149993
📒 Files selected for processing (46)
configure.acdoc/release-notes-29189.mddoc/shared-libraries.mdsrc/bitcoin-cli.cppsrc/chainparamsbase.hsrc/node/interface_ui.hsrc/policy/feerate.hsrc/secp256k1/.cirrus.ymlsrc/secp256k1/CHANGELOG.mdsrc/secp256k1/Makefile.amsrc/secp256k1/ci/cirrus.shsrc/secp256k1/ci/linux-debian.Dockerfilesrc/secp256k1/configure.acsrc/secp256k1/doc/ellswift.mdsrc/secp256k1/examples/CMakeLists.txtsrc/secp256k1/examples/examples_util.hsrc/secp256k1/include/secp256k1.hsrc/secp256k1/include/secp256k1_ecdh.hsrc/secp256k1/include/secp256k1_ellswift.hsrc/secp256k1/include/secp256k1_extrakeys.hsrc/secp256k1/include/secp256k1_schnorrsig.hsrc/secp256k1/sage/group_prover.sagesrc/secp256k1/src/CMakeLists.txtsrc/secp256k1/src/ecdsa_impl.hsrc/secp256k1/src/ecmult.hsrc/secp256k1/src/ecmult_const_impl.hsrc/secp256k1/src/ecmult_gen_compute_table_impl.hsrc/secp256k1/src/ecmult_impl.hsrc/secp256k1/src/field.hsrc/secp256k1/src/field_10x26_impl.hsrc/secp256k1/src/field_5x52_impl.hsrc/secp256k1/src/field_impl.hsrc/secp256k1/src/group.hsrc/secp256k1/src/group_impl.hsrc/secp256k1/src/int128_struct_impl.hsrc/secp256k1/src/modules/ellswift/Makefile.am.includesrc/secp256k1/src/modules/ellswift/tests_exhaustive_impl.hsrc/secp256k1/src/modules/ellswift/tests_impl.hsrc/secp256k1/src/precompute_ecmult.csrc/secp256k1/src/testrand_impl.hsrc/secp256k1/src/tests.csrc/secp256k1/src/tests_exhaustive.csrc/secp256k1/src/util.hsrc/test/fuzz/utxo_total_supply.cppsrc/test/validation_chainstate_tests.cppsrc/validation.cpp
💤 Files with no reviewable changes (2)
- src/secp256k1/examples/CMakeLists.txt
- src/secp256k1/configure.ac
✅ Files skipped from review due to trivial changes (14)
- src/secp256k1/src/ecmult.h
- src/secp256k1/src/ecmult_const_impl.h
- src/secp256k1/src/ecmult_impl.h
- src/secp256k1/examples/examples_util.h
- src/node/interface_ui.h
- src/secp256k1/sage/group_prover.sage
- src/secp256k1/include/secp256k1_extrakeys.h
- src/secp256k1/src/group.h
- src/secp256k1/src/ecdsa_impl.h
- src/chainparamsbase.h
- doc/shared-libraries.md
- doc/release-notes-29189.md
- src/secp256k1/src/precompute_ecmult.c
- src/secp256k1/src/ecmult_gen_compute_table_impl.h
🚧 Files skipped from review as they are similar to previous changes (27)
- src/secp256k1/src/modules/ellswift/Makefile.am.include
- src/secp256k1/Makefile.am
- src/bitcoin-cli.cpp
- src/secp256k1/src/util.h
- src/secp256k1/include/secp256k1_ecdh.h
- configure.ac
- src/test/validation_chainstate_tests.cpp
- src/policy/feerate.h
- src/secp256k1/src/modules/ellswift/tests_exhaustive_impl.h
- src/secp256k1/src/CMakeLists.txt
- src/validation.cpp
- src/secp256k1/src/modules/ellswift/tests_impl.h
- src/secp256k1/include/secp256k1_ellswift.h
- src/secp256k1/include/secp256k1_schnorrsig.h
- src/secp256k1/src/field_10x26_impl.h
- src/secp256k1/ci/cirrus.sh
- src/secp256k1/src/int128_struct_impl.h
- src/secp256k1/src/field_5x52_impl.h
- src/test/fuzz/utxo_total_supply.cpp
- src/secp256k1/src/group_impl.h
- src/secp256k1/src/tests_exhaustive.c
- src/secp256k1/include/secp256k1.h
- src/secp256k1/doc/ellswift.md
- src/secp256k1/src/testrand_impl.h
- src/secp256k1/ci/linux-debian.Dockerfile
- src/secp256k1/src/field_impl.h
- src/secp256k1/src/field.h
| - env: {CFLAGS: -O1, RECOVERY: yes, ECDH: yes, SCHNORRSIG: yes, ELLSWIFT: yes} | ||
| - env: { ECMULTGENPRECISION: 2, ECMULTWINDOW: 2 } | ||
| - env: { ECMULTGENPRECISION: 8, ECMULTWINDOW: 4 } | ||
| matrix: | ||
| - env: | ||
| CC: gcc | ||
| - env: | ||
| CC: clang | ||
| << : *MERGE_BASE | ||
| - env: | ||
| CC: gcc-snapshot | ||
| - env: | ||
| CC: clang-snapshot |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Merge the Linux matrix entries under a single matrix: key. The second matrix: replaces the first, so the CFLAGS/WIDEMUL combinations stop running and only the compiler variants remain.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 86-86: duplication of key "matrix" in mapping
(key-duplicates)
🤖 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/secp256k1/.cirrus.yml` around lines 83 - 94, The Cirrus Linux job
currently defines two separate matrix blocks, so the later one overwrites the
earlier CFLAGS/WIDEMUL entries and those combinations never run. Update the
.cirrus.yml job definition to use a single matrix under the Linux config and
include both the existing feature/build env variants and the compiler variants
in that same matrix, preserving the intended entries from the job’s matrix list.
| env: | ||
| ASM: no | ||
| WITH_VALGRIND: no | ||
| CTIMETESTS: no | ||
| CC: clang |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/secp256k1/.cirrus.yml"
# Show the relevant range with line numbers.
sed -n '105,140p' "$file" | cat -n
# Also inspect the full task block around the macOS job for structure.
python3 - <<'PY'
from pathlib import Path
p = Path("src/secp256k1/.cirrus.yml")
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 110 <= i <= 135:
print(f"{i:4}: {line}")
PYRepository: dashpay/dash
Length of output: 2870
🌐 Web query:
Cirrus CI YAML duplicate mapping keys env block same task behavior last key wins merge environment variables
💡 Result:
In Cirrus CI, the behavior of environment variables defined within an env block follows standard YAML merging rules and Cirrus CI-specific hierarchy rules [1]. 1. Duplicate Keys in YAML: When you define duplicate keys within the same YAML env block, standard YAML parsers typically follow the "last key wins" behavior. The last specified value for a given key will overwrite any preceding values for that same key within that specific block. 2. Merging Hierarchy: Cirrus CI merges environment variables defined at different levels of the configuration [1]. Specifically, variables defined at the root level of.cirrus.yml are merged with those defined at the task level [1]. If a key exists in both the root env block and the task-level env block, the task-level variable takes precedence and overrides the root-level value [1]. 3. Programming Tasks (Starlark): If you are using Starlark (.cirrus.star) to define your CI configuration, you are essentially constructing a dictionary or a list of tasks [2][3]. When returning these structures, the final environment is typically computed by merging dictionaries [2]. In these programmatic contexts, standard programming language rules for dictionary updates apply (i.e., subsequent updates overwrite existing keys). If you are encountering issues with environment variables not persisting as expected between script instructions, note that Cirrus CI does not automatically persist changes made to environment variables (like export VAR=value) between separate script steps, as each instruction may run in a isolated or independent context depending on the executor design [4]. To pass values between steps, you must typically write them to a file or use the provided CI mechanisms [1][4].
Citations:
- 1: https://cirrus-ci.org/guide/writing-tasks/
- 2: https://cirrus-ci.org/guide/programming-tasks/
- 3: https://github.com/postgresql-cfbot/postgresql/blob/4c0f1e4910184a7427ab133f1b233aa0ab5c26ec/.cirrus.star
- 4: Persist new environment variables between script instructions cirruslabs/cirrus-ci-docs#187
Merge the macOS task’s env: settings into one block. The second env: mapping replaces the first, so HOMEBREW_NO_AUTO_UPDATE, HOMEBREW_NO_INSTALL_CLEANUP, and MAKEFLAGS are dropped from this task.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 125-125: duplication of key "env" in mapping
(key-duplicates)
🤖 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/secp256k1/.cirrus.yml` around lines 125 - 129, The macOS task’s env
settings are split across multiple env mappings, and the later one in
.cirrus.yml overrides the earlier block so HOMEBREW_NO_AUTO_UPDATE,
HOMEBREW_NO_INSTALL_CLEANUP, and MAKEFLAGS get dropped. Merge all of the task’s
env entries into a single env block for that macOS task, keeping ASM,
WITH_VALGRIND, CTIMETESTS, CC, and the Homebrew/MAKEFLAGS settings together.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bf7f8dd5a
ℹ️ 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".
| current_block->hashMerkleRoot = BlockMerkleRoot(*current_block); | ||
| const bool was_valid = !MineBlock(node, current_block).IsNull(); | ||
|
|
||
| if (duplicate_coinbase_height == ActiveHeight()) { |
There was a problem hiding this comment.
This check now runs even when MineBlock rejects the candidate. For example, when duplicate_coinbase_height is 1 the loop starts at active height 1; if the fuzzer first creates an invalid height-2 block (such as by spending an immature coinbase from the append-only txos), MineBlock returns null and leaves ActiveHeight() at 1, so this branch fires and compares the height-2 coinbase script against the height-1 duplicate script. That makes the fuzz target abort on a valid rejection instead of only checking a block that was actually connected.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta resolves all six prior findings: bitcoin#29657's netinfo predicate at src/bitcoin-cli.cpp:556 no longer has the inverted == 0, bitcoin#27780 is now backported as commit 06ef315 ahead of bitcoin#28789 with the fuzz cap correctly at 2'00 matching upstream, and doc/release-notes-29189.md is Dash-adapted (dashconsensus, no Taproot, generic kernel library). Both general/backport agents independently agree no new in-scope defects exist in the latest delta or cumulative state. Clean backport stack.
5 participants
bitcoin backports July batch