💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2598867587)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920362724 and https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920531697

💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1920546152)
Does this hard-coded thread name cause any issues if sockman is used more than once?

💬 furszy commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920553269)
This could also be a WIP formatted private key right?
Should use the general term "key" here. E.g.
```c++
strprintf(key '%s is invalid due to an extra whitespace at %s of the string', key_str, (IsSpace(str.front())?"beginning" : "end"));`
```

🤔 sipa reviewed a pull request: "net: Use GetAdaptersAddresses to get local addresses on Windows"
(https://github.com/bitcoin/bitcoin/pull/31014#pullrequestreview-2559638620)
Concept and code review ACK 219872fc75e552c87c20b5c1ce7e15fa887eec20, but I am not familiar with the Windows network API, and have not tested it.

💬 sipa commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#discussion_r1920557024)
The loops seems unnecessary to me.

```c++
out_buf.resize(std::min(std::max(out_buf_len, out_buf.size() * 2), MAX_ADAPTER_ADDR_SIZE));
```

💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920583787)
Yes, it could be a WIF private key. I will change it to use "key".

💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920591046)
Done.

💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2598956762)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920553269 (thanks, @furszy)

🤔 ismaelsadeeq reviewed a pull request: "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict"
(https://github.com/bitcoin/bitcoin/pull/29680#pullrequestreview-2559684532)
Concept ACK

Left some initial comments

> Previously, I captured the replacement_tx when a tx was being kicked out of the Mempool, but with the addition of packages, any of the tx in the package could be the reason for the conflict.
I've changed the replacement_tx to be the last child tx in the package so the wallet doesn't have to watch the entire replacement package.

Can you elaborate on how that work, and the wallet detects that, what if the conflict is not from the child, it's from
...

💬 ismaelsadeeq commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1920580408)
In 4c64dd5a3cc4a48e5470fff26c99dde20f81c7e0 "Change MemPoolRemovalReason to variant type"
nit: Commit message should follow the guidelines in https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches

The explanation is a long should break it.

💬 ismaelsadeeq commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1920581830)
In 4c64dd5a3cc4a48e5470fff26c99dde20f81c7e0 "Change MemPoolRemovalReason to variant type"
nit: this line is too long, please break.

💬 ismaelsadeeq commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1920584345)
In 71200b72ab848c3066e5123dc0badc24c9648f47 "Handle double-spending of unrelated parents to wallet txs"
```suggestion
auto* replaced_reason = std::get_if<ReplacedReason>(&reason);
if (IsFromMe(*tx) && replaced_reason != nullptr) {

// Check if wallet transaction is being replaced by a parent transaction which is not from this wallet
if (replaced_reason->replacement_tx.has_value() && !IsFromMe(*replaced_reason->replacement_tx.value())) {
m_unrelated_conf
...

💬 ismaelsadeeq commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1920585609)
In 71200b72ab848c3066e5123dc0badc24c9648f47 "Handle double-spending of unrelated parents to wallet txs"

How do you know that without checking ?

💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows, OpenBSD)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2598968811)
I pushed an additional commit which adds `bitcoin-node` and `bitcoin-gui` to `Maintenance.cmake`. Before this PR they were already part of `installable_targets`, conditional on being built. So it makes sense to run the binary security, symbol and dynamic library checks on them.

I clarified the PR description on this topic.

If people prefer to defer including these binaries, e.g. for discussion in another PR, I could also drop this commit and instead remove them from `installable_targets`.
...

💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1920603848)
The reason I did this was because of the caching issue I described above which can be reproduced as follows with a clean docker cache:

```console
$ # clean docker cache on fedora 40
$ systemctl restart docker
$ docker system prune -a

$ docker run --platform=s390x docker.io/ubuntu:24.04 bash -c 'uname -m'
Unable to find image 'ubuntu:24.04' locally
24.04: Pulling from library/ubuntu
755503a8fb36: Pull complete
Digest: sha256:80dd3c3b9c6cecb9f1667e9290b3bc61b78c2678c02cbdae5f0fea92cc
...

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#issuecomment-2599027407)
IIUC the recently merged Silent Payments PSBT BIP _only_ works with PSBTv2?
https://github.com/bitcoin/bips/blob/master/bip-0375.mediawiki

Having a (rough) draft PR implementing those fields on top of this PR could also aid in review. cc @josibake @andrewtoth

💬 jonatack commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#issuecomment-2599059066)
> IIUC the recently merged Silent Payments PSBT BIP _only_ works with PSBTv2?

Suggested to consider renaming BIP375 to be more clear about this, unless there's a reason I'm missing: https://github.com/bitcoin/bips/pull/1687#discussion_r1913779155

🤔 Prabhat1308 reviewed a pull request: "test: Add expected result assertions"
(https://github.com/bitcoin/bitcoin/pull/31656#pullrequestreview-2559823633)
Code ack [0a45485](https://github.com/bitcoin/bitcoin/pull/31656/commits/0a4548589f9a50db588c20bdb9589983bec195c2)

left some comments

💬 Prabhat1308 commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#discussion_r1920656162)
```suggestion
for (int i = 0; i < 10; ++i) {
```
Preference is given to `++i` according to `developer-notes.md`

💬 Prabhat1308 commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#discussion_r1920657848)
nit: there seems to be a whitespace needed before expected result in the first loop. please run a formatter over the code if not done already.