💬 janb84 commented on pull request "test: Log IP of download server in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608082755)
oke, thanks, please validate that it returns the correct ipv6 addresses, see my edit :) happy to be wrong
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608082755)
oke, thanks, please validate that it returns the correct ipv6 addresses, see my edit :) happy to be wrong
💬 achow101 commented on pull request "test: Log IP of download server in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608109457)
Using your suggestion, I think it will work more reliably.
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608109457)
Using your suggestion, I think it will work more reliably.
💬 ajtowns commented on pull request "logging: API improvements":
(https://github.com/bitcoin/bitcoin/pull/34038#discussion_r2608196910)
Seems like a fine idea, but not sure how you'd provide compatibility for `include=["net"]` with deprecaterpc?
(https://github.com/bitcoin/bitcoin/pull/34038#discussion_r2608196910)
Seems like a fine idea, but not sure how you'd provide compatibility for `include=["net"]` with deprecaterpc?
💬 ajtowns commented on pull request "net: Waste less time in socket handling":
(https://github.com/bitcoin/bitcoin/pull/34025#discussion_r2608213446)
> Happy to take a look as well in a fresh commit, either here, or in a follow-up.
Happy to review in a followup.
When looking at this previously, it seemed like having an `AtomicTimePoint<Clock>` template would be helpful (for things like `m_last_send` which is currently `atomic<seconds>`), because time_point doesn't support being an atomic. Here's [a branch](https://github.com/ajtowns/bitcoin/commits/202509-atomictime/) where I last looked at this topic; the "Add AtomicTimePoint" commit m
...
(https://github.com/bitcoin/bitcoin/pull/34025#discussion_r2608213446)
> Happy to take a look as well in a fresh commit, either here, or in a follow-up.
Happy to review in a followup.
When looking at this previously, it seemed like having an `AtomicTimePoint<Clock>` template would be helpful (for things like `m_last_send` which is currently `atomic<seconds>`), because time_point doesn't support being an atomic. Here's [a branch](https://github.com/ajtowns/bitcoin/commits/202509-atomictime/) where I last looked at this topic; the "Add AtomicTimePoint" commit m
...
📝 l0rinc opened a pull request: "bench: run `FindByte` across block-sized buffer"
(https://github.com/bitcoin/bitcoin/pull/34046)
The current benchmark doesn't represent a realistic scenario:
* it only checks 200 bytes, while in reality we have 8 MB: https://github.com/bitcoin/bitcoin/blob/2c44c41984e0a170cf38d595635a9e861854573c/src/validation.cpp#L5009
* it only checks the very last position (which is basically the missing case), while in reality it's a circular buffer, so the magic bytes can be anywhere
The benchmark was updated to start from different file positions and iterate with a stride of ~32kb to exercise a
...
(https://github.com/bitcoin/bitcoin/pull/34046)
The current benchmark doesn't represent a realistic scenario:
* it only checks 200 bytes, while in reality we have 8 MB: https://github.com/bitcoin/bitcoin/blob/2c44c41984e0a170cf38d595635a9e861854573c/src/validation.cpp#L5009
* it only checks the very last position (which is basically the missing case), while in reality it's a circular buffer, so the magic bytes can be anywhere
The benchmark was updated to start from different file positions and iterate with a stride of ~32kb to exercise a
...
📝 hebasto opened a pull request: "cmake: Add fail pattern to `try_append_cxx_flags`"
(https://github.com/bitcoin/bitcoin/pull/34047)
This PR addresses scenarios where the build environment, such as OSS-Fuzz, provides the `-Wno-error=unknown-warning-option` flag, which undermines our use of `-Werror` when checking compiler flags.
Noticed while working on the migration to Ubuntu 24.04 in OSS-Fuzz.
(https://github.com/bitcoin/bitcoin/pull/34047)
This PR addresses scenarios where the build environment, such as OSS-Fuzz, provides the `-Wno-error=unknown-warning-option` flag, which undermines our use of `-Werror` when checking compiler flags.
Noticed while working on the migration to Ubuntu 24.04 in OSS-Fuzz.
🤔 l0rinc reviewed a pull request: "streams: replace `std::find` with `memchr` (5x improvement)"
(https://github.com/bitcoin/bitcoin/pull/34044#pullrequestreview-3564063293)
Concept ACK
`FindByte`’s only production use seems to be scanning block files during `-reindex` to find the first byte of the network magic in a circular buffer.
Because of documented historical bugs the data may be jumbled a bit, it's why we need to be able to find the beginning.
It's not something we expect users to do often, but this should speed it up a tiny bit. Left a few suggestions to make it more readable as well to make it easier to sell :)
I would be curious whether a `-reindex -sto
...
(https://github.com/bitcoin/bitcoin/pull/34044#pullrequestreview-3564063293)
Concept ACK
`FindByte`’s only production use seems to be scanning block files during `-reindex` to find the first byte of the network magic in a circular buffer.
Because of documented historical bugs the data may be jumbled a bit, it's why we need to be able to find the beginning.
It's not something we expect users to do often, but this should speed it up a tiny bit. Left a few suggestions to make it more readable as well to make it easier to sell :)
I would be curious whether a `-reindex -sto
...
💬 l0rinc commented on pull request "streams: replace `std::find` with `memchr` (5x improvement)":
(https://github.com/bitcoin/bitcoin/pull/34044#discussion_r2607867005)
I don't think this needs manual hoisting - it's loop-invariant so the compiler will likely do it.
Inling it will reduce the diff.
And I don't think we need the ugly `to_integer` here: https://godbolt.org/z/7MWzTbMdb
(https://github.com/bitcoin/bitcoin/pull/34044#discussion_r2607867005)
I don't think this needs manual hoisting - it's loop-invariant so the compiler will likely do it.
Inling it will reduce the diff.
And I don't think we need the ugly `to_integer` here: https://godbolt.org/z/7MWzTbMdb
💬 l0rinc commented on pull request "streams: replace `std::find` with `memchr` (5x improvement)":
(https://github.com/bitcoin/bitcoin/pull/34044#discussion_r2608033086)
the old implementation is quite confusing.
`buf_offset += inc` is only executed when `inc >= len` - and given it can't be greater, it's basically:
```C++
buf_offset += len;
```
which makes a lot more sense.
----
`m_read_pos += inc;` is either incremented by the distance of the find or by `len`, so if we do the `hit` check earlier, we can simplify this as well:
```C++
const auto len{std::min<size_t>(vchBuf.size() - buf_offset, nSrcPos - m_read_pos)};
const auto* start{vchBuf.data() + buf_offse
...
(https://github.com/bitcoin/bitcoin/pull/34044#discussion_r2608033086)
the old implementation is quite confusing.
`buf_offset += inc` is only executed when `inc >= len` - and given it can't be greater, it's basically:
```C++
buf_offset += len;
```
which makes a lot more sense.
----
`m_read_pos += inc;` is either incremented by the distance of the find or by `len`, so if we do the `hit` check earlier, we can simplify this as well:
```C++
const auto len{std::min<size_t>(vchBuf.size() - buf_offset, nSrcPos - m_read_pos)};
const auto* start{vchBuf.data() + buf_offse
...
💬 w0xlt commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2608266711)
If I’m understanding the API correctly, `m_tx_for_private_broadcast.DidNodeConfirmReception(nodeid)` returns null when there’s no associated transaction even if it was previously removed (in `PrivateBroadcast::Remove()` in a8e23ca6)
If that’s the case, can private broadcast connections with already removed transactions still trigger `NumToOpenAdd(1)`, resulting in pointless connections ?
```suggestion
if (node.IsPrivateBroadcastConn()) {
// Only schedule a replacement private
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2608266711)
If I’m understanding the API correctly, `m_tx_for_private_broadcast.DidNodeConfirmReception(nodeid)` returns null when there’s no associated transaction even if it was previously removed (in `PrivateBroadcast::Remove()` in a8e23ca6)
If that’s the case, can private broadcast connections with already removed transactions still trigger `NumToOpenAdd(1)`, resulting in pointless connections ?
```suggestion
if (node.IsPrivateBroadcastConn()) {
// Only schedule a replacement private
...
💬 l0rinc commented on pull request "test: Log IP of download server in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608312657)
new import seems unnecessary now
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608312657)
new import seems unnecessary now
💬 achow101 commented on pull request "test: Log IP of download server in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608326193)
Removed
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608326193)
Removed
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608380979)
Thanks, added in https://github.com/bitcoin/bitcoin/pull/33657/commits/44758e8387e66ec49d190b59f8a8fefab84b1e69.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608380979)
Thanks, added in https://github.com/bitcoin/bitcoin/pull/33657/commits/44758e8387e66ec49d190b59f8a8fefab84b1e69.
💬 l0rinc commented on pull request "test: Log IP of download server in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608382091)
I'm find with this as well, alternatively to document what we're connecting to, we could unpack the `ip` from the `(host, port)`/`(host, port, flowinfo, scope_id)` tuples with something like:
```suggestion
ip, *_ = response.fp.raw._sock.getpeername()
print(f"Connected to {ip}")
```
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608382091)
I'm find with this as well, alternatively to document what we're connecting to, we could unpack the `ip` from the `(host, port)`/`(host, port, flowinfo, scope_id)` tuples with something like:
```suggestion
ip, *_ = response.fp.raw._sock.getpeername()
print(f"Connected to {ip}")
```
👍 l0rinc approved a pull request: "test: Log IP of download server in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/34045#pullrequestreview-3564697400)
untested ACK cdaf25f9c3e56b4e80b90a471bdc0a175160534c
(https://github.com/bitcoin/bitcoin/pull/34045#pullrequestreview-3564697400)
untested ACK cdaf25f9c3e56b4e80b90a471bdc0a175160534c
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608385566)
No worries, thanks :)
<details>
<summary> git range-diff </summary>
```
1: 1be1c165b3 ! 1: 7a2a6b398b blockstorage: return an error code from `ReadRawBlock()`
@@ src/rest.cpp: static bool rest_block(const std::any& context,
+ const auto block_data{chainman.m_blockman.ReadRawBlock(pos)};
+ if (!block_data) {
+ switch (block_data.error()) {
-+ case node::ReadRawError::IO: return RESTERR(req, HTTP_NOT_FOUND, "I/O error reading " + hashStr)
...
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608385566)
No worries, thanks :)
<details>
<summary> git range-diff </summary>
```
1: 1be1c165b3 ! 1: 7a2a6b398b blockstorage: return an error code from `ReadRawBlock()`
@@ src/rest.cpp: static bool rest_block(const std::any& context,
+ const auto block_data{chainman.m_blockman.ReadRawBlock(pos)};
+ if (!block_data) {
+ switch (block_data.error()) {
-+ case node::ReadRawError::IO: return RESTERR(req, HTTP_NOT_FOUND, "I/O error reading " + hashStr)
...
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608388301)
Unified in f03f9d7dcd929f73f7b0c7030d9822e54f1621ec
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608388301)
Unified in f03f9d7dcd929f73f7b0c7030d9822e54f1621ec
💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608396257)
If we don't have a default, the compiler will warn since we have the `-Wswitch` set - in which case I don't see why the `assert(false)` is needed.
Alternatively, if we add a `default: assert(false)`, we don't need to specify all values and the compiler won't warn, the failure will be on runtime.
Anyway, off topic, I'm fine either way.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608396257)
If we don't have a default, the compiler will warn since we have the `-Wswitch` set - in which case I don't see why the `assert(false)` is needed.
Alternatively, if we add a `default: assert(false)`, we don't need to specify all values and the compiler won't warn, the failure will be on runtime.
Anyway, off topic, I'm fine either way.
💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3639185341)
ACK 44758e8387e66ec49d190b59f8a8fefab84b1e69
Since last push a `HTTP_NOT_FOUND` error code was changed to `HTTP_INTERNAL_SERVER_ERROR`, a test `BOOST_REQUIRE` was changed to a more permissive `BOOST_CHECK`, and in functional tests an assert was adjusted and a missing-block-data case was added for the status of the response.
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3639185341)
ACK 44758e8387e66ec49d190b59f8a8fefab84b1e69
Since last push a `HTTP_NOT_FOUND` error code was changed to `HTTP_INTERNAL_SERVER_ERROR`, a test `BOOST_REQUIRE` was changed to a more permissive `BOOST_CHECK`, and in functional tests an assert was adjusted and a missing-block-data case was added for the status of the response.
💬 achow101 commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3639185350)
ACK 6eb5ba569141b722a5e27ef36e04994886769c44
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3639185350)
ACK 6eb5ba569141b722a5e27ef36e04994886769c44