Bitcoin Core Github
45 subscribers
118K links
Download Telegram
👍 vasild approved a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2774917380)
ACK d0a2a5239d6c68d45f0ed9b113131fc4273d1214
💬 maflcko commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2048465023)
q in 1c2ea560300f57e6b5ed75dee2597ca55dd76c6f: Would it not be easier if this was auto-detected? That is, if the type is string, would it not be better to internally (before dispatching to the cli) add quotes to all strings?

This probably means that numbers must be passed as `int`, or `Decimal` on the python side, which seems fine.
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2812144686)
`0e97a68e13...1f71111e21`: fix AI and CI (even more)
fanquake closed an issue: "bench: WalletMigration benchmark broken"
(https://github.com/bitcoin/bitcoin/issues/32277)
🚀 fanquake merged a pull request: "bench: Fix WalletMigration benchmark"
(https://github.com/bitcoin/bitcoin/pull/32281)
💬 TheCharlatan commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2812207470)
Post-merge ACK
💬 TheCharlatan commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#discussion_r2048519877)
Not really, but I guess we can continue over there.
💬 maflcko commented on pull request "ci: Slim down lint image":
(https://github.com/bitcoin/bitcoin/pull/32250#issuecomment-2812217957)
rfm with two tested acks, or does it need more review?
💬 fanquake commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#issuecomment-2812247649)
If this ends up fixing all issues, should add `--usecli` to atleast one CI, so we catch regressions?
💬 fanquake commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048555899)
I don't think we need to introduce checks here, we already use `clock_gettime`. You could just #ifdef `CLOCK_THREAD_CPUTIME_ID` at the callsite.
💬 fanquake commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048557177)
I don't think we check for any other Windows functions like this, I think you can just #ifdef WIN32 at the callsite.
💬 fanquake commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048557940)
If you're going to link to external documentation, there's no need to copy paragraphs of it, verbatim, into our source code.
💬 fanquake commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048558857)
Comment seems unnecessary?
💬 fanquake commented on pull request "doc: Add note for building on macOS (Intel) with CMake ≥ 4.0":
(https://github.com/bitcoin/bitcoin/pull/32289#discussion_r2048575632)
This says it applies to users with Homebrew installed, when *not* building with homebrew-provided tools? What are the "tools" here? This seems vauge, and as a user reading this I probably wouldn't know if it applies to me.
💬 fanquake commented on pull request "doc: Add note for building on macOS (Intel) with CMake ≥ 4.0":
(https://github.com/bitcoin/bitcoin/pull/32289#issuecomment-2812293989)
Not sure. I see this is just copied from the CMake release notes, but it doesn't really explain anything, and it's not clear what warnings that were being supressed no-longer are. Also seems like the warnings are a sideeffect of some other issues, that should probably be fixed *properly*, in some way.
💬 fanquake commented on pull request "build: Restore cross-compilation for Android":
(https://github.com/bitcoin/bitcoin/pull/32262#issuecomment-2812297608)
> as CI integration still needs to be added.

Not sure. As long as we aren't shipping this in releases, I don't think it needs to be added to the CI (the same could also be said for some other jobs), and shouldn't be a blocker for making changes (iirc the previous android job used to sporadically fail quite often, failing to download JDKs and related things). Especially when there are other more important things, which we do support, which we would be better off spending the CPU on (guix build
...
💬 fanquake commented on pull request "guix: Remove no longer necessary `file` package":
(https://github.com/bitcoin/bitcoin/pull/32242#issuecomment-2812301514)
> My only guess is that it was used by autotools detection code indirectly, somehow.

Yea, `file` was checked-for by Autotools, but maybe non of the related checks affected our build.

If the conclusion is that this was never used, can the PR title, commit message, and PR description be fixed to reflect that.
💬 hebasto commented on pull request "ci: drop -priority-level from bench in win cross CI":
(https://github.com/bitcoin/bitcoin/pull/32288#discussion_r2048583154)
Why not drop `-priority-level=high` here as well:https://github.com/bitcoin/bitcoin/blob/7a3afe6787bc36fd98684eac020f9abdbfae6f0a/src/bench/CMakeLists.txt#L82-L84 ?

Will it have a significant impact on execution time?
💬 fanquake commented on pull request "ci: drop -priority-level from bench in win cross CI":
(https://github.com/bitcoin/bitcoin/pull/32288#discussion_r2048585578)
I'm just changing CI, so that we catch regressions. Someone could follow up with changing this for all developers & users.
💬 maflcko commented on pull request "build: Restore cross-compilation for Android":
(https://github.com/bitcoin/bitcoin/pull/32262#issuecomment-2812310449)
I'd say a CI task config should exist. Otherwise, there is no way to test the build easily for non-android people. No opinion on adding it to the live CI matrix here in this repo.
💬 vasild commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2812322401)
`bb822a5ee1...ee16345af3`: address suggestions