💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569620435)
Added `-i2psam=` to make the I2P network reachable.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569620435)
Added `-i2psam=` to make the I2P network reachable.
🤔 fjahr reviewed a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-3515236075)
tACK be1af13426e0d4c7aa1da0eb78f15cf1fb166ba2
Tested this with my own Signet snapshot and with and without pruning.
Some of the renamings could be even more descriptive for my taste but this is a big step forward, so no need to address my nits unless you really want to. If others like them they could still be applied in a simple scripted-diff follow-up.
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-3515236075)
tACK be1af13426e0d4c7aa1da0eb78f15cf1fb166ba2
Tested this with my own Signet snapshot and with and without pruning.
Some of the renamings could be even more descriptive for my taste but this is a big step forward, so no need to address my nits unless you really want to. If others like them they could still be applied in a simple scripted-diff follow-up.
💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2568688269)
in a162863f2796cb4a5f15f2e80b8e51e7de9cfb50
nit: For me "current" isn't clearer than "active" and I'm very used to active, so I am kind of -0 on this particular name change. Maybe `MostWorkChainstate` or `TipChainstate` would make explicit what is currently written in the comment.
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2568688269)
in a162863f2796cb4a5f15f2e80b8e51e7de9cfb50
nit: For me "current" isn't clearer than "active" and I'm very used to active, so I am kind of -0 on this particular name change. Maybe `MostWorkChainstate` or `TipChainstate` would make explicit what is currently written in the comment.
💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2568652812)
in 21d638c92f56f5ad4d32c47240700b41e8868789
nit: How about `maybe_historical_target_stats` 🙈 It's not validated until later and the maybe refers to `std::optional` afaict.
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2568652812)
in 21d638c92f56f5ad4d32c47240700b41e8868789
nit: How about `maybe_historical_target_stats` 🙈 It's not validated until later and the maybe refers to `std::optional` afaict.
💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2568629658)
in 1107e04328735abe636692b1a0179c6e46613d4e
micro-nit: one of these you moved over to curly brace notation but not the others.
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2568629658)
in 1107e04328735abe636692b1a0179c6e46613d4e
micro-nit: one of these you moved over to curly brace notation but not the others.
💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2568578902)
in 0cb19b78cc297ceca591ee8be6f63b4ea7b494cc
nit: Might have been possible to get to a naming that makes these two values even more aligned. Something like `m_snapshot_base_blockhash` and `m_historical_target_blockhash`. I guess in general I would prefer if the "Target" functions would be named "HistoricalTarget" so it's clear these are assumeutxo functions. But it's also a bit too verbose for my taste. So feel free to ignore.
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2568578902)
in 0cb19b78cc297ceca591ee8be6f63b4ea7b494cc
nit: Might have been possible to get to a naming that makes these two values even more aligned. Something like `m_snapshot_base_blockhash` and `m_historical_target_blockhash`. I guess in general I would prefer if the "Target" functions would be named "HistoricalTarget" so it's clear these are assumeutxo functions. But it's also a bit too verbose for my taste. So feel free to ignore.
💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2569212831)
in 21d638c92f56f5ad4d32c47240700b41e8868789
nit: `DetectUnvalidatedChainstate` would have been more consistent with the rest of the naming choices in this commit
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2569212831)
in 21d638c92f56f5ad4d32c47240700b41e8868789
nit: `DetectUnvalidatedChainstate` would have been more consistent with the rest of the naming choices in this commit
💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2569487654)
in f85ff75dad4ce8712fe65a636cf36d57b4066785
If `RemoveChainstate` returned a `nullptr` this would crash. Maybe not a scenario that can be realistically hit currently but might still be good to handle it explicitly here or by changing `RemoveChainstate`.
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2569487654)
in f85ff75dad4ce8712fe65a636cf36d57b4066785
If `RemoveChainstate` returned a `nullptr` this would crash. Maybe not a scenario that can be realistically hit currently but might still be good to handle it explicitly here or by changing `RemoveChainstate`.
💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2569493043)
in d4cbeaff4ae82203b588c3d5b9da011e4bb644d2
nit: Could have added a comment
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2569493043)
in d4cbeaff4ae82203b588c3d5b9da011e4bb644d2
nit: Could have added a comment
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569633692)
The first link shows a benefit to using them, and that is now in our codebase.
The second and third links reference [this blog post](https://blog.aaronballman.com/2020/08/dont-use-the-likely-or-unlikely-attributes/) which has crazy convoluted usages. I don't find that blog post convincing. The usages here are straightforward and in very hot paths. They are only used in paths where we know a valid block will always or never go into (aside from BIP30).
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569633692)
The first link shows a benefit to using them, and that is now in our codebase.
The second and third links reference [this blog post](https://blog.aaronballman.com/2020/08/dont-use-the-likely-or-unlikely-attributes/) which has crazy convoluted usages. I don't find that blog post convincing. The usages here are straightforward and in very hot paths. They are only used in paths where we know a valid block will always or never go into (aside from BIP30).
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569633840)
like with the includes, having a predictable order (e.g. alphabetic) can help with merge conflicts and are just generally less opinionated. Ignore if you think it doesn't apply here.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569633840)
like with the includes, having a predictable order (e.g. alphabetic) can help with merge conflicts and are just generally less opinionated. Ignore if you think it doesn't apply here.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569644851)
Taken. `-listenonion=0` is not needed for the first test, removed.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569644851)
Taken. `-listenonion=0` is not needed for the first test, removed.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3586923032)
`50c8320f20...e0c9a75fc1`: address suggestions
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3586923032)
`50c8320f20...e0c9a75fc1`: address suggestions
💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#issuecomment-3586940490)
- Addressed some clang-format nits
- Updated the comments to use struct name instead of raw names as suggested by @stickies-v.
(https://github.com/bitcoin/bitcoin/pull/33822#issuecomment-3586940490)
- Addressed some clang-format nits
- Updated the comments to use struct name instead of raw names as suggested by @stickies-v.
💬 maflcko commented on pull request "ubsan: add another suppression for InsecureRandomContext":
(https://github.com/bitcoin/bitcoin/pull/33949#issuecomment-3586968767)
To explain my comment: On this pull and current master the compile flags are: `C++ compiler flags .................... -ftrivial-auto-var-init=pattern -O2 -g -std=c++20 ...`
On the failing push, they are: `C++ compiler flags .................... -ftrivial-auto-var-init=pattern -O2 -std=c++20 ...` (with `-g` removed)
So I fail to see why the correct fix is to add a suppression. See also the docs:
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#runtime-suppressions
> Someti
...
(https://github.com/bitcoin/bitcoin/pull/33949#issuecomment-3586968767)
To explain my comment: On this pull and current master the compile flags are: `C++ compiler flags .................... -ftrivial-auto-var-init=pattern -O2 -g -std=c++20 ...`
On the failing push, they are: `C++ compiler flags .................... -ftrivial-auto-var-init=pattern -O2 -std=c++20 ...` (with `-g` removed)
So I fail to see why the correct fix is to add a suppression. See also the docs:
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#runtime-suppressions
> Someti
...
💬 cobratbq commented on pull request "docs: sync external-signer.md with current external signer flow":
(https://github.com/bitcoin/bitcoin/pull/33947#issuecomment-3587011977)
I tried to include some of your work in #33765. Feel free to comment or collaborate.
(https://github.com/bitcoin/bitcoin/pull/33947#issuecomment-3587011977)
I tried to include some of your work in #33765. Feel free to comment or collaborate.
📝 l0rinc opened a pull request: "test: tighten `V2TransportTester::SendMessage` bounds for GCC 15.2"
(https://github.com/bitcoin/bitcoin/pull/33963)
### Summary
#33840 adjusted V2TransportTester::SendMessage to avoid a `-Warray-bounds` false positive with **GCC 13**, but **GCC 15.2** still warns.
### Fix
Implemented the suggestion in https://github.com/bitcoin/bitcoin/pull/33840#pullrequestreview-3447477940:
- take `mtype` as `std::string_view`;
- assert `mtype.size() <= CMessageHeader::MESSAGE_TYPE_SIZE`;
- build `[0x00][12-byte type][payload]` via `std::ranges::copy`.
### Reproducer
Before this change, using GCC 15.2 on a M
...
(https://github.com/bitcoin/bitcoin/pull/33963)
### Summary
#33840 adjusted V2TransportTester::SendMessage to avoid a `-Warray-bounds` false positive with **GCC 13**, but **GCC 15.2** still warns.
### Fix
Implemented the suggestion in https://github.com/bitcoin/bitcoin/pull/33840#pullrequestreview-3447477940:
- take `mtype` as `std::string_view`;
- assert `mtype.size() <= CMessageHeader::MESSAGE_TYPE_SIZE`;
- build `[0x00][12-byte type][payload]` via `std::ranges::copy`.
### Reproducer
Before this change, using GCC 15.2 on a M
...
⚠️ l0rinc opened an issue: "Startup crash on macOS with GCC 15.2: `std::source_location::file_name()` is `nullptr`"
(https://github.com/bitcoin/bitcoin/issues/33964)
### Summary
`bitcoind` crashes on startup when built with Homebrew `GCC 15.2.0 `on macOS (arm64).
The crash occurs inside `SourceLocationHasher::operator()` when constructing a `std::basic_string_view` from `std::source_location::file_name()`.
The same commit runs successfully when built with the default `AppleClang` toolchain.
### Versions
* Bitcoin Core: `master` at `808f1d972be35f4c66bdc30ab0f4060dab0c43c0`
* Platform: macOS 26.1 (25B78), Apple M4 Pro (arm64)
* CMake: `cmake version 4.2.0
...
(https://github.com/bitcoin/bitcoin/issues/33964)
### Summary
`bitcoind` crashes on startup when built with Homebrew `GCC 15.2.0 `on macOS (arm64).
The crash occurs inside `SourceLocationHasher::operator()` when constructing a `std::basic_string_view` from `std::source_location::file_name()`.
The same commit runs successfully when built with the default `AppleClang` toolchain.
### Versions
* Bitcoin Core: `master` at `808f1d972be35f4c66bdc30ab0f4060dab0c43c0`
* Platform: macOS 26.1 (25B78), Apple M4 Pro (arm64)
* CMake: `cmake version 4.2.0
...
👍 sedited approved a pull request: "kernel: Add block header support and validation"
(https://github.com/bitcoin/bitcoin/pull/33822#pullrequestreview-3516825286)
re-ACK 30367c1b3e7e94ed59d0a3ee816da14a3ee0424f
(https://github.com/bitcoin/bitcoin/pull/33822#pullrequestreview-3516825286)
re-ACK 30367c1b3e7e94ed59d0a3ee816da14a3ee0424f
💬 maflcko commented on pull request "test: tighten `V2TransportTester::SendMessage` bounds for GCC 15.2":
(https://github.com/bitcoin/bitcoin/pull/33963#issuecomment-3587196960)
I don't understand this change:
* The tighter bounds check does not fix the false positive warning: https://godbolt.org/z/bn4sn5ehx
* Using ranges fixes it, but likely only because they break the array-bounds analysis.
* There is no CI task that runs into this warning
* I can't reproduce this on GCC 15.2 on Linux: https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19735442483/job/56546112570#step:5:8
* It reproduces with GCC 16, but there are many more warnings: https://github.c
...
(https://github.com/bitcoin/bitcoin/pull/33963#issuecomment-3587196960)
I don't understand this change:
* The tighter bounds check does not fix the false positive warning: https://godbolt.org/z/bn4sn5ehx
* Using ranges fixes it, but likely only because they break the array-bounds analysis.
* There is no CI task that runs into this warning
* I can't reproduce this on GCC 15.2 on Linux: https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19735442483/job/56546112570#step:5:8
* It reproduces with GCC 16, but there are many more warnings: https://github.c
...