💬 maflcko commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2210242534)
re-ACK de71d4dece604907afc4fc26b7788e9c1a4cbecb 🎆

<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 de71d4dece604907afc4
...

💬 paplorinc commented on pull request "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin":
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2210308399)
Thanks for the review @andrewtoth.
I'm not sure I see the advantage of inverting the `if (inserted) {` call, just to have two `return it;` calls after.
But I did rename `it` to `ret`, since we're getting the most likely return value immediately now.

💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2210322982)
Updated c1d6e525131cb9e54bbedb79ea64e2469a77aed8 -> 606a7ab862470413ced400aa68a94fd37c8ad3d3 ([noGlobalScriptCache_13](https://github.com/TheCharlatan/bitcoin/tree/noGlobalScriptCache_13) -> [noGlobalScriptCache_14](https://github.com/TheCharlatan/bitcoin/tree/noGlobalScriptCache_14), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalScriptCache_13..noGlobalScriptCache_14))

* Addressed @glozow's [comment](https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1664039214), a
...

👍 TheCharlatan approved a pull request: "fuzz: improve utxo_snapshot target"
(https://github.com/bitcoin/bitcoin/pull/30329#pullrequestreview-2160094330)
ACK de71d4dece604907afc4fc26b7788e9c1a4cbecb

💬 ajtowns commented on pull request "wallet: use LogTrace for walletdb log messages at trace level":
(https://github.com/bitcoin/bitcoin/pull/30355#issuecomment-2210376239)
> > Could also change it to `[warn]` when the user request could not be fulfilled?
>
> @ajtowns Did you see this? Seems rfm either way, but maybe address this or reply, so that maintainers don't hold it back on this nit?

Think it would make more sense to change that along with bumping other `LogPrint{f,}()` calls (eg, "WARNING SQLite is configured to not wait for data to be flushed...", ). Also whether that should be `[warn]` seems up in the air at the moment: with #30347 or #30361 it migh
...

📝 maflcko opened a pull request: " rpc: Use untranslated error strings in loadtxoutset "
(https://github.com/bitcoin/bitcoin/pull/30395)
Motivation:
* Some are not translated at all, anyway. See https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663631973
* For others translation is not yet needed, because they are not called by the GUI (yet)
* For others translations will never be needed, because they are RPC code. See https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663611194

Also, while touching this:
* Remove the trailing `\n`. See https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663647981

...

💬 TheCharlatan commented on pull request "rpc: Use untranslated error strings in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/30395#issuecomment-2210422683)
Concept ACK

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2210448369)
A different approach that stays closer to the original design but does not add noise encryption and a dedicated network stack:

- (Maybe) introduce `SocketTransport` to enable p2p messages over unix sockets with less overhead (but how to handle multiple connections?)
- Add a whitelist permission `mining`
- Peers with that permission have an extra `CNode` field (akin to `Sv2Client`)
- `m_coinbase_output_data_size`
- We send these peers a `HEADERS` message for every tip update (equivalent
...

💬 HerWayBit commented on pull request "rpc: Use untranslated error strings in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/30395#issuecomment-2210450157)
> Motivation:
>
> * Some are not translated at all, anyway. See https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663631973
>
> * For others translation is not yet needed, because they are not called by the GUI (yet)
>
> * For others translations will never be needed, because they are RPC code. See https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663611194
>
>
>
> Also, while touching this:
>
> * Remove the trailing `\n`. See https://github.com/bitcoin/bitcoin/pull/3026
...

💬 maflcko commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2210469835)
Maybe something like https://github.com/OSGeo/PROJ/commit/8f29b8f48b1b78aed4cb54f19ce7ea5c6c6fb469 is needed in oss-fuzz?

💬 maflcko commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#issuecomment-2210544601)
ACK 7c9684a5a357e300f4901167a4bb0e1bdafd075d

💬 fjahr commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2210547243)
utACK de71d4dece604907afc4fc26b7788e9c1a4cbecb

💬 m3dwards commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2210562334)
Post merge Guix build matched:

```shell
d702d02df48bc540da55c47ca7110d122a27ba179ab728fb8bdb6e27589f754c guix-build-f59e9057e2aa/output/aarch64-linux-gnu/SHA256SUMS.part
d806e1994bc873a975714ce75bca87fd3fcec4055e24d1ed0afe99ebc3503288 guix-build-f59e9057e2aa/output/aarch64-linux-gnu/bitcoin-f59e9057e2aa-aarch64-linux-gnu-debug.tar.gz
a9844e6a4d3e86df69b98281c942ac01136b4b115fc6a2504740368c5d441fbd guix-build-f59e9057e2aa/output/aarch64-linux-gnu/bitcoin-f59e9057e2aa-aarch64-linux-gnu.ta
...

💬 TheCharlatan commented on pull request "contrib: use c++ compiler rather than c compiler for binary checks":
(https://github.com/bitcoin/bitcoin/pull/30387#issuecomment-2210565170)
Getting a check failure during a guix build:
```
.
----------------------------------------------------------------------
Ran 1 test in 5.525s

OK
F
======================================================================
FAIL: test_ELF (__main__.TestSymbolChecks)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/distsrc-base/distsrc-dae34eb480c8-arm-linux-gnueabihf/./contrib/devtools/test-symbol-check.py", line 69, in te
...

💬 naiyoma commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#discussion_r1666612849)
Would it make sense to also change this string `nchaintx` to `m_chain_tx_count` for consistency?

💬 alfonsoromanz commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1666620945)
I deleted the logs mentioned.

💬 TheCharlatan commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2210605752)
I'm not sure what a good approach for reviewing this might be. How can I validate the changes without tests? Should I be running this as part of a parent pull request? The first two commits seem fairly mechanical, but the last one less so. The comments in the code are helpful, but some pointers in the PR description for what to look out for, or a broad description of the approaches taken would be helpful too. E.g. why is so much custom functionality introduced for `findBlock`?

💬 alfonsoromanz commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1666625190)
Thanks @mzumsande. Makes sense.

I removed the third commit since there is agreement that it is redundant with `test_snapshot_with_less_work()`.

> Unrelated: @alfonsoromanz Thanks for the mention, but could you remove the "@" from my name in the PR description - if it'd get merged with it, I'll get notifications whenever some altcoin fork cherrypicks that commit later.

Sorry about that. The "@" was already removed by fanquake.

💬 alfonsoromanz commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2210613111)
I removed the third commit (https://github.com/bitcoin/bitcoin/commit/af0f401258e0c189799a36f4487eaa751d779e7b) since there is agreement that it is redundant with `test_snapshot_with_less_work()` (https://github.com/bitcoin/bitcoin/pull/29428). See discussion: https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1665838469

💬 naiyoma commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#issuecomment-2210623450)
Concept ACK.
Nit: IMO, [https://github.com/bitcoin/bitcoin/commit/64d35be20c43f764e5c22657085a790d0adcfe22](https://github.com/bitcoin/bitcoin/commit/64d35be20c43f764e5c22657085a790d0adcfe22) message should be amended to also include `nTxCount` and `nTxDiff`, since it's not just `nChainTx` that's changing to uint64_t.