π hodlinator opened a pull request: "doc: Improve CI docs on env and qemu-user-static"
(https://github.com/bitcoin/bitcoin/pull/33887)
Should at least partially fix #31199
(https://github.com/bitcoin/bitcoin/pull/33887)
Should at least partially fix #31199
π¬ hodlinator commented on issue "CI: Improve documentation around replicating CI locally":
(https://github.com/bitcoin/bitcoin/issues/31199#issuecomment-3542031492)
PR is live: #33887
(https://github.com/bitcoin/bitcoin/issues/31199#issuecomment-3542031492)
PR is live: #33887
π¬ waketraindev commented on pull request "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3542059320)
I think the `AutoFile::size` implementation and the subsequent refactoring would fit better in their own dedicated refactoring PR, or in a PR where they're actually required. They don't seem to be needed by either of the other two commits (the `LogWarning` change in `asmap.cpp` or the newly added test vectors in `netbase_tests.cpp`).
I understand this is listed as "selected minor preparatory work", and I realize this is nitpicking a bit, but it would still be cleaner to introduce these change
...
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3542059320)
I think the `AutoFile::size` implementation and the subsequent refactoring would fit better in their own dedicated refactoring PR, or in a PR where they're actually required. They don't seem to be needed by either of the other two commits (the `LogWarning` change in `asmap.cpp` or the newly added test vectors in `netbase_tests.cpp`).
I understand this is listed as "selected minor preparatory work", and I realize this is nitpicking a bit, but it would still be cleaner to introduce these change
...
π¬ marcofleon commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#issuecomment-3542072403)
Nice, lgtm ACK 51917d94e657024cc0af8aa748a9d5d39a617e85
(https://github.com/bitcoin/bitcoin/pull/32624#issuecomment-3542072403)
Nice, lgtm ACK 51917d94e657024cc0af8aa748a9d5d39a617e85
π maflcko approved a pull request: "doc: Improve CI docs on env and qemu-user-static"
(https://github.com/bitcoin/bitcoin/pull/33887#pullrequestreview-3472936753)
lgtm ACK a8afb65ce9b8cd5fe90fcc570087f175c3662cad
nit: Not sure if the commit message is fully correct
(https://github.com/bitcoin/bitcoin/pull/33887#pullrequestreview-3472936753)
lgtm ACK a8afb65ce9b8cd5fe90fcc570087f175c3662cad
nit: Not sure if the commit message is fully correct
π¬ maflcko commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2534252107)
For reference, this was half-fixed in commit fd813bf863b1ffa91429de6342285b35bab2bfa4 for env vars that have nothing to do with the CI (like `DEBUGINFOD_URLS`). However, it is still needed for env vars that have a meaning outside and inside CI (like `HOST`)
However, your `HOST` is not set, so the comment "Should help in cases such as: https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2526410039" may not be accurate here.
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2534252107)
For reference, this was half-fixed in commit fd813bf863b1ffa91429de6342285b35bab2bfa4 for env vars that have nothing to do with the CI (like `DEBUGINFOD_URLS`). However, it is still needed for env vars that have a meaning outside and inside CI (like `HOST`)
However, your `HOST` is not set, so the comment "Should help in cases such as: https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2526410039" may not be accurate here.
π¬ laanwj commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2534280150)
Isn't this `ip_bytes` now? (though bits is technically still valid ofc)
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2534280150)
Isn't this `ip_bytes` now? (though bits is technically still valid ofc)
π¬ maflcko commented on issue "CI: Improve documentation around replicating CI locally":
(https://github.com/bitcoin/bitcoin/issues/31199#issuecomment-3542114544)
> Another related idea was to move the CC/CXX/SANITZER/... build options into a cmake preset, which could make it easier to reproduce some CI failures. Ref: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-09-05#1050658 and some more discussion in [#30871 (comment)](https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344047118)
this was moved to dev-mode in https://github.com/bitcoin/bitcoin/pull/33824, so maybe this is already a step in the right direction?
(https://github.com/bitcoin/bitcoin/issues/31199#issuecomment-3542114544)
> Another related idea was to move the CC/CXX/SANITZER/... build options into a cmake preset, which could make it easier to reproduce some CI failures. Ref: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-09-05#1050658 and some more discussion in [#30871 (comment)](https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344047118)
this was moved to dev-mode in https://github.com/bitcoin/bitcoin/pull/33824, so maybe this is already a step in the right direction?
π¬ roconnor-blockstream commented on issue "Standardness policy rules for legacy Multisig script is incoherent":
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3542129055)
> The requirement is for m not to be greater than n, as far as I can see? (via both IsStandard() and Solver())
Yeah, I meant to say "less than or equal to". However, AFAICT this inequality is only enforced in standardness, not solvability.
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3542129055)
> The requirement is for m not to be greater than n, as far as I can see? (via both IsStandard() and Solver())
Yeah, I meant to say "less than or equal to". However, AFAICT this inequality is only enforced in standardness, not solvability.
π¬ laanwj commented on issue "guix build fails on RISC-V due to python-py-cpuinfo test failure":
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3542166449)
Right! i could take a look at fixing this upstream.
From a high level perspective, i don't quite understand why a cpuinfo module would be needed for deterministic cross-compiling in the first place. Nor why it'd run tests that depend on local CPU configuration. But i guess it's some deeper nested dependency that gets pulled in unconditionally (through LIEF in this case?). GUIX sadly has more of those, basically same complaint as in #30042.
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3542166449)
Right! i could take a look at fixing this upstream.
From a high level perspective, i don't quite understand why a cpuinfo module would be needed for deterministic cross-compiling in the first place. Nor why it'd run tests that depend on local CPU configuration. But i guess it's some deeper nested dependency that gets pulled in unconditionally (through LIEF in this case?). GUIX sadly has more of those, basically same complaint as in #30042.
π¬ fanquake commented on pull request "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`":
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2534330486)
Any idea why `BUILD_GUI_TESTS=OFF` was previously set? Looks like it predates CMake, and was just ported; I guess no-longer needed?
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2534330486)
Any idea why `BUILD_GUI_TESTS=OFF` was previously set? Looks like it predates CMake, and was just ported; I guess no-longer needed?
π¬ maflcko commented on issue "upstream: GUIX closure contains too much unnecessary stuff":
(https://github.com/bitcoin/bitcoin/issues/30042#issuecomment-3542213579)
The linked issue is now closed with the last comment:
> With commit 9fa92acbf0c4dbc734ac7d83b31bd6d12e09a401 (https://codeberg.org/guix/guix/commit/9fa92acbf0c4dbc734ac7d83b31bd6d12e09a401) this is mostly
fixed. Thereβs still another path leading to libx11 though:
```
$ ./pre-inst-env guix graph --path guix libx11
guix@1.4.0-6.dc5430c
guile-avahi@0.4.1
avahi@0.8
dbus@1.14.0
libx11@1.8.1
(The same applies to βguix pullβ.)
Not sure what can be done about it.
Ludoβ.
(https://github.com/bitcoin/bitcoin/issues/30042#issuecomment-3542213579)
The linked issue is now closed with the last comment:
> With commit 9fa92acbf0c4dbc734ac7d83b31bd6d12e09a401 (https://codeberg.org/guix/guix/commit/9fa92acbf0c4dbc734ac7d83b31bd6d12e09a401) this is mostly
fixed. Thereβs still another path leading to libx11 though:
```
$ ./pre-inst-env guix graph --path guix libx11
guix@1.4.0-6.dc5430c
guile-avahi@0.4.1
avahi@0.8
dbus@1.14.0
libx11@1.8.1
(The same applies to βguix pullβ.)
Not sure what can be done about it.
Ludoβ.
π¬ maflcko commented on issue "guix build fails on RISC-V due to python-py-cpuinfo test failure":
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3542218159)
Yeah, if it is pulled in through LIEF, it could make sense to strip the unmaintained dependency from LIEF upstream.
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3542218159)
Yeah, if it is pulled in through LIEF, it could make sense to strip the unmaintained dependency from LIEF upstream.
π¬ maflcko commented on pull request "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`":
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2534374440)
I think it couldn't be run in wine. Could make sense to at least build it. Should I do it here, or in a follow-up?
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2534374440)
I think it couldn't be run in wine. Could make sense to at least build it. Should I do it here, or in a follow-up?
π¬ maflcko commented on pull request "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`":
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2534381180)
oh wait, I already removed it here in this commit 6666980e8653d98ef556f71a3e6907d3deda7147, it is also mentioned in the commit message.
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2534381180)
oh wait, I already removed it here in this commit 6666980e8653d98ef556f71a3e6907d3deda7147, it is also mentioned in the commit message.
π¬ fanquake commented on pull request "depends: update xcb-util packages to latest versions":
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3542279628)
Guix Build:
```bash
70818d2b6cce3f70f1b558049b7d24e3e0d688b51b9ec4257f9e21b4167b1753 guix-build-7d2352b6757e/output/aarch64-linux-gnu/SHA256SUMS.part
fe7f75313ba0ad242e42b2fec0918d4ed21b5b466fc9c373b16496151a6bbe96 guix-build-7d2352b6757e/output/aarch64-linux-gnu/bitcoin-7d2352b6757e-aarch64-linux-gnu-debug.tar.gz
4e3226f11a59d5d417d03296a8defbc43fc169b5eb625a3f234fa4edfb5df303 guix-build-7d2352b6757e/output/aarch64-linux-gnu/bitcoin-7d2352b6757e-aarch64-linux-gnu.tar.gz
9809d8794969a4a7
...
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3542279628)
Guix Build:
```bash
70818d2b6cce3f70f1b558049b7d24e3e0d688b51b9ec4257f9e21b4167b1753 guix-build-7d2352b6757e/output/aarch64-linux-gnu/SHA256SUMS.part
fe7f75313ba0ad242e42b2fec0918d4ed21b5b466fc9c373b16496151a6bbe96 guix-build-7d2352b6757e/output/aarch64-linux-gnu/bitcoin-7d2352b6757e-aarch64-linux-gnu-debug.tar.gz
4e3226f11a59d5d417d03296a8defbc43fc169b5eb625a3f234fa4edfb5df303 guix-build-7d2352b6757e/output/aarch64-linux-gnu/bitcoin-7d2352b6757e-aarch64-linux-gnu.tar.gz
9809d8794969a4a7
...
π¬ roconnor-blockstream commented on issue "Standardness policy rules for legacy Multisig script is incoherent":
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3542353380)
> we should make it easy to spend bare multisigs with embedded data, or with hybrid pubkeys
Just to be clear, we cannot allow hybrid pubkeys to be processed by `CHECKMULTISIG` without undermining the purpose of the hybrid pubkey ban. (However we can allow hybrid pubkeys that are unprocessed by `CHECKMULTISIG`.)
My understanding of the hybrid pubkey ban is that it is in place in case we wish to, one day, introduce a soft-fork that would allow implementations of Bitcoin Core to use a normal ECD
...
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3542353380)
> we should make it easy to spend bare multisigs with embedded data, or with hybrid pubkeys
Just to be clear, we cannot allow hybrid pubkeys to be processed by `CHECKMULTISIG` without undermining the purpose of the hybrid pubkey ban. (However we can allow hybrid pubkeys that are unprocessed by `CHECKMULTISIG`.)
My understanding of the hybrid pubkey ban is that it is in place in case we wish to, one day, introduce a soft-fork that would allow implementations of Bitcoin Core to use a normal ECD
...
π¬ hodlinator commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2534494637)
Thanks! Pushed new version which is hopefully slightly more accurate.
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2534494637)
Thanks! Pushed new version which is hopefully slightly more accurate.
π¬ furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534425987)
renamed.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534425987)
renamed.
π¬ furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534443997)
Done.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534443997)
Done.