💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2388899694)
> This is a good argument except we do have some strings only known at runtime, notably translations

I chose my wording "known at compile-time" carefully for exactly this reason: they're _known_ at compile-time, we just choose not to implement the (complete) validation logic because it much more complex than just returning an error string. It's not worth arguing semantics too much, but in my view that's very much still a programming error and not something that callsites should be aware of, l
...

💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1784734716)
> I would at least want to make it clearer that format strings are separate from other parameters

I agree with that, and I'll incorporate your first suggestion (I [don't agree](https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2388899694) with the translations bit) if I have to force-push, thanks!

✅ mzumsande closed a pull request: "net: Make AddrFetch connections to fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/26114)

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-2388967707)
The refactoring commits are pretty annoying to rebase (and probably also to review) - maybe I'll open another version without the refactoring (although I think it makes sense to get the fixed seeds logic out of ThreadOpenConnections).

💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388990278)
> Could you please confirm whether reinstalling Xcode resolves the issue for you?

I tried to reinstall Xcode 16.0 from the App Store, but it keeps failing to install. So I downloaded it instead, and started it once. Just mentioning this because it could indicate some other issue with this particular machine.

And ran `make` again on the same commit as above. It built qt6.

Building Bitcoin Core fails, but I'll try that again after your next push.

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2388991602)
I've added a commit introducing a delayed set to store transactions that are being reconciled, but won't be added to the sketch should it be constructed at this time. This is to mimic the fanout trickling logic where transactions are only made available to peers if they would have been INVed to them. This was already part of @naumenkogs approach, but was scrapped out when reworking when data is added to the reconciliation set in a22b2364f5942266596c78ad306b1e32aa89aa0f

✅ Mackain closed a pull request: "doc: update IBD requirements in doc/README.md"
(https://github.com/bitcoin/bitcoin/pull/30992)

📝 Mackain reopened a pull request: "doc: update IBD requirements in doc/README.md"
(https://github.com/bitcoin/bitcoin/pull/30992)
A small change to the first paragraph of the Setup part of the README that has been bugging me for a while.
The disk space required for the Bitcoin transactions can no longer be described as "a few" hundred gigabytes.
So I thought it was time it was changed to "several" instead.

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
h
...

💬 Mackain commented on pull request "doc: update IBD requirements in doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/30992#issuecomment-2389012540)
> > The commit message was created automatically when I clicked the "Commit Suggestion" button on your previous feedback. Fixing it now.
>
> @Mackain The commit message wasn't updated yet, could you update it and re-push the same commit (thanks!)
>
> Edit: if helpful, reckon you could use the same commit message as the PR title.

Ah, sorry. I now understand what you mean. I originally thought you wanted me to update the commit message of [3208df2](https://github.com/bitcoin/bitcoin/commi
...

⚠️ tdb3 opened an issue: "ci: macOS 14 CI failure `Invalid value detected for '-wallet' or '-nowallet'`"
(https://github.com/bitcoin/bitcoin/issues/31019)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Encountered the following failure in PR #30793 (https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2387502458) for `CI / macOS 14 native, arm64, no depends, sqlite only, gui (pull_request)`. The failure wasn't seen on the other CI jobs.

### Expected behaviour

No failure

### Steps to reproduce

CI run with #39793 commit e6853592361341c27103ed74b25470ac1e098d6d

### Relevant log
...

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2389078228)
> Please report the CI failure.
Issue #31019 opened

💬 tdb3 commented on issue "ci: macOS 14 CI failure `Invalid value detected for '-wallet' or '-nowallet'`":
(https://github.com/bitcoin/bitcoin/issues/31019#issuecomment-2389081000)
At the time of the failure, the PR commits were on top of commit a74bdeea1b8e27b2335f0f7da78006e87ecfb235 (Sept 2nd).
I'm probably missing something, but it almost seemed like the CI job was executing code that was introduced in commit ee47ca29d6ef55650a0af63bca817c5d494f31ef (dated Aug 22nd, from PR #30684, but merged with commit fb52023ee69c346dd101770716b6d8c0525c38aa on Sept 9th).

💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2389082792)
I also get the segfault when rebuilding (depends and bitcoin-qt) with 354f5d79270c0784a6ba59d61191ee7eaaa2a53d.

```
% lldb build-depends/src/qt/bitcoin-qt
(lldb) target create "build-depends/src/qt/bitcoin-qt"
Current executable set to '/Users/sjors/dev/bitcoin/build-depends/src/qt/bitcoin-qt' (x86_64).
(lldb) run
Process 7195 launched: '/Users/sjors/dev/bitcoin/build-depends/src/qt/bitcoin-qt' (x86_64)
2024-10-02 18:18:32.237316+0200 bitcoin-qt[7195:247862] SecTaskLoadEntitlements fai
...

💬 jonatack commented on issue "rfc: DATUM mining interface requirements":
(https://github.com/bitcoin/bitcoin/issues/31002#issuecomment-2389106217)
> So far it's proprietary and undocumented, but hopefully that changes.

Agree. I believe it may be MIT-licensed and public soon.

🤔 glozow reviewed a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2343446923)
ACK b6368fc285b. I have some picky review comments if you're interested, but I don't feel that strongly on anything except the default argument one.

I tested with @0xB10C's visualizer as well, very cool!
<img width="833" alt="image" src="https://github.com/user-attachments/assets/c1e81c89-3d4d-4a5a-a092-51d633ea10db">

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1784873140)
It is a bit funky that a negative verbosity turns into 0! I was kind of expecting a `JSONRPCError`. I don't mind and it looks like it's the same for `getblock`, so not asking for a change, but could be worth a test case?

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1784854966)
nit: comment is unnecessary. Also my personal preference is to not put them on the same line

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1784861016)
an annotation can help here
```suggestion
int verbosity{ParseVerbosity(request.params[1], /*default_verbosity=*/1)};
```

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1784859575)
I find default arguments to be pretty footgunny and prefer to require explicit. Since you're introducing this function for the first time, I suggest no default arg.

```suggestion
int ParseVerbosity(const UniValue& arg, int default_verbosity);
```

🤔 instagibbs reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2343039535)
some last nits/suggestions