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

💬 maflcko commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1666644819)
I wrote a simple patch to make the existing `Result` more flexible (allow inner error types other than `bilingual_str`), for places that want to use something like `std::expected<A, B>` in code today by using `Result<A, B>`.

I understand that the `ErrorString` helper won't be working as-is anymore. But that seems fine, because a standalone function can be provided to turn `B` into a string.

I also understand that the approach does not allow for multiple failures, errors, and warnings.

N
...

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1666649324)
Perhaps slightly so, because here at least `setup_bytes()` isn't called again.

My main point was that with `InitScriptExecutionCache` it's clearly an initialization function that's affecting global state. On the other hand, one shouldn't have to expect that simply constructing an object is going to invalidate all other objects of the same type, that's unintuitive and rather opaque I think.

But we can leave this as is, it's resolved in the next commit after all.

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1666663060)
will do if I retouch

💬 dergoegge commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1666684208)
Creating the sig cache every iteration (instead of the global) would be better

💬 willcl-ark commented on pull request "Fix cases of calls to `FillPSBT` errantly returning `complete=true`":
(https://github.com/bitcoin/bitcoin/pull/30357#discussion_r1666695881)
Thanks @furszy, this indeed feels unclean to me.

I made a few changes in another test branch and I wonder if they make sense to you: https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:walletprocesspsbt-no-finalize-fixups

I took inspiration from https://github.com/furszy/bitcoin-core/commit/698e090ead3d0603146c98bd2ec06e44389f34cc (thanks) but it seems that I can probably just clear the entire output map as done in [L94](https://github.com/bitcoin/bitcoin/compare/master.
...

💬 ismaelsadeeq commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1666706287)
`Entry` struct does not have any other distinguishing information, are you suggesting adding txid to `Entry`?

💬 ismaelsadeeq commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1666706550)
which order?