π¬ kevkevinpal commented on pull request "ci: check if file or directory already exists for ${HOME}/.bitcoin instead of failing":
(https://github.com/bitcoin/bitcoin/pull/33486#issuecomment-3347489891)
> I don't think it is supported or even encouraged to run this internal CI script on an outside machine.
Yea I understand this, I was just playing around with the scripts to understand better how the CI works
> Also, I fail to see how this could even work, as you did not include any steps how to reproduce.
Yes, I updated the description. Basically, just create a directory in `$HOME/.bitcoin` and then try to run `./ci/test/03_test_script.sh` and you should see the error.
For me it is
...
(https://github.com/bitcoin/bitcoin/pull/33486#issuecomment-3347489891)
> I don't think it is supported or even encouraged to run this internal CI script on an outside machine.
Yea I understand this, I was just playing around with the scripts to understand better how the CI works
> Also, I fail to see how this could even work, as you did not include any steps how to reproduce.
Yes, I updated the description. Basically, just create a directory in `$HOME/.bitcoin` and then try to run `./ci/test/03_test_script.sh` and you should see the error.
For me it is
...
π¬ kevkevinpal commented on pull request "ci: check if file or directory already exists for ${HOME}/.bitcoin instead of failing":
(https://github.com/bitcoin/bitcoin/pull/33486#issuecomment-3347493339)
closing as it doesn't fix any currently existing issue
(https://github.com/bitcoin/bitcoin/pull/33486#issuecomment-3347493339)
closing as it doesn't fix any currently existing issue
β
kevkevinpal closed a pull request: "ci: check if file or directory already exists for ${HOME}/.bitcoin instead of failing"
(https://github.com/bitcoin/bitcoin/pull/33486)
(https://github.com/bitcoin/bitcoin/pull/33486)
π¬ plebhash commented on issue "`bitcoin-node` is unkillable after mining IPC connection is established":
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3347520481)
@ryanofsky I don't really used `gdb` nor `lldb` on a regular basis, so I apologize if I did anything wrong. I hope the logs below are giving you the right info.
Here's the sequence of steps I took:
- do `lldb -- ./build/bin/bitcoin-node -testnet4 -ipcbind=unix`
- inside `lldb` terminal, do `process handle -p true -s false -n false SIGINT` so that the `SIGINT` goes to the process and not to `lldb`
- inside `lldb` terminal, do `run`
- launch my (now outdated) Rust code that does `waitTipChanged`
...
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3347520481)
@ryanofsky I don't really used `gdb` nor `lldb` on a regular basis, so I apologize if I did anything wrong. I hope the logs below are giving you the right info.
Here's the sequence of steps I took:
- do `lldb -- ./build/bin/bitcoin-node -testnet4 -ipcbind=unix`
- inside `lldb` terminal, do `process handle -p true -s false -n false SIGINT` so that the `SIGINT` goes to the process and not to `lldb`
- inside `lldb` terminal, do `run`
- launch my (now outdated) Rust code that does `waitTipChanged`
...
π stickies-v approved a pull request: "ci: Drop support for EOL macOS 13"
(https://github.com/bitcoin/bitcoin/pull/33489#pullrequestreview-3280376156)
ACK fab9ddaf554837624fa8f388e046a30d5bf7626f
macOS 13 is EOL, and the minimum Xcode version that ships the documented minimum clang-16 is Xcode 16.
(https://github.com/bitcoin/bitcoin/pull/33489#pullrequestreview-3280376156)
ACK fab9ddaf554837624fa8f388e046a30d5bf7626f
macOS 13 is EOL, and the minimum Xcode version that ships the documented minimum clang-16 is Xcode 16.
π¬ stickies-v commented on pull request "ci: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2388333312)
For my understanding: because Xcode 15 supports macOS 14 this change is unrelated to dropping macOS 13 support, and instead could/should have been part of #30263 since Xcode 15 doesn't satisfy our minimum clang version anymore, right? If that's correct, it looks like there's lingering usage of Xcode 15 that can be bumped up, but orthogonal to this PR.
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2388333312)
For my understanding: because Xcode 15 supports macOS 14 this change is unrelated to dropping macOS 13 support, and instead could/should have been part of #30263 since Xcode 15 doesn't satisfy our minimum clang version anymore, right? If that's correct, it looks like there's lingering usage of Xcode 15 that can be bumped up, but orthogonal to this PR.
π stickies-v approved a pull request: "ci: use latest versions of lint deps"
(https://github.com/bitcoin/bitcoin/pull/33487#pullrequestreview-3280415193)
utACK aeab2c7f1d151beda3eff94fb4effc85a4d4a344
(https://github.com/bitcoin/bitcoin/pull/33487#pullrequestreview-3280415193)
utACK aeab2c7f1d151beda3eff94fb4effc85a4d4a344
π naiyoma opened a pull request: "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value"
(https://github.com/bitcoin/bitcoin/pull/33498)
TLDR: When a node is connected to multiple networks (e.g., clearnet and Tor), it keeps separate ADDR response caches for each network. These are refreshed about once per day (randomized between 21 and 27 hours). See β[ https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L3519](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L3519)
This is an example of a dual-homed nodeβs addrman response, with separate caches:
_IPv4 response:_
```
{102.130.242.11:8333 : 1741100851}
{119
...
(https://github.com/bitcoin/bitcoin/pull/33498)
TLDR: When a node is connected to multiple networks (e.g., clearnet and Tor), it keeps separate ADDR response caches for each network. These are refreshed about once per day (randomized between 21 and 27 hours). See β[ https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L3519](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L3519)
This is an example of a dual-homed nodeβs addrman response, with separate caches:
_IPv4 response:_
```
{102.130.242.11:8333 : 1741100851}
{119
...
π ryanofsky approved a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3280437895)
Code review ACK 24391ed7804a724a62034b21150c89e45ac9b625. Just rebased after #33230
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3280437895)
Code review ACK 24391ed7804a724a62034b21150c89e45ac9b625. Just rebased after #33230
π¬ ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2388373555)
In commit "rpc: Handle -named argument parsing where '=' character is used" (04e78595410c557c970eb3994a40586231d5b38e)
This new paragraph is nice, but I think it would make sense to move it earlier in the description so JSON parameters (which are more important to list) are covered first, and STRING parameters (which are less important to list) are covered second). This should also be clearer than talking about JSON parameters, then string parameters, then JSON parameters again. Would suggest
...
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2388373555)
In commit "rpc: Handle -named argument parsing where '=' character is used" (04e78595410c557c970eb3994a40586231d5b38e)
This new paragraph is nice, but I think it would make sense to move it earlier in the description so JSON parameters (which are more important to list) are covered first, and STRING parameters (which are less important to list) are covered second). This should also be clearer than talking about JSON parameters, then string parameters, then JSON parameters again. Would suggest
...
π¬ ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2388389106)
In commit "test: Add functional tests for named argument parsing" (24391ed7804a724a62034b21150c89e45ac9b625)
It seems like this is getting pretty repetitive. Maybe just match once with a `(STRING|JSON_OR_STRING|JSON)` pattern?
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2388389106)
In commit "test: Add functional tests for named argument parsing" (24391ed7804a724a62034b21150c89e45ac9b625)
It seems like this is getting pretty repetitive. Maybe just match once with a `(STRING|JSON_OR_STRING|JSON)` pattern?
π¬ maflcko commented on pull request "ci: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2388397349)
Heh, that's a bit confusing, as the Apple versioning here is horrible. Xcode 15.0 ships with llvm 16.0 but the clang version string is 15.0 (https://en.wikipedia.org/wiki/Xcode#Toolchain_versions)
Xcode 16 ships with llvm 17.0 and clang 16.0.
However, I don't know if the Apple clang version string corresponds to the vanilla Clang version scheme.
In any case, you may be right that the workaround may be possible to drop:
```cpp
src/test/fuzz/fuzz.cpp:78: const auto [it, ins]{FuzzTa
...
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2388397349)
Heh, that's a bit confusing, as the Apple versioning here is horrible. Xcode 15.0 ships with llvm 16.0 but the clang version string is 15.0 (https://en.wikipedia.org/wiki/Xcode#Toolchain_versions)
Xcode 16 ships with llvm 17.0 and clang 16.0.
However, I don't know if the Apple clang version string corresponds to the vanilla Clang version scheme.
In any case, you may be right that the workaround may be possible to drop:
```cpp
src/test/fuzz/fuzz.cpp:78: const auto [it, ins]{FuzzTa
...
π¬ fanquake commented on pull request "ci: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#issuecomment-3347675885)
> This does not affect cross-compilation to macOS. If there is reason or need to change that, it can be done in a separate pull.
The minimum supported version in the release notes should only be changed with the along with changing the minimum supported version in depends.
(https://github.com/bitcoin/bitcoin/pull/33489#issuecomment-3347675885)
> This does not affect cross-compilation to macOS. If there is reason or need to change that, it can be done in a separate pull.
The minimum supported version in the release notes should only be changed with the along with changing the minimum supported version in depends.
π¬ maflcko commented on pull request "ci: check if file or directory already exists for ${HOME}/.bitcoin instead of failing":
(https://github.com/bitcoin/bitcoin/pull/33486#issuecomment-3347684298)
> For me it is just weird to see the script failing here since creating a file or dir shouldn't fail
It actually should fail. The goal of the check is to verify no mainnet datadir access is happening from any of the tests. Maybe the comment could be clarified to mention this?
(Disabling this check would be better by just removing it)
(https://github.com/bitcoin/bitcoin/pull/33486#issuecomment-3347684298)
> For me it is just weird to see the script failing here since creating a file or dir shouldn't fail
It actually should fail. The goal of the check is to verify no mainnet datadir access is happening from any of the tests. Maybe the comment could be clarified to mention this?
(Disabling this check would be better by just removing it)
π¬ fjahr commented on pull request "rpc: Fix dumptxoutset rollback with competing forks":
(https://github.com/bitcoin/bitcoin/pull/33444#issuecomment-3347787910)
> Couldn't this just be a contrib script or client-side bitcoin-cli feature?
FWIW, the rollback part used to be a contrib script and I integrated it into `dumptxoutset` for better robustness and easier testing in #29553. But the idea of implementing it client-side in bitcoin-cli was never discussed as far as I can remember.
(https://github.com/bitcoin/bitcoin/pull/33444#issuecomment-3347787910)
> Couldn't this just be a contrib script or client-side bitcoin-cli feature?
FWIW, the rollback part used to be a contrib script and I integrated it into `dumptxoutset` for better robustness and easier testing in #29553. But the idea of implementing it client-side in bitcoin-cli was never discussed as far as I can remember.
π MateuszBratyslawski opened a pull request: "Create code"
(https://github.com/bitcoin/bitcoin/pull/33499)
<!--
*** 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/33499)
<!--
*** 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
...
π¬ MateuszBratyslawski commented on pull request "Create code":
(https://github.com/bitcoin/bitcoin/pull/33499#issuecomment-3347795916)
not sure about this one
(https://github.com/bitcoin/bitcoin/pull/33499#issuecomment-3347795916)
not sure about this one
π¬ Aa777263100 commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2388491359)
fab9ddaf554837624fa8f388e046a30d5bf7626f
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2388491359)
fab9ddaf554837624fa8f388e046a30d5bf7626f
π€ l0rinc reviewed a pull request: "validation: fetch block inputs on parallel threads >10% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-2831320761)
I have re-reviewed the changes again lightly and did quite a few benchmarks on different platforms.
There were a lot of surprises, see my measurements:
<details>
<summary>rpi5-16 IBD from local node or & reindex-chainstate seem is ~27% faster</summary>
```
COMMITS="688c03597afb0b76077f1ffc4608eef19481056e af8a366bd6a08d9362e69a89b0b89b5c94eb63ca"; \
STOP=915961; DBCACHE=450; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
...
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-2831320761)
I have re-reviewed the changes again lightly and did quite a few benchmarks on different platforms.
There were a lot of surprises, see my measurements:
<details>
<summary>rpi5-16 IBD from local node or & reindex-chainstate seem is ~27% faster</summary>
```
COMMITS="688c03597afb0b76077f1ffc4608eef19481056e af8a366bd6a08d9362e69a89b0b89b5c94eb63ca"; \
STOP=915961; DBCACHE=450; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
...
π¬ l0rinc commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2083465171)
We should let the objects do their job instead of querying their internals and doing it ourselves ("tell, don't ask"):
```suggestion
m_chainman.FetchInputs(CoinsTip(), CoinsDB(), blockConnecting);
```
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2083465171)
We should let the objects do their job instead of querying their internals and doing it ourselves ("tell, don't ask"):
```suggestion
m_chainman.FetchInputs(CoinsTip(), CoinsDB(), blockConnecting);
```