💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198680321)
974b4293a5f2fe01302c608bf0063e3b52721d0b: Any reason to accept a nullptr for something that can never be null and would lead to UB if it was null? Might be better to use a reference.

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198753425)
c05c15cc7b4fb9052b87bc2f8ea0500cd5218f4e: question for clarity: This commit is not needed and unrelated to kernel? Seems fine to do, just asking.

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198757020)
nit in e850319d5336335a9efbf04307261fb565b7bc68: Would be good to either not touch it or remove the comment?

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198767568)
d96c82a76775b1a41c098e6af60130fbdbba9975: I think you forgot the call to `std::abort`, otherwise it seems that the error isn't treated fatal?

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198758293)
nit in https://github.com/bitcoin/bitcoin/commit/e850319d5336335a9efbf04307261fb565b7bc68: Wrong section

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198784478)
brainstorming nit in d96c82a76775b1a41c098e6af60130fbdbba9975: Not sure about adding test-only code to real code. Would it be a lot more effort to add a test-only derived `MockNotif : node::KernelNotifications` struct that no-ops the fatalError method and use it in the one test?

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198753949)
Same for `AnyPtr`?

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198788990)
Just seemed weird to go from `CBlockIndex*` to `CBlockIndex&` back to `CBlockIndex*`

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198790430)
Not strictly required, but I decided this makes sense, since these are currently in `namespace util` and are just a small amount of header-only code. See the discussion we had here: https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1196623448.

💬 fanquake commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198790811)
Yea. In general, it's fine for those comments to just be dropped if touching the line.

💬 hebasto commented on pull request "msvc: Provide `ObjectFileName` explicitly":
(https://github.com/bitcoin/bitcoin/pull/27687#issuecomment-1554347352)
Friendly ping @sipsorcery :)

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198791887)
Yeah, it may be odd in the meantime, but if it is done for all the lines that are touched anyway and if longer term the ui_interface lines are touched, they can also be switched over?

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198793373)
Ah sure. You didn't want to move them to common, so you moved them to util ahead of the other change.

💬 MarcoFalke commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1554355740)
> Here's a test commit that adds that functionality https://github.com/bitcoin/bitcoin/commit/f26a85059fb4eaf29f9c1652b2b5e14060d79a36

Thanks. Did you want me to push it here (removing the logging), or did you want to open a fresh pull yourself?

Also, I think we dropped support for non-witness compact block relay, so I guess you can drop `txid` from your map and only use `wtxid`?

💬 MarcoFalke commented on pull request "[WIP] wallet: standardize change output detection process":
(https://github.com/bitcoin/bitcoin/pull/25979#issuecomment-1554360733)
Needs rebase if still relevant

💬 MarcoFalke commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1554361678)
Looks like CI failed?

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198809696)
The changes here and [here](https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198758293) are done by `git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v`. Should I always drop these format changes that just re-order includes? I already drop a bunch of other changes that re-format moved code, or squash code onto a single line, that would be easier to read with its current hand-written multi line formatting.

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198812663)
> Should I always drop these format changes that just re-order includes?

No, it is just a style nit. Feel free to ignore. I just think that instead of moving one include in the wrong section to another place in the wrong section is worse than just moving it to the right place in the right section in one go.

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198815026)
See the discussion in [this](https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1428804291) and [this](https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1550107951) comment. To summarize: Calling `std::abort` or `exit(0)` would skip over destructors and lead to an unclean exit. Functions calling `fatalError` should already be returning values that would lead to `bitcoin-chainstate` shutting down normally.

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198817207)
I ran through the scenario in [this comment](https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1553391517). We also already have a bunch of other test-only or debug-only options, so I don't think we are setting a precedent here.