💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524170909)
> Do we still need this after https://github.com/bitcoin/bitcoin/pull/33550?
I think it's still needed, I added it to get rid of linter warnings if `std::filesystem::copy` is instead called directly from the test code.
> Are we adding dead code in this commit? It's hard to judge if a solution is accurate if we see it before the problem is presented - can we make the question obvious before we provide the answer?
Fair point, I'll rework the commits so it's a bit easier to tell where thin
...
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524170909)
> Do we still need this after https://github.com/bitcoin/bitcoin/pull/33550?
I think it's still needed, I added it to get rid of linter warnings if `std::filesystem::copy` is instead called directly from the test code.
> Are we adding dead code in this commit? It's hard to judge if a solution is accurate if we see it before the problem is presented - can we make the question obvious before we provide the answer?
Fair point, I'll rework the commits so it's a bit easier to tell where thin
...
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524183047)
> Can we add a simpler fuzz test which needs this in the same commit that introduces it - and extend the test with other functionality separately.
Yes, I'll do some variation of this. I think it might be cleaner to put it as one of the last commits.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524183047)
> Can we add a simpler fuzz test which needs this in the same commit that introduces it - and extend the test with other functionality separately.
Yes, I'll do some variation of this. I think it might be cleaner to put it as one of the last commits.
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524195439)
This is copied from the `process_message(s)` harness. Unlike those, this fuzz test should always construct correctly serialized messages (even if the messages themselves are deemed "protocol invalid"), so I think the catch case can just be `assert(false)`.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524195439)
This is copied from the `process_message(s)` harness. Unlike those, this fuzz test should always construct correctly serialized messages (even if the messages themselves are deemed "protocol invalid"), so I think the catch case can just be `assert(false)`.
💬 stickies-v commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524238329)
Ah, thanks for those 2 links, that's helpful. I've been digging through cppreference and struggle to find a definitive explanation on what `std::filesystem::absolute(std::filesystem::path())` represents or why it's (il)legal, and so far haven't been able to find anything, so I just went with the behaviour of my implementation. I can see the viewpoint that an empty path does not exist in the filesystem, and thus should not have an absolute path associated with it.
Since it's (not unreasonably)
...
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524238329)
Ah, thanks for those 2 links, that's helpful. I've been digging through cppreference and struggle to find a definitive explanation on what `std::filesystem::absolute(std::filesystem::path())` represents or why it's (il)legal, and so far haven't been able to find anything, so I just went with the behaviour of my implementation. I can see the viewpoint that an empty path does not exist in the filesystem, and thus should not have an absolute path associated with it.
Since it's (not unreasonably)
...
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524253265)
Will do, I think this was used in multiple places in an earlier version of this code.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524253265)
Will do, I think this was used in multiple places in an earlier version of this code.
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524255627)
I'll incrementally add the cases so it's a bit easier to digest.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524255627)
I'll incrementally add the cases so it's a bit easier to digest.
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524277972)
IIRC I tried comparing the `uint256` block hashes similar to your second suggestion, and while it fixed the leveldb non-determinism, it introduced its own non-determinism.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524277972)
IIRC I tried comparing the `uint256` block hashes similar to your second suggestion, and while it fixed the leveldb non-determinism, it introduced its own non-determinism.
💬 stickies-v commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524354194)
Marking as resolved as this code is now outdated anyway.
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524354194)
Marking as resolved as this code is now outdated anyway.
💬 stickies-v commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#issuecomment-3528952987)
Force-pushed to disallow empty and null directories, addressing feedback from @maflcko both wrt [technical concerns](https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523891180) and [conceptual concerns](https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523930442). Also updated PR title and description.
(https://github.com/bitcoin/bitcoin/pull/33867#issuecomment-3528952987)
Force-pushed to disallow empty and null directories, addressing feedback from @maflcko both wrt [technical concerns](https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523891180) and [conceptual concerns](https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523930442). Also updated PR title and description.
💬 ryanofsky commented on issue "Mining interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3528997008)
Was looking at the IRC discussion and I think a nuance it doesn't address is that there is a difference between breaking compatibility explicitly so clients see an error if they try to call a method that's been changed or removed, and breaking compatibility silently so if a client tries to call a method that's changed or removed, a different method may be called instead or arguments may be interpreted incorrectly.
#### How to break compatibility explicitly
You can break compatibility explicitl
...
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3528997008)
Was looking at the IRC discussion and I think a nuance it doesn't address is that there is a difference between breaking compatibility explicitly so clients see an error if they try to call a method that's been changed or removed, and breaking compatibility silently so if a client tries to call a method that's changed or removed, a different method may be called instead or arguments may be interpreted incorrectly.
#### How to break compatibility explicitly
You can break compatibility explicitl
...
💬 vasild commented on pull request "net_processing: reorder the code that handles the VERSION message":
(https://github.com/bitcoin/bitcoin/pull/33792#issuecomment-3528997261)
`431dd57d67...52149b0e2b`: drop some hunks from the diff, `m_addrman.Good()` need not be moved around. Thanks, @w0xlt!
(https://github.com/bitcoin/bitcoin/pull/33792#issuecomment-3528997261)
`431dd57d67...52149b0e2b`: drop some hunks from the diff, `m_addrman.Good()` need not be moved around. Thanks, @w0xlt!
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3529005336)
`7547f915bc...36ae854b1d`: take changes from https://github.com/bitcoin/bitcoin/pull/33792, apply similar simplification in the handling of `VERACK` message and some nit changes around logging.
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3529005336)
`7547f915bc...36ae854b1d`: take changes from https://github.com/bitcoin/bitcoin/pull/33792, apply similar simplification in the handling of `VERACK` message and some nit changes around logging.
👍 maflcko approved a pull request: "kernel: handle null or empty directories in implementation"
(https://github.com/bitcoin/bitcoin/pull/33867#pullrequestreview-3460982378)
lgtm
(https://github.com/bitcoin/bitcoin/pull/33867#pullrequestreview-3460982378)
lgtm
💬 maflcko commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524403236)
nit: I think you are just testing the stdlib string constructor. Probably want to drop those, or use string_view?
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524403236)
nit: I think you are just testing the stdlib string constructor. Probably want to drop those, or use string_view?
💬 maflcko commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524399383)
nit: could add back the attributes for (1,2,4)?
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524399383)
nit: could add back the attributes for (1,2,4)?
📝 theStack opened a pull request: "init: completely remove `-maxorphantx` option"
(https://github.com/bitcoin/bitcoin/pull/33872)
This is a small follow-up for #32941 (commit 1384dbaf6d0bfcdb05f97e1e3cb3d5e498bee505), removing the `-maxorphantx` option completely, now that v30 has been released. If removing it for v31 is seen as controversial/premature (I personally don't think it is), the merge can be delayed for a future release.
(https://github.com/bitcoin/bitcoin/pull/33872)
This is a small follow-up for #32941 (commit 1384dbaf6d0bfcdb05f97e1e3cb3d5e498bee505), removing the `-maxorphantx` option completely, now that v30 has been released. If removing it for v31 is seen as controversial/premature (I personally don't think it is), the merge can be delayed for a future release.
💬 stickies-v commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524442289)
Wouldn't that introduce UB for e.g. the tests that exercise the null path? According to the [docs](https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html):
> The compiler may also perform optimizations based on the knowledge that nonnull parameters cannot be null.
I thought we removed the attribute for the same reason in #33853 ?
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524442289)
Wouldn't that introduce UB for e.g. the tests that exercise the null path? According to the [docs](https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html):
> The compiler may also perform optimizations based on the knowledge that nonnull parameters cannot be null.
I thought we removed the attribute for the same reason in #33853 ?
💬 ryanofsky commented on issue "Mining interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3529071835)
re: https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3502947184
> That would allow us to avoid shoehorning block templates into a `CBlock`, which e.g. means we no longer need a dummy coinbase transaction.
It does seem error-prone to expose the dummy coinbase transaction, because if any clients are relying on it and we make internal changes, they could easily break.
At some point after getCoinbase() is introduced in #33819, it could make sense to swap `m_block_template->block.vtx[0
...
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3529071835)
re: https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3502947184
> That would allow us to avoid shoehorning block templates into a `CBlock`, which e.g. means we no longer need a dummy coinbase transaction.
It does seem error-prone to expose the dummy coinbase transaction, because if any clients are relying on it and we make internal changes, they could easily break.
At some point after getCoinbase() is introduced in #33819, it could make sense to swap `m_block_template->block.vtx[0
...
💬 stickies-v commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524472427)
Good catch, thanks. I've also updated the `ChainstateManagerOptions` ctor to take a string_view so we can properly exercise both the null and empty test case.
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524472427)
Good catch, thanks. I've also updated the `ChainstateManagerOptions` ctor to take a string_view so we can properly exercise both the null and empty test case.
💬 stickies-v commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#issuecomment-3529150552)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33870#issuecomment-3529150552)
Concept ACK
💬 murchandamus commented on pull request "chainparams: remove dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3529185505)
ACK https://github.com/bitcoin/bitcoin/commit/b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
Luke has been and continues to be actively hostile to the Bitcoin Core project and therefore should not be trusted with any sort of elevated privileges by the project.
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3529185505)
ACK https://github.com/bitcoin/bitcoin/commit/b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
Luke has been and continues to be actively hostile to the Bitcoin Core project and therefore should not be trusted with any sort of elevated privileges by the project.