👍 hodlinator approved a pull request: "doc: Improve dependencies documentation"
(https://github.com/bitcoin/bitcoin/pull/31634#pullrequestreview-2546121330)
re-ACK 1fcc1ce0edcc8ae522ab9103ad79adf58fc0fe0e
Thanks for applying my suggestions.
(https://github.com/bitcoin/bitcoin/pull/31634#pullrequestreview-2546121330)
re-ACK 1fcc1ce0edcc8ae522ab9103ad79adf58fc0fe0e
Thanks for applying my suggestions.
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586611555)
@fanquake, is the problem you reported above https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2573295321 when the tests run on the host, without a docker?
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586611555)
@fanquake, is the problem you reported above https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2573295321 when the tests run on the host, without a docker?
💬 l0rinc commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2586629442)
@Sjors, your setup is already extremely fast - it seems this optimization shines mostly on commodity hardware, which I assume is used more often.
Let's wait for other reproducers.
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2586629442)
@Sjors, your setup is already extremely fast - it seems this optimization shines mostly on commodity hardware, which I assume is used more often.
Let's wait for other reproducers.
💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2586643113)
> When I do `sudo cmake --install build` on macOS 15.2 it puts the binaries in `/usr/local/bin` and `/usr/local/lib` (not `/usr/local/libexec` or `/usr/libexec`). Is that intended?
Target output locations and their installation paths are independent properties. This PR modifies the former only. I'll look into modifying the latter accordingly.
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2586643113)
> When I do `sudo cmake --install build` on macOS 15.2 it puts the binaries in `/usr/local/bin` and `/usr/local/lib` (not `/usr/local/libexec` or `/usr/libexec`). Is that intended?
Target output locations and their installation paths are independent properties. This PR modifies the former only. I'll look into modifying the latter accordingly.
💬 fanquake commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1912951874)
Python isn't required.
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1912951874)
Python isn't required.
👍 dergoegge approved a pull request: "p2p: track and use all potential peers for orphan resolution"
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2546241855)
Code review ACK 86d7135e36efd39781cf4c969011df99f0cbb69d
Other reviewers already mentioned the nits that I would have had as well. I think they are fine to address in a follow up.
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2546241855)
Code review ACK 86d7135e36efd39781cf4c969011df99f0cbb69d
Other reviewers already mentioned the nits that I would have had as well. I think they are fine to address in a follow up.
💬 NicolaLS commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1913007533)
ah yes..thanks. should I move [Linux Kernel](https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md?plain=1#L22) to optional too then ?
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1913007533)
ah yes..thanks. should I move [Linux Kernel](https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md?plain=1#L22) to optional too then ?
💬 Sjors commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586797138)
Concept ACK
I tested this with https://github.com/Sjors/bitcoin/pull/76 on my own CI setup, which uses Cirrus workers configured using (roughly) the instructions here (using podman-docker): https://github.com/bitcoin/bitcoin/blob/master/.cirrus.yml#L8
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586797138)
Concept ACK
I tested this with https://github.com/Sjors/bitcoin/pull/76 on my own CI setup, which uses Cirrus workers configured using (roughly) the instructions here (using podman-docker): https://github.com/bitcoin/bitcoin/blob/master/.cirrus.yml#L8
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586828644)
I guess an env var is leaking into the docker. To reproduce you could try setting the URL via the env var `DEBUGINFOD_URLS=https://debuginfod.fedoraproject.org/ `, but I haven't tried it.
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586828644)
I guess an env var is leaking into the docker. To reproduce you could try setting the URL via the env var `DEBUGINFOD_URLS=https://debuginfod.fedoraproject.org/ `, but I haven't tried it.
💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#issuecomment-2586835482)
Rebased for merge conflict, and updated for @sipa's comment (thanks) to add a constant for the initial mocked clock value.
(https://github.com/bitcoin/bitcoin/pull/31022#issuecomment-2586835482)
Rebased for merge conflict, and updated for @sipa's comment (thanks) to add a constant for the initial mocked clock value.
💬 maflcko commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913034308)
For the merge conflict, isn't this missing in the next line?
```cpp
if (!mocktime.count()) {
g_used_system_time = true;
}
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913034308)
For the merge conflict, isn't this missing in the next line?
```cpp
if (!mocktime.count()) {
g_used_system_time = true;
}
💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913040379)
i first didn't because i thought it would require all tests that indirectly use the steady clock to be updated, which i deemed outside the scope of this PR.
But it actually doesn't, the new clock is (intentially) only used in one place. And besides, this is checked for the fuzz framework not the unit tests.
Will add.
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913040379)
i first didn't because i thought it would require all tests that indirectly use the steady clock to be updated, which i deemed outside the scope of this PR.
But it actually doesn't, the new clock is (intentially) only used in one place. And besides, this is checked for the fuzz framework not the unit tests.
Will add.
✅ maflcko closed a pull request: "rpc: getdescriptorinfo also returns normalized descriptor"
(https://github.com/bitcoin/bitcoin/pull/29396)
(https://github.com/bitcoin/bitcoin/pull/29396)
💬 maflcko commented on pull request "rpc: getdescriptorinfo also returns normalized descriptor":
(https://github.com/bitcoin/bitcoin/pull/29396#issuecomment-2586850491)
Closing for now. The issue remains open (https://github.com/bitcoin/bitcoin/issues/29320) and discussion can continue there. This can also be picked up again, a fresh pull can be created, or it can be re-opened.
(https://github.com/bitcoin/bitcoin/pull/29396#issuecomment-2586850491)
Closing for now. The issue remains open (https://github.com/bitcoin/bitcoin/issues/29320) and discussion can continue there. This can also be picked up again, a fresh pull can be created, or it can be re-opened.
💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2586865081)
Reworked per recent [feedback](https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2586587671).
`libexec` is an installation path for targets whose output path is set to `libexec`.
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2586865081)
Reworked per recent [feedback](https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2586587671).
`libexec` is an installation path for targets whose output path is set to `libexec`.
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586911979)
Checked locally that the following fails for me as well: `time env -i HOME="$HOME" PATH="$PATH" USER="$USER" MAKEJOBS="-j$( nproc )" FILE_ENV="./ci/test/00_setup_env_native_asan.sh" DEBUGINFOD_URLS='https://debuginfod.fedoraproject.org/' ./ci/test_run_all.sh`
In theory it is not recommended to run the tests without a clean `env`, according to the `ci/README.md`. So I am not sure if this can be left as-is, or a workaround for DEBUGINFOD_URLS is convenient .
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586911979)
Checked locally that the following fails for me as well: `time env -i HOME="$HOME" PATH="$PATH" USER="$USER" MAKEJOBS="-j$( nproc )" FILE_ENV="./ci/test/00_setup_env_native_asan.sh" DEBUGINFOD_URLS='https://debuginfod.fedoraproject.org/' ./ci/test_run_all.sh`
In theory it is not recommended to run the tests without a clean `env`, according to the `ci/README.md`. So I am not sure if this can be left as-is, or a workaround for DEBUGINFOD_URLS is convenient .
💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1913097418)
I'll do that next time I have to push.
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1913097418)
I'll do that next time I have to push.
📝 DevRatnu opened a pull request: "Update README.md"
(https://github.com/bitcoin/bitcoin/pull/31647)
Pow Similar from 2009-2010 SAme from Satoshi history
<!--
*** 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
significan
...
(https://github.com/bitcoin/bitcoin/pull/31647)
Pow Similar from 2009-2010 SAme from Satoshi history
<!--
*** 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
significan
...
📝 fanquake locked a pull request: "Update README.md"
(https://github.com/bitcoin/bitcoin/pull/31647)
Pow Similar from 2009-2010 SAme from Satoshi history
<!--
*** 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
significan
...
(https://github.com/bitcoin/bitcoin/pull/31647)
Pow Similar from 2009-2010 SAme from Satoshi history
<!--
*** 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
significan
...