💬 Sjors commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2448504460)
Done in https://github.com/bitcoin-core/gui/issues/843
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2448504460)
Done in https://github.com/bitcoin-core/gui/issues/843
💬 Sjors commented on pull request "test: Don't enforce BIP94 on regtest unless specified by arg":
(https://github.com/bitcoin/bitcoin/pull/31156#issuecomment-2448519062)
TIL about the `-test=<option>` hook, that's a nice solution. Post-merge concept ACK.
(https://github.com/bitcoin/bitcoin/pull/31156#issuecomment-2448519062)
TIL about the `-test=<option>` hook, that's a nice solution. Post-merge concept ACK.
✅ Sjors closed an issue: "macOS 13.7 depends build can't find qt"
(https://github.com/bitcoin/bitcoin/issues/31050)
(https://github.com/bitcoin/bitcoin/issues/31050)
💬 Sjors commented on issue "macOS 13.7 depends build can't find qt":
(https://github.com/bitcoin/bitcoin/issues/31050#issuecomment-2448527957)
That seems plausible. I'll keep an eye on that issue. Once that's resolved and there's still a macOS specific problem remaining, I'll reopen.
(https://github.com/bitcoin/bitcoin/issues/31050#issuecomment-2448527957)
That seems plausible. I'll keep an eye on that issue. Once that's resolved and there's still a macOS specific problem remaining, I'll reopen.
🤔 hodlinator reviewed a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2405166236)
> In case you do want a test with different behavior, I think you can use %n specifier which is not supported by tinyformat
Could document non-parity like so (unless you prefer I do it as part of #30933):
```C++
// Non-parity
int n{};
BOOST_CHECK_EXCEPTION(tfm::format(std::string{"%n"}, n), tfm::format_error,
HasReason{"tinyformat: %n conversion spec not supported"});
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%n");
```
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2405166236)
> In case you do want a test with different behavior, I think you can use %n specifier which is not supported by tinyformat
Could document non-parity like so (unless you prefer I do it as part of #30933):
```C++
// Non-parity
int n{};
BOOST_CHECK_EXCEPTION(tfm::format(std::string{"%n"}, n), tfm::format_error,
HasReason{"tinyformat: %n conversion spec not supported"});
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%n");
```
💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822782509)
(One could expect that at least one precision-digit was required after '.' but it is not in tinyformat so this behavior is consistent).
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822782509)
(One could expect that at least one precision-digit was required after '.' but it is not in tinyformat so this behavior is consistent).
💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822780992)
Should this be removed?
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822780992)
Should this be removed?
💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1823481424)
Why not use `FormatStringCheck<sizeof...(Args)>` in `printf` and `printfln` directly below as well?
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1823481424)
Why not use `FormatStringCheck<sizeof...(Args)>` in `printf` and `printfln` directly below as well?
💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1823488811)
Could also add
```C++
PassFmt<1>("%.f");
```
maybe grouped with the `<1>` calls above. Confirmed tinyformat allows for it, see how the success status returned by `parseWidthOrPrecision` is ignored:
https://github.com/bitcoin/bitcoin/blob/e53829d3952c6ed275507a66e77636aad67d106b/src/tinyformat.h#L778-L788
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1823488811)
Could also add
```C++
PassFmt<1>("%.f");
```
maybe grouped with the `<1>` calls above. Confirmed tinyformat allows for it, see how the success status returned by `parseWidthOrPrecision` is ignored:
https://github.com/bitcoin/bitcoin/blob/e53829d3952c6ed275507a66e77636aad67d106b/src/tinyformat.h#L778-L788
💬 github12101 commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2448576849)
> > In my main machine, is ryzen with 32gb ddr4 with 2 sticks of memory. The other machine is Xeon, with 32gb memory, in quad channel ddr3. So i dont think its memory, because the 2 computers are stable. I can run prime95 in both for 2 hours with no problem.
>
> it's true, I just ran disk diagnostics, no issues. this is a new 4tb ssd. going to run a memory diagnostic... but my system has been stable so I would be very surprised if it was that. I'm also running on ZFS. I keep hitting this, eve
...
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2448576849)
> > In my main machine, is ryzen with 32gb ddr4 with 2 sticks of memory. The other machine is Xeon, with 32gb memory, in quad channel ddr3. So i dont think its memory, because the 2 computers are stable. I can run prime95 in both for 2 hours with no problem.
>
> it's true, I just ran disk diagnostics, no issues. this is a new 4tb ssd. going to run a memory diagnostic... but my system has been stable so I would be very surprised if it was that. I'm also running on ZFS. I keep hitting this, eve
...
👍 ryanofsky approved a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2404680812)
Code review ACK 3b5b2457f713014fd5073da34b76a5a8cd796e4a. Nice change that should make the API more safe and also makes tests more complete and easier to maintain.
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2404680812)
Code review ACK 3b5b2457f713014fd5073da34b76a5a8cd796e4a. Nice change that should make the API more safe and also makes tests more complete and easier to maintain.
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822513330)
In commit "test: Migrate GetCoinsMapEntry to return MaybeCoin" (e8e70a4ae3cbd7a648629c2df4b5a7f6f0686438)
Would be better to leave this outside of the try loop so it is clearer this is not expected to throw std::logic_error, and if it it did, the test framework would catch it.
Same for CheckWriteCoins test below.
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822513330)
In commit "test: Migrate GetCoinsMapEntry to return MaybeCoin" (e8e70a4ae3cbd7a648629c2df4b5a7f6f0686438)
Would be better to leave this outside of the try loop so it is clearer this is not expected to throw std::logic_error, and if it it did, the test framework would catch it.
Same for CheckWriteCoins test below.
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822507538)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
Not very important, but this commit would be easier to understand if AddFlags function were not moving at the same time as it being modified. Specifically it is harder to see the new `Assume(flags & (DIRTY | FRESH))` change and related changes because of this.
Would suggest not moving AddFlags this commit, and instead moving it in the next commit "coins: Remove direct GetFlags access" g
...
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822507538)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
Not very important, but this commit would be easier to understand if AddFlags function were not moving at the same time as it being modified. Specifically it is harder to see the new `Assume(flags & (DIRTY | FRESH))` change and related changes because of this.
Would suggest not moving AddFlags this commit, and instead moving it in the next commit "coins: Remove direct GetFlags access" g
...
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822525401)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
For this commit and other commits, it would be helpful if there was an indication about whether they were supposed to change behavior or test coverage. Would suggest including "refactor" in the subject for commits which are not supposed to change behavior like "coins, refactor: Split up AddFlags to remove invalid states" or could just say behavior isn't changing in the commit description.
...
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822525401)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
For this commit and other commits, it would be helpful if there was an indication about whether they were supposed to change behavior or test coverage. Would suggest including "refactor" in the subject for commits which are not supposed to change behavior like "coins, refactor: Split up AddFlags to remove invalid states" or could just say behavior isn't changing in the commit description.
...
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823231744)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
Not directly related to this PR, but would it make sense for SetClean to set m_prev and m_next to null here?
It seems that would be better than keeping dangling or invalid pointers. This would also make it possible to add asserts in AddFlags to confirm that the linked list state is valid when it is called.
Not suggesting any change for this commit, just wondering if this idea makes s
...
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823231744)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
Not directly related to this PR, but would it make sense for SetClean to set m_prev and m_next to null here?
It seems that would be better than keeping dangling or invalid pointers. This would also make it possible to add asserts in AddFlags to confirm that the linked list state is valid when it is called.
Not suggesting any change for this commit, just wondering if this idea makes s
...
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823243237)
In commit "coins: Remove direct GetFlags access" (8df16f23a22b2e0b72dd6338ed37c09485f5ede3)
Not important, but usually test code and output is more readable with `CHECK(a); CHECK(b);` instead of `CHECK(a && b)`
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823243237)
In commit "coins: Remove direct GetFlags access" (8df16f23a22b2e0b72dd6338ed37c09485f5ede3)
Not important, but usually test code and output is more readable with `CHECK(a); CHECK(b);` instead of `CHECK(a && b)`
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823185560)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
Maybe wait for other opinions to see if this suggestion is worthwhile, but IMO this commit provides a good opportunity to remove the `Assume(&self.second == this);` footgun and make calling these functions more straightforward:
<details><summary>diff</summary>
<p>
```diff
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -49,7 +49,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(cons
...
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823185560)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
Maybe wait for other opinions to see if this suggestion is worthwhile, but IMO this commit provides a good opportunity to remove the `Assume(&self.second == this);` footgun and make calling these functions more straightforward:
<details><summary>diff</summary>
<p>
```diff
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -49,7 +49,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(cons
...
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823264089)
In commit "test: Migrate GetCoinsMapEntry to return MaybeCoin" (e8e70a4ae3cbd7a648629c2df4b5a7f6f0686438)
It seems like these methods are not called in this commit, but they could be called in `InsertCoinsMapEntry` below. Maybe call them there or drop them here?
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823264089)
In commit "test: Migrate GetCoinsMapEntry to return MaybeCoin" (e8e70a4ae3cbd7a648629c2df4b5a7f6f0686438)
It seems like these methods are not called in this commit, but they could be called in `InsertCoinsMapEntry` below. Maybe call them there or drop them here?
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823433791)
In commit "test: Group values and states in tests into CoinEntry wrappers" (553db5dec1334ed5e8e07e34306f16de9afc22bb)
Review note: It's not really easy to verify by looking at code that new and old test cases are equivalent, but an approach that worked for me was to add extra prints showing the test cases, and then diff the output from `test_bitcoin -t coins_tests` before and after this commit, and confirm there are were no changes. My change used to test this was:
<details><summary>diff</
...
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823433791)
In commit "test: Group values and states in tests into CoinEntry wrappers" (553db5dec1334ed5e8e07e34306f16de9afc22bb)
Review note: It's not really easy to verify by looking at code that new and old test cases are equivalent, but an approach that worked for me was to add extra prints showing the test cases, and then diff the output from `test_bitcoin -t coins_tests` before and after this commit, and confirm there are were no changes. My change used to test this was:
<details><summary>diff</
...
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823241356)
In commit "coins: Remove direct GetFlags access" (8df16f23a22b2e0b72dd6338ed37c09485f5ede3)
May should check `!IsFresh()` here so new check is equivalent to previous check. Alternately, could just mention in commit message that this commit reduces test coverage a little bit by not checking some flags that are not relevant to the tests.
Same comment applies to other tests below like `linked_list_random_deletion`
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823241356)
In commit "coins: Remove direct GetFlags access" (8df16f23a22b2e0b72dd6338ed37c09485f5ede3)
May should check `!IsFresh()` here so new check is equivalent to previous check. Alternately, could just mention in commit message that this commit reduces test coverage a little bit by not checking some flags that are not relevant to the tests.
Same comment applies to other tests below like `linked_list_random_deletion`