💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193855223)
In commit "refactor: Pass BlockNotify in options to ChainstateManager" (620faa21ce0210dddca243511b7674c3b8f4e901)

Would suggest `ValidationNotifications& notifications` or `ValdiataionNotifications* notifcations{nullptr}` here, and adding a `std::unique_ptr<node::ValidationNotificationsImpl>` member to `NodeContext`. I don't think this should be a shared pointer and also don't think it should be const.

On const: This is an options struct that is supposed to be filled with options set by ex
...

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193951680)
In commit "refactor: Pass DoWarning in options to ChainstateManager" (bcd417ad1847cd4c93bffbc9d8a32255e7f2b156)

I think "notify warning" would be more descriptive than "do warning" and more consistent with other methods.

Also commit title "Pass DoWarning in options to ChainstateManager" maybe should be updated to something like "Add NotifyWarning kernel notification"

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193904693)
In commit "refactor: Pass BlockNotify in options to ChainstateManager" (620faa21ce0210dddca243511b7674c3b8f4e901)

Note: After this commit, there is only one line of code sending the `uiInterface.NotifyBlockTip` global boost signal, so it should be easier to drop the signal, probably replacing it with a `std::list<NotifyHeaderTipFn>` or similar member. Other boost signals could be eliminated after this in a similar way.

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193883274)
In commit "refactor: Pass BlockNotify in options to ChainstateManager" (620faa21ce0210dddca243511b7674c3b8f4e901)

I don't see the point of having one line ChainstateManager wrapper methods for all the notifications. It seems like they just make the call sequence harder to follow, make code hard coder to grep, and create name conflicts with other functions that have to be overcome with `::` specifiers and the `FatalErrorf` renaming.

The one benefit of these wrapper methods is that they redu
...

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193977100)
In commit "refactor: Pass FatalError in options to ChainstateManager" (2f9c2d245360b3fad6d469a76c2916d75b027b57)

I think this method should just be printing the messages to cout, letting error handling code below clean up after the failure. The chainstate example program shouldn't need to be logging things and calling StartShutdown.

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193943783)
In commit "refactor: Pass ShowProgress in options to ChainstateManager" (8fe776b303dddf9b3f1381d263bba6c913591dde)

Would encourage using modern variable names in new code, especially if adding an API that is supposed to be part of the kernel interface. Specifically would suggest renaming `nProgress` to `progress_percent` or something like that here.

Also since this is a kernel API and we don't know what the application UI might be, maybe calling it "notify progress" might be better than "s
...

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1194002643)
In commit "refactor: Pass FatalError in options to ChainstateManager" (2f9c2d245360b3fad6d469a76c2916d75b027b57)

I think this signature should be cleaned up and not be added to kernel like this. Would suggest:

- Changing `bool` return type to `void`. The return value is never used and the only value that is ever returned is false. Having the return value makes the interface confusing because it's unclear how the value even could be used.
- Renaming `strMessage` parameter to `debug_message
...

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193969953)
In commit "refactor: Pass DoWarning in options to ChainstateManager" (bcd417ad1847cd4c93bffbc9d8a32255e7f2b156)

Behavior isn't changing here, but it seems strange that `fWarned` check is only letting the warning notification to be sent and the GUI progress bar to be updated once over the lifetime of the application. May be something to look into in a followup.

:lock: fanquake locked an issue: "v0.6.5 on iPad mini 6 Security Center button incorrent"
(https://github.com/bitcoin/bitcoin/issues/27654)

:lock: fanquake locked an issue: "Mine bitcoin using FPGA"
(https://github.com/bitcoin/bitcoin/issues/27650)

:lock: fanquake locked an issue: "React-native. const { STATUS_CODES } = require('http'); Unable to resolve module http"
(https://github.com/bitcoin/bitcoin/issues/27644)

:lock: fanquake locked an issue: "Keep it simple"
(https://github.com/bitcoin/bitcoin/issues/27618)

:lock: fanquake locked an issue: "BIP 39 words update"
(https://github.com/bitcoin/bitcoin/issues/27649)

:lock: fanquake locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/27567)

💬 Xekyo commented on pull request "Switch hardened derivation marker to h":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1194046865)
The “nor” here seems misplaced:

```suggestion
For legacy wallets the `hdkeypath` field in `getaddressinfo`
and the serialization format of wallet dumps remain unchanged. (#26076)
```

💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1548177296)
so sorry for the delay and thank you for the suggestions! the CI is all green now. i've updated the PR to include:
- addrman new/tried/total count for "all networks"
- test coverage for "all networks" and for total (total refers to both new/tried table count)

I'd want this RPC to be the alternative way to access the actual count of all addresses in the addrman.

💬 instagibbs commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548180934)
@glozow ok so the tree thing is a bandaid in lieu of #26711 , and you'd rather just to that

@joostjager

> If I understood the various threads here and in other prs correctly, I think the consequence of this is that the mempool will be sub-optimal and lead to a block template that earns fewer fees for the miner exposing the endpoint?

Also can have weird effects like your mempool min rate going higher than it should normally.

I think we can easily get #26711 in next release

💬 furszy commented on pull request "wallet: fix deadlock in bdb read write operation":
(https://github.com/bitcoin/bitcoin/pull/27556#issuecomment-1548189712)
rebased, conflicts solved.

💬 Sjors commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1194086866)
@dergoegge `INVENTORY_BROADCAST_MAX` has been poorly named for a while. It applies to inbound peers, but to outbounds we send 5 / 2 more.

💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1548198822)
CI failure seems unrelated