💬 sipa commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1766668776)
Done in #30927.
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1766668776)
Done in #30927.
💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1766676838)
Good point and good catch. This should be trivial to fix by using a macro approach to concatenate the two format strings. (In C++ `"a" "b"` is equivalent to `"ab"`). A macro approach was my initial implementation when I wrote this in July, but the benefit wasn't clear, so I switched to this. Basically,
* Breaking this requires an intentionally malformed format string, such as `%n` or `%*s`, which may or may not be caught during review.
* The wallet logging is unconditional, so this requires
...
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1766676838)
Good point and good catch. This should be trivial to fix by using a macro approach to concatenate the two format strings. (In C++ `"a" "b"` is equivalent to `"ab"`). A macro approach was my initial implementation when I wrote this in July, but the benefit wasn't clear, so I switched to this. Basically,
* Breaking this requires an intentionally malformed format string, such as `%n` or `%*s`, which may or may not be caught during review.
* The wallet logging is unconditional, so this requires
...
💬 maflcko commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766685876)
unrelated style-nit (feel free to ignore): I think this "paragraph" isn't part of the "Stream subset". It would be good to either remove the "Stream subset" comment above, or move the "paragraph" (Commit and Truncate) a few lines up.
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766685876)
unrelated style-nit (feel free to ignore): I think this "paragraph" isn't part of the "Stream subset". It would be good to either remove the "Stream subset" comment above, or move the "paragraph" (Commit and Truncate) a few lines up.
👍 maflcko approved a pull request: "Follow-up after AutoFile position caching: remove unused code"
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315289563)
lgtm ACK 67a3d590768301fb46a93fdb0a5c66c0c2de1082
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315289563)
lgtm ACK 67a3d590768301fb46a93fdb0a5c66c0c2de1082
💬 sipa commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766702454)
Done, in a separate commit. I have also added comments to all the affected functions.
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766702454)
Done, in a separate commit. I have also added comments to all the affected functions.
💬 maflcko commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#issuecomment-2360791249)
re-ACK caac06f784c5d94c6a5f7d0b586f9ddbbe55c369
Only change is fixup up the doxygen dev docs
(https://github.com/bitcoin/bitcoin/pull/30927#issuecomment-2360791249)
re-ACK caac06f784c5d94c6a5f7d0b586f9ddbbe55c369
Only change is fixup up the doxygen dev docs
💬 mcelrath commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2360944838)
Why cannot these results be consistent? Isn't that the purpose of a mutex on the Chainstate? It's not clear to me why there "obviously can't" be multi-threaded consistency on the chain state. The (apparent) order of operations here is:
1. Chain tip received
2. ZMQ broadcast
3. RPC call getblocktemplate
If 3 were reversed with 1 or 2 I would agree with the "can't" statement, but that doesn't seem to be what's happening.
It seems to me that the ZMQ message is being sent while that mutex i
...
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2360944838)
Why cannot these results be consistent? Isn't that the purpose of a mutex on the Chainstate? It's not clear to me why there "obviously can't" be multi-threaded consistency on the chain state. The (apparent) order of operations here is:
1. Chain tip received
2. ZMQ broadcast
3. RPC call getblocktemplate
If 3 were reversed with 1 or 2 I would agree with the "can't" statement, but that doesn't seem to be what's happening.
It seems to me that the ZMQ message is being sent while that mutex i
...
🤔 l0rinc requested changes to a pull request: "Follow-up after AutoFile position caching: remove unused code"
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315477617)
Concept ACK
Could be the right time to do some minor cleanup (return unsigned) and maybe to cover missing method and exceptions with tests.
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315477617)
Concept ACK
Could be the right time to do some minor cleanup (return unsigned) and maybe to cover missing method and exceptions with tests.
💬 l0rinc commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766813742)
isn't this obvious from the implementation?
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766813742)
isn't this obvious from the implementation?
💬 l0rinc commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766797789)
Does it still make sense for `AutoFile::tell()` to return a `int64_t` instead of a `uint64_t` or `unsigned int`?
Now that we've removed the invalid usages, maybe it's a good opportunity to transition `AutoFile` (and maybe `FlatFilePos`) to `std::optional<uint64_t> m_position;`
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766797789)
Does it still make sense for `AutoFile::tell()` to return a `int64_t` instead of a `uint64_t` or `unsigned int`?
Now that we've removed the invalid usages, maybe it's a good opportunity to transition `AutoFile` (and maybe `FlatFilePos`) to `std::optional<uint64_t> m_position;`
💬 l0rinc commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766838829)
Slightly unrelated, but since we're modifying its usage here: `streams_tests` doesn't seem to cover this method at all.
(+ Commit, Truncate and many `std::ios_base::failure` throws seem also uncovered)
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766838829)
Slightly unrelated, but since we're modifying its usage here: `streams_tests` doesn't seem to cover this method at all.
(+ Commit, Truncate and many `std::ios_base::failure` throws seem also uncovered)
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1766854879)
Ah, missed the line where it was being set to `diff`.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1766854879)
Ah, missed the line where it was being set to `diff`.
👍 instagibbs approved a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2315608829)
ACK aab53ddcd8fcbc3c0be0da9383f8e06abe5badda
reviewed via `git range-diff master 4476915bc76b48eff9b5e67fd6bd7647cc12793f aab53ddcd8fcbc3c0be0da9383f8e06abe5badda`
Fuzz coverage seems good now, with additional documentation/asserts where requested. I didn't dive deep into the new un-serializer logic.
Did you figure out the slowdown? I didn't reproduce locally.
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2315608829)
ACK aab53ddcd8fcbc3c0be0da9383f8e06abe5badda
reviewed via `git range-diff master 4476915bc76b48eff9b5e67fd6bd7647cc12793f aab53ddcd8fcbc3c0be0da9383f8e06abe5badda`
Fuzz coverage seems good now, with additional documentation/asserts where requested. I didn't dive deep into the new un-serializer logic.
Did you figure out the slowdown? I didn't reproduce locally.
💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2361045807)
utACK d59f5a31a1b888051eb47e2f8a5fb8d901de1d10
I previously tested this with https://github.com/Sjors/bitcoin/pull/48, will try to do that again.
Would be nice to get a unified `concept Serializable`, see above.
For the most part it should be quite straightforward for other developers in the future to change `src/ipc/capnp/mining.capnp` when something changes in the Mining interface. I've been doing that myself.
The only part that seems (slightly) more involved is in 00a82f45004a887c
...
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2361045807)
utACK d59f5a31a1b888051eb47e2f8a5fb8d901de1d10
I previously tested this with https://github.com/Sjors/bitcoin/pull/48, will try to do that again.
Would be nice to get a unified `concept Serializable`, see above.
For the most part it should be quite straightforward for other developers in the future to change `src/ipc/capnp/mining.capnp` when something changes in the Mining interface. I've been doing that myself.
The only part that seems (slightly) more involved is in 00a82f45004a887c
...
💬 sipa commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766881296)
I think this is a bit of a philosophical question.
Both `int64_t` and `uint64_t` have sufficient range to represent the data here (because `ftell` returns a `long`, which matches `int64_t` on all our supported platforms, plus we know the result cannot be negative), so both are perfectly functional.
The advantage of `uint64_t` (and presumably the reason you're suggesting this) is because it signals that the number cannot be negative.
On the other hand, signed types have the advantage tha
...
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766881296)
I think this is a bit of a philosophical question.
Both `int64_t` and `uint64_t` have sufficient range to represent the data here (because `ftell` returns a `long`, which matches `int64_t` on all our supported platforms, plus we know the result cannot be negative), so both are perfectly functional.
The advantage of `uint64_t` (and presumably the reason you're suggesting this) is because it signals that the number cannot be negative.
On the other hand, signed types have the advantage tha
...
💬 sipa commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766882491)
Someone shouldn't need to look at the implementation to understand what a function does.
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766882491)
Someone shouldn't need to look at the implementation to understand what a function does.
💬 l0rinc commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766898995)
> The advantage of uint64_t (and presumably the reason you're suggesting this) is because it signals that the number cannot be negative.
Yes, to make it stricter and closer aligned to the problem domain.
> signed types have the advantage that they actually model integers
But here we're modelling a size where potential negative values are confusing (especially since we're storing it as unsigned int going forward).
The negative values were already handled, it seems to me that they lost t
...
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766898995)
> The advantage of uint64_t (and presumably the reason you're suggesting this) is because it signals that the number cannot be negative.
Yes, to make it stricter and closer aligned to the problem domain.
> signed types have the advantage that they actually model integers
But here we're modelling a size where potential negative values are confusing (especially since we're storing it as unsigned int going forward).
The negative values were already handled, it seems to me that they lost t
...
💬 sipa commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766913104)
@l0rinc All the same arguments apply to the size of spans, which cannot be negative, and yet people strongly (and unsuccessfully, but only barely) argued that it should have a signed size type. I don't believe the concern is about specific worries around overflow and its behavior; it is about the fact that sizes shouldn't be thought of as modular arithmetic, and that C++ has no type to model natural numbers - the best approximation is signed types which model integers.
And again, I also see t
...
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766913104)
@l0rinc All the same arguments apply to the size of spans, which cannot be negative, and yet people strongly (and unsuccessfully, but only barely) argued that it should have a signed size type. I don't believe the concern is about specific worries around overflow and its behavior; it is about the fact that sizes shouldn't be thought of as modular arithmetic, and that C++ has no type to model natural numbers - the best approximation is signed types which model integers.
And again, I also see t
...
💬 l0rinc commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766913861)
I'm not exactly sure in which cases this comment would help us avoid having to check the sources - but I don't have strong opinions about it, please resolve this comment.
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766913861)
I'm not exactly sure in which cases this comment would help us avoid having to check the sources - but I don't have strong opinions about it, please resolve this comment.
💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2361124220)
Added a CMake build flag `-DWITH_SV2` that's `OFF` by default.
Also rebased and addressed comments.
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2361124220)
Added a CMake build flag `-DWITH_SV2` that's `OFF` by default.
Also rebased and addressed comments.