🤔 glozow reviewed a pull request: "Fee estimation: avoid serving stale fee estimate"
(https://github.com/bitcoin/bitcoin/pull/27622#pullrequestreview-1434616046)
Approach ACK

💬 glozow commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1199085331)
What is the use case for being able to configure the maximum file age?

💬 glozow commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1199078486)
in accf995cf095d7ff3d10c0403e70bdbc1273e852

Seems like the error message here should be something like "there is no fee estimates file" instead of "failed to read" ?

💬 furszy commented on issue "Sign PSBT: Can't verify change output":
(https://github.com/bitcoin-core/gui/issues/732#issuecomment-1554758441)
My note was for other devs. Based on your comments, you want to get a visual distinction on addresses owned by the signer wallet in the dialog. Which is doable and not hard to do.
My comment was just referring to the `IsMine(addr)` usage instead of the `IsChange(addr)`. Which fulfills the required purposes in the same way (and also accepts external addresses which is more accurate).

💬 kallerosenbaum commented on issue "Sign PSBT: Can't verify change output":
(https://github.com/bitcoin-core/gui/issues/732#issuecomment-1554767293)
Got it, thanks!

💬 hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554771916)
> Install the package in editable mode:

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

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