💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1795066784)
It should match the spec. I'm guessing the comment is wrong, because otherwise it wouldn't work against SRI, but will check.

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1795071626)
It's only used in tests, but I think we should review the code in both directions under the assumption that they'll be used in production.

💬 l0rinc commented on pull request "init: Correct coins db cache size setting":
(https://github.com/bitcoin/bitcoin/pull/31064#discussion_r1795072559)
Is this the same as
```suggestion
auto init_cache_fraction = chainman.IsSnapshotActive() ? 0.2 : 1.0;
```
?

💬 TheCharlatan commented on pull request "init: Correct coins db cache size setting":
(https://github.com/bitcoin/bitcoin/pull/31064#discussion_r1795086831)
I think in practice it is, since we call this after `DetectSnapshotChainstate()`, which in turn calls `ActivateExistingSnapshot` if there is a snapshot chainstate, but checking if there is more than one chainstate seems more defensive to me, since it relies on fewer assumptions.

💬 fanquake commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2404642324)
> It's ready for review, but not for merge:

> > The parent PR may need more conceptual review

This should probably be a draft then. Re more conceptual review, I think it's been made (fairly?) clear by multiple contributors (and from the progress of the sidecar PRs from #29432) which approach is favoured, and it doesn't seem to be the one involving this PR, and implementing all the SV2 logic inside Core.

✅ fanquake closed a pull request: "Add -pausebackgroundsync startup option"
(https://github.com/bitcoin/bitcoin/pull/31023)

💬 fanquake commented on pull request "Add -pausebackgroundsync startup option":
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2404649271)
Going to close this for now, given the feedback.

💬 hodlinator commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2404665924)
One of the [OpenRPC examples](https://github.com/open-rpc/examples/blob/dce69463ba9a3ca2232506b734606fa97f25dd45/service-descriptions/petstore-openrpc.json) has:
```json5
...
"components": {
"contentDescriptors": {
"PetId": {
"name": "petId",
"required": true,
"description": "The id of the pet to retrieve",
"schema": {
"$ref": "#/components/schemas/PetId"
}
}
},
"schemas": {
"PetId": {
"ty
...

💬 fanquake commented on pull request "build: Bump minimum supported macOS to 12.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2404671671)
> Running Bitcoin Core on unsupported OSes may expose users to security issues.
> macOS Big Sur 11 received its last security update ([11.7.10](https://support.apple.com/en-us/106338)) over a year ago.

If this is the reasoning we are going to use, shouldn't this be bumping to `10.13`, given `10.12` is also "unsupported" in terms of security updates?

💬 theStack commented on pull request "build: scripted-diff: drop config/ subdir for bitcoin-config.h":
(https://github.com/bitcoin/bitcoin/pull/30937#issuecomment-2404737373)
Rebased on master and added another commit fixing the outdated comment in the linter (https://github.com/bitcoin/bitcoin/pull/30937#discussion_r1773075606).

💬 theStack commented on pull request "build: scripted-diff: drop config/ subdir for bitcoin-config.h":
(https://github.com/bitcoin/bitcoin/pull/30937#discussion_r1795181976)
Fixed now.

💬 naumenkogs commented on pull request "p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/29664#issuecomment-2404771253)
ACK ab45a0654c189da3013ef0c8d30478601052d922

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1795202925)
> Addressed in https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402990346.

How was it addressed? I still see the same output using 2d009aa8e861c161820a4c0619ecd4b2329267af:
```bash
make -C depends -j12 HOST=x86_64-apple-darwin
gmake: Entering directory '/bitcoin/depends'
Fetching qtbase-everywhere-src-6.7.3.tar.xz from https://download.qt.io/official_releases/qt/6.7/6.7.3/submodules
% Total % Received % Xferd Average Speed Time Time Time Current

...

💬 maflcko commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2404783263)
> I think we could consider removing the honggfuzz netdriver instructions in a follow

Not sure about touching real code with the motivation to make instructions happy that may be removed anyway.

If the changes are meaningful for the fuzz target, then it may be fine.

> It also makes the `p2p_transport_serialization` fuzz target simpler since we do not need to assist the network magic and checksum creation anymore.

Can you provide more details here? IIUC a hash will still be calculated
...

👍 itornaza approved a pull request: "rpc: getorphantxs follow-up"
(https://github.com/bitcoin/bitcoin/pull/31043#pullrequestreview-2360026747)
Concept ACK ae17f9050e0c2a6fd31cd9f5d2f3ad8066f02cc9

Enriches the functional tests for the the `getorphantxs` rpc and offers added functionality to conveniently access the expiration times of transactions in the orphanage.

Broadening the context with refactoring from `rpc_getorphantxs.py` to `rpc_orphans.py` as a future provision for more orphanage rpcs looks good to me.

💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1795269088)
To extend a bit: Previously (and after my pull right now), it was easy to toggle between translation and non-translation by replacing `_` with `Untranslated` (or vice-versa):

```diff
-strprintf(Untranslated("Cannot create database file %s"), filename);
+strprintf( _("Cannot create database file %s"), filename);
```

With your change, the function call order has to be changed as well.

💬 maflcko commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2404888004)
I liked the summary stats per folder produced by the current approach, but if others like other stuff better, I don't mind.

💬 fanquake commented on pull request "build: scripted-diff: drop config/ subdir for bitcoin-config.h":
(https://github.com/bitcoin/bitcoin/pull/30937#issuecomment-2404945576)
Guix Build:
```bash
9cf65a99fa046a1a3acbee14d6dec658b9fe7f4e55a4b6f50d063ebb4029e3eb guix-build-882f736d0a60/output/aarch64-linux-gnu/SHA256SUMS.part
46780332298b988bc9b4c2903d077f49978dc92250cf5221557ce4459383b18e guix-build-882f736d0a60/output/aarch64-linux-gnu/bitcoin-882f736d0a60-aarch64-linux-gnu-debug.tar.gz
72362b6f0394f4b8b1e69e4520c892d5b3e9f3d86788c8f0ee59f2b0d9e51717 guix-build-882f736d0a60/output/aarch64-linux-gnu/bitcoin-882f736d0a60-aarch64-linux-gnu.tar.gz
c4b5294efed908af
...

👍 fanquake approved a pull request: "build: scripted-diff: drop config/ subdir for bitcoin-config.h"
(https://github.com/bitcoin/bitcoin/pull/30937#pullrequestreview-2360160618)
ACK 882f736d0a607976ee5e3a6cbcb5385524bc72c6

🚀 fanquake merged a pull request: "build: scripted-diff: drop config/ subdir for bitcoin-config.h"
(https://github.com/bitcoin/bitcoin/pull/30937)