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

🤔 Prabhat1308 reviewed a pull request: "qa: Improve framework.generate* enforcement (#31403 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/31599#pullrequestreview-2559846404)
ACK [1b51616](https://github.com/bitcoin/bitcoin/pull/31599/commits/1b51616f2e3b58a1c63a19e6dba8e7e9c2aefdeb)

💬 theuni commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1920648795)
Not worth doing now, but in the future it'd be helpful to split up changes like this into separate commits so it's more clear to reviewers what's necessary for the described change vs what's a helpful cleanup.

👍 theuni approved a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2559811192)
This is much simpler than the last time I looked. Thanks ryanofsky for all the feedback here. We can argue about libexec in the other PR :)

utACK 186680acef661bd139bf3d16f494365ea55993b5

💬 theuni commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#issuecomment-2599106040)
> > This caused an incompatibility with blocksdirs from previous versions of Core. Setting to draft whlie I play around with a fix.
>
> Once you figure that out, you can use a previous release to demonstrate that this works. See e.g. `mempool_compatibility.py`.

The compatibility problem was actually caught by the `feature_unsupported_utxo_db` previous release check. So passing here indicates that we're good with old blockstores (and thankfully that we have a working check for that :)

💬 theuni commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1920688000)
The arg that `blocksdir` takes is actually the profiledir which contains the blocks dir, not the blocks dir itself. So passing blocksdir=datadir here means "use my own datadir but node0's blocksdir", which is what this test intends to cover. Unless I'm missing something?

💬 theuni commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1920690469)
I'd rather stay old-school unless you have a strong preference :)

💬 theuni commented on pull request "depends: Always set `CMAKE_BUILD_TYPE=None` globally":
(https://github.com/bitcoin/bitcoin/pull/31661#issuecomment-2599131430)
> For packages that do not set a default build type, this change does not affect their behaviour.

Are you sure this is true? It definitely _could_ affect behavior. I'm wondering if we should do it per-package, even if it's every package, as a sign-off that we've actually checked that it does the right thing.

💬 brunoerg commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1920735492)
3474524ee56dc3d0d009fe1462092ae5d6cbd775: It should be in the solvables wallet, so we could check it in `getaddressinfo`:
```diff
@@ -1346,6 +1346,7 @@ class WalletMigrationTest(BitcoinTestFramework):
# The multisig address should be in the solvables wallet
addr_info = solvables.getaddressinfo(addr)
assert_equal(addr_info["ismine"], True)
+ assert_equal(addr_info['solvable'], True)
assert "hex" in addr_info
```

💬 darosior commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#issuecomment-2599279456)
Pushed a new commit to avoid the implicit `u16` -> `u8` conversion, which for some reason i can't reproduce (and didn't hit) locally.

💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#issuecomment-2599279517)
rebased to address merge conflicts and dropped unnecessary logic for getting the host machine arch.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920876351)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893655528

Thanks, fixed