💬 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

💬 maflcko commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1378806543)
Any reason to use adjusted time, when the tip update time is recorded as NodeClock?

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1788967144)
re-ACK 1bbddaa060b1826466740f580a604eb3d87a28c0 🙍

<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: re-ACK 1bbddaa060b182646674
...

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

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378813401)
> 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

In that case, compression would be using ser params, so the witness flag would also have to be passed down via net_processing? So it seems better to set the witness flag in net processing for compact blocks and blocktxn?

💬 maflcko commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#discussion_r1378819836)
what about leaving this function as-is and making the two members `std::optional` instead, then add an early-return one-line happy-path for the case when both are not nullopt?