π¬ ryanofsky commented on pull request "test: skip interface_ipc if python version is freethreaded and PYTHON_GIL=0 is not set":
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2501459500)
In commit "test: Ignore error message give from python because of GIL" (136dfde1c2284e30ecb1dfb35520f93878ec7335)
I think it's probably be good to just write the entire warning message here instead of just the beginning of the message, so it is clear what warning this code is trying to suppress.
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2501459500)
In commit "test: Ignore error message give from python because of GIL" (136dfde1c2284e30ecb1dfb35520f93878ec7335)
I think it's probably be good to just write the entire warning message here instead of just the beginning of the message, so it is clear what warning this code is trying to suppress.
π¬ ryanofsky commented on pull request "test: skip interface_ipc if python version is freethreaded and PYTHON_GIL=0 is not set":
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2501455784)
In commit "test: Ignore error message give from python because of GIL" (136dfde1c2284e30ecb1dfb35520f93878ec7335)
Could this change to test_framework.py be reverted?
I think it should be impossible for the warning to trigger here since the capnp module will already be imported by interface_ipc.py before this runs.
If changing this method is actually necessary, I think it'd be good to move common code importing capnp and suppressing the warning to a function in test_framework/util.py th
...
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2501455784)
In commit "test: Ignore error message give from python because of GIL" (136dfde1c2284e30ecb1dfb35520f93878ec7335)
Could this change to test_framework.py be reverted?
I think it should be impossible for the warning to trigger here since the capnp module will already be imported by interface_ipc.py before this runs.
If changing this method is actually necessary, I think it'd be good to move common code importing capnp and suppressing the warning to a function in test_framework/util.py th
...
π¬ ryanofsky commented on pull request "test: skip interface_ipc if python version is freethreaded and PYTHON_GIL=0 is not set":
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2501470819)
re: https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2500471963
> I am thinking about setting `PYTHON_GIL=1` in the test runner, so that tests pick it up.
This seems like a reasonable alternative, but if we take this approach I hope we only do it selectively for the tests which need it.
I understand we may have some tests which aren't thread safe (though hopefully not too many if most tests are single-threaded) and it may be a lot easier to set `PYTHON_GIL=1` than to fix them.
...
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2501470819)
re: https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2500471963
> I am thinking about setting `PYTHON_GIL=1` in the test runner, so that tests pick it up.
This seems like a reasonable alternative, but if we take this approach I hope we only do it selectively for the tests which need it.
I understand we may have some tests which aren't thread safe (though hopefully not too many if most tests are single-threaded) and it may be a lot easier to set `PYTHON_GIL=1` than to fix them.
...
π¬ 0xB10C commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3500330037)
I load the `asmap.dat` file on by using `asmap=/absolute/path/to/asmap.dat` in my [config](https://github.com/0xB10C/peer-observer-infra-library/blob/e14fc1e29a3adbd35d5e3af73766039279fbf5c4/modules/node/node.nix#L225). Since this PR doesn't change anything for me, I'm neutral on this too.
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3500330037)
I load the `asmap.dat` file on by using `asmap=/absolute/path/to/asmap.dat` in my [config](https://github.com/0xB10C/peer-observer-infra-library/blob/e14fc1e29a3adbd35d5e3af73766039279fbf5c4/modules/node/node.nix#L225). Since this PR doesn't change anything for me, I'm neutral on this too.
β οΈ alejandrothugs9119 opened an issue: "lawl"
(https://github.com/bitcoin/bitcoin/issues/33812)
https://github.com/alejandrothugs9119/cryptoblock/tree/master
(https://github.com/bitcoin/bitcoin/issues/33812)
https://github.com/alejandrothugs9119/cryptoblock/tree/master
π Ataraxia009 opened a pull request: "Changing the rpcbind argument being ignored to a pop up warning, instβ¦"
(https://github.com/bitcoin/bitcoin/pull/33813)
When we ignore a users explicit request to an `rpcbind`, i think it warrants for a warning instead of a debug log.
(https://github.com/bitcoin/bitcoin/pull/33813)
When we ignore a users explicit request to an `rpcbind`, i think it warrants for a warning instead of a debug log.
π¬ Ataraxia009 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#issuecomment-3500930821)
> I notice that CTransaction::ToString() prints the inputs and the input script witnesses additionally, with that change script witnesses will be included duplicated. The order there is first all inputs, then all witnesses. Please consider removing the duplication.
Hey, good call out. Updated to remove duplicate. Think this makes the most sense.
(https://github.com/bitcoin/bitcoin/pull/33711#issuecomment-3500930821)
> I notice that CTransaction::ToString() prints the inputs and the input script witnesses additionally, with that change script witnesses will be included duplicated. The order there is first all inputs, then all witnesses. Please consider removing the duplication.
Hey, good call out. Updated to remove duplicate. Think this makes the most sense.
β οΈ alejandrothugs9119 opened an issue: "alex"
(https://github.com/bitcoin/bitcoin/issues/33814)
https://github.com/alejandrothugs9119/cryptoblock/blob/master/filter.txt
(https://github.com/bitcoin/bitcoin/issues/33814)
https://github.com/alejandrothugs9119/cryptoblock/blob/master/filter.txt
β
alejandrothugs9119 closed an issue: "alex"
(https://github.com/bitcoin/bitcoin/issues/33814)
(https://github.com/bitcoin/bitcoin/issues/33814)
β
willcl-ark closed an issue: "lawl"
(https://github.com/bitcoin/bitcoin/issues/33812)
(https://github.com/bitcoin/bitcoin/issues/33812)
π€ hebasto reviewed a pull request: "guix: build for Linux HOSTS with `-static-libgcc`"
(https://github.com/bitcoin/bitcoin/pull/33181#pullrequestreview-3432302524)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/33181#pullrequestreview-3432302524)
Concept ACK.
π fanquake merged a pull request: "refactor: remove dead branches in `SingletonClusterImpl`"
(https://github.com/bitcoin/bitcoin/pull/33768)
(https://github.com/bitcoin/bitcoin/pull/33768)
π¬ maflcko commented on pull request "ci: Add fast IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2502764382)
I think both CI tasks should use the same config (`debian:trixie`). Otherwise, if someone tries to reproduce the CI config, they two configs will contradict each other, which doesn't seem helpful?
Also, I find it confusing that one CI task is printing the iwyu errors and the other is printing the warnings. What is the goal here? It seems clearer to just print the errors and warnings in one task, like it was done before.
As mentioned previously, iwyu takes 9 minutes (https://github.com/bit
...
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2502764382)
I think both CI tasks should use the same config (`debian:trixie`). Otherwise, if someone tries to reproduce the CI config, they two configs will contradict each other, which doesn't seem helpful?
Also, I find it confusing that one CI task is printing the iwyu errors and the other is printing the warnings. What is the goal here? It seems clearer to just print the errors and warnings in one task, like it was done before.
As mentioned previously, iwyu takes 9 minutes (https://github.com/bit
...
π fanquake merged a pull request: "doc: add cmake help option in Windows build docs"
(https://github.com/bitcoin/bitcoin/pull/33789)
(https://github.com/bitcoin/bitcoin/pull/33789)
π¬ vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3501908260)
`ada059e714...e5e16de7b5`: do https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2501363004 and https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2493783865
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3501908260)
`ada059e714...e5e16de7b5`: do https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2501363004 and https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2493783865
π¬ vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2502844387)
Added.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2502844387)
Added.
π¬ vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2502847950)
Extended the test with the above to cover the case where the transaction is already in the mempool.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2502847950)
Extended the test with the above to cover the case where the transaction is already in the mempool.
π¬ fanquake commented on pull request "ci: Add fast IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3501942613)
> providing convenient feedback to developers.
How can we improve the quality of the feedback (https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3496969502)? Getting it faster is nice, but if it can't be taken and applied directly without fixing the sorting, and the formatting, and in the worse case, changing headers entirely for `modernize-deprecated-headers`, then it doesn't seem that much more useful.
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3501942613)
> providing convenient feedback to developers.
How can we improve the quality of the feedback (https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3496969502)? Getting it faster is nice, but if it can't be taken and applied directly without fixing the sorting, and the formatting, and in the worse case, changing headers entirely for `modernize-deprecated-headers`, then it doesn't seem that much more useful.
β
fanquake closed a pull request: "fuzz: avoid returning non-conforming results from FuzzedSock::GetSockName()"
(https://github.com/bitcoin/bitcoin/pull/32109)
(https://github.com/bitcoin/bitcoin/pull/32109)
π¬ fanquake commented on pull request "fuzz: avoid returning non-conforming results from FuzzedSock::GetSockName()":
(https://github.com/bitcoin/bitcoin/pull/32109#issuecomment-3501954989)
Closing for now, as there doesn't seem to be agreement to do this. Any discussion can continue.
(https://github.com/bitcoin/bitcoin/pull/32109#issuecomment-3501954989)
Closing for now, as there doesn't seem to be agreement to do this. Any discussion can continue.