💬 yuvicc commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2488700233)
typo
```suggestion
throw std::runtime_error("Failed to write serialization data");
```

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3484111220)
Tested mainnet query performance using https://mempool.space/address/1BitcoinEaterAddressDontSendf59kuE
Assuming warm OS block cache, https://github.com/romanz/bindex-rs/pull/63#issuecomment-3484098889 takes <1s to fetch 5190 txs.

💬 frankomosh commented on pull request "test: Add bitcoin-chainstate test for assumeutxo functionality":
(https://github.com/bitcoin/bitcoin/pull/33728#issuecomment-3484126740)
ACK 3e7d272

💬 GBKS commented on issue "Async Payjoin":
(https://github.com/bitcoin/bitcoin/issues/33684#issuecomment-3484165074)
> > Given that this is adding new features to the GUI. Is the plan to add them to the "legacy" GUI, the new QML GUI, or implement Payjoin into both? cc [@hebasto](https://github.com/hebasto)
>
> cc [@GBKS](https://github.com/GBKS) [@johnny9](https://github.com/johnny9)

I am not aware of any plans, so this is probably something to be decided. Considering the limited resources that go into the QT and QML GUIs and how hard it is to just make one piece of software extremely good, I don't see it as
...

💬 GBKS commented on pull request "Increase tooltip wrap threshold from 80 to 100 characters":
(https://github.com/bitcoin-core/gui/pull/905#issuecomment-3484175160)
I think some examples would be helpful to evaluate this change. Could you possibly share before and after screenshots?

💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2488985360)
We could, but I think I prefer not to mark function arguments as const if their state is mutated internally.

💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3484270389)
Thank you for the review @yuvicc,

e95efc00842d5d0df96ee9294cdf818741be539e -> 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969 ([kernelApi_80](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_80) -> [kernelApi_81](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_81), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_80..kernelApi_81))

* Addressed a bunch of typo and documentation nits.
* Addressed @yuvicc's [comment](https://github.com/bitcoin/bitcoin/pull/30595#disc
...

✅ maflcko closed a pull request: "ci: Run unit tests parallel with functional tests"
(https://github.com/bitcoin/bitcoin/pull/33000)

💬 maflcko commented on pull request "ci: Run unit tests parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33000#issuecomment-3484400040)
Closing for now, but I may implement this in another way later.

💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#issuecomment-3484508481)
review ACK 5d784bebaff5e3acc0b5180ee51d9a16aec0e356 🎒

<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 5d784bebaff5
...

💬 maflcko commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2489242068)
nit: now that the clang minimum is clang-17, you could revert the workaround again, if you want

💬 Sjors commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3484608786)
> maybe it would be about time to expand the scope since 28.x is no longer maintained.

Yes. Staying one major version behind seems acceptable to me.

@gmaxwell wrote:

> I do not believe luke-jr's comments on twitter explaining the current behavior as a product of ordinarily excluding 'new' versions is particularly credible.

Do you still believe that after his comment above?

💬 Sjors commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2489308021)
Done!

💬 Sjors commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#issuecomment-3484657001)
Rebased past #33555 to get rid of the ancient clang workaround, see https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2489242068.

👍 willcl-ark approved a pull request: "doc: update Guix INSTALL.md"
(https://github.com/bitcoin/bitcoin/pull/33574#pullrequestreview-3415100841)
ACK b4d0288c467f82a94041b51d10d38e66bb5c33ae

Changes here are fine. "various versions of Debian" is arguably a bit generous if it's planned to be dropped from (all?) upcoming versions, but it's not incorrect and the repology link provides up-to-date info in any case.

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3484908644)
Changes applies:
- Fix lint issue
- Added check on the median as well
- some minor formatting

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2489540239)
Indeed. Inline is only a hint, not crucial for modern compliers.

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2489550775)
I did that for the longer values, for shorter values with only a few ones is not really need.

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2489552705)
Added median check as well. Much lower than the avg!

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2489553665)
Added explanatory comments