💬 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.

💬 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.

💬 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.

💬 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.

💬 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
...

💬 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!

💬 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.

👍 maflcko approved a pull request: "kernel: handle null or empty directories in implementation"
(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?

💬 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)?

📝 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.

💬 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 ?

💬 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
...

💬 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.

💬 stickies-v commented on pull request "refactor: remove incorrect lifetimebounds":
(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.

🤔 w0xlt reviewed a pull request: "net_processing: rename RelayTransaction to better describe what it does"
(https://github.com/bitcoin/bitcoin/pull/33565#pullrequestreview-3461208281)
reACK https://github.com/bitcoin/bitcoin/pull/33565/commits/78595ae0e71b833ebe7640d5120eea5da14f55ab

👍 ryanofsky approved a pull request: "cmake: Move IPC tests to `ipc/test`"
(https://github.com/bitcoin/bitcoin/pull/33774#pullrequestreview-3461148887)
Code review ACK 866bbb98fd365962840ee99df0d9f7ad557cc025, just adding back the suggested comment, and also fixing bad include arguments passed to target_capnp_sources. It would probably be a little better if the include fix was done in an earlier commit, since it's not really related to the other changes in the last commit, but would also be ok to make both changes at the same time.

As mentioned below i am confused about include paths used to build the `bitcoin_ipc` librariy which somehow loo
...

💬 ryanofsky commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#discussion_r2524524789)
In commit "cmake, test: Improve locality of `bitcoin_ipc_test` library description" (866bbb98fd365962840ee99df0d9f7ad557cc025)

This change should be correct, but I'm confused about how it was working before. This change causes includes in generated files such `build/src/ipc/capnp/common.capnp.proxy.h` to be generated correctly relative to `src/`:

```diff
-#include <src/ipc/capnp/common.capnp.h> // IWYU pragma: keep
+#include <capnp/common.capnp.h> // IWYU pragma: keep
```

So that see
...

🤔 w0xlt reviewed a pull request: "net_processing: reorder the code that handles the VERSION message"
(https://github.com/bitcoin/bitcoin/pull/33792#pullrequestreview-3461290288)
Thank you for the detailed explanation.
ACK https://github.com/bitcoin/bitcoin/pull/33792/commits/52149b0e2b7897cc2ea059a9b50c4cb3b9fcd722