💬 pinheadmz commented on pull request "net: use interruptible async getaddrinfo wrapper from libevent for DNS":
(https://github.com/bitcoin/bitcoin/pull/27505#issuecomment-1531618658)
@fanquake @dergoegge good feedback, thanks. I'll look into a different approach moving the lib call `getaddrinfo` to a detachable thread before abandoning, and then we may just have to close https://github.com/bitcoin/bitcoin/issues/16778 as "won't fix"

There is another thought I had about name resolution moving forward: If bitcoin can make more interesting DNS queries (either using a library or just implementing some bare necessities) we could ask the DNS seeders for I2P and onion addresses
...

💬 fanquake commented on pull request "net: use interruptible async getaddrinfo wrapper from libevent for DNS":
(https://github.com/bitcoin/bitcoin/pull/27505#issuecomment-1531621948)
> either using a library

It's very unlikely we are going to add a new external dependency to make "interesting" DNS queries.

💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1531627646)
> 🐙 This pull request conflicts with the target branch and [needs rebase](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes).

done ✅

💬 MarcoFalke commented on pull request "test: Treat `bitcoin-wallet` binary in the same way as others":
(https://github.com/bitcoin/bitcoin/pull/27554#discussion_r1182685633)
```suggestion
self.options.bitcoinwallet = os.getenv("BITCOINWALLET", default=fname("bitcoin-wallet"))
```

What about making this a function to reduce code bloat while touching this?

💬 ccdle12 commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1182690915)

nit: This constructor could use the default keyword since `byte_count` and `msg_count` will default to 0.

e.g. https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c80-use-default-if-you-have-to-be-explicit-about-using-the-default-semantics

```
MsgStat() = default;
```

💬 instagibbs commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1531653495)
What's the problem with just doing error handling, and manually applying your own fallback fee? Faking a result is another layer of confusion imo

💬 ccdle12 commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1182691546)
According to this review comment: https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1146305999 - It looks like it would be preferable for CNode to have net_stats as a member variable (I could be wrong) but that might mean each CNode would need to have a shared_ptr to a NetStats instance.

💬 ccdle12 commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#issuecomment-1531657509)
Concept tACK - left a few very minor comments, happy to keep testing and reviewing in more depth

⚠️ TheBlueMatt opened an issue: "Avoid serving stale fees"
(https://github.com/bitcoin/bitcoin/issues/27555)
After conversation on irc it appears there's some cases where Bitcoin core will serve feerate information even though it loaded a fairly old fee estimates file and is still syncing the chain. This is pretty dangerous for lightning nodes, at least on legacy channels or pre-package-relay.

The immediate improvement would be to store fee estimates to disk once an hour or so to reduce the chance of having an old file. From there this case could probably be detected and refuse to serve estimates unti
...

💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1182709725)
Oops, fixed

💬 instagibbs commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#issuecomment-1531678984)
Special-casing it is a value-add imo: v3/ephemeral anchor tx should be aggressively fee bumping anyways.

💬 kylezs commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1531690323)
> What's the problem with just doing error handling, and manually applying your own fallback fee? Faking a result is another layer of confusion imo

Because a -fallback fee already exists. Why can't it exist for other use cases?

💬 achow101 commented on pull request "rpc: Return block hash & height in getbalances, gettransaction and getwalletinfo":
(https://github.com/bitcoin/bitcoin/pull/26094#issuecomment-1531700243)
ACK 710b83938ab5bbc4bd324d8b2e69461a2a1d2eec

💬 instagibbs commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1531711091)
If you absolutely cannot handle RPC exceptions your test harness could also:

1) mock out estimatesmartfee calls to give something deterministic
2) load a saved fee estimation file

✅ achow101 closed an issue: "Return block hash with wallet calls"
(https://github.com/bitcoin/bitcoin/issues/18567)

🚀 achow101 merged a pull request: "rpc: Return block hash & height in getbalances, gettransaction and getwalletinfo"
(https://github.com/bitcoin/bitcoin/pull/26094)

💬 dergoegge commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1531714320)
re-ACK f59f0c91acb7a35b767bb0dc75ed8b10add81d9f

🤔 jarolrod reviewed a pull request: "Remove confusing "Dust" label from coincontrol dialog"
(https://github.com/bitcoin-core/gui/pull/719#pullrequestreview-1409413698)
Concept ACK, the changes to the UI file aside from removing the dust label are changing the positioning of the Titles relevant to the value. The screenshot below shows this with the "Bytes" and "Change" field. The fully visible text is master, and the slightly transparent text is the pr.

<img width="1112" alt="QA-change" src="https://user-images.githubusercontent.com/23396902/235720786-50cf5c32-3d2f-46c7-ab06-0f4b6cf865ea.png">

💬 jarolrod commented on pull request "Switch RPCConsole wallet selection to the one most recently opened/restored/created":
(https://github.com/bitcoin-core/gui/pull/696#issuecomment-1531728458)
cc @john-moffett still working on this?

💬 fanquake commented on pull request "contrib: add ELF OS ABI check to symbol-check.py":
(https://github.com/bitcoin/bitcoin/pull/26953#discussion_r1182751218)
Thanks, added a check