💬 l0rinc commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2252828381)
we don't usually `auto` parameters and not sure why we're copying the result:
```suggestion
inline const std::vector<size_t>& FindInPackageParents(size_t tx_index, const std::vector<std::vector<size_t>>& relations)
{
return relations[tx_index];
}
```

💬 l0rinc commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2252819980)
C++20 allows `::contains` (+ nit: braces):
```suggestion
if (possible_parents.contains(input.prevout.hash)) {
pindexes.insert(possible_parents[input.prevout.hash]);
}
```

Or even better, to avoid double hashing (via existence check first, and getting the value, second), we might as well:
```suggestion
if (const auto it{possible_parents.find(input.prevout.hash)}; it != possible_parents.end()) {
pindexes.insert(it->seco
...

💬 nervana21 commented on issue "Crash on launch in PruneBlockIndexCandidates":
(https://github.com/bitcoin/bitcoin/issues/33129#issuecomment-3152894540)
Ahh, okay, my understanding of the problem that [Ataraxia009](https://github.com/Ataraxia009) is having is a bit better now, but I still don't understand the problem well enough to adequately characterize it better than I've done in this issue. I've updated the title to more accurately describe what's believed to be the source of the error

💬 ajtowns commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2252854451)
Shouldn't this mimic `VerifyScript` more closely? ie:

* check the scriptSig is push-only
* check the redeemScript matches the hash in the p2sh prev_spk
* check the scriptSig doesn't push other things on the stack (ie `stack.size() == 1`)

All those are permanent errors for the txid (fixing them requires changing the scriptSig which changes the txid), so the rejection would be valid to cache.

> Currently we check scripts sequentially from first input to last. If input N is consensus-
...

💬 luke-jr commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3152986009)
Why would this case ever occur? Seems like a lot of code to handle an unexpected path. And would-be DoS attackers can defeat it with a dummy witness anyway, right?

💬 Ataraxia009 commented on pull request "Adding alert for failure to prevent dead-end user crash":
(https://github.com/bitcoin/bitcoin/pull/33127#issuecomment-3152989766)
> Looking at the relevant code, this feels like a symptom of a problem we could/should detect earlier somehow (which may also be early enough to follow the reindex GUI path)

Yeah agreed overall. However i cant repro and don't have the data dir. That being said this deadend the user to a crash with no explanation or logs. And the crash is repeated on every launch. This would just a lot of node runners to give up and move on. Atleast this way they have some way out. It's quite a soft change.

💬 luke-jr commented on pull request "build: note that sysctl was removed from Linux":
(https://github.com/bitcoin/bitcoin/pull/33111#issuecomment-3152998064)
If it's removed, the check fails anyway...? The current message seems better IMO

💬 Sammie05 commented on pull request "refactor, index: Remove member variables in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33134#issuecomment-3153027156)
Concept ACK, I agree with the goal of removing unnecessary class members to simplify the code.

But consider splitting the commit as suggested by @l0rinc to isolate the risky accounting changes.

Reusing DBVal as a container simplifies the logic. Good improvement.

Please update the docstring of the method (still says “initialize internal state”), which might no longer be accurate.

💬 luke-jr commented on pull request "qt: clear command history when clearing the console":
(https://github.com/bitcoin-core/gui/pull/882#issuecomment-3153047499)
Concept NACK, agree we should match expected behaviour here (not clearing command history). Maybe some way can be added, but I'm not sure there's an obvious right way.

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2252952435)
Thanks, love the inlining of `GetSigOpCount` (bothered me, too - it's a lot clearer this way), and the new `CountSigOps` name as well.
Added your comments (+ you as co-author, of course) and adjusted all the tests, now that `CountSigOps` is separated from `CountP2SHSigOps`

💬 luke-jr commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2252956253)
Kinda ugly to build this string when it's likely not going to be used.

💬 luke-jr commented on pull request "rpc: Fix 'getdescriptoractivity' RPCHelpMan, add test to verify fix":
(https://github.com/bitcoin/bitcoin/pull/33119#issuecomment-3153070601)
utACK 3543bfdfec345cf2c952143c31674ef02de2a64b

💬 luke-jr commented on pull request "rpc: Fix 'getdescriptoractivity' RPCHelpMan, add test to verify fix":
(https://github.com/bitcoin/bitcoin/pull/33119#issuecomment-3153072911)
The commit message should probably also be corrected (maybe "Docfix: RPC/blockchain: ..."?)

💬 luke-jr commented on pull request "rpc: fix getpeerinfo ping duration unit docs":
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3153083380)
utACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff

📝 waketraindev converted_to_draft a pull request: "qt: clear command history when clearing the console"
(https://github.com/bitcoin-core/gui/pull/882)
Clears the command history in the qt console when the user clears the console output.

💬 Ataraxia009 commented on issue "Crash on launch in PruneBlockIndexCandidates":
(https://github.com/bitcoin/bitcoin/issues/33129#issuecomment-3153271827)
> Which version of Bitcoin-Core are you running?

28

💬 Ataraxia009 commented on issue "Crash on launch in PruneBlockIndexCandidates":
(https://github.com/bitcoin/bitcoin/issues/33129#issuecomment-3153273769)

> > I cannot figure out how the data got to that state because it seems like this state should never happen.
>
> Bitcoin Core makes heavy use of CPU, RAM and storage IO. Hardware defects might only become visible when running Bitcoin Core. You might want to check your hardware for defects.
>
> * Use software such as memtest86 to check your RAM.
> * Use software such as linpack, or Prime95 to check the CPU behaviour under load.
> * Use software such as smartctl, fsck, badblocks, or CrystalDisk
...

💬 HowHsu commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#issuecomment-3153322424)
> My understanding is that this isn't a bottleneck, optimizing it without a benchmark or an easily reproducible usecase seems like it's a solution begging for a problem - I speak from personal experience, that's how I started as well :)
>
> I left a few comments while reviewing anyway, but without a concept ack on the idea itself from those who are more familiar with it, it likely won't get merged.

Thanks, l0rinc. I'll leave this patch alone for now.

💬 HowHsu commented on pull request "index: remove unnecessary locater cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3153348125)
> ACK [1f678d8](https://github.com/bitcoin/bitcoin/commit/1f678d832b1196ca7d7c2ff7e70d2172ae8c90b6)
>
> Recreated and retested it locally. It's a similar change to #33042, we have a few more places which should be cleanup up like this.
>
> I think this already makes the code slightly simpler, but alternatively, you could return an optional and not rely output params and nullness
>
> Details

Thanks, l0rinc. I've update the code as what you suggested

💬 l0rinc commented on pull request "index: remove unnecessary locater cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3153355495)
The commit message and PR description needs to be updated now.
Also consider adding co-authors when someone else also contributed.