💬 laanwj commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2449234882)
Even ignoring the spurious setting would be better in the GUI case. No need to rub this in the users face. i didn't expect this consequence when i suggested adding a message 😅
Though it's still better than creashing with generic "-upnp: Unknown setting". i guess...
But yes this needs to be informational level at most, it should just continue.
Even worse: i guess it even happens if the settings json specifies upnp=0 to explicitly disable it?
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2449234882)
Even ignoring the spurious setting would be better in the GUI case. No need to rub this in the users face. i didn't expect this consequence when i suggested adding a message 😅
Though it's still better than creashing with generic "-upnp: Unknown setting". i guess...
But yes this needs to be informational level at most, it should just continue.
Even worse: i guess it even happens if the settings json specifies upnp=0 to explicitly disable it?
📝 maflcko converted_to_draft a pull request: "ci: Place datadirs for tests under tmpfs "
(https://github.com/bitcoin/bitcoin/pull/31182)
This should speed up the tests a bit (see comment below for results).
(https://github.com/bitcoin/bitcoin/pull/31182)
This should speed up the tests a bit (see comment below for results).
💬 maflcko commented on pull request "ci: Place datadirs for tests under tmpfs ":
(https://github.com/bitcoin/bitcoin/pull/31182#issuecomment-2449255282)
> > (see comment below for results).
>
> What is this referring to?
I was still running the commit (and master) twice each for all CI tasks, but I couldn't find any difference at all.
I checked that the folder is correctly picked up and used by the tests, so I guess modern storage is fast enough to not make a difference? Alternatively, there is an issue I was missing.
It would be good if someone else double checked the code and whether there is a performance difference at all.
For
...
(https://github.com/bitcoin/bitcoin/pull/31182#issuecomment-2449255282)
> > (see comment below for results).
>
> What is this referring to?
I was still running the commit (and master) twice each for all CI tasks, but I couldn't find any difference at all.
I checked that the folder is correctly picked up and used by the tests, so I guess modern storage is fast enough to not make a difference? Alternatively, there is an issue I was missing.
It would be good if someone else double checked the code and whether there is a performance difference at all.
For
...
💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2449259598)
> > ramdisk
>
> Benchmarking this is a bit more involved and I think requires changes to the CI scripts, so that the ramdisk is (1) mounted into the CI container and (2) picked up as the temp dir. I can take a look in the future, unless someone beats me to it.
Done in https://github.com/bitcoin/bitcoin/pull/31182, but I couldn't find any difference at all so far, which is a bit surprising.
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2449259598)
> > ramdisk
>
> Benchmarking this is a bit more involved and I think requires changes to the CI scripts, so that the ramdisk is (1) mounted into the CI container and (2) picked up as the temp dir. I can take a look in the future, unless someone beats me to it.
Done in https://github.com/bitcoin/bitcoin/pull/31182, but I couldn't find any difference at all so far, which is a bit surprising.
💬 l0rinc commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449260674)
Could we obviate in the [DrahtBot's response](https://github.com/bitcoin/bitcoin/pull/31093#issuecomment-2413637472) that the link doesn't just provide a `test coverage report` - since the regression was already visible there: https://corecheck.dev/bitcoin/bitcoin/pulls/31093
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449260674)
Could we obviate in the [DrahtBot's response](https://github.com/bitcoin/bitcoin/pull/31093#issuecomment-2413637472) that the link doesn't just provide a `test coverage report` - since the regression was already visible there: https://corecheck.dev/bitcoin/bitcoin/pulls/31093
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824060041)
I was looking for something like "As a general rule, we keep the transaction that appeared first".
Currently, the `AddToSet` documentation seems outdated—it basically claims that the only way to fail is to reach a set limit.
How about the following replacement:
```
/**
* Step 1. Add a to-be-announced transaction to the local reconciliation set of the target peer.
* Returns false if the set is at capacity, or if the set contains a colliding transaction.
* Returns t
...
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824060041)
I was looking for something like "As a general rule, we keep the transaction that appeared first".
Currently, the `AddToSet` documentation seems outdated—it basically claims that the only way to fail is to reach a set limit.
How about the following replacement:
```
/**
* Step 1. Add a to-be-announced transaction to the local reconciliation set of the target peer.
* Returns false if the set is at capacity, or if the set contains a colliding transaction.
* Returns t
...
💬 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.
-
...