💬 maflcko commented on issue "test: pycapnp doesn't support free threaded Python":
(https://github.com/bitcoin/bitcoin/issues/33582#issuecomment-3386064377)
> [@maflcko](https://github.com/maflcko) My understanding is that from the perspective of the interpreter Python code, GIL or no GIL is largely unobservable, apart from a few minor things. How do you think it would affect our test framework?
Python is not thread-safe by design and there are no thread-safety annotations. Historically, this didn't matter much, because the GIL in the majority of cases will (by accident or by design?) add thread-safety. The tests may or may not pass without the GIL
...
(https://github.com/bitcoin/bitcoin/issues/33582#issuecomment-3386064377)
> [@maflcko](https://github.com/maflcko) My understanding is that from the perspective of the interpreter Python code, GIL or no GIL is largely unobservable, apart from a few minor things. How do you think it would affect our test framework?
Python is not thread-safe by design and there are no thread-safety annotations. Historically, this didn't matter much, because the GIL in the majority of cases will (by accident or by design?) add thread-safety. The tests may or may not pass without the GIL
...
💬 sipa commented on issue "test: pycapnp doesn't support free threaded Python":
(https://github.com/bitcoin/bitcoin/issues/33582#issuecomment-3386147387)
@maflcko Thanks - of course.
(https://github.com/bitcoin/bitcoin/issues/33582#issuecomment-3386147387)
@maflcko Thanks - of course.
💬 darosior commented on pull request "refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator.{h,cpp}`":
(https://github.com/bitcoin/bitcoin/pull/33218#issuecomment-3386163528)
nit: if it's already in a `fees/` subfolder i think naming them `estimator.{h,cpp}` would be as clear and thrice as short?
(https://github.com/bitcoin/bitcoin/pull/33218#issuecomment-3386163528)
nit: if it's already in a `fees/` subfolder i think naming them `estimator.{h,cpp}` would be as clear and thrice as short?
💬 TheQuantumPhysicist commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3386177191)
Besides the tribal us vs them. Mempool unification vs relay policy.
I will say this because it's necessary.
### **Possible legal consequences for bitcoin core devs:**
Please understand that many blockchain projects tried to absolve themselves from what their users have done in court (Samourai wallet, for example, which pled guilty for what their users did). That doesn't work. A judge in the criminal case investigation will ask "why didn't this happen before", and then the judge will learn t
...
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3386177191)
Besides the tribal us vs them. Mempool unification vs relay policy.
I will say this because it's necessary.
### **Possible legal consequences for bitcoin core devs:**
Please understand that many blockchain projects tried to absolve themselves from what their users have done in court (Samourai wallet, for example, which pled guilty for what their users did). That doesn't work. A judge in the criminal case investigation will ask "why didn't this happen before", and then the judge will learn t
...
💬 weerden-io commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3386179293)
Hello Core,
I’ve never contributed to or interacted directly with the Bitcoin codebase or developer community, but I’ve been following this discussion closely from the sidelines. From that perspective, I think changing the OP_RETURN limit is too controversial and risky. This parameter is part of Bitcoin’s base consensus rules, and altering it introduces unnecessary uncertainty.
Bitcoin has matured to a point where predictability/stability is *far more* valuable than new flexibility. Expanding
...
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3386179293)
Hello Core,
I’ve never contributed to or interacted directly with the Bitcoin codebase or developer community, but I’ve been following this discussion closely from the sidelines. From that perspective, I think changing the OP_RETURN limit is too controversial and risky. This parameter is part of Bitcoin’s base consensus rules, and altering it introduces unnecessary uncertainty.
Bitcoin has matured to a point where predictability/stability is *far more* valuable than new flexibility. Expanding
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2417058639)
Good catch! I missed this, because I have the library installed system-wide, so never ran into a failure.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2417058639)
Good catch! I missed this, because I have the library installed system-wide, so never ran into a failure.
💬 glozow commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3386227576)
> It's also been pointed out that this has also (effectively) broken the Guix build on the release branches.
I've been testing for 29.2 and hitting this problem. After the old tarball was restored yesterday I was able to do a fresh guix build, but it's now failing again (the fukuchi.org one doesn't exist and the fallback depends-source has re-pulled the new tarball with a different hash, as described above).
```
Fetching qrencode-4.1.1.tar.gz from https://fukuchi.org/works/qrencode/
%
...
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3386227576)
> It's also been pointed out that this has also (effectively) broken the Guix build on the release branches.
I've been testing for 29.2 and hitting this problem. After the old tarball was restored yesterday I was able to do a fresh guix build, but it's now failing again (the fukuchi.org one doesn't exist and the fallback depends-source has re-pulled the new tarball with a different hash, as described above).
```
Fetching qrencode-4.1.1.tar.gz from https://fukuchi.org/works/qrencode/
%
...
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417108154)
I think I've mostly used "idx" in function arguments, and "index" in member/local variable names, but it's probably not entirely consistent.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417108154)
I think I've mostly used "idx" in function arguments, and "index" in member/local variable names, but it's probably not entirely consistent.
🤔 janb84 reviewed a pull request: "refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator.{h,cpp}`"
(https://github.com/bitcoin/bitcoin/pull/33218#pullrequestreview-3319461020)
ACK 8a83178ba27ce9bb21a22fd013bf7bc25df8cc3e
Would drop c70781d4241cb8b30fe33e7205868382031b4670 (or create separate PR). That commit add's new code, the rest of the PR (except the comment of maflcko) is just a refactor (as per PR title)
The motivation of the refactor seems logical and an improvement in the light of the new code. Even if the new code does not get merged, the new folder structure does not seems to be bad.
(https://github.com/bitcoin/bitcoin/pull/33218#pullrequestreview-3319461020)
ACK 8a83178ba27ce9bb21a22fd013bf7bc25df8cc3e
Would drop c70781d4241cb8b30fe33e7205868382031b4670 (or create separate PR). That commit add's new code, the rest of the PR (except the comment of maflcko) is just a refactor (as per PR title)
The motivation of the refactor seems logical and an improvement in the light of the new code. Even if the new code does not get merged, the new folder structure does not seems to be bad.
💬 fjahr commented on pull request "doc: how to update a subtree":
(https://github.com/bitcoin/bitcoin/pull/33568#issuecomment-3386233368)
ACk a1226bc760c70a22ef4a197d5690aca4d83cb74c
(https://github.com/bitcoin/bitcoin/pull/33568#issuecomment-3386233368)
ACk a1226bc760c70a22ef4a197d5690aca4d83cb74c
💬 m3dwards commented on pull request "Revert "depends: Update URL for `qrencode` package source tarball"":
(https://github.com/bitcoin/bitcoin/pull/33577#issuecomment-3386220619)
Revert looks good to me.
> We will need to manually reinstate the original tarball
Just checking someone has the original tarball? Otherwise I'm assuming this revert will not work.
> Could you explain the need for [46135d9](https://github.com/bitcoin/bitcoin/commit/46135d90ea9002e273f2a75283444afd080b81b1) as well? Since its justification was supporting behavior that we really don't want to be supported, is it necessary now that we're reverting that behavior?
My understanding wasn't
...
(https://github.com/bitcoin/bitcoin/pull/33577#issuecomment-3386220619)
Revert looks good to me.
> We will need to manually reinstate the original tarball
Just checking someone has the original tarball? Otherwise I'm assuming this revert will not work.
> Could you explain the need for [46135d9](https://github.com/bitcoin/bitcoin/commit/46135d90ea9002e273f2a75283444afd080b81b1) as well? Since its justification was supporting behavior that we really don't want to be supported, is it necessary now that we're reverting that behavior?
My understanding wasn't
...
💬 willcl-ark commented on pull request "ci: upgrade GitHub Action to download-artifact@v5":
(https://github.com/bitcoin/bitcoin/pull/33584#issuecomment-3386231802)
ACK b35341b9ba63a0108596e56e9eecc851a4558d98
I don't see the reason to upgrade this action (nor actions/github-script@v6) as the changes in both don't concern our usage of the actions, but neither upgrade seems harmful either.
(https://github.com/bitcoin/bitcoin/pull/33584#issuecomment-3386231802)
ACK b35341b9ba63a0108596e56e9eecc851a4558d98
I don't see the reason to upgrade this action (nor actions/github-script@v6) as the changes in both don't concern our usage of the actions, but neither upgrade seems harmful either.
💬 theStack commented on pull request "test: add functional test for `TestShell` (matching doc example)":
(https://github.com/bitcoin/bitcoin/pull/33546#discussion_r2417135650)
Thanks, done.
(https://github.com/bitcoin/bitcoin/pull/33546#discussion_r2417135650)
Thanks, done.
💬 glozow commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3386227576)
> It's also been pointed out that this has also (effectively) broken the Guix build on the release branches.
I've been testing for 29.2 and hitting this problem. After the old tarball was restored yesterday I was able to do a fresh guix build, but it's now failing again (the fukuchi.org one doesn't exist and the fallback depends-source has re-pulled the new tarball with a different hash, as described above).
```
Fetching qrencode-4.1.1.tar.gz from https://fukuchi.org/works/qrencode/
%
...
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3386227576)
> It's also been pointed out that this has also (effectively) broken the Guix build on the release branches.
I've been testing for 29.2 and hitting this problem. After the old tarball was restored yesterday I was able to do a fresh guix build, but it's now failing again (the fukuchi.org one doesn't exist and the fallback depends-source has re-pulled the new tarball with a different hash, as described above).
```
Fetching qrencode-4.1.1.tar.gz from https://fukuchi.org/works/qrencode/
%
...
💬 glozow commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2417108700)
The transaction isn't really scheduled; its broadcast time hasn't been decided. Apologies for the pedantry but given this PR aims to rename something for clarity, I point out that all other `schedule`s in net_processing involve deciding on the time at which something will happen.
My suggestion: here, we `QueueAnnouncement`. Later, depending on what's in the queue and what else we receive, might inv it. "Announce" is more specific than "Broadcast" for tx relay.
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2417108700)
The transaction isn't really scheduled; its broadcast time hasn't been decided. Apologies for the pedantry but given this PR aims to rename something for clarity, I point out that all other `schedule`s in net_processing involve deciding on the time at which something will happen.
My suggestion: here, we `QueueAnnouncement`. Later, depending on what's in the queue and what else we receive, might inv it. "Announce" is more specific than "Broadcast" for tx relay.
🚀 fanquake merged a pull request: "ci: upgrade GitHub Action to download-artifact@v5"
(https://github.com/bitcoin/bitcoin/pull/33584)
(https://github.com/bitcoin/bitcoin/pull/33584)
🚀 fanquake merged a pull request: "ci: Properly include $FILE_ENV in DEPENDS_HASH"
(https://github.com/bitcoin/bitcoin/pull/33581)
(https://github.com/bitcoin/bitcoin/pull/33581)
📝 purpleKarrot opened a pull request: "cmake: Use builtin support for .manifest files"
(https://github.com/bitcoin/bitcoin/pull/33585)
Remove some redundant logic from the CMake code:
* The `WIN32_EXECUTABLE` target property only has an effect when building for `WIN32`. Checking `WIN32` is redundant.
* CMake has builtin support for `.rc` and `.manifest` files. Both may be added to sources unconditionally. They only have an effect when building for `WIN32`.
(https://github.com/bitcoin/bitcoin/pull/33585)
Remove some redundant logic from the CMake code:
* The `WIN32_EXECUTABLE` target property only has an effect when building for `WIN32`. Checking `WIN32` is redundant.
* CMake has builtin support for `.rc` and `.manifest` files. Both may be added to sources unconditionally. They only have an effect when building for `WIN32`.
💬 plebhash commented on issue "RFC: Cancelling waitNext calls in the IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3386438916)
the current implementation has two concurrent loops:
- one that monitors for incoming Sv2 messages
- one that monitors `waitNext` (which is restarted upon arrival of `CoinbaseOutputConstraints`)
I guess in theory they could be unified into a single one, however `interruptWait` would have to be called whenever **some** incoming Sv2 message arrived, not only if that was a `CoinbaseOutputConstraints`.
that's because under a single unified loop, the futures for incoming messages now would be "comp
...
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3386438916)
the current implementation has two concurrent loops:
- one that monitors for incoming Sv2 messages
- one that monitors `waitNext` (which is restarted upon arrival of `CoinbaseOutputConstraints`)
I guess in theory they could be unified into a single one, however `interruptWait` would have to be called whenever **some** incoming Sv2 message arrived, not only if that was a `CoinbaseOutputConstraints`.
that's because under a single unified loop, the futures for incoming messages now would be "comp
...
💬 stringintech commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2417231848)
Since the crash I explained wasn't observed by others, I doubted whether it was actually coming from multiple child processes running simultaneously or from missed data dir cleanup. So I added a bunch of logs (including pid and parent pid in each) to investigate this further and found this after reading the logs for multiple runs:
I observed that AFL++ ran one worker process at a time, not multiple workers in parallel. Each worker created a static cached directory once, then ran multiple fuzz
...
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2417231848)
Since the crash I explained wasn't observed by others, I doubted whether it was actually coming from multiple child processes running simultaneously or from missed data dir cleanup. So I added a bunch of logs (including pid and parent pid in each) to investigate this further and found this after reading the logs for multiple runs:
I observed that AFL++ ran one worker process at a time, not multiple workers in parallel. Each worker created a static cached directory once, then ran multiple fuzz
...