🤔 stickies-v reviewed a pull request: "log: Use ConstevalFormatString"
(https://github.com/bitcoin/bitcoin/pull/30889#pullrequestreview-2315195560)
Approach ACK and code LGTM facbcd4cef8890ae18976fb53b67ea56b3c04454 modulo a `tinyformat::format_error` concern.
Very elegant way to force more compile-time format string checks with minimal code overhaul, I really like this.
(https://github.com/bitcoin/bitcoin/pull/30889#pullrequestreview-2315195560)
Approach ACK and code LGTM facbcd4cef8890ae18976fb53b67ea56b3c04454 modulo a `tinyformat::format_error` concern.
Very elegant way to force more compile-time format string checks with minimal code overhaul, I really like this.
💬 stickies-v commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1766633746)
By not letting `LogInfo` do the formatting, there's no `tinyformat::format_error` error handling anymore. This makes the code less robust to the malformed format strings we do not catch at compile time. For example, with this diff:
```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 83e96adf07..e6a2576f0a 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3822,7 +3822,7 @@ void CWallet::LoadActiveScriptPubKeyMan(uint256 id, OutputType type, bool inter
...
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1766633746)
By not letting `LogInfo` do the formatting, there's no `tinyformat::format_error` error handling anymore. This makes the code less robust to the malformed format strings we do not catch at compile time. For example, with this diff:
```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 83e96adf07..e6a2576f0a 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3822,7 +3822,7 @@ void CWallet::LoadActiveScriptPubKeyMan(uint256 id, OutputType type, bool inter
...
🚀 fanquake merged a pull request: "log: Use ConstevalFormatString"
(https://github.com/bitcoin/bitcoin/pull/30889)
(https://github.com/bitcoin/bitcoin/pull/30889)
🤔 maflcko reviewed a pull request: "streams: cache file position within AutoFile"
(https://github.com/bitcoin/bitcoin/pull/30884#pullrequestreview-2315004429)
review ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35 🍽
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK a240e150e837
...
(https://github.com/bitcoin/bitcoin/pull/30884#pullrequestreview-2315004429)
review ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35 🍽
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK a240e150e837
...
💬 maflcko commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1766522550)
Same
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1766522550)
Same
💬 maflcko commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1766522257)
Isn't this dead code now? Previously it was required, because `std::ftell` could fail and return `-1L`.
However, now that `AutoFile::tell()` is used, which does not return an error value (but throws on error), this can't happen.
A negative value here could only happen if seeking prior to the start of a file would be possible, which I don't think it is. Also, the AutoFile constructor already assumes this isn't possible.
In any case, the prior line writes to the file, which isn't possible
...
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1766522257)
Isn't this dead code now? Previously it was required, because `std::ftell` could fail and return `-1L`.
However, now that `AutoFile::tell()` is used, which does not return an error value (but throws on error), this can't happen.
A negative value here could only happen if seeking prior to the start of a file would be possible, which I don't think it is. Also, the AutoFile constructor already assumes this isn't possible.
In any case, the prior line writes to the file, which isn't possible
...
✅ fanquake closed a pull request: "test: fix RPC coverage check"
(https://github.com/bitcoin/bitcoin/pull/29387)
(https://github.com/bitcoin/bitcoin/pull/29387)
👍 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.