👍 ryanofsky approved a pull request: "fix: handle invalid `-rpcbind` port earlier"
(https://github.com/bitcoin/bitcoin/pull/30679#pullrequestreview-2315230133)
Code review ACK e6994efe08b282dd9e46602bcbad69567fe91dcd
No changes since last review, just rebased due to conflict
(https://github.com/bitcoin/bitcoin/pull/30679#pullrequestreview-2315230133)
Code review ACK e6994efe08b282dd9e46602bcbad69567fe91dcd
No changes since last review, just rebased due to conflict
💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1766639028)
974e041b794072395e41701cd7a71ebdcc362e48: `serialize.h` already defines a `concept Unserializable`, introduced by @maflcko in #29056.
Though the concept is defined is defined differently there, and either version creates a compiler error when used in both places.
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1766639028)
974e041b794072395e41701cd7a71ebdcc362e48: `serialize.h` already defines a `concept Unserializable`, introduced by @maflcko in #29056.
Though the concept is defined is defined differently there, and either version creates a compiler error when used in both places.
📝 sipa opened a pull request: "Follow-up after AutoFile position caching: remove unused code"
(https://github.com/bitcoin/bitcoin/pull/30927)
This is a follow-up to #30884.
Remove a number of dead code paths.
(https://github.com/bitcoin/bitcoin/pull/30927)
This is a follow-up to #30884.
Remove a number of dead code paths.
💬 sipa commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1766668993)
Done in #30927.
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1766668993)
Done in #30927.
💬 sipa commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1766669674)
Done in #30927.
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1766669674)
Done in #30927.
💬 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
...