💬 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

💬 brunoerg commented on pull request "fuzz: addrman, add coverage for `network` field in `Select()`, `Size()` and `GetAddr()`":
(https://github.com/bitcoin/bitcoin/pull/27549#issuecomment-1531731272)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/27549#pullrequestreview-1408940288

Added coverage for `Size()` and `GetAddr()`, since I got code from https://github.com/bitcoin/bitcoin/commit/8f91e5898b3571e0802f2197981c54dda9ee71fa, I added @mzumsande and @amitiuttarwar as co-authors. PR description has been updated.

💬 real-or-random commented on pull request "Add ASM optimizations for MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/19181#issuecomment-1531736632)
> but I would also be curious if there is another way to tell GCC to do the same optimizations clang seems to be doing.


I played around with this a bit, and I don't see any obvious trick to make that work. If someone else wants to give it a try, https://gcc.godbolt.org/z/hhGfeEoKq could be a nice starting point.

💬 hebasto commented on pull request "test: Treat `bitcoin-wallet` binary in the same way as others":
(https://github.com/bitcoin/bitcoin/pull/27554#discussion_r1182788522)
> What about making this a function to reduce code bloat while touching this?

Done.

💬 laanwj commented on pull request "contrib: add ELF OS ABI check to symbol-check.py":
(https://github.com/bitcoin/bitcoin/pull/26953#issuecomment-1531794631)
Code review ACK 65ba8a79a2919a0bd89f2f2d981e072d4f2f549d