backport: trivial 2025 04 23 #6649
Conversation
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
force-pushed
the
develop-trivial-2025-04-22-p2
branch
from
e95c02d to
2b8fce6
Compare
WalkthroughThis set of changes introduces updates across CI configuration, build scripts, documentation, and application logic. The CI linting workflow is refactored to use a Docker-based environment with new scripts and a Dockerfile, and the commit range logic for linting is revised for better local and CI consistency. The ARM architecture detection in ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (7)
test/lint/README.md (1)
3-15: Rename Docker image reference to match Dash repository
The instructions reference an image namedbitcoin-linter, which may confuse contributors working in the Dash repo. Consider renaming it todash-linter(ordashpay-linter) in both the Dockerfile and README to align with the project’s branding.ci/lint/06_script.sh (1)
9-15: Add support for non-masterdefault branches
The merge-base logic hardcodesmaster. If upstream renames the primary branch (e.g., tomain), local lint runs will fail. Consider making the base branch configurable via an environment variable or dynamically detecting it (e.g.,git symbolic-ref refs/remotes/origin/HEAD)..cirrus.yml (1)
26-27: Use shallow fetch to speed up CI checkout
Fetching the entire merge commit history can be slow. You could add--depth=1to:- git fetch $CIRRUS_REPO_CLONE_URL "pull/${CIRRUS_PR}/merge" + git fetch --depth=1 $CIRRUS_REPO_CLONE_URL "pull/${CIRRUS_PR}/merge"This will reduce bandwidth and time without affecting the single‑commit checkout.
ci/lint/04_install.sh (2)
16-34: Verify Python availability when skipping installation
The conditional correctly wraps the pyenv install underSKIP_PYTHON_INSTALL. However, in skip mode, there’s no check thatpython3is present. Add a guard after theifto error out ifpython3is missing, e.g.:if [ -n "${SKIP_PYTHON_INSTALL}" ] && ! command -v python3 &>/dev/null; then echo "ERROR: python3 not found; either install it or unset SKIP_PYTHON_INSTALL" exit 1 fi
43-46: Simplify ShellCheck extraction path
Movingshellcheckto/usr/binis fine, but you can streamline thetarstep with:- curl … | tar --xz -xf - --directory /tmp/ - mv "/tmp/shellcheck-${SHELLCHECK_VERSION}/shellcheck" /usr/bin/ + curl … | tar --strip-components=1 -C /usr/bin -xJf -This avoids leaving the extracted directory behind and removes the extra
mv.ci/lint/Dockerfile (1)
8-8: Consider a slimmer or more recent Python base imageYou currently use
python:3.7-buster, which is past its end-of-life and includes extra packages. You might switch topython:3.7-slim-buster(if Python 3.7 is still required) or bump to a newer supported version like 3.10/3.11. This will reduce image size and improve security.ci/lint/docker-entrypoint.sh (1)
8-12: Propagate signals by exec’ing the lint processWhen launching the lint script with
bash -ic, consider usingexecso the script replaces PID 1 and receives signals properly:if [ -z "$1" ]; then - LOCAL_BRANCH=1 bash -ic "./ci/lint/06_script.sh" + exec env LOCAL_BRANCH=1 bash -ic "./ci/lint/06_script.sh" else exec "$@" fi
🛑 Comments failed to post (1)
ci/lint/docker-entrypoint.sh (1)
1-3: 🛠️ Refactor suggestion
Enable strict Bash execution flags
Add
set -euo pipefailafter the shebang to fail fast on errors and undefined variables, improving script robustness:#!/usr/bin/env bash +set -euo pipefail export LC_ALL=C📝 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.#!/usr/bin/env bash set -euo pipefail export LC_ALL=C
WalkthroughThe changes introduce several updates across the continuous integration (CI), build configuration, wallet logic, GUI, and documentation components of the project. In the CI configuration, the merge base logic in Build configuration in ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
ci/lint/06_script.sh (1)
9-15: Improve commit-range logic for local linting
IntroducingLOCAL_BRANCHto mirror CI commit-range behavior is valuable. One caveat: the base branch is hardcoded asmaster. If the default branch is renamed (e.g., tomain), this will break range detection. Consider deriving the default branch dynamically or parameterizing it.ci/lint/04_install.sh (1)
16-34: Conditional Python installation behind SKIP_PYTHON_INSTALL
Wrapping thepyenvinstall logic inif [ -z "$SKIP_PYTHON_INSTALL" ]is a solid way to skip building Python in the Docker image. Please verify thatBASE_ROOT_DIRis set before this script runs (e.g., via the Dockerfile) sopython-build "$(cat "${BASE_ROOT_DIR}/.python-version")"resolves correctly. Alternatively, consider usinggit rev-parse --show-toplevelto locate the repo root dynamically.ci/lint/docker-entrypoint.sh (3)
1-3: Enable strict bash mode.To fail fast on errors and undefined variables, add strict mode flags right after the shebang:
#!/usr/bin/env bash +set -euo pipefail export LC_ALL=C
2-3: Align locale with Dockerfile.The Dockerfile sets
LC_ALL=C.UTF-8while this script exportsLC_ALL=C. For consistent UTF‑8 behavior, consider:-export LC_ALL=C +export LC_ALL=C.UTF-8
8-12: Improve argument detection and exec.
- Use
$#to detect zero arguments.- Use
execto replace the shell process (proper signal handling).-if [ -z "$1" ]; then - LOCAL_BRANCH=1 bash -ic "./ci/lint/06_script.sh" -else - exec "$@" -fi +if [ $# -eq 0 ]; then + LOCAL_BRANCH=1 exec bash -ic "./ci/lint/06_script.sh" +else + exec "$@" +fici/lint/Dockerfile (4)
8-12: Consider pinning or updating the base image.Relying on
python:3.7-busterwithout a digest can lead to non‑reproducible builds. You could:
- Pin to a specific digest.
- Upgrade to a newer patch version.
- Document the choice of this tag.
19-21: Verify COPY paths relative to context.Since the Dockerfile lives in
ci/lint/, confirm your build context is set such that:
ci/lint/docker-entrypoint.sh→/entrypoint.shci/lint/04_install.sh→/install.shOtherwise the build will fail.
22-25: Optimize apt installs to reduce image size.In your install script or here, use
--no-install-recommendsand remove lists in oneRUN:-RUN /install.sh && \ +RUN /install.sh && \ echo 'alias lint="./ci/lint/06_script.sh"' >> ~/.bashrc && \ chmod 755 /entrypoint.sh && \ - rm -rf /var/lib/apt/lists/* + rm -rf /var/lib/apt/lists/*
28-29: Clarify entrypoint vs CMD.Using only
ENTRYPOINTcan make overriding commands cumbersome. You might add aCMD ["bash"]or document the expected invocation to guide users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.cirrus.yml(1 hunks)ci/lint/04_install.sh(2 hunks)ci/lint/06_script.sh(1 hunks)ci/lint/Dockerfile(1 hunks)ci/lint/docker-entrypoint.sh(1 hunks)configure.ac(1 hunks)doc/release-notes-26899.md(1 hunks)src/init.cpp(2 hunks)src/qt/bitcoingui.cpp(1 hunks)src/wallet/wallet.cpp(1 hunks)test/functional/wallet_groups.py(1 hunks)test/lint/README.md(1 hunks)
🔇 Additional comments (13)
src/wallet/wallet.cpp (1)
3439-3440: Improved flexibility by allowing zero value for -mintxfee parameterThe code now only rejects the mintxfee parameter if parsing fails, allowing users to explicitly set a zero minimum transaction fee. Previously, zero values were rejected, which limited user configuration options.
src/qt/bitcoingui.cpp (1)
532-532: Improved signal-slot threading safety with explicit Qt::QueuedConnectionThis change explicitly sets a queued connection for the
openedsignal fromOpenWalletActivity, ensuring that thesetCurrentWalletslot runs in the GUI thread rather than the thread that emitted the signal. This is a good practice for thread safety in Qt applications, especially when dealing with UI updates following wallet operations.test/functional/wallet_groups.py (1)
30-31: Setting uniform transaction fee rate across test nodesGood addition! Setting a consistent transaction fee rate across all test nodes with
-paytxfeehelps ensure deterministic behavior in tests, particularly for wallet operations that consider fee calculations. The calculation20 * 1e3 / 1e8correctly represents 20 satoshis per vbyte in the BTC per kilobyte format expected by the parameter.src/init.cpp (3)
559-559: Documentation update for-dnsseedparameter is clear and accurate.The parameter description now correctly indicates that
-dnsseedis disabled by default when-maxconnections=0is set, in addition to when-connectis used.
563-563: Documentation update for-listenparameter is clear and accurate.The parameter description now correctly indicates that
-listenis disabled by default when-maxconnections=0is set, in addition to when-connectis used or when-proxyis specified.
912-918: Improved parameter interaction logic for disabling networking features.The condition for disabling DNS seeding and listening has been expanded to include when
-maxconnections=0is set, which is a logical extension of the existing behavior. The log messages have been appropriately updated to reflect this change. This implementation aligns with the documentation in the release notes.doc/release-notes-26899.md (1)
1-5: Well-documented behavior change in release notes.The release notes clearly explain the new behavior where setting
-maxconnections=0will disable both DNS seeding and listening by default, while still allowing users to explicitly override this behavior. This provides good communication to users about the change in settings behavior.configure.ac (2)
602-602: Split ARMv8 CRC compile flag
Decouples CRC support detection from SHA-NI by checking-march=armv8-a+crcindependently. This improves build flexibility on platforms with only CRC extensions. Please verify that CRC-dependent code is guarded viaAC_CONDITIONAL([ENABLE_ARM_CRC])or a corresponding macro inconfig.h—the current check setsenable_arm_crc, but noAC_DEFINE(ENABLE_ARM_CRC)is emitted.
603-603: Split ARMv8 crypto (SHA-NI) compile flag
Adds an independent check for-march=armv8-a+cryptoand correctly definesARM_SHANI_CXXFLAGSandENABLE_ARM_SHANIwhen available, aligning with the SHA-NI intrinsics check.test/lint/README.md (1)
3-15: Document local linting via Docker
The new "Running locally" section provides clear, step-by-step Docker instructions to match the CI environment. Consider adding a note for non-Linux users (e.g., using${PWD}substitution in PowerShell or adjusting volume syntax on Windows)..cirrus.yml (1)
26-28: Fetch and checkout pre-merged PR commit for CI
Switching from a manual merge to fetchingpull/${CIRRUS_PR}/mergeand checking outFETCH_HEADstreamlines conflict detection and ensures the lint CI runs on the exact merge result.ci/lint/04_install.sh (1)
43-45: Simplify ShellCheck installation
Extracting theshellcheckbinary and moving it directly into/usr/bin/removes the need to modifyPATHat runtime. Ensure/usr/bin/is writable in all build environments; if not,/usr/local/binis a more portable location.ci/lint/Dockerfile (1)
16-16:✅ Verification successful
Validate
SKIP_PYTHON_INSTALLin install script.Ensure this environment variable is actually used in
04_install.sh. If it’s unused, it can be removed to simplify the build.
🏁 Script executed:
grep -R "SKIP_PYTHON_INSTALL" ci/lint/04_install.shLength of output: 92
No action needed:
SKIP_PYTHON_INSTALLis used in the install script
Theci/lint/04_install.shscript contains:if [ -z "${SKIP_PYTHON_INSTALL}" ]; then # install Python… fiSo the environment variable is actively checked and should be kept.
| git config --global --add safe.directory /bitcoin | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use dynamic safe.directory.
Hardcoding /bitcoin binds you to that path. Making it dynamic improves portability:
-git config --global --add safe.directory /bitcoin
+git config --global --add safe.directory "$(pwd)"📝 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.
| git config --global --add safe.directory /bitcoin | |
| git config --global --add safe.directory "$(pwd)" |
| cd ./ci/lint | ||
| docker build -t bitcoin-linter . | ||
|
|
||
| cd /root/of/bitcoin/repo |
There was a problem hiding this comment.
should dashify this file I think
There was a problem hiding this comment.
nah; imo, keeping it non-dashified indicates this is not yet tested / adapted. It does need additional changes it appears before being usable.
test/functional/wallet_groups.py
Outdated
| ["-maxapsfee=0.00000294"], | ||
| ] | ||
| # whitelist peers to speed up tx relay / mempool sync | ||
|
|
There was a problem hiding this comment.
25990: CI fails for wallet_groups.py (all jobs that have wallet functional tests)
wallet_groups.py --legacy-wallet | ✖ Failed | 20 s
wallet_groups.py --descriptors | ✖ Failed | 14 s
0507d7f to
d4a5d53
Compare
…it merge fad7281 ci: Use remote pull/merge ref instead of local git merge (MacroFake) Pull request description: The merge strategy on the remote may be different than the local one. This may cause local merges to be different or fail completely. Fix this by using the result of the remote merge. Fixes bitcoin#26163 ACKs for top commit: hebasto: ACK fad7281, I regularly use the same commands locally. Tree-SHA512: 0febbf5db8c1536e31b374a7599a92037ca814174809075f42c7c7c4e1daaab5b3df09cf82f2de0d1e847c41eb30e770daaf7a85287f5d8d43ebd642d1234d3c
20adaea build: split ARM crc & crypto extension checks (fanquake) Pull request description: We currently perform the same check twice, to put the same set of flags in two different variables. Split the checks so we test for the `crc` and `crypto` extensions independently. If we don't want to split, we should just delete the second `AX_CHECK_COMPILE_FLAG` check, and set `ARM_CRC_CXXFLAGS` & `ARM_SHANI_CXXFLAGS` at the same time. Guix Build: ```bash 045392a6a4f538723b7759c67eeafd832735de7294b72b3a7f488d05a13711f7 guix-build-20adaeaef5fa/output/aarch64-linux-gnu/SHA256SUMS.part 054fda86577d757788a1c87508268402535fcbe869240309a2c91997234389cf guix-build-20adaeaef5fa/output/aarch64-linux-gnu/bitcoin-20adaeaef5fa-aarch64-linux-gnu-debug.tar.gz 92dc2513b2b6d87c0869ae18493fd9d0e2690b5b02bfd4310d54f4d394cfccdf guix-build-20adaeaef5fa/output/aarch64-linux-gnu/bitcoin-20adaeaef5fa-aarch64-linux-gnu.tar.gz 2515cfc708cc6ce0e650ca00c49de8dad856b54741ddc0c195845fc6ce2d67db guix-build-20adaeaef5fa/output/arm-linux-gnueabihf/SHA256SUMS.part fa0a956365e62b484f66dcf9763a02858db5c7e99317861819a54a15589ced80 guix-build-20adaeaef5fa/output/arm-linux-gnueabihf/bitcoin-20adaeaef5fa-arm-linux-gnueabihf-debug.tar.gz 1b3ddf2b1bbdc7632696ca78908e69b4fd156ccf7afa8078b5541d2ac10ab931 guix-build-20adaeaef5fa/output/arm-linux-gnueabihf/bitcoin-20adaeaef5fa-arm-linux-gnueabihf.tar.gz f87d8e23df60b208a631f6642f6c2cc0fc8e4e5e9563b36d1de9d371f22a69d9 guix-build-20adaeaef5fa/output/arm64-apple-darwin/SHA256SUMS.part c24ac07bfa935fd40358823d95ef01128a03b80deec6b2cb8bed122994e8adc2 guix-build-20adaeaef5fa/output/arm64-apple-darwin/bitcoin-20adaeaef5fa-arm64-apple-darwin-unsigned.dmg 696660e030accadc27901dfb4e120aa2fefefa8cc2a33ae887e3c98e5d4795f5 guix-build-20adaeaef5fa/output/arm64-apple-darwin/bitcoin-20adaeaef5fa-arm64-apple-darwin-unsigned.tar.gz 30dcd3f543781ac0e07e36336c2901a25a0829e0d1425c25b3c7aba1d0e5420e guix-build-20adaeaef5fa/output/arm64-apple-darwin/bitcoin-20adaeaef5fa-arm64-apple-darwin.tar.gz 4d63db45f28fcb99aa8f3b30cf06afef80dd308a8d2fdf874752accb3f341258 guix-build-20adaeaef5fa/output/dist-archive/bitcoin-20adaeaef5fa.tar.gz eb208b98b3118e9f8240aab91c7ecb2f9b778109bc19d81d0ba73b3e35aa1123 guix-build-20adaeaef5fa/output/powerpc64-linux-gnu/SHA256SUMS.part 8b0de7008b1932ed18d3ab71ca309dc4919096e226e0a7197bd192e1ba96da82 guix-build-20adaeaef5fa/output/powerpc64-linux-gnu/bitcoin-20adaeaef5fa-powerpc64-linux-gnu-debug.tar.gz bcbc269cc4b5883397c516ef3ef6df564f4a81c240d5afcf912a2bf9554ff148 guix-build-20adaeaef5fa/output/powerpc64-linux-gnu/bitcoin-20adaeaef5fa-powerpc64-linux-gnu.tar.gz e5f7fd823056449a495a68d18fe941b472479bc59d9d4d11a041a4e2cc4044ec guix-build-20adaeaef5fa/output/powerpc64le-linux-gnu/SHA256SUMS.part 73ee7e786372b32ab840f0c00ca0479ddd022b3d37219cd929cb49f744c174e3 guix-build-20adaeaef5fa/output/powerpc64le-linux-gnu/bitcoin-20adaeaef5fa-powerpc64le-linux-gnu-debug.tar.gz 08f64c9aae4d9beef88d8fbae8ad0152517de74bedc88540775c4f757c8b6b9a guix-build-20adaeaef5fa/output/powerpc64le-linux-gnu/bitcoin-20adaeaef5fa-powerpc64le-linux-gnu.tar.gz fe3c28fdb1ee9d5b6ca3ba4510d61c052567edb3b93fdde929ed197072c0fd66 guix-build-20adaeaef5fa/output/riscv64-linux-gnu/SHA256SUMS.part 890d6b96edcc431620eede6239dec51368aff917010e03dabeb29d6a672d7a28 guix-build-20adaeaef5fa/output/riscv64-linux-gnu/bitcoin-20adaeaef5fa-riscv64-linux-gnu-debug.tar.gz df1fc0c9af4799cfe170444e21965f2a600aa193fdd0da542fedceeb3081b194 guix-build-20adaeaef5fa/output/riscv64-linux-gnu/bitcoin-20adaeaef5fa-riscv64-linux-gnu.tar.gz f69cae0b2d0eadb336cf314a888b1e0bed241f38954fe58ca9c9c2d00e49b74e guix-build-20adaeaef5fa/output/x86_64-apple-darwin/SHA256SUMS.part acc5fa9725bba738d10bb8b1e7df2d8a7b0e648015e1c046f67451d343f68224 guix-build-20adaeaef5fa/output/x86_64-apple-darwin/bitcoin-20adaeaef5fa-x86_64-apple-darwin-unsigned.dmg 7e4d8cb6d74434ba9084f487187c49cd5a4138c9ae03a6c2236cdffadb236bc8 guix-build-20adaeaef5fa/output/x86_64-apple-darwin/bitcoin-20adaeaef5fa-x86_64-apple-darwin-unsigned.tar.gz 8d93add28b20dfb2a556d3867cfbf218db336d7eefee6ab6f76a1bb4dd4ae20b guix-build-20adaeaef5fa/output/x86_64-apple-darwin/bitcoin-20adaeaef5fa-x86_64-apple-darwin.tar.gz ba0863eda963db706d2880daa8bc526e6332097010fa7227f513a2d715b6cd6c guix-build-20adaeaef5fa/output/x86_64-linux-gnu/SHA256SUMS.part 6915794f3cdc8ad9b305b6baa58f89f7493097b88c0af190d52d93457a17e8d8 guix-build-20adaeaef5fa/output/x86_64-linux-gnu/bitcoin-20adaeaef5fa-x86_64-linux-gnu-debug.tar.gz 467b05298058ec507c3b247c423f3ea7e027ecf62e45d7ae4b81160118bc0d02 guix-build-20adaeaef5fa/output/x86_64-linux-gnu/bitcoin-20adaeaef5fa-x86_64-linux-gnu.tar.gz 51a534803deaabcbba27d82359ef46e4d5b9e7b121ab71e1975c2a0d1c4c6f45 guix-build-20adaeaef5fa/output/x86_64-w64-mingw32/SHA256SUMS.part c9c5496f20bac01ed6439746aff9ca3dd55708718902c898e99f3d5741b167a3 guix-build-20adaeaef5fa/output/x86_64-w64-mingw32/bitcoin-20adaeaef5fa-win64-debug.zip cfaac54be36789927f83172c0af44c50648f63df7cdc9d81774a170e5ab6e3e5 guix-build-20adaeaef5fa/output/x86_64-w64-mingw32/bitcoin-20adaeaef5fa-win64-setup-unsigned.exe 759b79660c291dcc7da88088de3bb666162fed5c9d94bb24f10cef6e781c565f guix-build-20adaeaef5fa/output/x86_64-w64-mingw32/bitcoin-20adaeaef5fa-win64-unsigned.tar.gz f0124333d384ff6962e2131c7b2814bf5c968e77b63ff1b2c7d19cb4c571757c guix-build-20adaeaef5fa/output/x86_64-w64-mingw32/bitcoin-20adaeaef5fa-win64.zip ``` ACKs for top commit: jarolrod: ACK 20adaea Tree-SHA512: 8b515b95ba4d41ca2ce91448339841dcfb80feb028e9e3bc67a72e72d93669e1257534c11286489a60ae240f6ad6e68f56615818fefd1c09a07a1bee4976fa6e
b8b59ff gui: update the screen after loading wallet (w0xlt) Pull request description: Currently, the user loads a wallet and the screen does not switch to the selected wallet after loading (File -> Open Wallet -> wallet name). This PR changes that by making the `OpenWalletActivity::opened` signal connection a `Qt::QueuedConnection` type. ACKs for top commit: jarolrod: ACK b8b59ff hebasto: ACK b8b59ff, tested on Ubuntu 22.04. Tree-SHA512: 43cd755638b643f481014a7933a0af25df2d109e859cb5f878bc04e562950d550716fa38465140060e28526b2441688580cbcbe4ec6819566b4f95162ca5e527
fa5cbf2 ci: Properly set COMMIT_RANGE in lint task (MarcoFalke) Pull request description: Currently the variable holds (apart from the commits in the pull request) all commits to master since the pull was opened. This is problematic, because already merged commits are linted in unrelated pulls, leading to: * Wasted resources. For example, currently the lint task may take 9 minutes, when it should take 1. See https://cirrus-ci.com/task/6032782770569216?logs=lint#L1449 * False failures. For example, when a "wrong" commit is in master it can lead to some pulls failing unrelatedly, and others not. Now that the CI has the `/merge` commit (since commit fad7281), `COMMIT_RANGE` can simply be set to `HEAD~..HEAD` to only hold the changes in the pull. ACKs for top commit: fanquake: ACK fa5cbf2 Tree-SHA512: e85fca4ca9d2615ddd2544403485e06885769a3f70bca297e23eefda2a1d28f47c5271f6adfa6ce0e5e972335c78098b76e0db4b109f59d0986bf508cef7528f
b68e5a7 lint: specify the right commit range when running locally (James O'Beirne) dff7ed5 test: add an easy way to run linters locally (James O'Beirne) Pull request description: Adds a Dockerfile configuration ~~(originally written mostly by fanquake)~~ that allows straightforward running of linters with compatible versions locally. This removes a ton of annoyance when trying to appease CI, because many of the linter versions are quite old and difficult to maintain locally. I realize that people may not be thrilled to add more ancillary tooling to the repo, but I think this makes a lot of sense given the linter versions listed in this container configuration are dictated by this repo (within the CI configuration), so having these things live in two separate places is a recipe for version mismatches. Eventually we can likely just use this container on CI directly to avoid any chance of inconsistencies between local dev experience and CI. ACKs for top commit: aureleoules: ACK b68e5a7 stickies-v: ACK b68e5a7 john-moffett: ACK b68e5a7 Tree-SHA512: 7ef7a5dae023d81fdb6296d5d92dfa074ee321c7993e607c9f014d0f21c91558611aa00fc3ce1edc7b5e68371aea0d27fa1931291a79bb867a6c783bb536775f
f11eb1f wallet: permit mintxfee=0 (willcl-ark) Pull request description: Fixes bitcoin#26797 Permit nodes to use `-mintxfee=0`. Values below 0 are handled by the ParseMoney() check. ACKs for top commit: MarcoFalke: review ACK f11eb1f john-moffett: ACK f11eb1f Tree-SHA512: 3bf50362bced4fee8e3a846cfb46f1c65dd607c9c824aa3f8c52294371b0646d167a04772d5302bdbee35bbaf407ef0aa634228f70e522c3e423f4213b4ae071
…onnections=0` fabb95e doc: add release note for 26899 (brunoerg) c84c5f6 p2p: set `-dnsseed` and `-listen` false if `maxconnections=0` (brunoerg) Pull request description: If `maxconnections=0`, it means our possible connections are going to be manual (e.g via `addnode`). For this reason, we can skip DNS seeds and set `listen` false. ACKs for top commit: achow101: ACK fabb95e vasild: ACK fabb95e 1440000bytes: reACK bitcoin@fabb95e Tree-SHA512: 33919a784723a32450f39ee4f6de3e27cc7c7f4c6ab4b8ce673981d461df334197deaf43e3f882039fa1ac36b2fddc6c6ab4413512d6c393d4a6865302dd05e7
…" test 228d6a2 build: Fix regression in "ARMv8 CRC32 intrinsics" test (Hennadii Stepanov) Pull request description: In the master branch, the `aarch64` binaries lack support for CRC32 intrinsics. The `vmull_p64` is a part of the Crypto extensions from the ACLE. They are optional extensions, so they get enabled with a `+crypto` for architecture flags. The regression was introduced in bitcoin#26183 (v25.0). The `./configure` script log excerpts: - the master branch @ d752349: ``` checking whether C++ compiler accepts -march=armv8-a+crc... yes checking whether C++ compiler accepts -march=armv8-a+crypto... yes checking for ARMv8 CRC32 intrinsics... no checking for ARMv8 SHA-NI intrinsics... yes ``` - this PR: ``` checking whether C++ compiler accepts -march=armv8-a+crc+crypto... yes checking whether C++ compiler accepts -march=armv8-a+crypto... yes checking for ARMv8 CRC32 intrinsics... yes checking for ARMv8 SHA-NI intrinsics... yes ``` Guix build: ``` x86_64 2afd81f540c6d3b36ff305e88bafe935e4272cd3efef3130aa69d49a0522541b guix-build-228d6a2969e4/output/aarch64-linux-gnu/SHA256SUMS.part 6c704d6d30d495adb3fb86befdb500eb389a02c1167163f14ab5c3c3e630e6b3 guix-build-228d6a2969e4/output/aarch64-linux-gnu/bitcoin-228d6a2969e4-aarch64-linux-gnu-debug.tar.gz e4419963c9c0d99adc4e38538900b648f2c14f793b60c8ee2e6f5acc9d3fadd3 guix-build-228d6a2969e4/output/aarch64-linux-gnu/bitcoin-228d6a2969e4-aarch64-linux-gnu.tar.gz 7d11052b6bd28cdf26d5f2a4987f02d32c93a061907bcd048fb6d161a0466ca9 guix-build-228d6a2969e4/output/dist-archive/bitcoin-228d6a2969e4.tar.gz ``` ACKs for top commit: TheCharlatan: ACK 228d6a2 Tree-SHA512: 4c27ca8acb953bf56e972d907a282ee19e3f30f7a4bf8a9822395fe0e28977cd6233e8b65b4a25cc1d3d5ff6a796d7af07653e18531c44ee3efaff1563d96d32
… one most recently opened/restored/created 99c0eb9 Fix RPCConsole wallet selection (John Moffett) Pull request description: If a user opens multiple wallets in the GUI from the menu bar, the last one opened is the active one in the main window. However, For the RPC Console window, the _first_ one opened is active. This can be confusing, as wallet RPC commands may be sent to a wallet the user didn't intend. This PR makes the RPC Console switch to the wallet just opened / restored / created from the menu bar, which is how the main GUI now works. Similar to bitcoin-core/gui#665 and specifically requested [in a comment](bitcoin-core/gui#665 (comment)). ACKs for top commit: luke-jr: utACK 99c0eb9 hebasto: ACK 99c0eb9, tested on Ubuntu 23.04. Tree-SHA512: d5e5acdaa114130ad4d27fd3f25393bc8d02d92b5001cd39352601d04283cdad3bd62c4da6d369c69764e3b188e9cd3e83152c00b09bd42966082ad09037c328
PastaPastaPasta
force-pushed
the
develop-trivial-2025-04-22-p2
branch
from
d4a5d53 to
e9f079d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/qt/rpcconsole.cpp (1)
867-871: Implementation looks good, but consider adding error handling.The implementation correctly uses QVariant to set the wallet selector's current index. However, there's no error handling if the wallet model isn't found in the selector (which would return -1 from findData).
Consider adding a check before setting the index:
void RPCConsole::setCurrentWallet(WalletModel* const wallet_model) { QVariant data = QVariant::fromValue(wallet_model); - ui->WalletSelector->setCurrentIndex(ui->WalletSelector->findData(data)); + int index = ui->WalletSelector->findData(data); + if (index >= 0) { + ui->WalletSelector->setCurrentIndex(index); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.cirrus.yml(1 hunks)ci/lint/04_install.sh(2 hunks)ci/lint/06_script.sh(1 hunks)ci/lint/Dockerfile(1 hunks)ci/lint/docker-entrypoint.sh(1 hunks)configure.ac(1 hunks)doc/release-notes-6649.md(1 hunks)src/init.cpp(2 hunks)src/qt/bitcoingui.cpp(3 hunks)src/qt/rpcconsole.cpp(1 hunks)src/qt/rpcconsole.h(1 hunks)src/wallet/wallet.cpp(1 hunks)test/lint/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- doc/release-notes-6649.md
🚧 Files skipped from review as they are similar to previous changes (10)
- ci/lint/06_script.sh
- test/lint/README.md
- ci/lint/04_install.sh
- configure.ac
- src/wallet/wallet.cpp
- ci/lint/Dockerfile
- .cirrus.yml
- src/qt/bitcoingui.cpp
- src/init.cpp
- ci/lint/docker-entrypoint.sh
🔇 Additional comments (1)
src/qt/rpcconsole.h (1)
146-149: LGTM: Well-defined new public slot for wallet selection.This new public slot method for setting the current wallet is well-structured and properly enclosed within wallet-specific preprocessor directives. The method signature with a const pointer parameter is consistent with the existing codebase style.
6 participants
Issue being fixed or feature implemented
Batch of trivial backports
How Has This Been Tested?
Built locally
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.