💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499430732)
```suggestion
std::ranges::sort(m_txids);
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499446404)
not yet sure I fully understand why this is needed - it's more complex, but does it result in at least a theoretic speedup?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499464047)
should we document here what happens in case of a cache miss or collision?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499449442)
`continue` skipping a single line is a bit confusing an dangerous - it should only skip the next line:
```suggestion
if (!input.coin.IsSpent()) {
temp_cache.EmplaceCoinInternalDANGER(COutPoint{input.outpoint}, std::move(input.coin));
}
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499442182)
based on https://en.cppreference.com/w/cpp/atomic/atomic_flag/wait.html
```suggestion
input.ready.wait(/*old*/false, std::memory_order_acquire);
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499486476)
this will fix the linter failure as well (whitespace after `*`):
```suggestion
*
* @return whether there are more inputs in the queue to fetch
```

💬 jurraca commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3497961898)
ACK 44717da29b626bfdcb20a32318689d74c4113b7b

I think it's a better pattern to force users to specify the filename, but I can't tell whether users currently depend on the default.

💬 sipa commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3498015913)
I'm neutral on this PR.

I do use the `asmap.dat` loading by default, but (a) don't care about changing that to specify it explicitly and (b) I hope #28794 happens soonish so it becomes a non-issue (as I will switch to built-in asmap files rather than explicit ones in the datadir).

💬 maflcko commented on pull request "script: remove dead code in `CountWitnessSigOps`":
(https://github.com/bitcoin/bitcoin/pull/33786#issuecomment-3498072962)
review ACK 24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7 🐏

<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: review ACK 24bcad3d4df5
...

💬 maflcko commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3498077725)
Since when is this "unused"?

💬 l0rinc commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3498102849)
> Since when is this "unused"?

Can you be more specific please?
These methods were always called with the same values (`nullptr`), always disabling that functionality.

💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#issuecomment-3498178885)
> CBlockIndexWorkComparator seems unrelated to the issue

The original issue won't be fixed since it's not exactly a bug, but we could still speed up the regression by optimizing another bottleneck.
Since you found the etymology confusing, I have removed the details from the description, only mentioned https://github.com/bitcoin/bitcoin/blob/master/src/validation.h#L637 for the flame graph that show this being one of the bottlenecks. Not the biggest one, but the one we can easily optimize and
...

💬 Pttn commented on issue "qt: Amount field too narrow on Windows in Send Coins dialog":
(https://github.com/bitcoin-core/gui/issues/906#issuecomment-3498286137)
Can confirm on several Windows 11 24H2/25H2 machines with Bitcoin Core Master 5c5704e730796c6f31e2d7891bf6334674a04219.
Built with Guix on Debian 12. For some reason, the field is fine if running the .Exe via Wine...

💬 vasild commented on pull request "Run feature_bind_port_(discover|externalip).py in CI":
(https://github.com/bitcoin/bitcoin/pull/33362#issuecomment-3498341743)
`9b30b0bea8...e245da32dc`: rebase due to conflicts

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499916845)
can we call it something else to not coincide with `CCoinsViewCache::FetchCoin`

💬 vasild commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2499928634)
@mzumsande ?

💬 dergoegge commented on pull request "fuzz: avoid returning non-conforming results from FuzzedSock::GetSockName()":
(https://github.com/bitcoin/bitcoin/pull/32109#issuecomment-3498443216)
Also leaning towards concept NACK on this

1. It does not seem like fuzzers struggle to produce canonical `getsockname` results using our current approach
2. Even if super unlikely, this would catch incorrect handling of `getsockname` results (in case of OS bug) on our side, which seems like a nice thing if it's basically free (given 1.)

👍 stickies-v approved a pull request: "util: Allow `Assert` (et al.) in contexts without __func__"
(https://github.com/bitcoin/bitcoin/pull/33785#pullrequestreview-3429500574)
ACK fad6efd3bef1d123f806d492f019e29530b03a5e

💬 stickies-v commented on pull request "util: Allow `Assert` (et al.) in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2500004322)
There are 2 places in `validation.cpp` where `STR_INTERNAL_BUG` could be used too, but I suppose you didn't because it (afaik) requires us to abandon the compile time format string checking?

📝 waketraindev converted_to_draft a pull request: "Comment out sensitive console commands in history to prevent re-execution"
(https://github.com/bitcoin-core/gui/pull/909)
Prefix filtered commands with "#" before adding them to the RPC console history.

Console lines starting with "#" are ignored by the executor, preventing accidental re-execution of filtered sensitive commands when recalled from history.

Also adds a noop signal to handle ignored commands cleanly and documents comment behavior in the console help text (`help-console`).