📝 0xf58ce opened a pull request: "list 6467600: 334knUNdf1FSCDvrYsmK3N4uGv3nJnj7a3"
(https://github.com/bitcoin/bitcoin/pull/31786)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/31786)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
✅ fanquake closed a pull request: "list 6467600: 334knUNdf1FSCDvrYsmK3N4uGv3nJnj7a3"
(https://github.com/bitcoin/bitcoin/pull/31786)
(https://github.com/bitcoin/bitcoin/pull/31786)
📝 fanquake locked a pull request: "list 6467600: 334knUNdf1FSCDvrYsmK3N4uGv3nJnj7a3"
(https://github.com/bitcoin/bitcoin/pull/31786)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/31786)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 hebasto commented on pull request "cmake: Fix `-pthread` flags in summary":
(https://github.com/bitcoin/bitcoin/pull/31724#discussion_r1939606312)
> I agree, but in that situation we would be back to printing generator expressions in the flags summary...
Unfortunately, it happens not only with unknown future changes but also with older yet supported CMake versions, such as 3.25, where `$<$<NOT:$<COMPILE_LANG_AND_ID:CUDA,NVIDIA>>:-pthread>` is printed.
(https://github.com/bitcoin/bitcoin/pull/31724#discussion_r1939606312)
> I agree, but in that situation we would be back to printing generator expressions in the flags summary...
Unfortunately, it happens not only with unknown future changes but also with older yet supported CMake versions, such as 3.25, where `$<$<NOT:$<COMPILE_LANG_AND_ID:CUDA,NVIDIA>>:-pthread>` is printed.
👍 willcl-ark approved a pull request: "lint: Call more checks from test_runner"
(https://github.com/bitcoin/bitcoin/pull/31653#pullrequestreview-2590369181)
tACK faf8fc5487d409eeff7b7b260eabb6929a7b7a5f
Makes sense to move this into test_runner.
Worked correctly for me, although I did forget that we currently can't run the linter (and local CI) in a worktree, which I was using.
I'll try and open a PR to fix that shortly.
(https://github.com/bitcoin/bitcoin/pull/31653#pullrequestreview-2590369181)
tACK faf8fc5487d409eeff7b7b260eabb6929a7b7a5f
Makes sense to move this into test_runner.
Worked correctly for me, although I did forget that we currently can't run the linter (and local CI) in a worktree, which I was using.
I'll try and open a PR to fix that shortly.
💬 maflcko commented on pull request "Pass custom DEP_OPTS and CONFIG_FLAGS to guix-build":
(https://github.com/bitcoin/bitcoin/pull/31763#issuecomment-2631420290)
It seems a bit confusing to leak the env into guix, which may then cause non-determinism, compiling the same source commit id. It seems simpler to just create a commit with your modifications to the config.
(https://github.com/bitcoin/bitcoin/pull/31763#issuecomment-2631420290)
It seems a bit confusing to leak the env into guix, which may then cause non-determinism, compiling the same source commit id. It seems simpler to just create a commit with your modifications to the config.
💬 maflcko commented on pull request "lint: Call more checks from test_runner":
(https://github.com/bitcoin/bitcoin/pull/31653#issuecomment-2631425896)
> can't run the linter (and local CI) in a worktree
Jup, see https://github.com/bitcoin/bitcoin/issues/30028 for the CI part.
(https://github.com/bitcoin/bitcoin/pull/31653#issuecomment-2631425896)
> can't run the linter (and local CI) in a worktree
Jup, see https://github.com/bitcoin/bitcoin/issues/30028 for the CI part.
📝 willcl-ark opened a pull request: "ci: run in worktrees"
(https://github.com/bitcoin/bitcoin/pull/31787)
Fixes: #30028
- Run CI in worktrees by mounting the .git root target into the container
- Run the linter in worktrees by mounting the .git root target into the container (using a new helper script)
AFAIK mounting the git root should not present any additional security issues, as this would already be mounted (in RO mode) when not using a worktree.
The new run_lint.sh script is not currently used in CI (or anywhere else).
(https://github.com/bitcoin/bitcoin/pull/31787)
Fixes: #30028
- Run CI in worktrees by mounting the .git root target into the container
- Run the linter in worktrees by mounting the .git root target into the container (using a new helper script)
AFAIK mounting the git root should not present any additional security issues, as this would already be mounted (in RO mode) when not using a worktree.
The new run_lint.sh script is not currently used in CI (or anywhere else).
💬 maflcko commented on pull request "ci: run in worktrees":
(https://github.com/bitcoin/bitcoin/pull/31787#discussion_r1939667249)
Why is any `.git` data needed to be copied, when an init `.git` is sufficient?
It would be easier to just call `git init`?
(https://github.com/bitcoin/bitcoin/pull/31787#discussion_r1939667249)
Why is any `.git` data needed to be copied, when an init `.git` is sufficient?
It would be easier to just call `git init`?
💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2631519464)
Personally, I think it is fine to just have it in the nightly repo as per https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2626845471, but no strong opinion.
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2631519464)
Personally, I think it is fine to just have it in the nightly repo as per https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2626845471, but no strong opinion.
📝 brunoerg converted_to_draft a pull request: "fuzz: wallet: add target for spkm migration"
(https://github.com/bitcoin/bitcoin/pull/29694)
This PR adds a fuzz target for `ScriptPubKeyMan` migration. It creates a `LegacyDataSPKM` which can have some keys/scripts/etc, and then migrate it to descriptor. I tried to keep it as compatible as possible with future legacy wallet removal.
(https://github.com/bitcoin/bitcoin/pull/29694)
This PR adds a fuzz target for `ScriptPubKeyMan` migration. It creates a `LegacyDataSPKM` which can have some keys/scripts/etc, and then migrate it to descriptor. I tried to keep it as compatible as possible with future legacy wallet removal.
💬 willcl-ark commented on pull request "ci: run in worktrees":
(https://github.com/bitcoin/bitcoin/pull/31787#discussion_r1939723475)
Hmmm, this was actually my first approach in the lint script, as it was simpler. Once I got this working in the Ci script, I backported a "matching" version to the linter script.
Looking at:
https://github.com/bitcoin/bitcoin/blob/1172bc4157eefe80d1aaf0b56459857ec651e535/ci/test/01_base_install.sh#L11-L16
...I mistakenly thought that `.git` was actually needed, but i) it's mounted RO, and ii) it's only used as above here as far as I can see in the whole CI.
Can we just touch a (
...
(https://github.com/bitcoin/bitcoin/pull/31787#discussion_r1939723475)
Hmmm, this was actually my first approach in the lint script, as it was simpler. Once I got this working in the Ci script, I backported a "matching" version to the linter script.
Looking at:
https://github.com/bitcoin/bitcoin/blob/1172bc4157eefe80d1aaf0b56459857ec651e535/ci/test/01_base_install.sh#L11-L16
...I mistakenly thought that `.git` was actually needed, but i) it's mounted RO, and ii) it's only used as above here as far as I can see in the whole CI.
Can we just touch a (
...
📝 rhysbeynon opened a pull request: "Case fix"
(https://github.com/bitcoin-core/gui/pull/851)
fixes letter case in icon filename to prior default.
(https://github.com/bitcoin-core/gui/pull/851)
fixes letter case in icon filename to prior default.
✅ rhysbeynon closed a pull request: "Case fix"
(https://github.com/bitcoin-core/gui/pull/851)
(https://github.com/bitcoin-core/gui/pull/851)
💬 maflcko commented on pull request "ci: run in worktrees":
(https://github.com/bitcoin/bitcoin/pull/31787#discussion_r1939733626)
> ...I mistakenly thought that `.git` was actually needed, but i) it's mounted RO, and ii) it's only used as above here as far as I can see in the whole CI.
It is also used in tidy via `git --no-pager diff`.
> Can we just touch a (any) file to persist the flag then, and drop all `.git` requirement?
Yes, but I don't know how to do that in a one-liner cross-platform (linux+mac), taking into account the different root trees and permissions. So just using git seems simplest?
(https://github.com/bitcoin/bitcoin/pull/31787#discussion_r1939733626)
> ...I mistakenly thought that `.git` was actually needed, but i) it's mounted RO, and ii) it's only used as above here as far as I can see in the whole CI.
It is also used in tidy via `git --no-pager diff`.
> Can we just touch a (any) file to persist the flag then, and drop all `.git` requirement?
Yes, but I don't know how to do that in a one-liner cross-platform (linux+mac), taking into account the different root trees and permissions. So just using git seems simplest?
📝 rhysbeynon opened a pull request: "Updated MacOS icon to more closely fit Apple's design standards"
(https://github.com/bitcoin-core/gui/pull/852)
_This is a duplicate of my mis-appropriated fork of [bitcoin/bitcoin](https://github.com/bitcoin/bitcoin)_
### Mac OS Minor icon change
**This is a fix to an outdated format of app icon. I am not trying to change re-invent the wheel with this, I am simply fixing a minor perceived issue. This should not be seen as a subjective design change as it does not change anything substantially.**
Apple's design guidelines have changed since the previous iteration of the Bitcoin-Core app icon. As
...
(https://github.com/bitcoin-core/gui/pull/852)
_This is a duplicate of my mis-appropriated fork of [bitcoin/bitcoin](https://github.com/bitcoin/bitcoin)_
### Mac OS Minor icon change
**This is a fix to an outdated format of app icon. I am not trying to change re-invent the wheel with this, I am simply fixing a minor perceived issue. This should not be seen as a subjective design change as it does not change anything substantially.**
Apple's design guidelines have changed since the previous iteration of the Bitcoin-Core app icon. As
...
💬 Sjors commented on pull request "Have createNewBlock() ensure m_tip_block is set":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2631596373)
There were slightly more worms in this can.
Instead I've now changed `createNewBlock()` to first call `waitTipChanged()` and the latter to return null during a shutdown. This shifts some work to the RPC, but I think makes the overall behaviour easier to understand.
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2631596373)
There were slightly more worms in this can.
Instead I've now changed `createNewBlock()` to first call `waitTipChanged()` and the latter to return null during a shutdown. This shifts some work to the RPC, but I think makes the overall behaviour easier to understand.
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1939755012)
The latest push has `waitTipChanged` and `createNewBlock` return null if the node shuts down, and documentation is updated to reflect that.
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1939755012)
The latest push has `waitTipChanged` and `createNewBlock` return null if the node shuts down, and documentation is updated to reflect that.
✅ Sjors closed a pull request: "Pass custom DEP_OPTS and CONFIG_FLAGS to guix-build"
(https://github.com/bitcoin/bitcoin/pull/31763)
(https://github.com/bitcoin/bitcoin/pull/31763)
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939763961)
> though I could include it here.
It turned into a bigger change than I expected, so better to keep it separate from this. I don't think it's blocking.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939763961)
> though I could include it here.
It turned into a bigger change than I expected, so better to keep it separate from this. I don't think it's blocking.