💬 ajtowns commented on pull request "Move `FreespaceChecker` class into its own module":
(https://github.com/bitcoin-core/gui/pull/881#issuecomment-3136166736)
ACK 3a03f075606b19e411b8bd19870242e0e0b58fcb
(https://github.com/bitcoin-core/gui/pull/881#issuecomment-3136166736)
ACK 3a03f075606b19e411b8bd19870242e0e0b58fcb
📝 hebasto opened a pull request: "cmake: Proactively avoid use of `SECP256K1_DISABLE_SHARED`"
(https://github.com/bitcoin/bitcoin/pull/33101)
The `SECP256K1_DISABLE_SHARED` CMake variable has been [removed](https://github.com/bitcoin-core/secp256k1/pull/1688) upstream.
This PR removes its usage ahead of the next `secp256k1` subtree update to prevent breakage and simplify integration.
(https://github.com/bitcoin/bitcoin/pull/33101)
The `SECP256K1_DISABLE_SHARED` CMake variable has been [removed](https://github.com/bitcoin-core/secp256k1/pull/1688) upstream.
This PR removes its usage ahead of the next `secp256k1` subtree update to prevent breakage and simplify integration.
📝 brunoerg opened a pull request: "fuzz: cover BanMan::IsDiscouraged"
(https://github.com/bitcoin/bitcoin/pull/33102)
This PR adds fuzz coverage for the `IsDiscouraged` function in the banman target. This is the only function missing from `BanMan`.
(https://github.com/bitcoin/bitcoin/pull/33102)
This PR adds fuzz coverage for the `IsDiscouraged` function in the banman target. This is the only function missing from `BanMan`.
💬 maflcko commented on pull request "fuzz: cover BanMan::IsDiscouraged":
(https://github.com/bitcoin/bitcoin/pull/33102#issuecomment-3136269998)
lgtm ACK c2ed576d2caf282a51aa9df5df55d844ab1da794
(https://github.com/bitcoin/bitcoin/pull/33102#issuecomment-3136269998)
lgtm ACK c2ed576d2caf282a51aa9df5df55d844ab1da794
💬 TheBlueMatt commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3136293939)
> The multiprocess binaries are produced by this project, built using the same deterministic build system, using the same code as the monolithic binaries. There are already concrete ways that the binaries can be distributed with minimal changes to our own release process. Users who run the multiprocess binaries don't have to do any special configuration for them to work.
Right, I was ancitipating that, ultimately, an external-process DNS resolution thing would be built and dstributed the same
...
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3136293939)
> The multiprocess binaries are produced by this project, built using the same deterministic build system, using the same code as the monolithic binaries. There are already concrete ways that the binaries can be distributed with minimal changes to our own release process. Users who run the multiprocess binaries don't have to do any special configuration for them to work.
Right, I was ancitipating that, ultimately, an external-process DNS resolution thing would be built and dstributed the same
...
💬 maflcko commented on pull request "ci: allow for any libc++ intrumentation & use it for TSAN":
(https://github.com/bitcoin/bitcoin/pull/33099#discussion_r2242662552)
```suggestion
export USE_INSTRUMENTED_LIBCPP="Thread"
```
Can flatten the option into a single one?
Also, adjust the check below to
```sh
if [ -n "${USE_INSTRUMENTED_LIBCPP}" ]; then
(https://github.com/bitcoin/bitcoin/pull/33099#discussion_r2242662552)
```suggestion
export USE_INSTRUMENTED_LIBCPP="Thread"
```
Can flatten the option into a single one?
Also, adjust the check below to
```sh
if [ -n "${USE_INSTRUMENTED_LIBCPP}" ]; then
💬 maflcko commented on pull request "ci: allow for any libc++ intrumentation & use it for TSAN":
(https://github.com/bitcoin/bitcoin/pull/33099#issuecomment-3136330481)
lgtm, but the tsan depends build seems to be taking significantly longer now for some reason?
(https://github.com/bitcoin/bitcoin/pull/33099#issuecomment-3136330481)
lgtm, but the tsan depends build seems to be taking significantly longer now for some reason?
💬 fanquake commented on pull request "ci: allow for any libc++ intrumentation & use it for TSAN":
(https://github.com/bitcoin/bitcoin/pull/33099#issuecomment-3136334394)
> lgtm, but the tsan depends build seems to be taking significantly longer now for some reason?
Yea, that QT build has started taking a ridiculous amount of time. Maybe we can just disable it in this job?
(https://github.com/bitcoin/bitcoin/pull/33099#issuecomment-3136334394)
> lgtm, but the tsan depends build seems to be taking significantly longer now for some reason?
Yea, that QT build has started taking a ridiculous amount of time. Maybe we can just disable it in this job?
🤔 marcofleon reviewed a pull request: "fuzz: cover BanMan::IsDiscouraged"
(https://github.com/bitcoin/bitcoin/pull/33102#pullrequestreview-3071548512)
ACK c2ed576d2caf282a51aa9df5df55d844ab1da794
(https://github.com/bitcoin/bitcoin/pull/33102#pullrequestreview-3071548512)
ACK c2ed576d2caf282a51aa9df5df55d844ab1da794
💬 maflcko commented on pull request "ci: allow for any libc++ intrumentation & use it for TSAN":
(https://github.com/bitcoin/bitcoin/pull/33099#issuecomment-3136364439)
Yeah, I guess it may be best to disable it for now, if the instrumented libc++ turns the qt depends build into a time sink. For reference, the task never in history took that long, looking at https://0xb10c.github.io/bitcoin-core-ci-stats/graph/build/#TSan,%20depends,%20gui.
Obviously, it would be good to retain depends-qt-tsan as a nightly CI task, but I can open a general brainstorming issue about it.
(https://github.com/bitcoin/bitcoin/pull/33099#issuecomment-3136364439)
Yeah, I guess it may be best to disable it for now, if the instrumented libc++ turns the qt depends build into a time sink. For reference, the task never in history took that long, looking at https://0xb10c.github.io/bitcoin-core-ci-stats/graph/build/#TSan,%20depends,%20gui.
Obviously, it would be good to retain depends-qt-tsan as a nightly CI task, but I can open a general brainstorming issue about it.
🚀 fanquake merged a pull request: "fuzz: cover BanMan::IsDiscouraged"
(https://github.com/bitcoin/bitcoin/pull/33102)
(https://github.com/bitcoin/bitcoin/pull/33102)
💬 fanquake commented on pull request "ci: allow for any libc++ intrumentation & use it for TSAN":
(https://github.com/bitcoin/bitcoin/pull/33099#discussion_r2242730767)
Squashed this down.
(https://github.com/bitcoin/bitcoin/pull/33099#discussion_r2242730767)
Squashed this down.
💬 fanquake commented on pull request "ci: allow for any libc++ intrumentation & use it for TSAN":
(https://github.com/bitcoin/bitcoin/pull/33099#issuecomment-3136406144)
Squashed into a single option. Should have fixed the linter. Dropped Qt.
(https://github.com/bitcoin/bitcoin/pull/33099#issuecomment-3136406144)
Squashed into a single option. Should have fixed the linter. Dropped Qt.
💬 maflcko commented on pull request "ci: Only pass documented env vars":
(https://github.com/bitcoin/bitcoin/pull/33002#discussion_r2242739108)
Thx, done in a move-only doc-comment-only force push
(https://github.com/bitcoin/bitcoin/pull/33002#discussion_r2242739108)
Thx, done in a move-only doc-comment-only force push
🤔 marcofleon reviewed a pull request: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33076#pullrequestreview-3071658257)
lgtm ACK 4d145f9f20a333cbe22579db525292e956af66c9
(https://github.com/bitcoin/bitcoin/pull/33076#pullrequestreview-3071658257)
lgtm ACK 4d145f9f20a333cbe22579db525292e956af66c9
💬 maflcko commented on pull request "ci: Only pass documented env vars":
(https://github.com/bitcoin/bitcoin/pull/33002#issuecomment-3136462428)
> Should we update
It is possible to remove the `env -i ...` wrapping, because it is less required. However, I still somewhat prefer to explicitly and cleanly pass all CI options in a single Bash command, instead of taking anything that matches from the global env. For example, OpenSuse (or other Linux distros) may set `HOST` globally in the env, which is then taken by the CI system, because it is (correctly) recognized as a CI variable. The build will fail in that case. Similarly, if you exp
...
(https://github.com/bitcoin/bitcoin/pull/33002#issuecomment-3136462428)
> Should we update
It is possible to remove the `env -i ...` wrapping, because it is less required. However, I still somewhat prefer to explicitly and cleanly pass all CI options in a single Bash command, instead of taking anything that matches from the global env. For example, OpenSuse (or other Linux distros) may set `HOST` globally in the env, which is then taken by the CI system, because it is (correctly) recognized as a CI variable. The build will fail in that case. Similarly, if you exp
...
💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#issuecomment-3136494016)
Latest push 526403df23a2db781709e4494da3a9f79284531d -> d434155db5f9c34b8ab3e19038d824d161b34195 modifies `logging_filesize_rate_limit` to scan `ReadDebugLogLines` for the `[warning] Restarting logging` string in one place. This is hacky and is needed to make the CI pass when `DEBUG_LOCKCONTENTION` is specified.
The failure happens because:
- `Reset` is in the task queue of the scheduler
- `MockForwardAndSync` calls `MockForward`
- `Reset` will be called and will schedule another `Reset`.
...
(https://github.com/bitcoin/bitcoin/pull/33011#issuecomment-3136494016)
Latest push 526403df23a2db781709e4494da3a9f79284531d -> d434155db5f9c34b8ab3e19038d824d161b34195 modifies `logging_filesize_rate_limit` to scan `ReadDebugLogLines` for the `[warning] Restarting logging` string in one place. This is hacky and is needed to make the CI pass when `DEBUG_LOCKCONTENTION` is specified.
The failure happens because:
- `Reset` is in the task queue of the scheduler
- `MockForwardAndSync` calls `MockForward`
- `Reset` will be called and will schedule another `Reset`.
...
👍 Sjors approved a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-3071553399)
ACK 5fe7915c865a8e7c0a95ec376d0f1ff737f5d1c2
I would suggest a followup PR to improve various code comments and such.
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-3071553399)
ACK 5fe7915c865a8e7c0a95ec376d0f1ff737f5d1c2
I would suggest a followup PR to improve various code comments and such.
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2242695935)
In https://github.com/bitcoin/bitcoin/commit/d576079ab470db4f500d0f2df5ddc77ab65e74cc _tests: Test musig() parsing:_ alternatively, mention BIP 328 here
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2242695935)
In https://github.com/bitcoin/bitcoin/commit/d576079ab470db4f500d0f2df5ddc77ab65e74cc _tests: Test musig() parsing:_ alternatively, mention BIP 328 here
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2242689740)
In d576079ab470db4f500d0f2df5ddc77ab65e74cc _tests: Test musig() parsing_: could mention BIP 328, which explains where the extra key comes from.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2242689740)
In d576079ab470db4f500d0f2df5ddc77ab65e74cc _tests: Test musig() parsing_: could mention BIP 328, which explains where the extra key comes from.