💬 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.
💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1766927347)
Taken
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1766927347)
Taken
👍 theStack approved a pull request: "Follow-up after AutoFile position caching: remove unused code"
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315710653)
Code-review ACK caac06f784c5d94c6a5f7d0b586f9ddbbe55c369
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315710653)
Code-review ACK caac06f784c5d94c6a5f7d0b586f9ddbbe55c369
💬 l0rinc commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766933675)
> Mixing signed and unsigned numbers is a common source of confusion and bugs. Most coding guidelines recommend against mixing them.
But we *are* mixing them, that's my main concern
```C++
pos.nPos = (unsigned int)fileOutPos
```
I'd be fine with using a signed position in both cases, but if one is signed and the other is not, most of us will assume that we have to check the signed value for negatives (as the existence of this PR demonstrates).
But changing the usages is outside the sco
...
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766933675)
> Mixing signed and unsigned numbers is a common source of confusion and bugs. Most coding guidelines recommend against mixing them.
But we *are* mixing them, that's my main concern
```C++
pos.nPos = (unsigned int)fileOutPos
```
I'd be fine with using a signed position in both cases, but if one is signed and the other is not, most of us will assume that we have to check the signed value for negatives (as the existence of this PR demonstrates).
But changing the usages is outside the sco
...
👍 l0rinc approved a pull request: "Follow-up after AutoFile position caching: remove unused code"
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315718035)
ACK caac06f784c5d94c6a5f7d0b586f9ddbbe55c369
My only remaining concern is lack of related test coverage, but it's not a blocker
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315718035)
ACK caac06f784c5d94c6a5f7d0b586f9ddbbe55c369
My only remaining concern is lack of related test coverage, but it's not a blocker