💬 willcl-ark commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2449385872)
> > 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-2449385872)
> > 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
...
💬 maflcko commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449435667)
Sure, it is open source and pull requests for re-wording are welcome: https://github.com/maflcko/DrahtBot/blob/main/webhook_features/src/features/summary_comment.rs#L194
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449435667)
Sure, it is open source and pull requests for re-wording are welcome: https://github.com/maflcko/DrahtBot/blob/main/webhook_features/src/features/summary_comment.rs#L194
💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824164226)
@dergoegge any thoughts on my suggestion of altering `FuzzedDataProvider` to trigger asserts when calls like `ConsumeIntegralInRange<int>(0, decoded.size() - 1)` are made after calls to `ConsumeRemainingBytes*()`? Maybe such an improvement could be upstreamed even (possibly with a flag to turn it off for sloppy tests).
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824164226)
@dergoegge any thoughts on my suggestion of altering `FuzzedDataProvider` to trigger asserts when calls like `ConsumeIntegralInRange<int>(0, decoded.size() - 1)` are made after calls to `ConsumeRemainingBytes*()`? Maybe such an improvement could be upstreamed even (possibly with a flag to turn it off for sloppy tests).
💬 maflcko commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449446364)
I wonder if an `[[unlikely]]` annotation can take care of this, so that the compiler can treat the Assume as an (almost) no-op in the happy path. Before and after the `g_fuzzing` change, `val` would have to always be evaluated. However, it seems the `!val` evaluation is the overhead in this case?
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449446364)
I wonder if an `[[unlikely]]` annotation can take care of this, so that the compiler can treat the Assume as an (almost) no-op in the happy path. Before and after the `g_fuzzing` change, `val` would have to always be evaluated. However, it seems the `!val` evaluation is the overhead in this case?
💬 maflcko commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824176187)
I'd say it would be easier to sell to upstream if it was checked at compile-time at no runtime cost/overhead. Is there a benefit of doing it at runtime?
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824176187)
I'd say it would be easier to sell to upstream if it was checked at compile-time at no runtime cost/overhead. Is there a benefit of doing it at runtime?
💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824185306)
It might add extra friction to agree on preprocessor logic to make the code still compile on other compilers. Also, doing it at compile-time means it's an all/nothing thing that can't be turned off for individual sloppy tests. But I agree it would be great to see if upstream would take a compile-time version first.
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824185306)
It might add extra friction to agree on preprocessor logic to make the code still compile on other compilers. Also, doing it at compile-time means it's an all/nothing thing that can't be turned off for individual sloppy tests. But I agree it would be great to see if upstream would take a compile-time version first.
💬 maflcko commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824197377)
> agree on preprocessor logic
Maybe I am missing something, but it should be pretty standard, similar to
https://github.com/bitcoin/bitcoin/blob/f07a533dfcb172321972e5afb3b38a4bd24edb87/src/span.h#L22-L30
?
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824197377)
> agree on preprocessor logic
Maybe I am missing something, but it should be pretty standard, similar to
https://github.com/bitcoin/bitcoin/blob/f07a533dfcb172321972e5afb3b38a4bd24edb87/src/span.h#L22-L30
?
💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824202809)
Yeah, hopefully shouldn't be much of an obstacle, maybe I'm too pessimistic about that aspect.
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1824202809)
Yeah, hopefully shouldn't be much of an obstacle, maybe I'm too pessimistic about that aspect.
💬 TheCharlatan commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2449494945)
> Makes me wonder if there is more historical context to this.
Yes, me too. If there are good reasons to keep these checks, then we should document why (and why they are not needed for the other interfaces). I could not come up with one, so I opened this PR, but I may well be missing something.
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2449494945)
> Makes me wonder if there is more historical context to this.
Yes, me too. If there are good reasons to keep these checks, then we should document why (and why they are not needed for the other interfaces). I could not come up with one, so I opened this PR, but I may well be missing something.
💬 maflcko commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2449496369)
An easy first fix would be to downgrade `InitError` to `InitWarning`, as the alert shouldn't be fatal, as it is possible to continue without it. (This applies to bitcoind as well)
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2449496369)
An easy first fix would be to downgrade `InitError` to `InitWarning`, as the alert shouldn't be fatal, as it is possible to continue without it. (This applies to bitcoind as well)
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824208275)
I agree now, it's equivalent. Still wrapping my head around `RelayTransaction` refactoring :)
---------------------------
A difference could be in handling dependencies: during the trickle window after the first transaction arrives.
Say, a parent arrives, and then a child arrives in a second. In my approach we may notice the child when handling (announcement-wise) the parent (because handling the parent is postponed via trickle).
In your approach, you handle the parent independently.
...
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824208275)
I agree now, it's equivalent. Still wrapping my head around `RelayTransaction` refactoring :)
---------------------------
A difference could be in handling dependencies: during the trickle window after the first transaction arrives.
Say, a parent arrives, and then a child arrives in a second. In my approach we may notice the child when handling (announcement-wise) the parent (because handling the parent is postponed via trickle).
In your approach, you handle the parent independently.
...
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824212857)
another question: say a child arrives a few minutes after a parent (parent still in the mempool, but it's been announced to peers a while ago). Why bother with this inv? Perhaps we should do it only if `TryRemovingFromSet` actually have removed it?
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824212857)
another question: say a child arrives a few minutes after a parent (parent still in the mempool, but it's been announced to peers a while ago). Why bother with this inv? Perhaps we should do it only if `TryRemovingFromSet` actually have removed it?
💬 maflcko commented on pull request "ci: Do not error on unused-member-function in test each commit":
(https://github.com/bitcoin/bitcoin/pull/31187#issuecomment-2449507386)
Can you add a dummy commit to trigger the CI, so that the flag difference can be seen, and to confirm that there are no unwanted interactions?
(https://github.com/bitcoin/bitcoin/pull/31187#issuecomment-2449507386)
Can you add a dummy commit to trigger the CI, so that the flag difference can be seen, and to confirm that there are no unwanted interactions?
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824219529)
Apparently I was wrong.
```
// If the transaction fails because it collides with an existing one,
// we also remove and fanout the conflict and all its descendants.
// This is because our peer may have added the conflicting transaction
// to its set, in which reconciliation of these two would fail
```
Say #1, #2, #3 arrive.
- First #1 added to the set.
- then #2 doesn't make it to the set, but also drops #1 from it.
-
...
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824219529)
Apparently I was wrong.
```
// If the transaction fails because it collides with an existing one,
// we also remove and fanout the conflict and all its descendants.
// This is because our peer may have added the conflicting transaction
// to its set, in which reconciliation of these two would fail
```
Say #1, #2, #3 arrive.
- First #1 added to the set.
- then #2 doesn't make it to the set, but also drops #1 from it.
-
...
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824221603)
yes
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824221603)
yes
👍 hodlinator approved a pull request: "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests"
(https://github.com/bitcoin/bitcoin/pull/30746#pullrequestreview-2407594484)
ACK 6fd185c035c1cc4dd961cf14a2087e97fb069440
range-diff shows all notable changes since my [Concept ACK](https://github.com/bitcoin/bitcoin/pull/30746#pullrequestreview-2397249971) are in response to my suggestions.
Ran each new fuzz target with:
```
₿ FUZZ=<target name> build_fuzz/src/test/fuzz/fuzz -max_total_time=30 -jobs=8
```
Also ran:
```
₿ ctest -j<X> --test-dir build --output-on-failure -R base
```
(https://github.com/bitcoin/bitcoin/pull/30746#pullrequestreview-2407594484)
ACK 6fd185c035c1cc4dd961cf14a2087e97fb069440
range-diff shows all notable changes since my [Concept ACK](https://github.com/bitcoin/bitcoin/pull/30746#pullrequestreview-2397249971) are in response to my suggestions.
Ran each new fuzz target with:
```
₿ FUZZ=<target name> build_fuzz/src/test/fuzz/fuzz -max_total_time=30 -jobs=8
```
Also ran:
```
₿ ctest -j<X> --test-dir build --output-on-failure -R base
```
💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2449539386)
Rebased due to the conflict with the merged bitcoin/bitcoin#31015.
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2449539386)
Rebased due to the conflict with the merged bitcoin/bitcoin#31015.
🤔 maflcko reviewed a pull request: "bench: Remove various extraneous benchmarks"
(https://github.com/bitcoin/bitcoin/pull/31153#pullrequestreview-2407602211)
No strong opinion on the removals. I think the doc update is valuable and could be extended. Simply declaring the removed benchmarks as potentially not valuable, when looked at too narrowly, could be less controversial and still achieve the essence of the goal of this pull request.
(https://github.com/bitcoin/bitcoin/pull/31153#pullrequestreview-2407602211)
No strong opinion on the removals. I think the doc update is valuable and could be extended. Simply declaring the removed benchmarks as potentially not valuable, when looked at too narrowly, could be less controversial and still achieve the essence of the goal of this pull request.
💬 maflcko commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1824235336)
Maybe add a note to say that performance optimizations of micro-benchmarks are not valuable, if they do not show a visible end-user end-to-end performance improvement? Also could mention that the contribution of benchmarks that do not cover an end-user end-to-end scenario may not be valuable?
Even if they do, I think some improvements may still be rejected, if the code bloat or review/maintenance is too much to justify the improvement.
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1824235336)
Maybe add a note to say that performance optimizations of micro-benchmarks are not valuable, if they do not show a visible end-user end-to-end performance improvement? Also could mention that the contribution of benchmarks that do not cover an end-user end-to-end scenario may not be valuable?
Even if they do, I think some improvements may still be rejected, if the code bloat or review/maintenance is too much to justify the improvement.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824243351)
I've checked once more. It fails without patching: https://cirrus-ci.com/task/4951518795792384.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824243351)
I've checked once more. It fails without patching: https://cirrus-ci.com/task/4951518795792384.