💬 josibake 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_r1609556427)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

nit: I think it's more correct to leave this as "Reason." "Reasons" implies a single transaction can have multiple reasons for being removed at the same time.

🤔 josibake 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-2070549155)
Still reviewing the later commits, but had some initial feedback/questions for the first commit.

💬 josibake 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_r1609633243)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

I think it would be better to pass a dummy `CTxRef` here instead of nullptr.

💬 josibake 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_r1609617090)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

Was a bit surprised that `pthisBlock->GetHash()` isn't returning a member variable and instead is calculating the hash each time its called. Worth mentioning we are adding an extra hash to `ConnectTip`. Probably negligible but wanted to mention it.

💬 josibake 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_r1609583688)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

Don't we also need to `#include kernel/mempool_removal_reasons.h` , for Include What You Use (IWYU)? I'll admit, I'm totally sure what our conventions are on this. cc @TheCharlatan or @maflcko

💬 josibake 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_r1609571510)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

AFAICT, these are unused (I was able to compile this commit fine without them). Maybe leftover from a different approach?

💬 josibake 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_r1609644369)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

Any reason why `conflicting_block_hash` is passed by reference but `replacement_tx` isn't?

Would also be nice if we prevented these objects from be constructed with `nullptrs`. I'm not sure if we have a convention in our codebase around this or other examples to point to, but would be worth looking into.

💬 josibake 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_r1609666831)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

I haven't finished reviewing the later commits yet, but seems odd to pass both `conflicting_block_hash` and `conflicting_block_height`. Seems like we should be able to only use `conflicting_block_hash`?

👍 theStack approved a pull request: "wallet, tests: Avoid stringop-overflow warning in PollutePubKey"
(https://github.com/bitcoin/bitcoin/pull/30131#pullrequestreview-2070750537)
utACK 2289d4524053ab71c0d9133987cb36412797c1a2

💬 maflcko commented on issue "test: rpc_bind.py --ipv4 fails when run on s390x or arm in debian":
(https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-2124403378)
Possibly unrelated, but `rpc_bind.py --ipv6 | ✖ Failed | 2 s`

With ASan, on GHA: https://github.com/m3dwards/bitcoin/actions/runs/9178831587/job/25239590853#step:5:5070

```
test 2024-05-21T18:27:24.024000Z TestFramework.node0 (DEBUG): RPC successfully started
test 2024-05-21T18:27:24.078000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "
...

💬 maflcko commented on issue "ci: Enable bpfcc-tools":
(https://github.com/bitcoin/bitcoin/issues/29804#issuecomment-2124403920)
Nice. I think you can just temporarily disable rpc_bind, due to https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-2124403378 I presume?

👍 laanwj approved a pull request: "wallet, tests: Avoid stringop-overflow warning in PollutePubKey"
(https://github.com/bitcoin/bitcoin/pull/30131#pullrequestreview-2070783321)
ACK 2289d4524053ab71c0d9133987cb36412797c1a2

💬 maflcko commented on pull request "wallet, tests: Avoid stringop-overflow warning in PollutePubKey":
(https://github.com/bitcoin/bitcoin/pull/30131#issuecomment-2124451770)
ACK 2289d4524053ab71c0d9133987cb36412797c1a2 with g++ 14.1.1 🦄

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 2289d45240
...

💬 maflcko commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1609716833)
Yeah, it could make sense to switch this from brittle raw pointers literals to type-safe `string_view` or `string` in a follow-up.

💬 maflcko commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1609718736)
Ah, IIRC `string_view` was tried in this pull and needed more patches, so it was left for a follow-up what to do here.

💬 pablomartin4btc commented on pull request "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection":
(https://github.com/bitcoin-core/gui/pull/815#issuecomment-2124481593)
Updates:
- Addressed @luke-jr's [feedback](https://github.com/bitcoin-core/gui/pull/815#pullrequestreview-2013545536).

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2124488325)
> Avoiding this complexity is another reason to add a NAT-PMP fallback, then we'd be right to keep the option named the same and only change the descriptions.

If you think such a fallback is easier than dealing with QT settings migration :-)

We can just sort them PCP, NAT-PMP, UPNP in the GUI. If we also add the word "recommended" to PCP, it should mitigate choice-paralysis. Then in the future we can add a little green dot to indicate that it actually works.

I can test on Windows, (Inte
...

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1609746833)
```
curl -s localhost:8000/tweak-index/709632?dustLimit=992 | jq
{
"error": "Server does not allow dustLimits"
}
```

Do I need to add a setting or rebuild the index?

💬 brunoerg commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1609748019)
done

💬 brunoerg commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1609748162)
done