💬 darosior commented on pull request "descriptor: do not parse from script unspendable Miniscript descriptors":
(https://github.com/bitcoin/bitcoin/pull/28112#issuecomment-1643932769)
Closing this, sorry for the noise. I don't think it is useful to forbid parsing a Miniscript descriptor from Script since it cannot be a footgun for end user and it's always better to parse a Miniscript, even though unspendable, than a raw Script to be displayed in some utilities (such as `decodescript` for instance).

✅ darosior closed a pull request: "descriptor: do not parse from script unspendable Miniscript descriptors"
(https://github.com/bitcoin/bitcoin/pull/28112)

💬 hebasto commented on pull request "refactor: Make more transaction size variables `int32_t`":
(https://github.com/bitcoin/bitcoin/pull/28059#issuecomment-1643933181)
> Not sure. The other types are 64 bit, so this will overflow eventually

> Are you still working on this?

Since the recent push the `totalSizeWithAncestors` is `int64_t`.

💬 darosior commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1269467464)
This is not true if the Miniscript descriptor was parsed from Script. I'll update this.

💬 furszy commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1269467782)
@whitslack yes. That would be an ugly outcome. We could have fixed the RPC docs here.

Would say to work towards #27601 to fix this scenario (and others) properly.
This issue comes from a workaround code that avoids the dup change outputs bug. Basically, the change output is manually discarded from the recipients list and expected to be re-added later, with the new fees subtracted, by the inner transaction creation process logic.

With #27601, we will be able to specify the output to reduce
...

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1643935993)
@MarcoFalke @furszy @ryanofsky thanks for the reviews, latest push addresses all comments except where noted.

👍 ryanofsky approved a pull request: "wallet: Filter-out "send" addresses from `listreceivedby*`"
(https://github.com/bitcoin/bitcoin/pull/25973#pullrequestreview-1539294064)
Code review ACK 62880a9bb167bc0569175d597a865e6baccce71b, but this needs to be rebased due to a silent conflict with #27217. This seems like a helpful fix and the test cleanup is also nice.

Another suggestion if you feel like it would be to add the new test before making the code change and have it verify the previous behavior. Then fix the bug and update the test in the same commit, so it is easier to see how behavior changes in the diff.

💬 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!.