💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608002092)
Could add a test for this:
```diff
--- a/test/functional/interface_rest.py
+++ b/test/functional/interface_rest.py
@@ -9,6 +9,7 @@ from enum import Enum
from io import BytesIO
import http.client
import json
+import os
import typing
import urllib.parse
@@ -84,7 +85,7 @@ class RESTTest (BitcoinTestFramework):
conn.request('POST', rest_uri, body)
resp = conn.getresponse()
- assert_equal(resp.status, status)
+ assert resp.status == status, f"Expected: {st
...
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608002092)
Could add a test for this:
```diff
--- a/test/functional/interface_rest.py
+++ b/test/functional/interface_rest.py
@@ -9,6 +9,7 @@ from enum import Enum
from io import BytesIO
import http.client
import json
+import os
import typing
import urllib.parse
@@ -84,7 +85,7 @@ class RESTTest (BitcoinTestFramework):
conn.request('POST', rest_uri, body)
resp = conn.getresponse()
- assert_equal(resp.status, status)
+ assert resp.status == status, f"Expected: {st
...
💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607254720)
nit: Sorry for not suggesting earlier, this could possibly be more appropriate:
```suggestion
case node::ReadRawError::IO: return RESTERR(req, HTTP_INTERNAL_SERVER_ERROR, "I/O error reading " + hashStr);
```
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607254720)
nit: Sorry for not suggesting earlier, this could possibly be more appropriate:
```suggestion
case node::ReadRawError::IO: return RESTERR(req, HTTP_INTERNAL_SERVER_ERROR, "I/O error reading " + hashStr);
```
💬 davidgumberg commented on pull request "doc: guix: Troubleshooting zdiff3 issue and uninstalling.":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2608030842)
I'm not sure which is the "canonical" one, but git.guix.gnu.org redirects to codeberg and that is what I actually see in the error message:
```
Updating channel 'guix' from Git repository at 'https://codeberg.org/guix/guix.git'...
guix time-machine: error: Git error: unknown style 'zdiff3' given for 'merge.conflictstyle'
```
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2608030842)
I'm not sure which is the "canonical" one, but git.guix.gnu.org redirects to codeberg and that is what I actually see in the error message:
```
Updating channel 'guix' from Git repository at 'https://codeberg.org/guix/guix.git'...
guix time-machine: error: Git error: unknown style 'zdiff3' given for 'merge.conflictstyle'
```
💬 w0xlt commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2608045683)
Purely cosmetic nit: The error message could be shorter.
```suggestion
return InitError(_("-privatebroadcast is incompatible with -connect because it must open random Tor/I2P connections. Use -maxconnections=0 and -addnode=... instead."
```
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2608045683)
Purely cosmetic nit: The error message could be shorter.
```suggestion
return InitError(_("-privatebroadcast is incompatible with -connect because it must open random Tor/I2P connections. Use -maxconnections=0 and -addnode=... instead."
```
💬 davidgumberg commented on pull request "doc: guix: Troubleshooting zdiff3 issue and uninstalling.":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2608052261)
Thanks for catching this, I've updated the error message to what is actually printed. I think fixing the rest of the savannah->codeberg links could be done in a follow-up that also changes the `guix time-machine --url` in `/contribib/guix/libexec/prelude.bash`
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2608052261)
Thanks for catching this, I've updated the error message to what is actually printed. I think fixing the rest of the savannah->codeberg links could be done in a follow-up that also changes the `guix time-machine --url` in `/contribib/guix/libexec/prelude.bash`
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2608055960)
I think that is unnecessary, and unlikely to happen.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2608055960)
I think that is unnecessary, and unlikely to happen.
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2608056860)
Fixed
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2608056860)
Fixed
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2608058579)
Done
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2608058579)
Done
💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3638795504)
I've squashed in the new explainer comment, plus a number of other changes:
* Split out loading of an existing linearization into a separate commit.
* Split out adding support for reading non-topological inputs to `ReadLinearization` into a separate commit.
* Various small improvements to benchmarks, tests, comments.
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3638795504)
I've squashed in the new explainer comment, plus a number of other changes:
* Split out loading of an existing linearization into a separate commit.
* Split out adding support for reading non-topological inputs to `ReadLinearization` into a separate commit.
* Various small improvements to benchmarks, tests, comments.
💬 achow101 commented on pull request "test: Log IP of download server in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608077343)
No, it can be IPv6. But since we aren't ever going to actually use this socket, `AF_INET` doesn't matter. The logging still works correctly with IPv6.
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2608077343)
No, it can be IPv6. But since we aren't ever going to actually use this socket, `AF_INET` doesn't matter. The logging still works correctly with IPv6.
💬 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
...