💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020907384)
The current CI persistent workers, or a local CI run, or a local depends build, doesn't invalidate the depends cache on any change in any file. If this could lead to problems, I'd say it should be fixed in depends itself. Otherwise, almost every place where depends is called, will have to apply the workaround here.
But this is just a nit here.
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020907384)
The current CI persistent workers, or a local CI run, or a local depends build, doesn't invalidate the depends cache on any change in any file. If this could lead to problems, I'd say it should be fixed in depends itself. Otherwise, almost every place where depends is called, will have to apply the workaround here.
But this is just a nit here.
🤔 hodlinator reviewed a pull request: "fuzz: Make partially_downloaded_block more deterministic"
(https://github.com/bitcoin/bitcoin/pull/32158#pullrequestreview-2728650579)
Concept ACK fa1e2995d9996641e79f92549e99a6b37e36d140
(https://github.com/bitcoin/bitcoin/pull/32158#pullrequestreview-2728650579)
Concept ACK fa1e2995d9996641e79f92549e99a6b37e36d140
💬 hodlinator commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020666474)
nit: `[]`-brackets are commonly used for optional arguments.
```suggestion
Usage: program ./build_dir ./qa-assets/fuzz_corpora fuzz_target_name [parallelism={DEFAULT_PAR}]
```
Might be nice to standardize on `-jN`, but keeping the logic simple is also good.
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020666474)
nit: `[]`-brackets are commonly used for optional arguments.
```suggestion
Usage: program ./build_dir ./qa-assets/fuzz_corpora fuzz_target_name [parallelism={DEFAULT_PAR}]
```
Might be nice to standardize on `-jN`, but keeping the logic simple is also good.
💬 hodlinator commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020707730)
I'm worried this means we get decreased fuzz-coverage. Maybe could introduce another var?
```suggestion
.worker_threads_num = G_FUZZING && G_DETERMINISTIC? 0 : 2,
```
Although being able to reproduce fuzz failures is also nice. :\
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020707730)
I'm worried this means we get decreased fuzz-coverage. Maybe could introduce another var?
```suggestion
.worker_threads_num = G_FUZZING && G_DETERMINISTIC? 0 : 2,
```
Although being able to reproduce fuzz failures is also nice. :\
💬 hodlinator commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020914099)
Appreciate you adding the "[N/M]" output, but it's spammed away by fuzz-output even on success. Suggest capturing stdio/stderr and only output them on failure: ea3e89e48250013ea818abcefd7be8e72d31f23d
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020914099)
Appreciate you adding the "[N/M]" output, but it's spammed away by fuzz-output even on success. Suggest capturing stdio/stderr and only output them on failure: ea3e89e48250013ea818abcefd7be8e72d31f23d
💬 hodlinator commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020911351)
Found a way to reduce this code by 14 lines. What do you think about f8a0a32280d7636135f6821401d7f3b18d10476b?
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020911351)
Found a way to reduce this code by 14 lines. What do you think about f8a0a32280d7636135f6821401d7f3b18d10476b?
💬 maflcko commented on pull request "doc/descriptors.md: Update next halvening heights":
(https://github.com/bitcoin/bitcoin/pull/32168#discussion_r2020919177)
Not sure it is appropriate to have to update this doc for every halving, also the descriptor uses tpub, which is for test networks? So the halving heights are unclear anyway. Finally, the checksum doesn't match, so this will fail regardless.
(https://github.com/bitcoin/bitcoin/pull/32168#discussion_r2020919177)
Not sure it is appropriate to have to update this doc for every halving, also the descriptor uses tpub, which is for test networks? So the halving heights are unclear anyway. Finally, the checksum doesn't match, so this will fail regardless.
💬 hodlinator commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020926999)
Would be happy to be listed as commit co-author if you take this or the output spam reduction (https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020914099) as this is partially inspired by https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2001916136.
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020926999)
Would be happy to be listed as commit co-author if you take this or the output spam reduction (https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020914099) as this is partially inspired by https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2001916136.
💬 instagibbs commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2020930001)
can you give a concrete suggestion with a diff?
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2020930001)
can you give a concrete suggestion with a diff?
💬 instagibbs commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2020933005)
tried being a little more explicit, but I'm punting larger refactors for Future Work
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2020933005)
tried being a little more explicit, but I'm punting larger refactors for Future Work
💬 ajtowns commented on issue "RPC: `getblockstats` might not return the *effective* percentile fee rate":
(https://github.com/bitcoin/bitcoin/issues/31140#issuecomment-2766061375)
> However, the fee rate of a transaction at a given percentile may not reflect its _effective_ fee rate, as it could be fee-bumped by some descendant transactions.
The block's txs are put into `feerate_array` in order, but `CalculatePercentilesByWeight` sorts them by feerate, so if you have a block with txs at fee rates `[5, 4, 3, 100, 2, 2, 1, 1, 1]` it will be treated as `[100, 5, 4, 3, 2, 2, 1, 1, 1]`, so even if the "100" is a child paying for the "5" and "4" txs, it will come first when ca
...
(https://github.com/bitcoin/bitcoin/issues/31140#issuecomment-2766061375)
> However, the fee rate of a transaction at a given percentile may not reflect its _effective_ fee rate, as it could be fee-bumped by some descendant transactions.
The block's txs are put into `feerate_array` in order, but `CalculatePercentilesByWeight` sorts them by feerate, so if you have a block with txs at fee rates `[5, 4, 3, 100, 2, 2, 1, 1, 1]` it will be treated as `[100, 5, 4, 3, 2, 2, 1, 1, 1]`, so even if the "100" is a child paying for the "5" and "4" txs, it will come first when ca
...
💬 instagibbs commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2766074114)
@ajtowns Bit of a tomato/tomato situation imo, because I think the rules as-is invite more trouble than clearly marking the exact case we're worried about.
The strongest case against this PR, imo, is not some application-level danger about discontinuity but the fact that once you open it up it's difficult to close. But it's been many years and no one has made an argument why this would be dangerous at a network level.
> OTOH, it might be plausible to just not worry about small-tx's at all,
...
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2766074114)
@ajtowns Bit of a tomato/tomato situation imo, because I think the rules as-is invite more trouble than clearly marking the exact case we're worried about.
The strongest case against this PR, imo, is not some application-level danger about discontinuity but the fact that once you open it up it's difficult to close. But it's been many years and no one has made an argument why this would be dangerous at a network level.
> OTOH, it might be plausible to just not worry about small-tx's at all,
...
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2766097507)
> > I do wonder how much our limited GH cache may suffer by adding the new 'Linux->Windows cross, no tests' job? I think this takes us to 8 GHA jobs now, although not all have a cache-save step.
>
> This is probably the last task (or second-to-last task) that can be moved to GHA without degrading cache performance.
Here are the most recently used caches on the master branch:
| Cache | Size |
|---|---|
| macos-native-arm64-standard-ccache | 470 MB |
| macos-native-arm64-fuzz-ccache | 47
...
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2766097507)
> > I do wonder how much our limited GH cache may suffer by adding the new 'Linux->Windows cross, no tests' job? I think this takes us to 8 GHA jobs now, although not all have a cache-save step.
>
> This is probably the last task (or second-to-last task) that can be moved to GHA without degrading cache performance.
Here are the most recently used caches on the master branch:
| Cache | Size |
|---|---|
| macos-native-arm64-standard-ccache | 470 MB |
| macos-native-arm64-fuzz-ccache | 47
...
🤔 Sjors reviewed a pull request: "net, pcp: handle multi-part responses and filter for default route while querying default gateway"
(https://github.com/bitcoin/bitcoin/pull/32159#pullrequestreview-2729086589)
Although the code this PR touches isn't compiled on macOS, I did briefly check that things still work there. I also briefly tested on Ubuntu 24.10.
Left some inline question to wrap my head around the changes and refresh my memory of the original...
(https://github.com/bitcoin/bitcoin/pull/32159#pullrequestreview-2729086589)
Although the code this PR touches isn't compiled on macOS, I did briefly check that things still work there. I also briefly tested on Ubuntu 24.10.
Left some inline question to wrap my head around the changes and refresh my memory of the original...
💬 Sjors commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020926628)
Since you're touching this line... According to the internet, we should also check `EWOULDBLOCK` even though it's usually the same as `EAGAIN`, and it's likely not relevant for any system we support.
https://stackoverflow.com/a/49421517
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020926628)
Since you're touching this line... According to the internet, we should also check `EWOULDBLOCK` even though it's usually the same as `EAGAIN`, and it's likely not relevant for any system we support.
https://stackoverflow.com/a/49421517
💬 Sjors commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020940099)
I know this is existing code, but I don't recall why there's no timeout here. And also, should there be a quick wait between `Recv` calls?
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020940099)
I know this is existing code, but I don't recall why there's no timeout here. And also, should there be a quick wait between `Recv` calls?
💬 Sjors commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020953743)
> checking of the sequence number on each message to ensure it was meant for our request
This seems like a good idea to at least do in debug builds.
It seems like a good precaution to check for the presence of `NLM_F_MULTI` and don't wait for `NLMSG_DONE` if it isn't. At least from my naive reading of https://man7.org/linux/man-pages/man7/netlink.7.html it seems `NLMSG_DONE` is only used for multipart messages.
Splitting into multiple commits would be useful, e.g. one commit that switch
...
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2020953743)
> checking of the sequence number on each message to ensure it was meant for our request
This seems like a good idea to at least do in debug builds.
It seems like a good precaution to check for the presence of `NLM_F_MULTI` and don't wait for `NLMSG_DONE` if it isn't. At least from my naive reading of https://man7.org/linux/man-pages/man7/netlink.7.html it seems `NLMSG_DONE` is only used for multipart messages.
Splitting into multiple commits would be useful, e.g. one commit that switch
...
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020970786)
I agree that the depends caching strategy could be optimized in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020970786)
I agree that the depends caching strategy could be optimized in a follow-up.
💬 maflcko commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020971496)
Thx, will fixup, if I have to re-touch
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020971496)
Thx, will fixup, if I have to re-touch
💬 maflcko commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020971596)
Not sure. If someone wants to fuzz test the script worker threads, a dedicated fuzz target seems ideal (See `./src/test/fuzz/checkqueue.cpp`). Relying on unrelated fuzz targets to achieve coverage here seems brittle at best, because those targets may change at any time, dropping the coverage silently. Also, I am not aware of any meaningful coverage here in any fuzz target that would be more than what the unit tests and functional tests already achieve.
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020971596)
Not sure. If someone wants to fuzz test the script worker threads, a dedicated fuzz target seems ideal (See `./src/test/fuzz/checkqueue.cpp`). Relying on unrelated fuzz targets to achieve coverage here seems brittle at best, because those targets may change at any time, dropping the coverage silently. Also, I am not aware of any meaningful coverage here in any fuzz target that would be more than what the unit tests and functional tests already achieve.
💬 maflcko commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020971674)
I don't think your suggestion works. In Rust, the `?` operator is the early-return operator. This means, if you use it in a while loop that joins threads, it may return early and leave some threads dangling.
This is not much of an issue here, as `thread::scope` takes care of joining them, once they go out of scope. However, the main thread may now sometimes return an Err, or panic, when a child panics. This seems confusing. Also, if in the future all errors want to be collected and returned,
...
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020971674)
I don't think your suggestion works. In Rust, the `?` operator is the early-return operator. This means, if you use it in a while loop that joins threads, it may return early and leave some threads dangling.
This is not much of an issue here, as `thread::scope` takes care of joining them, once they go out of scope. However, the main thread may now sometimes return an Err, or panic, when a child panics. This seems confusing. Also, if in the future all errors want to be collected and returned,
...