π OGbencox opened a pull request: "Readme change"
(https://github.com/bitcoin/bitcoin/pull/33811)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/33811)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
π¬ w0xlt commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2501363004)
Perhaps a debug-level log here indicating poor quality addresses ?
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2501363004)
Perhaps a debug-level log here indicating poor quality addresses ?
π¬ ryanofsky commented on issue "RFC: Add multiprocess fuzz target":
(https://github.com/bitcoin/bitcoin/issues/23015#issuecomment-3500144388)
Thanks! This looks really great and I am already dreading the crashes it may uncover, especially since I didn't fix the [first one](https://github.com/bitcoin/bitcoin/issues/33341#issuecomment-3266653805) yet.
I was looking at what I think is the main implementation file: [ipc_mining.rs](https://github.com/marcofleon/fuzzamoto/blob/ipc-mining/fuzzamoto-scenarios/bin/ipc_mining.rs) and it appears pretty neat and fairly easy to extend and maintain.
I am wondering what next steps may be needed to
...
(https://github.com/bitcoin/bitcoin/issues/23015#issuecomment-3500144388)
Thanks! This looks really great and I am already dreading the crashes it may uncover, especially since I didn't fix the [first one](https://github.com/bitcoin/bitcoin/issues/33341#issuecomment-3266653805) yet.
I was looking at what I think is the main implementation file: [ipc_mining.rs](https://github.com/marcofleon/fuzzamoto/blob/ipc-mining/fuzzamoto-scenarios/bin/ipc_mining.rs) and it appears pretty neat and fairly easy to extend and maintain.
I am wondering what next steps may be needed to
...
π€ ryanofsky reviewed a 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#pullrequestreview-3431290651)
Code review 136dfde1c2284e30ecb1dfb35520f93878ec7335. I think this is a good approach, and that suppressing the warning is the most direct fix for the test failure that avoids reducing test coverage. I left some suggestions below for avoiding code duplication and documenting the workaround better. Also think the PR title and description could be updated since the test is no longer skipped.
(https://github.com/bitcoin/bitcoin/pull/33795#pullrequestreview-3431290651)
Code review 136dfde1c2284e30ecb1dfb35520f93878ec7335. I think this is a good approach, and that suppressing the warning is the most direct fix for the test failure that avoids reducing test coverage. I left some suggestions below for avoiding code duplication and documenting the workaround better. Also think the PR title and description could be updated since the test is no longer skipped.
π¬ 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.