💬 MarcoFalke commented on pull request "fuzz: Generate rpc fuzz targets individually":
(https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1614624535)
Would be nice if someone also implemented this for OSS-Fuzz: It would require building a fuzz exe for each RPC method, and then probably also adjust https://github.com/bitcoin-core/qa-assets/blob/main/download_oss_fuzz_inputs.py to somehow concatenate all RPC methods into one `rpc.zip` file.

💬 lontivero commented on issue "Improving fee estimation accuracy":
(https://github.com/bitcoin/bitcoin/issues/27995#issuecomment-1614639756)
Fee rates are a everyday discussion in many teams I guess. This is from today's internal discussion about a problem in our UI regarding fees:

💬 TheCharlatan commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1247847963)
In commit "Implement handling of other endianness in BerkeleyRODatabase" 0b8eac6a78351a68c1b5a6126564493ab50031dc

I might be reading something wrong, or confusing something, but I don't quite understand why the record data itself does not have to get its endianness adjusted.

💬 TheCharlatan commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1247852182)
In commit "wallet: implement independent BDB deserializer in BerkeleyRODatabase" d5fe130106bdf367c0bca157b02d12254590b585

Can this `data` field be removed? Seems to be unused.

🤔 brunoerg reviewed a pull request: "fuzz: Generate rpc fuzz targets individually"
(https://github.com/bitcoin/bitcoin/pull/28015#pullrequestreview-1507224273)
Concept ACK

👍 TheCharlatan approved a pull request: "contrib: add macOS test for fixup_chains usage"
(https://github.com/bitcoin/bitcoin/pull/27999#pullrequestreview-1507226540)
ACK 7f96638723a08edf4341a2f4c06b2aa41378fcf7

💬 brunoerg commented on pull request "fuzz: call `LookupSubNet` before calling `Ban` with a subnet":
(https://github.com/bitcoin/bitcoin/pull/27935#issuecomment-1614675792)
Force-pushed to remove "--exclude banman" in Mac

💬 fanquake commented on pull request "ci: label docker images and prune dangling images selectively":
(https://github.com/bitcoin/bitcoin/pull/27793#issuecomment-1614676019)
Concept ACK

👍 hebasto approved a pull request: "ci: re-enable gui tests for s390x"
(https://github.com/bitcoin/bitcoin/pull/28014#pullrequestreview-1507232955)
ACK 9be4565c2db6d7a420d032d5c41843d473ed32d1, tested on Ubuntu 23.04.

💬 hebasto commented on issue "ci: s390x qemu fails after qt5.15":
(https://github.com/bitcoin/bitcoin/issues/23730#issuecomment-1614682004)
It is not an issue anymore. See: https://github.com/bitcoin/bitcoin/pull/28014.

🚀 fanquake merged a pull request: "ci: re-enable gui tests for s390x"
(https://github.com/bitcoin/bitcoin/pull/28014)

📝 sr-gi opened a pull request: "p2p: gives seednode priority over dnsseed if both are provided"
(https://github.com/bitcoin/bitcoin/pull/28016)
This is a follow-up of #27577

If both `seednode` and `dnsseed` are provided, the node will start a race between them in order to fetch data to feed the `addrman`.

This PR gives priority to `seednode` over `dnsseed` so if some nodes are provided as seeds, they can be tried before defaulting to the `dnsseeds`

💬 sr-gi commented on pull request "p2p: gives seednode priority over dnsseed if both are provided":
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-1614740856)
I wanted to have an exit early strategy that will hand the logic back to `dnsseed` if all `seednodes` have been tried and no data has been added to the `addrman`. However, looks like checking if both `m_nodes` and `m_addr_fetch` are empty may not be sufficient (i.e. a node can be removed from `m_addr_fetch`, tried but not yet put at `m_nodes`. This seems to be specifically sensitive for privacy networks where the latency may be higher)

💬 MarcoFalke commented on pull request "p2p: gives seednode priority over dnsseed if both are provided":
(https://github.com/bitcoin/bitcoin/pull/28016#discussion_r1247939565)
nit: Could use `NodeClock::now()` for new code? Also `30s` should work for the constant.

💬 furszy commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#discussion_r1247950494)
Yeah, it gets cleaned at the end of the tx creation process (check https://github.com/bitcoin/bitcoin/pull/27601#discussion_r1247203254).

But new tests never hurt, so just added it inside e46c1276. Thanks for pushing me to do it.

💬 furszy commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#discussion_r1247950832)
done

🤔 furszy reviewed a pull request: "wallet: don't duplicate change output if already exist"
(https://github.com/bitcoin/bitcoin/pull/27601#pullrequestreview-1507352028)
Updated per feedback, thanks @Xekyo.

Added test coverage verifying that the same fee is paid when the wallet creates a change output automatically vs when the user provides the change output and the wallet just use it as recipient for the fee surplus.

👍 fanquake approved a pull request: "script, test: python typing and linter updates"
(https://github.com/bitcoin/bitcoin/pull/28009#pullrequestreview-1507399382)
ACK 6c97757a480b6e71a0750330d69ff18ac7cc6127

🚀 fanquake merged a pull request: "script, test: python typing and linter updates"
(https://github.com/bitcoin/bitcoin/pull/28009)

💬 sipa commented on pull request "Miniscript: always treat unsatisfiable scripts as insane":
(https://github.com/bitcoin/bitcoin/pull/27997#issuecomment-1614821612)
The requirement for a signature actually stems from the same question, though it's not as exact.

If a script can be satisfied without signatures, it means there may be no signatures at all in the whole transaction or input, and thus nothing that actually commits to the nLockTime or nSequence values respectively, and so an attacker could just modify those values without invalidating anything.