💬 willcl-ark commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1378673670)
I was wondering if if could be a good idea to log the removed keys at this point as a backup (safe-ish seeing as we are deleting), but I'm struggling to think of a scenario where a user with this type of wallet might somehow need these keys again...

💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378706743)
I suspect it'd be nicer to have `PushMessage(pfrom, msgMaker.Make(BLOCKTXN, BIP_XXX_TX_COMPRESSION(resp)))` so that net_processing can turn compression on based on whether it's been negotiated or not, and drop `TransactionCompression` entirely? Seems out of scope for this PR either way.

💬 fanquake commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1788834198)
> failure seems unrelated.

Are you sure?
```bash
Traceback (most recent call last):
File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/rpc_createmultisig.py", line 65, in run_test
self.do_multisig()
File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/b
...

💬 vasild commented on pull request "Do the SOCKS5 handshake reliably":
(https://github.com/bitcoin/bitcoin/pull/28649#discussion_r1378708089)
I prefer to keep all implementation in one place and to keep the interface separate from the implementation.

💬 dergoegge commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1788837597)
> Are you sure?

Yes, it passes locally.

💬 fanquake commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1788841475)
Want to document it as an intermittent failure?

🤔 stickies-v reviewed a pull request: "Improve peformance of CTransaction::HasWitness (28107 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/28766#pullrequestreview-1708054652)
Approach ACK

I had similar thoughts while reviewing 28107 but figured the computation was still fast enough. Given that `ComputeWitnessHash` is only called once during initialization (and is private) and `HasWitness` is called frequently, this approach is strictly better.

💬 stickies-v commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#discussion_r1378700622)
nit: I'd at least use a range based loop
```suggestion
bool has_witness{false};
for (const auto input : vin) {
if (!input.scriptWitness.IsNull()) {
has_witness = true;
break;
}
}
```

but probably okay to refactor to use `algorithm`?

```suggestion
bool has_witness = std::any_of(vin.begin(), vin.end(), [](const auto& input) {
return !input.scriptWitness.IsNull();
});
```

💬 stickies-v commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#discussion_r1378705398)
Could simplify return statement too so the whole fn just becomes:


```cpp
Wtxid CTransaction::ComputeWitnessHash() const
{
bool has_witness = std::any_of(vin.begin(), vin.end(), [](const auto& input) {
return !input.scriptWitness.IsNull();
});

return Wtxid::FromUint256(has_witness ? (CHashWriter{0} << *this).GetHash() : hash.ToUint256());
}
```

💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378718755)
Maybe? Being explicit seems not that bad though?

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378716304)
nit: template implies `inline`

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378717260)
nit: (same above), given that this function is only for internal use for this module, and only used twice, is it needed?

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378719331)
nit: (same above), given that this function is only for internal use for this module, and only used once, is it needed?

💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378757410)
> Also, `GetSerializeSize` could drop the int param completely?

Looks like; but seems a bit much for this PR. See https://github.com/ajtowns/bitcoin/commits/202310-getserializesize

💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378762942)
That ditto one sure doesn't... Removed them both.

💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378768625)
No idea; seems easier to review without additional changes, though?

💬 fanquake commented on pull request "Fix crash on selecting "Mask values" in transaction view":
(https://github.com/bitcoin-core/gui/pull/774#issuecomment-1788929772)
Pulled this into a branch for 25.x backporting.

💬 fanquake commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1788945972)
The `rpc_createmultisig.py` test is now failing in two other CI jobs and MSAN is unhappy:
```bash
==19201==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x563f069e85d0 in bcmp /msan/llvm-project/compiler-rt/lib/msan/../sanitizer_common/sanitizer_common_interceptors.inc:856:10
#1 0x563f0956bff5 in base_blob<256u>::Compare(base_blob<256u> const&) const src/./uint256.h:54:66
#2 0x563f0956bff5 in operator!=(base_blob<256u> const&, base_blob<256u> const&) src/./uint256.h:57
...

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1378798478)
That's just me trying to save few characters. Will rename it to `past_net_limited`.

📝 fanquake opened a pull request: "[25.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/28768)
Collecting backports for the 25.x branch. Currently:
* https://github.com/bitcoin-core/gui/pull/774