💬 furszy commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1199080037)
> Do you think the the main benefit of this commit is cleaner code, and the worse error detection a side effect? Or are there other benefits to this commit?

Aside from the cleaner code (which I think that it's salable on its own), it also have maintainability and test coverage benefits with the global flag removal. Also, the indexes threads active-wait isn't that good in terms of processors context switching (the index threads are waking up every 0.5s and the reindex process could take a whil
...

💬 kallerosenbaum commented on issue "Sign PSBT: Can't verify change output":
(https://github.com/bitcoin-core/gui/issues/732#issuecomment-1554739241)
I don't consider this as a mere inconvenience, but as a security issue. I can't sign this transaction because I don't know which of the outputs, if any, belongs to me. If people do sign these transactions, they're going to be vulnerable to scammers.

If the fix is hard to do, I think the "sign PSBT" feature should be removed from the GUI, as a security measure, until the issue is fixed.

🤔 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)
```