💬 ryanofsky commented on pull request "wallet: Filter-out "send" addresses from `listreceivedby*`":
(https://github.com/bitcoin/bitcoin/pull/25973#discussion_r1269477246)
In commit "wallet: Filter-out "send" addresses from `listreceivedby*`" (ce7fc736576664701b09bea811d77113948c38bb)

It like it would be better to call IsMine here instead of relying on the purpose field. The PR description says this fix only covers the include_empty=true, include_watchonly=true case, not the include_watchonly=false case, so maybe you could fix both cases by using the `isminetype` result returned by `IsMine` instead of purpose == "send".

Also, I'm not 100% sure about this, an
...

👍 ryanofsky approved a pull request: "test: miner: add coverage for `-blockmintxfee` setting"
(https://github.com/bitcoin/bitcoin/pull/27620#pullrequestreview-1539327933)
Code review ACK bbbb89d238e9bdaa9f426d55b0a3b714dac1d39b, nice test

👍 brunoerg approved a pull request: "test: miner: add coverage for `-blockmintxfee` setting"
(https://github.com/bitcoin/bitcoin/pull/27620#pullrequestreview-1539355512)
reACK bbbb89d238e9bdaa9f426d55b0a3b714dac1d39b

changes since my last review looks good!

📝 TheCharlatan opened a pull request: "kernel: Remove UniValue from kernel library"
(https://github.com/bitcoin/bitcoin/pull/28113)
Besides the build system changes, this is a move-only change for moving the few UniValue-related functions out of kernel files.

UniValue is not required by any of the kernel components and a JSON library should not need to be part of a consensus library.

🤔 glozow reviewed a pull request: "test: miner: add coverage for `-blockmintxfee` setting"
(https://github.com/bitcoin/bitcoin/pull/27620#pullrequestreview-1539358558)
ACK bbbb89d238e9bdaa9f426d55b0a3b714dac1d39b, sorry for the late re-review!

💬 hebasto commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1643996052)
Concept ACK.

💬 stickies-v commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1643999326)
Concept ACK

💬 furszy commented on pull request "descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#discussion_r1269523002)
upps, true. Fixed.

💬 jamesob commented on pull request "test: Add missing `set -ex` to ci/lint/06_script.sh":
(https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1269534435)
Nah, this is a step in the right direction I think.

💬 furszy commented on pull request "descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#discussion_r1269536762)
I initially did it without the early return. But IIRC, the inference of a non-standard script as an addr() instead of a raw() made me have a second thought.

Still, all good. Removed the early return.

💬 fanquake commented on issue "ci: failure in Docker build step":
(https://github.com/bitcoin/bitcoin/issues/27492#issuecomment-1644018895)
Closing this again.

✅ fanquake closed an issue: "ci: failure in Docker build step"
(https://github.com/bitcoin/bitcoin/issues/27492)

🤔 furszy reviewed a pull request: "descriptors: do not return top-level only funcs as sub descriptors"
(https://github.com/bitcoin/bitcoin/pull/28067#pullrequestreview-1539391376)
Updated per feedback. Thanks @darosior!.

💬 ryanofsky commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269529793)
re: https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1262333855

> hm see [#27850 (comment)](https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1239761122) not sure what to do

I don't actually understand what this issue is in that comment since the variable seems to be used just once.

It does seem strange to define these commands as variables instead of inlining them when the variables only seem to be used only once. Also would expect them to be shell functions instead of
...

👍 ryanofsky approved a pull request: "test: Add unit & functional test coverage for blockstore"
(https://github.com/bitcoin/bitcoin/pull/27850#pullrequestreview-1539378350)
Code review ACK 08b275c6d27f06cd7d2580b4c0200e32d5b7b1b6. This adds nice clear tests and a more realistic CI environment.

💬 achow101 commented on issue "build: Windows debug cross-build fails with `-O0`":
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1644029992)
How big is too big?

`wallet.cpp` is pretty much entirely `CWallet`'s members, so breaking it up will require breaking up `CWallet` which I'm not sure we want to do? I suppose not all of the members need to be in `CWallet`, but I think it'll still be necessary for that class to be kinda big.

🚀 fanquake merged a pull request: "test: miner: add coverage for `-blockmintxfee` setting"
(https://github.com/bitcoin/bitcoin/pull/27620)

💬 theuni commented on issue "build: Windows debug cross-build fails with `-O0`":
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1644049013)
> How big is too big?

Good question :)

>
> `wallet.cpp` is pretty much entirely `CWallet`'s members, so breaking it up will require breaking up `CWallet` which I'm not sure we want to do?

Yeah, agreed that we shouldn't be breaking up classes for something like this. I suspect this actually has more to do with the _giant_ list of includes. Specifically, getting rid of boost signals ([here's a POC from a while back](https://github.com/theuni/bitcoin/commits/replace-boost-signals)) would
...

💬 sipa commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1269574745)
Fixed.

💬 sipa commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1269576094)
That'd mean grabbing the lock twice, no?