💬 hebasto commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1199115073)
> Would suggest `ValidationNotifications& notifications` or `ValdiataionNotifications* notifcations{nullptr}` here,

I'd prefer the pointer type to do not bother about lifetime of variables that were used for assigning.

💬 hebasto commented on pull request "Add translatable address error message ":
(https://github.com/bitcoin-core/gui/pull/534#issuecomment-1554786608)
Closing due to lack of interest.

✅ hebasto closed a pull request: "Add translatable address error message "
(https://github.com/bitcoin-core/gui/pull/534)

💬 hebasto commented on pull request "Add more detailed address error message":
(https://github.com/bitcoin-core/gui/pull/533#issuecomment-1554787346)
Closing due to lack of interest.

✅ hebasto closed a pull request: "Add more detailed address error message"
(https://github.com/bitcoin-core/gui/pull/533)

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1199141298)
re: https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1199037376

> Why is it safe to use a reference type here?

I wouldn't call it "safe", since in general pointers and references in C++ are pretty dangerous, but as long as the notifications object is destroyed after the last notification is sent, there is not a bug. Could add a comment to the `ChainstateManagerOpts` struct to make this explicit, saying the notifications option is mandatory and the object lifetime needs to be at le
...

💬 hebasto commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1199157134)
> Could add a comment to the `ChainstateManagerOpts` struct to make this explicit, saying the notifications option is mandatory and the object lifetime needs to be at least as long as the `ChainstateManager` object.

Right. But that is not the case for the `notifications` variable in: https://github.com/bitcoin/bitcoin/blob/d96c82a76775b1a41c098e6af60130fbdbba9975/src/init.cpp#L1022-L1029

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1199159229)
> I'd prefer the pointer type to do not bother about lifetime of variables that were used for assigning.

I'd agree with you if we passing a string or some other passive data structure as an option to the kernel. Better to just use copy/move/reference counts to get the options struct to manage the memory rather than worry about lifetime of external variables.

But `kernel::Notifications` option is not just a passive set-and-forget option because it actively receives callbacks from the kernel
...

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1199164171)
> Right. But that is not the case for the `notifications` variable in:

That code looks safe to me, but is there a bug?

It definitely is verbose and duplicative, and probably the repeated parsing could be removed later with a refactoring.

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1199176025)
thanks fixed with
```
LogPrintf("Flushed fee estimates to %s.", fs::PathToString(m_estimation_filepath.filename()));
```
so as to get the filename only not the dir

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1199183106)
Thanks fixed

💬 stickies-v commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1199184833)
Suggested documentation since it's not immediately obvious why this is necessary.
```suggestion
# This allows `test_runner.py` to work from an out-of-source build directory using a symlink,
# hard link or a copy on any platform. See https://github.com/bitcoin/bitcoin/pull/27561.
sys.path.append(tests_dir)
```

👍 stickies-v approved a pull request: "test: Explicitly specify directory where to search tests for"
(https://github.com/bitcoin/bitcoin/pull/27561#pullrequestreview-1434790272)
ACK 342a0c865d8b4b00a088af7b70b1ee0df1864f5c modulo improved documentation (left a suggestion).

> The drawback of such an approach is that it creates additional files in the source tree.

You're right, I can't find a way to install the package without it affecting the source tree in one way or another. So the current approach is probably the best approach.

👋 achow101's pull request is ready for review: "bumpfee: Allow the user to choose which output is change"
(https://github.com/bitcoin/bitcoin/pull/26467)

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1199199343)
I was thinking that introducing flexibility would enable users to define their preferred threshold for considering fee estimates as stale. As @instagibbs suggested extending the default 12 hours further. what is your view on it?

💬 hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1555065798)
Updated 342a0c865d8b4b00a088af7b70b1ee0df1864f5c -> c44f3f231988dc05c4c7a8a96bc2e7b1a54da277 ([pr27561.03](https://github.com/hebasto/bitcoin/commits/pr27561.03) -> [pr27561.04](https://github.com/hebasto/bitcoin/commits/pr27561.04), [diff](https://github.com/hebasto/bitcoin/compare/pr27561.03..pr27561.04)):

- addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1199184833)

💬 hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1199252249)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1555065798).

👍 stickies-v approved a pull request: "test: Explicitly specify directory where to search tests for"
(https://github.com/bitcoin/bitcoin/pull/27561#pullrequestreview-1434932654)
re-ACK https://github.com/bitcoin/bitcoin/commit/c44f3f231988dc05c4c7a8a96bc2e7b1a54da277

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1199336080)
> Maybe more ideally we could drop the bool by just having the test call BOOST_CHECK(ShutdownRequested()) and AbortShutdown() after it calls MaybeCompleteSnapshotValidation. This would make the test more realistic and add better coverage, but I didn't check if that worked.

This doesn't work, because calling `StartShutdown` without calling `InitShutdownState` beforehand triggers an `assert(0)`.

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1199375548)
> This doesn't work, because calling `StartShutdown` without calling `InitShutdownState` beforehand triggers an `assert(0)`.

I guess then question would be why can't the test framework call `InitShutdownState`? But this suggests that if we remove globals in shutdown code later (I don't know if that's required for the kernel), we should be able to get rid of the `m_shutdown_on_fatal_error` variable pretty easily later too.

💬 mzumsande commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1199338711)
Good catch! Is my understanding correct that this race cannot actually happen in practice on master right now, because the indexes are set up (`Init()`) by the init thread, which starts the networking and loadblk threads only later - so I can't see from which other thread `BlockConnected` signals could come from at this stage. However, once index initialization is deferred to the loadblk thread in ca3041984cf3665e27b6783c923ab1c32682500a, I think this race could easily happen.
(this might aff
...